Skip to content

Conversation

captainsafia
Copy link
Member

Part of #58619.

Update to the latest preview of Microsoft.OpenApi ahead of preview3. Changes consumed:

  • Tags are modeled as a set and handled correctly during deserialziation by M.A so we can remove our comparer implementation.
  • Bug fix around serialization of date-based examples was resolved so we can remove the scrubbing in our snapshot tests.
  • OpenApiReaderRegistry was deprecated in favor of registering supported readers on OpenApiReader settings.

A change unrelated to the upgrade:

  • The Microsoft.AspNetCore.Identity APIs are special-cased in our XML resolution logic so that docs for those are automatically resolved.

@Copilot Copilot AI review requested due to automatic review settings March 5, 2025 21:18
@captainsafia captainsafia requested review from a team and wtgodbe as code owners March 5, 2025 21:18
@ghost ghost added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Mar 5, 2025
@captainsafia captainsafia requested a review from mikekistler March 5, 2025 21:18
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Overview

This PR updates the Microsoft.OpenApi dependency to v2.0.0-preview.11 and removes now‑redundant custom implementations. It also removes workarounds for serialization issues and updates reader registration to align with the new API.

  • Updates OpenAPI deserialization by replacing comparer logic with native set handling.
  • Removes snapshot test scrubbing for date-based examples due to an upstream bug fix.
  • Adapts endpoint routing tests to use new reader settings for YAML support.

Reviewed Changes

File Description
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Build.Tests/GenerateAdditionalXmlFilesForOpenApiTests.cs Adds a check for Microsoft.AspNetCore.Identity XML generation.
src/OpenApi/src/Services/OpenApiDocumentService.cs Removes redundant tag comparer usage and adjusts tag collection types to use sets.
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Integration/OpenApiDocumentIntegrationTests.cs Removes date-time scrubbing now that the upstream serialization issue is fixed.
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Extensions/OpenApiEndpointRouteBuilderExtensionsTests.cs Updates reader registration from deprecated methods to OpenApiReaderSettings.
src/OpenApi/src/Services/OpenApiGenerator.cs Updates tag collection handling by moving from lists to sets for unique tags.
src/OpenApi/src/Comparers/OpenApiTagComparer.cs Removes the now‑redundant custom tag comparer.

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

Comments suppressed due to low confidence (2)

src/OpenApi/src/Services/OpenApiDocumentService.cs:349

  • [nitpick] For clarity and consistency, consider explicitly using the constructor name ('new OpenApiTagReference(controllerName, document)') rather than relying on type inference in the collection initializer.
return new HashSet<OpenApiTagReference> { new(controllerName, document) };

src/OpenApi/src/Services/OpenApiGenerator.cs:364

  • [nitpick] Consider explicitly specifying the type when creating a new OpenApiTagReference for consistency and clarity, e.g. 'new OpenApiTagReference(controllerName, document)'.
return new HashSet<OpenApiTagReference>
        {
            new(controllerName, document)
        };

OpenApiReaderRegistry.RegisterReader(OpenApiConstants.Yaml, new OpenApiYamlReader());
var result = await OpenApiDocument.LoadAsync(documentStream, format);
var readerSettings = new OpenApiReaderSettings();
readerSettings.AddYamlReader();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is so much nicer than that static registration.

var outputDirectory = Path.Combine(baseSnapshotsDirectory, version.ToString());
await Verifier.Verify(json)
.UseDirectory(outputDirectory)
.ScrubLinesWithReplace(line => DateTimeRegex().Replace(line, "[datetime]"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this that bug I reported last week?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! It got fixed in preview9 or preview10 I believe.

Condition="'%(ReferencePath.Extension)' == '.dll' AND
Exists('%(ReferencePath.RootDir)%(ReferencePath.Directory)%(ReferencePath.Filename).xml') AND
('%(ReferencePath.ReferenceSourceTarget)' == 'ProjectReference')"
Condition="
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a comment here for what we're doing?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Funny story I have to revert this because of #60783 but I will be sure to add a comment once I've recolved that issue.

description.ActionDescriptor.EndpointMetadata.OfType<IEndpointNameMetadata>().LastOrDefault()?.EndpointName;

private static List<OpenApiTagReference> GetTags(ApiDescription description, OpenApiDocument document)
private static ISet<OpenApiTagReference> GetTags(ApiDescription description, OpenApiDocument document)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extreme nit: Generally for private/internal methods we can return the concrete type instead of an interface, this allows for optimizations at the usage site. I know the return values here are just set on an OpenAPI type, but just a general pattern.

@captainsafia captainsafia enabled auto-merge (squash) March 6, 2025 18:57
@captainsafia captainsafia added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc and removed area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework labels Mar 6, 2025
@captainsafia captainsafia merged commit bd7a372 into main Mar 6, 2025
27 checks passed
@captainsafia captainsafia deleted the cs/moa-preview11 branch March 6, 2025 21:59
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview3 milestone Mar 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-openapi

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants