Aspnetcore.docs: File Upoads - Master issue

[Rick-Anderson]

Moved from #5331

https://docs.microsoft.com/aspnet/core/mvc/models/file-uploads

• [x] Provide 2.0 code samples (address #5217)
• [x] Address #3989
• [ ] Address #4777 MOVED TO https://github.com/aspnet/AspNetCore.Docs/issues/12285
• [x] Address #4964
• [x] Address #6247
• [x] Address #6349
• [x] Address #6927
• [x] Address #7702
• [x] Address #7742
• [x] Address #8039
• [x] Address #8100
• [x] Address #8298
• [x] Address #8797
• [x] Address #9116
• [x] Address #9327
• [x] Address #9473
• [x] Address #10161
• [ ] Address #10537 wontfix ... sample+topic are going RP
• [x] Address #11898
• [x] Address #11949
• [ ] Address #12130 – Won't fix because we're not going to cover reading from Request.Form directly on the updates. If more devs ask about the approach, we can reconsider it later.
• [x] Address #12204
• [x] Address #6609
• [x] Address #6610
• [x] Address #6962
• [x] Address #11822

---

Document Details

⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.

• ID: 9a5cfa38-0e6c-4346-dafc-84ea5c513fcf
• Version Independent ID: c6787f0f-3e9a-6efb-0e6e-5ccb957d3f4a
• Content: File uploads in ASP.NET Core
• Content Source: aspnetcore/mvc/models/file-uploads.md
• Product: aspnet-core
• GitHub Login: @ardalis
• Microsoft Alias: riande

[guardrex]

@Rick-Anderson @wadepickett @scottaddie @tdykstra In addition to problems folks identified :point_up:😅, this will be an overhaul effort.

Because developers implement file uploads by scenario, the content should follow the same pattern with sections for cross-cutting concerns. I propose the following outline (exact section names are TBD) ...

• Introduction paragraph; Download samples link
• Security
• Storage scenarios (EF/dB, physical file, Azure Blob Storage)
• File upload scenarios (to dB w/EF Core and physical storage)
  
  
  
  • Buffered IFormFile (with validation)
  
  
  • Single file upload one control
  
  
  • Multiple file upload one control
  
  
  • Two file upload two control
  
  
  • Streaming (without validation)
  
  
• Server configuration (Kestrel, IIS, Apache, Nginx)
• Background queued file processing (long-range plan only: QueueBackgroundWorkItem should be offered by the framework for 3.0, so this is best placed in a post-3.0 issue for future work)
• Troubleshoot
• Additional resources

The sample app should follow the same pattern: One page for each scenario makes it easy for the reader to focus on the scenario that they plan to implement and easy to compare the scenarios SxS when looking at the sample app and the topic.

I'd like to get the RP sample off of the RP Movies tutorial sample app. They're bound to diverge over time; but more important, a tutorial example doesn't match the scenario approach that I think would work much better for this subject matter. Having the current sample based on that tutorial is just an artifact of when the file uploads topic was part of the tutorial. The RP file uploads topic isn't part of the tutorial any longer. There's no reason to continue using that sample app.

We don't need two topics for this content. One topic based on a RP sample app with a SxS MVC version of the sample app should do the trick. I think having an MVC sample is appropriate in this case. It will compose a little different given how the models are set up. The topic (probably) doesn't need to address it (other than its existence). It's a nice-to-have for MVC 🐈🐈🐈.

@tdykstra I'd like to place the topic at Web apps > Advanced in the TOC and aspnetcore/mvc/advanced physical path to go with the other advanced cross-cutting scenarios. I'll remove the two that we have today and point the old topics to the new, single topic.

Any concerns? ....... complaints? ................ 💀 Death Threats??!! :skull: :smile:

[ardalis]

Since often apps that need uploads will also need secure downloads it may be worth a note mentioning this pattern, perhaps when you reference Azure blob storage.
https://docs.microsoft.com/en-us/azure/architecture/patterns/valet-key

[Rutix]

@guardrex Could you not add to the top of the current article that it's due for an overhaul and link to this issue? I kinda found out about this after I read the article and got confused and got more questions. Warning the reader beforehand might be wise :)

[guardrex]

@Rutix We can't update the docs right now ... not for a week. By that time, the PR for this issue should be done.

[Rutix]

@guardrex Is there a place where I can follow the WIP?

[guardrex]

Almost there! I haven't submitted the PR yet. I hope to have it finished today and plan to send it in tomorrow (Wednesday, 5/8) morning.

[guardrex]

@ardalis Do u want to take a look at _me Rex hacks_? 🙈

I put the sample up at ...

https://github.com/guardrex/FileUploadsSampleApp

If ur busy and just want to see the key bit, take a look at the validation bits. I put a method in for each of buffered IFormFile and streamed files at ...

https://github.com/guardrex/FileUploadsSampleApp/blob/master/Utilities/FileHelpers.cs

The StreamingController is at ...

https://github.com/guardrex/FileUploadsSampleApp/blob/master/Controllers/StreamingController.cs

Keep in mind if you look at the whole app that the app is very scenario driven and set up to facilitate cut-'n-paste by devs for a specific scenario. For example, I avoided little helpers for common code. If the reader needs to do that, they should do it in their app.

I don't have file overwrite checks, but ...

• There's only so much that can be done here. It's a small-ish add, but I'm out of time ⏲ ... gotta get on to other things. 🏃😅 Got a half-a-gazillion other issues that I need to address.
• If a dev wants overwrite behavior, they're good to go with this example. I'll call out the behavior in the README.

I also don't format the ModelState JSON when it's returned to the client in the streaming scenarios. I just show the JSON. Again, I'm out of time here. Anyway ... trivial for them to do whatever they like with the response on the client side.

Thanks for what you did on streaming here. I really enjoyed working with your base code.

@Rutix ... Plz take a look and see if you spot anything bad here. 💀 Death Threats Welcome! :skull: :smile:

I'll see if I can finish the topic tonight, proof it tomorrow morning (Wed), and get it submitted. No promises tho ... if I make a TON of updates tomorrow morning, it might take one more day.

[Rutix]

@guardrex I am reviewing the sample. I found some little stuff which are wrong. Do you want me to give the feedback here or send in a PR on your SampleApp? I am not fully done reviewing yet so if you want them here I will gather them and give them all when done :).

