Improve Model Binding for HTTP Request Body as Stream

[commonsensesoftware]

Is there an existing issue for this?

• I have searched the existing issues

Is your feature request related to a problem? Please describe the problem.

When building a web API, authors occasionally need to support file uploads. There are two possible ways this can be achieved today:

1. Process HttpContext.Request.Body directly
2. Use model binding with IFormFile or IFormFileCollection

This issue exists for traditional controller actions as well as Minimal APIs

Option 1 - Process Request Body Directly

This implementation will correctly upload a file; however, HttpRequest has special binding semantics and will not modeled by the API Explorer, making it impossible to use with OpenAPI without additional work on the developer's part. Even if HttpRequest was explored, it would be incorrect because the intent is to document the HTTP body and not the entire HTTP request.

app.MapPost(
    "order/import",
    async (
        HttpRequest request,
        [FromHeader( Name = "Content-Disposition" )] string contentDisposition,
        CancellationToken cancellationToken) =>
    {
    	if (!ContentDispositionHeaderValue.TryParse(contentDisposition, out var header) ||
    		!header.FileName.HasValue)
        {
            return Results.BadRequest();
        }

        var source = request.Body;
        var path = Path.Combine( Path.GetTempPath(), "Quarantine", header.FileName.Value );
        using var destination = new FileStream( path, FileMode.Create );

        await source.CopyToAsync( destination, cancellationToken );
        await destination.FlushAsync( cancellationToken );
        destination.Seek(0L, SeekOrigin.Begin);

        var id = await GetOrderId(id, cancellationToken);
        var scheme = request.Scheme;
        var host = request.Host;
        var location = new Uri( $"{scheme}{Uri.SchemeDelimiter}{host}/order/{id}" );

        return Results.Created( location, default );
    })
    .Accepts<Stream>( "application/pdf" )
    .Produces( 201 );

Option 2 - Use IFormFile or IFormFileCollection

This is a common approach which uses multipart/form-data as the media type to upload one or more files. As specified in RFC 7578, this approach is intended for HTML forms. When a HTML <form> sends a POST back to the server with its key/value pairs, it needs a way to include other content, such as files, in a distinctly separate way - e.g. multipart.

While this approach can work for a web API, it is unnecessary. A web API should not have to use HTML semantics to upload a file. Moreover, this approach requires clients to format request bodies in a particular way thus changing the wire protocol of the API.

The following is a completely valid file upload:

POST /message HTTP/2
Host: my.microblog.com
Content-Type: text/plain
Content-Length: 42
Content-Disposition: inline; filename="infomericial.txt"

I'm a Pepper! Wouldn't you like to be a Pepper too?

The following is also a valid file upload:

POST /message HTTP/2
Host: my.microblog.com
Content-Type: application/json
Content-Length: 42
Content-Disposition: inline; name="infomericial"

{"message": "I'm a Pepper! Wouldn't you like to be a Pepper too?"}

Describe the solution you'd like

Any HTTP request with a body can potentially be considered a file upload. "There can be only one" parameter bound to the HTTP request body.

For traditional, controller actions, an argument defined as [FromBody] Stream body, where the name body is irrelevant, should be handled by BodyModelBinderProvider and BodyModelBinder. The current implementation does not allow zero InputFormatter instances, but it should. Binding to Stream should be considered a special case and should be bound when the following are true:

1. ModelBinderProviderContext.BindingInfo.BindingSource.CanAcceptDataFrom(BindingSource.Body)
2. context.Metadata.ModelType.Equals(typeof(Stream))

InputFormatter instances need not be considered. The onus of understanding and processing the content is on the developer who makes a conscience decision to use this setup. A developer can specify which file types are allowed by declaratively using [Consumes] or imperatively asserting the content.

For Minimal APIs, a method parameter defined as Stream body or [FromBody] Stream body should be sufficient. A developer can specify which file types are allowed by declaratively using Accepts or imperatively asserting the content.

Additional context

Related to #4868

There should also be better support multipart/*; specifically for file uploads. This feature request has general applicability beyond file uploads so I'll track that as a separate issue.

[davidfowl]

In .NET 7 Minimal APIs supports binding the request body #38153 and IFormFile/IFormFileCollection #35158.

[commonsensesoftware]

@davidfowl Great news! Thanks for clarifying. If there is parity with controller actions, then I think this issue could be wrapped up. From my cursory observation of the code, the changes would be made in BodyModelBinderProvider and either return a different, simpler IModelBinder for a non-empty body Stream or update the existing BodyModelBinder. If that is amenable to you, I don't mind creating a PR for it.

[davidfowl]

That sounds right. We don't support this for today MVC? We should probably make sure this is consistent for 7.0.

cc @brunolins16

[brunolins16]

@davidfowl I quickly look at how Minimal API supports Stream/PipeReader, .NET 7, and looks like we do support (Stream body) but something like ([FromBody]Stream body) will try deserialize the body.

System.NotSupportedException: Deserialization of types without a parameterless constructor, a singular parameterized constructor, or a parameterized constructor annotated with 'JsonConstructorAttribute' is not supported. Type 'System.IO.Stream'. Path: $ | LineNumber: 0 | BytePositionInLine: 1.

Is that expected and we should do exactly the same thing for MVC?

[davidfowl]

@davidfowl I quickly look at how Minimal API supports Stream/PipeReader, .NET 7, and looks like we do support (Stream body) but something like ([FromBody]Stream body) will try deserialize the body.

We need to fix that behavior 😄

[brunolins16]

We need to fix that behavior 😄

#41486

[commonsensesoftware]

@brunolins16 to be clear, I believe that @davidfowl was saying:

• Implicit model binding should work for Stream body in Minimal APIs
• Explicit model binding should work for [FromBody] Stream body in Minimal APIs
• MVC should have parity with these behaviors

@davidfowl do you want both approaches supported in MVC or just with an explicit [FromBody]? I think most could live with the explicit path and that would be on par with how other intrinsic model binding works (ex: [FromBody] JsonElement body). It's hard to imagine how you'd ever have multiple Stream parameters, but maybe 🤷🏽‍♂️ .

[brunolins16]

@commonsensesoftware 👍. The issue that I've created is just for "Explicit model binding should work for [FromBody] Stream body in Minimal APIs" (BTW I will use it as issue title 😂).

For Mvc we should be able to track from this current issue or maybe we can create a new of just to make it clear.

[commonsensesoftware]

@brunolins16 no problem. You're free to track it with as many different issues as you like. I just want to make sure both Minimal APIs and MVC are covered for parity. Currently (e.g. in .NET 6.0), neither are supported. I opened this issue with the intent that both are/should be supported. 😉

[davidfowl]

@davidfowl do you want both approaches supported in MVC or just with an explicit [FromBody]? I think most could live with the explicit path and that would be on par with how other intrinsic model binding works (ex: [FromBody] JsonElement body). It's hard to imagine how you'd ever have multiple Stream parameters, but maybe 🤷🏽‍♂️ .

For Stream I think implicit support should work as well.

[captainsafia]

Triage: Bruno shared the scope of the problem here. Assigning to @brunolins16 to split up the work into new issues here.

[ghost]

Thanks for contacting us.
We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. Because it's not immediately obvious that this is a bug in our framework, we would like to keep this around to collect more feedback, which can later help us determine the impact of it. We will re-evaluate this issue, during our next planning meeting(s).
If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues.
To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[brunolins16]

I have split the work and we will use this issue to track all of them:

• Support binding request body as Stream/ReadOnlySequence<byte> maybe ReadOnlyMemory<byte>/ReadOnlySpan<byte> #38153
• Explicit binding source should work for [FromBody] Stream body in Minimal APIs #41486
• Support binding request body as Stream in Controller Actions #41540

[brunolins16]

@commonsensesoftware Closing this issue since the working for Minimal is completed and we have a follow up issue for Controller. Feel free to reopen this if needed.