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][Jersey2] various improvements #6518

Merged
merged 20 commits into from
Jun 9, 2020
Merged

[Java][Jersey2] various improvements #6518

merged 20 commits into from
Jun 9, 2020

Conversation

wing328
Copy link
Member

@wing328 wing328 commented Jun 2, 2020

  • add the option "oneOfDiscriminatorLookup" to speed up the oneOf schema matching process
  • add additionalProperties support
  • add serializer and deserializer to oneof/anyof schemas
  • remove oneof/anyof logic from ApiClient

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.

sebastien-rosset and others added 2 commits June 2, 2020 15:47
…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]>
@wing328 wing328 added the WIP Work in Progress label Jun 2, 2020
wing328 and others added 14 commits June 2, 2020 17:19
* 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]>
…arge 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]>
@wing328 wing328 marked this pull request as ready for review June 9, 2020 07:26
@wing328 wing328 added this to the 5.0.0 milestone Jun 9, 2020
@wing328 wing328 merged commit 233087c into master Jun 9, 2020
@wing328 wing328 deleted the jersey2-improvement branch June 9, 2020 16:38
case "BasquePig":
deserialized = tree.traverse(jp.getCodec()).readValueAs(BasquePig.class);
newPig.setActualInstance(deserialized);
return newPig;
Copy link
Contributor

@spacether spacether Jun 9, 2020

Choose a reason for hiding this comment

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

So it looks like this sets this.instance = to the deserialized instance of BasquePig where this is a Pig instance? Is that right?
Why include the BasquePig instances in this Pig instance vs just returning the instance of BasquePig?
You mentioned a use case when a client may want to change the type of an instance from
BasquePig to DanishPig.
That is possible if one knows about the ancestor class Pig, which the client would know because that is the data type that is being deserialized.
My understanding is that you want a client to be able to change a model instance class type(BasquePig/DanishPig) given only the deserialized instance and NOT having to know about the ingested data type, is that right?
Can you describe a more realistic use case than converting pig types?
Why do clients need this ability to change model instance types given a model instance only?

Copy link
Member Author

@wing328 wing328 Jun 10, 2020

Choose a reason for hiding this comment

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

So it looks like this sets this.instance = to the deserialized instance of BasquePig where this is a Pig instance? Is that right?

Yes

Why include the BasquePig instances in this Pig instance vs just returning the instance of BasquePig?

Because Pig is an oneOf. Just returning BasqueBig means the client will lose the information that the return type is oneOf and DanishPig is another possibility.

That is possible if one knows about the ancestor class Pig, which the client would know because that is the data type that is being deserialized.

You mean DanishPig stores the oneOf schema information (Pig in this case)? What if DanishPig happens to be the oneOf schema node in "Creature" or "Animal" oneOf schema?

My understanding is that you want a client to be able to change a model instance class type(BasquePig/DanishPig) given only the deserialized instance and NOT having to know about the ingested data type, is that right?

One can change the model (DanishPig or BasqueBig) in the client side independent of how "Pig" is created - whether it's deserialized from a payload returned by the server or it's created in the client side from scratch.

Can you describe a more realistic use case than converting pig types?

  • The client calls the petPig and gets a Pig (which happens to be a DanishPig in this example)
  • The client changes the instance from DanishPig to Snake and got an error (which is the expected behaviour according to the spec)
  • The client changes the instance form DanishPig to BasqueBig and got no error
  • The client calls updatePet with the Pig object (originally obtained from getPig) with the underlying instance set to BasqueBig in the above step
  • The server checks the payload (BasqueBig) and accepts it since it conforms to the Pig definition (oneOf)

I would suggest you step back from actual use case as OpenAPI spec is a contract and the implementation should work with respect to the contract. (this is not to say real use cases are bad)

Copy link
Contributor

@sebastien-rosset sebastien-rosset Jun 10, 2020

Choose a reason for hiding this comment

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

I would suggest you step back from actual use case as OpenAPI spec is a contract and the implementation should work with respect to the contract. (this is not to say real use cases are bad)

Can you point to a section of the OAI spec where the "contract" would be broken if the SDK returns a BasquePig instead of Pig? The OAI spec is mostly about validation. I don't see wording that states what kind of implementation objects a client is supposed to return or generate.

IMO, a client should be able to expose all of the following methods. I don't see anything in the spec that would block a client from exposing useful methods to the application code. There isn't a single "right" method.

  1. A method that returns the direct oneOf schema, e.g. Pig in this example.
  2. A method that returns the most concrete schema instance, e.g. BasquePig in this example
  3. A method that returns the raw JSON node
  4. Any other method that the SDK developer think is useful to application developers.

@@ -39,9 +51,16 @@ import javax.validation.Valid;
{{#performBeanValidation}}
import org.hibernate.validator.constraints.*;
{{/performBeanValidation}}
import {{invokerPackage}}.JSON;

Choose a reason for hiding this comment

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

Hello @wing328 ,
Would you know why this import is required? I'm trying to generate code without the supporting files and it does not compile because of this import

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.

4 participants