Allow IFormFile parameters annotated with [FromForm] to be correctly … #8452

pranavkm · 2018-09-12T22:56:55Z

…bound in ApiControllers

…bound in ApiControllers Fixes #8311

pranavkm · 2018-09-12T22:59:33Z

src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/InferParameterBindingInfoConvention.cs

@@ -166,5 +170,12 @@ private bool IsComplexTypeParameter(ParameterModel parameter)
                .GetMetadataForType(parameter.ParameterInfo.ParameterType);
            return metadata.IsComplexType && !metadata.IsCollectionType;
        }
+
+        private static bool IsFormFile(Type parameterType)


BindingSource.Form is non-greedy, while BindingSource.FormFile that's inferred from model metadata is greedy. The difference causes the parameter name to be reset by ApiController.

The above code is intended to reset the name iff the binding source is non-greedy.

dougbu · 2018-09-13T00:05:48Z

src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/InferParameterBindingInfoConvention.cs

@@ -112,7 +114,8 @@ internal void InferBoundPropertyModelPrefixes(ControllerModel controllerModel)
                if (property.BindingInfo != null &&
                    property.BindingInfo.BinderModelName == null &&
                    property.BindingInfo.BindingSource != null &&
-                    !property.BindingInfo.BindingSource.IsGreedy)
+                    !property.BindingInfo.BindingSource.IsGreedy &&
+                    !IsFormFile(property.ParameterType))


Something else is going on here because this line should be redundant. IFormFile and IEnumerable<IFormFile> parameters should be inferred to have BindingSource.FromFile. IsGreedy is true for that binding source. This line and line 139 should not be reachable for the parameters affected by #8311. (Modulo the #7770 fix for parameters that aren't exactly IFormFile or IFormFileCollection.)

@pranavkm did you debug the scenario and confirm these BinderModelName settings are the issue? If yes, why were property.BindingInfo.BindingSource.IsGreedy or parameter.BindingInfo.BindingSource.IsGreedy (below) false for an IFormFile property or parameter?

The issue specifically manifests in this scenario: Post([FromForm] IFormFile formFile)

Got it now. Thanks for the offline conversation @pranavkm

dougbu · 2018-09-13T00:13:53Z

src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/InferParameterBindingInfoConvention.cs

+        private static bool IsFormFile(Type parameterType)
+        {
+            return typeof(IFormFile).IsAssignableFrom(parameterType) ||
+                typeof(IFormFileCollection).IsAssignableFrom(parameterType) ||


Don't need to check for IFormFileCollection because that implements IEnumerable<IFormFile>.

dougbu · 2018-09-13T00:14:42Z

src/Microsoft.AspNetCore.Mvc.Core/ApplicationModels/InferParameterBindingInfoConvention.cs

@@ -166,5 +170,12 @@ private bool IsComplexTypeParameter(ParameterModel parameter)
                .GetMetadataForType(parameter.ParameterInfo.ParameterType);
            return metadata.IsComplexType && !metadata.IsCollectionType;
        }
+
+        private static bool IsFormFile(Type parameterType)


The above code is intended to reset the name iff the binding source is non-greedy.

dougbu · 2018-09-13T00:18:39Z

...rosoft.AspNetCore.Mvc.Core.Test/ApplicationModels/InferParameterBindingInfoConventionTest.cs

@@ -720,6 +720,87 @@ public void InferParameterModelPrefixes_SetsModelPrefix_ForComplexTypeFromValueP
            Assert.Equal(string.Empty, parameter.BindingInfo.BinderModelName);
        }

+        [Fact]
+        public void InferParameterModelPrefixes_DoesNotSetModelPrefix_ForFormFileParametersAnnotatedWithFromForm()


These new tests aren't really regression tests for #8311. Need integration or functional tests that include the default BindingSourceMetadataProviders. See

Mvc/src/Microsoft.AspNetCore.Mvc.Core/Internal/MvcCoreMvcOptionsSetup.cs

Lines 96 to 99 in cb88e90

modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IFormFile), BindingSource.FormFile));

modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IFormCollection), BindingSource.FormFile));

modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IFormFileCollection), BindingSource.FormFile));

modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IEnumerable<IFormFile>), BindingSource.FormFile));

