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

Added jackson as supported serialization library for retrofit2 #16853

Conversation

gf-smtzgr
Copy link
Contributor

@gf-smtzgr gf-smtzgr commented Oct 17, 2023

To close #7435

This adds support for jackson as serialization library to the retrofit2 java generator. Basically this seems already mostly supported by the general java classes/templates as an option. Apparently it is just explicitly excluded and lacks the jackson configuration example file. This PR adds the file (copied from variants that already support jackson) and otherwise relies on the java model files already supporting this option. Usage of jackson should only be a question of setting serialzationLibrary to jackson (e.g. in the maven config).
Note: The steps below were executed but did not generate additional files to add.
Note: I'm aware of the existing PR #7435 . This PR basically provides equivalent functionality, but supports the standard option for jackson support and is based on the most recent master version. Therefore I assume that PR can be closed should this one be accepted.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
    These must match the expectations made by your contribution.
    You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
    For Windows users, please run the script in Git BASH.
  • File the PR against the correct branch: master (upcoming 7.1.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks);
    -> master as I assume support for an already existing configuration parameter is non-breaking.
    Note: if you want to be pedantic you could consider it a breaking change as it can change behavior in the following scenario: If a project currently has jackson as serialization library configured with retrofit2 this would automatically be switched to gson and gson support would be generated. With this change the configuration of such a project would not be overridden, but instead jackson support would be generated now as configured; however, a project configuring jackson as serialization library and then relying on the fallback to gson seems misconfigured on the project's side in the first place. However, feel free to redirect the PR if you feel that level of change is not wanted in master branch.
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request. Since this is a java targeting change: @bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @karismann @Zomzog @lwlee2608 @martin-mfg

@wing328
Copy link
Member

wing328 commented Nov 1, 2023

Thanks for the PR. I did a test with java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g java -i modules/openapi-generator/src/test/resources/3_0/petstore.yaml -o /tmp/java --library retrofit2 --additional-properties serializationLibrary=jackson but the output won't compile, e.g. :

[ERROR]   location: class org.openapitools.client.model.User
[ERROR] /private/tmp/java/src/main/java/org/openapitools/client/model/User.java:[210,4] cannot find symbol
[ERROR]   symbol:   class JsonInclude
[ERROR]   location: class org.openapitools.client.model.User

Can you please take a look when you've time?

…vely include only gson or jackson specific conversion support depending on which serialization framework is selected
…ctively only include those necessary for gson or jackson depending on which serialization framework is selected
@gf-smtzgr
Copy link
Contributor Author

Replicated the issue. Working on fixing the imports and the ApiClient template. I will let you know when I'm done or need assistance.

@wing328
Copy link
Member

wing328 commented Nov 7, 2023

can you please resolve the merge conflicts when you've time?

…at had gson dependencies but does not seem to use it and now fully relies on the jackson setting to control the respective dependencies; play version property and dependency changed place because the jackson dependencies are grouped together and the play26 ones are placed after
@gf-smtzgr
Copy link
Contributor Author

@wing328 Fixed the merge issues and updated the dependency handling. Note that the PR now also slightly changes the playws26 sample: It removes some gson dependencies (those seem unused to me, at least I could compile the generated sample) and shifts the others a bit around. the playws26 support already included jackson but now the respective dependencies are grouped together and controlled separately with the "jackson" flag. Please take a look whether this is acceptable now - assuming the test pipelines run through.

@wing328 wing328 modified the milestones: 7.1.0, 7.2.0 Nov 13, 2023
@wing328 wing328 removed this from the 7.2.0 milestone Dec 22, 2023
@wing328
Copy link
Member

wing328 commented Feb 8, 2024

can you please resolve it one more time and PM me via Slack when it's done?

thanks again for the PR.

https://join.slack.com/t/openapi-generator/shared_invite/zt-12jxxd7p2-XUeQM~4pzsU9x~eGLQqX2g

@wing328
Copy link
Member

wing328 commented Feb 19, 2024

i tested with java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g java --library retrofit2 -i modules/openapi-generator/src/test/resources/3_0/petstore.yaml -o /tmp/retrofit2/ (which should use gson) but the output pom.xml has issue

