-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Comments
👍 Thanks for opening this issue! The team will review the labels and make any necessary changes. |
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. |
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. |
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 |
@jonrimmer you are right, there should be some more tests. would you like to give a hand? |
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. |
Could this be a related issue? #2580 |
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. |
There's already issues referencing multiple path parameters as 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. |
@wing328 This is not about enhancing generators, it's an issue of fixing 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 |
I'll retest tomorrow and next week to repeat the problem you reported about |
@jonrimmer I read your issue report again and notice there may be an incorrect interpretation of In the example you provided, Ref: |
@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
This is just not right, and is happening because |
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 Then, I was able to generate the client code :) PSA: This is a hotfix and any code generated should be well tested. |
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? |
@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. |
Hi, Here is the relevant part of my OpenAPI spec yaml file:
With generator v3.3.4, I get the following typescript classes generated:
With generator v4.0.0, I get the following typescript classes:
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? |
related: #3058 |
Inheritance using discriminator is working with 4.0.0-beta3. Releases after that are broken. Is this recognized as a bug or has the |
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. |
@wing328 could you have a look at this one again? I think this is a regression since it used to work correctly |
@macjohnny thanks for bringing it to my attention. I definitely want to take another look at the issue as I thought 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. |
Is there a previous version I can rollback to so I can get |
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 |
This is blocking us from upgrading from Our use case is we have a common model definition:
And then many models that extend from this, adding an
None of the operations have a polymorphic return type, they will all return a particular Eg:
Reading earlier in this thread it seems that the recommendation is to add a discriminator property, defined on the supertype, in this case 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 Is there some other way I can express this relationship to achieve the desired effect? |
@vgunda which version of openApi and gradle did it work for you ? I'm having the same problem with java and spring |
@erohana gradle 5.5 and openapi 4.0.3 worked. |
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. |
Any update on this? I cannot manage to get inheritance when generating client for dart |
|
@InvictusMB according to the spec, discriminator should be used to denote inheritance relationship between models. Extra |
@amakhrov 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": {}}
]
}, |
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. |
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 |
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 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 |
@amakhrov That worked well, thanks. I have one more remark, which probably belongs to #5171 |
|
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. public class RealCommand extends Command {
...
} Notice that I did not specify a discriminator in Command. I expect this definition to generate a composition of There should not be any inheritance here because there is no discriminator. |
I am having similar issues with Spring generator: getting UNKNOWN_BASE_TYPE and "oneOf{Model1}{Model2}.java" is not generated.. |
I'm facing the same issues reported by @iekdosha |
I solved the inheritance problem for the typescript/angular generator indenting the property object with the ref object.
to
In this way the generated typescript interface is
|
palufra90 : Could you confirm the version of the generator used ? Because I'm still facing the missing "extends" in the interface definition. Loïc |
@loicdesguin |
@loicdesguin @palufra90 I think some of the indentation is wrong. I got it to work with this:
to
Notice how Tested on 4.3.1 with |
Hi, is there an update for this please? |
@kristof-mattei Thanks for finding this very delicate solution. |
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:
|
Bug Report Checklist
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:
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
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
schema.yaml
locally.Suggest a fix
I'm not familiar with the codebase, but it appears that
getParentName
inModelUtils.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.The text was updated successfully, but these errors were encountered: