Skip to content

Setting the Description in a ProducesResponseTypeAttribute works correctly for Minimal API #60539

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Mar 11, 2025
Merged

Setting the Description in a ProducesResponseTypeAttribute works correctly for Minimal API #60539

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
aee729a
Improve existing test
sander1095 Feb 21, 2025
ef31d31
Add more tests for regression
sander1095 Feb 21, 2025
4730e8f
Add tests which now succeed
sander1095 Feb 21, 2025
38606e9
Add more tests
sander1095 Feb 21, 2025
cf68bbe
Merge branch 'dotnet:main' into fix-openapi-description-bug-minimal-api
sander1095 Feb 25, 2025
917bacc
Merge remote-tracking branch 'origin/main' into fix-openapi-descripti…
sander1095 Mar 3, 2025
6fcf274
Set the description to the final one encountered and add tests
sander1095 Mar 4, 2025
a083307
Merge remote-tracking branch 'origin/main' into fix-openapi-descripti…
sander1095 Mar 11, 2025
fad4923
Use local method as requested by PR comment
sander1095 Mar 11, 2025
636c009
Fix comment
sander1095 Mar 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/Mvc/Mvc.ApiExplorer/src/EndpointMetadataApiDescriptionProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,9 @@ private static void AddSupportedResponseTypes(
apiResponseType.ApiResponseFormats.Add(defaultResponseFormat);
}

// We set the Description to the first non-null value we find that matches the status code.
apiResponseType.Description ??= responseMetadataTypes.FirstOrDefault(x => x.StatusCode == apiResponseType.StatusCode && x.Description is not null)?.Description;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason the existing logic in the ApiResponseTypeProvider doesn't handle this? That feels like the right place to do this instead of having to filter on the types returned by it to set the description.

Copy link
Contributor Author

@sander1095 sander1095 Feb 25, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @captainsafia ! Great question! We need to discuss how we want to approach this.

Short answer: The ApiRespnseTypeProvider does set the description, but the EndpointMetadataProvider throws it away. I found out about this specific behavior thanks to your comment. Below is the explanation and some suggestions on ways we could perhaps make the code a bit more clear:

I dove a bit deeper into this, and this is what happens:

The code above your comment does this (AddSupportedResponseTypes() in EndpointMetadataApiDescriptionProvider.cs)

var responseProviderMetadataTypes = ApiResponseTypeProvider.ReadResponseMetadata(
    responseProviderMetadata, responseType, defaultErrorType, contentTypes, out var errorSetByDefault);
var producesResponseMetadataTypes = ApiResponseTypeProvider.ReadResponseMetadata(producesResponseMetadata, responseType);
  • responseProviderMetadataTypes contains the Description set in ApiResponseTypeProvider, as you correctly said!
  • producesResponseMetadataTypes does not contain the Description.

Next, the following happens:

// We favor types added via the extension methods (which implements IProducesResponseTypeMetadata)
// over those that are added via attributes.
var responseMetadataTypes = producesResponseMetadataTypes.Values.Concat(responseProviderMetadataTypes.Values);
if (responseMetadataTypes.Any())
{
    foreach (var apiResponseType in responseMetadataTypes)
    {

        // Code you commented on happens here
   
        if (!supportedResponseTypes.Any(existingResponseType => existingResponseType.StatusCode == apiResponseType.StatusCode))
        {
            supportedResponseTypes.Add(apiResponseType);
        }
    }
}

SupportedResponseTypes is eventually returned and used.

As you can see, both lists are combined with producesResponseMetadataTypes being iterated over first, and thus that's the first apiResponseType that's added to the supportedResponseTypes, causing it to not contain a description.

At some point we loop over the responseProviderMetadataTypes which DO contain the description, but that doesn't get added to the list of supportedResponseTypes because that type already exists thanks to the metadata.

My code forces the Description to be set when we loop over the first iteration of the metadata, which fixes the issue. But this is a bit confusing, now that I understand the code a bit better.

What would you like as a solution? The core problem is that metadata is iterated over first, which doesn't contain the description. It then fails to enrich the response types from ApiDescriptionProvider which supplies the description.

  • IProducesResponseTypeMetadata should also have a Description.
    • On line 330 we call endpointMetadata.GetOrderedMetadata<IProducesResponseTypeMetadata>();. This has a Description property, but is null. I'm not sure if this is intended, or if I forgot to set the Description for metadata in a previous PR. (Please advise if this is desirable/possible).
    • If it is desired that IProducesResponseTypeMetadata has a Description, I will need to make that happen AND update line 247 in ApiDescriptionProvider to set the description.
  • Setting description once
    • I can keep the code the same, so it uses the first non-null description
    • I could change my code to use the LAST description it encounters in the list, in case multiple descriptions are set
  • Keep code the same, add comments to explain it
  • Move the code that sets the description out of the loop and populate the Descriptions after the loop, based on the data from responseProviderMetadataTypes. This makes the code more explicit.
  • Other thoughts are welcome!

This also explains why this bug happens in Minimal API's and not in Controllers. I believe EndpointMetadataApiDescriptionProvider is only used for Minimal API, whereas ApiDescriptionProvider is used for both Controllers and Minimal API. Because the EndpointMetadataApiDescriptionProvider forgets to set enrich response types it has already added to the list, the description goes missing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sander1095 Thanks for digging into this and sharing detailed notes. Based on this analysis, I expect this option:

I could change my code to use the LAST description it encounters in the list, in case multiple descriptions are set

To be the prefered one since it aligns most closely with the way metadata gets respected in other scenarios. We'll have to sanity check this implementation with multiple ProducesResponseType attributes on the same endpoint to make sure the additions only apply to the target type and status code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@captainsafia Done! See the latest commits


if (!supportedResponseTypes.Any(existingResponseType => existingResponseType.StatusCode == apiResponseType.StatusCode))
{
supportedResponseTypes.Add(apiResponseType);
Expand Down
60 changes: 60 additions & 0 deletions src/Mvc/Mvc.ApiExplorer/test/ApiResponseTypeProviderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,66 @@ public void GetApiResponseTypes_ReturnsResponseTypesFromApiConventionItem()
});
}

[Fact]
public void GetApiResponseTypes_ReturnsDescriptionFromProducesResponseType()
{
// Arrange

const string expectedOkDescription = "All is well";
const string expectedBadRequestDescription = "Invalid request";
const string expectedNotFoundDescription = "Something was not found";

var actionDescriptor = GetControllerActionDescriptor(
typeof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController),
nameof(GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController.DeleteBase));

actionDescriptor.Properties[typeof(ApiConventionResult)] = new ApiConventionResult(new[]
{
new ProducesResponseTypeAttribute(200) { Description = expectedOkDescription},
new ProducesResponseTypeAttribute(400) { Description = expectedBadRequestDescription },
new ProducesResponseTypeAttribute(404) { Description = expectedNotFoundDescription },
});

var provider = GetProvider();

// Act
var result = provider.GetApiResponseTypes(actionDescriptor);

// Assert
Assert.Collection(
result.OrderBy(r => r.StatusCode),
responseType =>
{
Assert.Equal(200, responseType.StatusCode);
Assert.Equal(typeof(BaseModel), responseType.Type);
Assert.False(responseType.IsDefaultResponse);
Assert.Equal(expectedOkDescription, responseType.Description);
Assert.Collection(
responseType.ApiResponseFormats,
format =>
{
Assert.Equal("application/json", format.MediaType);
Assert.IsType<TestOutputFormatter>(format.Formatter);
});
},
responseType =>
{
Assert.Equal(400, responseType.StatusCode);
Assert.Equal(typeof(void), responseType.Type);
Assert.False(responseType.IsDefaultResponse);
Assert.Empty(responseType.ApiResponseFormats);
Assert.Equal(expectedBadRequestDescription, responseType.Description);
},
responseType =>
{
Assert.Equal(404, responseType.StatusCode);
Assert.Equal(typeof(void), responseType.Type);
Assert.False(responseType.IsDefaultResponse);
Assert.Empty(responseType.ApiResponseFormats);
Assert.Equal(expectedNotFoundDescription, responseType.Description);
});
}

[ApiConventionType(typeof(DefaultApiConventions))]
public class GetApiResponseTypes_ReturnsResponseTypesFromDefaultConventionsController : ControllerBase
{
Expand Down
75 changes: 75 additions & 0 deletions src/Mvc/Mvc.ApiExplorer/test/EndpointMetadataApiDescriptionProviderTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,81 @@ public void AddsMultipleResponseFormatsForTypedResults()
Assert.Empty(badRequestResponseType.ApiResponseFormats);
}

[Fact]
public void AddsResponseDescription()
{
const string expectedCreatedDescription = "A new item was created";
const string expectedBadRequestDescription = "Validation failed for the request";

var apiDescription = GetApiDescription(
[ProducesResponseType(typeof(TimeSpan), StatusCodes.Status201Created, Description = expectedCreatedDescription)]
[ProducesResponseType(StatusCodes.Status400BadRequest, Description = expectedBadRequestDescription)]
() => TypedResults.Created("https://example.com", new TimeSpan()));

Assert.Equal(2, apiDescription.SupportedResponseTypes.Count);

var createdResponseType = apiDescription.SupportedResponseTypes[0];

Assert.Equal(201, createdResponseType.StatusCode);
Assert.Equal(typeof(TimeSpan), createdResponseType.Type);
Assert.Equal(typeof(TimeSpan), createdResponseType.ModelMetadata?.ModelType);
Assert.Equal(expectedCreatedDescription, createdResponseType.Description);

var createdResponseFormat = Assert.Single(createdResponseType.ApiResponseFormats);
Assert.Equal("application/json", createdResponseFormat.MediaType);

var badRequestResponseType = apiDescription.SupportedResponseTypes[1];

Assert.Equal(400, badRequestResponseType.StatusCode);
Assert.Equal(typeof(void), badRequestResponseType.Type);
Assert.Equal(typeof(void), badRequestResponseType.ModelMetadata?.ModelType);
Assert.Equal(expectedBadRequestDescription, badRequestResponseType.Description);
}

[Fact]
public void WithEmptyMethodBody_AddsResponseDescription()
{
const string expectedCreatedDescription = "A new item was created";
const string expectedBadRequestDescription = "Validation failed for the request";

var apiDescription = GetApiDescription(
[ProducesResponseType(typeof(TimeSpan), StatusCodes.Status201Created, Description = expectedCreatedDescription)]
[ProducesResponseType(StatusCodes.Status400BadRequest, Description = expectedBadRequestDescription)]
() => new InferredJsonClass());

Assert.Equal(3, apiDescription.SupportedResponseTypes.Count);

var rdfInferredResponseType = apiDescription.SupportedResponseTypes[0];

Assert.Equal(200, rdfInferredResponseType.StatusCode);
Assert.Equal(typeof(InferredJsonClass), rdfInferredResponseType.Type);
Assert.Equal(typeof(InferredJsonClass), rdfInferredResponseType.ModelMetadata?.ModelType);

var rdfInferredResponseFormat = Assert.Single(rdfInferredResponseType.ApiResponseFormats);
Assert.Equal("application/json", rdfInferredResponseFormat.MediaType);
Assert.Null(rdfInferredResponseType.Description); // There is no description set for the default "200" code, so we expect it to be null.

var createdResponseType = apiDescription.SupportedResponseTypes[1];

Assert.Equal(201, createdResponseType.StatusCode);
Assert.Equal(typeof(TimeSpan), createdResponseType.Type);
Assert.Equal(typeof(TimeSpan), createdResponseType.ModelMetadata?.ModelType);
Assert.Equal(expectedCreatedDescription, createdResponseType.Description);

var createdResponseFormat = Assert.Single(createdResponseType.ApiResponseFormats);
Assert.Equal("application/json", createdResponseFormat.MediaType);

var badRequestResponseType = apiDescription.SupportedResponseTypes[2];

Assert.Equal(400, badRequestResponseType.StatusCode);
Assert.Equal(typeof(InferredJsonClass), badRequestResponseType.Type);
Assert.Equal(typeof(InferredJsonClass), badRequestResponseType.ModelMetadata?.ModelType);
Assert.Equal(expectedBadRequestDescription, badRequestResponseType.Description);

var badRequestResponseFormat = Assert.Single(badRequestResponseType.ApiResponseFormats);
Assert.Equal("application/json", badRequestResponseFormat.MediaType);
}

[Fact]
public void AddsResponseFormatsForTypedResultWithoutReturnType()
{
Expand Down
87 changes: 79 additions & 8 deletions ...re.OpenApi.Tests/Services/OpenApiDocumentService/OpenApiDocumentServiceTests.Responses.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,8 +305,11 @@ await VerifyOpenApiDocument(builder, document =>
});
}

