Item in child collection not being removed

[danspam]

Removing an item from a collection mapped as extra-lazy has sometimes no effect: the item stays in the collection.
This happens when the remove is done while the collection is not yet initialized, and if it is enumerated after the remove. In such case, the enumeration not only yields the item, but also at later flush, the item is not deleted from the collection.

Corresponding Hibernate issue: https://hibernate.atlassian.net/browse/HHH-5742

Initial report follows:

---

Not sure if this is a bug or something I am doing but I have an entity with a child collection, exposed as an IEumerable<> and I am having issues removing the item:

public class Parent {
    private ISet<ChildItem> _childItems;

    public virtual IEnumerable<ChildItem> ChildItems
    {
            get { return _childItems; }
    }

    public virtual void RemoveChildItem(ChildItem item)
    {
           ...
            _childItems.Remove(item);
           ...
    }
}

I do the following to remove the item:

myChildItem.Parent.RemoveChildItem(myChildItem);

If I stick a breakpoint after the _childItems.Remove(item); line then I discover that the _childItems collection still contains the item.

I think it has something with initialising the _childItems collection (extra lazy load) because if I add a check before it like:

if (!_childItems.Any(c => c == item)) throw new Exception();

then the _childItems.Remove(item); works correctly. Likewise if I put a breakpoint before the remove and inspect the collection (causing it to initialise), then it works.

[hazzik]

It's by design. The deletion of items is queued and performed on a session flush. Do you experience any real troubles apart from seeing the removed item in the debugger?

[danspam]

The real issue is that the item is never deleted unless I initialise the collection first.

Also, if I want to do anything with the collection afterwards, like do something with the count of items, it is incorrect as the item was not removed.

[fredericDelaporte]

There is already a very complete test of lazy="extra" with removal of elements here, and it does not fail, while the collection remains uninitialized during the test (checked with an assert on IsInitialized).

Can you provide a failing test case? (Preferably added in the test fixture linked above, using the same entities if possible.)

[danspam]

No problem, I can give that go when I get a spare moment. May have to add to the entities, as I think this may have something to do with read only properties (IEnumerable).

[danspam]

See pull request #1876 for failing test.

[danspam]

If you uncomment line 132, the test passes.

[fredericDelaporte]

The easiest fix seems to me to change this code:

nhibernate-core/src/NHibernate/Collection/AbstractPersistentCollection.cs

Lines 460 to 471 in 0d61f46

	protected virtual void Initialize(bool writing)
	{
	if (!initialized)
	{
	if (initializing)
	{
	throw new LazyInitializationException("illegal access to loading collection");
	}
	ThrowLazyInitializationExceptionIfNotConnected();
	session.InitializeCollection(this, writing);
	}
	}

By adding in it

if (HasQueuedOperations)
    session.Flush();

on line 469, right before InitializeCollection.

But this would then disable half of the operation queue features, which has two usages:

1. On flush, it allows the session to flush pending addition/removal done on non-initialized extra-lazy collections.
2. After collection load, it allows the collection to adjust its content according to additions and removals done on it before loading it.

Doing the above change would disable the second usage, since the first one would always be triggered by the collection load. So it does not seem to be the adequate fix. (Or else, it would have to also remove all the code supporting the second usage, since it will be turned to dead code otherwise.)

This also implies that the bug lies in the second usage. Indeed, the trouble is the sequence of events during it:

1. The collection data is loaded.
2. The collection data is adjusted according to pending operations.
3. The collection snapshot is taken.

The third point occurs too late, the snapshot being the reference for checking what changes has occurred on the collection. The snapshot should be taken on the unmodified loaded data, not on the data adjusted according to pending operations.
By the way, the second point will fail if pending removal were made with detached entities not properly handling Equals and GetHashCode. But I consider this as expected, and to be fixed on user side. (The failure will be lack of removal: it will attempt to remove from the set an element that the set will consider as not equal to any of its elements.)

I am tempted to do the easiest fix, because anyway, many operations on an extra-lazy collection do already force a flush on the session. By example, doing a second remove will force a flush. In fact, with current implementation of the NHibernate generic set, only one operation can be queued. Following ones will trigger a forced flush, emptying the queue before being added themselves.

So this feature of allowing loading the collection without flushing pending operations has already little usefulness in the set case.

That said, it may be possible to improve the logic for reducing the cases triggering a forced flush, which would be better. A forced flush triggered by simply adding or removing elements into a collection, or just querying (Contains) it, is likely to be unexpected by many users. With FlushMode.Auto, we could explain that is to be expected with an extra-lazy collection which does query the database, but it does occurs even with auto-flushes disabled.
Doing the simplest fix would be going into the wrong direction, regarding this concern.