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.

Sign up for GitHub

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

Jump to bottom

[Java][jersey2] Use builder pattern for requests #4666

Merged
merged 8 commits into from
Jan 18, 2020
Merged

[Java][jersey2] Use builder pattern for requests #4666

merged 8 commits into from
Jan 18, 2020

Conversation

zippolyte
Copy link
Contributor

@zippolyte zippolyte commented Dec 2, 2019 •
edited
Loading

Add support for group parameters in the jersey2 client.
It follows the pattern used in #1341, with a small adjustment: it only adds path params, and not all required parameters.

This is done in order to generate a more future proof client and prevent potential backward incompatible changes when required query parameters become optional for instance.

The reasoning for adding only path params is that a change (addition/removal of a path param) would be considered a major API contract change, and thus require a major bump of the API version, hence a major of the client as well.

Please let me know what you think.

cc @bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10)

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

@wing328
Copy link
Member

wing328 commented Dec 11, 2019

Looks like it's causing issue as reported by the CI:

[ERROR] COMPILATION ERROR : 
[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/jersey2/src/test/java/org/openapitools/client/api/FakeApiTest.java:[223,12] method testGroupParameters in class org.openapitools.client.api.FakeApi cannot be applied to given types;
  required: no arguments
  found: java.lang.Integer,java.lang.Boolean,java.lang.Long,java.lang.Integer,java.lang.Boolean,java.lang.Long
  reason: actual and formal argument lists differ in length
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.6.1:testCompile (default-testCompile) on project petstore-jersey2: Compilation failure
[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/jersey2/src/test/java/org/openapitools/client/api/FakeApiTest.java:[223,12] method testGroupParameters in class org.openapitools.client.api.FakeApi cannot be applied to given types;
[ERROR] required: no arguments
[ERROR] found: java.lang.Integer,java.lang.Boolean,java.lang.Long,java.lang.Integer,java.lang.Boolean,java.lang.Long
[ERROR] reason: actual and formal argument lists differ in length
[ERROR] -> [Help 1]
[ERROR]

Does it work for you locally?

@wing328 wing328 added this to the 4.2.3 milestone Dec 11, 2019
@zippolyte zippolyte mentioned this pull request Dec 13, 2019
Merged
5 tasks
@zippolyte
Copy link
Contributor Author

Friendly ping, I could really use some eyes on this one 🙇

@zippolyte
Copy link
Contributor Author

Bumping again, I would really appreciate some feedback on this 🙇

@jimschubert
Copy link
Member

jimschubert commented Jan 10, 2020 •
edited
Loading

I think this looks good, and I personally prefer the builder pattern.

I wonder if this will I impact existing users (consumers of generated code)? If so, should we target the 4.3.x branch?

@zippolyte
Copy link
Contributor Author

It should not, because I made the change behind the x-group-parameters extension flag. So the code of existing users should not change as long as they haven’t set this in their specifications.
That said, I don’t have a problem targeting 4.3.x instead if you think it’s safer.

@zippolyte
Copy link
Contributor Author

@jimschubert anything preventing the merge here ?

@jimschubert
Copy link
Member

No concerns from me as long as there are no breaking changes. I wonder if someone on the technical committee might have a sample app which uses this client and can verify?

cc @bbdouglas (2017/07) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09) @jeff9finger (2018/01) @karismann (2019/03) @Zomzog (2019/04) @lwlee2608 (2019/10)

@zippolyte
Copy link
Contributor Author

FWIW, if we look at the generated samples, the only places where the code actually changes if for samples explicitly setting x-group-parameters to true, like here for example: https://github.com/OpenAPITools/openapi-generator/pull/4666/files#diff-8afa920066ea22142ddcda6b8ad0d6ecL768-R940

wing328
wing328 reviewed Jan 18, 2020
View reviewed changes
samples/client/petstore/java/jersey2-java6/docs/FakeApi.md
apiInstance.testBodyWithFileSchema(body);


Copy link
Member

Choose a reason for hiding this comment

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

@zippolyte I'll see if I can get rid of these empty lines and the trailing space after try { in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

Filed #5031

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this ! I always find it tricky to format properly with the templating.

wing328
wing328 approved these changes Jan 18, 2020
View reviewed changes
Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

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

LGTM

@wing328 wing328 merged commit ee984c3 into OpenAPITools:master Jan 18, 2020
@zippolyte zippolyte deleted the hippo/jov branch January 19, 2020 13:45
aloisklink added a commit to aloisklink/openapi-generator that referenced this pull request May 11, 2020
@aloisklink
[java-jersey2-java6] Update failing old tests
30c6d2e 
Pull-request OpenAPITools#4666 changed jersey generation, but didn't update the
test samples Tests now succeed.

on-behalf-of: @nqminds <[email protected]>
wing328 added a commit that referenced this pull request Jun 16, 2020
@wing328 @aloisklink
[java] Specify Java version for maven-javadoc-plugin (fixes Java 11 e…
213c38c 
…rror) (#6679)

* Add jersey2-experimental to petstore build script

on-behalf-of: @nqminds <[email protected]>

* [java] Add <source> to javadoc in pom.mustache

We add the following <source> tag to the <configuration> of
maven-javadoc-plugin for most pom.mustache files that use it.
This tells javadoc which version of java the compiler used.

This fixes the following error when running Java 11:
[ERROR] Exit code: 1 - javadoc: error - The code being documented uses
modules but the packages defined in
https://docs.oracle.com/javase/8/docs/api/ are in the unnamed module

Additionally, we also add maven-compiler-plugin to jersey2/pom.mustache
to specify that the source code is Java 6/7/8.

on-behalf-of: @nqminds <[email protected]>

* [java-jersey2-java6] Update failing old tests

Pull-request #4666 changed jersey generation, but didn't update the
test samples Tests now succeed.

on-behalf-of: @nqminds <[email protected]>

* [java-retrofit2-play24] Fix integration-tests

Running mvn integration-test failed in
samples/client/petstore/java/retrofit2-play24

This merges pull requests #1735 and #5527 into
retrofit2-play24.

Also removes the jackson-databind-version field,
since it should always be the same as jackson-version,
and updates build.gradle/build.sbt

on-behalf-of: @nqminds <[email protected]>

Co-authored-by: Alois Klink <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wing328 wing328 wing328 approved these changes

Assignees
No one assigned
Labels
Client: Java Enhancement: Feature
Projects
None yet
Milestone
4.2.3
Development

Successfully merging this pull request may close these issues.
3 participants
@zippolyte @wing328 @jimschubert