/// <remarks>
/// Regression test for https://github.com/dotnet/aspnetcore/issues/60518
/// </remarks>
[Fact]
public async Task GetOpenApiResponse_UsesDescriptionSetByUser()
public async Task GetOpenApiResponse_WithEmptyMethodBody_UsesDescriptionSetByUser()
{
// Arrange
var builder = CreateBuilder();
Expand All @@ -315,8 +318,8 @@ public async Task GetOpenApiResponse_UsesDescriptionSetByUser()
const string expectedBadRequestDescription = "Validation failed for the request";

// Act
builder.MapGet("/api/todos",
[ProducesResponseType(typeof(TimeSpan), StatusCodes.Status201Created, Description = expectedCreatedDescription)]
builder.MapPost("/api/todos",
[ProducesResponseType<Todo>(StatusCodes.Status200OK, Description = expectedCreatedDescription)]
[ProducesResponseType(StatusCodes.Status400BadRequest, Description = expectedBadRequestDescription)]
() =>
{ });
Expand All @@ -328,7 +331,41 @@ await VerifyOpenApiDocument(builder, document =>
Assert.Collection(operation.Responses.OrderBy(r => r.Key),
response =>
{
Assert.Equal("201", response.Key);
Assert.Equal("200", response.Key);
Assert.Equal(expectedCreatedDescription, response.Value.Description);
},
response =>
{
Assert.Equal("400", response.Key);
Assert.Equal(expectedBadRequestDescription, response.Value.Description);
});
});
}

[Fact]
public async Task GetOpenApiResponse_UsesDescriptionSetByUser()
{
// Arrange
var builder = CreateBuilder();

const string expectedCreatedDescription = "A new todo item was created";
const string expectedBadRequestDescription = "Validation failed for the request";

// Act
builder.MapPost("/api/todos",
[ProducesResponseType<Todo>(StatusCodes.Status200OK, Description = expectedCreatedDescription)]
[ProducesResponseType(StatusCodes.Status400BadRequest, Description = expectedBadRequestDescription)]
() =>
{ return TypedResults.Ok(new Todo(1, "Lorem", true, DateTime.UtcNow)); }); // This code doesn't return Bad Request, but that doesn't matter for this test.

// Assert
await VerifyOpenApiDocument(builder, document =>
{
var operation = Assert.Single(document.Paths["/api/todos"].Operations.Values);
Assert.Collection(operation.Responses.OrderBy(r => r.Key),
response =>
{
Assert.Equal("200", response.Key);
Assert.Equal(expectedCreatedDescription, response.Value.Description);
},
response =>
Expand All @@ -346,8 +383,42 @@ public async Task GetOpenApiResponse_UsesStatusCodeReasonPhraseWhenExplicitDescr
var builder = CreateBuilder();

// Act
builder.MapGet("/api/todos",
[ProducesResponseType(typeof(TimeSpan), StatusCodes.Status201Created, Description = null)] // Explicitly set to NULL
builder.MapPost("/api/todos",
[ProducesResponseType<Todo>(StatusCodes.Status200OK, Description = null)] // Explicitly set to NULL
[ProducesResponseType(StatusCodes.Status400BadRequest)] // Omitted, meaning it should be NULL
() =>
{ return TypedResults.Ok(new Todo(1, "Lorem", true, DateTime.UtcNow)); }); // This code doesn't return Bad Request, but that doesn't matter for this test.

// Assert
await VerifyOpenApiDocument(builder, document =>
{
var operation = Assert.Single(document.Paths["/api/todos"].Operations.Values);
Assert.Collection(operation.Responses.OrderBy(r => r.Key),
response =>
{
Assert.Equal("200", response.Key);
Assert.Equal("OK", response.Value.Description);
},
response =>
{
Assert.Equal("400", response.Key);
Assert.Equal("Bad Request", response.Value.Description);
});
});
}

/// <remarks>
/// Regression test for https://github.com/dotnet/aspnetcore/issues/60518
/// </remarks>
[Fact]
public async Task GetOpenApiResponse_WithEmptyMethodBody_UsesStatusCodeReasonPhraseWhenExplicitDescriptionIsMissing()
{
// Arrange
var builder = CreateBuilder();

// Act
builder.MapPost("/api/todos",
[ProducesResponseType<Todo>(StatusCodes.Status200OK, Description = null)] // Explicitly set to NULL
[ProducesResponseType(StatusCodes.Status400BadRequest)] // Omitted, meaning it should be NULL
() =>
{ });
Expand All @@ -359,8 +430,8 @@ await VerifyOpenApiDocument(builder, document =>
Assert.Collection(operation.Responses.OrderBy(r => r.Key),
response =>
{
Assert.Equal("201", response.Key);
Assert.Equal("Created", response.Value.Description);
Assert.Equal("200", response.Key);
Assert.Equal("OK", response.Value.Description);
},
response =>
{
Expand Down
Loading