- Notifications
You must be signed in to change notification settings - Fork10.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 ourterms of service andprivacy 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
| returncomments; | ||
| } | ||
| internalstaticIEnumerable<(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 toParseCommentsApplicationAssembly 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 | RenamedParseComments 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 ofsymbol.IsAccessible() up onL124?
Specifically, it seems like this source generator should default toexcluding types marked withEmbeddedAttribute, 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 aspublic.
https://github.com/dotnet/aspnetcore/blob/main/src%2FOpenApi%2Fgen%2FHelpers%2FISymbolExtensions.cs#L193
| varparsedCommentsFromXmlFile=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 theSample.Duplicate which is internal in the Application assembly. When it processes it from a different assembly it would resolve toSample.Duplicate.
I duplicated theParseComments intoParseCommentsReference andParseCommentsApplicationAssembly to have a different condition.ParseCommentsReference only includespublic types.ParseCommentsApplicationAssembly includespublic types and types available in the compilation.
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
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