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

SchemaType.Swagger2 does not generate Swagger2 complaint schema #732

Closed
fizxmike opened this issue Jul 3, 2018 · 8 comments
Closed

SchemaType.Swagger2 does not generate Swagger2 complaint schema #732

fizxmike opened this issue Jul 3, 2018 · 8 comments

Comments

@fizxmike
Copy link

fizxmike commented Jul 3, 2018

Specifically, the behavior of allOf.

When generating a schema for a derived class, NJsonSchema will define both a list of properties AND a list of allOf (reference to base class). However, Swagger2 will ignore the list of properties! When using swagger tooling/codegen, the result is multiple clones of the base class, all properties of the derived classes are lost, only the name of the class is different.

I actually fault swagger for breaking from JSON Schema spec w/r/t polymorphism. But NJsonSchema advertises this SchemaType.Swagger2 switch, which should work.

When I try to flatten the dependency tree, I get multiple definition of property errors very similar to this open issue.

The code I am trying to spec out is legacy, therefore I cannot declare inheritance. Although I'm not sure declaring inheritance is the issue, since the schema generated in the linked example is likewise not swagger2 compliant.

I have a feeling this is somehow related to the $ref issue in the OpenAPI Spec itself.

@fizxmike
Copy link
Author

fizxmike commented Jul 3, 2018

I guess I'm expecting the SchemaType.Swagger2 to move the 'properties' item from the root of the schema, into the list of 'allOf', like this example (pardon the yaml). Or, for FlattenInheritanceHierarchy to work (in my use case).

@fizxmike fizxmike changed the title SchemaType.Swagger2 does not generate Swagger2 complient schema SchemaType.Swagger2 does not generate Swagger2 complaint schema Jul 3, 2018
@RicoSuter
Copy link
Owner

So swagger/openapi has to treat allof differently then json schema?

@fizxmike
Copy link
Author

fizxmike commented Jul 3, 2018

I'm not sure exactly... I just dove into this issue today after creating POC of my API toolchain over the past few weeks. I came back to patch holes and found this. Any insights would be appreciated.

Quote from my first link above (re: swagger spec, allOf behavior):
"Swagger allows combining and extending model definitions using the allOf property of JSON Schema, in effect offering model composition. allOf takes in an array of object definitions that are validated independently but together compose a single object."

@RicoSuter
Copy link
Owner

RicoSuter commented Jul 4, 2018

I think i began working on this in a separate branch but it involves a lot of work and is really complicated because all schema properties have to be resolved when refs are in place... certainly on my todo list

@RicoSuter
Copy link
Owner

See: #733

@fizxmike
Copy link
Author

fizxmike commented Jul 5, 2018

I think the $refs are fine. One way (for me) to do it is to post-process the NJsonSchema object before writing out to file. Basically, if a schema has an "allOf" property, create a new object in that list and move the properties into it.

Maybe something like...

Before

    "Dog": {
      "properties": {
        "Bar": {
          "type": [
            "null",
            "string"
          ]
        }
      },
      "allOf": [
        {
          "$ref": "#/definitions/Animal"
        }
      ]
    },

After

    "Dog": {
      "allOf": [
        {
          "$ref": "#/definitions/Animal"
        },
        {
          "properties": {
            "Bar": {
              "type": [
                "null",
                "string"
              ]
            }
          }
        }
      ]
    },

After digging a bit more, this seems the correct way to do Extension/Composition in JSON Schema. So, my initial thought that swagger was not obeying spec might be wrong. Thoughts?

@RicoSuter
Copy link
Owner

Yes, you can implement this with a schema visitor: https://github.com/RSuter/NJsonSchema/tree/master/src/NJsonSchema/Visitors

Also see my PR, working on this - main problem is that this will complicate the C# models extremely as it will introduce many Actual* properties (which resolve refs) which have to be used everywhere... (ie. generators)

@RicoSuter
Copy link
Owner

RicoSuter commented Aug 6, 2018

PR: #733

But this one needs more testing as it might break lots of stuff :-)

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

2 participants