-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Fix response description #63778
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?
Fix response description #63778
Conversation
Updated `ResponseEndpoints` to include a `/custom-description` endpoint that returns a "Hello World" message with custom metadata. Enhanced `OpenApiGenerator` to track and apply custom response descriptions, falling back to defaults when necessary. Fixed project file to ensure proper inclusion of Helix content. Added unit tests in `OpenApiOperationGeneratorTests` to validate the handling of custom and default response descriptions across various scenarios.
This commit introduces new endpoint mappings in `MapResponsesEndpoints.cs` to support custom descriptions using attributes and extension methods. The `OpenApiDocumentService` is updated to check for these custom descriptions in endpoint metadata, ensuring accurate representation in OpenAPI documentation. Additionally, a new test is added to verify the correct usage of custom descriptions in the OpenAPI response.
Removed unnecessary line breaks and adjusted comments in `MapResponsesEndpoints.cs` and `OpenApiGeneratorTests.cs`. Streamlined `GetOpenApiOperation` method calls for cleaner code while maintaining existing functionality and logic.
Introduced two new endpoints: `/responses/custom-description-attribute` and `/responses/custom-description-extension-method`. Each has a `GET` method with a `200` response status, including custom descriptions and content types. The first endpoint returns a plain text response, while the second returns an HTML response, enhancing API documentation.
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 the handling of custom response descriptions in OpenAPI generation. Previously, the implementation wasn't properly tracking or using custom descriptions provided through ProducesResponseTypeMetadata
.
- Implements proper tracking and usage of custom descriptions from response metadata
- Adds fallback logic to use default HTTP reason phrases when custom descriptions are null or empty
- Adds comprehensive test coverage for various custom description scenarios
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
src/OpenApi/src/Services/OpenApiGenerator.cs |
Adds tracking of custom descriptions from ProducesResponseTypeMetadata and uses them in response generation |
src/OpenApi/src/Services/OpenApiDocumentService.cs |
Adds logic to check for custom descriptions in endpoint metadata when ApiResponseType.Description is null |
src/OpenApi/sample/Endpoints/MapResponsesEndpoints.cs |
Adds sample endpoints demonstrating custom description usage |
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiGeneratorTests.cs |
Adds comprehensive unit tests for custom description scenarios |
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Services/OpenApiDocumentService/OpenApiDocumentServiceTests.Responses.cs |
Adds integration test for custom description metadata |
Various snapshot files | Updates test snapshots to reflect new custom description endpoints |
src/OpenApi/test/Microsoft.AspNetCore.OpenApi.Tests/Microsoft.AspNetCore.OpenApi.Tests.csproj |
Minor formatting improvement to MSBuild item |
/cc @sander1095 as you added the |
Hi! I'm looking into it now, thanks for tagging. Will update this comment when I'm done! |
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.
UPDATE: I just turned off my laptop and realized that I looked at this completely wrong, so I'm back!
This works:
app.MapGet("/endpoint", () =>
{
return "hi";
})
.WithMetadata(new ProducesResponseTypeAttribute(typeof(string), 200) { Description = "hey" });
But, like @IeuanWalker said in this original issue, this doesn't:
app.MapGet("/endpoint", () =>
{
return "hi";
})
.WithMetadata(new ProducesResponseTypeMetadata(200, typeof(string)) { Description = "hey" });
I completely misread! I thought that @IeuanWalker was using ProducesResponseType
, but he was using ProducesResponseTypeMetadata
instead.
The reason: The ProducesResponseTypeMetadata description never gets populated (I believe!) I have even wondered if I should have added Description
to that class in the first place.... See my link in the next paragraph...
ASSUMPTION: ProducesResponseTypeMetadata , at least for minimal API's, is used for discovering the metadata of an endpoint, so user specified data is ignored(??). In @IeuanWalker's case, they should have used ProducesResponseType
instead, which is set by the user and overrides the metadata. However, I think if we do want to support ProducesResponseTypeMetadata
with WithMetadata
, changes need to be made to ApiResponseTypeProvider
and/or EndpointMetadataApiDescriptionProvider
, though this statement needs verification!
I discussed an issue with IProducesResponseTypeMetadata.Description
never getting populated in one of my earlier PR's: #60539 (comment). I think this discussion is very relevant to what is happening here. To avoid guesswork and putting people on false trails, I can't currently say more, but I think this is what a team member (like @captainsafia ) should focus on! :D
Please treat my existing comments with care, as some comments (like suggestions/thoughts on code approaches) may now be irrelevant. I would suggest to wait with making changes until a team member has officially reviewed this changeset, too!
I do wonder if a lot of these changed need to be made in OpenApiDocumentService
and OpenApiGenerator
, vs something like EndpointMetadataApiDescriptionProvider
.
Metadata is added by calling WithMetadata
, but this is then ignored by existing code. I think it would make more sense that the existing OpenAPI -> ApiExplorer infrastructure would deal with it.
For example, line 414 in OpenApiDocumentService
tries to grab the apiResponseType.Description
. Shouldn't that have been filled by WithMetadata
in the first place?
This is perhaps also the reason why I didn't catch this bug in the first place. When using the "intended" (currently tested) behavior, EndpointMetadataApiDescriptionProvider
finds and deals with this description:
app.MapGet("/endpoint", [ProducesResponseType<string>(200, Description = "hello")] () =>
{
return "Hi!";
});
Why is this not the case for WithMetadata
? If I am right, and we can fix this in another place, we can prevent lots of duplicate code here, which is my main concern on this PR (and some extra tests/questions on behavioral differences).
@captainsafia can probably say more about what code should be modified (The current proposed changeset, or something more related to the integration between WithMetadata
and EndpointMetadataApiDescriptionProvider
).
Let me know if I can help out more!
.OfType<IProducesResponseTypeMetadata>() | ||
.Where(m => m.StatusCode == statusCode) | ||
.LastOrDefault()?.Description; | ||
|
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.
I can't truly say if this is the right approach or not, but I can say that I think we're creating (even more) duplicate code here.
The reason why your bug exists, is because I skipped over this scenario when adding support for response descriptions in minimal API. (Thanks so much for spotting it, and creating a PR!)
There are a lot of places where the response description gets set. In OpenApiGenerator
, in OpenApiDocumentService
, and in EndpointMetadataApiDescriptionProvider
. Compare that to Controllers, where https://github.com/dotnet/aspnetcore/blob/main/src/Mvc/Mvc.ApiExplorer/src/ApiResponseTypeProvider.cs#L134-L135 is the only place I needed to make (significant) changes to get controllers to play nice with the new property.
In this case, this code here looks quite similar to the code I wrote in these 2 PR's (#60539 and #62695 ), but in the last PR I still added more check to deal with the issue of inferred types.
I am not sure if that code also belongs here, but it does look a bit like a copy, which can be a hazard for the future.
// Capture custom description if provided | ||
if (!string.IsNullOrEmpty(responseMetadata.Description)) | ||
{ | ||
customDescriptions[statusCode] = responseMetadata.Description; |
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.
I believe this will cause issues in cases where there are multiple of the same status codes, with different descriptions (and/or different response bodies). I think this needs test cases to check how that is dealt with. It has to be in line with existing controller and minimal API logic (do we grab the first or last match?)
My code for this in another place deals with this by checking for status, type and description. Perhaps that is relevant here: https://github.com/dotnet/aspnetcore/pull/62695/files
} | ||
|
||
[Fact] | ||
public void EmptyCustomDescriptionFallsBackToDefault() |
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.
Treating an empty string as null shouldnt be the case according to the existing Response Description support in Minimal API
RC1:
app.MapGet("/endpoint", [ProducesResponseType<string>(200, Description = "")] () =>
{
return "Hi!";
});
"/endpoint": {
"get": {
"tags": [
"openapi"
],
"responses": {
"200": {
"description": "", // not null!
"content": {
"text/plain": {
"schema": {
"type": "string"
}
}
}
}
}
}
}
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.
Controllers:
[ApiController]
[Route("[controller]")]
public class WeatherForecastController : ControllerBase
{
[HttpGet]
[ProducesResponseType<string>(200, Description = "")]
public IActionResult Get()
{
return Ok("hi");
}
}
"paths": {
"/WeatherForecast": {
"get": {
"tags": [
"WeatherForecast"
],
"responses": {
"200": {
"description": "", // not null
"content": {
"text/plain": {
"schema": {
"type": "string"
}
},
"application/json": {
"schema": {
"type": "string"
}
},
"text/json": {
"schema": {
"type": "string"
}
}
}
}
}
}
}
{ | ||
// Look for custom description in endpoint metadata | ||
var customDescription = apiDescription.ActionDescriptor.EndpointMetadata? | ||
.OfType<IProducesResponseTypeMetadata>() |
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.
I believe you'd need to check more types. According to my initial PR, description support has been added to more attributes/classes that do not implement this interface:
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.
Preferably, we'd not need this call at all, as I think this is the wrong place for this kind of check. See my overall PR review comment for more info.
Just a quick add-on:
|
thanks @sander1095, didn't realise when I got into it how many places i would be touching lol. Started adding in unit tests and realised I fixed one way and wasn't working another way, so started changing more code to fix it fully. Will wait to see where the appropriate place is. The reason I found it is because I'm in the middle of writing a source generator which will write the minimal api code for you, but let you have similar syntax to FastEndpoints, and the way it's currently implemented doesn't work with the attributes, so I have to use the extension methods
|
@IeuanWalker Please check my updated main comment! Regardless of this PR, Using
|
thanks @sander1095, wrote this transformer and it seems to do exactly what i needed now - public static class OpenApiExtensions
{
public static RouteHandlerBuilder WithResponse(this RouteHandlerBuilder builder, int statusCode, string description, string? contentType = null)
{
builder.WithResponse(null, statusCode, description, contentType);
return builder;
}
public static RouteHandlerBuilder WithResponse<T>(this RouteHandlerBuilder builder, int statusCode, string description, string? contentType = null)
{
builder.WithResponse(typeof(T), statusCode, description, contentType);
return builder;
}
static RouteHandlerBuilder WithResponse(this RouteHandlerBuilder builder, Type? type, int statusCode, string description, string? contentType = null)
{
if (contentType is null)
{
builder.WithMetadata(new ProducesResponseTypeMetadata(statusCode, type ?? typeof(void)));
}
else
{
builder.WithMetadata(new ProducesResponseTypeMetadata(statusCode, type ?? typeof(void), [contentType]));
}
builder.AddOpenApiOperationTransformer((operation, context, _) =>
{
operation.Responses[statusCode.ToString()].Description = description;
return Task.CompletedTask;
});
return builder;
}
} ![]() Does seem strange that all this behaviour isnt part of the |
I agree! I created
PS: Keep in mind your code will break when the same status code appears multiple times, but with different descriptions. Your dictionary key should be status code + response type to support edge cases. Please share your project if you haven't done so! Would love to learn. |
@sander1095, is it even possible to have the same status code appear multiple times, as it's a dictionary and the key is the status code? if so, what would be the updated code? Ye will do, im hoping to release it when .NET 10 releases, but got a holiday and work so will see how it goes lol |
You are right, @IeuanWalker , this isn't possible and not a part of the spec either. My mistake.
Therefore, 1 HTTP status code can have 1 response description. If someone has multiple descriptions for different response body for 1 status code, that isn't legal, and I think ASP.NET core currently just uses the last one defined. My apologies for creating confusion. |
Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime. |
@sander1095, this is the project I was talking about, plan to release it fully the same time as .NET 10 |
Fix response description
Summary of the changes (Less than 80 chars)
Description
Previous implementation wasn't looking/ tracking the custom descriptions
Fixes #63766