Ensure IFormFile binding for nested properties works #12847
Conversation
pranavkm
added
the
ask-mode
dougbu
left a comment
dougbu
left a comment
There was a problem hiding this comment.
Not sure about this change and I'd like to discuss on Monday. Having a value provider for a greedy BindingSource blurs lines we've been careful about in the past (and hit issues when we weren't). Seems likely to change semantics, admittedly in ways our existing tests didn't catch.
src/Mvc/Mvc.Core/src/ModelBinding/FormFileValueProviderFactory.cs
Outdated
Show resolved
Hide resolved
Eilon
added
the
area-mvc
|
Added some more tests based on our discussion. |
| /// specifically responds to <see cref="ContainsPrefix(string)"/> queries. This allows the model binding system to | ||
| /// recurse in to deeply nested object graphs with only values for form files. | ||
| /// </remarks> | ||
| public sealed class FormFileValueProvider : IValueProvider |
There was a problem hiding this comment.
What motivated the choice to make this separate from FormValueProvider? Is the idea that someone could remove this value provider if they don't want it?
There was a problem hiding this comment.
Yeah, that was one of the reasons. The other reason is that FormValueProvider is meant to be derived, and it now makes user code much more complicated since they now have to track two separate prefix groups.
| var request = context.ActionContext.HttpContext.Request; | ||
| if (request.HasFormContentType) | ||
| { | ||
| // Allocating a Task only when the body is multipart form. |
There was a problem hiding this comment.
This comment isn't super duper accurate. HasFormContentType will be true for both form content types, not just multipart.
There was a problem hiding this comment.
Ahh, I changed the code, forgot to update the comment.
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>netcoreapp3.0</TargetFramework> | ||
| <RootNamespace>Microsoft.AspNetCore.Mvc</RootNamespace> |
dougbu
left a comment
dougbu
left a comment
There was a problem hiding this comment.
Small question and a nit but it's up to you whether another test is needed. Integration test coverage looks good!
src/Mvc/test/Mvc.IntegrationTests/FormFileModelBindingIntegrationTest.cs
Outdated
Show resolved
Hide resolved
| { | ||
| UpdateRequest(request, data + 1, "FormFiles[0]"); | ||
| AddFormFile(request, data + 2, "FormFiles[1]"); | ||
| AddFormFile(request, data + 3, "FormFiles[1]"); |
There was a problem hiding this comment.
Does anything different happen if the files are explicitly named e.g. "FormFiles[0][0]", "FormFiles[1][0]" and "FormFiles[1][1]"?
pranavkm
force-pushed
the
prkrishn/form-file
branch
from
4400ef7 to
bc554e7
Compare
pranavkm
force-pushed
the
prkrishn/form-file
branch
from
bc554e7 to
fb59968
Compare
|
This comment was made automatically. If there is a problem contact aspnetcore-build@microsoft.com. I've triaged the above build. I've created/commented on the following issue(s) |
ghost
deleted the
Fixes #9510
Description
Binding for IFormFile does not work correctly when the form file is nested but no sibling properties are specified.
Customer Impact
This allows a fairly narrow set of model binding scenarios where form files are posted to be bound correctly.
Regression
We had a regression in 2.2 that was fixed in 3.0. However, this was identified as one of the scenarios that wasn't correctly addressed.
Risk
Medium. Model binding changes have a fairly large test area, however we have a fair bit of test in this area.