Fix ManyToOneType.IsModified to handle both object instance and identifier passed to the parameter “old”.

[druidroad]

PROBLEM:

ManyToOneType.IsModified won't resolve identifier if object instance (not the identifier) was passed to the “old” parameter.
Similar issue https://hibernate.atlassian.net/browse/HHH-3730
was reported in Hibernate with pull request hibernate/hibernate-orm#728

DESCRIPTION:

The current code of IsModified method accepts 4 parameters:

public override bool IsModified(object old, object current, bool[] checkable, ISessionImplementor session)

It assumes that “old” is always Identifier and “current” is the object instance, so it tries to resolve Identifier for the “current” only:

return “GetIdentifierOrUniqueKeyType(session.Factory).IsDirty(old, GetIdentifier(current, session), session)”;

SCENARIO 1: SelectBeforeUpdate

Typically, the method ManyToOneType.IsModified(…) is fired when flushing a session with an entity which contains a "many-to-one" property, but only when the "select-before-update" option is turned on for that class (and performed). For this scenario the identifier is always passed to “old” and it works fine.

SCENARIO 2: Event Listeners
It became common practice to do auditing with NHibernate Event Listeners and call Persister.FindModified() method:

public class AuditEventListener : IPostUpdateEventListener
{
public void OnPostUpdate(PostUpdateEvent @event)
	{
		var modifiedItems = @event.Persister.FindModified(@event.OldState, @event.State, @event.Entity, @event.Session);
		// Process modified items.	
	}
}

For this scenario, @event.OldState and @event.State hold the object instances. It ends up going through calls: AbstractEntityPersister.FindModified(..) -> TypeHelper.FindModified(..) -> ManyToOneType. IsModified(..)
Eventually the object instances make into “old” parameter in ManyToOneType.IsModified(..).
Once it happens, we have two problems:

1. ManyToOneType.IsModified(..) returns always true comparing two ManyToOneType entities with regular (not composite) Ids.
2. ManyToOneType.IsModified(..) throws InvalidCastException exception comparing two ManyToOneType entities with composite Ids

SOLUTION:

We have to handle 4 possible scenarios for “old”

1. Id
2. Composite Id
3. Object Instance with Id
4. Object Instance with Composite Id

A. Leaving “old” as it is fails for (3) and (4)
B. Resolving GetIdentifier(old, session) fails for (2). It was offered in hibernate/hibernate-orm#728 and the code was pulled back because of composite id. In NHibernate NHibernate.Test.TypedManyToOne. TypedManyToOneTest will fail if we will do it this way.

C. I propose to check if “old” is Identifier before calling GetIdentifier(old, session):

---

OLD CODE (ManyToOneType)

---

*****************************************************************
public class ManyToOneType : EntityType
{
...
public override bool IsModified(object old, object current, bool[] checkable, ISessionImplementor session)
{
...
return GetIdentifierOrUniqueKeyType(session.Factory).IsDirty(old, GetIdentifier(current, session), session);
}
}

---

NEW FIXED CODE (ManyToOneType)

---

public class ManyToOneType : EntityType
{
...
public override bool IsModified(object old, object current, bool[] checkable, ISessionImplementor session)
{
...
var oldIdentifier = IsIdentifier(old, session) ? old : GetIdentifier(old, session);
	var currentIdentifier = GetIdentifier(current, session);
	return GetIdentifierOrUniqueKeyType(session.Factory).IsDirty(oldIdentifier, currentIdentifier, session);
}

private bool IsIdentifier(object value, ISessionImplementor session)
{
	var identifierType = GetIdentifierType(session);
	if (identifierType == null)
	{
		return false;
	}
	return value.GetType() == identifierType.ReturnedClass;
}
}	

---

[bahusoid]

It became common practice to do auditing with NHibernate Event Listeners and call Persister.FindModified() method:

It's some hacky fix I would say. Shouldn't Persister.FindDirty should be called instead in Event Listeners which handles instances? It would be better just to document behavior of FindModified and keep it be...