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][Spring] fix one of #10463

Closed
wants to merge 4 commits into from
Closed

[Java][Spring] fix one of #10463

wants to merge 4 commits into from

Conversation

Orachigami
Copy link

@Orachigami Orachigami commented Sep 23, 2021

Recreated as @wing328 asked

Fixes: #2906
Fixes: #5381
Fixes: #8647
Fixes: #8860
Fixes: #9601
Fixes: #9981

Added tests oneOfModelsGeneration for Java Spring and Java Client which iterate through several specs with OneOf usage cases and check that OneOf models are generated and subtypes implement generated models. Java Client test also creates a cartesian product of specs and client libraries for regress tests.

How inline OneOf models generation is fixed

Added x-deduction and x-deduction-model-names for vendorExtensions. These extensions are used to generated Jackson annotation when the discriminator cannot be used. The discriminator uses the specific field value for deserialization decisions. However, according to the https://swagger.io/specification#discriminator-object first example, oneOf can be used without the discriminator object. This case can be handled with Jackson recently added JsonTypeInfo.Id.DEDUCTION option. DEDUCTION option forces Jackson to make deserialization decisions according to the available fields in the payload and Models which implement oneOf interface/class.

OneOf with Java base classes -> Object

Inline OneOf with Java base classes for RequestSchema and ResponseSchema are replaced with an empty schema.
When OneOf is specified it is expected that the output will produce a common interface/class for all schemas in OneOf. All schemas in OneOf also have to implement/extend the generated OneOf model.
In the case where users specify OneOf with strings, numbers, dates, etc, the generator is unable to force OneOf implementation for Java base classes (and it shouldn't). So one of the ways to solve an issue is to use Object as the common parent for all base types.
Another way to solve this issue is to generate Wrappers for selected base types and force them to implement some common generated OneOf interface.

Example output screenshots

Output examples
Specification usage OneOf code Model code

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/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 (5.3.0), 6.0.x
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@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) @nmuesch (2021/01)

@Orachigami Orachigami marked this pull request as ready for review September 23, 2021 18:48
@Orachigami
Copy link
Author

Orachigami commented Sep 24, 2021

@rgranadosd, I prepared a script to show the result:

mkdir temp
set CLIENT=-g java  --additional-properties library=resttemplate
set SPRING=-g spring
REM Some non-existing version
mvnw versions:set -DnewVersion=5.3.0.0-LOCAL
mvnw clean install -pl !modules/openapi-generator-online,!modules/openapi-generator-maven-plugin,!modules/openapi-generator-gradle-plugin -DskipTests=true -U --settings CI/settings.xml
REM Replace SPRING with CLIENT for java client generation
java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -i modules/openapi-generator/src/test/resources/3_0/oneOf_interface.yaml %SPRING% -o temp

Screenshot for Spring:
image

Screenshot for Java+Rest template
image

This was referenced Sep 27, 2021
}
}
}

Copy link
Member

Choose a reason for hiding this comment

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

FYI. Since this changes the default codegen, it will take a lot more time to review.

}
return false;
}

Copy link
Member

Choose a reason for hiding this comment

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

FYI. Since these changes occur in the abstract java codegen, which is used by the java client codegen, we will need to test the Java client (jersey2, native) generator carefully which already supports oneOf implementation (not using interface though)

Have you tried the oneOf implementation in Java client (jersey2, native) generator? Does it meet your requirement?

@bmaassenee
Copy link

Thanks for trying to fix this, i tried your code, and the following yaml will not generate correct code:

openapi: 3.0.0
info:
  version: "1.0.0"
  title: "API should have import for OneOf Class, models should extend OneOf Class"
paths:
  /addFruits:
    post:
      responses:
        '200':
          description: Returns a list of fruits
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Fruit'
      requestBody:
        content:
          application/json:
            schema:
              $ref: '#/components/schemas/Fruit'
components:
  schemas:
    Fruit:
      type: object
      oneOf:
        - $ref: '#/components/schemas/Apple'
        - $ref: '#/components/schemas/Orange'
      discriminator:
        propertyName: kind
      properties:
        kind:
          type: string
          enum:
            - Apple
            - Orange
      required:
        - kind
    Apple:
      type: object
      properties:
        identifier:
          type: string
    Orange:
      type: object
      properties:        
        identifier2:
          type: string

I've also tried with defining the enum as a ref(such that it generates its own object), but it still generates the interface with string

components:
  schemas:
    FruitKind:
      type: string
      enum:
        - Apple
        - Orange
        
        

@MichaelAAU
Copy link

I would like to know also if there will be a new version of the openapi-generator-maven-plugin after this fix! So far it does not generate code for the OneOf models. Thank you very much!!

@nikolasten
Copy link

Any news on timeline of releasing fix for oneOf ?

@cachescrubber
Copy link
Contributor

cachescrubber commented Jan 13, 2022

@wing328 I favour the implementation in DefaultCodegen and AbstractJavaCodegen even though it is harder to review / verify. Model generation should be as much consistent as possible across the different Dialects.

@cachescrubber
Copy link
Contributor

@Orachigami The branch has merge conflicts. Could you please merge in the current master? If you need any help merging pojo.java or other changed templates, feel free to contact me.

@cachescrubber
Copy link
Contributor

cachescrubber commented Jan 27, 2022

@Orachigami I tested your PR and git the same issue as described here: #10392 (comment)

Basically the same behaviour @bmaassenee described above.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment