Skip to content

Improve Minimal APIs support for request media types & body schema/shape #35082

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

Closed
DamianEdwards opened this issue Aug 5, 2021 · 6 comments · Fixed by #35230
Closed

Improve Minimal APIs support for request media types & body schema/shape #35082

DamianEdwards opened this issue Aug 5, 2021 · 6 comments · Fixed by #35230
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-minimal-actions Controller-like actions for endpoint routing
Milestone

Comments

@DamianEdwards
Copy link
Member

DamianEdwards commented Aug 5, 2021

(Partially an extension to work happening in #34906)

Right now, when minimal APIs are added to routing they are not constrained with a MatcherPolicy based on which media types their logic expects requests to format the request body with. This results in an exception being thrown when a request is sent with a body formatted as anything other than "application/json", rather than correctly returning an HTTP status code of 415 HTTP Unsupported Media Type based on the stated media type in the request's Content-Type header.

Minimal APIs parameter binding for complex types only supports the "application/json" media type when parsing from the request body, however a minimal API method can accept HttpContext or HttpRequest and do its own request body parsing to support whichever format it wishes. In this case, there needs to be a way for the method to control the media types and request body schema that:

  1. Get reported to API descriptions (e.g. OpenAPI)
  2. Get used to configure the routing MatcherPolicy

Note that there is currently no support in ASP.NET Core for defining incoming request body schema via a Type such that it is populated in ApiExplorer and subsequently by OpenAPI libraries like Swashbuckle and nSwag. The IApiRequestMetadataProvider interface only allows configuration of incoming media types and the IApiRequestFormatMetadataProvider interface is designed to support MVC's "formatters" feature.

Proposal

  1. IApiRequestMetadataProvider should have a new property added that allows the specification of a Type to represent the request body shape/schema: Type? Type => null;
  2. ASP.NET Core should add a ConsumesRequestTypeAttribute (the incoming equivalent of ProducesResponseTypeAttribute) that implements IApiRequestMetadataProvider. It can describe supporting incoming media types and optionally a Type that represents the incoming request body shape/schema. It might be beneficial to have it derive from ConsumesAttribute.
  3. Minimal APIs should add a ConsumesRequestTypeAttribute instance to the endpoint metadata for methods with complex parameters that are bound from the request body (either implicitly or explicitly via [FromBody])
  4. Minimal APIs should be able to add their own ConsumesRequestTypeAttribute to their endpoint metadata, either by decorating the method/lambda with the ConsumesRequestTypeAttribute, or via new Accepts(string contentType) or Accepts<TRequest>(string contentType) methods on MinimalActionEndpointConventionBuilder. This should override any consumes metadata that was added via the 1st proposal (i.e. the 1st proposal is likely implemented as fallback logic in the endpoint builder).
  5. Minimal APIs should add a ConsumesMatcherPolicy to the route for a mapped method based on the media types declared via the IApiRequestMetadataProvider instance in the matching endpoint's metadata (note ConsumesRequestTypeAttribute implements IApiRequestMetadataProvider). This will result in a 415 response being sent to requests to that route with an unsupported Content-Type media type.

API additions:

namespace Microsoft.AspNetCore.Builder
{
  public static class OpenApiEndpointConventionBuilderExtensions
  {
+    public static MinimalActionEndpointConventionBuilder Accepts(this MinimalActionEndpointConventionBuilder builder, string contentType, params string[] otherContentTypes);
+    public static MinimalActionEndpointConventionBuilder Accepts<TRequest>(this MinimalActionEndpointConventionBuilder builder, string? contentType = null, params string[] otherContentTypes);
+    public static MinimalActionEndpointConventionBuilder Accepts(this MinimalActionEndpointConventionBuilder builder, Type requestType, string? contentType = null, params string[] otherContentTypes);
  }
}
namespace Microsoft.AspNetCore.Http
{
 public static partial class RequestDelegateFactory 
 {
+    public static RequestDelegateResult Create(Delegate action, RequestDelegateFactoryOptions? options = null);
+    public static RequestDelegateResult Create(MethodInfo methodInfo, Func<HttpContext, object>? targetFactory = null, RequestDelegateFactoryOptions? options = null);

  }
}

+namespace Microsoft.AspNetCore.Http
+{
+   /// <summary>
+    /// The result of creating a <see cref="RequestDelegate" /> from a <see cref="Delegate" />
+    /// </summary>
+   public sealed class RequestDelegateResult
+    {
+        /// <summary>
+        /// Creates a new instance of <see cref="RequestDelegateResult"/>.
+        /// </summary>
+    public RequestDelegateResult(RequestDelegate requestDelegate, IReadOnlyList<object> metadata);

+        /// <summary>
+       /// Gets the <see cref="RequestDelegate" />
+        /// </summary>
+       /// <returns>A task that represents the completion of request processing.</returns>
+        public RequestDelegate RequestDelegate { get; init; }

+        /// <summary>
+        /// Gets endpoint metadata inferred from creating the <see cref="RequestDelegate" />
+        /// </summary>
+        public IReadOnlyList<object> EndpointMetadata { get; init; }
+    }

+ }
+namespace Microsoft.AspNetCore.Http.Metadata
+{
+    /// <summary>
+    /// Metadata that specifies the supported request content types.
+    /// </summary>
+    public sealed class AcceptsMetadata : IAcceptsMetadata
+    {
+        /// <summary>
+        /// Creates a new instance of <see cref="AcceptsMetadata"/>.
+        /// </summary>
+        public AcceptsMetadata(string[] contentTypes);

+        /// <summary>
+        /// Creates a new instance of <see cref="AcceptsMetadata"/> with a type.
+        /// </summary>
+        public AcceptsMetadata(Type? type, string[] contentTypes);
   
+        /// <summary>
+        /// Gets the supported request content types. 
+        /// </summary>
+        public IReadOnlyList<string> ContentTypes { get; }


+       /// <summary>
+       /// Accepts request content types of any shape. 
+        /// </summary>
+        public Type? RequestType { get; }
+    }
+}

+namespace Microsoft.AspNetCore.Http.Metadata
+{
+    /// <summary>
+    /// Interface for accepting request media types.
+    /// </summary>
+    public interface IAcceptsMetadata
+    {
+        /// <summary>
+        /// Gets a list of request content types.
+        /// </summary>
+        IReadOnlyList<string> ContentTypes { get; }

+        /// <summary>
+        /// Accepts request content types of any shape. 
+        /// </summary>
+        Type? RequestType { get; }
+    }
+}

Accepts Extension method Usage

app.MapPost("/todos/xmlorjson", async (HttpRequest request, TodoDb db) =>
    {
        string contentType = request.Headers.ContentType;

        var todo = contentType switch
        {
            "application/json" => await request.Body.ReadAsJsonAsync<Todo>(),
            "application/xml" => await request.Body.ReadAsXmlAsync<Todo>(request.ContentLength),
            _ => null,
        };

        if (todo is null)
        {
            return Results.StatusCode(StatusCodes.Status415UnsupportedMediaType);
        }

        if (!MinimalValidation.TryValidate(todo, out var errors))
            return Results.ValidationProblem(errors);

        db.Todos.Add(todo);
        await db.SaveChangesAsync();

        return AppResults.Created(todo, contentType);
    })
    .WithName("AddTodoXmlOrJson")
    .WithTags("TodoApi")
    .Accepts<Todo>("application/json", "application/xml")
    .Produces(StatusCodes.Status415UnsupportedMediaType)
    .ProducesValidationProblem()
    .Produces<Todo>(StatusCodes.Status201Created, "application/json", "application/xml");

Request DelegateResult Usage

     var requestDelegateResult = RequestDelegateFactory.Create(action, options);

    var builder = new RouteEndpointBuilder(
        requestDelegateResult.RequestDelegate,
        pattern,
        defaultOrder)
    {
        DisplayName = pattern.RawText ?? pattern.DebuggerToString(),
    };

    //Add add request delegate metadata 
    foreach(var metadata in requestDelegateResult.EndpointMetadata)
    {
        builder.Metadata.Add(metadata);
    }
 

AcceptsMetadata Usage Example

    builder.WithMetadata(new AcceptsMetadata(requestType, GetAllContentTypes(contentType, additionalContentTypes)));
@DamianEdwards DamianEdwards added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-minimal-actions Controller-like actions for endpoint routing labels Aug 5, 2021
@rafikiassumani-msft rafikiassumani-msft 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 Aug 6, 2021
@halter73
Copy link
Member

halter73 commented Aug 6, 2021

Accepts() should throw if there is a [FromBody] param explicit or implicit and it's not just "application/json". We could also look at an analyzer. Unfortunately, I don't think there's a great way to query this at the moment.

@DamianEdwards
Copy link
Member Author

@halter73 perhaps it could throw in the builder rather than in Accepts, once all the metadata is present?

@rafikiassumani-msft rafikiassumani-msft added this to the 6.0-rc1 milestone Aug 6, 2021
@rafikiassumani-msft rafikiassumani-msft self-assigned this Aug 6, 2021
@DamianEdwards DamianEdwards changed the title Improve Minimal APIs support for request media types Improve Minimal APIs support for request media types & body schema/shape Aug 12, 2021
@rafikiassumani-msft rafikiassumani-msft added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Aug 17, 2021
@ghost
Copy link

ghost commented Aug 17, 2021

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

  • The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
  • The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
  • Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

@pranavkm
Copy link
Contributor

pranavkm commented Aug 17, 2021

 public static class OpenApiEndpointConventionBuilderExtensions
 {
+    public static MinimalActionEndpointConventionBuilder Accepts<TRequest>(
+        this MinimalActionEndpointConventionBuilder builder, 
+        string contentType, 
+        params string[] otherContentTypes);

+    public static MinimalActionEndpointConventionBuilder Accepts(
+        this MinimalActionEndpointConventionBuilder builder, 
+        Type requestType, 
+        string contentType, 
+        params string[] otherContentTypes);
 }


* Remove `init` property setters. 
* Remove `<returns />` doc comment for property
* 
```diff
+   public sealed class RequestDelegateResult
+    {
+        /// <summary>
+        /// Creates a new instance of <see cref="RequestDelegateResult"/>.
+        /// </summary>
+       public RequestDelegateResult(RequestDelegate requestDelegate, IReadOnlyList<object> metadata);

+        /// <summary>
+        /// Gets the <see cref="RequestDelegate" />
+        /// </summary>
+        public RequestDelegate RequestDelegate { get; }

+        /// <summary>
+        /// Gets endpoint metadata inferred from creating the <see cref="RequestDelegate" />
+        /// </summary>
+        public IReadOnlyList<object> EndpointMetadata { get; }
+    }
  • Move the extenstion methods in to MVC, add ConsumesAttribute instead.
- ConsumesAttribute : IApiRequestMetadataProvider
+ ConsumesAttribute : IApiRequestMetadataProvider, IAcceptMetadata
{
     ConsumesAttribute(string[] contentTypes);
+   ConsumesAttribute(Type requestType, string contentType, params string[] contentTypes);

+   Type IApiRequestMetadataProvider.RequestType { get; set; }
}
+namespace Microsoft.AspNetCore.Http.Metadata
+{
+    /// <summary>
+    /// Interface for accepting request media types.
+    /// </summary>
+    public interface IAcceptsMetadata
+    {
+        /// <summary>
+        /// Gets a list of the allowed request content types. If the incoming request does not have a <c>Content-Type</c> with one of these values, the request will be rejected with a 415 response.
+        /// </summary>
+        IReadOnlyList<string> ContentTypes { get; }

+        /// <summary>
+        /// Gets the type being read from the request. 
+        /// </summary>
+        Type? RequestType { get; }
+    }
+}

Make AcceptsMetadata non-public / internal.

@pranavkm pranavkm added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews labels Aug 17, 2021
@davidfowl
Copy link
Member

We're missing the RequestDelegateFactory.Create changes

@rafikiassumani-msft
Copy link
Contributor

rafikiassumani-msft commented Aug 17, 2021

@davidfowl are you referring to this ( :

 namespace Microsoft.AspNetCore.Http
{
 public static partial class RequestDelegateFactory 
 {
+    public static RequestDelegateResult Create(Delegate action, RequestDelegateFactoryOptions? options = null);
+    public static RequestDelegateResult Create(MethodInfo methodInfo, Func<HttpContext, object>? targetFactory = null, RequestDelegateFactoryOptions? options = null);

  }
}

It's actually there.

rafikiassumani-msft added a commit that referenced this issue Aug 21, 2021
* add support for request media types

* change namespace for acceptsmatcher policy

* additional changes

* enable 415 when unsupported content type is provide

* add accepts extension method on minimalActions endpoint

* add IAcceptsMetadata to API description

* add empty content type test

* feat: add types for iacceptmetadata

* change requestdelegate factory to return metatdata

* clean RequestDelegateFactoryOptions.cs

* change request delegate to return requestdelegateresult type

* make apis property init only

* adding constructor to requestdelegatefactoryResult

* Fixups

* fix merge errors

* address pr comment

* fix test error

* remove options from params

* implements iacceptsMetadata

* fix test failures

* fix test failures

* move iacceptmetadata to shared source

* add acceptsmetadata shared code to mvc

* fix tests

* address pr comments

* address another comment

* nit

* fix duplicate media types

* fix test failures

Co-authored-by: Pranav K <[email protected]>
rafikiassumani-msft added a commit to rafikiassumani-msft/aspnetcore that referenced this issue Aug 21, 2021
…tnet#35230)

* add support for request media types

* change namespace for acceptsmatcher policy

* additional changes

* enable 415 when unsupported content type is provide

* add accepts extension method on minimalActions endpoint

* add IAcceptsMetadata to API description

* add empty content type test

* feat: add types for iacceptmetadata

* change requestdelegate factory to return metatdata

* clean RequestDelegateFactoryOptions.cs

* change request delegate to return requestdelegateresult type

* make apis property init only

* adding constructor to requestdelegatefactoryResult

* Fixups

* fix merge errors

* address pr comment

* fix test error

* remove options from params

* implements iacceptsMetadata

* fix test failures

* fix test failures

* move iacceptmetadata to shared source

* add acceptsmetadata shared code to mvc

* fix tests

* address pr comments

* address another comment

* nit

* fix duplicate media types

* fix test failures

Co-authored-by: Pranav K <[email protected]>
rafikiassumani-msft added a commit that referenced this issue Aug 21, 2021
dougbu added a commit that referenced this issue Aug 24, 2021
* [release/6.0-rc1] Update dependencies from dotnet/runtime dotnet/efcore (#35513)

* Set HttpSys read error log levels to debug #35490 (#35542)

Co-authored-by: Chris R <[email protected]>

* [release/6.0-rc1] Treat reference type parameters in oblivious nullability context as optional (#35526)

* Treat parameters in oblivious nullability context as optional

* Only apply fix for reference types

* Update optionality check in API descriptor

* Update check in BindAsync and Mvc.ApiExplorer test

* HTTP/3: Use new QuicStream.ReadsCompleted property in transport (#35483)

Co-authored-by: James Newton-King <[email protected]>

* HTTP/3: Fix incorrectly pooling aborted streams (#35441)

* [release/6.0-rc1] Binding support for 'bool' values with InputRadioGroup and InputSelect (#35523)

* Binding support for 'bool' values with InputRadioGroup and InputSelect (#35318)

* Update CodeCheck.ps1

Co-authored-by: Brennan <[email protected]>

* [release/6.0-rc1] Update dependencies from dotnet/efcore dotnet/runtime (#35558)

* Update dependencies from https://github.com/dotnet/efcore build 20210820.19

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.19

* Update dependencies from https://github.com/dotnet/efcore build 20210820.22

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.22

* Update dependencies from https://github.com/dotnet/efcore build 20210820.30

Microsoft.EntityFrameworkCore.Tools , dotnet-ef , Microsoft.EntityFrameworkCore , Microsoft.EntityFrameworkCore.SqlServer , Microsoft.EntityFrameworkCore.InMemory , Microsoft.EntityFrameworkCore.Relational , Microsoft.EntityFrameworkCore.Sqlite , Microsoft.EntityFrameworkCore.Design
 From Version 6.0.0-rc.1.21420.8 -> To Version 6.0.0-rc.1.21420.30

* Update dependencies from https://github.com/dotnet/runtime build 20210820.15

Microsoft.NETCore.Platforms , Microsoft.NETCore.BrowserDebugHost.Transport , Microsoft.NETCore.App.Runtime.win-x64 , Microsoft.Win32.SystemEvents , Microsoft.NET.Runtime.MonoAOTCompiler.Task , Microsoft.Internal.Runtime.AspNetCore.Transport , Microsoft.Extensions.Primitives , Microsoft.Extensions.Options.DataAnnotations , Microsoft.Extensions.Options.ConfigurationExtensions , Microsoft.NET.Runtime.WebAssembly.Sdk , Microsoft.Extensions.Options , Microsoft.Extensions.Logging.TraceSource , Microsoft.Extensions.Logging.EventSource , Microsoft.Extensions.Configuration.UserSecrets , Microsoft.Extensions.Configuration.Json , Microsoft.Extensions.Configuration.Ini , Microsoft.Extensions.Configuration.FileExtensions , Microsoft.Extensions.Configuration.EnvironmentVariables , Microsoft.Extensions.Configuration.CommandLine , Microsoft.Extensions.Configuration.Binder , Microsoft.Extensions.Configuration.Xml , Microsoft.Extensions.Configuration.Abstractions , Microsoft.Extensions.Caching.Memory , Microsoft.Extensions.Caching.Abstractions , Microsoft.Extensions.Configuration , Microsoft.Extensions.DependencyInjection , Microsoft.Extensions.DependencyInjection.Abstractions , Microsoft.Extensions.Logging.EventLog , Microsoft.Extensions.Logging.Debug , Microsoft.Extensions.Logging.Console , Microsoft.Extensions.Logging.Configuration , Microsoft.Extensions.Logging.Abstractions , Microsoft.Extensions.Logging , Microsoft.Extensions.Http , Microsoft.Extensions.Hosting.Abstractions , Microsoft.Extensions.Hosting , Microsoft.Extensions.HostFactoryResolver.Sources , Microsoft.Extensions.FileSystemGlobbing , Microsoft.Extensions.FileProviders.Physical , Microsoft.Extensions.FileProviders.Composite , Microsoft.Extensions.FileProviders.Abstractions , Microsoft.Extensions.DependencyModel , Microsoft.NETCore.App.Runtime.AOT.win-x64.Cross.browser-wasm , Microsoft.NETCore.App.Ref , System.Windows.Extensions , System.Threading.Channels , System.Text.Json , System.Text.Encodings.Web , System.ServiceProcess.ServiceController , System.Drawing.Common , System.DirectoryServices.Protocols , System.Diagnostics.EventLog , System.Diagnostics.DiagnosticSource , System.IO.Pipelines , System.Security.Permissions , System.Security.Cryptography.Xml , System.Security.Cryptography.Pkcs , System.Runtime.CompilerServices.Unsafe , System.Resources.Extensions , System.Reflection.Metadata , System.Net.Http.WinHttpHandler , System.Net.Http.Json
 From Version 6.0.0-rc.1.21420.7 -> To Version 6.0.0-rc.1.21420.15

Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>

* Improve Minimal APIs support for request media types #35082 (#35230) (#35579)

* add support for request media types

* [release/6.0-rc1] Update dependencies from dotnet/efcore dotnet/runtime (#35573)

Co-authored-by: dotnet-maestro[bot] <42748379+dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Chris R <[email protected]>
Co-authored-by: Safia Abdalla <[email protected]>
Co-authored-by: James Newton-King <[email protected]>
Co-authored-by: Mackinnon Buck <[email protected]>
Co-authored-by: Brennan <[email protected]>
Co-authored-by: dotnet-maestro[bot] <dotnet-maestro[bot]@users.noreply.github.com>
Co-authored-by: Rafiki Assumani <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Sep 20, 2021
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
@danmoseley danmoseley removed the old-area-web-frameworks-do-not-use *DEPRECATED* This label is deprecated in favor of the area-mvc and area-minimal labels label Jul 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-minimal-actions Controller-like actions for endpoint routing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants