Skip to content

Android & Java template modifications to check collectionFormat when creating query parameters#893

Merged
fehguy merged 9 commits into
swagger-api:develop_2.0from
sudohippie:develop_2.0-fork
Jul 9, 2015
Merged

Android & Java template modifications to check collectionFormat when creating query parameters#893
fehguy merged 9 commits into
swagger-api:develop_2.0from
sudohippie:develop_2.0-fork

Conversation

@sudohippie

Copy link
Copy Markdown
Contributor

This is to solve the issue #484. Changes are for Android and Java clients. They will create query parameters in the appropriate format based on the collectionFormat defined in the spec.

Made the following changes:

  • Added a new template called QueryParam.mustache
  • Changed queryParams data structure from Map to Set
  • Made the templates aware of collectionFormat, and generate query params based on defined format.
  • Method signature changes to address new data structure.
  • Changes to sample client and tests to reflect template changes.

@sudohippie

Copy link
Copy Markdown
Contributor Author

@xhh, @webron, @who - Created this PR based on comments in #880 .

@who

who commented Jun 19, 2015

Copy link
Copy Markdown
Contributor

:shipit: 👍 Thanks, @sudohippie

@xhh

xhh commented Jun 20, 2015

Copy link
Copy Markdown
Contributor

@sudohippie Thanks for this! Here are some suggestions:

Regarding the QueryParam class

  • I guess a simple Pair class would be better (to store key-value pair) since collectionFormat could also be used in form parameters, we may reuse the class and the logic of processing collectionFormat for both query and form params.
  • I saw that "invalid string" parameter value is ignored, i.e. null or blank string. I think there's possibly a case of needing to send empty string (not null) for a parameter (e.g. param1 in "param1=&param2=value"). Anyway, I suggest we do not restrict it, and when applying the parameters into request, ignore the parameter only when it's null (for optional parameters support, which could be checked in the api class rather than here).
  • The type/class of the parameter value could be anything, e.g. Date, Integer, String. Using generic type would be better, i.e. new Pair<String, Object>. And when concatenating the parameter values, we should use parameterToString to support various types.

Regarding the Set class for queryParams, may I ask why using Set for it? Would List be more straightforward?

@sudohippie

Copy link
Copy Markdown
Contributor Author

@xhh - Thanks for the comments.

I have made several changes including re-factoring the formerly parameterToQueryParams(...) to parameterToPairs(...) for brevity. However, I left the Pair<String, Object> as is and let parameterToString(...) take care of the conversion.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

the previous logic was to ignore the parameter if its value is null, which means the parameter is optional (as required parameter's value should have been checked and ensured to not be null)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@xhh - So, what are you trying to say? That if ``value == null` just return? Which is what it was, in the first place.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

when value is null, it should not be included in the params (with a value of empty string), it should be ignored completely (the same with when name is null)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@xhh - Okay, I'll add that logic back in soon.

@sudohippie

Copy link
Copy Markdown
Contributor Author

@xhh - Done.

@xhh

xhh commented Jun 23, 2015

Copy link
Copy Markdown
Contributor

@sudohippie thanks! it looks good to me

@sudohippie

Copy link
Copy Markdown
Contributor Author

@webron, @xhh - Can we merge these changes?

@xhh

xhh commented Jun 24, 2015

Copy link
Copy Markdown
Contributor

@sudohippie it's OK to me, but I'd like to ask could you please add some test cases for all possible values of collectionFormat? to make sure it works now and in the future

@wing328

wing328 commented Jun 24, 2015

Copy link
Copy Markdown
Contributor

@sudohippie we've to wait for Tony (owner) to review and merge. Test result would definitely help speed up the process.

@sudohippie

Copy link
Copy Markdown
Contributor Author

@xhh @webron @wing328 - I'v added unit tests.

Up on running the Java integration tests PetApiTest.testFindPetsByTags() failed. I noticed that http://petstore.swagger.io/v2/pet/findByTags?tags=friendly is returning 500, With the body:

<apiResponse>
  <message>something bad happened</message>
  <type>unknown</type>
</apiResponse>

Not sure what is going on. Android seems fine.

@sudohippie

Copy link
Copy Markdown
Contributor Author

The server was restarted. The integration tests are passing for Java and Android now.

@who

who commented Jul 9, 2015

Copy link
Copy Markdown
Contributor

@fehguy @webron

Could this please be merged?

fehguy added a commit that referenced this pull request Jul 9, 2015
Android & Java template modifications to check collectionFormat when creating query parameters
@fehguy fehguy merged commit 461f556 into swagger-api:develop_2.0 Jul 9, 2015
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.

5 participants