Fix property ref handling #1872

fredericDelaporte · 2018-10-14T18:00:51Z

#378 (NH-3480, v4.1) was a wrong fix, with an heavy impact on the property ref handling, and causing bugs in many-to-many cases.

This PR adds a many-to-many test demonstrating the introduced bugs, reverts most of #378, and fixes its distinctive cause.

#378 case was a one-to-many with property ref failure, requiring having the one side having its identifier mapped as a component. Otherwise, no failure was occurring. This was due to the component equality logic always assuming its operand were components of the right class. Fixing the equality logic is enough to solve the case and have the collection loading work as "good" as other cases not using an entity with a composite key.

Now, the root cause of the bug is the collection loading mechanism ending up resolving the owning entity with the collection key as an id, while it is not the id in the property ref case. Fortunately, resolving the owner there has no usage with default NHibernate implementation. (It may be an issue for collections of custom types.) So having the right owner or an owner at all here does not cause issues indeed.
(Update: additional commits have been added for resolving the owner correctly in any case.)

#378 fix was to cause the collection key to become the id even when a property ref was used. But this was invalid for a many-to-many and inefficient for one-to-many, because it requires additional joins.

Moreover I think the root cause can be fixed by checking if the collection yields the owner id or a unique key, and either resolve the id from the key or perform a lookup by unique key instead of a lookup by id. I will look into this in a separated PR. (Update: done as additional commits.)

About this fix, #378 is indeed a regression introduced in NHibernate v4.1, so ideally, it should be backported to it and to v5.0 and v5.1. But few people (or maybe no-one) have complain about it, so I do not think it deserves being back-ported.

I would still rather have this fixed in 5.2.

Possible breaking change: a collection mapped with a property-ref will no more support being accessed when the referenced property is null. It will throw. Previously, the collection was not throwing but was always loaded empty.

fredericDelaporte · 2018-10-14T18:05:08Z

src/NHibernate.Test/PropertyRef/ManyToManyPropertyRefFixture.cs

+		}
+
+		[Test, Ignore("Not fixed yet")]
+		public void Getting_a_ManyA_object_with_fetchmode_join_will_work()


This test case is originally meant for #1214. This test here is now failing again like initially reported. #1214 cannot be fixed without first fixing the regression caused by #378.
The other test was not failing in the original context of #1214, and is the one demonstrating #378 regression.

fredericDelaporte · 2018-10-14T18:09:38Z

src/NHibernate/Persister/Collection/AbstractCollectionPersister.cs

