Support resolving form files in complex form mapping

[captainsafia]

Addresses #49526.

[javiercn]

@captainsafia this looks great!

One last nit. Do you think we can make it support IBrowserFile.cs since that's the "Blazor" specific abstraction for it? I believe it should be pretty straightforwards, but if not, let me know.

[javiercn]

I took another look, and this might be a bit more nuanced than we think.

For reference, MVC seems to only support binding files as a Top-level property/object, (which is fine, we can decide to do something different here), but I believe we need to be careful and make sure that the following scenarios work:

• Multiple file inputs binding to a single file

  <input type="file" name="Resume" />
  <input type="file" name="Degree" />
  

  with a model like

  public class MyModel
  {
      public IFormFile Resume { get; set; }
      public IFormFile Degree { get; set; }
  }

• Multiple file inputs binding to collections of files

  <input type="file" multiple name="Reports" />
  <input type="file" multiple name="OtherDocuments" />
  

  with a model like

  public class MyModel
  {
      public IFormFileCollection Reports { get; set; }
      public IFormFile OtherDocuments { get; set; }
  }

• We should also probably produce an error if there are many files with the same name and we are trying to bind to an IFormFile

This is a sample payload that chrome will send for input and input multiple:

------WebKitFormBoundaryTxMDCYZlym9qHpwC
Content-Disposition: form-data; name="SampleFile"; filename="MySampleFile.txt"
Content-Type: text/plain


------WebKitFormBoundaryTxMDCYZlym9qHpwC
Content-Disposition: form-data; name="OtherFile"; filename="MyOtherFile.txt"
Content-Type: text/plain


------WebKitFormBoundaryTxMDCYZlym9qHpwC
Content-Disposition: form-data; name="Reports"; filename="MyReports2.txt"
Content-Type: text/plain


------WebKitFormBoundaryTxMDCYZlym9qHpwC
Content-Disposition: form-data; name="Reports"; filename="MyReports.txt"
Content-Type: text/plain


------WebKitFormBoundaryTxMDCYZlym9qHpwC
Content-Disposition: form-data; name="OtherDocuments"; filename="MyOtherDocuments2.txt"
Content-Type: text/plain


------WebKitFormBoundaryTxMDCYZlym9qHpwC
Content-Disposition: form-data; name="OtherDocuments"; filename="MyOtherDocuments.txt"
Content-Type: text/plain


------WebKitFormBoundaryTxMDCYZlym9qHpwC
Content-Disposition: form-data; name="__RequestVerificationToken"

CfDJ8NQYaZebqUlGr0rlYIlnIDg5dKVRjgYAN_qNiT2n1Fw0M3XRiweKM4YcI2rzoBE9BBzidDyoPzG7YumuVUkV5qNUz0GIEpBGlNb2muhD-OoFPhKZoHLXKzQV0StDU0_iVSDXivsQu2RVtcOBPOdA-lg
------WebKitFormBoundaryTxMDCYZlym9qHpwC--

The contents of the files are not shown because the chrome dev tools scraps them. The form that I used is like this:

<form method="post" enctype="multipart/form-data">
    <input type="file" name="SampleFile" />
    <input type="file" name="OtherFile" />
    <input type="file" multiple name="Reports" />
    <input type="file" multiple name="OtherDocuments" />
    <input type="submit" value="Upload" />
</form>

[captainsafia]

public IFormFileCollection Reports { get; set; }

This one is interesting. At the moment, minimal APIs assumes that a top-level IFormFileCollection type refers to HttpContext.Request.Form.Files in all occasions. We don't use the parameter name to extract a subset of files by their name. IMO, it probably makes more sense to support that by supporting an IReadOnlyList<IFormFile> type similar to what IFormFIleCollection.GetFiles returns.

Binding for multiple IFormFiles with different names and erroring for multiple files with the same name makes sense though.

[javiercn]

@captainsafia I'm ok if we keep IFormFileCollection binding the entire set of files independent of the prefix (the type acts as the signal/discriminator) of what you want, and it's a well-defined behavior.

What I am interested in making sure that we do correctly is handling input type=file and input type=file multiple in a way that people can either use the inputs directly or rely on InputFile to author the UI.

We need to get this right because whatever we bake in now, will become a breaking change in the future otherwise. So I think your proposal of having IFormFileCollection bind to the entire thing and have <<(Collection|Dictionary)Type>><IFormFile> bind to the set that match the proposed subset makes sense.

[captainsafia]

OK, I've updated the PR to accommodate some of the functionality here. InputFile works with:

• IBrowserFile
• IFormFile
• IReadOnlyList
• IFormFileCollection

as types backing the property.

Binding for multiple IFormFiles with different names and erroring for multiple files with the same name makes sense though.

I take this back. It appears the current behavior for IFormFileCollection is to return the first file with a given name if they are multiple so I kept this pattern in the implementation.

[javiercn]

nit: Can we also do IReadOnlyList<IBrowserFile>? Otherwise it can't be used on RCLs if they want to support wasm

[captainsafia]

Sure

[javiercn]

Can you copy the logic from https://github.com/dotnet/aspnetcore/blob/main/src/Components/Web/src/Forms/InputBase.cs#L199-L221 on to InputFile so that it doesn't require specifying the name manually?

[javiercn]

It'll need to be added https://github.com/dotnet/aspnetcore/blob/main/src/Components/Web/src/Forms/InputFile.cs#L62

[captainsafia]

I ended up keeping this change out of this PR to keep the scope smaller.

[javiercn]

That's fair. I'll file an issue for it.

[javiercn]

Filed #50728 to track it

[SteveSandersonMS]

InputFile doesn't have any Value you can bind to, so this looks like a mistake. I was surprised at first that the compiler even allowed this, but it must follow from the fact that we don't restrict passing parameters to ones that actually exist. It's just unfortunate that the editor highlights @bind-Anything in purple as if it's definitely correct.

For these E2E tests, the right move is probably to replace <InputFile @bind-Value> with <input type="file" name="Model.HeaderPhoto"> etc.

[captainsafia]

InputFile doesn't have any Value you can bind to, so this looks like a mistake. I was surprised at first that the compiler even allowed this, but it must follow from the fact that we don't restrict passing parameters to ones that actually exist.

Yep, this is correct. The name attribute is the only reason this works correctly for posted forms.

For these E2E tests, the right move is probably to replace <InputFile @bind-Value> with etc.

I'm fine with this. I assume we'll use #50728 to track whether InputFile should be updated to more closely match InputBase.

[javiercn]

Yes. This was a small oversight on my part.

[javiercn]

Looks great!

Signing-off to make sure you aren't blocked on me.