Skip to content

Conversation

ottobackwards
Copy link
Contributor

@ottobackwards ottobackwards commented Mar 29, 2018

Ryan, first thank you for your great library.
I have created a derivative work to add support for aws gateway api to the apache nifi project,
NIFI PR#2588
here are a couple of changes that I've made.

  • add support for request parameters
  • add to git ignore for intellj files and target directory

This change is Reviewable

- add support for request parameters
- add to git ignore for intellj files and target directory
Copy link
Owner

@rpgreen rpgreen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Few minor comments

request.setResourcePath(resourcePath);
request.setHeaders(buildRequestHeaders(headers, apiKey));

if (parameters != null) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this null check necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your unit tests fail without it.

private final String resourcePath;
private final InputStream body;
private final Map<String, String> headers;
private final Map<String, List<String>> parameters;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be called queryStringParameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

parameters seemed to be in the same flavor of 'headers', 'body' etc. Also it is setParameters() in the DefaultRequest

return this;
}

public boolean hasBody() {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this if it is not used

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do use it ;), but I can remove

return this;
}

public GenericApiGatewayRequestBuilder withParameters(Map<String,List<String>> parameters) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be withQueryStringParameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see above

if (httpResponse.getContent()!= null) {
this.body = IOUtils.toString(httpResponse.getContent());
} else {
this.body = null;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this else block required? Isn't body null by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because it is final, there are "Error:(17, 5) java: variable body might not have been initialized" without it

@rpgreen rpgreen merged commit 32eea44 into rpgreen:master Mar 29, 2018
@rpgreen
Copy link
Owner

rpgreen commented Mar 29, 2018

Merged, thanks again. Can you elaborate on how you plan to use this in Apache Nifi?

@ottobackwards
Copy link
Contributor Author

I'm using the classes ( with reference to you in the NOTICE as the apache lic. requires ) to add a gateway api processor that lets you invoke http methods on it from nifi.
Are you familiar with nifi?

@ottobackwards ottobackwards deleted the params_null_body branch April 3, 2018 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants