dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.46k stars 10.03k forks source link

Issue with multipart/form-data post and json.net deserializtion (using interfaces) #4868

Open mbonatsos opened 6 years ago

mbonatsos commented 6 years ago

We use the aspnetcore 2.

We have configured JsonOptions in startup like this.

services.AddMvc().AddJsonOptions(options =>
            {
                options.SerializerSettings.ContractResolver = new DefaultContractResolver();
                options.SerializerSettings.Converters = new List<JsonConverter> {
                    new JsonDocumentConverter(),
                    new JsonDocumentMetaConverter(),
                    //new Bll.FileRecords.JsonDocumentMetaListConverter(),
                };
            });

public class JsonDocumentConverter : CustomCreationConverter<IDocument>
    {
        public override IDocument Create(Type objectType)
        {
            return new LocalDocument();
        }
    }
public class JsonDocumentMetaConverter : CustomCreationConverter<IDocumentMeta>
    {
        public override IDocumentMeta Create(Type objectType)
        {
            return new LocalDocumentMeta();
        }
    }

In our controller we have 2 methods

  1. The first one is called PostUrlencoded in which we make a http post request with content-type : application/x-www-form-urlencoded.
    [HttpPost("PostUrlencoded")]
    public async Task<IActionResult> PostUrlencoded([FromBody]IDocument document)
    { /*...*/ }

    2.. The second one is called PostΜultipart in which we make a http post request with content-type : multipart/form-data.

    [HttpPost("PostΜultipart")]
    public async Task<IActionResult> PostΜultipart([FromForm]IDocument document)
    { /*...*/ }

The first method works fine, and the deserialization works fine. In the second method deserialization fails. The error is: Model bound complex types must not be abstract or value types and must have a parameterless constructor.

That's because the JsonOptions isn't applied.

When we remove the jsonOptions from the startup.cs, we get the same error in both cases.

tangdf commented 6 years ago

If you use the Fromform attribute, the model's binding is not using JSON.NET.

mbonatsos commented 6 years ago

Why is this happening?

The http request is sent like this

--d76a5e72-cf14-4a63-ab96-44b45457a6a5
Content-Type: application/json; charset=utf-8
Content-Disposition: form-data; name=document
{...json..}

Shouldn't this be deserialized?

mkArtakMSFT commented 6 years ago

@mbonatsos , can you please share a sample project with us to reproduce the issue?

tangdf commented 6 years ago

https://docs.microsoft.com/en-us/aspnet/core/mvc/models/model-binding#customize-model-binding-behavior-with-attributes [FromHeader], [FromQuery], [FromRoute], [FromForm]: Use these to specify the exact binding source you want to apply. [FromBody]: Use the configured formatters to bind data from the request body. The formatter is selected based on content type of the request.

mbonatsos commented 6 years ago

I've created a sample project for you to see the issues and the workarounds. In my opinion the modelbinder solution is not clean.

The mvc framework should deserialize a model from some data that is posted (http), regardless what the post request type is "multipart/form-data" or "application/x-www-form-urlencoded".

Sample.zip

mkArtakMSFT commented 6 years ago

@dougbu , can you please investigate this?

dougbu commented 6 years ago

The JSON input formatter accepts application/json, text/json and application/*+json. It does not support any multipart content types by default.

To support multipart and input formatters together, need to position the Body stream after the boundary for the appropriate section. See https://github.com/aspnet/Entropy/blob/dev/samples/Mvc.FileUpload/Controllers/FileController.cs for example. (This isn't simple. MVC helps most when combining files and application/x-www-form-urlencoded sections into a multipart/form-data submission. That is the normal use of multipart/form-data.)

As a smaller detail, also need to update the JsonInputFormatter because it will check the overall content type, not that of the "current" section. E.g. in your ConfigureServices(IServiceCollection services):

services.AddMvc(options =>
{
    var jsonInputFormatter = options.InputFormatters.OfType<JsonInputFormatter>().First();
    jsonInputFormatter.SupportedMediaTypes.Add("multipart/form-data");
};
dougbu commented 6 years ago

Clearing the investigate label.

dougbu commented 6 years ago

I probably misunderstood the concern, focusing on using input formatters with multipart requests.

The sample contains a number of issues:

xrkolovos commented 6 years ago

I believe and would expect the mvc framework to deserialize this http request, with out any other configuration. I would also expect [FromForm] working in the same way with [FromBody] for the deserialization of posted data. I mean that if i post this

--d76a5e72-cf14-4a63-ab96-44b45457a6a5
Content-Type: application/json; charset=utf-8
Content-Disposition: form-data; name=document
{...json..}

MvcJsonOptions should be used/respected. Is confusing not to work there.

I also think that the implementation with IModelBindingProvider is not so clean.

xrkolovos commented 6 years ago

I would like to see some more opinions from other team members.

mkArtakMSFT commented 6 years ago

Thank you for your feedback. We're closing this issue as the behavior discussed seems to be by design.

xrkolovos commented 6 years ago

I am not conviced that the behavior should be this. I coundm't understand the arguments behind this decision.

I always hear from the members of the asp.net team in confernces and videos that the framework is openly developed, not only open source.

I ask for more info about this and you close the issue not fair.

joosera commented 6 years ago

I was hoping this would have worked with either [FromForm] or automatically. Is there any reason why it doesn't? I can send multiple IFormFile in a similar way and they get picked up without problems, why doesn't this?

------WebKitFormBoundaryuBv1RMQSgoCHMWae
Content-Disposition: form-data; name="news"
Content-Type: application/json

{"title":"title1","text":"text1"}
public async Task<IActionResult> PostNews([FromForm]News news, IFormFile attachment = null, IFormFile image = null)
{
...
}
joosera commented 6 years ago

I guess I can just do something like this, but this feels REALLY clunky.

public async Task<IActionResult> PostNews(IFormFile news, IFormFile attachment = null, IFormFile image = null)
{
    if (news != null && news.ContentType == "application/json")
    {
        News newsObject = null;

        var js = new JsonSerializer();
        using (var ms = new MemoryStream())
        using (var sr = new StreamReader(ms))
        using (var jtr = new JsonTextReader(sr))
        {
            await news.CopyToAsync(ms);
            ms.Seek(0, SeekOrigin.Begin);
            newsObject = js.Deserialize<News>(jtr);
        }
    }
}
xrkolovos commented 6 years ago

I knew someone else would say something about this. I expect more reactions. It was pretty authoritarian the way this was closed.

mkArtakMSFT commented 6 years ago

Reopening this issue as there seems to be some customer interest in this being a first-class experience. Will track this as part of the 3.0 milestone.

rasmuschristensen commented 6 years ago

I guess what we are looking for is something similar to this https://stackoverflow.com/questions/41367602/upload-files-and-json-in-asp-net-core-web-api

Where we want the parameter in [FromBody] to be parsed using a jsonbinding/serialization.

foconnor-DS commented 5 years ago

Was refactoring some code, and found my old JsonModelBinder class. Scratched my head on why I would need that, after all it's {currentYear}. Figured I could delete it...

I was wrong.

We could really use this.

For everyone else still waiting, I've switched to Bruno's Nuget: https://github.com/BrunoZell/JsonModelBinder

khellang commented 4 years ago

@pranavkm Did you have a look at this? I'm wondering what it would take to get this scenario supported out-of-the-box and whether it would be a good PR candidate?

pranavkm commented 4 years ago

I haven't, but we wanted to look at improvements in the multipart area. I'll bring this up in our planning, we should at the very least get a design out for this that we'd accept a PR for.

ghost commented 4 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

MaxxDelusional commented 4 years ago

Whelp, I just wasted an afternoon by assuming this was possible.

Requests matching actions with [FromForm] parameters must contain an application/form-url-encoded body or a multipart/form-data body with an application/x-www-form-urlencoded sectoin.

I have an application/x-www-form-urlencoded section in my request, but the model binder still does not work for me. This is with standard form content (no json).

Simon-Gregory-LG commented 4 years ago

Whelp, I just wasted an afternoon by assuming this was possible.

Requests matching actions with [FromForm] parameters must contain an application/form-url-encoded body or a multipart/form-data body with an application/x-www-form-urlencoded sectoin.

I have an application/x-www-form-urlencoded section in my request, but the model binder still does not work for me. This is with standard form content (no json).

Ditto - would be great to finally make json + IFormFile multipart requests a first class citizen

pranavkm commented 4 years ago

Yup it's on our radar. We'd hoped to address it in 5.0 but unfortunately weren't able to fit it.

ghost commented 4 years ago

Thanks for contacting us. We're moving this issue to the Next sprint planning milestone for future evaluation / consideration. We will evaluate the request when we are planning the work for the next milestone. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

ghost commented 3 years ago

We've moved this issue to the Backlog milestone. This means that it is not going to be worked on for the coming release. We will reassess the backlog following the current release and consider this item at that time. To learn more about our issue management process and to have better expectation regarding different types of issues you can read our Triage Process.

dazinator commented 3 years ago

Multipart support is definately lacking! I had a simple use case: A form that has a text input, and a file input, must be submitted and validated together. Pretty common not to want to split this into 2 separate roundtripped requests.

I spent 4+ hours trying to get this right (using .net 5.0) before giving up, locating this issue and switching to https://www.nuget.org/packages/JsonModelBinder/ which works.

It's supposedly possible to add an IFormFile property to a model and model binding should just magically work. I was unsuccessful in that endeavour. Read this blog post and look at the comments below for examples of where people are struggling with this use case: https://thomaslevesque.com/2018/09/04/handling-multipart-requests-with-json-and-file-uploads-in-asp-net-core/

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 7 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. 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.

Simon-Gregory-LG commented 2 years ago

OK, so sounds like this is the time window you need re-convincing on this.

This is required any time you need to send files to a RestAPI with additional structured data. This is a common problem for which multipart requests are the accepted solution. Except this is not natively supported in ASPNET and IMO its become such a standard now that it should be.

There are workarounds as people have suggested, but these workarounds are unsupported and are often broken in new versions of ASPNET Core or with certain more complex binding scenarios and need re-adapting. A tighter integration with ASPNET Core would provide a more optimal, flexible and future proof implementation.

This is also now a first class feature in the Open Api spec:

https://swagger.io/docs/specification/describing-request-body/multipart-requests/

However, the lack of a first class implementation in ASPNET Core means that for example it is not so easy for Swashbuckle to natively implement this spec functionality either. So a further custom implementation is also required to produce the correct output for OpenApI.

There are even third party implementations available, like this one that has 25k downloads (and the Binder one mentioned by somebody else above has nearly 250k):

https://www.nuget.org/packages/Swashbuckle.AspNetCore.JsonMultipartFormDataSupport/1.4.0

But all of these workarounds usually involve some kind of additional attribute and struggle sometimes with more complex binding requests. Until there's an official standard attribute then this functionality is always going to be held back.

These workarounds can be made to work, but a lack of official support for this means that there is always a barrier to upgrading ASPNET Core version and the Open API libraries as extensive testing and remediation needs to be performed each and every time.

If anybody else is still monitoring this thread then please weigh in on the conversation to show this is still needed (and share on any appropriate groups). Lets get this finally properly implemented!

aardvark79 commented 2 years ago

I agree with the other commenters. I think it's a definite miss that not all multipart request scenarios are supported out-of-the-box. What is the point of model binding if sometimes I have to go directly to the Request to get some additional data I may be looking for? I should be able to create a controller method with a signature consisting of any combination of binding hints, ([FromBody], [FromForm], [FromRoute], etc) and, as long as the request is to spec, they should all be deserialized without any fragile custom work.

knightmeister commented 2 years ago

As other commenters have said, there is nothing exotic about this issue. It makes no sense to have additional round trips to the server in order to include metadata about a file.

Particularly given API design with a restful approach, form/multipart is the correct way to include metadata (whether JSON or otherwise) along with a file. Really hope this makes the .NET 7 release.

Anyone who wants to hack in this functionality to you can do what you need to do, check this on SO: https://stackoverflow.com/a/46344854/366038

sliekens commented 2 years ago

Bumping this because I have a real need to upload a file along with a complex JSON object containing file metadata in a single transaction. The metadata cannot be expressed as simple key=value pairs. I also really don't want to make it a two-phase commit just because the framework won't let me do it atomically.

ghost commented 2 years ago

Thanks for contacting us.

We're moving this issue to the .NET 8 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. 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.

sliekens commented 12 months ago

I hope this can be prioritized for the next milestone.

ElliotRG commented 11 months ago

Still can't believe this hasn't been addressed as the prioritization it deserves and the demand it has had so far, after 5 years.

As others said, this is a common scenario and no workarounds or custom unstable solutions should be needed, but a consistent framework-native should.

DaveNatalieTripArc commented 11 months ago

I ran into this problem when creating a ticketing system. I wanted a user to be able to describe the details of their issue, and attach files while creating the support request.

Ideally this would be all one API call, so that the notification system could send something like, "Dave created a new request with 1 attachment", and include a link to the attachment.

I could break this into multiple API calls, where the ticket is first created, and then I do a second call to upload the attachment, but now the notification system is more complex. I'd need to know that an attachment is expected, and delay sending the notification. But what happens if the follow-up file never arrives? No notification would be sent.

Alternatively, I could send two notifications (1 for ticket created, and 1 for file attached), but this is annoying for users.

These workarounds add unnecessary complexity. It would be much easier for my API to accept both bindable json data and files at the same time.

sliekens commented 11 months ago

It looks like @pranavkm took a stab at solving this in #22373 but then... what happened?