[guardrex]

@Rutix Cool ... send a PR to that repo. I'll roll them into my local sample.

[guardrex]

Updating everyone on the progress with the File Uploads topic. The PR is in review at https://github.com/aspnet/AspNetCore.Docs/pull/12344.

_Thank you everyone!_ ... especially for your patience. We're sorry that it took so long, but we knew that this one was going to need a major pass, especially just to address the security aspects. It took some time for one of us to get free. I'm happy thus far with the updates, and we'll see what the engineers have to say about it shortly. Hopefully, I didn't put a bunch of 😵 RexHacks:tm: 😵 in there! :smile:

Thanks to @Rutix :beer:, who worked on the topic and sample with me.

Every request wasn't met. However, I opened #12285 to consider several of them on a future pass.

The following are some specific notes about what happened with your request(s). In a few cases, I mention that you can provide further feedback on the tracking issue ... #12285. That's probably best. If you use the This Page feedback button at the bottom of the topic, we'll end up closing your issue and moving the feedback to the tracking issue anyway, so it's best if you comment further on this topic on #12285. After #12285 is worked and closed, we can all go back to using the This Page feedback process for this topic.

• @AhmedSaid #5217 – .Value ... Done! Thank you.
• @chikien276 #3989 – Fixed the namespace issue you raised, but didn't really look closely at your https://github.com/chikien276/MultipartHelper/ approach. If you think your approach is better, add a remark to #12285 to discuss your approach versus the one that the topic uses.
• @nickalbrecht #4777 – Moved to https://github.com/aspnet/AspNetCore.Docs/issues/12285 for future work ... didn't have time to address it on this round.
• @weitzhandler #4964 – Client-side AJAX covered now; however, this doesn't cover HttpClient (perhaps with CORS). That's perhaps a good idea, but it's also rather very advanced for this doc set.
• @KevinBurton #6247 – Includes file name validation now.
• @rswetnam @harmonicradius #6349 – See if these updates address your concerns. If not, add feedback to the tracking issue at #12285.
• @nburkitt #6927 – Sample+topic are Razor Pages now.
• @heapwalker #7702 – Added a section to cover.
• @Praveen-Rai #7742 – Calling .Value now on the StringSegments, as @AhmedSaid remarked on #5217, so it should be all good now. :+1:
• @ShreyanshDaga #8039 – I removed the content from the topic that shows the IFormForm props and methods. Yes, I think you hint at a good idea: _Don't lock the docs into something that the engineers may change!_
• @davidekarasek #8100 – "assembly requirements for the iForm interface" ... idk on that. "bloated our ... simple and effecient .Net Core API" ... I'm not surprised, but not much we can do on the docs side if that's the case ... open feedback on aspnet/AspNetCore repo for that. "jQuery AJAX" ... this uses plain old JavaScript. jQuery AJAX will be similar. "reducing file size on upload" ... good idea, but I fear it's beyond the scope here. The topic here will always address base case scenarios. We hope blogs will cover advanced scenarios. We don't have the staffing to produce the depth that would be required in some subject areas.
• @davidekarasek #8298 – The sample app handles this (in the buffered/model-based file upload case) by having model properties bound to the form's fields. In the streaming file case with AJAX, it isn't a hidden field ... it's a form field in the form (note), but the same concept applies. note is picked up when the form data is bound. It's not exactly the same thing (hidden vs. form field), but let's see if more readers run into a problem and ask about it. We can add it on a future PR if devs ask for that specifically. We'll keep an :eye: out for more asks on binding hidden fields (both in the buffered and streaming cases).
• @meixger, @sgf #8797 – I think we're ok with the updated sample and 2.2 going forward. If you're still having trouble after this goes live, add feedback on the tracking issue #12285.
• @LeonardoSobrado #9116 – We're dropping coverage for 1.x. If you haven't done so already, update to 2.2.
• @lexugax #9327 – I didn't have time to address it on this pass. I marked the follow-up issue with your recommendation for the next pass on the topic. https://github.com/aspnet/AspNetCore.Docs/issues/12285
• @devbrsa @gmareater #9473 – I added some code for getting the file name: contentDisposition.FileName.Value. See the updated sample app on this PR or after this merges pull the sample from the repo to see the new bits.
• @ericbrunner #10161 – Addressed your feedback in these updates.
• @AGuyCalledGerald #10537 – This won't be addressed because the topic+sample are embracing Razor Pages. Consult with devs on Stack Overflow, Slack, or Gitter if you get stuck on the MVC approach.
• @shunjid #11898 – I've added remarks to the topic WRT background file processing. I noted on the follow-up issue to consider expanding with a concrete example. #12285
• @jeanmarc3 #11949 – Angular references are removed. The new sample app has a Razor Pages page that issues an AJAX call to stream files to the server. The updated example shows how to have additional form fields passed to the streaming controller and then binds them and saves them to a SQL Server dB. See the updated sample app or pull it down after this PR merges. The sample is 2.2 now, and we'll put up a 3.0 sample at 3.0 RTM.
• @xcaptain #12130 – The topic+sample won't use Request.Form. It either binds to a model with buffering or streams the file. We can reconsider in the future if more devs ask.
• @shadinamrouti #12204 – The updated sample shows how to save to any location on disk (or a network share) both via streaming and buffered (model binding) approaches, including sanitizing the file name (to an extent ... production apps should do more file name checks ... length, for example ... possibly existing file check so that overwrites don't occur .... possibly using an app-generated GUID). The sample now makes use of a MemoryStream to save the file to a database or uses System.IO.File.Create to create the file at a specific path (webroot in the sample).
• @tony-long #6609 – Didn't hit every use case you listed, but several of them are covered now on this PR: saving to dB or physical storage, listing files, deleting files, downloading files.
• @tony-long #6610 – Updated sample runs with latest 2.2 runtime and packages.
• @Habikki #6962 – Addressed your feedback in these updates.

_Thanks again everyone!!!_ :beers:

[ardalis]

Looking forward to this. I just had to go through a few of these issues myself today on my stream as I was working on large video file uploads. Do you cover the various ways to increase the file upload limits in this now? That was something I needed to search for.

[Rutix]

@ardalis For the server configuration Kestrel maximum request body size and IIS content length limit are covered. I think those are the most important ones.

[guardrex]

@ardalis Unfortunately, they haven't resolved all of the issues. https://github.com/aspnet/AspNetCore/issues/2711 is still open, and it was left at 'investigate' and moved to the backlog. The problems seem to kick in at 4GB with IIS. I didn't try to cover it ... and I even didn't try to test it here.

I have some free time coming up this weekend. I would like to at least verify say 3.9 GB is :+1: on IIS ... then see it fail here at 4.1 GB. If it does fail at 4.1 GB as they're reporting, I can cross-link the engineering issue in the Troubleshoot section.

[ardalis]

I found 3 things to set:

[RequestFormLimits(MultipartBodyLengthLimit = Constants.MAX_UPLOAD_FILE_SIZE)]
[RequestSizeLimit(Constants.MAX_UPLOAD_FILE_SIZE)]

and

                .CreateDefaultBuilder(args)
                .UseStartup<Startup>()
                .UseKestrel(options =>
                {
                    options.Limits.MaxRequestBodySize = Constants.MAX_UPLOAD_FILE_SIZE; //500MB
                });

[guardrex]

Ah ... yes ... default MultipartBodyLengthLimit is 134,217,728 (128 MB). I'll get that covered.

I suspect (thus far) that RequestSizeLimit just sets the MaxRequestBodySize. Even if that's correct, I still need to add content for it. Would the attribute only apply the filter to the decorated controller? ... so it wouldn't be an app-wide setting like using the Kestrel limit? Do you know the answer? ... or should I go on an engineering hunt for it?

[guardrex]

https://github.com/aspnet/AspNetCore/blob/master/src/Mvc/Mvc.Core/src/Filters/RequestSizeLimitFilter.cs

[Rutix]

@guardrex @ardalis [RequestSizeLimit] does indeed set the options.Limits.MaxRequestBodySize only on the action. [RequestFormLimits] does the same as the following for that action:

public void ConfigureServices(IServiceCollection services)
{
    services.AddMvc();
    services.Configure<FormOptions>(x =>
    {
        x.MultipartBodyLengthLimit = 209715200;
    });
}

See also: https://github.com/aspnet/AspNetCore/blob/v2.2.4/src/Mvc/Mvc.Core/src/RequestFormLimitsAttribute.cs

[guardrex]

Thanks @Rutix and @ardalis ... I'll roll this content into the topic.

[guardrex]

Reminder to everyone that I pinged .... half the Internet! .... You can unsubscribe from the thread if our chatting is filling your Inbox with junk.

[guardrex]

@Rutix @ardalis Let's move our discussions over to the PR to save these good folks a lot of Inbox pain.

12344

[ryanski44]

@ardalis Unfortunately, they haven't resolved all of the issues. aspnet/AspNetCore#2711 is still open, and it was left at 'investigate' and moved to the backlog. The problems seem to kick in at 4GB with IIS. I didn't try to cover it ... and I even didn't try to test it here.

I have some free time coming up this weekend. I would like to at least verify say 3.9 GB is 👍 on IIS ... then see it fail here at 4.1 GB. If it does fail at 4.1 GB as they're reporting, I can cross-link the engineering issue in the _Troubleshoot_ section.

@guardrex,

Can you check if the fix I presented in [aspnet/AspNetCore#2711] has been included? It was an obvious coding error that I was able to fix and test in a forked repository. We have been running my modified code on our production servers since last year, and we would love to be able to use the official Asp.NET Core Module again.

Here is the actual fix in my forked repository:

https://github.com/ryanski44/AspNetCore/commit/fdec0d797fcefeb7af8278e1e98701442a2bd6e8#diff-f6125591f48ad0a58d38a1cb7cef88c0

[guardrex]

@ryanski44 That engineering issue is still open and on the backlog. You'll have to inquire with them on when they might take it on (or allow you to submit a PR for the change).