ObservableHashSet collection changed notifications

[sjb-sjb]

I am using change notifying entities. Entity Framework Core uses ObservableHashSet for the collections as described in https://blog.oneunicorn.com/2016/11/16/notification-entities-in-ef-core-1-1.

The problem I have encountered is that when I call ObservableHashSet.Clear, the collection change notification that it generates is a Replace change with OldItems = the current items and NewItems = an empty list. Semantically this would appear to be fine, however it doesn't play well with .NET and UWP classes.

Specifically, the UWP ListView class expects that Replace only occurs with an equal number of new and old items -- in other words, a 1 to 1 replacement. To delete items in a ListView one must use a Remove notification instead of a Replace. Consequently, one cannot bind a ListView to the collection provided by Entity Framework Core.

I recommend that the ObservableHashSet collection change notifications be adjusted so that Replace is only used for a 1 to 1 replacement with NewStartingIndex = OldStartingIndex.

There is a good article explaining the NotifyCollectionChangedEventArgs at http://blog.stephencleary.com/2009/07/interpreting-notifycollectionchangedeve.html

• sjb

[ajcvickers]

Short answer: use ObservableCollection for data binding. It's less efficient, but it preserves order and matches the semantics required by UWP.

[sjb-sjb]

Thanks for your reply. I take your point about ListView requiring an ordering, which hash sets don't have. Having said that, it would make sense to me that navigation sets and ListView work together right out of the box.

Maybe EF could use a combined HashSet and ObservableCollection for this. The hash set would give fast tests for membership while ObservableCollection would enable the use of ListView. The addition or removal of elements would still be reasonably fast given that the ObservableCollection is (I believe) implemented using bit-blitting.

[divega]

From what I remember we decided not to make things more complicated than they needed to be. A more efficient ObservableCollection maybe a general need for UWP/WPF applications.

[sjb-sjb]

Well, I agree the combined data structure is a bit complicated, and not very elegant.

I am not aware of how Contains( entity) is implemented for ObservableCollection, however it would make sense that it map to a firmware operation that searches an array for a bit pattern equal to the pointer to the entity. Theoretically linear time, but in reality very fast. If that is the case then from a performance point of view I would be happy with ObservableCollection as the default and be able to override it to ObservableHashSet if necessary e.g. for a really large collection.

It's a question of how large you think the relationship collections will be in most cases and what they will be used for. Personally my use would be like typically less than 64 and basically listed in UI for a user operation. If the collection is really large then one is probably going to be doing some kind of lazy loading, rather than loading them all via an Include. For example a drill-down type of situation … well in that case it would likely be UI. It's only if you are doing a lot of calculation and no UI that you really need the hash set and don't need the ObservableCollection.

[divega]

@sjb-sjb just to improve my understanding, when you mention the “default”, are you referring to what DbContext scaffolding generates when you point it to an existing database? Or what we instantiate if you make the collection of an interface type?

[ajcvickers]

Marking for re-triage. We could consider:

• Allowing the type we use to be configured
• Making ObservableCollection the default for notification entities, based on the assumption that the most common reason for using them is data binding. However, we don't really have much evidence that this assumption is correct.

[ajcvickers]

Putting this on the backlog to make the type configurable.

[sjb-sjb]

@divega, by "default" I meant what type EF instantiates for a collection, if your class does not already instantiate it. I don't feel strongly about this point, because there is a fairly easy workaround just by instantiating the collection yourself. I guess that instantiating it yourself is less efficient if the collection is frequently unused.

One other point about using ObservableCollection for related entities. The point of using an ObservableCollection instead of an observable hash set is, obviously, I want the ordering to be preserved. In my case I want it for the UI which has a ListView / TreeView.

However, EF does not preserve the ordering through a Save and subsequent materialization. During a materialization, it looks like EF puts children into relationship collections in a more or less random order... perhaps it is iterating over a hash table?

What would be far more convenient would be if EF would consider the materialized entities in the ordering provided by the query results. I can easily query the entities in an ordering consistent with the desired order in all of the ObservableCollections (by storing the ordering field in the queried entities). I just need EF to then apply ObservableCollection.Add in the same order as I queried.

Failing that, as a workaround, what I am doing is redefining my collection's Add method so that it adds the entity at the correct location in the collection, rather than at the end of the collection. This, of course, is inconsistent with the usual definition of Add.

[divega]

@sjb-sjb thanks for clarifying. FWIW, we are tracking ordered collection support in the backlog at #2919.

[DamirLisak]

There is another incorrect implementation of the INotifyCollectionChanged interface in the method Remove(T item) in Line 155:
https://github.com/dotnet/efcore/blob/main/src/EFCore/ChangeTracking/ObservableHashSet.cs#L155

In line 153, the element is removed first:
_set.Remove(item);
and then OnCollectionChanged() is called in line 155 with NotifyCollectionChangedAction.Remove:
OnCollectionChanged(NotifyCollectionChangedAction.Remove, item);
However, no index is passed!