can you please take a look when you've time?

…used independently of jackson support it seems
@gf-smtzgr
Copy link
Contributor Author

gf-smtzgr commented Feb 19, 2024

I just pushed a fix for the issues introduced with the merge. That should also solve what you noticed. And sorry, but have no Slack account yet.

@wing328 wing328 changed the title added jackson as supported serialization library for retrofit2; resolves #7435 Added jackson as supported serialization library for retrofit2 Feb 20, 2024
@wing328 wing328 added this to the 7.4.0 milestone Feb 20, 2024
@wing328
Copy link
Member

wing328 commented Feb 21, 2024

I tested with both gson and jackson serialization library. mvn works fine but gradle build results in error in both tests.

Can you please take a look when you've time?

@gf-smtzgr
Copy link
Contributor Author

gf-smtzgr commented Feb 21, 2024

Ah, sorry, not very familiar with gradle and didn't try that. I've updated the gradle dependencies to match the last changes I did to the maven ones. Now retrofit2 example compiles for me via gradle both with jackson and with the default. However this adds the jackson-databind dependency and the javax.ws.rs:javax.ws.rs-api dependency in some of the retrofit/play gradle projects where they were apparently not needed before and I'm not sure why (perhaps some other dependency brought them along there? or maybe the classes that use them in the retrofit2 library are not present/overwritten)

@gf-smtzgr
Copy link
Contributor Author

gf-smtzgr commented Feb 21, 2024

I tried to compile the other affected samples and most of them do compile, except for retrofit2-play26 - where I get this error

> Task :compileJava FAILED
samples/client/petstore/java/retrofit2-play26/src/main/java/org/openapitools/client/Play26CallFactory.java:138: error: method does not override or implement a method from a supertype
        @Override
        ^
Note: /samples/client/petstore/java/retrofit2-play26/src/main/java/org/openapitools/client/Play26CallFactory.java uses or overrides a deprecated API.

but that occurs with or without my change and seems to refer to the 'Call' parent class. So that sample might be outdated with regard to the retrofit Call class in general?

@gf-smtzgr
Copy link
Contributor Author

Tried the same sample on master branch and get the same error so that's likely an issue independent of this MR... (could also be my jvm, not sure which you use for the tests) let me know if you disagree or anything else needs addressing.

@wing328 wing328 merged commit 2d15510 into OpenAPITools:master Mar 4, 2024
59 checks passed
Philzen added a commit to Philzen/openapi-generator that referenced this pull request Jun 2, 2024
wing328 pushed a commit that referenced this pull request Jun 2, 2024
* Drop separate version property

Not required as all Jackson packages usually tether on a version bump.

* Update Jackson to v2.17.1

* Sync Jackson version used by Spring Boot with project version

* Sync jackson update to v2.17.1 with generator templates

* Regenerate samples with updated versions

* Adjust test to current exception msg behavior

* Add dependency mgmt to ensure matching version for transitive dependencies

* Update library descriptions with correct Jackson version number

* Update library descriptions with correct GSON and JSONB versions

* Update retrofit library description with correct version number

* Update retrofit description to include Jackson as an option

This should have been done as part of in #16853.

* Update remaining libary version descriptions with correct versions

* Generate updated doc
welshm pushed a commit to welshm/openapi-generator that referenced this pull request Jun 5, 2024
)

* Drop separate version property

Not required as all Jackson packages usually tether on a version bump.

* Update Jackson to v2.17.1

* Sync Jackson version used by Spring Boot with project version

* Sync jackson update to v2.17.1 with generator templates

* Regenerate samples with updated versions

* Adjust test to current exception msg behavior

* Add dependency mgmt to ensure matching version for transitive dependencies

* Update library descriptions with correct Jackson version number

* Update library descriptions with correct GSON and JSONB versions

* Update retrofit library description with correct version number

* Update retrofit description to include Jackson as an option

This should have been done as part of in OpenAPITools#16853.

* Update remaining libary version descriptions with correct versions

* Generate updated doc
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.

2 participants