-
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
Changes from all commits
83fa1d7
1a7206f
fc89118
4eb1dd9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -409,9 +409,24 @@ private async Task<OpenApiResponse> GetResponseAsync( | |
IOpenApiSchemaTransformer[] schemaTransformers, | ||
CancellationToken cancellationToken) | ||
{ | ||
// Check for custom description from ProducesResponseTypeMetadata if ApiResponseType.Description is null | ||
var description = apiResponseType.Description; | ||
if (string.IsNullOrEmpty(description)) | ||
{ | ||
// Look for custom description in endpoint metadata | ||
var customDescription = apiDescription.ActionDescriptor.EndpointMetadata? | ||
.OfType<IProducesResponseTypeMetadata>() | ||
.Where(m => m.StatusCode == statusCode) | ||
.LastOrDefault()?.Description; | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. |
||
description = !string.IsNullOrEmpty(customDescription) | ||
? customDescription | ||
: ReasonPhrases.GetReasonPhrase(statusCode); | ||
} | ||
|
||
var response = new OpenApiResponse | ||
{ | ||
Description = apiResponseType.Description ?? ReasonPhrases.GetReasonPhrase(statusCode), | ||
Description = description, | ||
Content = new Dictionary<string, OpenApiMediaType>() | ||
}; | ||
|
||
|
@@ -437,9 +452,9 @@ private async Task<OpenApiResponse> GetResponseAsync( | |
// MVC's `ProducesAttribute` doesn't implement the produces metadata that the ApiExplorer | ||
// looks for when generating ApiResponseFormats above so we need to pull the content | ||
// types defined there separately. | ||
var explicitContentTypes = apiDescription.ActionDescriptor.EndpointMetadata | ||
var explicitContentTypes = apiDescription.ActionDescriptor.EndpointMetadata? | ||
.OfType<ProducesAttribute>() | ||
.SelectMany(attr => attr.ContentTypes); | ||
.SelectMany(attr => attr.ContentTypes) ?? Enumerable.Empty<string>(); | ||
foreach (var contentType in explicitContentTypes) | ||
{ | ||
response.Content.TryAdd(contentType, new OpenApiMediaType()); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -116,10 +116,19 @@ private static OpenApiResponses GetOpenApiResponses(MethodInfo method, EndpointM | |
|
||
var eligibileAnnotations = new Dictionary<int, (Type?, MediaTypeCollection)>(); | ||
|
||
// Track custom descriptions for each status code | ||
var customDescriptions = new Dictionary<int, string?>(); | ||
|
||
foreach (var responseMetadata in producesResponseMetadata) | ||
{ | ||
var statusCode = responseMetadata.StatusCode; | ||
|
||
// 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 commentThe 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 |
||
} | ||
|
||
var discoveredTypeAnnotation = responseMetadata.Type; | ||
var discoveredContentTypeAnnotation = new MediaTypeCollection(); | ||
|
||
|
@@ -204,10 +213,15 @@ private static OpenApiResponses GetOpenApiResponses(MethodInfo method, EndpointM | |
responseContent[contentType] = new OpenApiMediaType(); | ||
} | ||
|
||
// Use custom description if available, otherwise fall back to default | ||
var description = customDescriptions.TryGetValue(statusCode, out var customDesc) && !string.IsNullOrEmpty(customDesc) | ||
? customDesc | ||
: GetResponseDescription(statusCode); | ||
|
||
responses[statusCode.ToString(CultureInfo.InvariantCulture)] = new OpenApiResponse | ||
{ | ||
Content = responseContent, | ||
Description = GetResponseDescription(statusCode) | ||
Description = description | ||
}; | ||
} | ||
return responses; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1022,6 +1022,114 @@ public void DoesNotGenerateRequestBodyWhenInferredBodyDisabled() | |
Assert.Null(operation.RequestBody); | ||
} | ||
|
||
[Fact] | ||
public void MixedCustomAndDefaultResponseDescriptionsAreAppliedCorrectly() | ||
{ | ||
const string customOkDescription = "Custom success response"; | ||
|
||
var operation = GetOpenApiOperation(() => "", additionalMetadata: new[] | ||
{ | ||
new ProducesResponseTypeMetadata(StatusCodes.Status200OK, null, new[] { "application/json" }) | ||
{ | ||
Description = customOkDescription | ||
}, | ||
new ProducesResponseTypeMetadata(StatusCodes.Status400BadRequest, null, new[] { "application/json" }) // No custom description - should use default | ||
}); | ||
|
||
Assert.Equal(2, operation.Responses.Count); | ||
|
||
var okResponse = operation.Responses["200"]; | ||
Assert.Equal(customOkDescription, okResponse.Description); | ||
|
||
var badRequestResponse = operation.Responses["400"]; | ||
Assert.Equal("Bad Request", badRequestResponse.Description); // Default reason phrase | ||
} | ||
|
||
[Fact] | ||
public void EmptyCustomDescriptionFallsBackToDefault() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Controllers:
|
||
{ | ||
var operation = GetOpenApiOperation(() => "", additionalMetadata: new[] | ||
{ | ||
new ProducesResponseTypeMetadata(StatusCodes.Status200OK, null, new[] { "application/json" }) | ||
{ | ||
Description = "" // Empty string should fall back to default | ||
} | ||
}); | ||
|
||
var response = Assert.Single(operation.Responses); | ||
Assert.Equal("200", response.Key); | ||
Assert.Equal("OK", response.Value.Description); // Should use default, not empty string | ||
} | ||
|
||
[Fact] | ||
public void NullCustomDescriptionFallsBackToDefault() | ||
{ | ||
var operation = GetOpenApiOperation(() => "", additionalMetadata: new[] | ||
{ | ||
new ProducesResponseTypeMetadata(StatusCodes.Status200OK, null, new[] { "application/json" }) | ||
{ | ||
Description = null // Explicit null should fall back to default | ||
} | ||
}); | ||
|
||
var response = Assert.Single(operation.Responses); | ||
Assert.Equal("200", response.Key); | ||
Assert.Equal("OK", response.Value.Description); // Should use default | ||
} | ||
|
||
[Fact] | ||
public void MultipleMetadataWithSameStatusCodePreservesLastDescription() | ||
{ | ||
const string firstDescription = "First description"; | ||
const string secondDescription = "Second description"; | ||
|
||
var operation = GetOpenApiOperation(() => "", additionalMetadata: new[] | ||
{ | ||
new ProducesResponseTypeMetadata(StatusCodes.Status200OK, typeof(string), new[] { "text/plain" }) | ||
{ | ||
Description = firstDescription | ||
}, | ||
new ProducesResponseTypeMetadata(StatusCodes.Status200OK, typeof(InferredJsonClass), new[] { "application/json" }) | ||
{ | ||
Description = secondDescription | ||
} | ||
}); | ||
|
||
var response = Assert.Single(operation.Responses); | ||
Assert.Equal("200", response.Key); | ||
Assert.Equal(secondDescription, response.Value.Description); // Should use the last one | ||
} | ||
|
||
[Fact] | ||
public void CustomDescriptionWorksWithVariousStatusCodes() | ||
{ | ||
const string createdDescription = "Resource was created successfully"; | ||
const string notFoundDescription = "The requested resource was not found"; | ||
const string serverErrorDescription = "An internal server error occurred"; | ||
|
||
var operation = GetOpenApiOperation(() => "", additionalMetadata: new[] | ||
{ | ||
new ProducesResponseTypeMetadata(StatusCodes.Status201Created, null, new[] { "application/json" }) | ||
{ | ||
Description = createdDescription | ||
}, | ||
new ProducesResponseTypeMetadata(StatusCodes.Status404NotFound, null, new[] { "application/json" }) | ||
{ | ||
Description = notFoundDescription | ||
}, | ||
new ProducesResponseTypeMetadata(StatusCodes.Status500InternalServerError, null, new[] { "application/json" }) | ||
{ | ||
Description = serverErrorDescription | ||
} | ||
}); | ||
|
||
Assert.Equal(3, operation.Responses.Count); | ||
|
||
Assert.Equal(createdDescription, operation.Responses["201"].Description); | ||
Assert.Equal(notFoundDescription, operation.Responses["404"].Description); | ||
Assert.Equal(serverErrorDescription, operation.Responses["500"].Description); | ||
} | ||
|
||
private static OpenApiOperation GetOpenApiOperation( | ||
Delegate action, | ||
string pattern = null, | ||
|
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:
#58193
Uh oh!
There was an error while loading. Please reload this page.
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.