[Part1 of #4895] Modified FormCollectionModelBinderProvider to throw when binding for...

[kichalla]

…FormCollection model type.

Part 1 of [Fixes #4895] No parameterless Constructor defined
I will send the fix for ComplexTypeModelBinderProvider in a separate PR

@dougbu @rynowak

[rynowak]

This is different than what the bug suggests at the solution. #4895 (comment)

This special cases FormCollection - the suggestion was to check for a no-arg constructor in the Complex type provider and instead of throwing an exception, just let it pass through, the system will throw if a model binder can't be created.

[kichalla]

@dougbu and I discussed about this and divided this into 2 parts:

• In Part1 (this PR), we would explicitly check for FormCollection in FormCollectionModelBinderProvider and throw so that users do not mistake it to be a type that we support binding to.
• In Part2 we would like to handle the no arg constructor scenario in ComplexTypeModelBinderProvider

Question regarding:

the suggestion was to check for a no-arg constructor in the Complex type provider and instead of throwing an exception, just let it pass through, the system will throw if a model binder can't be created.

Following is the current(1.0.0) behavior of the system where it gives good enough details for a user to understand whats wrong, so wondering what you meant:

      An unhandled exception has occurred while executing the request
System.ArgumentException: Type 'MvcSandbox.Controllers.Customer' does not have a default constructor
Parameter name: type
   at System.Linq.Expressions.Expression.New(Type type)
   at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexTypeModelBinder.CreateModel(ModelBindingContext bindingContext) in C:\github\mvc\src\Microsoft.AspNetCore.Mvc.Core\ModelBinding\Binders\ComplexTypeModelBinder.cs:line 325
   at Microsoft.AspNetCore.Mvc.ModelBinding.Binders.ComplexTypeModelBinder.<BindModelCoreAsync>d__4.MoveNext() in C:\github\mvc\src\Microsoft.AspNetCore.Mvc.Core\ModelBinding\Binders\ComplexTypeModelBinder.cs:line 60
--- End of stack trace from previous location where exception was thrown ---

[kichalla]

Ah, ignore my clarification question...I understood what you meant.

[dougbu]

private class and stick this at the bottom of the file.

[rynowak]

I don't think it's the end of the world, but ideally MBPs don't throw exceptions. In this case it seems ok because we 'own' IFormCollection/FormCollection. Order matters with MPBs and so throwing an exception means that other providers can't handle it.

[dougbu]

⌚️ for a couple of small changes

[dougbu]

Would be more readable if this used string interpolation and the Assert.Throws<TEx> overload that takes the expected message.

[kichalla]

the Assert.Throws overload that takes the expected message.

I am unable to find this overload

[dougbu]

Yeah, I got confused between default and ExceptionAssert overloads. Don't change this further.

[kichalla]

🆙 📅

[dougbu]

[dougbu]

Minor but the first "to" and trailing "instead" make this less readable. "cannot bind a model ..." and "change the model type to ..." don't need the extra words.