Upgrade to Microsoft.OpenApi 2.x and support OpenAPI v3.1 #59480

captainsafia · 2024-12-14T00:30:46Z

Contributes towards #58619.

This PR updates our Microsoft.OpenApi dependency to the new major version (v2) to support Open API v3.1.

Changes in this PR include:

Using the new JsonSchemaType enum to represent schema types
Removing IOpenApiAny in favor of JsonNode per the change in the Microsoft.OpenApi library
Updates to code for parsing OpenAPI documents to use the new APIs

The biggest functional change in this PR is the removal of the OpenApiSchemaStore class in favor of the new OpenApiSchemaReference type. The OpenApiSchemaStore was our previous strategy of maintaining resolved references to OpenAPI schemas and their reference IDs. The OpenApiSchemaReference type supports this behavior by allowing you to hold a reference to a schema and support resolving that schema in one.

This change means that we can store schemas directly in the OpenApiDocument.Components.Schemas property and provide resolved references to them for use in transformers without additional overhead on our part.

This does temporarily introduce a regression into the logic we had for disambiguating duplicate schemas. Moving forward, I'd like to use this as a forcing function to implement the comparers in the Microsoft.OpenApi library (see microsoft/OpenAPI.NET#414) and avoid the overhead of having to maintain the comparers in our own codebase (removed here).

Another note: this change just lays the groundwork for this upgrade. There's still some more work required to support actual JSON schema features, like changing the way that nullability is modeled to use the type property or taking advantage of the const keyword. These changes will happen in follow up PRs.

eng/Versions.props

Copilot reviewed 38 out of 53 changed files in this pull request and generated no comments.

Files not reviewed (15)

eng/Versions.props: Language not supported
src/OpenApi/src/Comparers/OpenApiReferenceComparer.cs: Evaluated as low risk
src/OpenApi/src/Extensions/OpenApiSchemaExtensions.cs: Evaluated as low risk
src/OpenApi/perf/Microbenchmarks/OpenApiSchemaComparerBenchmark.cs: Evaluated as low risk
src/OpenApi/src/Comparers/OpenApiSchemaComparer.cs: Evaluated as low risk
src/OpenApi/src/Comparers/ComparerHelpers.cs: Evaluated as low risk
src/OpenApi/src/Comparers/OpenApiAnyComparer.cs: Evaluated as low risk
src/OpenApi/src/Comparers/OpenApiDiscriminatorComparer.cs: Evaluated as low risk
src/OpenApi/src/Comparers/OpenApiExternalDocsComparer.cs: Evaluated as low risk
src/OpenApi/src/Comparers/OpenApiXmlComparer.cs: Evaluated as low risk
src/OpenApi/src/Services/OpenApiOptions.cs: Evaluated as low risk
src/OpenApi/sample/Transformers/AddExternalDocsTransformer.cs: Evaluated as low risk
src/OpenApi/perf/Microbenchmarks/TransformersBenchmark.cs: Evaluated as low risk
src/OpenApi/src/Extensions/OpenApiServiceCollectionExtensions.cs: Evaluated as low risk
src/OpenApi/src/Schemas/OpenApiJsonSchema.Helpers.cs: Evaluated as low risk

captainsafia · 2024-12-14T00:40:27Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

        };
+        document.Paths = await GetOpenApiPathsAsync(document, scopedServiceProvider, operationTransformers, schemaTransformers, cancellationToken);
+        document.Tags = document.Tags?.Distinct(OpenApiTagComparer.Instance).ToList();


I filed microsoft/OpenAPI.NET#1983 to eventually make this a lot smoother.

captainsafia · 2024-12-14T00:41:51Z

src/OpenApi/src/Services/OpenApiDocumentService.cs

+        // resolution of references in the document.
+        document.Workspace ??= new();
+        document.Workspace.RegisterComponents(document);
+        if (document.Components?.Schemas is not null)


We do this to ensure that the schemas are always ordered lexicographically by reference ID in the document. This was previously done in the OpenApiSchemaReferenceTransformer but since that's been removed in favor of using OpenApiSchemaReference directly, we need to do the sorting somewhere.

captainsafia · 2024-12-14T00:42:47Z

src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/CreateSchemaReferenceIdTests.cs

@@ -199,7 +200,7 @@ await VerifyOpenApiDocument(builder, options, document =>
        });
    }

-    [Fact]
+    [ConditionalFact(Skip = "https://github.com/dotnet/aspnetcore/issues/58619")]
    public async Task HandlesDuplicateSchemaReferenceIdsGeneratedByOverload()


This is one of the tests we have to temporarily skip while we sort out how schema deduping looks in the new model.

captainsafia · 2024-12-14T00:44:03Z

...Core.OpenApi.Tests/Services/OpenApiDocumentService/OpenApiDocumentServiceTests.Parameters.cs

@@ -51,13 +51,13 @@ public async Task GetOpenApiParameters_RouteParametersAreAlwaysRequired()
        // Assert
        await VerifyOpenApiDocument(builder, document =>
        {
-            var pathParameter = Assert.Single(document.Paths["/api/todos/{id}"].Operations[OperationType.Get].Parameters);
+            var pathParameter = Assert.Single(document.Paths["/api/todos/{id}"].Operations[OperationType.Get].Parameters!);


Microsoft.OpenApi marks the parameter list property as nullable in the new model so we use a null suppression here and elsewhere to quiet nullability warnings.

I used Assert.NotNull in other places so it's not the most consistent but 🤷🏽 .

@darrelmiller Based on our discussions of Nullable vs Optional, I would not expect Parameters to be Nullable, since null is not an allowed value for the Parameters field of an Operation object. But perhaps I am still confused about this.

@mikekistler For my edification, are you expected to produce an empty list in the case where an operation has no parameters? Is it possible to omit the parameters field altogether?

captainsafia · 2024-12-14T02:10:37Z

##[error].packages\system.runtime.compilerservices.unsafe\6.0.0\buildTransitive\netcoreapp2.0\System.Runtime.CompilerServices.Unsafe.targets(4,5): error : System.Runtime.CompilerServices.Unsafe doesn't support netcoreapp2.1. Consider updating your TargetFramework to netcoreapp3.1 or later.

Not sure why we are getting these warnings now. They are coming from the ApiDescription.Server and adjacent dependencies. We're planning on changing this in .NET 10 anyways (see #58353), so I'll go ahead and remove the out-of-support runtime target in this PR.

.../Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiEndpointRouteBuilderExtensionsTests.cs

...NetCore.OpenApi.Tests/Services/OpenApiSchemaService/OpenApiSchemaService.ParameterSchemas.cs

martincostello · 2024-12-17T14:08:52Z

src/OpenApi/src/Extensions/JsonNodeSchemaExtensions.cs

@@ -86,7 +86,7 @@ internal static void ApplyValidationAttributes(this JsonNode schema, IEnumerable
        {
            if (attribute is Base64StringAttribute)
            {
-                schema[OpenApiSchemaKeywords.TypeKeyword] = "string";
+                schema[OpenApiSchemaKeywords.TypeKeyword] = JsonSchemaType.String.ToString();


Is it right that this is changing from string to String (and for all these from lower to sentence case)? If it is, maybe using nameof() would be better to avoid the ToString() call at runtime (unless the compiler is smart enough to do that automatically?) when it's a constant value.

Is it right that this is changing from string to String (and for all these from lower to sentence case)?

Depends on the definition of write. I primarily did this to make the values more resilient to being read by Enum.Parse<JsonSchemaType> without having case-insensitivity configured.

We do set ignoreCase: true everywhere we parse these values and users should never interact with the opaque version of the schema but this feels...safer? 😓

Is it right that this is changing from string to String (and for all these from lower to sentence case)? If it is, maybe using nameof() would be better to avoid the ToString() call at runtime (unless the compiler is smart enough to do that automatically?) when it's a constant value.

The compiler doesn't map ToString to nameof. nameof would work for simple types but things might get funky when we support nullability via the type argument (JsonSchemaType.String | JsonSchemaType.Null) so ToString felt like the best approach to rely on consistently.

martincostello · 2024-12-17T14:10:36Z

src/OpenApi/src/Extensions/OpenApiDocumentExtensions.cs

+        document.Components.Schemas ??= new Dictionary<string, OpenApiSchema>();
+        document.Components.Schemas[schemaId] = schema;
+        document.Workspace ??= new();
+        var location = document.BaseUri + "/components/schemas/" + schemaId;


Is there any risk here of BaseUri ending with a / and messing up the concatenation?

The BaseUri is currently implicitly configured by OpenAPI.NET (ref). The default value ends with a GUID and no trailing slash. They don't expose a setter for the property and I believe it's primarily there to have a base document URL for resolving schemas so I don't tink the risk is likely.

src/OpenApi/src/Services/OpenApiDocumentService.cs

src/OpenApi/src/Services/Schemas/OpenApiSchemaService.cs

…a/openapi-v2

captainsafia · 2024-12-19T17:20:53Z

@BrennanConroy Can I get your eye on this? I think you have the most context at the moment.

@DeagleGross Some of this code might be new to you but the delta here might be a good opportunity to share some info about the space here.

captainsafia · 2025-01-02T16:57:02Z

/azp run

azure-pipelines · 2025-01-02T16:57:22Z

Azure Pipelines successfully started running 3 pipeline(s).

…a/openapi-v2

mikekistler · 2025-01-11T00:34:25Z

src/OpenApi/src/Schemas/OpenApiJsonSchema.Helpers.cs

                        }
                        else
                        {
-                            schema.Type = type;
+                            schema.Type = Enum.Parse<JsonSchemaType>(type, ignoreCase: true);


Why ignoreCase here and elsewhere when parsing type? Where does this value of type come from that it might not not be the correct case (and that would be okay)?

When System.Text.Json gives us the schema type, it is serialized as a string in all lowercase (object, null, array, etc.) The JsonSchemaType enum provided by the OpenAPI uses title casing. Enum.Parse is case-sensitive by default so object won't map to Object unless we use this flag.

The JsonSchemaType enum provided by the OpenAPI uses title casing.

Isn't that a bug?

No, as most enums use title-case (or I guess Pascal scale) for the type names. STJ maps them to an all lower-case identifier here) which is why we get the inconsistency in casing between what Enum.ToString() while generate and what the schema will produce.

mikekistler · 2025-01-11T13:23:29Z

.../OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=controllers.verified.txt

-    {
-      "name": "Test"
-    }
+    "Test"


This does not look right. The top-level Tags field should be an array of TagObjects, not strings.

Yep, this is a known bug that I filed on the library over at microsoft/OpenAPI.NET#1991. It was fixed but the version with the fix contains a different bug microsoft/OpenAPI.NET#2055 so I am sticking with the devil I know for now. 😅

.../OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=controllers.verified.txt

mikekistler · 2025-01-11T13:31:32Z

...snapshots/OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=v2.verified.txt

@@ -28,12 +28,12 @@
                    "type": "string",
                    "externalDocs": {
                      "description": "Documentation for this OpenAPI schema",
-                      "url": "https://example.com/api/docs/schemas/string"
+                      "url": "https://example.com/api/docs/schemas/String"


This change looks odd. I suppose the URL is made up so there is no right or wrong -- but usually URLs follow conventions, like "all lowercase" (as the previous URL did) or "camel cased segments". Here there is a mix of lowercase and Pascal case, which is ... odd.

Yep, this is result of the same reason in #59480 (comment). Because we use JsonSchemaType.String.ToString() the default casing produced is titlecase. We can fix this by just using the literal "string" directly.

mikekistler

This looks really good!

I left a few comments (sorry I didn't lump them into a review -- I didn't expect to make so many), but the only one that is a real problem is the way tag objects are not rendered correctly.

…a/openapi-v2

mikekistler

Looks good! 👍

I think this can merge as is. Any remaining issues are minor and can be cleaned up in a follow-on PR.

Upgrade to Microsoft.OpenApi 2.x and support OpenAPI v3.1

20925c6

captainsafia requested review from a team and wtgodbe as code owners December 14, 2024 00:30

dotnet-issue-labeler bot added the area-web-frameworks *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Dec 14, 2024

wtgodbe reviewed Dec 14, 2024

View reviewed changes

eng/Versions.props Outdated Show resolved Hide resolved

captainsafia requested a review from Copilot December 14, 2024 00:35

Copilot AI reviewed Dec 14, 2024

View reviewed changes

captainsafia commented Dec 14, 2024

View reviewed changes

captainsafia added 3 commits December 16, 2024 09:55

Update usage in GetDocumentInsider

435e341

Fix packaging and AoT tests

61e318f

Update AoT test project exemptions

df3fe64

martincostello reviewed Dec 17, 2024

View reviewed changes

captainsafia mentioned this pull request Dec 17, 2024

OpenApi generation OpenApiSchemaComparer fails with null values for Properties, AllOf, or AnyOf #59507

Open

1 task

captainsafia added 2 commits December 18, 2024 17:14

Address feedback

ca25dfb

Merge branch 'main' of https://github.com/dotnet/aspnetcore into safi…

d347295

…a/openapi-v2

dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Dec 26, 2024

dotnet-policy-service bot removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 2, 2025

dotnet-policy-service bot added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 9, 2025

Merge branch 'main' of https://github.com/dotnet/aspnetcore into safi…

c76d336

…a/openapi-v2

build-analysis bot mentioned this pull request Jan 10, 2025

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

Try ignoring whitespace

76122ef

captainsafia force-pushed the safia/openapi-v2 branch from 500f8f3 to 76122ef Compare January 10, 2025 21:34

captainsafia removed the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jan 10, 2025

mikekistler reviewed Jan 11, 2025

View reviewed changes

.../OpenApiDocumentIntegrationTests.VerifyOpenApiDocument_documentName=controllers.verified.txt Show resolved Hide resolved

mikekistler reviewed Jan 11, 2025

View reviewed changes

Update to 2.0.0-preview.6

56fb84a

captainsafia force-pushed the safia/openapi-v2 branch from b2c5885 to 56fb84a Compare January 13, 2025 18:17

Merge branch 'main' of https://github.com/dotnet/aspnetcore into safi…

a07cc72

…a/openapi-v2

mikekistler approved these changes Jan 14, 2025

View reviewed changes

captainsafia merged commit d4880ed into main Jan 14, 2025
27 checks passed

captainsafia deleted the safia/openapi-v2 branch January 14, 2025 15:57

dotnet-policy-service bot added this to the 10.0-preview1 milestone Jan 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Microsoft.OpenApi 2.x and support OpenAPI v3.1 #59480

Upgrade to Microsoft.OpenApi 2.x and support OpenAPI v3.1 #59480

captainsafia commented Dec 14, 2024 •

edited

Loading

captainsafia Dec 14, 2024

captainsafia Dec 14, 2024

captainsafia Dec 14, 2024

captainsafia Dec 14, 2024

mikekistler Jan 11, 2025

captainsafia Jan 13, 2025

captainsafia commented Dec 14, 2024

martincostello Dec 17, 2024

captainsafia Dec 19, 2024

martincostello Dec 17, 2024

captainsafia Dec 19, 2024

captainsafia commented Dec 19, 2024

captainsafia commented Jan 2, 2025

azure-pipelines bot commented Jan 2, 2025

mikekistler Jan 11, 2025

captainsafia Jan 13, 2025

mikekistler Jan 13, 2025

captainsafia Jan 13, 2025

mikekistler Jan 11, 2025

captainsafia Jan 13, 2025

mikekistler Jan 11, 2025

captainsafia Jan 13, 2025

mikekistler left a comment

mikekistler left a comment

Upgrade to Microsoft.OpenApi 2.x and support OpenAPI v3.1 #59480

Upgrade to Microsoft.OpenApi 2.x and support OpenAPI v3.1 #59480

Conversation

captainsafia commented Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captainsafia commented Dec 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

captainsafia commented Dec 19, 2024

captainsafia commented Jan 2, 2025

azure-pipelines bot commented Jan 2, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikekistler left a comment

Choose a reason for hiding this comment

mikekistler left a comment

Choose a reason for hiding this comment

captainsafia commented Dec 14, 2024 •

edited

Loading