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

[BUG] Inheritance is broken #2845

Closed
5 tasks done
jonrimmer opened this issue May 8, 2019 · 87 comments · Fixed by #3771 or #3812
Closed
5 tasks done

[BUG] Inheritance is broken #2845

jonrimmer opened this issue May 8, 2019 · 87 comments · Fixed by #3771 or #3812

Comments

@jonrimmer
Copy link
Contributor

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • What's the version of OpenAPI Generator used?
  • Have you search for related issues/PRs?
  • What's the actual output vs expected output?
Description

Basic inheritance, as described in the OpenAPI spec does not work.

Instead of creating a class / interface hierarchy using inheritance to represent the composition, all properties are flattened into the subclass.

openapi-generator version

4.0.0-20190508.072036-627 or master

OpenAPI declaration file content or url

schema.yaml:

openapi: "3.0.1"
info:
  version: 1.0.0
  title: Inheritance test
paths:
  /order:
    get:
      summary: Product order details
      operationId: getOrder
      responses:
        '500':
          description: Successful load
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/ExtendedErrorModel'
components:
  schemas:
    BasicErrorModel:
      type: object
      required:
        - message
        - code
      properties:
        message:
          type: string
        code:
          type: integer
          minimum: 100
          maximum: 600
    ExtendedErrorModel:
      allOf:
        - $ref: '#/components/schemas/BasicErrorModel'
        - type: object
          required:
            - rootCause
          properties:
            rootCause:
              type: string
Expected output

The following class/interface hierarchy: to be generated:

  • BasicErrorModel <- ExtendedErrorModel
Actual output

Three unrelated classes/interfaces:

  • BasicErrorModel
  • ExtendedErrorModel
  • ExtendedErrorModelAllOf
Command line used for generation
java -jar openapi-generator-cli.jar generate -i ./schema.yaml -g typescript-angular -o src

I have also tried with the Java generator, and it creates the same hierarchy, so it is not just a Typescript problem.

Steps to reproduce
  1. Copy the above yaml into schema.yaml locally.
  2. Run the command above.
  3. Examine the generated code.
Suggest a fix

I'm not familiar with the codebase, but it appears that getParentName in ModelUtils.java only considers a class as a viable parent for inheritance if it has a discriminator, but the above inheritance pattern does not use discriminators.

@auto-labeler
Copy link

auto-labeler bot commented May 8, 2019

👍 Thanks for opening this issue!
🏷 I have applied any labels matching special text in your issue.

The team will review the labels and make any necessary changes.

@jonrimmer
Copy link
Contributor Author

As mentioned, this is not just a TypeScript issue, as I can also reproduce it with the Java generator, so the above label is wrong.

@jonrimmer
Copy link
Contributor Author

See also this inheritance example, again taken directly from the OpenAPI docs:

openapi: "3.0.1"
info:
  version: 1.0.0
  title: Inheritance test
paths:
  /pets:
    patch:
      requestBody:
        content:
          application/json:
            schema:
              oneOf:
                - $ref: '#/components/schemas/Cat'
                - $ref: '#/components/schemas/Dog'
              discriminator:
                propertyName: pet_type
      responses:
        '200':
          description: Updated
components:
  schemas:
    Pet:
      type: object
      required:
        - pet_type
      properties:
        pet_type:
          type: string
      discriminator:
        propertyName: pet_type
    Dog:     # "Dog" is a value for the pet_type property (the discriminator value)
      allOf: # Combines the main `Pet` schema with `Dog`-specific properties 
        - $ref: '#/components/schemas/Pet'
        - type: object
          # all other properties specific to a `Dog`
          properties:
            bark:
              type: boolean
            breed:
              type: string
              enum: [Dingo, Husky, Retriever, Shepherd]
    Cat:     # "Cat" is a value for the pet_type property (the discriminator value)
      allOf: # Combines the main `Pet` schema with `Cat`-specific properties 
        - $ref: '#/components/schemas/Pet'
        - type: object
          # all other properties specific to a `Cat`
          properties:
            hunts:
              type: boolean
            age:
              type: integer

The code generated for this spec is broken and unusable.

@jonrimmer
Copy link
Contributor Author

jonrimmer commented May 8, 2019

I am seeing the same issues for the test files in the project. These generate broken code:

Etc.

How on Earth is the project passing all its tests when the output is this state?

EDIT: I see now that the petstore.yaml that is being used to regression test every generator doesn't contain any use of anyOf, allOf, oneOf, discriminators, etc. There's no way you can catch issues with these features, even when they're massively broken, like now.

@macjohnny
Copy link
Member

macjohnny commented May 9, 2019

@jonrimmer you are right, there should be some more tests. would you like to give a hand?
cc @wing328

@macjohnny
Copy link
Member

you could e.g. add an integration-test similar to https://github.com/OpenAPITools/openapi-generator/blob/master/samples/client/petstore/typescript-angular-v6-provided-in-root/pom.xml with a separate yaml-file.

@lacksfish
Copy link

Could this be a related issue? #2580

@wing328
Copy link
Member

wing328 commented May 9, 2019

We just started adding better support for allOf, anyOf, oneOf to different generators and I foresee it will take a while for all generators to support these new features in OAS v3.

If you or anyone wants to help with the implementation of these new features in any generators, let me know and I'll show you some good starting points.

@lacksfish
Copy link

There's already issues referencing multiple path parameters as $ref at the same time to have them merged. allOf, anyOf and oneOf would be nice to have, but the generators get confused with way less complex input already.

Personally, I'm simply looking for the functionality that complies with the OAS spec and outputs the generated code. I'm patient, and in the meantime I will simply try to discard/replace parts of my yaml spec that are not supported or broken by openapi-generator.

Thank you for making this public library and tooling resource and I'm looking forward to the first upcoming 4.0 point release.

@jonrimmer
Copy link
Contributor Author

jonrimmer commented May 9, 2019

@wing328 This is not about enhancing generators, it's an issue of fixing DefaultCodegen.java to create the models correctly in the first place. The changes made to support multiple inheritance and OA3 appear to have made the situation worse, not better. For example, basic Swagger 2.0 allOf aggregation works in the current release version, but is broken in the snapshot, because the code for resolving parents now only understands discriminator-based parent relationships. If 4.0 is released as-is, it is going to break with a lot of specs that previously worked.

There should have been a comprehensive regression test for what aggregation was supported before the attempts to implement OA3 began, to ensure those changes didn't break anything. As things stand, the codebase is already broken WRT inheritance, so the task now is to both implement a comprehensive regression test while simultaneously fixing the default codegen. I'm happy to try to help with this, but it's going to take me a while to understand everything that's happening in DefaultCodegen.java.

@wing328
Copy link
Member

wing328 commented May 9, 2019

I'll retest tomorrow and next week to repeat the problem you reported about allOf

@wing328
Copy link
Member

wing328 commented May 10, 2019

Expected output
The following class/interface hierarchy: to be generated:

BasicErrorModel <- ExtendedErrorModel

@jonrimmer I read your issue report again and notice there may be an incorrect interpretation of allOf (composition vs inheritance).

In the example you provided, BasicErrorModel does not contain discriminator (example) so model composition is used instead

Ref:

@jonrimmer
Copy link
Contributor Author

jonrimmer commented May 10, 2019

@wing328 The discriminator is intended to tell API consumers how they can polymorphically distinguish the type of an incoming object, it doesn't imply that a code generator must flatten any relationship without a discriminator in order to represent the composition.

At the very least, this decision should be left up to the language-specific generator rather than being made prematurely in the default codegen. TypeScript for example has absolutely no problem representing this kind of discriminator-less composition via interfaces, which do not imply any kind of runtime inheritance hierarchy, but allow for types to represent an extension/composition of one or more parent types.

And, indeed, this is exactly how the current release of openapi-generator works!

Run the example I provided through the release available in homebrew, and you will get an interface ExtendedErrorModel that inherits from an interface BasicErrorModel, exactly as you would expect. It is only with the current snapshot that you get these three, unrelated interfaces:

BasicErrorModel
ExtendedErrorModel
ExtendedErrorModelAllOf

