[Java][client] oneof support for jackson clients

[bkabrda]

This PR represents implementation of oneOf support for Java clients that use Jackson as serialization library. I based this on discussion in #15.

This implements oneOf in the following way:

• The object/property that holds oneOf mapping is generated as an interface with proper jackson JsonSubTypes decorator derived from discriminator - this makes it possible for jackson to detect the implementing class that should be used when deserializing.
• The objects referenced in oneOf implement this interface.
• A getter for the discriminator attribute is generated in the above mentioned interface, making it easy to write code such as (assuming the API response contains x, which is the oneOf interface):if (x.getType() == "type1") { Type1 type1 = (Type1) x };
• If there are any additional properties defined on the oneOf-containing object, they are added to all of the oneOf members.

Limitations:

• Doesn't work with inline-defined (non-ref) members of oneOf.
• Doesn't work if discriminator is not used (more specifically, it won't be able to deserialize API responses, as the JsonSubTypes decorator on the interface will be empty).
• Currently it is assumed that the discriminator property has string type (this could probably be fixed).

I'll appreciate any feedback on this and will be happy to implement more improvements if someone has suggestions.

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 (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)

[bkabrda]

FTR, I think it would be great to have a more systematic solution to this, which we would be able to apply to all languages - but that would probably be something to do in a major release, as it might break a lot of things (or we'd need to take an iterative approach with many small changes and make sure nothing is getting broken). Here, I was basically trying to not break anything for other generators, so all the real functionality changes are limited to the Java client generator.

[bkabrda]

FTR, while trying to apply this to Go, I figured a different approach which is necessary for Go, but could also work for Java, let's consider following spec:

components:
  schemas:
    Animal:
      properties:
        name:
          type: string
      oneOf:
        - $ref: '#/components/schemas/Cat'
        - $ref: '#/components/schemas/Dog'
      discriminator:
        propertyName: type
        mapping:
          cat: '#/components/schemas/Cat'
          dog: '#/components/schemas/Dog'
    Dog:
      properties:
        type:
          type: string
        barksPerMinute:
          type: integer
    Cat:
      properties:
        type:
          type: string
        meowsPerMinute:
          type: integer

Right now, my PR will generate Java code like this:

// the jackson annotations to make deserialization work properly
public interface Animal {
  public string getType();
}

public class Cat implements Animal {
  public string getType() {...};
  // this gets "inherited" from Animal
  public string getName() {...};
  public int getMeowsPerMinute() {...};
  // ...
}

public class Dog implements Animal {
  public string getType() {...};
  // this gets "inherited" from Animal
  public string getName() {...};
  public int getBarksPerMinute() {...};
  // ...
}

Working on Go, one can't deserialize an interface type, so I figured a different approach which might also make sense for Java. Let's consider the same spec which would generate following code:

// the jackson annotations to make deserialization work properly
// CHANGE: make Animal a class
public class Animal {
  // CHANGE: don't make the oneOf choices inherit this attribute
  public string getName() {...};
  // CHANGE: make the oneOf choice a member of this class
  public AnimalInterface getAnimalOneOf() {...};
  public void setAnimalOneOf(AnimalInterface value) {...};
}

// CHANGE: create an AnimalInterface interface
public interface AnimalInterface {
  public string getType();
}

public class Cat implements AnimalInterface {
  public string getType() {...};
  // CHANGE: no attributes "inherited" from Animal
  public int getMeowsPerMinute() {...};
  // ...
}

public class Dog implements AnimalInterface {
  public string getType() {...};
  // CHANGE: no attributes "inherited" from Animal
  public int getBarksPerMinute() {...};
  // ...
}

I'm going to have to go with a similar solution for Go, as it's the only way to do this there. The question is whether we want this for Java as well. The benefit would be that the oneOf choices wouldn't "inherit" all the members of the parent schema (name in the example above). The downside is that it's more complicated and not as comfortable to use. This new solution for Java would likely also make it a bit harder for implementing (de)serialization correctly, but that could likely be manageable (although not 100 % sure). All in all, I'd like to use the current solution and not go for this approach for Java, but I though I'd outline this here anyway. WDYT?

[jimschubert]

@bkabrda I don't know that the last example is correct. oneOf is a validation property which says an animal must "validate against" one of the schemas. This would mean Cat and Dog have their properties and all those defined in Animal. I think being a discriminator as well would mean Animal is abstract (in Java, not sure about Go).

[bkabrda]

@jimschubert I think you can have either of these ways, it's "just" a matter of getting the deserialization logic right (which as I pointed out wouldn't be trivial). I think making Animal an abstract class would make sense in this case.

