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] No more inheritance for components generated by version 5.0.0 #8495

Closed
antonio-petricca opened this issue Jan 21, 2021 · 21 comments
Closed

Comments

@antonio-petricca
Copy link
Contributor

antonio-petricca commented Jan 21, 2021

Description

No schema inheritance.

openapi-generator version

5.0.0

OpenAPI declaration file content or url
schemas:
  UserDTO:
      type: object
      properties:
        id:
          type: string
          minLength: 1
          maxLength: 20

  QueryUserResponseDTO:
      type: object
      allOf:
        - $ref: '#/components/schemas/UserDTO'
Generation Details

The above YAML produces:

public class QueryUserResponseDTO   {
   private @Valid String id;
}

Instead as for the 4.3.1:

public class QueryUserResponseDTO extends UserDTO   {
}
Suggest a fix

Does exist a configuration parameter to enforce the DTO inheritance generation? Or it is a bug?

@antonio-petricca antonio-petricca changed the title [BUG] No more inheritence for components generated by version 5.0.0 [BUG] No more inheritance for components generated by version 5.0.0 Jan 21, 2021
@Msurrow
Copy link

Msurrow commented Jan 22, 2021

Just to confirm. I updated from 4.3.1 -> 5.0.0 and I'm seeing the same issue (with dotnet-core generator) and AllOf inheritance.
For us, this makes vers 5.0.0 unusable.

If any help, here is a git diff of 4.3.1 generated class and 5.0.0 generated class which shows the issue:

  • superclass gone form class signature
  • base construtor not called
  • superclass properties added to subclass