This is just not right, and is happening because DefaultCodegen is prematurely deciding to throw away the child-parent relationship expressed by the allOf, perhaps based on the incorrect assumption that Java-style class hierarchies are the only type of extension mechanism in existence?

@lacksfish
Copy link

For the impatient:

To fix my problem, I've used https://github.com/APIDevTools/json-schema-ref-parser (after turning my main yaml spec into json) and de-referenced all the $ref's automatically with the aforementioned tool.

Then, I was able to generate the client code :)

PSA: This is a hotfix and any code generated should be well tested.

@wing328
Copy link
Member

wing328 commented May 15, 2019

allOf takes an array of object definitions that are validated independently but together compose a single object.

While composition offers model extensibility, it does not imply a hierarchy between the models. To support polymorphism, the OpenAPI Specification adds the discriminator field

Ref: https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.2.md#composition-and-inheritance-polymorphism

I'm afraid we've to stick with the definitions defined in the specification.

In your case, why not update the OpenAPI documents to use the discriminator field?

@jonrimmer
Copy link
Contributor Author

@wing328 You don't appear to be even trying to read or understand what I'm writing, so I don't see much point in continuing this.

@Patre
Copy link

Patre commented May 20, 2019

Hi,
I am using OpenAPI generator v3.3.4 to generate a server in Java, Java clients, a python client, and an angular client. With v3.3.4, I have issues with the python client that does not handle correctly polymorphism. I have just tried to migrate to v4.0.0 to fix this python client issue. Migration does fix the issue with the python client, but breaks inheritance in the typescript/angular client.

Here is the relevant part of my OpenAPI spec yaml file:

JobEvent:
      type: object
      description: A job event.
      properties:
        date:
          type: integer
          format: int64
          description: date of this event
        jobId:
          type: string
          description: identifier of the job this event is refering to
        objectType:
          type: string
      required:
      - date
      - objectType
      discriminator:
        propertyName: objectType
    LogEvent:
      allOf:
      - $ref: '#/components/schemas/JobEvent'
      - type: object
        properties:
          message:
            type: string
          level:
            type: string
        required:
        - message
        - level

With generator v3.3.4, I get the following typescript classes generated:

import { JobStatusEvent } from './jobStatusEvent';
import { KpiEvent } from './kpiEvent';
import { LogEvent } from './logEvent';
import { MessageEvent } from './messageEvent';
import { ProgressEvent } from './progressEvent';

/**
 * A job event.
 */
export type JobEvent = MessageEvent | ProgressEvent | KpiEvent | JobStatusEvent | LogEvent;
export interface LogEvent { 
    /**
    * date of this event
    */
    date: number;
    /**
    * identifier of the job this event is refering to
    */
    jobId?: string;
    objectType: 'LogEvent';
    message: string;
    level: string;
}

With generator v4.0.0, I get the following typescript classes:

import { JobStatusEvent } from './jobStatusEvent';
import { KpiEvent } from './kpiEvent';
import { LogEvent } from './logEvent';
import { MessageEvent } from './messageEvent';
import { ProgressEvent } from './progressEvent';


/**
 * A job event.
 */
export type JobEvent = MessageEvent | ProgressEvent | KpiEvent | JobStatusEvent | LogEvent;
import { LogEventAllOf } from './logEventAllOf';


export interface LogEvent { 
    /**
    * date of this event
    */
    date: number;
    /**
    * identifier of the job this event is refering to
    */
    jobId?: string;
    objectType: 'LogEvent';
}
export interface LogEventAllOf { 
    message: string;
    level: string;
}

These new classes do not allow to call logEvent.message for example, because a 'LogEvent' does not have anything to do with a 'LogEventAllOf '.

Do you plan to fix this issue in v4.0.1, which is supposed to be released at the end of the month, if I understood well?
Thanks for these great improvements of inheritance support for generated python clients!
Regards

@macjohnny
Copy link
Member

related: #3058

@andys8
Copy link
Contributor

andys8 commented Jul 4, 2019

Inheritance using discriminator is working with 4.0.0-beta3. Releases after that are broken. Is this recognized as a bug or has the yaml to be adapted to work again?

@ghost
Copy link

