Skip to content

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Sep 14, 2022

This is fixing a minor regression from a few hours ago. This only has in impact if you're using the new TypedResults with MVC Controllers to populate your ApiExplorer model for things like Swashbuckle.

In #43961, I updated the internal ProducesResponseTypeMetadata type to accept a null Type to match the interface. I think null is still correct for "text/plain" strings returned directly from minimal route handlers. However, I went too far with that change and modified a constructor that our existing TypedResults were already using to start using null instead of typeof(void) even though these are slightly different.

builder.Metadata.Add(new ProducesResponseTypeMetadata(StatusCodes.Status200OK));

OpenApiGenerator and EndpointMetadataApiDescriptionProvider handle typeof(void) and null the same, so this has no real impact when minimal route handlers return TypedResults.

discoveredTypeAnnotation = discoveredTypeAnnotation == null || discoveredTypeAnnotation == typeof(void)
? responseType
: discoveredTypeAnnotation;

if (apiResponseType.Type is null || apiResponseType.Type == typeof(void))
{
apiResponseType.Type = responseType;
}

However, for some reason, ApiResponseTypeProvider does treat these differently and excludes all results with a null type but not a void type.

if (apiResponseType.Type != null)
{
results[apiResponseType.StatusCode] = apiResponseType;
}

I don't really understand why ApiResponseTypeProvider treats these differently while the others don't, but it's been that way for 4 years. I'm more comfortable reverting the unnecessary change I made a couple hours ago. Semantically this makes more sense too. typeof(void) means there's no response content and null means there is content but no schema beyond the Content-Type (e.g. "text/plain").

@Pilchie

@halter73 halter73 added this to the 7.0-rc2 milestone Sep 14, 2022
@halter73 halter73 added the Servicing-consider Shiproom approval is required for the issue label Sep 14, 2022
@ghost
Copy link

ghost commented Sep 14, 2022

Hi @halter73. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@Pilchie
Copy link
Member

Pilchie commented Sep 14, 2022

Approved for .NET 7 RC2.

@Pilchie Pilchie enabled auto-merge (squash) September 14, 2022 20:28
@Pilchie Pilchie added the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Sep 14, 2022
@Pilchie Pilchie merged commit 26d284a into release/7.0 Sep 14, 2022
@Pilchie Pilchie deleted the halter73/void-fix branch September 14, 2022 21:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels Servicing-consider Shiproom approval is required for the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants