Model Binder and Collections

[Kevin Amerson]

I'm seeing some strange behavior in the model binder setting items in a collection.  It seems like the binder is adding the items twice, and this method throws an illegal operation exception indicating that values in the collection in the loop have been modified, seems like "value" and "entityCollection" are the same collection.

/// <summary>
        /// If the property being bound is a simple, generic collection of entiy objects, then use
        /// reflection to get past the protected visibility of the collection property, if necessary.
        /// </summary>
        private void SetEntityCollectionProperty(ModelBindingContext bindingContext,
            PropertyDescriptor propertyDescriptor, object value) {

            if (value as IEnumerable != null &&
                IsSimpleGenericBindableEntityCollection(propertyDescriptor.PropertyType)) {

                object entityCollection = propertyDescriptor.GetValue(bindingContext.Model);
                Type entityCollectionType = entityCollection.GetType();

                foreach (object entity in (value as IEnumerable)) {
                    entityCollectionType.InvokeMember("Add",
                        BindingFlags.Public | BindingFlags.Instance | BindingFlags.InvokeMethod, null, entityCollection,
                        new object[] { entity });
                }
            }
        }

Here's the view snippet that causes the exception:

<input type="hidden" name="Client.Names.Index" value="0" />
<input type="text" name="Client.Names[0].GivenName" class="short-text-box2" />

And of course the Client class has an IList<Name> Names { get; protected set; }

The Name class just has five properties.

The collection of names seem to be getting set twice.  Is there a different way I should be setting up the view?

[David Archer]

Kevin,

I've run into this problem as well and I've come up with some changes
to the SharpModelBinder that resolve this issue. I've posted the
updated code to Issue 129: http://code.google.com/p/sharp-architecture/issues/detail?id=129
along with some comments. You should be able to add this to your Web
project and just update the Global.asax.cs to use the new
CustomSharpModelBinder as the default binder instead of the
SharpModelBinder from the SharpArch.Web assembly.

After digging into the SharpModelBinder code, I've got some concerns
on the rationale for how it is currently implemented in trunk.

1) It is handling binding scenarios for both Entities and Entity
Collections but if we only implemented custom binding for Entities,
the default model binder implementation will create the generic
collection and call out to the custom binding code for each entity
that needs to be fetched from the DB. This may be the main reason why
the issues with binding collections are happening.

2) Implementing custom valid providers seems more complicated than
just putting the code to pull entities from the DB in the binder. I'm
not sure if this design was to promote Seperation of Concerns but it
isn't strictly necessary and IMO just makes the binder more complex
and harder to reason about.

The updated code I posted basically just gets rid of all the custom
value provider code and implements pulling a single Entity from the DB
only when the Model.Name is an exact match to a value we've been
provided by the default value provider. This means we only pull from
the DB when your form contains a value that maps directly to an entity
property or a subindex of an entity collection property (see examples
below). Otherwise, we let the default model binding behavior create
new entities and set all the properties as necessary. I then added
back in all the validation-related code as well as the code for
setting the protected ID property.

1) Controller action taking lists of entities directly

public ActionResult Something(IList<MyEntity> ExistingEntities,
IList<MyEntity> NewEntities)
{ ... }

View Code:
<!-- These are ID values and the list will be populated with entity
objects from the DB -->
<input name="ExistingEntities[0]" value="1" />
<input name="ExistingEntities[1]" value="2" />
<input name="ExistingEntities[2]" value="3" />

<!-- The list will be populated with new entity objects and the Number
property set -->
<input name="NewEntities[0].Number" value="ABC" />
<input name="NewEntities[1].Number" value="DEF" />
<input name="NewEntities[2].Number" value="GHI" />

2) Controller action taking a viewmodel object that contains lists of
entities

public ActionResult Something(MyEntityViewModel viewModel)
{ ... }

public class MyEntityViewModel
{
public IList<MyEntity> NewEntities { get; set; }
public IList<MyEntity> ExistingEntities { get; set; }
}

View Code (The same view code from above works as well):

<!-- These are ID values and the list will be populated with entity
objects from the DB -->
<input name="viewModel.ExistingEntities[0]" value="1" />
<input name="viewModel.ExistingEntities[1]" value="2" />
<input name="viewModel.ExistingEntities[2]" value="3" />

<!-- The list will be populated with new entity objects and the Number
property set -->
<input name="viewModel.NewEntities[0].Number" value="ABC" />
<input name="viewModel.NewEntities[1].Number" value="DEF" />
<input name="viewModel.NewEntities[2].Number" value="GHI" />

---

Hopefully Billy or anyone else that has worked on the
DefaultModelBinder can review this and provide some feedback.

On Nov 25, 8:55 pm, Kevin Amerson <kevin.amer...@gmail.com> wrote:
> I'm seeing some strange behavior in the model binder setting items in a
> collection.  It seems like the binder is adding the items twice, and this
> method throws an illegal operation exception indicating that values in the
> collection in the loop have been modified, seems like "value" and
> "entityCollection" are the same collection.
>
> /// <summary>
>         /// If the property being bound is a simple, generic collection of
> entiy objects, then use
>         /// reflection to get past the protected visibility of the
> collection property, if necessary.
>         /// </summary>
>         private void SetEntityCollectionProperty(ModelBindingContext
> bindingContext,
>             PropertyDescriptor propertyDescriptor, object value) {
>
>             if (value as IEnumerable != null &&
>
> IsSimpleGenericBindableEntityCollection(propertyDescriptor.PropertyType)) {
>
>                 object entityCollection =
> propertyDescriptor.GetValue(bindingContext.Model);
>                 Type entityCollectionType = entityCollection.GetType();
>

>                 *foreach (object entity in (value as IEnumerable)) {

>                     entityCollectionType.InvokeMember("Add",
>                         BindingFlags.Public | BindingFlags.Instance |
> BindingFlags.InvokeMethod, null, entityCollection,
>                         new object[] { entity });

>                 }*

[Kevin Amerson]

Hey David,

I did end up writing a custom binder myself as well to pull out the pieces that I didn't want around how collections were being handled.  I'm not sure I understand going to the database to pull entities.  NHibernate's auto-merge should be used when the object is saved, I don't really want to go to the database during form serialization.

The flow that I'm after then is:

1. Post/Put form data
2. Serialize into model including model ids from hidden form fields where necessary
3. Enter the controller action with the transient model objects
4. Transient object from serialization is merged by NHibernate within the NHib session using NHib's merge so that if it needs to query db first, it will do so.

The only part failing for me was setting and resetting members of the collection so essentially I'd get an exception saying that the key already exists in the collection during serialization of the form.  Taking that code out of the binder and just having the MVC binder do the collection serialization works out.

I didn't ever post the code or the bug because I don't think we've standardized on the syntax/serialization and expected behavior for posting these forms back to the controller.  So I wasn't sure if I was just breaking a convention in my view that others are expecting to use, and therefore needed different behavior from the binder.

I'll see if I can grab your custom binder and test it out.

Thanks!
Kevin

--

You received this message because you are subscribed to the Google Groups "S#arp Architecture" group.
To post to this group, send email to sharp-arc...@googlegroups.com.
To unsubscribe from this group, send email to sharp-architect...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/sharp-architecture?hl=en.