-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Fix ProducesResponseType's Description not being set for Minimal API's when attribute and inferred types aren't an exact match #62695
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
Thanks for your PR, @@sander1095. Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
TODO:
|
src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
Outdated
Show resolved
Hide resolved
…of metadata, and removed duplicate code
I've identified scenarios that currently fail but could potentially work with deeper type analysis. Should these be supported? Example: Attribute specifies [ProducesResponseType<List<BaseClass>>(200, Description = "Items")]
() => new List<DerivedClass>().AsEnumerable()
Trade-offs of supporting this:
Should I implement recursive generic type compatibility checking, or is the current collection-level matching sufficient? Keep in mind that this PR is only relevant for Minimal API's, and that |
// Currently, we do a "simple" bidirectional check to see if the types are assignable to each other. | ||
// This isn't very thorough, but it works for most cases. | ||
// For more information, check the related bug: https://github.com/dotnet/aspnetcore/issues/60518 | ||
return apiResponseType == metadaType || |
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.
Potential Downsides of Bidirectional Type Compatibility
While the bidirectional approach (Type.IsAssignableFrom()
in both directions) solves the immediate problem, there are some trade-offs to consider:
1. Over-Matching Risk
The bidirectional check could match unintended type relationships:
[ProducesResponseType<object>(200, Description = "Any object")]
() => TypedResults.Ok("specific string")
Since object.IsAssignableFrom(string) is true, this would incorrectly match and use the generic "Any object" description instead of inferring a more specific string response description.
2. Ambiguous Inheritance Scenarios
With complex inheritance hierarchies, the matching might be too permissive:
[ProducesResponseType<BaseClass>(200, Description = "Base description")]
[ProducesResponseType<DerivedClass>(200, Description = "Derived description")]
() => TypedResults.Ok(new DerivedClass())
Alternative: Unidirectional Approach
A more conservative approach would only check if the attribute type is assignable from the inferred type:
// Only allow: attribute type ← inferred type (inheritance/interface direction)
if (attributeType.IsAssignableFrom(responseType.Type))
This would:
- ✅ Handle IEnumerable ← List (the main use case)
- ✅ Handle BaseClass ← DerivedClass
- ❌ Prevent List ← IEnumerable (less common scenario)
- ✅ Avoid most over-matching issues
@captainsafia @halter73
Should we switch to the more conservative unidirectional approach, or are the current bidirectional benefits worth the potential edge case risks?
The bidirectional approach handles more scenarios but could be "too smart" in some cases.
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 improves how ProducesResponseType
descriptions are applied in Minimal APIs by matching attribute types and inferred return types through inheritance and interface compatibility rather than requiring exact equality.
- Updated
AddSupportedResponseTypes
to use a newTypesAreCompatible
method for type matching. - Introduced
TypesAreCompatible
for bidirectionalIsAssignableFrom
checks. - Added unit tests covering generics, collections, typed results, and base-class scenarios.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
File | Description |
---|---|
src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs | Use TypesAreCompatible instead of strict equality for type checks. |
src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs | Added tests for generic types, collections, typed results, and inheritance. |
Comments suppressed due to low confidence (2)
src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs:417
- The parameter name
metadaType
is missing a 't' and may be confusing. Consider renaming it tometadataType
(and adjust usages) for consistency.
static bool TypesAreCompatible(Type? apiResponseType, Type? metadaType)
src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs:334
- Add a test for a non-collection inheritance scenario, e.g., attribute
[ProducesResponseType<BaseClass>]
with a return type ofDerivedClass
, to ensureTypesAreCompatible
handles simple base/derived type matching correctly.
[Fact]
Fix ProducesResponseType's Description not being set for Minimal API's when attribute and inferred types aren't an exact match
Fix ProducesResponseType Description matching for inheritance/interface compatibility
Description
I contributed a new
Description
property to ASP.NET Core'sProducesResponseType
in an earlier preview release of .NET 10.Currently,
ProducesResponseType
attributes for Minimal API only match response types using strict equality comparison (==
). This causes the attribute descriptions to be ignored when there's a valid type compatibility relationship between the attribute type and the inferred return type.For example:
In this case, the description "List of items" is lost because
IEnumerable<string> != List<string>
, even thoughList<string>
implementsIEnumerable<string>
.We can fix this by doing a better check:
This supports the following scenarios:
While this works, there are some possible downsides to this approach, and more complex type differences between the attribute type and inferred type aren't handled by this code. See the PR comments for more information and discussions.
Fixes #60518