[jimschubert]

@bkabrda in that last example how would I access Cat#name? I think I'm confused about the intention to break out AnimalInterface. Are your derived classes missing an extends, or do you mean you'd have an option to ignore properties on the discriminated type (Animal in your example)?

[bkabrda]

@jimschubert you wouldn't access it on the Cat object, but on the Animal object. Do note that in the spec outlined above, name is defined on Animal, not on Cat.

The current implementation in this PR makes it so that all properties of Animal are added to Cat (because Animal is interface). This might however not be practical when using Cat outside of the oneOf, as the Cat class is changed by the fact that it's a member of the oneOf. The other solution (which I'd like to use for Go) doesn't have that problem, as properties of Animal would be defined on Animal.

[jimschubert]

@bkabrda thanks for the clarification. I think your understanding of oneOf is close, but missed the point that you're able to move common properties up to the parent schema. Based on your comment above, that target wouldn't match the spec.

See https://json-schema.org/understanding-json-schema/reference/combining.html

[jimschubert]

Also:

This might however not be practical when using Cat outside of the oneOf, as the Cat class is changed by the fact that it's a member of the oneOf.

I think this would be an edge case (e.g. poorly written spec). oneOf would point to validation schemas, not structural schemas. It's unfortunate that the spec doesn't clarify this concept well enough, and that makes the implementation difficult.

[bkabrda]

@jimschubert I'm not sure I understand that, sorry. Does that mean that you think the proposal that I outlined and I want to pursue for Go is actually better and I should do it for Java as well? Or is there a specific issue that you could demonstrate on my implementation that I need to fix? Thanks!

[jimschubert]

@bkabrda sure, according to JSON Schema, the following should be considered the same schema:

(yours, modified to include weight on Dog and Cat)

openapi: 3.0.2
info:
  title: Test
  version: 0.1.0
servers:
  - url: http://localhost
paths:
  /:
    get:
      operationId: sample
      parameters:
        - name: name
          in: query
          required: true
          schema:
            $ref: '#/components/schemas/Animal'
      responses:
        200:
          description: test
          content:
            application/json:
              schema:
                type: array
                items:
                  $ref: '#/components/schemas/Animal'
components:
  schemas:
    Animal:
      properties:
        name:
          type: string
      oneOf:
        - $ref: '#/components/schemas/Cat'
        - $ref: '#/components/schemas/Dog'
      discriminator:
        propertyName: type
        mapping:
          cat: '#/components/schemas/Cat'
          dog: '#/components/schemas/Dog'
    Dog:
      properties:
        type:
          type: string
        weight:
          type: integer
          minimum: 0
          maximum: 100
        barksPerMinute:
          type: integer
    Cat:
      properties:
        type:
          type: string
        weight:
          type: integer
          minimum: 0
          maximum: 100
        meowsPerMinute:
          type: integer

And one that should potentially be valid according to https://json-schema.org/understanding-json-schema/reference/combining.html (extract weight up to Animal):

openapi: 3.0.2
info:
  title: Test
  version: 0.1.0
servers:
  - url: http://localhost
paths:
  /:
    get:
      operationId: sample
      parameters:
        - name: name
          in: query
          required: true
          schema:
            $ref: '#/components/schemas/Animal'
      responses:
        200:
          description: test
          content:
            application/json:
              schema:
                type: array
                items:
                  $ref: '#/components/schemas/Animal'