namespace FOOBAR.Model
 {
@@ -29,7 +29,7 @@ namespace FOOBAR.Model
     /// ProductBankLoan
     /// </summary>
     [DataContract(Name = "ProductBankLoan")]
-    public partial class ProductBankLoan : Product, IEquatable<ProductBankLoan>
+    public partial class ProductBankLoan : IEquatable<ProductBankLoan>, IValidatableObject
     {
         /// <summary>
         /// Initializes a new instance of the <see cref="ProductBankLoan" /> class.
@@ -38,9 +38,12 @@ namespace FOOBAR.Model
         /// <param name="productID">productID.</param>
         /// <param name="debtors">debtors.</param>
         /// <param name="collaterals">collaterals.</param>
-        public ProductBankLoan(double balance = default(double), int productID = default(int), List<Debtor> debtors = default(List<Debtor>), List<Collateral> collaterals = default(List<Collateral>)) : base(productID, debtors, collaterals)
+        public ProductBankLoan(double balance = default(double), int productID = default(int), List<Debtor> debtors = default(List<Debtor>), List<Collateral> collaterals = default(List<Collateral>))
         {
             this.Balance = balance;
+            this.ProductID = productID;
+            this.Debtors = debtors;
+            this.Collaterals = collaterals;
         }

@antonio-petricca
Copy link
Contributor Author

I am going to revert to 4.3.1. :(

@Msurrow
Copy link

Msurrow commented Jan 25, 2021

@antonio-petricca I'm getting this from the log when running the generator:

[main] INFO  o.o.codegen.utils.ModelUtils - [deprecated] inheritance without use of 'discriminator.propertyName' has been deprecated in the 5.x release. Composed schema name: null. Title: null
[main] INFO  o.o.codegen.utils.ModelUtils - [deprecated] inheritance without use of 'discriminator.propertyName' has been deprecated in the 5.x release. Composed schema name: null. Title: null

I havn't had time to dive into it fully yet (we've reverted to 4.3.1 for now) but from what I can gather it seems that my swagger.json doesnt have the discriminator field on the supertype. However I'm using asp.net swashbuckle to generate the swagger.json/.yaml which is setup to OAS3. So what I havn't figured out yet is if the problem really is the yaml not being generated correctly by swashbuckle (i.e. missing the discriminator field). I cant seem to get it to generate the discriminator field. (yet - working on it when I have time)

@felschr
Copy link

felschr commented Jan 27, 2021

This happened to me after upgrading from 5.0.0-beta3 to 5.0.0.
The generated code using the typescript-fetch & dotnet generators is missing the inheritance.
Our openapi schema hasn't changed.

UPDATE:
Like @Msurrow we're using Swasbuckle and we disabled the oneOf & discriminator generation because we were previously hitting some other issues with the resulting unions that openapi-generator is creating (#7367).
Using allOf inheritance with a discriminator instead doesn't seem to be currently supported by Swashbuckle, see domaindrivendev/Swashbuckle.AspNetCore/issues/1973.

@adsanche
Copy link

adsanche commented Feb 10, 2021

I have the same problem from 5.x.x.

Actually, the following warning doesn't make sense :

[INFO] [deprecated] inheritance without use of 'discriminator.propertyName' has been deprecated in the 5.x release. Composed schema name: null. Title: null

As I have a propertyName placed on the discriminator on the parent object :

DeliveryDateApi:
      description: 'Delivery date information.'
      oneOf:
        - $ref: '#/components/schemas/RangeDeliveryDateApi'
        - $ref: '#/components/schemas/FixedDeliveryDateApi'
      discriminator:
        propertyName: type
        mapping:
          RANGE: '#/components/schemas/RangeDeliveryDateApi'
          FIXED: '#/components/schemas/FixedDeliveryDateApi'

Are there any update on this that could avoid rollbacking to 4.3.1?

@antonio-petricca
Copy link
Contributor Author

I reverted to 4.3.1.

@tamershahin
Copy link

I have the same problems with kotlin-spring generator.
is this independent from the generator?

@Blackclaws
Copy link
Contributor

This might be a bug in how ambiguousness of inheritance is being determined. I'm on it

@tamershahin
Copy link

This might be a bug in how ambiguousness of inheritance is being determined. I'm on it

I don't know if it's related to this bug, but I documented what worked for me here:
#8687

@Blackclaws
Copy link
Contributor

So right now the issue is the following:

The spec itself says that allOf by itself does not imply a hierarchic relationship between the components. Therefore even if there is only a single reference in allOf this means that generating an inheritance relationship between them is not per se wrong (as the spec does not forbid it) but it also goes further than the spec and infers something that some people might not want to have.

I've opened an issue on the OpenAPI Spec itself to maybe get some clarification into the spec as to whether treating a single allOf ref as inheritance might make sense.

For now the solution is to simply add a discriminator property to the base object in order to get inheritance.

@Msurrow
Copy link

Msurrow commented Mar 12, 2021

For now the solution is to simply add a discriminator property to the base object in order to get inheritance.

@Blackclaws
Its some time since I've looked at this, but for me part of the problem is that the spec generated by Swashbuckle doesn't include a discriminator on the base object.

You can see a code example in my post above, and a snippet of the generated spec is shown below. Perhaps this is the wrong place for this question (but you mentioned discriminators), but am I doing something wrong here?


      "Product": {
        "type": "object",
        "properties": {
          "productId": {
            "type": "integer",
            "format": "int32"
          },
          "debtors": {
            "type": "array",
            "items": {
              "$ref": "#/components/schemas/Debtor"
            },
            "nullable": true
          },
          "collaterals": {
            "type": "array",
            "items": {
              "$ref": "#/components/schemas/Collateral"
            },
            "nullable": true
          }
        },
        "additionalProperties": false
      },
      "ProductBankLoan": {
        "type": "object",
        "allOf": [
          {
            "$ref": "#/components/schemas/Product"
          }
        ],
        "properties": {
          "balance": {
            "type": "number",
            "format": "double"
          }
        },
        "additionalProperties": false
      }

This is using the configuraton:

Services.AddSwaggerGen(c =>
            {
                // Add discrimitators to swagger generator to handle inheritance types e.g. Product
                c.UseAllOfForInheritance();
              
                c.SwaggerDoc("v1", new OpenApiInfo { Title = "FOOBARApi", Version = "v1" });
            });

@Blackclaws
Copy link
Contributor

You need to actually add a discriminator to your base class and then add the annotation [SwaggerDiscriminator("discriminatorPropertyName")] as well. That has worked for me.

@marabunta62
Copy link

I am going to revert to 4.3.1. :(

I should do the same.
Impossible to generate with 5.X.X "com.fasterxml.jackson.core.JsonParseException: Unrecognized token 'openapi': was expecting .... truncated XXX chars"
Inspect with cli validate my spec bring me here

@Blackclaws
Copy link
Contributor

The problem is that the behavior of adding inheritance to schema composition by allOf without a discriminator was actually non-compliant regarding the spec.

If you add a discriminator everything should work as expected. See also the linked issue in the spec repository. If you want this sort of behavior then please go ahead an lobby for general code generation hints as part of the spec.

@valkum
Copy link

valkum commented Jun 2, 2021

We hit the same issue with model composition as described here: https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

@rgoers
Copy link

rgoers commented Jun 26, 2021

The problem is that the base class can't always know all the classes that will extend it. In my case, #9756 it isn't even necessary as the base class isn't used for Serialization/Deserialization. But I need the base class for methods that operate on the fields it provides. In my environment we have a "BasePage" object for paged requests. All the services that need paging extend that. BasePage can't possibly know all the services that will support paging as it is in a common library that the services import. This change has made it impossible to support this.

@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

@endreszilagyi1
Copy link

Hi @wing328,

we tried it out with your suggestion, however didn't worked.
We are getting the following error:
Exception in thread "main" java.lang.NullPointerException: Cannot invoke "java.util.List.iterator()" because the return value of "io.swagger.v3.oas.models.media.Schema.getAllOf()" is null
at org.openapitools.codegen.OpenAPINormalizer.normalizeOneOf(OpenAPINormalizer.java:328)
at org.openapitools.codegen.OpenAPINormalizer.normalizeSchema(OpenAPINormalizer.java:276)
at org.openapitools.codegen.OpenAPINormalizer.normalizeContent(OpenAPINormalizer.java:154)
at org.openapitools.codegen.OpenAPINormalizer.normalizeRequestBody(OpenAPINormalizer.java:180)
at org.openapitools.codegen.OpenAPINormalizer.normalizePaths(OpenAPINormalizer.java:130)
at org.openapitools.codegen.OpenAPINormalizer.normalize(OpenAPINormalizer.java:101)
at org.openapitools.codegen.DefaultGenerator.configureGeneratorProperties(DefaultGenerator.java:261)
at org.openapitools.codegen.DefaultGenerator.generate(DefaultGenerator.java:907)
at org.openapitools.codegen.cmd.Generate.execute(Generate.java:473)
at org.openapitools.codegen.cmd.OpenApiGeneratorCommand.run(OpenApiGeneratorCommand.java:32)
at org.openapitools.codegen.OpenAPIGenerator.main(OpenAPIGenerator.java:66)

This is the command I'm using:
java -jar modules/openapi-generator-cli/target/openapi-generator-cli.jar generate -g java -i swagger.json -o .\generated --additional-properties hideGenerationTimestamp="true" --openapi-normalizer REF_AS_PARENT_IN_ALLOF=true

When executing the same command but without --openapi-normalizer REF_AS_PARENT_IN_ALLOF=true works fine.
Could you please check if we are doing something wrong or it is a bug?

I'm using windws 10 with JDK 19 x64 and maven 3.8.7.

Thank in advance for you help.

@wing328
Copy link
Member

wing328 commented Mar 23, 2023

can you share the spec to repeat the issue? hard to troubleshoot without the spec.

@wing328
Copy link
Member

wing328 commented Apr 15, 2023

@wing328 wing328 closed this as completed Apr 15, 2023
@wing328
Copy link
Member

wing328 commented Apr 15, 2023

@endreszilagyi1 I've merged #15224 with better null check. Hopefully that should address the issue you reported.

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

No branches or pull requests