Asked this on Scott Guthries blog and he suggested I post the question here:
Have a question about a breaking change with the ModelBinder. In the beta, when filling a collection it would look for a .index form field to know what indexes to process. This worked great because I could output one .index per item (order.Products.index
= 1 would look for order.Products[1].<fieldname>), then when I added a new one I could use negative numbers (so orders.Products[-1].<fieldname>). You could have as many index fields and the HTTP post would make a nice comma seperated list of values. Super
easy to inject new html for a new item into the page.
In the RC, it always starts at 0 and goes up until it cannot find any more. Besides making it harder to add because you have to know the next index, if you delete a member in the middle of your set, every one after will not get processed. For example, if
3 products were output ([0],[1],[2]), the middle one ([1]) is deleted via an AJAX call then the whole screen is saved, only the first ([0]) will be materialized with the model binder.
With the beta you could also use anything for the index, they did not have to be integers. This was nice.
Is there anyway to get the Beta functionality back, maybe flex if there is a .index? Or make some of the internal/private static methods more accessible (like DictionaryHelpers.DoesAnyKeyHavePrefix, VerifyValueUsability, CollectionHelpers.ReplaceCollection,
ShouldUpdateProperty would also be useful). I have already written a new binder that handles the case, but would be nice out of the box (plus I cannot match exactly without some of the helper routines).
The Beta behavior was fairly disliked by the community, so it's unlikely that it will be reintroduced. Some possible solutions:
If you're using AJAX to manipulate the set, then you shouldn't have to send the whole thing back to the server, since ostensibly each AJAX request would be responsible for adding, modifying, or deleting a single entry.
If you're using Javascript (sans AJAX) for deletions, you could replace products[1] with a dummy value. For example:
Note the introduction of two values, a placeholder for Products[1] and a key telling us which products in the array should be deleted. Pull this back like so:
public ActionResult MyMethod(List<Product> products, int[] productsToDelete) {
// iterate through productsToDelete (backward), deleting the specified indices from the
products collection
}
Alternatively, have some Javascript that keeps track of the highest index and the index that was just deleted, and upon a product being deleted silently change the names of all the fields corresponding to the last product to match the product that was just
deleted, then decrement the highest index count by one. This will plug any gaps in the list on submission to the server but is a little bit more work on the client.
The other problem with the new binding implementation is that there is no way to tie back a field that has an error. In your example above, say products[2].UnitPrice gets set to -3. In the binder, I don't know which field to tie the AddModelError to, unless
I add the primary key in, cluttering the HTML and requiring manual looping over the bindingContext to find the right PK, which seems awfully inefficient.
The binder always has full knowledge of the entire field name that it's currently checking. In your example, the binder knows that it's trying to match a property
UnitPrice from the field prefixed products[2], so it concatenates them to form a full field name of
products[2].UnitPrice.
If that's not the scenario you're operating in, please clarify. It's possible that I just misunderstood what you were trying to do. :)
I thnk what he is trying to say is that using a known index is easier to work with than a sequential number. products[<ProductID>].UnitPrice makes it easier to find related fields in JavaScript and server side code.
I know the validation message helpers output using the modelstate, but if you wanted to make messages based on other fields, they are easier to find (i.e. UnitPrice must be greater than UnitCost). Having to rely on positional notation when you could have
a known key is shaky. Suppose I wanted to load a list of products from the database, they push the values in by calling UpdateModel for each. With positional notation, I would need to loop through all product[x].ProductID to find the match, then call update
model with the correct product[x] prefix. With key based, I know they are product[<ProductID>], much easier.
On your previous comment to me: I understand where you guys believe the sequential is better, but honestly the code could flex based on the existence of a .index field and allow not integer indexes without too many changes. In the beta, I had honestly wished
it would have a without a .index because sometimes sequential is just fine.
Right now, everyone who used the beta code with model binding collections is broke.
Here is an implementation that flexes based on index, not this only handles ICollection<> interfaces, I may get to IDictionary<,> and straight arrays when I get a chance. If so, I will post them here. (This should compile, I quickly took apart my LLBLGen
model binder in notepad)
//Set ModelBinders.Binders.DefaultBinder = new IndexModelBinder(); in Global.asax.cs Application_Start
using System;
using System.Collections.Generic;
using System.Collections;
using System.Linq;
using System.Text;
using System.Web.Mvc;
using System.ComponentModel;
using System.Reflection;
namespace System.Web.Mvc
{
public class IndexModelBinder : DefaultModelBinder
{
#region handle Beta style collections with .index
public override object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext)
{
//handle collections with an indexer specified
Type collType = bindingContext.ModelType.GetInterfaces().FirstOrDefault(t => t.IsGenericType && t.GetGenericTypeDefinition() == typeof(ICollection<>));
if (collType != null)
{
//check for existence of model name fields, if not, fall back to empty model name
if (!string.IsNullOrEmpty(bindingContext.ModelName) && !bindingContext.ValueProvider.Keys.Any(s => s.StartsWith(bindingContext.ModelName)))
{
//this should only be true for top level objects
if (!bindingContext.FallbackToEmptyPrefix) return null;
bindingContext = new ModelBindingContext() { Model = bindingContext.Model, ModelState = bindingContext.ModelState, ModelType = bindingContext.ModelType, PropertyFilter = bindingContext.PropertyFilter, ValueProvider = bindingContext.ValueProvider };
}
ValueProviderResult vpr;
if (bindingContext.ValueProvider.TryGetValue(DefaultModelBinder.CreateSubPropertyName(bindingContext.ModelName, "index"), out vpr))
{
Type elementType = collType.GetGenericArguments()[0];
object model = bindingContext.Model ?? CreateModel(controllerContext, bindingContext, bindingContext.ModelType);
if (typeof(ICollection<>).MakeGenericType(new Type[] { elementType }).IsInstanceOfType(model))
{
ModelBindingContext collContext = new ModelBindingContext() { Model = model, ModelName = bindingContext.ModelName, ModelState = bindingContext.ModelState, ModelType = bindingContext.ModelType, PropertyFilter = bindingContext.PropertyFilter, ValueProvider = bindingContext.ValueProvider };
return UpdateCollectionByIndex(controllerContext, collContext, elementType, (string[])vpr.RawValue);
}
}
}
return base.BindModel(controllerContext, bindingContext);
}
protected object UpdateCollectionByIndex(ControllerContext controllerContext, ModelBindingContext bindingContext, Type elementType, string[] indexes)
{
IModelBinder binder = this.Binders.GetBinder(elementType);
List<object> items = new List<object>();
foreach (string index in indexes)
{
string prefix = string.Format("{0}[{1}]", bindingContext.ModelName, index);
if (!bindingContext.ValueProvider.Keys.Any(s => s.StartsWith(prefix))) continue;
ModelBindingContext objContext = new ModelBindingContext() { ModelName = prefix, ModelState = bindingContext.ModelState, ModelType = elementType, PropertyFilter = bindingContext.PropertyFilter, ValueProvider = bindingContext.ValueProvider };
object obj = binder.BindModel(controllerContext, objContext);
if (obj != null) items.Add(obj);
}
if (items.Count == 0) return null;
object collection = bindingContext.Model;
Type collectionType = typeof(ICollection<>).MakeGenericType(elementType);
MethodInfo addMethod = collectionType.GetMethod("Add");
MethodInfo clearMethod = collectionType.GetMethod("Clear");
clearMethod.Invoke(collection, null);
items.ForEach(o => addMethod.Invoke(collection, new object[] { o }));
return collection;
}
#endregion
}
}
Right now, everyone who used the beta code with model binding collections is broke.
That is incorrect. The overwhelming majority of users disliked the .index notation, but they put up with it anyway and just used sequential zero-based lists. Those applications still work (because they use zero-based lists), it's just that the .index fields
are unnecessary now and are ignored by the binder. Initial feedback from the community is that this change did much more good than bad.
I see your point, though, but I don't have much hope for .index notation making a comeback for RTM. Still, it couldn't hurt to ask around internally to see what the other devs think. :) I'll try to have a more definitive answer for you next week.
I'll try to put together a complete sample, but here was/is my use case. Assume Author and Book classes. In this case, an Author has a list of Books, and Book has a property to point back to the only Author (we'll ignore many to many for this discussion).
On page 1, we build up the Author object (demographic info, royalty, etc.). Pressing submit, you go to the books page where you tuck the Author values into hidden fields (e.g. Author.Name) and allow for an existing list of Books to be edited. The binding
on this page is still with Author, and if you create HTML fields like Author.Books[0].Price, everything binds up perfectly in RC1.
The problem I'm having is tying Author.Books[0].Price back to the actual HTML element so I can display validation errors (assume it's more complex than simply checking negative values so we don't get hung up there).
For now, I've changed the parameter on the controller from Author to IList<Book>, and by doing that, I can use the IndexModelBinder above (nice work, btw).I also have to new up an Author and populate it by bringing the hidden elements in manually.
So to sum up, I'm trying to use multiple pages to build up one object that has lists of other objects. On the final page, I would submit the object to the repository for saving. Am I just completely missing the point here and making things too tough on myself?
@levib - it is always good to have options. The sequential is nice and easy, but does break down in more complex scenarios and having an easy, workable fallback instead of writing a manual code would save a lot of time.
If it does not make it into the RTM, could we get some of the private static methods visibility changed to protected? Like VerifyValueUsability, ShouldUpdateProperty, ExtractGenericInterface, CanUpdateReadonlyTypeReference and ConvertProviderResult? All
of these would make it easier to create overridden model binders that behave the same way.
Also, CreateSubIndexName could take an object instead of an int, which would also help.
@dmiser - it doesn't sound like you are making it too complex, just need the flexibility
We opened a work item to reconsider this behavior. As Levi mentioned, it's not likely to make it in for ASP.NET MVC v1.0 since it's so late in the game, but it's on our list.
Thanks,
Eilon
Blog: http://weblogs.asp.net/LeftSlipper/
Marked as answer by levib on Feb 03, 2009 07:10 PM
I am another one who is unhappy that this behavior changed in RC1. I have found another change that has broken things for me. Say I have a controller action that gets posted to:
[AcceptVerbs("POST")]
public
string SaveBOM(int id,decimal?
freight,decimal? packaging,decimal?
other2,decimal? totalpieceprice,string
items)
{
//Definition...
}
I am passing xml through the items parameter. In beta, this works great. But now, this action is never being called unless I comment out the "string items" parameter. In my code its being called synchronously via AJAX. I set a break point
that never gets hits now.
There is definately a change in how the items parameter is being handled.
If anyone can help, it would be greatly appreciated.
bchance
Member
8 Points
19 Posts
RC ModelBinder breaking changes for collections
Jan 29, 2009 09:54 PM|LINK
Asked this on Scott Guthries blog and he suggested I post the question here:
Have a question about a breaking change with the ModelBinder. In the beta, when filling a collection it would look for a .index form field to know what indexes to process. This worked great because I could output one .index per item (order.Products.index = 1 would look for order.Products[1].<fieldname>), then when I added a new one I could use negative numbers (so orders.Products[-1].<fieldname>). You could have as many index fields and the HTTP post would make a nice comma seperated list of values. Super easy to inject new html for a new item into the page.
In the RC, it always starts at 0 and goes up until it cannot find any more. Besides making it harder to add because you have to know the next index, if you delete a member in the middle of your set, every one after will not get processed. For example, if 3 products were output ([0],[1],[2]), the middle one ([1]) is deleted via an AJAX call then the whole screen is saved, only the first ([0]) will be materialized with the model binder.
With the beta you could also use anything for the index, they did not have to be integers. This was nice.
Is there anyway to get the Beta functionality back, maybe flex if there is a .index? Or make some of the internal/private static methods more accessible (like DictionaryHelpers.DoesAnyKeyHavePrefix, VerifyValueUsability, CollectionHelpers.ReplaceCollection, ShouldUpdateProperty would also be useful). I have already written a new binder that handles the case, but would be nice out of the box (plus I cannot match exactly without some of the helper routines).
Brian Chance
levib
Star
7702 Points
1099 Posts
Microsoft
Re: RC ModelBinder breaking changes for collections
Jan 30, 2009 05:49 PM|LINK
The Beta behavior was fairly disliked by the community, so it's unlikely that it will be reintroduced. Some possible solutions:
If you're using AJAX to manipulate the set, then you shouldn't have to send the whole thing back to the server, since ostensibly each AJAX request would be responsible for adding, modifying, or deleting a single entry.
If you're using Javascript (sans AJAX) for deletions, you could replace products[1] with a dummy value. For example:
products[0].ProductName = "Chai Tea"
products[0].UnitPrice = "2.50"
products[1] = ""
productsToDelete = 1
products[2].ProductName = "Colombian Coffee"
products[2].UnitPrice = "3.00"
Note the introduction of two values, a placeholder for Products[1] and a key telling us which products in the array should be deleted. Pull this back like so:
public ActionResult MyMethod(List<Product> products, int[] productsToDelete) {
// iterate through productsToDelete (backward), deleting the specified indices from the products collection
}
Alternatively, have some Javascript that keeps track of the highest index and the index that was just deleted, and upon a product being deleted silently change the names of all the fields corresponding to the last product to match the product that was just deleted, then decrement the highest index count by one. This will plug any gaps in the list on submission to the server but is a little bit more work on the client.
Hope this helps! :)
dmiser
Member
6 Points
7 Posts
Re: RC ModelBinder breaking changes for collections
Jan 30, 2009 08:37 PM|LINK
levib
Star
7702 Points
1099 Posts
Microsoft
Re: RC ModelBinder breaking changes for collections
Jan 30, 2009 09:00 PM|LINK
The binder always has full knowledge of the entire field name that it's currently checking. In your example, the binder knows that it's trying to match a property UnitPrice from the field prefixed products[2], so it concatenates them to form a full field name of products[2].UnitPrice.
If that's not the scenario you're operating in, please clarify. It's possible that I just misunderstood what you were trying to do. :)
bchance
Member
8 Points
19 Posts
Re: RC ModelBinder breaking changes for collections
Jan 31, 2009 03:52 PM|LINK
I thnk what he is trying to say is that using a known index is easier to work with than a sequential number. products[<ProductID>].UnitPrice makes it easier to find related fields in JavaScript and server side code.
I know the validation message helpers output using the modelstate, but if you wanted to make messages based on other fields, they are easier to find (i.e. UnitPrice must be greater than UnitCost). Having to rely on positional notation when you could have a known key is shaky. Suppose I wanted to load a list of products from the database, they push the values in by calling UpdateModel for each. With positional notation, I would need to loop through all product[x].ProductID to find the match, then call update model with the correct product[x] prefix. With key based, I know they are product[<ProductID>], much easier.
On your previous comment to me: I understand where you guys believe the sequential is better, but honestly the code could flex based on the existence of a .index field and allow not integer indexes without too many changes. In the beta, I had honestly wished it would have a without a .index because sometimes sequential is just fine.
Right now, everyone who used the beta code with model binding collections is broke.
Here is an implementation that flexes based on index, not this only handles ICollection<> interfaces, I may get to IDictionary<,> and straight arrays when I get a chance. If so, I will post them here. (This should compile, I quickly took apart my LLBLGen model binder in notepad)
//Set ModelBinders.Binders.DefaultBinder = new IndexModelBinder(); in Global.asax.cs Application_Start using System; using System.Collections.Generic; using System.Collections; using System.Linq; using System.Text; using System.Web.Mvc; using System.ComponentModel; using System.Reflection; namespace System.Web.Mvc { public class IndexModelBinder : DefaultModelBinder { #region handle Beta style collections with .index public override object BindModel(ControllerContext controllerContext, ModelBindingContext bindingContext) { //handle collections with an indexer specified Type collType = bindingContext.ModelType.GetInterfaces().FirstOrDefault(t => t.IsGenericType && t.GetGenericTypeDefinition() == typeof(ICollection<>)); if (collType != null) { //check for existence of model name fields, if not, fall back to empty model name if (!string.IsNullOrEmpty(bindingContext.ModelName) && !bindingContext.ValueProvider.Keys.Any(s => s.StartsWith(bindingContext.ModelName))) { //this should only be true for top level objects if (!bindingContext.FallbackToEmptyPrefix) return null; bindingContext = new ModelBindingContext() { Model = bindingContext.Model, ModelState = bindingContext.ModelState, ModelType = bindingContext.ModelType, PropertyFilter = bindingContext.PropertyFilter, ValueProvider = bindingContext.ValueProvider }; } ValueProviderResult vpr; if (bindingContext.ValueProvider.TryGetValue(DefaultModelBinder.CreateSubPropertyName(bindingContext.ModelName, "index"), out vpr)) { Type elementType = collType.GetGenericArguments()[0]; object model = bindingContext.Model ?? CreateModel(controllerContext, bindingContext, bindingContext.ModelType); if (typeof(ICollection<>).MakeGenericType(new Type[] { elementType }).IsInstanceOfType(model)) { ModelBindingContext collContext = new ModelBindingContext() { Model = model, ModelName = bindingContext.ModelName, ModelState = bindingContext.ModelState, ModelType = bindingContext.ModelType, PropertyFilter = bindingContext.PropertyFilter, ValueProvider = bindingContext.ValueProvider }; return UpdateCollectionByIndex(controllerContext, collContext, elementType, (string[])vpr.RawValue); } } } return base.BindModel(controllerContext, bindingContext); } protected object UpdateCollectionByIndex(ControllerContext controllerContext, ModelBindingContext bindingContext, Type elementType, string[] indexes) { IModelBinder binder = this.Binders.GetBinder(elementType); List<object> items = new List<object>(); foreach (string index in indexes) { string prefix = string.Format("{0}[{1}]", bindingContext.ModelName, index); if (!bindingContext.ValueProvider.Keys.Any(s => s.StartsWith(prefix))) continue; ModelBindingContext objContext = new ModelBindingContext() { ModelName = prefix, ModelState = bindingContext.ModelState, ModelType = elementType, PropertyFilter = bindingContext.PropertyFilter, ValueProvider = bindingContext.ValueProvider }; object obj = binder.BindModel(controllerContext, objContext); if (obj != null) items.Add(obj); } if (items.Count == 0) return null; object collection = bindingContext.Model; Type collectionType = typeof(ICollection<>).MakeGenericType(elementType); MethodInfo addMethod = collectionType.GetMethod("Add"); MethodInfo clearMethod = collectionType.GetMethod("Clear"); clearMethod.Invoke(collection, null); items.ForEach(o => addMethod.Invoke(collection, new object[] { o })); return collection; } #endregion } }levib
Star
7702 Points
1099 Posts
Microsoft
Re: RC ModelBinder breaking changes for collections
Jan 31, 2009 07:38 PM|LINK
That is incorrect. The overwhelming majority of users disliked the .index notation, but they put up with it anyway and just used sequential zero-based lists. Those applications still work (because they use zero-based lists), it's just that the .index fields are unnecessary now and are ignored by the binder. Initial feedback from the community is that this change did much more good than bad.
I see your point, though, but I don't have much hope for .index notation making a comeback for RTM. Still, it couldn't hurt to ask around internally to see what the other devs think. :) I'll try to have a more definitive answer for you next week.
dmiser
Member
6 Points
7 Posts
Re: RC ModelBinder breaking changes for collections
Jan 31, 2009 09:31 PM|LINK
I'll try to put together a complete sample, but here was/is my use case. Assume Author and Book classes. In this case, an Author has a list of Books, and Book has a property to point back to the only Author (we'll ignore many to many for this discussion).
On page 1, we build up the Author object (demographic info, royalty, etc.). Pressing submit, you go to the books page where you tuck the Author values into hidden fields (e.g. Author.Name) and allow for an existing list of Books to be edited. The binding on this page is still with Author, and if you create HTML fields like Author.Books[0].Price, everything binds up perfectly in RC1.
The problem I'm having is tying Author.Books[0].Price back to the actual HTML element so I can display validation errors (assume it's more complex than simply checking negative values so we don't get hung up there).
For now, I've changed the parameter on the controller from Author to IList<Book>, and by doing that, I can use the IndexModelBinder above (nice work, btw).I also have to new up an Author and populate it by bringing the hidden elements in manually.
So to sum up, I'm trying to use multiple pages to build up one object that has lists of other objects. On the final page, I would submit the object to the repository for saving. Am I just completely missing the point here and making things too tough on myself?
bchance
Member
8 Points
19 Posts
Re: RC ModelBinder breaking changes for collections
Feb 02, 2009 01:24 PM|LINK
@levib - it is always good to have options. The sequential is nice and easy, but does break down in more complex scenarios and having an easy, workable fallback instead of writing a manual code would save a lot of time.
If it does not make it into the RTM, could we get some of the private static methods visibility changed to protected? Like VerifyValueUsability, ShouldUpdateProperty, ExtractGenericInterface, CanUpdateReadonlyTypeReference and ConvertProviderResult? All of these would make it easier to create overridden model binders that behave the same way.
Also, CreateSubIndexName could take an object instead of an int, which would also help.
@dmiser - it doesn't sound like you are making it too complex, just need the flexibility
Eilon
Contributor
5753 Points
976 Posts
Microsoft
Re: RC ModelBinder breaking changes for collections
Feb 02, 2009 04:53 PM|LINK
Hi everyone,
We opened a work item to reconsider this behavior. As Levi mentioned, it's not likely to make it in for ASP.NET MVC v1.0 since it's so late in the game, but it's on our list.
Thanks,
Eilon
danambrose
Member
16 Points
8 Posts
Re: RC ModelBinder breaking changes for collections
Feb 03, 2009 05:50 PM|LINK
I am another one who is unhappy that this behavior changed in RC1. I have found another change that has broken things for me. Say I have a controller action that gets posted to:
[AcceptVerbs("POST")]
public string SaveBOM(int id,decimal? freight,decimal? packaging,decimal? other2,decimal? totalpieceprice,string items) {//Definition...
}
I am passing xml through the items parameter. In beta, this works great. But now, this action is never being called unless I comment out the "string items" parameter. In my code its being called synchronously via AJAX. I set a break point that never gets hits now.
There is definately a change in how the items parameter is being handled.
If anyone can help, it would be greatly appreciated.