-			}
-			else
-			{
-				keyType = collection.Owner.Key.Type;


This change was breaking the many-to-many case with property-ref. It was causing NHibernate to try insert in the many-to-many table a column named after the entity id instead of using the referenced property name column name.

fredericDelaporte · 2018-10-14T18:11:17Z

src/NHibernate/Type/CollectionType.cs

@@ -471,16 +471,39 @@ public override bool IsDirty(object old, object current, bool[] checkable, ISess
 		}

 		/// <summary>
-		/// Get the key value from the owning entity instance.
+		/// Get the key value from the owning entity instance. It is usually the identifier, but it might be some
+		/// other unique key, in the case of a property-ref.
 		/// </summary>
 		public object GetKeyOfOwner(object owner, ISessionImplementor session)


Ceasing to return the referenced property value from this method causes bug in the many-to-many case, causing it to insert in the many-to-many table the identifier instead of the referenced property value.

fredericDelaporte · 2018-10-14T18:13:37Z

src/NHibernate/Type/ComponentType.cs

+
+			var componentType = EntityMode == EntityMode.Poco ? ComponentTuplizer.MappedClass : typeof(IDictionary);
+			if (!componentType.IsInstanceOfType(x) || !componentType.IsInstanceOfType(y))
+				return false;


GetPropertyValues calls below can only fail if the parameter is not of the expected type. So an additional check on the type of object to compare together has been added in the IsEqual implementation.

fredericDelaporte · 2018-10-14T18:25:57Z

src/NHibernate/Type/CollectionType.cs

+				: entityEntry.Persister.GetPropertyValue(owner, foreignKeyPropertyName);
+			// NOTE VERY HACKISH WORKAROUND!!
+			var keyType = GetPersister(session).KeyType;
+			if (!keyType.ReturnedClass.IsInstanceOfType(id))


Indeed, hackish and failing if the unique key type is the same than the identifier type: in such case, when the owner is not yet hydrated, it will use the value of its identifier instead of the unique key value. It is not the cause of an invalid cast exception. But it may cause a collection to load data belonging to another entity. I will try to add a test case for this (then a proper fix).

Indeed, hackish and failing if the unique key type is the same than the identifier type:

This does not happen. When not resolved, we have an id instead of an entity, or properties values instead of a component "id" type, but not an id instead of a unique key. But the first case (id instead of an entity) is not supported by the workaround. The workaround works only for the later case (properties values instead of component "id"). So I will do a fix for no more needing this hack.

fredericDelaporte · 2018-10-16T13:12:18Z

Moreover I think the root cause can be fixed by checking if the collection yields the owner id or a unique key, and either resolve the id from the key or perform a lookup by unique key instead of a lookup by id. I will look into this in a separated PR.

I will add this as an additional commit here instead. #378 was dodging the trouble by causing the collection key to hold the owner id instead of the referenced property. So the lookup, which was not handling the unique key case, was no more failing, since it was always the id. Now that it is reverted, we are back to the collection key being actually the mapped referenced property value. So better fix the lookup, even if prior to #378, having it failing was almost never causing issues. (Only in #378 case, it was throwing; in other cases, failing was meaning a null return or even another entity, but apparently without causing much issues.)

I am now checking the VERY HACKISH WORKAROUND which is back with the revert, in case it deserves an additional commit here.

fredericDelaporte · 2018-10-17T14:43:39Z

src/NHibernate/Engine/StatefulPersistenceContext.cs

@@ -782,7 +782,15 @@ public object ProxyFor(object impl)
 		/// <summary> Get the entity that owns this persistent collection</summary>
 		public object GetCollectionOwner(object key, ICollectionPersister collectionPersister)
 		{
-			return GetEntity(session.GenerateEntityKey(key, collectionPersister.OwnerEntityPersister));
+			if (collectionPersister.CollectionType.UseLHSPrimaryKey)
+				return GetEntity(session.GenerateEntityKey(key, collectionPersister.OwnerEntityPersister));


This GetEntity overload is valid only with identifiers. When the key is not the entity identifier, the entity has to be resolved with an EntityUniqueKey.

One code path, in GetLoadedCollectionOwnerOrNull below, was compensating for this by trying to resolve the id from the key, but other code paths were not.

fredericDelaporte · 2018-10-17T14:50:19Z

src/NHibernate/Engine/StatefulPersistenceContext.cs

+				return GetEntity(session.GenerateEntityKey(key, collectionPersister.OwnerEntityPersister));
+
+			return GetEntity(
+				new EntityUniqueKey(


For this to not miss (yield null), CollectionType.LHSPropertyName has:

to be "cache enabled" in the entity persister. This was done only for RHS side (excepted for many-to-many, were it was also missing). I have made the required changes in the mapping compilation for enabling it for LHS (and many-to-many RHS).

to be cached on query loading. So some changes have been made in the Loader too.

fredericDelaporte · 2018-10-17T15:25:24Z

src/NHibernate/Engine/StatefulPersistenceContext.cs

@@ -801,10 +809,9 @@ public virtual object GetLoadedCollectionOwnerOrNull(IPersistentCollection colle
 			object loadedOwner = null;
 			// TODO: an alternative is to check if the owner has changed; if it hasn't then
 			// return collection.getOwner()
-			object entityId = GetLoadedCollectionOwnerIdOrNull(ce);
-			if (entityId != null)
+			if (ce.LoadedKey != null)


Now that GetCollectionOwner(object key, ... correctly handles unique key, this small hack here attempting to resolve the id from the key is no more needed.
Moreover, all other calls to GetCollectionOwner were transmitting the collection key without this hack, so not always the id. (Due to bad naming, `DefaultInitializeCollectionEventListener.InitializeCollectionFromCache could lets think this one was supplying the identifier, but indeed it is supplying the key too.)

fredericDelaporte · 2018-10-17T15:39:08Z

src/NHibernate/Loader/Loader.cs

+						// been cached too for all its unique keys, provided its persister implement it. With this new
+						// way of caching unique keys, it is no more needed to handle caching for alreadyLoaded path
+						// too.
+						(persister as IUniqueKeyLoadable).CacheByUniqueKeys(obj, session);


Here is the change for avoiding cache misses when looking-up by unique key.

Due to #1226, this caching of unique key has a bit evolved.
Prior to #1226, the caching by unique key was done only for entities not yet loaded, when they were loaded through a query with eager loads referencing the entity by its unique key and as their RHS, excepted in the many-to-many case (which was failing and is still failing).
For fixing #1226, this caching has been moved for occurring also with entities already loaded.
Now with this PR, it is moved back to the path of entities not yet loaded, and no more done for this already loaded. This is done because now, instead of caching only by unique key used by RHS side of associations loaded by the query, it is done for all alternate unique keys declared in the entity. So it does no more depends on which associations are eagerly loaded in the query loading the entity, the cache feeding will be done wherever the entity is actually loaded.

This is an overhead for session usages which were not doing any eager load, but I think it is acceptable, because anyway the unique key caching is actually done only in case of property-ref usage in mappings, feature which should be used as little as possible. (Mapped unique keys do not trigger this caching. Only the property target of a property-ref is cached as a unique key.)

fredericDelaporte · 2018-10-17T15:43:15Z

src/NHibernate/Engine/BatchFetchQueue.cs

 		/// <param name="batchSize">the maximum number of keys to return</param>
 		/// <param name="checkCache">Whether to check the cache for uninitialized collection keys.</param>
 		/// <param name="collectionEntries">An array that will be filled with collection entries if set.</param>
 		/// <returns>An array of collection keys, of length <paramref name="batchSize"/> (padded with nulls)</returns>
-		internal object[] GetCollectionBatch(ICollectionPersister collectionPersister, object id, int batchSize, bool checkCache,
+		internal object[] GetCollectionBatch(ICollectionPersister collectionPersister, object key, int batchSize, bool checkCache,


Renaming for avoiding confusion. It may be something else than an identifier, it is a collection key, which can be a unique key property instead. The code calling this is actually supplying a key, not an id.

fredericDelaporte · 2018-10-17T15:49:03Z

src/NHibernate/Cfg/XmlHbmBinding/CollectionBinder.cs

 			{
 				manyToMany.ReferencedPropertyName = manyToManyMapping.propertyref;
+				mappings.AddUniquePropertyReference(manyToMany.ReferencedEntityName, manyToMany.ReferencedPropertyName);


This enable caching of the referenced entity by property-ref done on RHS in the many-to-many case.

fredericDelaporte · 2018-10-17T15:50:29Z

src/NHibernate.Test/NHSpecificTest/Properties/Mappings.hbm.xml

@@ -27,6 +27,8 @@
      </key>
      <one-to-many class="Account"/>
    </set>
+    <!-- Having it mapped after the collection referencing it can cause trouble when resolving the collection, if
+         not accounted for. So keep it mapped after for checking this case is still supported. -->


This pre-existing test is already checking the case of a collection having a property-ref on a "component like" (here a properties mapping) property mapped after the collection.

fredericDelaporte · 2018-10-17T15:51:35Z

src/NHibernate.Test/PropertyRef/KeyEntityPropertyRef.hbm.xml

+      <one-to-many class="B"/>
+    </set>
+    <!-- Having it mapped after the collection referencing it was causing trouble when resolving the collection -->
+    <many-to-one name="C"/>


A test for this case was lacking. And it was broken, with or without this PR first commits. Mapping the referenced many-to-one before the collection was working.

fredericDelaporte · 2018-10-17T15:53:11Z

src/NHibernate.Test/PropertyRef/KeyEntityPropertyRefFixture.cs

+						.Single(a => a.Id != _aWithItemsId);
+
+				Assert.That(NHibernateUtil.IsInitialized(entity.CItems), Is.False, "a without items");
+				Assert.That(entity.CItems, Has.Count.EqualTo(0), "a without items");


An additional failure was occurring in case of empty collection, so testing for this case too. The pre-existing test for the component case was already testing both case, empty and non-empty collections.

fredericDelaporte · 2018-10-17T15:55:33Z

src/NHibernate/Engine/TwoPhaseLoad.cs

+					if (types[i].IsCollectionType)
+					{
+						// Resolve them last, because they may depend on other properties if they use a property-ref
+						collectionToResolveIndexes.Add(i);


The change in CollectionType.GetKeyOfOwner renders this change here no more mandatory. But I think it is better to remove a code path relying on CollectionType.GetKeyOfOwner "best effort", which the change here does.

hazzik · 2018-10-21T23:19:44Z

src/NHibernate/Type/CollectionType.cs

+			// This should not happen. If that changes, at least yield null, instead of yielding
+			// a value of an unexpected type.
+			throw new AssertionFailure(
+				$"Unable to correctly resolve the owner key, property {foreignKeyPropertyName} for" +


Need a space at the end

src/NHibernate/Engine/TwoPhaseLoad.cs

hazzik · 2018-10-22T21:04:56Z

src/NHibernate/Loader/Loader.cs

+						// been cached too for all its unique keys, provided its persister implement it. With this new
+						// way of caching unique keys, it is no more needed to handle caching for alreadyLoaded path
+						// too.
+						(persister as IUniqueKeyLoadable).CacheByUniqueKeys(obj, session);


Did you mean (persister as IUniqueKeyLoadable)?.CacheByUniqueKeys(obj, session);?

Or ((IUniqueKeyLoadable) persister).CacheByUniqueKeys(obj, session);. I have to check if it must always be castable or not.

I think your version is the safer one. Maybe there is some loading cases, when there are no associations loaded or none with a unique key, where a custom persister could be something not implementing this interface, although all NHibernate ILoadable current implementations do also implement it.

fredericDelaporte · 2018-10-23T10:48:00Z

Rebased, then three new commits added for addressing the review, each with its own async regen.

For merging, I intend to squash fixups only, then do a merge commit.

The NH-3453 part is not reverted. The NH-3480 part was wrecking the many-to-many mapping with property ref.

Minimal fix for NH-3480

Root cause of NH-3480

Reverting initial fix of NH-3480 is re-introducing this related failure, which was lacking adequate tests

fredericDelaporte added t: Bug c: Core p: Minor Regression labels Oct 14, 2018

fredericDelaporte added this to the 5.2 milestone Oct 14, 2018

fredericDelaporte commented Oct 14, 2018

View reviewed changes

fredericDelaporte force-pushed the GH1214 branch from 13bad0b to 5596176 Compare October 15, 2018 12:55

fredericDelaporte mentioned this pull request Oct 15, 2018

Fix many-to-many with property-ref #1873

Merged

fredericDelaporte changed the title ~~Fix property ref handling~~ WIP - Fix property ref handling Oct 16, 2018

fredericDelaporte force-pushed the GH1214 branch 2 times, most recently from 18b222a to b76108d Compare October 17, 2018 15:41

fredericDelaporte commented Oct 17, 2018

View reviewed changes

fredericDelaporte changed the title ~~WIP - Fix property ref handling~~ Fix property ref handling Oct 17, 2018

hazzik reviewed Oct 21, 2018

View reviewed changes

hazzik reviewed Oct 22, 2018

View reviewed changes

src/NHibernate/Engine/TwoPhaseLoad.cs Outdated Show resolved Hide resolved

hazzik reviewed Oct 22, 2018

View reviewed changes

hazzik previously approved these changes Oct 22, 2018

View reviewed changes

fredericDelaporte dismissed stale reviews from hazzik via 76de6fb October 23, 2018 09:53

fredericDelaporte force-pushed the GH1214 branch from 76de6fb to ca3d2ab Compare October 23, 2018 10:45

fredericDelaporte added 5 commits October 24, 2018 15:09

Add test case for many-to-many with property ref

9ed4314

Revert nhibernate#378 - NH-3480

5456d85

The NH-3453 part is not reverted. The NH-3480 part was wrecking the many-to-many mapping with property ref.

Fix component equality failure

0245389

Minimal fix for NH-3480

Resolve collection owner correctly in case of property-ref

1102acf

Root cause of NH-3480

Resolve owner's key correctly in case of property-ref

8124cb5

Reverting initial fix of NH-3480 is re-introducing this related failure, which was lacking adequate tests

fredericDelaporte force-pushed the GH1214 branch from ca3d2ab to 8124cb5 Compare October 24, 2018 13:09

fredericDelaporte merged commit 932ffff into nhibernate:master Oct 24, 2018

fredericDelaporte deleted the GH1214 branch October 24, 2018 13:12

fredericDelaporte added the r: Fixed label Oct 24, 2018

fredericDelaporte mentioned this pull request Oct 24, 2018

NH-2180 - Many-To-Many with Property-ref fails to get subitems with FetchMode Join #1214

Closed

fredericDelaporte mentioned this pull request Dec 3, 2018

Property-ref on many-to-one with composite id fails #1918

Merged

fredericDelaporte mentioned this pull request Jan 20, 2019

Unable to correctly resolve the owner key with null value property #1983

Closed

fredericDelaporte mentioned this pull request Feb 17, 2019

Fix #1994 (many-to-many filter in hql) #1995

Merged

fredericDelaporte added the possible breaking change label Feb 24, 2019

Fix property ref handling #1872

Fix property ref handling #1872

Uh oh!

Conversation

fredericDelaporte commented Oct 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fredericDelaporte Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Oct 23, 2018

Uh oh!

Uh oh!

fredericDelaporte commented Oct 14, 2018 •

edited

Loading

fredericDelaporte commented Oct 16, 2018 •

edited

Loading

fredericDelaporte Oct 17, 2018 •

edited

Loading

fredericDelaporte Oct 17, 2018 •

edited

Loading