Fix IsModified so that a null equates empty components when using select-before-update #1487

druidroad · 2017-12-14T04:34:38Z

Test case and proposed solution for (#1486)

…Unable to cast…). Test case and proposed solution for nhibernate#1486

fredericDelaporte · 2017-12-14T11:38:24Z

src/NHibernate/Type/ComponentType.cs

@@ -534,7 +534,7 @@ public override bool IsModified(object old, object current, bool[] checkable, IS
 				return current != null;
 			}
 			object[] currentValues = GetPropertyValues(current, session);
-			object[] oldValues = (Object[]) old;
+			object[] oldValues = old is object[] ? (Object[]) old : GetPropertyValues(old, session);


This is a double cast implementation, which should be avoided. And better apply the fix from Hibernate as pointed by @hazzik. (hibernate/hibernate-orm@550c2e3).

Granted, this will fix another issue by the way: bad handling of empty components. (They should be considered equal to null, which current implementation (with or without your fix) does not handle.) So their own test (hibernate/hibernate-orm@667136c) should be imported, and this PR/issue title and description should be adjusted.

Thank you for the comments. Will do.

I adjusted Hibernate HHH-11237 test case. Hibernate runs the test for CREATE_EMPTY_COMPOSITES_ENABLED true and false that sets createEmptyCompositesEnabled for ComponentType. We don’t have that property/case in NHibernate. I had to change ComponentType.GetPropertyValue(object component, int i) to handle nulls. I applied the same code from Hibernate.

druidroad · 2017-12-15T04:20:18Z

I committed new updated test case and fix.

…shed.

fredericDelaporte

I have added a commit for addressing the points I have reviewed.

fredericDelaporte · 2017-12-15T13:15:09Z

src/NHibernate/Type/ComponentType.cs

+			if (component is Object[])
+			{
+				return ((Object[]) component)[i];
+			}


This object[] type test is a thing from yet another Hibernate change, hibernate/hibernate-orm@ac16473 for HHH-9003 about reducing array allocations. Ideally it should no be taken here. But it mixes with the way they have fixed IsModified too, so it also implies some adjustments on IsModified too.

By the way, when porting Java code, we do some c# specific changes, like respecting c# styling (object rather than Object, namings, ...), exploiting c# features (pattern matching usable here for avoiding double cast), ...
By example this part could be written:

switch (component) { case null: return null; case object[] componentArray: return componentArray[i]; }

Sure. I will respect c# styling.

fredericDelaporte · 2017-12-15T13:23:23Z

src/NHibernate/Type/ComponentType.cs

