Closed Rick-Anderson closed 5 years ago
@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) ...
IFormFile
(with validation)QueueBackgroundWorkItem
should be offered by the framework for 3.0, so this is best placed in a post-3.0 issue for future work)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:
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
@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 :)
@Rutix We can't update the docs right now ... not for a week. By that time, the PR for this issue should be done.
@guardrex Is there a place where I can follow the WIP?
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.
@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 ...
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.
@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 :).
@Rutix Cool ... send a PR to that repo. I'll roll them into my local sample.
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.
.Value
... Done! Thank you.HttpClient
(perhaps with CORS). That's perhaps a good idea, but it's also rather very advanced for this doc set..Value
now on the StringSegments
, as @AhmedSaid remarked on #5217, so it should be all good now. :+1: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!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).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.Request.Form
. It either binds to a model with buffering or streams the file. We can reconsider in the future if more devs ask.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).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.
@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.
@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.
I found 3 things to set:
[RequestFormLimits(MultipartBodyLengthLimit = Constants.MAX_UPLOAD_FILE_SIZE)]
[RequestSizeLimit(Constants.MAX_UPLOAD_FILE_SIZE)]
and
WebHost
.CreateDefaultBuilder(args)
.UseStartup<Startup>()
.UseKestrel(options =>
{
options.Limits.MaxRequestBodySize = Constants.MAX_UPLOAD_FILE_SIZE; //500MB
});
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 @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
Thanks @Rutix and @ardalis ... I'll roll this content into the topic.
Reminder to everyone that I pinged .... half the Internet! .... You can unsubscribe from the thread if our chatting is filling your Inbox with junk.
@Rutix @ardalis Let's move our discussions over to the PR to save these good folks a lot of Inbox pain.
@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:
@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).
Moved from #5331
https://docs.microsoft.com/aspnet/core/mvc/models/file-uploads
Request.Form
directly on the updates. If more devs ask about the approach, we can reconsider it later.Document Details
⚠ Do not edit this section. It is required for docs.microsoft.com ➟ GitHub issue linking.