Api: Incorrect types in generated schema.

[gitFurious]

Using User.UserInfoCard as an example, but this problem is widespread.

From what I understand, when using a common definition (through the use of a $ref style reference), you lose the ability to specify any other schema related fields (like a description).

The workaround appears to be to wrap the $ref in an allOf and then specify fields like description. These workarounds all appear to have been given a type of object (when they're not).

Example: User.UserInfoCard.MembershipType is an enum (int) of BungieMembershipType, but the schema (both version two and three) both say that there's an object called membershipType.

https://github.com/Bungie-net/api/blob/d191be164b83651113ff93bb97ad5d34db806984/openapi-2.json#L6830-L6838

https://github.com/Bungie-net/api/blob/d191be164b83651113ff93bb97ad5d34db806984/openapi.json#L4506-L4514

This causes issues with code-generation and model validation.

Invalid type. Expected Object but got Integer. Path 'membershipType'.

Either removing the type from the schema (outside the allOf) or changing the type from object to integer resolves the issue (in this case).

[pylaligand]

I'm having similar issues with these definitions. From what I understand of the spec (note: I'm learning the spec as I go so I'm not 100% sure about what follows), you're not supposed to use allOf in a property declaration but within a definition. So in the example @gitFurious uses, you'd have to do something like:

"membershipType": { 
   "$ref": "#/components/schemas/MySuperMembershipType",
   "description": "Type of the membership." 
 },
. . .
"definitions": {
  "MySuperMembershipType":
    "allOf": [ 
     { 
       "$ref": "#/components/schemas/BungieMembershipType" 
     } 
   ]
}

This would raise another issue though: now membershipType has a different type from BungieMembershipType, whereas in spirit you want them to be the same thing. I've noticed the allOf with one element pattern all over the spec; instead, the following would give exactly what you want:

"membershipType": { 
   "$ref": "#/components/schemas/BungieMembershipType",
 },

No more type mismatch.

[gitFurious]

... instead, the following would give exactly what you want:

"membershipType": { 
  "$ref": "#/components/schemas/BungieMembershipType",
},

No more type mismatch.

Yup, you're right. The only problem with this is that you lose the ability to specify other fields (like description) at the membershipType level.

This means the following is lost:

"description": "Type of the membership."

While this example doesn't show much being lost, there might be example where a outer level description is more informative than the description offered by a generic reference.

Using allOf is perfectly valid to include many properties on a parent object. In this case we just want to include a single property membershipType, but it's valid spec to include many properties without a child object.

(membershipType) shouldn't be given a type of object, because the API doesn't return an object, it returns a property.

Hopefully the generated spec's are given some love 👍

[pylaligand]

Hmm I'm not sure I follow. When using allOf on membershipType this effectively creates a new composite type for that property. So that property is not a BungieMembershipType anymore, it's just a new type that looks a whole lot like one. That makes using resulting bindings really clumsy.

[gitFurious]

Sorry, I messed up. I must of thought the allOf was higher in the hierarchy. You're right, if there was additional properties in membershipType's allOf, you'd be forced to create a object property called membershipType.

[vthornheart-bng]

Indeed, the allOf workaround is legal (and as far as I can tell, the only "legal" way in Swagger to add information such as description in these situations - it's the recommended approach from a variety of sources), but unfortunately it sucks. We do it because indeed there are many situations where we have descriptions on the property itself that provide context which would not make sense to provide on the type itself, and thus the generator I wrote uses this awkward but recommended workaround to provide the documentation that would have otherwise gone missing.

What I'd really like would be for both documentation properties and the $ref property to be allowed to co-exist in the Swagger spec. But barring that, we're left with this awkwardness.

But at the least, what I can do is fix the type issue mentioned originally. I can't fix the awkwardness of Swagger, and I would prefer to have the awkwardness err in favor of exposing documentation over having more ideal generated clients, which is why I intend to fix the type issue rather than remove the allOfs. I just have not had time to do either yet, so right now it's still open unfortunately. I'll let you all know once I finally have time to loop back around to this.

[xlxCLUxlx]

Hi @vthornheart-bng,

I was wondering if you could give an example of how you will plan on fixing the type issue originally mentioned? I came across this bug as this is one of my last major hurdles in getting my custom generator in the swagger-codegen project to work. I have been able to make use of your extensions for enumerations etc; however, it seems that the swagger-parser that creates the swagger object that is used by the swagger-codegen actually drops the allOf from the JSON and you can no longer get at this. I checked the JSON and properties for the model in a debugging session of swagger-codegen. Since you are limited by Swagger Types and can't really use the $ref effectively when using the allOf I was hoping maybe you could provide just a quick snippet example for future planning since it seems like you have an idea on how you may address this.

I would agree I would rather have the documentation over more ideal generated clients as generating the client is more a convenience and not something we're constantly regenerating (at least not me except for getting my generator process down). One thought I had (_full disclosure this is my first outing with the OpenAPI spec and Swagger_) is maybe leaving things as is and creating an additional extension called x-ref-type or something that could be used to update the type in the generator should each developer choose to make use of it. Since I am already going to the trouble of creating my own generator for my client for enumerations, making use of manifest tables, etc. I could just use this x-ref-type extension to update my types where needed in the postModelProcessing.

This is obviously a use case that benefits me and I am not sure what you or other developers think about this approach. Like I said this is my first go at using OpenAPI and Swagger and not sure if this is even a good approach that I am suggesting.

Any insights or thoughts are appreciated by all!

[vthornheart-bng]

I was only planning on fixing the "type" property. So if I'm understanding correctly, it sounds like there's a second issue on top of that where swagger-parser isn't honoring the use of $ref inside of the allOf property? If so, that is a bummer: they really need to fix that, because according to their spec:

"Alternatively, any time a Schema Object can be used, a Reference Object can be used in its place. This allows referencing definitions instead of defining them inline."

You should file a bug in the Swagger Codegen project when you get the chance:

https://github.com/swagger-api/swagger-codegen/issues

As it is, I'm definitely not opposed to adding an extension property: though the generator wouldn't recognize it:

A) it sounds like the generator's not recognizing their own recommended solution anyways

B) the extension method would be able to imply the true relationship desired, that we can only awkwardly work around (and apparently not even, in practice) with the current Swagger spec

