search

OpenAPITools / openapi-generator

OpenAPI Generator allows generation of API client libraries (SDK generation), server stubs, documentation and configuration automatically given an OpenAPI Spec (v2, v3)
https://openapi-generator.tech
Apache License 2.0
21.37k stars 6.46k forks source link

[BUG] [Java] inheritance is generated without discriminator field #5097

Closed vitaliysapounov closed 4 years ago

vitaliysapounov commented 4 years ago
Description

We see that inheritance (instead of composition) is generated where, as we think, it should not.

Namely, we have the following model file:

openapi: "3.0.2"

info:
  version: "1.0.0"
  title: Test

  # We only care about schemas, but this prevents codegen error: "attribute paths is missing"
paths:
  /dummy:
    get:
      responses:
        '200':
          description: OK

components:
  schemas:

    PreventInheritance:
      description: We declare dummy dependency on this empty class to prevent inheritance

    Base:
      properties:
        base1:
          type: integer
        base2:
          type: string

    Inheritance:
      allOf:
        - $ref: "#/components/schemas/Base"
        - properties:
            # doses
            Inheritance1:
              type: integer
            Inheritance2:
              type: string

    NoInheritance:
      allOf:
        - $ref: "#/components/schemas/Base"
        - $ref: "#/components/schemas/PreventInheritance"
        - properties:
            NoInheritance1:
              type: integer
            NoInheritance2:
              type: string

The following two classes are of interest:

  1. Basically, Inheritance class is declared as having all the properties of Base and some additional. Note that there is no discriminator defined. Thus, since there is no discriminator, our understanding that composition should be used, not inheritance. In the generated Java class we see that inheritance is generated, however:
public class Inheritance extends Base implements Serializable {

OBSERVED: inheritance is generated EXPECTED: composition should be used, as there is no discriminator field (?)