ghost commented Jul 9, 2019

Inheritance is broken for Java as well. And it worked in 3.3.4. Currently both extra class "...AllOf" is generated and inheritance is missing. I tried all of the versions starting from 4.0.0-beta1 up to 4.0.2, it is broken everywhere.

@macjohnny
Copy link
Member

@wing328 could you have a look at this one again? I think this is a regression since it used to work correctly

@wing328
Copy link
Member

wing328 commented Jul 10, 2019

@macjohnny thanks for bringing it to my attention. I definitely want to take another look at the issue as I thought discriminator should setup the model inheritance properly but @Patre suggested otherwise in #2845 (comment).

However I'm very busy as usual and I'll try to look into it again after the Open Summit Japan 2019 next week.

If anyone wants to help on the issue, please feel free to submit a PR to start with.

@sohrab-
Copy link

sohrab- commented Aug 2, 2019

Is there a previous version I can rollback to so I can get extends back?

@ghost
Copy link

ghost commented Aug 2, 2019

In our project we had to temporarily switch to another Gradle plugin 'org.hidetake.swagger.generator' which works well with Gradle 5 and allows downgrading open-api generator to 3.3.4(org.openapitools:openapi-generator-cli:3.3.4). And 3.3.4 properly handles inheritance, so if you need it(inheritance i mean), then you definitely have to somehow use this particular version

@mnahkies
Copy link
Contributor

This is blocking us from upgrading from 3.3.4 which is unfortunate as the desire to upgrade was prompted by an unrelated bug that is present in 3.3.4 but fixed in the latest release.

Our use case is we have a common model definition:

    Page:
      properties:
        page:
          type: integer
        pageSize:
          type: integer
        totalPages:
          type: integer
        _links:
          $ref: '#/components/schemas/PageLinks'
      required:
        - page
        - pageSize
        - totalPages
        - _links

And then many models that extend from this, adding an items property. Eg:

    PagedAnEntity:
      allOf:
        - $ref: '#/components/schemas/Page'
        - properties:
            items:
              type: array
              items:
                $ref: '#/components/schemas/AnEntity'
          required:
            - items

None of the operations have a polymorphic return type, they will all return a particular PagedX model. However, we do have some generic code that we use to help construct the different paged entities, which relies on the inheritance of PagedX from Page

Eg:

doSomethingWithPage<T extends Page>(value: T)

Reading earlier in this thread it seems that the recommendation is to add a discriminator property, defined on the supertype, in this case Page. This doesn't seem like a great solution in this case, as suddenly our shared Page definition has to reference every page-able model in our system.

This would be a circular dependency between our separated definition files (or require putting every model into a single definition file) and feels wrong to me.

Is there going to be a restoration of the pre-4.0 functionality where allOf relationships of length 2 create an inheritance structure in the java generator?

Is there some other way I can express this relationship to achieve the desired effect?

@erohana
Copy link

erohana commented Apr 19, 2020

@vgunda which version of openApi and gradle did it work for you ? I'm having the same problem with java and spring

@vgunda
Copy link

vgunda commented Apr 23, 2020

@erohana gradle 5.5 and openapi 4.0.3 worked.
Gradle 6 needed openapi >4.2.1 afaik and with all those inheritance didn't work for me

@dennisschroer
Copy link

dennisschroer commented May 19, 2020

This works perfectly in 4.3.0, but is broken again in 4.3.1

In 4.3.0 inheritance works as expected. In 4.3.1 the subclass does not inherit the superclass, but instead gets all properties from the superclass.

@villqrd
Copy link

villqrd commented May 24, 2020

Any update on this?

I cannot manage to get inheritance when generating client for dart

@InvictusMB
Copy link

typescript-fetch generator is also broken in 4.3.1
Inheritance without a discriminator works perfectly fine in 4.3.0.
In 4.3.1 a subclass gets all the properties of a superclass instead of inheritance and an extra class named SubclassAllOf is generated.
@wing328 Please reopen this

@amakhrov
Copy link
Contributor

amakhrov commented May 26, 2020

@InvictusMB according to the spec, discriminator should be used to denote inheritance relationship between models.
Without a discriminator, models are not extended - hence properties are copied.

Extra AllOf class is a known issue and is tracked separately here (at least, for typescript generators)

@InvictusMB
Copy link

@amakhrov
The part about the spec I understood from the deprecation warning. However this is a breaking change between 4.3.0 and 4.3.1 which should be in a major release instead. And the warning should then say that inheritance without a discriminator has already been removed.

Also 4.3.1 still generates inheritance without a discriminator if there are no own properties in a child subclass.

\\ this will produce inheritance
"Child": {
  "title": "Child",
  "allOf": [
    {"$ref": "#/components/schemas/Parent"},
    {"properties": {}}
  ]
},

@amakhrov
Copy link
Contributor

Also 4.3.1 still generates inheritance without a discriminator

I believe this is the case mentioned in the deprecation warning. This logic is still there (as it says - "will be removed in a future release").

I assume in 4.3.0 some related PRs have introduced undesired behavior, which was then reverted in 4.3.1.

@InvictusMB
Copy link

I have no issues with the spec other than that I fail to understand what should be a discriminator in this case:

schema
{
"definitions": {
  "Category": {
    "title": "Category",
    "additionalProperties": false,
    "properties": {
      "id": {
        "type": "number"
      },
      "tags": {
        "type": "array",
        "items": {
          "type": "string",
          "enum": [
            "foo",
            "bar",
            "baz"
          ]
        }
      }
    }
  },
  "Product": {
    "title": "Product",
    "additionalProperties": false,
    "properties": {
      "id": {
        "type": "number"
      },
      "categoryId": {
        "type": "number"
      }
    }
  }
},
"title": "CategoryWithRelations",
"allOf": [
  {
    "$ref": "#/definitions/Category"
  },
  {
    "properties": {
      "products": {
        "type": "array",
        "items": {
          "$ref": "#/definitions/Product"
        }
      }
    }
  }
]
}

But that's OK, I can live with composition in TypeScript case. The problem I have is that I see no way to distinguish own properties and those pulled from ref in templates. isInherited flag is not set and there is no isMixedIn flag.
In the example above this makes the generator create 2 separate tags enums which leads to Category and CategoryWithRelations being not compatible TypeScript types.

@amakhrov
Copy link
Contributor

2 separate tags enums which leads to Category and CategoryWithRelations being not compatible TypeScript types

Unfortunately it's a limitation of OAS2. Even without inheritance at all - OAS2 doesn't allow to define two fields in the same or different models sharing the same enum, because enum definition is always inlined and cannot be reused via ref.

I also see this issue in my TS project.

An ultimate solution for this enums problem is to migrate to OAS3.

Here is a related discussion about generated enums for TS - you might find it helpful: #6360 (review)

As for inheritance/composition - you could probably fix it at your side by overriding templates like this: #4805

@InvictusMB
Copy link

As for inheritance/composition - you could probably fix it at your side by overriding templates like this: #4805

@amakhrov That worked well, thanks.

I have one more remark, which probably belongs to #5171
In case of allOf composition vars should not be populated with properties from refs. I believe allVars is intended for that. This way a model with allOf can have own properties as vars and there is no need for extra ModelAllOf class. And templates could still generate inheritance if they wish so and there is only one $ref.

@Song2017
Copy link

Song2017 commented Jun 17, 2020

python type is broken in 4.2.3

@jeff9finger
Copy link
Contributor

jeff9finger commented Jun 29, 2020

I am not able to get the following definition to generate java or type script correctly.

swagger: "2.0"
info:
  title: Test Command model generation
  description: Test Command model generation
  version: 1.0.0
host: localhost:8080
schemes:
  - https
definitions:
  Command:
    title: Command
    description: The base object for all command objects.
    x-swagger-router-model: CommandDto
    type: object
    properties: {}
  RealCommand:
    title: RealCommand
    description: The real command.
    x-swagger-router-model: RealCommandDto
    allOf:
      - $ref: '#/definitions/Command'
  ApiError:
    description: The base object for API errors.
    x-swagger-router-model: ApiGeneralException
    type: object
    required:
    - code
    - message
    properties:
      code:
        description: The error code. Usually, it is the HTTP error code.
        type: string
        readOnly: true
      message:
        description: The error message.
        type: string
        readOnly: true
    title: ApiError

