Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Populate XML doc comments into OpenAPI document #60326

Merged
merged 13 commits into from
Feb 14, 2025
Merged

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Feb 12, 2025

Resolves #39927.

This PR adds support for populating OpenAPI documents with XML doc comments discovered on method, class, and member definitions.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Feb 12, 2025
@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 Feb 12, 2025
Copy link
Member

@BrennanConroy BrennanConroy left a comment

Choose a reason for hiding this comment

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

Cache needs to be keyed differently, it can conflict with the same method names but different arguments.

/// </summary>
internal sealed class AssemblyTypeSymbolsVisitor(IAssemblySymbol assemblySymbol, CancellationToken cancellation) : SymbolVisitor
{
private readonly CancellationToken _cancellationToken = cancellation;
Copy link
Member

Choose a reason for hiding this comment

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

nit: when do you decide to drop a primary constructor arg into an explicit field on a class?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I didn't commit a fix to this. I think this might have been caught in a refactor. I wasn't previously passing in the CancellationToken via the primary constructor.

@DeagleGross DeagleGross requested a review from Copilot February 14, 2025 13:21
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.

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

Files not reviewed (15)
  • AspNetCore.sln: Language not supported
  • eng/Dependencies.props: Language not supported
  • eng/Versions.props: Language not supported
  • src/OpenApi/OpenApi.slnf: Language not supported
  • src/OpenApi/build/Microsoft.AspNetCore.OpenApi.targets: Language not supported
  • src/OpenApi/gen/Microsoft.AspNetCore.OpenApi.SourceGenerators.csproj: Language not supported
  • src/OpenApi/gen/Helpers/AddOpenApiOverloadVariant.cs: Evaluated as low risk
  • src/OpenApi/gen/Helpers/AddOpenApiInvocation.cs: Evaluated as low risk
  • src/OpenApi/gen/XmlComments/MemberKey.cs: Evaluated as low risk
  • src/OpenApi/gen/XmlCommentGenerator.cs: Evaluated as low risk
  • src/OpenApi/gen/XmlComments/XmlComment.cs: Evaluated as low risk
  • src/OpenApi/gen/XmlCommentGenerator.Parser.cs: Evaluated as low risk
  • src/OpenApi/gen/Helpers/AssemblyTypeSymbolsVisitor.cs: Evaluated as low risk
  • src/OpenApi/sample/Controllers/XmlController.cs: Evaluated as low risk
  • src/OpenApi/gen/Helpers/ISymbolExtensions.cs: Evaluated as low risk
Comments suppressed due to low confidence (3)

src/OpenApi/gen/XmlComments/XmlParameterComment.cs:16

  • [nitpick] The method name 'GetXmlParameterListComment' is ambiguous. It should be renamed to 'GetXmlParameterComments' for clarity.
public static List<XmlParameterComment> GetXmlParameterListComment(XPathNavigator navigator, string xpath)

src/OpenApi/gen/Helpers/StringExtensions.cs:17

  • The documentation for the 'TrimEachLine' method should clarify that the 'indent' parameter is applied to all lines except the first one.
/// <summary>

src/OpenApi/gen/Helpers/AddOpenApiInvocationComparer.cs:22

  • Ensure that the Equals method correctly compares all relevant properties of AddOpenApiInvocation. If Variant is the only property that determines equality, the current implementation is fine. Otherwise, update the method to include other properties.
return x.Variant.Equals(y.Variant);

@captainsafia
Copy link
Member Author

Cache needs to be keyed differently, it can conflict with the same method names but different arguments.

I updated the chace to more fully handle method signatures by adding a MemberKey as the method cache that accounts for the:

  • Declaring type name
  • Property/method name
  • Method return type
  • Parmater types of method

While working on this, I briefly considered just using the DocumentationId as the format for the cache key. Documentation IDs have a format like T:Foo or M:Foo.Bar and already do the work of disambiguating between different method overloads. However, they are a language/compiler construct and it's non-trivial to create a DocumentationId at runtime when we do the key resolution there. There's an issue filed about adding support for DocumentationId for the runtime but it hasn't gained traction: dotnet/roslyn#61838.

As far this PR goes, it appears that these changes might've broken the template tests. Taking a peek at that now. I can address any further feedback on 6633e28 in follow-up commits while I work to get the tests passing.

@captainsafia
Copy link
Member Author

As far this PR goes, it appears that these changes might've broken the template tests. Taking a peek at that now. I can address any further feedback on 6633e28 in follow-up commits while I work to get the tests passing.

OK, I figured out the source of the issue. The auth related template tests were failing because the MSBuild was to eager about pulling the XML files for the auth dependencies (and their transitive dependencies 😬). I've removed the logic that tries to discover XML files from package references for now.

I'm hoping that auto-populating from the current assmebly + project references should be enough for the common cases where people have an API project and a DTOs project for shared types for example.

@captainsafia captainsafia merged commit 0b520c3 into main Feb 14, 2025
27 checks passed
@captainsafia captainsafia deleted the safia/xml-docs-gen branch February 14, 2025 19:39
@dotnet-policy-service dotnet-policy-service bot added this to the 10.0-preview2 milestone Feb 14, 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.

Support XML-based OpenAPI docs for minimal and controller-based APIs with Microsoft.AspNetCore.OpenApi
4 participants