  1. To prevent generating of inheritance, we used a hack: in NoInheritance class we add not only Base but also empty PreventInheritance. It seems that in this case, the generator chooses to use composition (why?):
public class NoInheritance implements Serializable {

OBSERVED: inheritance is not generated EXPECTED: ? (we are confused what should be used, composition or inheritance, the spec is unclear in that regard)

openapi-generator version

Gradle plugin org.openapitools:openapi-generator-gradle-plugin:4.2.2 (latest as of this writing)

OpenAPI declaration file content or url

Please see above

Command line used for generation

./gradlew build

Steps to reproduce
  1. Unpack the attached Gradle project
  2. Run ./gradlew build
Related issues/PRs
Suggest a fix

We are not sure how/why inheritance vs composition should be selected, as per the spec. But, the current behavior described above seems to be very confusing (the generator seems to choose inheritance in one case and composition in another case, for no clear reason). test_openapi_inheritance.zip

jimschubert commented 4 years ago

@vitaliysapounov it looks like the confusion may come from our early implementations of OAS3 allOf and our cross-compatibility with OAS2 spec documents.

Can you verify the existence of something like this output when you generate?

[main] WARN  o.o.codegen.utils.ModelUtils - [deprecated] inheritance without use of 'discriminator.propertyName' is deprecated and will be removed in a future release. Generating model for null

This comes from here: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/java/org/openapitools/codegen/utils/ModelUtils.java#L981

See #3771 for some background and more details.

I'm not saying the current implementation is "correct", but you can see that a single $ref in allOf is currently treated as though it implies inheritance. I'm not sure when we intend for the removal, but I'll tag this as 4.3.0 since this is a "break change with fallback" release (fallback here being updating spec or pinning to 4.2.3).

cc @wing328 I saw your feedback in the linked PR, did you have a goal release for changing this functionality? If we're targeting 5.0, can we skip the confusing behavior with a configuration option in the interim?

vitaliysapounov commented 4 years ago

@jimschubert

Yes, we see the warning you mentioned ("inheritance without use of 'discriminator.propertyName' is deprecated and will be removed in a future release"). Here is the full Gradle build output:

17:45:44: Executing task 'build'...

> Task :openApiGenerate
'host' (OAS 2.0) or 'servers' (OAS 3.0) not defined in the spec. Default to [http://localhost] for server URL [http://localhost/]
'host' (OAS 2.0) or 'servers' (OAS 3.0) not defined in the spec. Default to [http://localhost] for server URL [http://localhost/]
[deprecated] inheritance without use of 'discriminator.propertyName' is deprecated and will be removed in a future release. Generating model for null
More than one inline schema specified in allOf:. Only the first one is recognized. All others are ignored.
'host' (OAS 2.0) or 'servers' (OAS 3.0) not defined in the spec. Default to [http://localhost] for server URL [http://localhost/]
Successfully generated code to task ':openApiGenerate' property 'outputDir'

> Task :compileJava
> Task :processResources UP-TO-DATE
> Task :classes
> Task :jar UP-TO-DATE
> Task :assemble UP-TO-DATE
> Task :compileTestJava NO-SOURCE
> Task :processTestResources NO-SOURCE
> Task :testClasses UP-TO-DATE
> Task :test NO-SOURCE
> Task :check UP-TO-DATE
> Task :build UP-TO-DATE

Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.
Use '--warning-mode all' to show the individual deprecation warnings.
See https://docs.gradle.org/6.1/userguide/command_line_interface.html#sec:command_line_warnings

BUILD SUCCESSFUL in 20s
4 actionable tasks: 2 executed, 2 up-to-date
17:46:07: Task execution finished 'build'.
wing328 commented 4 years ago

I'll look into this issue this weekend. Stay tuned.

wing328 commented 4 years ago

It's bug and here is the workaround by adding "type: object" to the last schema definition in allOf:

components:
  schemas:
    Base:
      properties:
        base1:
          type: integer
        base2:
          type: string

    Inheritance:
      allOf:
        - $ref: "#/components/schemas/Base"
        - type: object
          properties:
            # doses
            Inheritance1:
              type: integer
            Inheritance2:
              type: string

    NoInheritance:
      allOf:
        - $ref: "#/components/schemas/Base"
        - type: object
          properties:
            NoInheritance1:
              type: integer
            NoInheritance2:
              type: string

and the output models no longer have inheritance generated.

I'll see if I can hunt down the bug and file a PR this weekend. Please use the workaround before we come up with a permanent fix.

pvdbosch commented 4 years ago

I find inheritance without descriminator rather useful and would prefer it to be kept. I stumbled on this issue because I didn't get inheritance in generated code (I was using "type: object").

I've encountered two use cases where it is useful:

An example:

responses:
  '400':
    schema:
      $ref: '#/definitions/InvalidParamProblem'
  '403':
    schema:
      $ref: '#/definitions/AuthorizationProblem'

InvalidParamProblem:
  allOf:
    - $ref: './problem.yaml#/definitions/Problem'
    - type: object
      properties:
        invalidParams:
          type: array
          items:
            $ref: '#/definitions/InvalidParam'

  AuthorizationProblem:
    allOf:
    - $ref: './problem.yaml#/definitions/Problem'
    - type: object
      properties:
        requiredScopes:
          type: array
          items:
            type: string

Referenced problem.yaml schema: 

  Problem:
    description: A Problem Details object (RFC 7807)
    type: object
    properties:
      type:
        type: string
        format: uri
        default: about:blank
      title:
        type: string
      status:
        type: integer
        format: int32
        minimum: 400
        maximum: 600
        exclusiveMaximum: true
      detail:
        type: string
      instance:
        type: string
        format: uri

Only a single Problem subtype is specified iper response code, so I don't need polymorphism. I also can't use discriminator here at the parent type declaration because it's in another file than the subtypes, and I'd still want to write Java code that uses the parent type Problem.

On another note, there's some talk on an OAS issue about deprecating the discriminator functionality entirely: https://github.com/OAI/OpenAPI-Specification/issues/2143

wolfdale commented 4 years ago

@wing328 I tried the workaround that you mentioned by adding "type: object" but output models still have inheritance generated instead of composition. Currently $ref in allOf is treated as it implies inheritance even if there are no discriminator field in schema file.

wing328 commented 4 years ago

@wolfdale there's a bug (regression) in 4.3.0 release. I've filed and merged a PR fix it in the latest master.

Can you give it a try with the latest master or snapshot version?

If it's still an issue, I will reopen this issue.

nhoughto commented 4 years ago

Being able to trigger inheritance instead of composition when polymorphism isn't needed seems like a useful feature, with this changed does that mean there is now no longer a way to achieve it?