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

[Spring][server] oneOf Polymorphism support for spring boot #5661

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

alexsuperdev
Copy link
Contributor

@alexsuperdev alexsuperdev commented Mar 22, 2020

Fix 5381
Implemented OneOf for java spring library(server side).

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.

@bbdouglas @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger @karismann @Zomzog @lwlee2608 @bkabrda

added x-is-one-of-interface extension for oneOf interface in mustache
template
fixed name of model from UNKNOWN_BASE_TYPE to right one in api: operationId + OneOf

Fix OpenAPITools#5381
parcelableModel is not required
removed not needed methods
catch NPE cases in preprocessOpenAPI
updated samples
@alexsuperdev alexsuperdev changed the title Spring fix 5381 [Java][Spring] fix 5381 Mar 22, 2020
@alexsuperdev alexsuperdev changed the title [Java][Spring] fix 5381 [Spring][server] oneOf Polymorphism support for spring boot Mar 22, 2020
Copy link
Contributor

@bkabrda bkabrda left a comment

Choose a reason for hiding this comment

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

Hey 👋 this mostly looks good, but I left 2 questions and 1 comment in the code to get answered/addressed.

fixed generation of oneOf Models
addOneOfInterfaceModel only for cases when useOneOfInterfaces is true and for spring
…rator into spring_fix_5381

