cyclosproject / ng-openapi-gen

An OpenAPI 3.0 codegen for Angular
MIT License
397 stars 132 forks source link

Support (again) Swashbuckle's way of generating allOf and type: object in the same level #206

Closed viktorvej closed 2 years ago

viktorvej commented 2 years ago

Hi!

After upgrading from 0.17 to 0.20 my models no longer extend their respective base class when using inheritance. The file is getting imported correctly but are not being extended.

In 0.17 it looked like this:

/ tslint:disable / / eslint-disable / import { BaseClassA } from './base-class-a'; export interface ClassB extends BaseClassA { propA?: null | string; propB?: null | string; }

After upgrade to 0.20:

/ tslint:disable / / eslint-disable / import { BaseClassA } from './base-class-a'; export interface ClassB { propA?: null | string; propB?: null | string; }

Why is this happening and how can I fix it?

luisfpg commented 2 years ago

Can you share your schema definitions (yaml / json)?

luisfpg commented 2 years ago

We have #184 which was similar to this, and was fixed in 0.20.0

viktorvej commented 2 years ago

Yeah I had the same problem when going from 0.17 > 0.19, but decided to stay at 0.17 for the time being. But same problem for me with 0.20 and now I would prefer fixing it rather than staying on an older version.

My use case is that all my response models are a separate class with all its properties types being defined as nested classes inside the class of the response model. And this separate case I am also using a base class for the different types of nested classes. Not sure if it maskes sense, but here are the relevant parts from the schema following the example above:

 ...
 "ParentClass+ClassA": {
    "type": "object",
    "allOf": [
      {
        "$ref": "#/components/schemas/ParentClass+BaseClassA"
      }
    ],
    "properties": {
      "propA": {
        "type": "boolean",
        "readOnly": true
      },
      "propB": {
        "type": "string",
        "nullable": true,
        "readOnly": true
      },
    },
    "additionalProperties": false
  },
 "ParentClass+ClassB": {
    "type": "object",
    "allOf": [
      {
        "$ref": "#/components/schemas/ParentClass+BaseClassA"
      }
    ],
    "properties": {
      "propC": {
        "type": "boolean",
        "readOnly": true
      }
    },
    "additionalProperties": false
  },
 "ParentClass+BaseClassA": {
    "type": "object",
    "properties": {
      "basePropA": {
        "type": "integer",
        "format": "int32",
        "readOnly": true
      },
      "basePropB": {
        "type": "string",
        "nullable": true,
        "readOnly": true
      }
    },
    "additionalProperties": false
  },
  "ParentClass": {
    "type": "object",
    "properties": {
      "objectProp": {
        "$ref": "#/components/schemas/ParentClass+BaseClassA"
      }
    },
    "additionalProperties": false
  },
 ...

I hope it's enough to get a read on what might be wrong.

luisfpg commented 2 years ago

Mmm... seems that I've spotted the problem. This is invalid, according to the spec:

"ParentClass+ClassB": {
    "type": "object",
    "allOf": [
      {
        "$ref": "#/components/schemas/ParentClass+BaseClassA"
      }
    ],
    "properties": {
      "propC": {
        "type": "boolean",
        "readOnly": true
      }
    },
    "additionalProperties": false
  }

You cannot have both type:object AND allOf in the same level. The correct way to define it would be:

"ParentClass+ClassB": {
  "allOf": [
    {
      "$ref": "#/components/schemas/ParentClass+BaseClassA",
    },
    {
      "type": "object",
      "properties": {
        "propC": {
          "type": "boolean",
          "readOnly": true
        }
      },
      "additionalProperties": false
    }
  ]
}

Maybe the generator was more lenient before, but that's the correct way... See https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

viktorvej commented 2 years ago

Alright, then I guess I need to find a way to get the schema generated the correct way in the API... are you familiar with C# and their swashbuckle/swagger and maybe can point me in the right direction?

The C# for this example looks like this:

...

public class ParentClass
    {
        public BaseClassA ObjectProp { get; }

        public abstract class BaseClassA
        {
            public int BasePropA { get; }
            public string BasePropB { get; }
        }

        public class ClassA : BaseClassA
        {
            public bool PropA { get; }
            public bool PropB { get; }
        }
        public class ClassB : BaseClassA
        {
            public bool PropC { get; }
        }
}

...

viktorvej commented 2 years ago

In the swashbuckle documentation they are using allOf and type at the same level...

https://github.com/domaindrivendev/Swashbuckle.AspNetCore#inheritance-and-polymorphism

luisfpg commented 2 years ago

Ah, DeJaVu... I had totally forgot about this. This was already raised before: https://github.com/domaindrivendev/Swashbuckle.AspNetCore/issues/1942 It was fixed in a previous generator version, however, with the changes in the generated schema to use type intersection instead of interface extension, this is back. Swashbuckle claims that this is valid, although the OpenAPI specification says otherwise. As such, I see no other way than implementing it (again) in ng-openapi-gen :-/ Disclaimer: I don't use .NET, nor have any experience with it. I'll see when I have some time to work on it. It would be much faster if you (or any other Swashbuckle users) can contribute a PR, it would obviously be much faster to implement... Also, a specific test case is needed to make sure this isn't re-introduced in the future, as Swashbuckle seems the only thing that generates code in that way...

viktorvej commented 2 years ago

Ah, too bad the spec is not more strict and open for interpretation in these kind of cases...

Im afraid I have no knowledge whatsoever in how to implement any of this, im just a simple frontend-developer using these fantastic tools.

I guess I will stick to 0.17 for a while longer and hope this will be solved by you or any other clever man who is experiencing the same problems :)

Meanwhile... thanks for the fast response, and kudos for all your work, it is much appreciated!

ophirKatz commented 2 years ago

Hi @luisfpg . I just created a PR for this issue as you can see. I would love for you to take a look at it :) By the way, just wanted to say thanks a lot for this project, it is truly awesome.

ophirKatz commented 2 years ago

@luisfpg Looks great, thanks for the fix and for the attention