Modify the collections of an entity "collection was modified" exception

[Matteo Migliore]

Hi,

I obtain the error "Collection was modified; enumeration operation may not execute." when I try to save (merge)
a loaded entity.

I use AutoMapper to write the values from the DTO (command) to the loaded entity and this method to synchronize
the collection between the entity and the command:

public static ICollection<TFirst> Synchronize<TFirst, TSecond>(
	this ICollection<TFirst> first,
	IEnumerable<TSecond> second,
	Func<TSecond, TFirst> convert = null,
	Func<TFirst, int> firstHash = null,
	Func<TSecond, int> secondHash = null)
{
	if (firstHash == null)
	{
		firstHash = x => x.GetHashCode();
	}

	if (secondHash == null)
	{
		secondHash = x => x.GetHashCode();
	}

	if (convert == null)
	{
		convert = x => Mapper.Map<TFirst>(x);
	}

	var firstCollection = first.ToDictionary(x => firstHash(x), x => x);
	var secondCollection = second.ToDictionary(x => secondHash(x), x => x);

	var toAdd = secondCollection.Where(item => firstCollection.All(x => x.Key != item.Key)).Select(x => convert(x.Value));

	foreach (var item in toAdd)
	{
		first.Add(item);
	}

	var toRemove = firstCollection.Where(item => secondCollection.All(x => x.Key != item.Key));

	foreach (var item in toRemove)
	{
		first.Remove(item.Value);
	}

	return first;
}

How can I fix the problem?

Thanks,
Matteo

[Chris Bingham]

sounds like you need a .ToList() somewhere...

i'd guess either:
1: What do you pass in as argument for the 'second' param (the IEnumerable<TSecond>) ? if it's a queryable, then try and ToList() it before enumerating?

2: .ToDictionary is lazily evaluated, so you need to .ToList() before calling .ToDictionary..

[Pete Appleton]

Change

var toRemove = firstCollection.Where(item => secondCollection.All(x => x.Key != item.Key));

to
var toRemove = firstCollection.Where(item => secondCollection.All(x => x.Key != item.Key)).ToArray();

As it stands, your code is lazily iterating toRemove; as soon as something's removed from it, you'll get exception you described. This is not related to NHibernate, it's a standard 'feature' of .NET :).

/Pete

--
You received this message because you are subscribed to the Google Groups "nhusers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to nhusers+u...@googlegroups.com.
To post to this group, send email to nhu...@googlegroups.com.
Visit this group at http://groups.google.com/group/nhusers.
For more options, visit https://groups.google.com/groups/opt_out.

[Matteo Migliore]

Hi Chris and Pete :)

You are right, I suspected that was a problem of deffered execution.

Now I try your suggestion :)

Thanks!
Matteo

[Matteo Migliore]

I've a unit test of the above method, it works correctly with List<T> for example,
now I changed the implementation calling the ToArray but the problem is the same.

For now I solved cloning the entity using the BinaryFormatter, but clearly is not the final solution.

Any idea?

Thanks!
Matteo

On Thursday, July 4, 2013 5:11:30 PM UTC+2, PeteA wrote:

[Matteo Migliore]

Chris, Pete

the ToDictionary is not deferred exactly the ToList and ToArray,
so for me is not needed to do anything more.

Instead the clone of the entity works, but I don't like it, because the collections
are modified also if not necessary.

Idea? :)

Thanks!
Matteo

On Thursday, July 4, 2013 5:11:30 PM UTC+2, PeteA wrote:

[Pete Appleton]

Cloning is definitely the wrong solution IMO, the underlying issue is exactly what the exception message says. My original code assumed that 'toRemove' was the collection that was getting modified during iteration, but clearly I'm wrong if it didn't work. Best suggestion for you is to carefully work through the exception stack trace to find out which collection is the problem; once you've found that, then use the ToList() / ToArray() technique. NB, you may find it helpful to untick the 'Just my code' option in the VS debugger options.

To unsubscribe from this group and stop receiving emails from it, send an email to nhusers+u...@googlegroups.com <javascript:> .
To post to this group, send email to nhu...@googlegroups.com <javascript:> .
Visit this group at http://groups.google.com/group/nhusers <http://groups.google.com/group/nhusers> .
For more options, visit https://groups.google.com/groups/opt_out <https://groups.google.com/groups/opt_out> .

[Matteo Migliore]

Hi Pete,

I completely agree, clonin is just a patch, but I placed ToArray
everywhere and the problem persists.

I also wrote a unit test and with not lazy collection everything works fine.

I've the exception when I call Session.Merge, and the StackTrace does
not contain nothing,
also without "Just my code" enabled.

I really cannot understand the problem and how to reproduce it in a unit test.

> You received this message because you are subscribed to a topic in the Google Groups "nhusers" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/nhusers/OLXzSw0sU2s/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to nhusers+u...@googlegroups.com.

[Matteo Migliore]

I found the issue, it was because I did the mapping from the DTO (command) to the entity.

In a child entity there was a navigation property to the parent that came from the mapping (AutoMapper) instead that from the loaded entity from the session. So the Child.Parent instance was different from "var parent = Session.Load(id)" instance.

This situation caused the problem. Now I copied the loaded instance in the child property and everything works.

P.S.

So the Synchronize method is correct, is possible to use it.

Thanks!

> To unsubscribe from this group and stop receiving emails from it, send an email to nhusers+unsubscribe@googlegroups.com.

> To post to this group, send email to nhu...@googlegroups.com.
> Visit this group at http://groups.google.com/group/nhusers.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>
>
> --
> You received this message because you are subscribed to a topic in the Google Groups "nhusers" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/nhusers/OLXzSw0sU2s/unsubscribe.

> To unsubscribe from this group and all its topics, send an email to nhusers+unsubscribe@googlegroups.com.

[Pete Appleton]

Thanks for the update - glad you've got it sorted :).  Collection sync is always a pain…

 

I've just started testing a custom AutoMapper IObjectMapper implementation which does a full model collection <-- --> entity collection sync; if anyone's interested in giving feedback or improvements then drop me a note.  It's currently at a very primitive stage, but works for basic scenarios and allows code such as

 

return this.WorkUnit.Execute( (session, context) => {

                var entity = session.Get<Entity>(model.Id);

                if(entity == null) context.Fail(…);

                return context.Map(model, entity);                                        // Entire object graph gets synced here

})

 

/Pete

> To unsubscribe from this group and stop receiving emails from it, send an email to nhusers+u...@googlegroups.com.

> To post to this group, send email to nhu...@googlegroups.com.
> Visit this group at http://groups.google.com/group/nhusers.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>
>
>
> --
> You received this message because you are subscribed to a topic in the Google Groups "nhusers" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/nhusers/OLXzSw0sU2s/unsubscribe.

> To unsubscribe from this group and all its topics, send an email to nhusers+u...@googlegroups.com.

> To post to this group, send email to nhu...@googlegroups.com.
> Visit this group at http://groups.google.com/group/nhusers.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

--

You received this message because you are subscribed to the Google Groups "nhusers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to nhusers+u...@googlegroups.com.

[Matteo Migliore]

Great Pete, interesting!

Can you post a little sample solution? :)

Thanks!
Matteo