Skip to content

[java][jersey2] Add support for discriminator, fix nullable typo and nullable deserialization #6495

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

Merged

Conversation

sebastien-rosset
Copy link
Contributor

@sebastien-rosset sebastien-rosset commented May 30, 2020

  1. Fix nullable typo, the mustache template had isNulalble
  2. Add support for discriminator in jersey2 library.
  3. The deserialization code was incorrectly accepting null as input data for non-nullable anyOf/oneOf schemas. If the input data is null and the anyOf/oneOf does not accept null (either because nullable: true or type: 'null' is a child anyOf/oneOf, then a deserialization exception should be raised. Note: it's likely this problem exists for other schema types too.
  4. When there is more than one level of anyOf/oneOf, the deserialization was previously failing. This is shown with the existing sample OAS 3.0 doc that works with the python and go clients.

Newly discovered issues:

  1. The existing jersey2 implementation of oneOf/anyOf is incomplete. It is supposed to validate the input data against the JSON schema, but it does not validate any constraint (enum, min, max...)
  2. Other issues listed with TODO comments in the Unit test file.

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.


@Test
public void testOneOfNestedComposedSchema() throws Exception {
/*
Copy link
Contributor Author

@sebastien-rosset sebastien-rosset Jun 2, 2020

Choose a reason for hiding this comment

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

@wing328 , I have temporarily commented out this code. If I enable it, then deserialization fails and the unit test fails. It looks like there is a problem when a compose schema (Drawing) has the additionalProperties keyword, in which case the generated Drawing.class extends from HashMap<String, X>

return null;
{{/isNullable}}
{{^isNullable}}
throw new JsonMappingException("{{classname}} cannot be null");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wing328 , is there a better way to fail deserialization of the null value that would work across all OAS schema types?

@@ -69,6 +86,7 @@ public class {{classname}} extends AbstractOpenApiSchema{{#vendorExtensions.x-im
schemas.put("{{{.}}}", new GenericType<{{{.}}}>() {
});
{{/anyOf}}
JSON.registerDescendants({{classname}}.class, Collections.unmodifiableMap(schemas));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@wing328 , it's a bit hacky for now, I'd rather not use a public method, but it's needed to have proper deserialization when there are multiple levels of oneOf/anyOf.

@sebastien-rosset sebastien-rosset marked this pull request as ready for review June 2, 2020 05:48
@sebastien-rosset sebastien-rosset changed the title [java][jersey2] Add support for discriminator, bug fixes [java][jersey2] Add support for discriminator, fix nullable typo and nullable deserialization Jun 2, 2020
@wing328
Copy link
Member

wing328 commented Jun 2, 2020

The CircleCI failed with the following errors (partial):

[ERROR] location: class JSON
[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/jersey2-java8/src/main/java/org/openapitools/client/JSON.java:[256,45] error: cannot find symbol
[ERROR] symbol:   class IsoscelesTriangle
[ERROR] location: class JSON
[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/jersey2-java8/src/main/java/org/openapitools/client/JSON.java:[257,43] error: cannot find symbol
[ERROR] symbol:   class ScaleneTriangle
[ERROR] location: class JSON
[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/jersey2-java8/src/main/java/org/openapitools/client/JSON.java:[258,36] error: cannot find symbol
[ERROR] symbol:   class Triangle
[ERROR] location: class JSON
[ERROR] /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/java/jersey2-java8/src/main/java/org/openapitools/client/JSON.java:[259,30] error: cannot find symbol
[ERROR] symbol:   class Triangle
[ERROR] location: class JSON
[ERROR] -> [Help 1]
[ERROR] 
[ERROR] To see the full stack trace of the errors, re-run Maven with the -e switch.
[ERROR] Re-run Maven using the -X switch to enable full debug logging.
[ERROR] 

I'll merge it into a branch first and further improve it.

@wing328 wing328 changed the base branch from master to jersey2-improvement June 2, 2020 07:35
@wing328 wing328 merged commit e05439c into OpenAPITools:jersey2-improvement Jun 2, 2020
@sebastien-rosset
Copy link
Contributor Author

For additionalProperties, I propose to use a Java field of type Map<String, X> instead of currently using class inheritance. The reason is that 1) additionalProperties keyword can be used with allOf composed schema, and 2) it makes more sense to use class inheritance for allOf rather than additionalProperties.

This would require a custom java deserializer.

wing328 added a commit that referenced this pull request Jun 9, 2020
* [java][jersey2] Add support for discriminator, fix nullable typo and nullable deserialization (#6495)

* Mustache template should use invokerPackage tag to generate import

* fix typo, fix script issue, add log statement for troubleshooting

* Add java jersey2 samples with OpenAPI doc that has HTTP signature security scheme

* Add sample for Java jersey2 and HTTP signature scheme

* Add unit test for oneOf schema deserialization

* Add unit test for oneOf schema deserialization

* Add log statements

* Add profile for jersey2

* Temporarily disable unit test

* Temporarily disable unit test

* support for discriminator in jersey2

* fix typo in pom.xml

* disable unit test because jersey2 deserialization is broken

* disable unit test because jersey2 deserialization is broken

* fix duplicate jersey2 samples

* fix duplicate jersey2 samples

* Add code comments

* fix duplicate artifact id

* fix duplicate jersey2 samples

* run samples scripts

* resolve merge conflicts

* Add unit tests

* fix unit tests

* continue implementation of discriminator lookup

* throw deserialization exception when value is null and schema does not allow null value

* continue implementation of compose schema

* continue implementation of compose schema

* continue implementation of compose schema

* Add more unit tests

* Add unit tests for anyOf

* Add unit tests

Co-authored-by: Vikrant Balyan (vvb) <[email protected]>

* update samples

* add tests to oas3 java jersey2 petstore

* comment out jersey2 ensure uptodate

* Jersey2 supports additional properties with composed schema (#6523)

* Mustache template should use invokerPackage tag to generate import

* fix typo, fix script issue, add log statement for troubleshooting

* Add java jersey2 samples with OpenAPI doc that has HTTP signature security scheme

* Add sample for Java jersey2 and HTTP signature scheme

* Add unit test for oneOf schema deserialization

* Add unit test for oneOf schema deserialization

* Add log statements

* Add profile for jersey2

* Temporarily disable unit test

* Temporarily disable unit test

* support for discriminator in jersey2

* fix typo in pom.xml

* disable unit test because jersey2 deserialization is broken

* disable unit test because jersey2 deserialization is broken

* fix duplicate jersey2 samples

* fix duplicate jersey2 samples

* Add code comments

* fix duplicate artifact id

* fix duplicate jersey2 samples

* run samples scripts

* resolve merge conflicts

* Add unit tests

* fix unit tests

* continue implementation of discriminator lookup

* throw deserialization exception when value is null and schema does not allow null value

* continue implementation of compose schema

* continue implementation of compose schema

* continue implementation of compose schema

* Add more unit tests

* Add unit tests for anyOf

* Add unit tests

* Set supportsAdditionalPropertiesWithComposedSchema to true for Java jersey2

* Support additional properties as nested field

* Support additional properties as nested field

* add code comments

* add customer deserializer

* Fix 'method too big' error with generated code

* resolve merge conflicts

Co-authored-by: Vikrant Balyan (vvb) <[email protected]>

* [Jersey2] Fix code generation of 'registerDiscriminator' method for large models (#6535)

* Mustache template should use invokerPackage tag to generate import

* fix typo, fix script issue, add log statement for troubleshooting

* Add java jersey2 samples with OpenAPI doc that has HTTP signature security scheme

* Add sample for Java jersey2 and HTTP signature scheme

* Add unit test for oneOf schema deserialization

* Add unit test for oneOf schema deserialization

* Add log statements

* Add profile for jersey2

* Temporarily disable unit test

* Temporarily disable unit test

* support for discriminator in jersey2

* fix typo in pom.xml

* disable unit test because jersey2 deserialization is broken

* disable unit test because jersey2 deserialization is broken

* fix duplicate jersey2 samples

* fix duplicate jersey2 samples

* Add code comments

* fix duplicate artifact id

* fix duplicate jersey2 samples

* run samples scripts

* resolve merge conflicts

* Add unit tests

* fix unit tests

* continue implementation of discriminator lookup

* throw deserialization exception when value is null and schema does not allow null value

* continue implementation of compose schema

* continue implementation of compose schema

* continue implementation of compose schema

* Add more unit tests

* Add unit tests for anyOf

* Add unit tests

* Fix 'method too big' error with generated code

* resolve merge conflicts

Co-authored-by: Vikrant Balyan (vvb) <[email protected]>

* update samples

* comment out tests

* support additional properties in serialize and deserialize

* add discriminator lookup

* remove oneof/anyof logic in apilcient

* add serializer to mammal.java

* add serialize to oneOf model

* add serializer to anyof model

* comment out test cases that are subject to further discussion

* add back files

* update configs, samples

Co-authored-by: Sebastien Rosset <[email protected]>
Co-authored-by: Vikrant Balyan (vvb) <[email protected]>
wing328 added a commit that referenced this pull request Jun 10, 2020
* Mustache template should use invokerPackage tag to generate import

* fix typo, fix script issue, add log statement for troubleshooting

* Add java jersey2 samples with OpenAPI doc that has HTTP signature security scheme

* Add sample for Java jersey2 and HTTP signature scheme

* Add unit test for oneOf schema deserialization

* Add unit test for oneOf schema deserialization

* Add log statements

* Add profile for jersey2

* Temporarily disable unit test

* Temporarily disable unit test

* support for discriminator in jersey2

* fix typo in pom.xml

* disable unit test because jersey2 deserialization is broken

* disable unit test because jersey2 deserialization is broken

* fix duplicate jersey2 samples

* fix duplicate jersey2 samples

* Add code comments

* fix duplicate artifact id

* fix duplicate jersey2 samples

* run samples scripts

* resolve merge conflicts

* Add unit tests

* fix unit tests

* continue implementation of discriminator lookup

* throw deserialization exception when value is null and schema does not allow null value

* continue implementation of compose schema

* continue implementation of compose schema

* continue implementation of compose schema

* Add more unit tests

* Add unit tests for anyOf

* Add unit tests

* [java][jersey2] Add support for discriminator, fix nullable typo and nullable deserialization (#6495)

* Mustache template should use invokerPackage tag to generate import

* fix typo, fix script issue, add log statement for troubleshooting

* Add java jersey2 samples with OpenAPI doc that has HTTP signature security scheme

* Add sample for Java jersey2 and HTTP signature scheme

* Add unit test for oneOf schema deserialization

* Add unit test for oneOf schema deserialization

* Add log statements

* Add profile for jersey2

* Temporarily disable unit test

* Temporarily disable unit test

* support for discriminator in jersey2

* fix typo in pom.xml

* disable unit test because jersey2 deserialization is broken

* disable unit test because jersey2 deserialization is broken

* fix duplicate jersey2 samples

* fix duplicate jersey2 samples

* Add code comments

* fix duplicate artifact id

* fix duplicate jersey2 samples

* run samples scripts

* resolve merge conflicts

* Add unit tests

* fix unit tests

* continue implementation of discriminator lookup

* throw deserialization exception when value is null and schema does not allow null value

* continue implementation of compose schema

* continue implementation of compose schema

* continue implementation of compose schema

* Add more unit tests

* Add unit tests for anyOf

* Add unit tests

Co-authored-by: Vikrant Balyan (vvb) <[email protected]>

* update samples

* add tests to oas3 java jersey2 petstore

* Fix 'method too big' error with generated code

* resolve merge conflicts

* comment out jersey2 ensure uptodate

* fix compiler warnings

* Jersey2 supports additional properties with composed schema (#6523)

* Mustache template should use invokerPackage tag to generate import

* fix typo, fix script issue, add log statement for troubleshooting

* Add java jersey2 samples with OpenAPI doc that has HTTP signature security scheme

* Add sample for Java jersey2 and HTTP signature scheme

* Add unit test for oneOf schema deserialization

* Add unit test for oneOf schema deserialization

* Add log statements

* Add profile for jersey2

* Temporarily disable unit test

* Temporarily disable unit test

* support for discriminator in jersey2

* fix typo in pom.xml

* disable unit test because jersey2 deserialization is broken

* disable unit test because jersey2 deserialization is broken

* fix duplicate jersey2 samples

* fix duplicate jersey2 samples

* Add code comments

* fix duplicate artifact id

* fix duplicate jersey2 samples

* run samples scripts

* resolve merge conflicts

* Add unit tests

* fix unit tests

* continue implementation of discriminator lookup

* throw deserialization exception when value is null and schema does not allow null value

* continue implementation of compose schema

* continue implementation of compose schema

* continue implementation of compose schema

* Add more unit tests

* Add unit tests for anyOf

* Add unit tests

* Set supportsAdditionalPropertiesWithComposedSchema to true for Java jersey2

* Support additional properties as nested field

* Support additional properties as nested field

* add code comments

* add customer deserializer

* Fix 'method too big' error with generated code

* resolve merge conflicts

Co-authored-by: Vikrant Balyan (vvb) <[email protected]>

* [Jersey2] Fix code generation of 'registerDiscriminator' method for large models (#6535)

* Mustache template should use invokerPackage tag to generate import

* fix typo, fix script issue, add log statement for troubleshooting

* Add java jersey2 samples with OpenAPI doc that has HTTP signature security scheme

* Add sample for Java jersey2 and HTTP signature scheme

* Add unit test for oneOf schema deserialization

* Add unit test for oneOf schema deserialization

* Add log statements

* Add profile for jersey2

* Temporarily disable unit test

* Temporarily disable unit test

* support for discriminator in jersey2

* fix typo in pom.xml

* disable unit test because jersey2 deserialization is broken

* disable unit test because jersey2 deserialization is broken

* fix duplicate jersey2 samples

* fix duplicate jersey2 samples

* Add code comments

* fix duplicate artifact id

* fix duplicate jersey2 samples

* run samples scripts

* resolve merge conflicts

* Add unit tests

* fix unit tests

* continue implementation of discriminator lookup

* throw deserialization exception when value is null and schema does not allow null value

* continue implementation of compose schema

* continue implementation of compose schema

* continue implementation of compose schema

* Add more unit tests

* Add unit tests for anyOf

* Add unit tests

* Fix 'method too big' error with generated code

* resolve merge conflicts

Co-authored-by: Vikrant Balyan (vvb) <[email protected]>

* Add unit test for date time

* Add unit test for date time

* update samples

* comment out tests

* support additional properties in serialize and deserialize

* add discriminator lookup

* remove oneof/anyof logic in apilcient

* add serializer to mammal.java

* add serialize to oneOf model

* add serializer to anyof model

* comment out test cases that are subject to further discussion

* add back files

* update configs, samples

* resolve merge conflicts

Co-authored-by: Vikrant Balyan (vvb) <[email protected]>
Co-authored-by: William Cheng <[email protected]>
@wing328 wing328 added this to the 5.0.0 milestone Jul 3, 2020
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