TryUpdateModel: Determine model type at runtime

[maxtoroq]

The TryUpdateModel methods require that you explicitly declare the type you are binding at compile time, and if you attempt to bind a subclass instance the subclass properties are ignored. This is because the model metadata is requested using the type parameter:

var modelMetadata = metadataProvider.GetMetadataForType(
    modelAccessor: null,
    modelType: typeof(TModel));

...instead of using the type of the model instance:

var modelMetadata = metadataProvider.GetMetadataForType(
    modelAccessor: null,
    modelType: model.GetType());

Even with this small change you are still forced to provide the TModel type parameter, but at least you can pass a base type for TModel and have a derived instance be properly updated.

[pranavkm]

We now have precedence in doing this via TryValidateModel - https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNet.Mvc.Core/Controller.cs#L1062-L1064

[dougbu]

true though TryUpdateModel<TModel> and TryValidateModel are also inconsistent in MVC 5. so there's a back-compat argument in favour of leaving MVC 6 as-is in this respect. not sure how strong that argument is 😺

if we do make a change here, probably still want to prevent boxing and attempting to update a value type model. the type parameter avoids that problem and moves errors to compile-time.

[maxtoroq]

I didn't suggest removing TModel because the includeExpressions parameter needs it, although you could have one overload with TModel and the others without it.

[dougbu]

@maxtoroq perhaps. then again, the use of TModel in includeExpressions is an argument in favour of using TModel everywhere in this method group. would be confusing otherwise.

[maxtoroq]

@dougbu I agree. Although I haven't tested it, perhaps simply using object for TModel works. If it does then the only change needed is getting the modelType from the model instance.

[kirthik]

checked in - f1c62ef

[yishaigalatzer]

We need to revert this change. This was an intended security guarantee. Instead we need to provide a non-generic version that takes the type as input in a non-generic way

[dougbu]

for how many of the existing Controller.TryUpdateModelAsync<TModel>(...) overloads will we create TryUpdateModelAsync(Type modelType, ...) equivalents? e.g. suspect overloads with [NotNull] params Expression<Func<object, object>>[] includeExpressions parameters won't be that helpful.

separately can we remove the ModelBindingHelper.TryUpdateModelAsync<TModel>() overload that just defaults the predicate (do that in Controller) and change the overload that takes a predicate to use Type modelType as well? that is, let's try not to duplicate too much.

[maxtoroq]

@yishaigalatzer Would you mind explaining the security concerns?

[yishaigalatzer]

In short people using mvc5 have exact opposite expectations (because that's how the code worked) so that would create an overbinding problem. Hence we provide an explicit overload just like mvc5 did.

-----Original Message-----
From: "Max Toro" notifications@github.com
Sent: ‎1/‎29/‎2015 5:17 PM
To: "aspnet/Mvc" Mvc@noreply.github.com
Cc: "Yishai Galatzer" yishai_galatzer@yahoo.com
Subject: Re: [Mvc] TryUpdateModel: Determine model type at runtime (#1837)

@yishaigalatzer Would you mind explaining the security concerns?
—
Reply to this email directly or view it on GitHub.=

[yishaigalatzer]

@doug if you have concerns with existing design file a separate bug. Lets keep the concerns separate

-----Original Message-----
From: "Doug Bunting" notifications@github.com
Sent: ‎1/‎29/‎2015 2:16 PM
To: "aspnet/Mvc" Mvc@noreply.github.com
Cc: "Yishai Galatzer" yishai_galatzer@yahoo.com
Subject: Re: [Mvc] TryUpdateModel: Determine model type at runtime (#1837)

for how many of the existing Controller.TryUpdateModelAsync(...) overloads will we create TryUpdateModelAsync(Type modelType, ...) equivalents? e.g. suspect overloads with [NotNull] params Expression<Func<object, object>>[] includeExpressions parameters won't be that helpful.
separately can we remove the ModelBindingHelper.TryUpdateModelAsync() overload that just defaults the predicate (do that in Controller) and change the overload that takes a predicate to use Type modelType as well? that is, let's try not to duplicate too much.
—
Reply to this email directly or view it on GitHub.=

[dougbu]

@yishaigalatzer sure. I'll file a bug if we end up with a couple of extra methods.

however

how many of the existing Controller.TryUpdateModelAsync<TModel>(...) overloads will we create TryUpdateModelAsync(Type modelType, ...) equivalents?

was a question about the new way forward for this issue. and my second paragraph was, in part, a plea not to go too crazy with helper overloads.

[maxtoroq]

@yishaigalatzer Cannot argue against back compat. There's little value in adding more overloads, which anyone can do. If the default behavior is not changed, what's the point.

There are other parts of the framework that work against the runtime type instead of the declared type, e.g. Html.EditorForModel(). My intention was to make these different parts work together without special configuration.

[yishaigalatzer]

I get your point. This is dealing with unsafe inputs scenarios, so we cannot be liberal with that.

The additional overloads provide an easy escape hatch.

-----Original Message-----
From: "Max Toro" notifications@github.com
Sent: ‎1/‎29/‎2015 8:07 PM
To: "aspnet/Mvc" Mvc@noreply.github.com
Cc: "Yishai Galatzer" yishai_galatzer@yahoo.com
Subject: Re: [Mvc] TryUpdateModel: Determine model type at runtime (#1837)

@yishaigalatzer Cannot argue against back compat. There's little value in adding more overloads, which anyone can do. If the default behavior is not changed, what's the point.
There are other parts of the framework that work against the runtime type instead of the declared type, e.g. Html.EditorForModel(). My intention was to make these different parts work together without special configuration.
—
Reply to this email directly or view it on GitHub.=

[kirthik]

checked in - f6e701b