Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Java][RestTemplate] Fix query parameter URL encoding #260

Merged
merged 3 commits into from
Jun 10, 2018

Conversation

simingweng
Copy link
Contributor

@simingweng simingweng commented Jun 8, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Fix for #249.

add URL encoding to every query parameter string in case it has unsafe character before building it into the request entity.

CI test on my branch has passed.

@wing328
Copy link
Member

wing328 commented Jun 9, 2018

@simingweng Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@wing328 wing328 added this to the 3.0.1 milestone Jun 9, 2018
@jmini jmini changed the title Bug/fix issue249 [Java][RestTemplate] Fix query parameter URL encoding Jun 9, 2018
@jmini
Copy link
Member

jmini commented Jun 9, 2018

cc java technical committee:
@bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01)

@jmini
Copy link
Member

jmini commented Jun 9, 2018

@simingweng : Thank you a lot for this contribution.

The Shippable CI job indicates that the generated samples we use as reference (under samples/ in this repository were not updated.

On your branch, can you please run:

  • mvn verify
  • bin/java-petstore-resttemplate.sh
  • bin/java-petstore-resttemplate-withxml.sh

And commit the changed files:

samples/client/petstore/java/resttemplate-withXml/src/main/java/org/openapitools/client/ApiClient.java
samples/client/petstore/java/resttemplate/src/main/java/org/openapitools/client/ApiClient.java

Please let me know if you need help.

@simingweng
Copy link
Contributor Author

@wing328 I've amended the author of the commits, they're now linked to my account.
@jmini I've updated both the java-petstore-resttemplate and java-petstore-resttemplate-with-xml samples, now the Shippable CI job has passed, but the circleci job has an error in jaxrs-jersey2 server sample. Do you see any possibility it could be related to this change?

@wing328
Copy link
Member

wing328 commented Jun 9, 2018

@simingweng I don't think it's related to your change. I've restarted the job. Let's see how it goes.

@jmini
Copy link
Member

jmini commented Jun 10, 2018

Job 545:

ci/circleci — Your tests failed on CircleCI

This job is unrelated:

sbt.ResolveException: download failed: joda-time#joda-time;2.2!joda-time.jar

Job 541:

ci/circleci — Your tests failed on CircleCI

This job is unrelated:

2018-06-09 14:06:49.706:WARN:oejw.WebAppContext:main: Failed startup of context o.e.j.m.p.JettyWebAppContext@21a2bad0{/,file:/home/ubuntu/openapi-generator/samples/server/petstore/jaxrs-datelib-j8/src/main/webapp/,STARTING}{file:/home/ubuntu/openapi-generator/samples/server/petstore/jaxrs-datelib-j8/src/main/webapp/}
javax.servlet.ServletException:
Caused by: 
org.glassfish.jersey.internal.inject.ExtractorException: org.glassfish.jersey.internal.inject.ExtractorException: java.lang.NumberFormatException
Caused by: 
org.glassfish.jersey.internal.inject.ExtractorException: java.lang.NumberFormatException
Caused by: 
java.lang.NumberFormatException

@wing328
Copy link
Member

wing328 commented Jun 10, 2018

https://circleci.com/gh/OpenAPITools/openapi-generator/545?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link only failed with JDK8 but not JDK7 due to failure unrelated to this change so I'll merge it into master.

@wing328 wing328 merged commit 43b60e6 into OpenAPITools:master Jun 10, 2018
jimschubert added a commit to jimschubert/openapi-generator that referenced this pull request Jun 12, 2018
* master: (26 commits)
  Prepare 3.0.1 release (OpenAPITools#280)
  [typescript-angular] strict type checking (OpenAPITools#218)
  [C++ server] Adjust the names (script, sample folder, generator) to lang option (OpenAPITools#250)
  Removed warnings for packages included in SDK for Net Core 2.0 (OpenAPITools#269)
  [cli] Completions command for suggestions (OpenAPITools#213)
  [Java][RestTemplate] Fix query parameter URL encoding (OpenAPITools#260)
  [cpp-qt5] Remove std::shared_ptr from Qt5 (OpenAPITools#267)
  Adds some links to the README (OpenAPITools#261)
  Update sec.gpg.enc to binary encoded secret
  Add gpg --check-trustdb, limit gpg to master
  Re-do encrypted gpg and reference in settings.xml
  Fix trailing semicolons in after_success Travis CI scripts
  Use ubuntu keyserver instead of mit (due to timeout)
  [gradle] Plugin release management (OpenAPITools#201)
  Updates small typo in qna.md (OpenAPITools#262)
  Fix ModelUtils.getUnusedSchema() (OpenAPITools#253)
  Add JaxRS to bin/ensure-up-to-date (OpenAPITools#248)
  feat(security): add cookie-auth support (OpenAPITools#240)
  Add 'unblu inc.' to company list (OpenAPITools#246)
  put company list in alphabetical order (OpenAPITools#244)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants