Skip to content

Conversation

brunolins16
Copy link
Member

@brunolins16 brunolins16 commented May 11, 2022

Closes #41421

This PR will introduce a new internal API Convention (EndpointMetadataConvention), included as default, that will discover metadata from return type or parameter types that implements IEndpointProviderMetadata or IEndpointParameterProviderMetadata and contribute to Action.Selectors endpoint metadata.

It also includes the following changes to APIExplorer:

  1. IProducesResponseTypeMetadata will contribute to the API Response types metadata.
  2. IAcceptsMetadata will contribute to the Supported Request Types.

These changes will affect only how the metadata is generated since the new metadata will not be added as a Filter.

Example:

[ApiController]
[Route("[controller]")]
public class WeatherForecastController : ControllerBase
{
     // Both actions will produce a similar metadata since Microsoft.AspNetCore.Http.HttpResults.Created/BadRequest
     // implements IEndpointMetadataProvider, the same can be accomplished with an ActionResult 
     // if it implements the interface, and WeatherForecast implements
     // IEndpointParameterMetadataProvider

    [HttpPost]
    public Results<Created<WeatherForecast>, BadRequest<string>> Post(
             WeatherForecast weatherForecast)
        => weatherForecast.TemperatureC < 10 ? 
                   TypedResults.BadRequest("It is cold!") : 
                   TypedResults.Created("some-location", weatherForecast);

    [HttpPost("post-with-consumes")]
    [ProducesResponseType(typeof(WeatherForecast), StatusCodes.Status201Created)]
    [ProducesResponseType(typeof(string), StatusCodes.Status400BadRequest)]
    public ActionResult PostActionResult(WeatherForecast weatherForecast)
        => weatherForecast.TemperatureC < 10 ? 
                   BadRequest("It is cold!") : 
                   Created("some -location", weatherForecast);
}

public class WeatherForecast  : IEndpointParameterMetadataProvider
{
    public DateTime Date { get; set; }
    public int TemperatureC { get; set; }
    public string Summary { get; set; }

    public static void PopulateMetadata(EndpointParameterMetadataContext parameterContext)
    {
        parameterContext.EndpointMetadata.Add(new ConsumesAttribute(typeof(WeatherForecast), "application/json+custom"));
    }
}

Metadata

{
  "openapi": "3.0.1",
  "info": {
    "title": "Issue41421",
    "version": "1.0"
  },
  "paths": {
    "/WeatherForecast": {
      "post": {
        "tags": [
          "WeatherForecast"
        ],
        "requestBody": {
          "content": {
            "application/json+custom": {
              "schema": {
                "$ref": "#/components/schemas/WeatherForecast"
              }
            }
          }
        },
        "responses": {
          "201": {
            "description": "Success",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/WeatherForecast"
                }
              }
            }
          },
          "400": {
            "description": "Bad Request",
            "content": {
              "application/json": {
                "schema": {
                  "type": "string"
                }
              }
            }
          }
        }
      }
    },
    "/WeatherForecast/post-with-consumes": {
      "post": {
        "tags": [
          "WeatherForecast"
        ],
        "requestBody": {
          "content": {
            "application/json+custom": {
              "schema": {
                "$ref": "#/components/schemas/WeatherForecast"
              }
            }
          }
        },
        "responses": {
          "201": {
            "description": "Success",
            "content": {
              "text/plain": {
                "schema": {
                  "$ref": "#/components/schemas/WeatherForecast"
                }
              },
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/WeatherForecast"
                }
              },
              "text/json": {
                "schema": {
                  "$ref": "#/components/schemas/WeatherForecast"
                }
              },
              "application/xml": {
                "schema": {
                  "$ref": "#/components/schemas/WeatherForecast"
                }
              },
              "text/xml": {
                "schema": {
                  "$ref": "#/components/schemas/WeatherForecast"
                }
              }
            }
          },
          "400": {
            "description": "Bad Request",
            "content": {
              "text/plain": {
                "schema": {
                  "type": "string"
                }
              },
              "application/json": {
                "schema": {
                  "type": "string"
                }
              },
              "text/json": {
                "schema": {
                  "type": "string"
                }
              },
              "application/xml": {
                "schema": {
                  "type": "string"
                }
              },
              "text/xml": {
                "schema": {
                  "type": "string"
                }
              }
            }
          }
        }
      }
    }
  },
  "components": {
    "schemas": {
      "WeatherForecast": {
        "type": "object",
        "properties": {
          "date": {
            "type": "string",
            "format": "date-time"
          },
          "temperatureC": {
            "type": "integer",
            "format": "int32"
          },
          "summary": {
            "type": "string",
            "nullable": true
          }
        },
        "additionalProperties": false
      }
    }
  }
}

@ghost ghost 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 May 11, 2022
@brunolins16 brunolins16 marked this pull request as ready for review May 11, 2022 23:22
@brunolins16 brunolins16 requested a review from a team as a code owner May 11, 2022 23:22
@brunolins16 brunolins16 requested a review from a team May 11, 2022 23:23
responseTypeMetadataProviders,
_modelMetadataProvider);

var responseTypes = responseTypesFromProvider.Concat(responseTypesFromEndpointMetadata.Values).ToList();
Copy link
Member

Choose a reason for hiding this comment

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

Which metadata do we prefer? In the case of duplicates it looks like the last one wins so we should concat the preferred list in order to iterate over it last.

Copy link
Member

Choose a reason for hiding this comment

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

Or, should we be removing duplicates? We do return this list and use it elsewhere, so I don't know the implications of duplicate items

Copy link
Member

Choose a reason for hiding this comment

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

It's additive. I think this is fine considering these are generally IResult implementations, so I hope MVC isn't inferring any other response types by default.

var producesResponseMetadata = endpointMetadata.GetOrderedMetadata<IProducesResponseTypeMetadata>();


Or, should we be removing duplicates?

To go off on a bit of a tangent, this got me thinking about how we have to be careful dealing with multiple pieces of Endpoint metadata of the same type going forward. We can no longer assume that conventions will be able to "see" duplicates because of the way route groups add metadata. Everything that wants to read additive metadata should be prepared for multiple pieces of metadata. This makes adding metadata easier, and I think more people are going to add metadata than read metadata.

See #41779 for an example of this. It's not really related to this PR but your comment got me thinking about this @BrennanConroy.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that it is additive, however, thinking more about it, before my change we were not generating duplicates here.

results[apiResponseType.StatusCode] = apiResponseType;

And I feel the right thing to do is processing in the following ordering:

  1. Process the metadata
  2. Process the attributes, overwriting any status code that we have set already (keep the users desired behavior)

@BrennanConroy @halter73 is that make sense?

Copy link
Member

Choose a reason for hiding this comment

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

I think it makes sense to prefer attributes applied to the action directly over metadata provided by the response type given a conflict for a given status code. Hopefully this never happens in practice.

@brunolins16 brunolins16 merged commit c983855 into dotnet:main May 25, 2022
@ghost ghost added this to the 7.0-preview6 milestone May 25, 2022
@brunolins16 brunolins16 deleted the brunolins16/issues/41421 branch August 2, 2022 20:47
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow parameter and return types of route handler delegates (API Controllers) to contribute to endpoint metadata
4 participants