Undocumented breaking change in 5.5.0, Session.Save now calls GetHashCode of related entities?

[cremor]

After updating from NHibernate 5.4.7 to 5.5.0 some of the tests in my application started failing. It looks like this is because of an undocumented breaking change.

NHibernate 5.5.0 now calls the GetHashCode method of entities which are implicitly saved by having a cascaded save from an entity that is saved by calling session.Save. Simplified model:

// An object of this class is passed to 'session.Save'
public class ParentEntity
{
    // Mapped with a sequence generator
    public virtual long Id { get; set; }

    // Mapped as a set with 'all-delete-orphan'
    public virtual ICollection<SubEntity> SubEntities { get; set; }
}

public class SubEntity
{
    // Mapped with a sequence generator
    public virtual long Id { get; set; }
}

The GetHashCode method of SubEntity is called in this case.

The call stack looks like this:

 	MyNamespace.Infrastructure.Entity<long>.GetHashCode()
 	System.Collections.Generic.ObjectEqualityComparer<System.__Canon>.GetHashCode(System.__Canon obj)
 	System.Collections.Generic.HashSet<MyEntityType>.InternalGetHashCode(MyEntityType item)
 	System.Collections.Generic.HashSet<MyEntityType>.AddIfNotPresent(MyEntityType value)
 	System.Collections.Generic.HashSet<MyEntityType>.UnionWith(System.Collections.Generic.IEnumerable<MyEntityType> other)
 	System.Collections.Generic.HashSet<MyEntityType>.HashSet(System.Collections.Generic.IEnumerable<MyEntityType> collection, System.Collections.Generic.IEqualityComparer<MyEntityType> comparer)
 	NHibernate.Type.GenericSetType<MyEntityType>.Wrap(NHibernate.Engine.ISessionImplementor session, object collection)
 	NHibernate.Event.Default.WrapVisitor.ProcessArrayOrNewCollection(object collection, NHibernate.Type.CollectionType collectionType)
 	NHibernate.Event.Default.WrapVisitor.ProcessComponent(object component, NHibernate.Type.IAbstractComponentType componentType)
 	NHibernate.Event.Default.WrapVisitor.ProcessValue(int i, object[] values, NHibernate.Type.IType[] types)
 	NHibernate.Event.Default.AbstractVisitor.ProcessEntityPropertyValues(object[] values, NHibernate.Type.IType[] types)
 	NHibernate.Event.Default.AbstractSaveEventListener.VisitCollectionsBeforeSave(object entity, object id, object[] values, NHibernate.Type.IType[] types, NHibernate.Event.IEventSource source)
 	NHibernate.Event.Default.AbstractSaveEventListener.PerformSaveOrReplicate(object entity, NHibernate.Engine.EntityKey key, NHibernate.Persister.Entity.IEntityPersister persister, bool useIdentityColumn, object anything, NHibernate.Event.IEventSource source, bool requiresImmediateIdAccess)
 	NHibernate.Event.Default.AbstractSaveEventListener.PerformSave(object entity, object id, NHibernate.Persister.Entity.IEntityPersister persister, bool useIdentityColumn, object anything, NHibernate.Event.IEventSource source, bool requiresImmediateIdAccess)
 	NHibernate.Event.Default.AbstractSaveEventListener.SaveWithGeneratedId(object entity, string entityName, object anything, NHibernate.Event.IEventSource source, bool requiresImmediateIdAccess)
 	NHibernate.Event.Default.DefaultSaveOrUpdateEventListener.SaveWithGeneratedOrRequestedId(NHibernate.Event.SaveOrUpdateEvent event)
 	NHibernate.Event.Default.DefaultSaveOrUpdateEventListener.EntityIsTransient(NHibernate.Event.SaveOrUpdateEvent event)
 	NHibernate.Event.Default.DefaultSaveOrUpdateEventListener.OnSaveOrUpdate(NHibernate.Event.SaveOrUpdateEvent event)
 	NHibernate.Impl.SessionImpl.FireSave(NHibernate.Event.SaveOrUpdateEvent event)
 	NHibernate.Impl.SessionImpl.Save(object obj)

With NHibernate 5.4.7 this call to GetHashCode via session.Save didn't happen at all! Instead GetHashCode is only called by session.Flush with this call stack:

 	MyNamespace.Infrastructure.Entity<long>.GetHashCode()
 	System.Collections.Generic.ObjectEqualityComparer<System.__Canon>.GetHashCode(System.__Canon obj)
 	System.Collections.Generic.HashSet<MyEntityType>.InternalGetHashCode(MyEntityType item)
 	System.Collections.Generic.HashSet<MyEntityType>.AddIfNotPresent(MyEntityType value)
 	System.Collections.Generic.HashSet<MyEntityType>.UnionWith(System.Collections.Generic.IEnumerable<MyEntityType> other)
 	System.Collections.Generic.HashSet<MyEntityType>.HashSet(System.Collections.Generic.IEnumerable<MyEntityType> collection, System.Collections.Generic.IEqualityComparer<MyEntityType> comparer)
 	NHibernate.Type.GenericSetType<MyEntityType>.Wrap(NHibernate.Engine.ISessionImplementor session, object collection)
 	NHibernate.Event.Default.WrapVisitor.ProcessArrayOrNewCollection(object collection, NHibernate.Type.CollectionType collectionType)
 	NHibernate.Event.Default.WrapVisitor.ProcessComponent(object component, NHibernate.Type.IAbstractComponentType componentType)
 	NHibernate.Event.Default.WrapVisitor.ProcessValue(int i, object[] values, NHibernate.Type.IType[] types)
 	NHibernate.Event.Default.AbstractVisitor.ProcessEntityPropertyValues(object[] values, NHibernate.Type.IType[] types)
 	NHibernate.Event.Default.DefaultFlushEntityEventListener.WrapCollections(NHibernate.Event.IEventSource session, NHibernate.Persister.Entity.IEntityPersister persister, NHibernate.Type.IType[] types, object[] values)
 	NHibernate.Event.Default.DefaultFlushEntityEventListener.OnFlushEntity(NHibernate.Event.FlushEntityEvent event)
 	NHibernate.Event.Default.AbstractFlushingEventListener.FlushEntities(NHibernate.Event.FlushEvent event)
 	NHibernate.Event.Default.AbstractFlushingEventListener.FlushEverythingToExecutions(NHibernate.Event.FlushEvent event)
 	NHibernate.Event.Default.DefaultFlushEventListener.OnFlush(NHibernate.Event.FlushEvent event)
 	NHibernate.Impl.SessionImpl.Flush()