components:
  schemas:
    Animal:
      type: object
      properties:
        name:
          type: string
        weight:
          type: integer
          minimum: 0
          maximum: 100
      oneOf:
        - $ref: '#/components/schemas/Cat'
        - $ref: '#/components/schemas/Dog'
      discriminator:
        propertyName: type
        mapping:
          cat: '#/components/schemas/Cat'
          dog: '#/components/schemas/Dog'
    Dog:
      properties:
        type:
          type: string
        barksPerMinute:
          type: integer
    Cat:
      properties:
        type:
          type: string
        meowsPerMinute:
          type: integer

According to OpenAPI specs, a Schema Object (Animal above) may include properties and all others including oneOf. OAS 3.0.2 has this definition of oneOf:

Inline or referenced schema MUST be of a Schema Object and not a standard JSON Schema.

Ignoring the "standard JSON Schema" part, this would suggest that Animal is all properties plus one of the $ref properties which itself can be another ref (with additional oneOf validation properties).

[bkabrda]

@jimschubert so right now, the implementation in this PR actually does generate the same code for both of the schemas - in both cases, the weight attribute (and all it's related getters/setters/additional validation) will end up on the Cat and Dog classes.

I was working some more on the Go implementation and I now see that it's actually much easier (and really seems to make more sense as you suggested) to push all the properties defined on Animal to the oneOf classes, so I'm going to do the same there. I will also do some refactoring while working on Go support, as it will use some of the logic that's part of the PR, so I'll move it to DefaultCodegen in the Go PR (all that assuming that this PR gets merged, obviously :)).

All that said, I do believe that this implementation does handle both cases you outlined correctly (== having the same result).

Thanks a lot for having this discussion, I really appreciate it and hope we can move it forward to get this merged eventually.

[jimschubert]

Is this x-oneOfName an extension taken for somewhere else, or one specific to our tool? If it is tooling specific, I think it will eventually need to be x-oneOf-name to match how other vendor extensions are named in https://github.com/OpenAPITools/openapi-generator/wiki/Vendor-Extensions. We'll need to re-evaluate our extensions and naming of these, because there doesn't seem to be an intuitive convention.

I created #4976 to track.

[bkabrda]

It's specific to our tooling. I'll rename to x-oneOf-name and amend this PR.

[bkabrda]

Done.

[jimschubert]

I've done a more in-depth review and I think things look good and the outputs from the two docs I presented above are indeed similar.

I think we can move forward with this PR, but wonder if @catarinacds (author of another PR attempting to add oneOf) or another member of the java technical committee could do a quick pass just to verify?

Pinging the technical committee one more time for review:

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

[bkabrda]

I fixed the naming as requested (x-oneOfName => x-oneOf-name) and also rebased on latest master to fix an import error that was caused by a recent master commit.

[jeff9finger]

I would like to better understand the Java generated classes. Will Animal be an interface with getters/setters for the properties common to both Dog and Cat? But the actual properties are defined and getters/setters implemented on the Dog/Cat classes?

Please provide an example (using the latest implementation) of the generated Java for the above schema, to understand what is generated here for Java.

Thanks

[bkabrda]

So considering this specific example:

openapi: 3.0.2
info:
  title: Test
  version: 0.1.0
servers:
  - url: http://localhost
paths:
  /:
    get:
      operationId: sample
      parameters:
        - name: name
          in: query
          required: true
          schema:
            $ref: '#/components/schemas/Animal'
      responses:
        200:
          description: test
          content:
            application/json:
              schema:
                type: array
                items:
                  $ref: '#/components/schemas/Animal'
components:
  schemas:
    Animal:
      type: object
      properties:
        name:
          type: string
        weight:
          type: integer
          minimum: 0
          maximum: 100
      oneOf:
        - $ref: '#/components/schemas/Cat'
        - $ref: '#/components/schemas/Dog'
      discriminator:
        propertyName: type
        mapping:
          cat: '#/components/schemas/Cat'
          dog: '#/components/schemas/Dog'
    Dog:
      properties:
        type:
          type: string
        barksPerMinute:
          type: integer
    Cat:
      properties:
        type:
          type: string
        meowsPerMinute:
          type: integer

