Fix nullability handling in OpenApi#67661
Conversation
|
@copilot Run OpenApi tests, and update the tests so that they pass. |
2b704f3 to
bbcca8b
Compare
|
@copilot Run the tests, and update snapshots for any failing tests so that the tests pass. |
336dc7c to
f2556eb
Compare
There was a problem hiding this comment.
Pull request overview
Improves OpenAPI nullability handling so nullable response types (including cases surfaced via ApiExplorer for typed results/value types) are represented using an explicit oneOf nullable wrapper, while ensuring the inner schema passed to the JSON schema exporter is non-nullable.
Changes:
- Update nullable-response detection to rely on value-type nullability metadata and return-parameter nullability (including
Task<T>/ValueTask<T>generic argument nullability). - Generate schemas using the non-nullable underlying type for
Nullable<T>so nullability is expressed viaoneOfwrapper instead of the inner schema matchingnull. - Update unit tests and integration snapshots to expect
oneOf: [ null, <schema/ref> ]and removenullfrom enum value lists.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/OpenApi/src/Extensions/TypeExtensions.cs | Refines ShouldApplyNullableResponseSchema logic for improved nullable response detection. |
| src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs | Unwraps Nullable<T> before exporting the inner schema to avoid inner schemas matching null. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiSchemaService/OpenApiSchemaService.ResponseSchemas.cs | Updates assertions to validate the oneOf nullable wrapper for nullable response schemas. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApiDocumentLocalizationTests.VerifyOpenApiDocumentIsInvariant.verified.txt | Snapshot updates to reflect oneOf nullable wrapper around referenced schemas. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_2/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-pascalcase-nullable-param.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_2/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-pascalcase-nullable-body-direct.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_2/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-camelcase-nullable-param.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_2/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-camelcase-nullable-body-direct.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_1/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-pascalcase-nullable-param.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_1/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-pascalcase-nullable-body-direct.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_1/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-camelcase-nullable-param.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_1/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-camelcase-nullable-body-direct.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper. |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_0/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-pascalcase-nullable-param.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper (OpenAPI 3.0 null encoding). |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_0/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-pascalcase-nullable-body-direct.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper (OpenAPI 3.0 null encoding). |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_0/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-camelcase-nullable-param.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper (OpenAPI 3.0 null encoding). |
| src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/snapshots/OpenApi3_0/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=enum-camelcase-nullable-body-direct.verified.txt | Snapshot updates for nullable enum response representation via oneOf wrapper (OpenAPI 3.0 null encoding). |
| "application/json": { | ||
| "schema": { | ||
| "$ref": "#/components/schemas/CamelCaseStatus" | ||
| "oneOf": [ |
There was a problem hiding this comment.
I do not get it - so per your PR description this is unlikely schema to get, but we now generate it with oneOf? Why isn't output the following?
"oneOf": [
{ "type": "null" },
{ "$ref": "#/components/schemas/CamelCaseStatus" }
]There was a problem hiding this comment.
This document here is OpenAPI 3.0.4. In OpenAPI 3.0, "type": "null" isn't supported. An equivalent schema which is supported by 3.0 is enum: [ null ]. This behavior is coming from Microsoft.OpenApi (and is a new behavior I introduced there in microsoft/OpenAPI.NET#2933).
Note that the part you commented on is the API response part of the schema, for which nullability was determined via ShouldApplyNullableResponseSchema, and it's the case I described where we previously considered the response non-nullable because the method return type Ok<CamelCaseStatus?>.
ShouldApplyNullableResponseSchemaimplementation was not accurate. It previously compared equality ofMethodInfo.ReturnTypeandApiResponseType.Type, and returned false immediately if they were not equal. That means that APIs that use typed results (e.g,Ok<T>) will always be considered non-nullable, becauseOk<T>isn't equal toT. The only "unwrapping" that used to exist is forTask<T>andValueTask<T>. The new implementation first does a quick and accurate check for value types, usingModelMetadata.IsNullableValueType. If it's true, we know for sure we have a nullable response. The tricky part is for reference types, which is still not handled very well in this PR and I'll leave it as a follow-up (it's existing bug anyways).For nullable types, we always create
oneOfnullable wrapper today. That means that the schema itself should never match null. Imagine the following:If the
SOME_SCHEMA_OR_REF_GOES_HEREmatches null. Then theoneOfconstrained isn't satisfies because it expects one and exactly one schema to match. So, when we hand off work to STJ, we always pass it non-nullable type. This also helps with ensuring that we don't end up with two componentized schema of the same name where one wants to be nullable and the other doesn't want to be nullable (the nullable wrapper is always inlined).I expect there could be more changes and improvements needed in this area, so this likely won't be the only PR to touch things around nullability here, but this is a good improvement so far.