We use TestModelMetadataProvider that's set up with these detail providers in these tests

dougbu · 2018-09-13T00:21:06Z

...rosoft.AspNetCore.Mvc.Core.Test/ApplicationModels/InferParameterBindingInfoConventionTest.cs

+        }
+
+        [Fact]
+        public void InferParameterModelPrefixes_DoesNotSetModelPrefix_ForFormFileCollectionParametersAnnotatedWithFromForm()


Rename. This test covers properties, not parameters.

pranavkm · 2018-09-13T19:13:24Z

🆙 📅

dougbu

LGTM. The main question I have is whether we should similarly special-case CancellationToken, IFormCollection, and anything with a specified BindingInfo.BinderType to ignore BindingSource (as the binder providers do). Thoughts?

dougbu · 2018-09-13T21:15:16Z

After another offline conversation, @pranavkm has agreed to file a couple of bugs to track separately. In addition, neither of us believe CancellationToken or IFormCollection will hit problems similar to this one -- the relevant binders ignore BinderModelName.

pranavkm · 2018-09-14T16:06:14Z

@dougbu would you mind filing the issue with the BinderModelType? You'd be able to explain better.

dougbu · 2018-09-14T18:37:36Z

@dougbu would you mind filing the issue with the BinderModelType? You'd be able to explain better.

@pranavkm after experimenting a bit, I don't think [ModelBinder] or [BindProperty] are likely to cause problems. Those are main ways BindingInfo.BinderType can be set. ApiBehaviorApplicationModelProvider will not override BinderModelName (or BindingSource) because those attributes implement IBindingSourceMetadata (through IBinderTypeProviderMetadata). The chosen BindingSource is BindingSource.Custom and BindingSource.Custom is greedy.

It's possible to subclass either attribute or implement IBinderTypeProviderMetadata directly and set BindingSource to something greedy. But, we're now in a dim corner of a corner case.

Allow IFormFile parameters annotated with [FromForm] to be correctly …

ce4fff7

…bound in ApiControllers Fixes #8311

pranavkm assigned dougbu Sep 12, 2018

pranavkm commented Sep 12, 2018

View reviewed changes

dougbu suggested changes Sep 13, 2018

View reviewed changes

PR comments

43f5dd6

pranavkm force-pushed the prkrishn/formfile branch from 29b6b99 to 43f5dd6 Compare September 13, 2018 19:13

dougbu reviewed Sep 13, 2018

View reviewed changes

dougbu approved these changes Sep 13, 2018

View reviewed changes

pranavkm merged commit 233140c into release/2.2 Sep 14, 2018

pranavkm deleted the prkrishn/formfile branch September 14, 2018 16:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow IFormFile parameters annotated with [FromForm] to be correctly … #8452

Allow IFormFile parameters annotated with [FromForm] to be correctly … #8452

pranavkm commented Sep 12, 2018

pranavkm Sep 12, 2018

dougbu Sep 13, 2018

dougbu Sep 13, 2018

pranavkm Sep 13, 2018

dougbu Sep 13, 2018

dougbu Sep 13, 2018

dougbu Sep 13, 2018

dougbu Sep 13, 2018

pranavkm Sep 13, 2018

dougbu Sep 13, 2018

dougbu Sep 13, 2018

pranavkm commented Sep 13, 2018

dougbu left a comment

dougbu commented Sep 13, 2018

pranavkm commented Sep 14, 2018

dougbu commented Sep 14, 2018

	modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IFormFile), BindingSource.FormFile));
	modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IFormCollection), BindingSource.FormFile));
	modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IFormFileCollection), BindingSource.FormFile));
	modelMetadataDetailsProviders.Add(new BindingSourceMetadataProvider(typeof(IEnumerable<IFormFile>), BindingSource.FormFile));

Allow IFormFile parameters annotated with [FromForm] to be correctly … #8452

Allow IFormFile parameters annotated with [FromForm] to be correctly … #8452

Conversation

pranavkm commented Sep 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pranavkm commented Sep 13, 2018

dougbu left a comment

Choose a reason for hiding this comment

dougbu commented Sep 13, 2018

pranavkm commented Sep 14, 2018

dougbu commented Sep 14, 2018