The following code would get generated (I'll try to cut out as much unnecessary code as possible to make it more digestible).

Notes:

• Right now, the only function defined on the Animal interface is derived from the discriminator property. However, it would make perfect sense to also add getters for any properties defined on Animal in the spec, as these will get inherited by all classes implementing the interface - I can certainly work on that (or we can add this later, as it will be a fully backwards compatible change).
• We only define getter for the discriminator attribute, not setter. This is because some of the oneOf members can actually define the discriminator property as readOnly, so it's not guaranteed that the setter could be implemented by all oneOf members. (This would also be the case for any additional properties defined on Animal, so if defined these would also only have getters defined on the Animal interface)

// the jackson annotations are here to make deserialization work properly
public interface Animal {
  public string getType();
}

public class Cat implements Animal {
  public string getType() {...};
  public int getMeowsPerMinute() {...};
  // these get "inherited" from Animal
  public string getName() {...};
  public int getWeight() {...};
  // ...
}

public class Dog implements Animal {
  public string getType() {...};
  public int getBarksPerMinute() {...};
  // these get "inherited" from Animal
  public string getName() {...};
  public int getWeight() {...};
  // ...
}

[jeff9finger]

Thanks. I can see pros and cons of using an interface. What about using an abstract class - at least in Java? This would make the model be more tightly coupled. And this is the behavior that I need for my project.

If both abstract class and an interface work and there is no consensus on one way or the other, perhaps we could add a vendor extension to specify the preferred option? (or create a PR to do that after this one is complete)

Does the serialization work when using an abstract class?

Also, I think that the properties and getters for Animal should be defined in Animal. The sub classes can define the setters if readOnly is false for those properties.

[jimschubert]

I agree with Jeff's proposal.

[bkabrda]

So I just tried implementing this using abstract classes and hit a major problem with this approach: some generated classes already extend another class, most notably if the model has additionalProperties: true, it will extend HashMap. I actually have a usecase where one of the oneOf models has this, so using abstract class is a no-go for me (AFAICS there's no way a class can extend two classes).

The solution using interfaces is more flexible, as Java doesn't limit number of implemented interfaces AFAIK. So given the state of things, would you accept this solution, assuming I added all the relevant getters to the interface? WDYT @jeff9finger?

Thanks!

[jeff9finger]

Thanks for the research. I was wondering if there would be unforeseen issues...

It appears that the use of interface is the only choice. You are correct in that a Java class may only extend one class, and that it may implement multiple interfaces.

LGTM 👍

[bkabrda]

Thanks for the feedback! @jimschubert does that mean that this PR can be merged now?

[jimschubert]

@wing328 could you give this PR a third review when you have time? I think it looks good, and seems to be backward compatible with parcelableModel and serializableModel.

I'd like to merge it for the next release, but I'm not sure if we'd consider the changes to code produced via oneOf as a breaking change, new feature, or bug fix.

[bkabrda]

After some more discussion with Jim about this, we agreed that it would be safer to submit this PR for 4.3.x branch, which I did in #5120. That PR has just been merged, so I'm closing this one. Thanks everyone involved for their comments/reviews!

[axelvuylsteke]

@bkabrda ,

One question regarding this. If your discriminator Value is a string, but behind this string is an enum value, this does not work. Any suggestion for solving this? Example below:

schemas:
    Animal:
      type: object
      properties:
        name:
          type: string
        weight:
          type: integer
          minimum: 0
          maximum: 100
      oneOf:
        - $ref: '#/components/schemas/Cat'
        - $ref: '#/components/schemas/Dog'
      discriminator:
        propertyName: type
        mapping:
          cat: '#/components/schemas/Cat'
          dog: '#/components/schemas/Dog'
    Dog:
      properties:
        type:
          type: string
          enum:
           -dog
        barksPerMinute:
          type: integer
    Cat:
      properties:
        type:
          type: string
          enum:
           - cat 
        meowsPerMinute:
          type: integer