� Conflicts:
�	modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
spring: fixed use of oneOf Models in API
@@ -764,7 +769,7 @@ public void preprocessOpenAPI(OpenAPI openAPI) {
Schema s = e.getValue();
String nOneOf = toModelName(n + "OneOf");
if (ModelUtils.isComposedSchema(s)) {
if (e.getKey().contains("/")) {
if (e.getKey().contains("/") || (useOneOfInterfaces && LIBRARY_ONEOF_IMPL.equals(templateDir))) {

Choose a reason for hiding this comment

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

The code is specific to the SpringCodegen class and must be written in it???

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the code will be executed only for java spring library, as the specification of this issue require.

Choose a reason for hiding this comment

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

If so, move the code in SpringCodegen.java and all if statments with (.... templateDir.equals(LIBRARY_ONEOF_IMPL....) should be in SpringCodegen.java

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To do it so, i must duplicate almost the whole preprocessOpenAPI method in SpringCodegen, only because of this condition. I wanted to avoid it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Scorpioncik should i do so?

Choose a reason for hiding this comment

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

You don't need to duplicate the whole preprocessOpenAPI, you can extract in a method the piece of code and override it in SpringCodegen.

example:
if (e.getKey().contains("/")) { ====> if (method1()) {

implementing oneOf for spring lib overriding methods with different behavior from default
@Override
protected void addOneOfForComposedSchema(Entry<String, Schema> stringSchemaEntry, String modelName, ComposedSchema composedSchema,
String nOneOf){
if (useOneOfInterfaces) {

Choose a reason for hiding this comment

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

useOneOfInterfaces is always true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, you are right useOneOfInterfaces is always true, i will change the code, that there is no if cases anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Scorpioncik please review this pull request again.

added x-is-one-of-interface extension for oneOf interface in mustache
template
fixed name of model from UNKNOWN_BASE_TYPE to right one in api: operationId + OneOf

Fix OpenAPITools#5381
removed not needed methods

Fix OpenAPITools#5381
fixed generation of oneOf Models

Fix OpenAPITools#5381
addOneOfInterfaceModel only for cases when useOneOfInterfaces is true and for spring

Fix OpenAPITools#5381
NPE fix for tests
fixed handing of composed schema with array
fixed NPE in addOneOfInterfaceModel
@jburgess
Copy link
Contributor

jburgess commented Apr 2, 2020

@alexsuperdev Thanks for you work on this!! I have been monitoring it closely and have been using your fork for testing both oneOf and discriminator patterns. I understand this fork may still be a WIP, but, I have noticed since @bfab304 my code gen has started to fail for the discriminator pattern of extension. I have created a simple OAS that captures the failure. The models are now (d8648310) being generated as empty interfaces:

public interface Cat  {
}

Would you mind running the below OAS with your codebase?

openapi: 3.0.1
info:
  title: OneOfTest01
  description: >+
    This test illustrates an issue with inheritance

  version: 4.0.0
servers:
  - url: 'https://serverRoot'

paths:
  /pets:
    get:
      responses:
        '200':
          $ref: '#/components/responses/PetResponse'
    post:
      requestBody:
        $ref: '#/components/requestBodies/PetRequest'
      responses:
        '201':
          $ref: '#/components/responses/PetResponse'
components:
  schemas:
    Pet:
      type: object
      properties:
        basePetProperty:
          type: string
        id:
          type: string
        petType:
          type: string
      discriminator:
        propertyName: 'petType'
        mapping:
          cat: '#/components/schemas/Cat'
          dog: '#/components/schemas/Dog'

    Cat:
      type: object
      properties:
        catProperty:
          type: string
      allOf:
        - $ref: '#/components/schemas/Pet'
        - type: object
    Dog:
      type: object
      properties:
        dogProperty:
          type: string
      allOf:
        - $ref: '#/components/schemas/Pet'
        - type: object

  requestBodies:
    PetRequest:
      content:
        application/json;charset=utf-8:
          schema:
            $ref: '#/components/schemas/Pet'
      required: true

  responses:
    PetResponse:
      description: Success
      content:
        application/json;charset=utf-8:
          schema:
            type: array
            items:
              $ref: '#/components/schemas/Pet'

fixed generation of oneOf models with descriminator
@alexsuperdev
Copy link
Contributor Author

@alexsuperdev Thanks for you work on this!! I have been monitoring it closely and have been using your fork for testing both oneOf and discriminator patterns. I understand this fork may still be a WIP, but, I have noticed since @bfab304 my code gen has started to fail for the discriminator pattern of extension. I have created a simple OAS that captures the failure. The models are now (d8648310) being generated as empty interfaces:

public interface Cat  {
}

Would you mind running the below OAS with your codebase?

openapi: 3.0.1
info:
  title: OneOfTest01
  description: >+
    This test illustrates an issue with inheritance

  version: 4.0.0
servers:
  - url: 'https://serverRoot'

paths:
  /pets:
    get:
      responses:
        '200':
          $ref: '#/components/responses/PetResponse'
    post:
      requestBody:
        $ref: '#/components/requestBodies/PetRequest'
      responses:
        '201':
          $ref: '#/components/responses/PetResponse'
components:
  schemas:
    Pet:
      type: object
      properties:
        basePetProperty:
          type: string
        id:
          type: string
        petType:
          type: string
      discriminator:
        propertyName: 'petType'
        mapping:
          cat: '#/components/schemas/Cat'
          dog: '#/components/schemas/Dog'

    Cat:
      type: object
      properties:
        catProperty:
          type: string
      allOf:
        - $ref: '#/components/schemas/Pet'
        - type: object
    Dog:
      type: object
      properties:
        dogProperty:
          type: string
      allOf:
        - $ref: '#/components/schemas/Pet'
        - type: object

  requestBodies:
    PetRequest:
      content:
        application/json;charset=utf-8:
          schema:
            $ref: '#/components/schemas/Pet'
      required: true

  responses:
    PetResponse:
      description: Success
      content:
        application/json;charset=utf-8:
          schema:
            type: array
            items:
              $ref: '#/components/schemas/Pet'

Thank you @jburgess also for the tip and example with discriminator.
I made a little change, so now the generated classes from your example seem to be okay for me.
I tested it locally. Please proove it.

@jburgess
Copy link
Contributor

jburgess commented Apr 2, 2020

@alexsuperdev, yes. This seems to have resolved this issue. Thanks!

@alexsuperdev
Copy link
Contributor Author

Hello @bkabrda @Scorpioncik @jburgess @jimschubert. Can any one of you review this pullrequest?

@nielicheng
Copy link

Hi @alexsuperdev Thanks for working on this. Do we have an ETA when this fix will be merged?

@alexsuperdev
Copy link
Contributor Author

Hi @alexsuperdev Thanks for working on this. Do we have an ETA when this fix will be merged?

I dont know when @bkabrda or @jimschubert have a time to look in this pull request and merge this

@ghost
Copy link

ghost commented Dec 3, 2020

Is there anything new about this? I only mean because 224 PRs are waiting at the moment. So I have little hope to ever see the fix...

@adzeeman
Copy link

adzeeman commented Feb 3, 2021

@alexsuperdev Thank you for taking the initiative on implementing this feature - are you still planning on completing this piece of work. It seems that there are maybe people interested in this feature (myself being one of them).

…rator into spring_fix_5381

� Conflicts:
�	modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
@alexsuperdev
Copy link
Contributor Author

alexsuperdev commented Feb 3, 2021

@alexsuperdev Thank you for taking the initiative on implementing this feature - are you still planning on completing this piece of work. It seems that there are maybe people interested in this feature (myself being one of them).

hello @adzeeman. I will try to fix merge conflicts and possibly junit tests this week

@ghost
Copy link

ghost commented Mar 10, 2021

Us SadaPay engineers are very keen to get this merged ASAP - let us know how we can support.

@Bellegar
Copy link

It seems that pojo.mustache must be the same as in #8091, it adds "implementats .." to Java class definition

public class {{classname}} {{#parent}}extends {{{parent}}}{{/parent}}{{^parent}}{{#hateoas}}extends RepresentationModel<{{classname}}> {{/hateoas}}{{/parent}} {{#serializableModel}}implements Serializable{{/serializableModel}} {

-->

public class {{classname}} {{#parent}}extends {{{parent}}}{{/parent}}{{^parent}}{{#hateoas}}extends RepresentationModel<{{classname}}> {{/hateoas}}{{/parent}} {{#vendorExtensions.x-implements}}{{#-first}}implements {{{.}}}{{/-first}}{{^-first}}, {{{.}}}{{/-first}}{{#-last}} {{/-last}}{{/vendorExtensions.x-implements}} {

@alexsuperdev
Copy link
Contributor Author

alexsuperdev commented Apr 27, 2021 via email

@jorgerod
Copy link
Contributor

jorgerod commented Jun 2, 2021

Hi

Any news? This feature is very important for us.
@bkabrda @Scorpioncik @alexsuperdev @jimschubert

@rj93
Copy link

rj93 commented Jun 14, 2021

any news?

@daberni
Copy link
Contributor

daberni commented Jul 1, 2021

Anything we can help with getting the pull request merged? Is there any open issue in the pull request or are we waiting for the maintainers?

@alexsuperdev
Copy link
Contributor Author

Anything we can help with getting the pull request merged? Is there any open issue in the pull request or are we waiting for the maintainers?

I also really don't know why this PR will not be merged by maintainers

@daberni
Copy link
Contributor

daberni commented Jul 2, 2021

Anything we can help with getting the pull request merged? Is there any open issue in the pull request or are we waiting for the maintainers?

I also really don't know why this PR will not be merged by maintainers

looks like there are 2 merge conflicts left, maybe you could fix them to increase the chance for a merge?

…rator into spring_fix_5381

� Conflicts:
�	modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultCodegen.java
�	modules/openapi-generator/src/main/java/org/openapitools/codegen/languages/SpringCodegen.java
@swathisprasad
Copy link

Hi @alexsuperdev,
Thanks for working on this! Are there any updates when this fix will be merged?

@alexsuperdev
Copy link
Contributor Author

Hi @alexsuperdev,
Thanks for working on this! Are there any updates when this fix will be merged?

Its too much changes that need to be manually merged, i don't want to do it, because after it, PR possibly will not be merged by maintainers also. So if someone want to do merge it, you are welcome

@petlju
Copy link

petlju commented Apr 4, 2022

Never contributed to someone else's pull request before. Do I fork your fork and fix it there and do a pull request to your fork? Or how does one go about contributing to someone else's?

@ViaSacra
Copy link

Colleagues, when will this functionality be released? He is sorely missed!

@samiron
Copy link

samiron commented Jan 25, 2024

Is there any chance to get this PR merged any time sooner?

@alexsuperdev
Copy link
Contributor Author

alexsuperdev commented Jan 26, 2024

Is there any chance to get this PR merged any time sooner?
As i understood #11650 have fixed the problem with oneOf and was already merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] [spring][server] oneOf Polymorphism support for spring boot