The difference is that in the NHibernate 5.4.7 case the generated Id is already set on the entity when GetHashCode is called. But in the NHibernate 5.5.0 case the Id is not generated yet. (It is mapped with a sequence generator in my case.)

Was this changed on purpose? If yes, it should be documented (and I have to find a fix for my application 😢)
If not, then please revert the change.

[cremor]

@csharper2010 It looks like your PR #3326 could be the cause of this.

[csharper2010]

Yes, that may well be the case.

With cascading Save() now the wrapped collection is already created when saving the parent.

And as your collection is a HashSet, the GetHashCode() method is called.
Edit: I'm wondering what kind of collection you use in the model. If it was a HashSet, the call to GetHashCode() would already happen when adding the children. Would it help to call Save already after creating the parent and before adding children to the collection?

Behavioral change was necessary to correctly support cascading save and is in line with a change implemented in Hibernate some time ago.

@maintainers: can I help with some organizational stuff here?

[fredericDelaporte]

Actually that is the result of fixing a bug in the cascading mechanism. The cascade was ineffective in some cases. Being ineffective, it did not properly walked collections, included sets, not triggering the call to the GetHashCode of children in a set. That was not a feature, but a bug. So, I cannot find a way to formulate this as a possible breaking change. It seems to me your application was relying on a bug to work: saving a parent with cascading enabled on sets containing transient children not supporting their GetHashCode while transient, but without having the cascade entirely triggered.

So, about fixing your application, I guess it has issues with this bug being fixed because its GetHashCode implementation does not properly supports transient entities. A GetHashCode not supporting transient entity is an application bug, NHibernate does not support this. Supporting transient entity may not be easy though, granted.

The easiest way to support it is to have an immutable natural key available on the entity (a business key), always already adequately valued on transient entities, and using it for your GetHashCode implementation. This is documented here.

What if you do not have a business key available? Then it is a wicked case. There are patterns available to handle it but they remain some "best effort" with limitations. See here or here (same pattern, but different authors).
The main limitation is, a previously transient entity instance which had its hashcode accessed while transient will not have the same hashcode as the same entity but loaded from another session, yielding a new instance. This can cause bugs if you add the later instance to some hashset or dictionary already containing the former: it will re-add it instead of detecting it is already there.

@csharper2010, as I do not consider this qualify as a possible breaking change, there is not much more to do.
Otherwise the fixed issue (#3325 for this case) having introduced it would have to be tagged with the "possible breaking change" tag, with an edit to its description explaining what is the possible breaking change. (I have started doing that then stopped, unable to find a proper way to formulate the breaking change: it then appeared to me that was not really a possible breaking change but a latent application bug not visible thanks to another bug, but on NHibernate side.) Then a PR would have to be done on 5.5.x branch for documenting the missing breaking change in the release note.

[cremor]

My application already implements GetHashCode correctly for transient entities. But the change in 5.5.0 triggers exactly that behavior that you described:

The main limitation is, a previously transient entity instance which had its hashcode accessed while transient will not have the same hashcode as the same entity but loaded from another session, yielding a new instance. This can cause bugs if you add the later instance to some hashset or dictionary already containing the former: it will re-add it instead of detecting it is already there.

[csharper2010]

My application already implements GetHashCode correctly for transient entities. But the change in 5.5.0 triggers exactly that behavior that you described:

If an entity changes its hash code on Flush(), I would argue that the implementation for transient entities is not correct. An instance should never change its HashCode after instantiation.

We rely on object.GetHashCode() as NHibernate correctly maintains reference identity within a session even with proxy instances. This is fine as long as you don't need entities from different sessions having the same hash code. If you work with detached entities or need to be able to have the hash code preserved over multiple sessions, that's a different story... then you really need your custom implementations of GetHashCode() and you can find the necessary information in @fredericDelaporte 's links. Still, with the approaches mentioned there, you can not guarantee a stable HashCode between the entity being transient and later loaded into a different session. The approaches only help with making them stable between loads outside the session which was initially saving the new instance.

[cremor]

My entity implementation does not change the result of the GetHashCode call after it has been called once. That's exactly why this problem is triggered.

Still, with the approaches mentioned there, you can not guarantee a stable HashCode between the entity being transient and later loaded into a different session. The approaches only help with making them stable between loads outside the session which was initially saving the new instance.

Exactly!

[fredericDelaporte]

A workaround, with other drawbacks, could be to add into the entity class a static ConcurrentDictionary indexing by entity id the transient hashcodes, and use that.

I mean, when an id get valued, in its setter, check if a transient hashcode was generated for the entity, and if yes, add it to the static dictionary.
And in the GetHashCode implementation, if not transient, check if a transient hashcode exists for that id in the static dictionary, and yield it if any, otherwise proceed with normal hashcode computation.

Of course the drawback is added complexity, static state with thread safety synchronization. Any better idea?