parameters:
  b_real_command:
    name: real_command
    in: body
    description: A payload for executing a real command.
    required: true
    schema:
      $ref: '#/definitions/RealCommand'

paths:
  /execute:
    post:
      produces: []
      x-swagger-router-controller: FakeController
      operationId: executeRealCommand
      parameters:
      - name: real_command
        in: body
        description: A payload for executing a real command.
        required: true
        schema:
          $ref: '#/definitions/RealCommand'
      responses:
        '204':
          description: Successful request. No content returned.
        '400':
          description: Bad request.
          schema:
            $ref: '#/definitions/ApiError'
        '404':
          description: Not found.
          schema:
            $ref: '#/definitions/ApiError'
        default:
          description: Unknown error.
          schema:
            $ref: '#/definitions/ApiError'

Have tried with 4.3.1.
In Java, RealCommand is generated as

public class RealCommand extends Command {
...
}

Notice that I did not specify a discriminator in Command. I expect this definition to generate a composition of Command and RealCommand.java and that Command.java would not be generated. Command.java file is not generated, but it is also expected as a base class in RealCommand.java, so this does not compile.

There should not be any inheritance here because there is no discriminator.

@iekdosha
Copy link

I am having similar issues with Spring generator: getting UNKNOWN_BASE_TYPE and "oneOf{Model1}{Model2}.java" is not generated..

@mmondino
Copy link

mmondino commented Aug 4, 2020

I'm facing the same issues reported by @iekdosha

@palufra90
Copy link

I solved the inheritance problem for the typescript/angular generator indenting the property object with the ref object.
From

ExtendedErrorModel:
      allOf:
        - $ref: '#/components/schemas/BasicErrorModel'
        - type: object
          required:
            - rootCause
          properties:
            rootCause:
              type: string

to

ExtendedErrorModel:
      allOf:
        - $ref: '#/components/schemas/BasicErrorModel'
        type: object
        required:
          - rootCause
        properties:
          rootCause:
            type: string

In this way the generated typescript interface is

export interface ExtendedErrorModel extends BaseErrorModel { 
    rootCause: string;
}

@loicdesguin
Copy link

loicdesguin commented Jan 25, 2021

palufra90 : Could you confirm the version of the generator used ? Because I'm still facing the missing "extends" in the interface definition.
Thanks

Loïc

@palufra90
Copy link

@kristof-mattei
Copy link

kristof-mattei commented Feb 10, 2021

@loicdesguin @palufra90 I think some of the indentation is wrong. I got it to work with this:

ExtendedErrorModel:
      allOf:
        - $ref: '#/components/schemas/BasicErrorModel'
        - type: object
          required:
            - rootCause
          properties:
            rootCause:
              type: string

to

ExtendedErrorModel:
      allOf:
        - $ref: '#/components/schemas/BasicErrorModel'
      type: object
      required:
      - rootCause
      properties:
      rootCause:
      type: string

Notice how allOf and type: object have to be at the same level for this to work!

Tested on 4.3.1 with typescript-axios.

@mizolight
Copy link

Hi, is there an update for this please?

@reitsma
Copy link

reitsma commented Apr 6, 2021

@kristof-mattei Thanks for finding this very delicate solution.

@wing328
Copy link
Member

wing328 commented Dec 20, 2022

Hi all, I've filed #14172 to allow using $ref as parent in allOf with a new option called "openapi-normalizer".

Please give it a try as follows:

git clone --depth 1 -b allof-parent-type https://github.com/OpenAPITools/openapi-generator/
cd openapi-generator
mvn clean package -DskipTests -Dmaven.javadoc.skip=true
java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g java -i /Users/williamcheng/Code/openapi-generator7/modules/openapi-generator/src/test/resources/3_0/allOf_extension_parent.yaml -o /tmp/java-okhttp/ --additional-properties hideGenerationTimestamp="true" --openapi-normalizer REF_AS_PARENT_IN_ALLOF=true

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment