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 🤷🏽 .

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.

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 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

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

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

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

Are you sure you want to change the base?

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

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 Dec 14, 2024 •

edited

Loading