A method with an index parameter is therefore missing:

private void OnCollectionChanged(NotifyCollectionChangedAction action, object? item, int index)
    => OnCollectionChanged(new NotifyCollectionChangedEventArgs(action, item, index));

We noticed this problem when binding navigation properties to the ItemSource property of ItemsControls (WPF), because the EnumerableCollectionView throws an exception at line 425 because of the index of -1:
https://github.com/dotnet/wpf/blob/026f338641b847dace824f36376beae5f5ad021a/src/Microsoft.DotNet.Wpf/src/PresentationFramework/MS/Internal/Data/EnumerableCollectionView.cs#L423

On the other hand, ObservableCollection are programmed correctly because they pass the index parameter:
https://github.com/microsoft/referencesource/blob/master/System/compmod/system/collections/objectmodel/observablecollection.cs#L203

[sjb-sjb]

@DamirLisak isn’t that technically correct to omit the index? Since hash sets do not have an ordering on them, there is no specific index associated with the element of the set —

[DamirLisak]

@sjb-sjb, then ObservableHashSet must not implement INotifyCollectionChanged. ObservableHashSet encapsulates a HashSet in the _set member. A HashSet, in turn, stores its elements in _entries, which is iterated by an enumerator when WPF creates its CollectionViews by binding. The index in _entries can be retrieved using the FindItemIndex method. However, the method is private. It only becomes more expensive in terms of INotifyCollectionChanged if the HashSet needs to increase its capacity.

I know, that a MVVM-Binding can be done to another Collection that is a copy of this ObservableHashSet. But if I don't need a sorted set, why should I use a copy with the side effect that the memory consumption increases. Also the implementation effort is unnecessarily increased. Unfortunately, since Microsoft is focusing on service business, Azure and ASP, a lot of concepts like Lazy Loading and MVVM-Binding doesn't play a role anymore.

[ajcvickers]

@DamirLisak ObservableHashSet is not suitable to use for data binding since it doesn't preserve order; use ObservableCollection for that. EF never forces you to use ObservableHashSet for anything. It is there because notification for change tracking (not using data binding) with a large collection can be slow.

[DamirLisak]

@ajcvickers , we use Lazy Loading without proxies. Where can I control or declare which Collection-Type is created?
The LazyLoader returns always ObservableHashSet when the Method GetOrCreateCollection is called inside of LazyLoader.Load():

efcore/src/EFCore/Metadata/Internal/ClrCollectionAccessor.cs

Line 111 in 253debb

private ICollection<TElement> GetOrCreateCollection(object instance, bool forMaterialization)

In this example _XXX_Collection is always a ObservableHashSet:

    private ICollection<XXX> _XXX_Collection;
    public virtual ICollection<XXX> XXX_Collection
    {
        get => LazyLoader.Load(this, ref _XXX_Collection);
        set => _XXX_Collection= value;
    }

[DamirLisak]

@ajcvickers : I apologize. This is my ignorance. I just tried instantiating an ObservableCollection beforehand and the LazyLoader accepts that. Thanks for your comment!

[DamirLisak]

@ajcvickers : I'm sorry, but still I have to come back to the actual problem. Why is the CollectionChanged event not implemented correctly and returns -1 as an index in NotifyCollectionChangedEventArgs? I mean, if INotifyCollectionChanged is already implemented, then it should be implemented correctly. According to the documentation, -1 is an invalid value and must be >= 0. https://learn.microsoft.com/en-us/dotnet/api/system.collections.specialized.notifycollectionchangedeventargs.oldstartingindex?view=net-7.0

Or why is ObservableHashSet used in EF-Core, because when I compare it to EF4, the EntityCollection also encapsulated the HashSet for performance reasons, but did not implement INotifyCollectionChanged and was therefore not a problem for MVVM.

So if you say "it doesn't preserve order" then INotifyCollectionChanged also mustn't be implemented.

[sjb-sjb]

@DamirLisak the INotifyCollectionChanged interface can be implemented by an ordered collection such as a list but also by an unordered collection such as a set. If you look at the constructors for the NotifyCollectionChangedEventArgs, there are versions that take the index location of the change but also versions that omit the index, for collections where the index is not meaningful. For example if you are doing an Add in a set, there is no specific location (index) in the set where you are adding the item. I don’t doubt your description of the implementation of HashSet, but the index locations are not stable, they are time and implementation dependent and are not part of the logical specification of a HashSet.

[DamirLisak]

@sjb-sjb : Thanks for the explanation.
Then, as a logical consequence, the documentation of INotifyCollectionChanged should be made more precise and it should be pointed out that unsorted collections are allowed. The whole text is related to sorted lists and only the phrase "You can enumerate over any collection that implements the IEnumerable interface" could imply the use of an unsorted collection.
So I see this is a construction site for WPF.

[sjb-sjb]

There’s a nice bit of private sector documentation by Stephen Cleary .