@@ -269,6 +269,14 @@ public override object NullSafeGet(DbDataReader rs, string name, ISessionImpleme

 		public object GetPropertyValue(object component, int i)
 		{
+			if (component == null)
+			{


Well, so the added test case demonstrates this part was lacking. In fact we are in a somewhat special situation here.
Treating a null component as equivalent to a component having all its properties null was added in NHibernate with NH-1101, which has done most of the changes in the tuplizer, not in the component type. Doing the change in component tuplizer GetPropertyValue was forgotten.

On Hibernate side, it has been done four years later with HHH-7610, with a different implementation, doing the changes mostly in component type instead of doing it in tuplizers.

For consistency with NH-1101, I think we should do the fix here in the tuplizer, instead of doing it in the component type.

Yes, it makes sense to fix in tuplizer.

fredericDelaporte · 2017-12-15T13:32:02Z

src/NHibernate/Type/ComponentType.cs

 			{
 				int len = propertyTypes[i].GetColumnSpan(session.Factory);
 				bool[] subcheckable = new bool[len];
 				Array.Copy(checkable, loc, subcheckable, 0, len);
-				if (propertyTypes[i].IsModified(oldValues[i], currentValues[i], subcheckable, session))
+				if (propertyTypes[i].IsModified(GetPropertyValue(old, i), GetPropertyValue(current, i), subcheckable, session))


This takes a part of HHH-9003 change too. I think we would have better going back to your initial change for this part, due to the conflicting changes mix between NH-1101, HHH-7610, HHH-9003 and HHH-11237 , sorry.

(By the way the HHH-9003 change here does not look very efficient, along with the change of the other method it causes a cast in the loop. Your change is really better: var oldValues = old is object[] objects ? objects : GetPropertyValues(old, session);.)

Yes, it makes sense.

fredericDelaporte · 2017-12-15T13:41:50Z

src/NHibernate.Test/NHSpecificTest/GH1486/Fixture.cs

+					interceptor.Reset();
+					john.Address = null;
+					session.Flush();
+					Assert.AreEqual(0, interceptor.CallCount);


We favor using the fluent assert model, as the model used here is the "old model" and lacks features. See two models.

Sure. I will use "fluent assert model" in my code.

fredericDelaporte · 2017-12-15T13:42:18Z

src/NHibernate.Test/NHSpecificTest/GH1486/Fixture.cs

+					transaction.Commit();
+				}
+			}
+		}


You own test case was testing another currently failing case, I think it is worth keeping it too.

druidroad

These code changes make sense. Thank you for the detail comments and tips how to proceed with NHibernate coding, patterns and issue investigation. Its my 1st contribution to NHibernate and sorry if I took a lot of your time for code review. Is there anything else I should do to complete this PR?

fredericDelaporte · 2017-12-15T20:27:18Z

No troubles, you have been helpful, no reason to be sorry. In fact it is more me who should, because "taking the fix from Hibernate" turned out as not a good idea for this case. Hibernate was indeed fixing your initial report only by accident, and taking its fix had cause fixing quite something else. So in effect we hijacked your issue&PR for fixing something else (fortunately while still fixing your initial report). That is not really a recommended practice.

Is there anything else I should do to complete this PR?

No, I intend to wait for end of tests, then squash & merge.

nhibernate#1486 ComponentType.IsModified(…) method throws exception (…

16121bb

…Unable to cast…). Test case and proposed solution for nhibernate#1486

fredericDelaporte added the t: Fix label Dec 14, 2017

fredericDelaporte reviewed Dec 14, 2017

View reviewed changes

druidroad changed the title ~~#1486 ComponentType.IsModified(…) method throws exception (Unable to cast...)~~ #1486 Fix IsModified so that a null equates empty components when using select-before-update Dec 15, 2017

Adjust and add more tests, code adjustments as per review, to be squa…

9f2d60a

…shed.

fredericDelaporte reviewed Dec 15, 2017

View reviewed changes

fredericDelaporte self-assigned this Dec 15, 2017

druidroad commented Dec 15, 2017

View reviewed changes

fredericDelaporte approved these changes Dec 15, 2017

View reviewed changes

fredericDelaporte merged commit 7e440bf into nhibernate:master Dec 16, 2017

fredericDelaporte added the r: Fixed label Dec 16, 2017

fredericDelaporte added this to the 5.1 milestone Dec 16, 2017

fredericDelaporte added a commit to fredericDelaporte/nhibernate-core that referenced this pull request Dec 16, 2017

Forgotten async generation for nhibernate#1487

cf4ac7a

fredericDelaporte added a commit that referenced this pull request Dec 16, 2017

Forgotten async generation for #1487

8ffa874

hazzik changed the title ~~#1486 Fix IsModified so that a null equates empty components when using select-before-update~~ Fix IsModified so that a null equates empty components when using select-before-update Dec 29, 2017

Fix IsModified so that a null equates empty components when using select-before-update #1487

Fix IsModified so that a null equates empty components when using select-before-update #1487

Uh oh!

Conversation

druidroad commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

druidroad commented Dec 15, 2017

Uh oh!

fredericDelaporte left a comment

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte Dec 15, 2017 • 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 Dec 15, 2017 • 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 Dec 15, 2017 • 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!

druidroad left a comment

Choose a reason for hiding this comment

Uh oh!

fredericDelaporte commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fredericDelaporte Dec 15, 2017 •

edited

Loading

fredericDelaporte Dec 15, 2017 •

edited

Loading

fredericDelaporte Dec 15, 2017 •

edited

Loading

fredericDelaporte commented Dec 15, 2017 •

edited

Loading