Skip to content
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

Support binding to IFormFile in minimal routing APIs #34303

Closed
davidfowl opened this issue Jul 13, 2021 · 10 comments
Closed

Support binding to IFormFile in minimal routing APIs #34303

davidfowl opened this issue Jul 13, 2021 · 10 comments
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc Docs This issue tracks updating documentation feature-minimal-actions Controller-like actions for endpoint routing
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Jul 13, 2021

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

Today we don't have any native support for binding to IFormFiles as part of the incoming request. This is convenient for applications that want to do file uploads.

Describe the solution you'd like

app.MapGet("/upload", async (IFormFile file) =>
{
    var uploads = Path.Combine(uploadsPath, file.FileName);
    using var fileStream = File.OpenWrite(uploads);
    using var uploadStream = file.OpenReadStream();
    await uploadStream.CopyToAsync(fileStream);
});

Additional context

  • Should we support IFormFileCollection?
  • Would we do matching by name? (yes)

Without this, it will look like this:

app.MapGet("/upload", async (HttpRequest req) =>
{
    if (!req.HasFormContentType)
    {
        return Results.BadRequest();
    }

    var form = await req.ReadFormAsync();
    var file = form.Files["file"];

    if (file is null)
    {
        return Results.BadRequest();
    }

    var uploads = Path.Combine(uploadsPath, file.FileName);
    using var fileStream = File.OpenWrite(uploads);
    using var uploadStream = file.OpenReadStream();
    await uploadStream.CopyToAsync(fileStream);

    return Results.NoContent();
});
@davidfowl davidfowl added the feature-minimal-actions Controller-like actions for endpoint routing label Jul 13, 2021
@rafikiassumani-msft rafikiassumani-msft added this to 6-0-rc1 All in Minimal APIs 6.0 Jul 19, 2021
@rafikiassumani-msft rafikiassumani-msft added this to the Backlog milestone Jul 19, 2021
@ghost
Copy link

ghost commented Jul 19, 2021

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.

@rafikiassumani-msft rafikiassumani-msft added help wanted Up for grabs. We would accept a PR to help resolve this issue and removed help wanted Up for grabs. We would accept a PR to help resolve this issue labels Jul 19, 2021
@rafikiassumani-msft rafikiassumani-msft removed this from Need review in Minimal APIs 6.0 Jul 19, 2021
@jchannon
Copy link
Contributor

jchannon commented Aug 4, 2021

The below API would be good too, to handle more than one file at a time.

app.MapGet("/upload", async (IEnumerable<IFormFile> files) =>
{
    //process the files 
});

@bradygaster
Copy link
Member

I worked this up today in my side-by-side comparison solution and was sad to come across this feature parity gap.

Whilst this works fine with Web API:

[HttpPost]
[Route("/upload")]
public async Task UploadFile(IFormFile file, CancellationToken token)
{
    using (var stream = System.IO.File.OpenWrite("upload.txt"))
    {
        await file.CopyToAsync(stream);
    }
}

A similar approach fails to work in minimal API:

app.MapPost("/upload", async(IFormFile file, CancellationToken token) =>
{
    using (var stream = System.IO.File.OpenWrite("upload.txt"))
    {
        await file.CopyToAsync(stream);
    }
});

I'd like to see how we get to party. Or maybe add an analyzer when folks do this to provide guidance to the alternative approach in this issue.

@davidfowl
Copy link
Member Author

The below API would be good too, to handle more than one file at a time.

@jchannon We would support IFormFileCollection.

The other thing missing here is [FromFile("name")] for completeness we'd want to add metadata to let you specify the name of the file entry.

The other piece of work is updating the API explorer to set the right request content type if we support file upload.

@martincostello
Copy link
Member

Would it also make sense to support IFormCollection here too (via ReadFormAsync() on the request), with IFormFile and IFormFileCollection being "specialisms" of that support?

@martincostello
Copy link
Member

I've put up a draft to try and get this working in #35158.

@davidfowl
Copy link
Member Author

davidfowl commented Aug 8, 2021

We didn't do forms because we haven't thought the CSRF/Antiforgery features

@davidfowl
Copy link
Member Author

@halter73 This is done right?

@davidfowl davidfowl modified the milestones: Backlog, .NET 7 Planning Dec 9, 2021
@ghost
Copy link

ghost commented Dec 9, 2021

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.

@martincostello
Copy link
Member

@davidfowl Yep PR was merged a few weeks ago. I was going to do some E2E validation on it with Swagger to check it over, but the installer repo's been a bit sad over the last few weeks ingesting the SDK for 7.

@ghost ghost locked as resolved and limited conversation to collaborators Jan 9, 2022
@BrennanConroy BrennanConroy added the Docs This issue tracks updating documentation label Feb 3, 2022
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc Docs This issue tracks updating documentation feature-minimal-actions Controller-like actions for endpoint routing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants