You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Redundant type: object emitted in 2.x preview #2342
Describe the bug
Trying to use 2.0 preview 17, I see the following change in the output document, compared to 1.x:
It seems that whenever OpenApiSchema.Type is set to JsonSchemaType.Null from code, this results in also emitting "type": "object". This is redundant in many cases, for example when allOf is used.
While probably technically still correct, this difference makes it a pain to assess whether the new version produces a similar output document in our test suite. We have about 12,000 tests, many of which compare fragments of the OAS file, failing on changes like this. The fact that "nullable": true moved to the top doesn't help either, but I suspect that will be addressed in a future preview, tracked at #1314.
So I wonder if this change was intentional. And if not, how hard it would be to not emit "type": "object" when the schema contains allOf, oneOf, etc.
I haven't investigated further, but both NSwag and Kiota (stable versions) don't seem to handle this very well, causing compile errors in existing code that uses the generated client.
Instead of the class ErrorLinks, it now generates a class named ErrorObject_links.
We fixed this issue with non-compliance with the specification. However, apparently now we need to go fix Kiota. I have opened an issue microsoft/kiota#6510
I conducted further research on this topic and found the motivation at https://github.com/OAI/OpenAPI-Specification/blob/main/proposals/2019-10-31-Clarify-Nullable.md for why type must be specified with nullable: true . This spec change occurred in a 3.0.x patch version, but has significant effects on implementations. I doubt whether tools like NSwag, Kiota and Swashbuckle special-case their interpretation based on the patch version. From what I've encountered, they all consider nullable: true an expanding assertion today (which the patch no longer allows).
Based on my experiments, Microsoft.OpenApi just adds type: object if only null is present in the Type property. That's way too simplistic. As explained at OAI/OpenAPI-Specification#1900 (comment), there are two interpretations:
Type = JsonSchemaType.Null is meaningless (there is no type), so do not emit nullable: true (breaking change)
Type = JsonSchemaType.Null implicitly means all other types are permitted. I noticed that setting Type = JsonSchemaType.String | JsonSchemaType.Array | JsonSchemaType.Null leaves out both type and nullable in the output, which is correct. Microsoft.OpenApi could interpret Type = JsonSchemaType.Null as: any type (including null) is allowed (when output version is 3.0.x). This is also a breaking change.
For example, the following program (built against preview.17):
This is incorrect, because the type is not an object. This case reflects interpretation 1 mentioned above. But leaving out nullable: true breaks existing code generators.
For example, the program:
usingSystem.Text;usingMicrosoft.OpenApi;usingMicrosoft.OpenApi.Models;usingMicrosoft.OpenApi.Models.Interfaces;usingMicrosoft.OpenApi.Models.References;usingMicrosoft.OpenApi.Writers;vardocument=CreateEntityDocument();varbuilder=newStringBuilder();using(varwriter=newStringWriter(builder)){document.SerializeAs(OpenApiSpecVersion.OpenApi3_0,newOpenApiYamlWriter(writer));}varjson=builder.ToString();Console.WriteLine(json);staticOpenApiDocumentCreateEntityDocument(){varidSchema=newOpenApiSchema{Properties=newDictionary<string,IOpenApiSchema>{["Id"]=newOpenApiSchema{Type=JsonSchemaType.Integer}}};varrefIdSchema=newOpenApiSchemaReference("EntityOfInt");varrequestBodySchema=newOpenApiSchema{Properties=newDictionary<string,IOpenApiSchema>{["Person"]=newOpenApiSchema{Type=JsonSchemaType.Null,// must add JsonSchemaType.Object to make it appear in the outputAllOf=[refIdSchema,newOpenApiSchema{Properties=newDictionary<string,IOpenApiSchema>{["Name"]=newOpenApiSchema{Type=JsonSchemaType.String}}}]}}};returnnewOpenApiDocument{Components=newOpenApiComponents{Schemas=newDictionary<string,IOpenApiSchema>{["EntityOfInt"]=idSchema,["RequestBody"]=requestBodySchema}}};}
Prints the following output:
openapi: 3.0.4info: { }paths: { }components:
schemas:
EntityOfInt:
properties:
Id:
type: integerRequestBody:
properties:
Person:
nullable: true # should not appear, according to the latest 3.0.x spectype: object # should not appear, according to the latest 3.0.x specallOf:
- $ref: '#/components/schemas/EntityOfInt'
- properties:
Name:
type: string
The problem is this. How client code generators behave today is:
When nullable: truedoes not appear in Person, they produce a class with a non-nullablePerson property.
When nullable: truedoes appear in Person, they produce a class with a nullablePerson property.
The type: object in Person is generally ignored (aside from a few bugs in NSwag, where the behavior changes if present, but they won't fix them because the bar for breaking changes is exceptionally high, due to high adoption and low test coverage)
Options:
If Microsoft.OpenApi is fixed to interpret Type = JsonSchemaType.Null as a no-op, this means that OAS document producers need to know the actual type they are generating to get nullable: true emitted. This isn't always easy to determine.
If Microsoft.OpenApi is fixed to interpret Type = JsonSchemaType.Null as "null with any type", then nullable: true will no longer appear in the OAS document, and then client code generators won't produce a nullable property. Because from their point of view, the absence of nullable: true means that nulls are not allowed (which isn't true anymore with the latest patch version of the spec: an empty object without a type and without nullable: true implicitly allows null).
All in all, changing the current behavior in Microsoft.OpenApi to comply with the latest patch of the spec has a considerable ripple effect. The meaning of nullable was poorly defined in the past, and tooling gave it their own interpretation. I wonder if it's wise to change the behavior of Microsoft.OpenApi 2.x at all in this aspect.
During parsing we could add a condition here if object model type field ONLY contains null, we return an error because that essentially means the type value in the serialized document we just parse did NOT contain anything but nullable was set to true, which is invalid according to the 3.0 spec.
This keyword only takes effect if type is explicitly defined within the same Schema Object
Ignored is the important part here, it doesn't mean it's invalid. Even in the scenario where people ONLY have nullable true in a 3.0 description, but no type, which does not mean anything, only means it's meaningless, we should NOT try to add the type object back as a "spec compliance" measure during serialization, because that messes the semantics (all of scalar scenario)
during 3.0 deserialization, garbage in, do not add any control flow that'd error out in case of something that's ignored (but not invalid) according to the specification. (maintain the current code to map nullable to type null)
during 3.0 serialization, change the code to STOP adding the type: object whenever only type null is present in the object model. (effectively only removing line 776 in the serialization code)
Lastly, I don't believe the naming issue you have from kiota in the screenshot is related to this nullable issue. Let's address the nullable issue first, and come back to kiota once the patch is released to see if the behaviour persists.
Thanks. I agree with restoring the old behavior. What I don't understand about the spec change: if nullable only applies to the direct schema, but also we must put all refs within an allOf to allow nullable/summary/description usage, how could it ever work? It's a contradiction. The spec change doesn't seem to consider that client generators produce a class definition in which the nested allOfs get merged.
For backwards compatibility, can the original position where nullable: true is emitted please be kept as-is too? It has moved to the top, which creates numerous diffs that makes our tests on json fragments fail.
Lastly, I don't believe the naming issue you have from kiota in the screenshot is related to this nullable issue. Let's address the nullable issue first, and come back to kiota once the patch is released to see if the behaviour persists.
The spec change doesn't seem to consider that client generators produce a class definition in which the nested allOfs get merged
I agree, I think the main scenario they had in mind when updating this was validation more than IDL/code generation. Apparently, it was "updated" this way because needing the nullable keyword to consider all/any/one ofs became much too complicated.
Anyway, I don't want to debate the spec too much here (beyond us getting a good understanding of what should be done and why).
Is this something you'd like to submit a pull request for provided some guidance?
Activity
bkoelman commentedon Apr 28, 2025
Here's another case:
The code that produces this schema:
bkoelman commentedon Apr 28, 2025
I haven't investigated further, but both NSwag and Kiota (stable versions) don't seem to handle this very well, causing compile errors in existing code that uses the generated client.
Instead of the class
ErrorLinks, it now generates a class namedErrorObject_links.MaggieKimani1 commentedon Apr 29, 2025
Hi Bart Koelman (@bkoelman), thank you for raising this. Let me investigate this and get back to you.
darrelmiller commentedon Apr 29, 2025
Bart Koelman (@bkoelman) Unfortunately the specification says this about the nullable keyword:
https://spec.openapis.org/oas/v3.0.4.html#fixed-fields-20
We fixed this issue with non-compliance with the specification. However, apparently now we need to go fix Kiota. I have opened an issue microsoft/kiota#6510
bkoelman commentedon Apr 30, 2025
Darrel (@darrelmiller) Thanks for looking into this. In that case, I suppose all consumers will need to adapt then.
bkoelman commentedon May 1, 2025
I conducted further research on this topic and found the motivation at https://github.com/OAI/OpenAPI-Specification/blob/main/proposals/2019-10-31-Clarify-Nullable.md for why
typemust be specified withnullable: true. This spec change occurred in a 3.0.x patch version, but has significant effects on implementations. I doubt whether tools like NSwag, Kiota and Swashbuckle special-case their interpretation based on the patch version. From what I've encountered, they all considernullable: truean expanding assertion today (which the patch no longer allows).Based on my experiments,
Microsoft.OpenApijust addstype: objectif onlynullis present in theTypeproperty. That's way too simplistic. As explained at OAI/OpenAPI-Specification#1900 (comment), there are two interpretations:Type = JsonSchemaType.Nullis meaningless (there is no type), so do not emitnullable: true(breaking change)Type = JsonSchemaType.Nullimplicitly means all other types are permitted. I noticed that settingType = JsonSchemaType.String | JsonSchemaType.Array | JsonSchemaType.Nullleaves out bothtypeandnullablein the output, which is correct. Microsoft.OpenApi could interpretType = JsonSchemaType.Nullas: any type (including null) is allowed (when output version is 3.0.x). This is also a breaking change.For example, the following program (built against preview.17):
Prints the following:
This is incorrect, because the type is not an object. This case reflects interpretation 1 mentioned above. But leaving out
nullable: truebreaks existing code generators.For example, the program:
Prints the following output:
The problem is this. How client code generators behave today is:
nullable: truedoes not appear inPerson, they produce a class with a non-nullablePersonproperty.nullable: truedoes appear inPerson, they produce a class with a nullablePersonproperty.type: objectinPersonis generally ignored (aside from a few bugs in NSwag, where the behavior changes if present, but they won't fix them because the bar for breaking changes is exceptionally high, due to high adoption and low test coverage)Options:
Type = JsonSchemaType.Nullas a no-op, this means that OAS document producers need to know the actual type they are generating to getnullable: trueemitted. This isn't always easy to determine.Type = JsonSchemaType.Nullas "null with any type", thennullable: truewill no longer appear in the OAS document, and then client code generators won't produce a nullable property. Because from their point of view, the absence ofnullable: truemeans that nulls are not allowed (which isn't true anymore with the latest patch version of the spec: an empty object without a type and withoutnullable: trueimplicitly allows null).All in all, changing the current behavior in Microsoft.OpenApi to comply with the latest patch of the spec has a considerable ripple effect. The meaning of
nullablewas poorly defined in the past, and tooling gave it their own interpretation. I wonder if it's wise to change the behavior of Microsoft.OpenApi 2.x at all in this aspect.Darrel (@darrelmiller) What are your thoughts?
baywet commentedon May 15, 2025
For reference, here is the code block responsible for this behaviour.
OpenAPI.NET/src/Microsoft.OpenApi/Models/OpenApiSchema.cs
Lines 766 to 786 in e2cd111
baywet commentedon May 15, 2025
During parsing we could add a condition here if object model type field ONLY contains null, we return an error because that essentially means the type value in the serialized document we just parse did NOT contain anything but nullable was set to true, which is invalid according to the 3.0 spec.
OpenAPI.NET/src/Microsoft.OpenApi/Reader/V3/OpenApiSchemaDeserializer.cs
Line 309 in 767d3fb
We can disagree with the spec's usefulness, I'm simply logging where the code is at this point.
baywet commentedon May 15, 2025
From the "updated" 3.0 spec
Ignored is the important part here, it doesn't mean it's invalid. Even in the scenario where people ONLY have nullable true in a 3.0 description, but no type, which does not mean anything, only means it's meaningless, we should NOT try to add the type object back as a "spec compliance" measure during serialization, because that messes the semantics (all of scalar scenario)
Here is the proposal after some discussions with Darrel (@darrelmiller) :
baywet commentedon May 15, 2025
Lastly, I don't believe the naming issue you have from kiota in the screenshot is related to this nullable issue. Let's address the nullable issue first, and come back to kiota once the patch is released to see if the behaviour persists.
bkoelman commentedon May 15, 2025
Thanks. I agree with restoring the old behavior. What I don't understand about the spec change: if nullable only applies to the direct schema, but also we must put all refs within an
allOfto allow nullable/summary/description usage, how could it ever work? It's a contradiction. The spec change doesn't seem to consider that client generators produce a class definition in which the nestedallOfs get merged.For backwards compatibility, can the original position where
nullable: trueis emitted please be kept as-is too? It has moved to the top, which creates numerous diffs that makes our tests on json fragments fail.Agreed.
baywet commentedon May 16, 2025
I agree, I think the main scenario they had in mind when updating this was validation more than IDL/code generation. Apparently, it was "updated" this way because needing the nullable keyword to consider all/any/one ofs became much too complicated.
Anyway, I don't want to debate the spec too much here (beyond us getting a good understanding of what should be done and why).
Is this something you'd like to submit a pull request for provided some guidance?
bkoelman commentedon May 16, 2025
Yes