If we do create this x-ref-type/x-ref-property-type/whatever we end up naming it, people will just have to know that this is a reference to a schema elsewhere in the spec. If there is no strong opposition to the concept, I'm game for doing that.

[xlxCLUxlx]

Yeah, I am going to reach out to them to see if they have any information on what I have seen. I have actually seen a number of bugs around the allOf for Swagger along with enhancement request. If almost seems like they really need to enhance the type in my opinion to first check the Swagger defined types and if a matching type is not found then check the types that are user defined. If still no type is found then log an error. This seems like a more natural work flow to me.

I actually like your x-ref-property-type naming. Hopefully other developers can weigh in on their thoughts. I may go ahead and create another fork and update the spec and check-in for your review down the road (I know there are other more pressing issues). This is like the awkwardness with enumerations where you had to create a new extension which if you don't mind adding a few lines of code to an existing generator for your language of choice is not so bad to deal with but kind of annoying how they implemented. I guess this is the nature of projects though that designed to be generic for multiple use cases. You always run into some gotcha's but I have still been pretty pleased with Swagger and it has made the client creation process overall pretty easy.

One other gotcha I ran into is around the format property that is related to the type property. For uint32Swagger will just place an int for me (C#). I added dictionary to store some format information in my generator code and in this case use a uint so I don't have issues with the hashes.

[vthornheart-bng]

Excellent - yeah, if you can convince them to make improvements, I imagine you'll have the thanks of many people in API communities - ours and others! I sadly don't have time to press the issue with them, but if someone can step up to the plate for that it'll make all our lives better in the long run. I'll gladly upvote/thumbs up any request that gets made and chime in with comments as time permits. Rock it!

[xlxCLUxlx]

I have submitted Issue 6776 to start dialog with the Swagger team in case anyone is interested.

[xlxCLUxlx]

Hi @vthornheart-bng,

I had a little time this weekend to dive back into this and debug swagger a little more to see what is specifically happening. Through debugging I have found that the swagger-core RefProperty class does support the use of description and title when used in conjunction with $ref. Since this is the case I went ahead and removed all of the allOf statements from the openapi-2.json resulting in the following:

Swagger Spec 2.0
swagger-codegen 2.2.3

_Before Removal:_

"membershipType": {
          "description": "Type of the membership.",
          "type": "object",
          "allOf": [
            {
              "$ref": "#/definitions/BungieMembershipType"
            }
          ]
        }

_After Removal:_

"membershipType": {
          "description": "Type of the membership.",
          "$ref": "#/definitions/BungieMembershipType"
        }

This allows for client creation with the correct type rather than being left with just an untyped object while at the same time being able to retain the description. In a few instances though I did have drop the x-destiny-component-type-dependency because a RefProperty only supports description and title. However, this seemed like a fair trade off since the component type is specifically called out in the description alerting the user what to use.

As mentioned in my previous post allOf will always be ignored when trying to use this in conjunction with a property. I took a look at both the PropertyDeserializer and PropertyBuilder classes in swagger-core and swagger-models and why it will never be detected (i.e. is dropped on deserialization). After debugging more this kind of makes sense to me. The idea being that allOf is a way to support polymorphic behavior via a JSON description. You would do this via the objects and then assign the objects to the properties not the other way around. The reason this would become tricky trying to use allOf in conjunction with a property for the definitions is you would have to start cross referencing all properties to make sure you are not duplicating objects that are being created via the property (if that makes sense).

In any event I created pull request 286 for this issue to hopefully improve quality of life for those using the spec for creating clients. I did not update the spec 3.0 file though in case this pull request is not taken into consideration since I am currently only using spec 2.0. If this is something that can be implemented I would be happy to update spec 3.0 at that point.

Cheers!