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] [Microprofile] Add Json-B polymorphism type info annotations #20164

Merged
merged 9 commits into from
Dec 3, 2024

Conversation

DielN
Copy link
Contributor

@DielN DielN commented Nov 22, 2024

Adds JSON-B type info annotations for the Microprofile library (and Jackson, it seems like type info was generally missing for micrprofile?).

I only added support for MP version 3.0 since the polymorphism stuff requires Jakarta Json Binding version 3 and therefore required jakarta namespace etc.
The reason I'm not simply checking for microprofile3 in the typeInfoAnnotation template is that it is placed in the Java directory and used by multiple libraries. So I decided to add a more general property jsonbPolymorphism that could also be used in other libraries. It indicates that at least version 3 of the json bind API is used.

Since none of the existing configs for the samples used a spec where these annotation could come into play, I changed one Jackson and one Json-B config to use an other petstore spec that does have polymorphism.
With that I also noticed a "bug" in the equals introduced in #20011. It seems like Microprofile does not support JsonNullable and therefore the dependency is missing. The JsonNullable should probably be removed from the equals, I'll create an issue for that. -> disabled openApiNullable for MP library instead since it is not supported anyway

This PR relates to the request in #12343 – but there are more generators that use JSON-B that would need to be implemented.

For the "java" generator, there are many mentions of jsonb in the templates of various libraries, but at the moment, only Microprofile supports it. So the change to the typeInfoAnnotations template in the java directory should not affect any other library at the moment.

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 || exit
    ./bin/generate-samples.sh ./bin/configs/*.yaml || exit
    ./bin/utils/export_docs_generators.sh || exit
    
    (For Windows users, please run the script in Git BASH)
    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*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@DielN
Copy link
Contributor Author

DielN commented Nov 22, 2024

It seems like the JsonNullable in the equals is actually not the problem. As soon as openApiNullable is true, the imports for JsonNullable get added even with a commit before #20011. So the problem is just with openApiNullable=true being unsupported. Since it is unsupported, I have set it to false in the configs now.

Edit: I completely disabled openApiNullable for the Microprofile generator - doesn't make sense to allow this to be true if it's not supported anyway IMO

import {{rootJavaEEPackage}}.json.bind.annotation.JsonbSubtype;
import {{rootJavaEEPackage}}.json.bind.annotation.JsonbTransient;
import {{rootJavaEEPackage}}.json.bind.annotation.JsonbTypeInfo;
{{/jsonbPolymorphism}}
import {{rootJavaEEPackage}}.json.bind.annotation.JsonbCreator;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The JsonbCreator import was previously never included, the vendorExtensions.x-has-readonly-properties is only available in the #model context I think. For Jackson the import only work because it is added in the #imports.

@DielN DielN marked this pull request as ready for review November 26, 2024 14:46
@DielN
Copy link
Contributor Author

DielN commented Nov 26, 2024

Not sure why that one CircleCI check fails on some Go test, seems to be unrelated/random?

This is ready for review now
@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) @martin-mfg (2023/08)

@wing328
Copy link
Member

wing328 commented Nov 30, 2024

restarted the failed job. let's see how that goes

Copy link
Contributor

@martin-mfg martin-mfg 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! It mostly looks good to me. I have only one small comment:

@@ -10,3 +10,4 @@ additionalProperties:
microprofileRestClientVersion: "3.0"
hideGenerationTimestamp: true
useReflectionEqualsHashCode: true
openApiNullable: false
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be removed now, since openApiNullable is being disabled for microprofile. Same comment applies to bin/configs/java-microprofile-rest-client-3.0.yaml.

@DielN DielN requested a review from martin-mfg December 2, 2024 14:28
@DielN
Copy link
Contributor Author

DielN commented Dec 2, 2024

Thanks for the review!
I updated the configs & added to the documentation that the openApiNullable option is not supported for microprofile. Let me know what you think.

@wing328 wing328 merged commit b9f6fe6 into OpenAPITools:master Dec 3, 2024
72 checks passed
github-merge-queue bot pushed a commit to infonl/dimpact-zaakafhandelcomponent that referenced this pull request Jan 27, 2025
Upgraded to openapi generator version 7.11. One of our issues was fixed
in this version but we got two new issues back for which I added
workarounds.. It looks like both are caused by the new support for
JSON-B polymorphism type annotations introduced by
OpenAPITools/openapi-generator#20164:

Solves PZ-5160
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