-
Notifications
You must be signed in to change notification settings - Fork 10.5k
OpenAPI: Fix duplicate xml documentation ids for Generic properties and references #64404
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
base: main
Are you sure you want to change the base?
Conversation
…nd internal references
| return comments; | ||
| } | ||
|
|
||
| internal static IEnumerable<(string, XmlComment?)> ParseCommentsReference( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If someone has a better idea for this, let me know. The main difference compared to ParseCommentsApplicationAssembly is that it only allows "accessible" types
|
Thanks for your PR, @@desjoerd. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes duplicate XML documentation IDs in OpenAPI source generation by addressing two specific issues:
- Generic type properties with different type arguments (e.g.,
GenericFoo<string>vsGenericFoo<int>) were generating duplicate documentation IDs - Internal classes with the same namespace in different referenced assemblies were causing duplicate documentation IDs
Key Changes
- Separated comment parsing logic into application assembly vs reference assembly methods
- Modified
AssemblyTypeSymbolsVisitorto use unbound generic types for documentation ID generation - Added new test case to verify that duplicate documentation IDs are not generated
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| XmlCommentDocumentationIdTests.cs | Added comprehensive test case verifying that generic properties and duplicate internal classes don't cause duplicate documentation IDs |
| OpenApiXmlCommentSupport.generated.verified.cs | Generated snapshot file showing expected output with properly deduplicated documentation IDs |
| XmlCommentGenerator.cs | Renamed ParseComments to separate methods for application and reference assemblies |
| XmlCommentGenerator.Parser.cs | Split comment parsing into two methods with different filtering logic for application vs reference assemblies |
| AssemblyTypeSymbolsVisitor.cs | Added logic to convert constructed generic types to unbound generic types for consistent documentation ID generation |
| // Only include symbols that are accessible from the application assembly. | ||
| symbol.IsAccessibleType() && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@desjoerd this seems okay, but should this PR also revisit the use of symbol.IsAccessible() up on L124?
Specifically, it seems like this source generator should default to excluding types marked with EmbeddedAttribute, since those are accessible from the application assembly, but seem to be of dubious worth w.r.t. embedding constants into the assembly for (what I assume are largely going to be) marker attributes for source generators, dev dependencies, etc.
(e.g. thinking about this from the aot perspective where I don't want paragraphs and paragraphs xml docs from internal/dev-time-only marker attributes getting baked into the executable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the application assembly types with [Embedded] are accessible and could even be used within your Api I think.
For referenced assemblies they will be already excluded as it will only emit xml comments for types marked as public.
https://github.com/dotnet/aspnetcore/blob/main/src%2FOpenApi%2Fgen%2FHelpers%2FISymbolExtensions.cs#L193
| var parsedCommentsFromXmlFile = commentsFromXmlFile | ||
| .Combine(context.CompilationProvider) | ||
| .Select(ParseComments); | ||
| .Select(ParseCommentsReference); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm...is this change related to the fix for generic types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one is to fix the comments coming from a documentation file when a type also exists in the main compilation.
When an internal type with the same namespace and class name exists in the Application assembly.
In the unit tests it's the Sample.Duplicate which is internal in the Application assembly. When it processes it from a different assembly it would resolve to Sample.Duplicate.
I duplicated the ParseComments into ParseCommentsReference and ParseCommentsApplicationAssembly to have a different condition. ParseCommentsReference only includes public types. ParseCommentsApplicationAssembly includes public types and types available in the compilation.
OpenAPI: Fix duplicate xml documentation ids for Generic properties and references
Fixes two instances where xml comment documentation ids where duplicated.
First one was for constructed generic type properties.
Foo<int>.Second one was for Xml comment files exposing internal classes with the same namespace.
Fixes #64378