allOf as schema reference is not allowed

[RicoSuter]

I discovered a problem with my Swagger spec which is generated with NSwag.

The problem applies to parameters and schema properties. It happens if the parameter or property has a "description" (which belongs only to the parameter/property) and needs a reference to another type from the "definitions". The problem is that $ref is only allowed as only property (we also have a "description"). This is why NSwag generates the "description" and the $ref in an "allOf".

See sample:

definitions:
  ContentOwnershipTransferRequest:
    type: object
    additionalProperties: false
    properties:
      contentId:
        type: string
        description: The content id.
      transferUserId:
        type: string
        description: The id of the user to whom the content document has to be transfered to.
  ContentDetail:
    type: object
    description: A content detail.
    additionalProperties: false
    required:
      - contentType
      - entityType
      - trashed
    properties:
      audit:
        description: Audit data with information regarding document creation and modification.
        allOf:
          - $ref: '#/definitions/StoreAudit'

My questions are:

1. Is this "valid" Swagger?
2. If not, how can this problem be solved so that the Swagger spec is valid?
3. Can this be fixed in AutoRest so that the reference is correctly resolved? (I currently get a Key not found in SwaggerModeler.cs, line 130)

Btw: I'm the developer of NSwag.

[fearthecowboy]

That's not valid the way you have it.

do you want the audit property to be a StoreAudit ? Don't use allOf, just reference it:

 properties:
      audit:
        description: Audit data with information regarding document creation and modification.
        $ref: '#/definitions/StoreAudit'  

[RicoSuter]

I'm pretty sure that this is not allowed according to the JSON $ref spec...

[RicoSuter]

See https://swagger.io/docs/specification/using-ref/

Any members other than $ref in a JSON Reference object are ignored.

[fearthecowboy]

Ah, yeah, that's technically not allowed -- the description should be provided by the $ref'd type.

The trouble with that is if I have properties eyecolor and haircolor and they both $ref to color -- the generated code never knows what purpose of the value is since they both will just get color's description.

Autorest is a smidgen lenient on that one, since there isn't a way to describe the use of a type opposed to it's declaration description.

if you want to be 100% conformant, you can't do what you're looking for.

[RicoSuter]

Ok, I see, AutoRest is in the good position to be only a consumer of specs...

But why is

      audit:
        description: Audit data with information regarding document creation and modification.
        allOf:
          - $ref: '#/definitions/StoreAudit'

not allowed/better: It does not violate the rules (as $ref would) and is semantically the same (allOf with one ref == $ref)?

[fearthecowboy]

Hmm. It does work.

I tried it with petstore, and it works just fine.

 "Pet": {
      "required": [
        "name",
        "photoUrls"
      ],
      "properties": {
        "id": {
          "type": "integer",
          "format": "int64",
          "title": "The id of the pet.",
          "description": "A more detailed description of the id of the pet."
        },
        "category": {
          "description": "mooooo ramble ramble ramble ramble ramble ramble ramble ramble ramble",
          "allOf" : [
           {  "$ref": "#/definitions/Category" }
          ]
        },
        "name": {
          "type": "string",
          "example": "doggie"
        },
        "photoUrls": {
          "type": "array",
          "xml": {
            "name": "photoUrl",
            "wrapped": true
          },
          "items": {
            "type": "string"
          }
        },
        "tags": {
          "type": "array",
          "xml": {
            "name": "tag",
            "wrapped": true
          },
          "items": {
            "$ref": "#/definitions/Tag"
          }
        },
        "status": {
          "type": "string",
          "description": "pet status in the store",
          "enum": [
            "available",
            "pending",
            "sold"
          ]
        }
      },
      "title": "A pet",
      "description": "A group of properties representing a pet.",
      "xml": {
        "name": "Pet"
      }
    },

AutoRest does make a subclass PetCategory of class Category ... which is a bit wasteful, but not broken.

[RicoSuter]

Strange, with the Java generator I had exceptions. However I still think you should handle this like a $ref.

[fearthecowboy]

I just tried the same thing with the java generator and it worked fine.

Is your reference correct?

[RicoSuter]

My spec is much more complex with discriminators etc. I'll try to create a reproduction sample tomorrow...

[olydis]

Using allOf works (should also work with Java) since at the end of the day, property definitions are just Schema objects as per spec, so saying allOf instead of using $ref just means that instead of importing another schema directly, you create a type: object (implicit) schema with an allOf. Complicated, but why not :) Yep, at that point we'll need a repro in order to investigate what's why your Swagger doesn't work.

[RicoSuter]

I hope i can look into this tomorrow...

[RicoSuter]

Currently I have to transform the Swagger spec the following way:

https://github.com/Picturepark/Picturepark.SDK.Playground/blob/master/src/AutoRestTransformer/Program.cs

The original spec:

https://github.com/Picturepark/Picturepark.SDK.DotNet/blob/master/swagger/PictureparkSwagger.json

The transformed spec:

https://github.com/Picturepark/Picturepark.SDK.Playground/blob/master/swagger/PictureparkSwaggerAutoRest.json

Generation works with the transformed specs, but I have to manually fix the following Java code:

https://github.com/Picturepark/Picturepark.SDK.Playground#java

[olydis]

@RSuter looking into it. 🙂 You wrote:

After the generation process, the Java classes have to be manually fixed:

• Remove @OverRide

• Add missing imports

  import okhttp3.MediaType;
  import okhttp3.RequestBody;

in TransferClientsImpl.java

Just to clarify, since I'm no expert about our Java generator: Are those fixes side-effects of the Swagger transformations you did? It does not look like it (why would imports be missing or @overrides be added), but I wanted to make sure before tagging our Java guys and throwing them at it. 😉

Now back to the actual allOf problem...

[RicoSuter]

No the fixes are additional problems... and should probably looked at by Java guys (I'm a .NET guy so maybe I also did something wrong, but the missing imports are surely a bug).

[olydis]

I split that issue out to #2665 so it doesn't get messy in here...

[olydis]

Okay, upon further investigation, I unfortunately have to say that some of the issues AutoRest has with the (untransformed) Swagger are kind of by design:
Although Swagger should be all about the wire format, we have made design choices that allow influencing code generation using the Swagger quite directly. One of the examples, which seems to hit here the hardest, is our relationship between allOf and inheritance. So while it is theoretically perfectly valid Swagger to declare your properties with allOf and then a single references instead of just the reference (in fact, those two options are pretty much equivalent as per JSON schema spec as far as I can tell), AutoRest will introduce an inheritance relationship.

This seems to cause even bigger troubles in other places:

• some of your types have an allOf on a type that has no defined properties, but additionalProperties: { ... }. We model that latter part as Dictionary<> (whatever that is in Java 😆 ), which is a decision made many years ago. Unfortunately you can't take an allOf on that (well, we could theoretically make a generated subtype implement IDictionary, but not sure we wanna go down that road). Essentially you have a conflict of the following goals:
  • Make the base type, looked at in isolation, idiomatic and usable (=> dictionary)
  • allOf on a single other type means inheritance (and it kind of has to - imagine you declared an operation to return the base type - at runtime, in C#/Java we gotta be able to now squeeze any type in the inheritance tree into the return type. And no, let's not make the return type object although there was a rich schema at our disposal.)
• you have some allOf on enum types. The story is similar to the one above: the idiomatic thing to do for the base type is, you know, model it as an enum (which in every language is basically a primitive type) 🙂 but then we try to inherit from that type - can't inherit from a primitive type!

So essentially we "solved" the above design conflicts by not generating code in any such case. 😞 I partied on your original Swagger and only ended up with pretty much identical transformations as you did. Sorry that there's no better answer at this point, but looking at your nice transformer, I guess you're at least not blocked by this - please let me know if you encounter anything that you feel like it is something we should implement/fix (like the Java stuff! 😃) to at least get a little closer to Nirvana (where every valid Swagger can be stuck inside AutoRest).

[RicoSuter]

For this project I'm not blocked, correct. But I develop the Swagger toolchain NSwag and I'd like to support as many other tools as possible. This is why these transformations are not really feasible for a greater audience...

In my code generators I treat $ref and allOf (with a single entry) the same. So this means that it is not a problem whether the spec has $refs or allOf refs. Isn't this an option for you too?

1. However the question is whether to use $ref or allOf refs in the Swagger generators. If all tools assume $ref with properties (invalid JSON pointers) then I have no other option as to generate this too...

2. However, you should fix this:
   https://github.com/Picturepark/Picturepark.SDK.Playground/blob/master/src/AutoRestTransformer/Program.cs#L24
   If a schema has no properties it should generate a class with no properties (and not no class as it does currently).

3. And what about this one?https://github.com/Picturepark/Picturepark.SDK.Playground/blob/master/src/AutoRestTransformer/Program.cs#L68
   Is this invalid swagger or is just your Java code gen failing?

[olydis]

Fair points regarding the fixes, will look into/discuss it 🙂 Regarding 2, I think that is one of the odd decisions that were once made (I think not having properties at all is interpreted as the intent to use the actual type object; having properties: {} on the other hand encodes the empty class, which then for some reason was decided to not be generated - and now we have customers depending on that, but maybe --generate-empty-classes=true helps). 3 looks like a bug indeed, will check and summon a fix.