Skip to content

Fixed Equals method for transformers #2000

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 3, 2019
Merged

Fixed Equals method for transformers #2000

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0ee23a2
Fixed Equals method for transformers
bahusoid Jan 31, 2019
8067ede
fixed tests
bahusoid Feb 1, 2019
1af577a
Undo change for linq
bahusoid Feb 1, 2019
7e53d86
Merge branch 'master' into transEqual
bahusoid Feb 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 43 additions & 47 deletions src/NHibernate.Test/NHSpecificTest/NH3957/ResultTransformerEqualityFixture.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
using System.Collections.Generic;
using System.Reflection;
using NHibernate.Transform;
using NUnit.Framework;

Expand All @@ -8,53 +7,20 @@ namespace NHibernate.Test.NHSpecificTest.NH3957
[TestFixture]
public class ResultTransformerEqualityFixture
{
/// <summary>
/// Allows to simulate a hashcode collision. Issue would be unpractical to test otherwise.
/// Hashcode collision must be supported for avoiding unexpected and hard to reproduce failures.
/// </summary>
private void TweakHashcode(System.Type transformerToTweak, object hasher)
{
var hasherTargetField = transformerToTweak.GetField("Hasher", BindingFlags.Static | BindingFlags.NonPublic);
if (!_hasherBackup.ContainsKey(transformerToTweak))
_hasherBackup.Add(transformerToTweak, hasherTargetField.GetValue(null));

// Though hasher is a readonly field, this works at the time of this writing. If it starts breaking and cannot be fixed,
// ignore those tests or throw them away.
hasherTargetField.SetValue(null, hasher);
}

private Dictionary<System.Type, object> _hasherBackup = new Dictionary<System.Type, object>();

[SetUp]
public void Setup()
{
var hasherForAll = typeof(AliasToEntityMapResultTransformer)
.GetField("Hasher", BindingFlags.Static | BindingFlags.NonPublic)
.GetValue(null);
TweakHashcode(typeof(DistinctRootEntityResultTransformer), hasherForAll);
TweakHashcode(typeof(PassThroughResultTransformer), hasherForAll);
TweakHashcode(typeof(RootEntityResultTransformer), hasherForAll);
TweakHashcode(typeof(ToListResultTransformer), hasherForAll);
}

[TearDown]
public void TearDown()
{
// Restore those types hashcode. (Avoid impacting perf of other tests, avoid second level query cache
// issues if it was holding cached entries (but would mean some tests have not cleaned up properly).)
foreach(var backup in _hasherBackup)
{
TweakHashcode(backup.Key, backup.Value);
}
}

public class CustomAliasToEntityMapResultTransformer : AliasToEntityMapResultTransformer { }
Copy link
Member Author

@bahusoid bahusoid Jan 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This custom class also provides hashcode collision with current implementaion. Not sure why TweakHashcode was required

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #564, it is required to check that two different result transformers will not be considered equal if they happen to have the same hashcode, which was NH-3957 bug. The collision for different instances of the same transformer is intentional and mandatory since they are considered equal.

Removing the tweak causes the Inequality suffixed tests to no more test the NH-3957 bug. You should change them to compare with the corresponding Custom descendant you have added. That is still a step away from what was NH-3957, but I do not see any other solution with the Instance optimization. NH-3957 was a random hashcode collision between different unrelated transformers causing them to wrongly consider themselves equals.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is required to check that two different result transformers will not be considered equal if they happen to have the same hashcode,.. Removing the tweak causes the Inequality suffixed tests to no more test the NH-3957 bug

But I'm already checking Inequality:
Assert.False(custom.Equals(transf1));
Assert.False(transf1.Equals(custom));

I've just put those checks in the wrong tests.

That is still a step away from what was NH-3957

I still see no difference between inequality check for custom implemented classes with collisions to existing ones ( custom and transf1) and TweakHashcode hack for existing classes. IMHO they still fully cover the case:

that two different result transformers will not be considered equal if they happen to have the same hashcode

public class CustomDistinctRootEntityResultTransformer : DistinctRootEntityResultTransformer { }
public class CustomPassThroughResultTransformer : PassThroughResultTransformer { }
public class CustomRootEntityResultTransformer : RootEntityResultTransformer { }

// Non reg test case
[Test]
public void AliasToEntityMapEquality()
{
var transf1 = new AliasToEntityMapResultTransformer();
var transf2 = new AliasToEntityMapResultTransformer();
HashSet<IResultTransformer> set = new HashSet<IResultTransformer>() { transf1, transf2, };

Assert.That(set.Count, Is.EqualTo(1));
Assert.IsTrue(transf1.Equals(transf2));
Assert.IsTrue(transf2.Equals(transf1));
}
Expand All @@ -63,8 +29,11 @@ public void AliasToEntityMapEquality()
public void AliasToEntityMapAndDistinctRootEntityInequality()
{
var transf1 = new AliasToEntityMapResultTransformer();
var transf2 = new DistinctRootEntityResultTransformer();
var transf2 = new CustomAliasToEntityMapResultTransformer();
HashSet<IResultTransformer> set = new HashSet<IResultTransformer>() { transf1, transf2, };

Assert.That(transf1.GetHashCode(), Is.EqualTo(transf2.GetHashCode()), "prerequisite");
Assert.That(set.Count, Is.EqualTo(2));
Assert.IsFalse(transf1.Equals(transf2));
Assert.IsFalse(transf2.Equals(transf1));
}
Expand All @@ -75,18 +44,35 @@ public void DistinctRootEntityEquality()
{
var transf1 = new DistinctRootEntityResultTransformer();
var transf2 = new DistinctRootEntityResultTransformer();
HashSet<IResultTransformer> set = new HashSet<IResultTransformer>() { transf1, transf2, };

Assert.That(set.Count, Is.EqualTo(1));
Assert.IsTrue(transf1.Equals(transf2));
Assert.IsTrue(transf2.Equals(transf1));
}

[Test]
public void DistinctRootEntityEqualityInequality()
{
var transf1 = new DistinctRootEntityResultTransformer();
var transf2 = new CustomDistinctRootEntityResultTransformer();
HashSet<IResultTransformer> set = new HashSet<IResultTransformer>() { transf1, transf2, };

Assert.That(transf1.GetHashCode(), Is.EqualTo(transf2.GetHashCode()), "prerequisite");
Assert.That(set.Count, Is.EqualTo(2));
Assert.IsFalse(transf1.Equals(transf2));
Assert.IsFalse(transf2.Equals(transf1));
}

// Non reg test case
[Test]
public void PassThroughEquality()
{
var transf1 = new PassThroughResultTransformer();
var transf2 = new PassThroughResultTransformer();
HashSet<IResultTransformer> set = new HashSet<IResultTransformer>() { transf1, transf2, };

Assert.That(set.Count, Is.EqualTo(1));
Assert.IsTrue(transf1.Equals(transf2));
Assert.IsTrue(transf2.Equals(transf1));
}
Expand All @@ -95,8 +81,11 @@ public void PassThroughEquality()
public void PassThroughAndRootEntityInequality()
{
var transf1 = new PassThroughResultTransformer();
var transf2 = new RootEntityResultTransformer();

var transf2 = new CustomPassThroughResultTransformer();
HashSet<IResultTransformer> set = new HashSet<IResultTransformer>() { transf1, transf2, };

Assert.That(transf1.GetHashCode(), Is.EqualTo(transf2.GetHashCode()), "prerequisite");
Assert.That(set.Count, Is.EqualTo(2));
Assert.IsFalse(transf1.Equals(transf2));
Assert.IsFalse(transf2.Equals(transf1));
}
Expand All @@ -107,7 +96,9 @@ public void RootEntityEquality()
{
var transf1 = new RootEntityResultTransformer();
var transf2 = new RootEntityResultTransformer();
HashSet<IResultTransformer> set = new HashSet<IResultTransformer>() { transf1, transf2, };

Assert.That(set.Count, Is.EqualTo(1));
Assert.IsTrue(transf1.Equals(transf2));
Assert.IsTrue(transf2.Equals(transf1));
}
Expand All @@ -116,8 +107,11 @@ public void RootEntityEquality()
public void RootEntityAndToListInequality()
{
var transf1 = new RootEntityResultTransformer();
var transf2 = new ToListResultTransformer();

var transf2 = new CustomRootEntityResultTransformer();
HashSet<IResultTransformer> set = new HashSet<IResultTransformer>() { transf1, transf2, };

Assert.That(transf1.GetHashCode(), Is.EqualTo(transf2.GetHashCode()), "prerequisite");
Assert.That(set.Count, Is.EqualTo(2));
Assert.IsFalse(transf1.Equals(transf2));
Assert.IsFalse(transf2.Equals(transf1));
}
Expand All @@ -128,9 +122,11 @@ public void ToListEquality()
{
var transf1 = new ToListResultTransformer();
var transf2 = new ToListResultTransformer();
HashSet<IResultTransformer> set = new HashSet<IResultTransformer>() { transf1, transf2 };

Assert.That(set.Count, Is.EqualTo(1));
Assert.IsTrue(transf1.Equals(transf2));
Assert.IsTrue(transf2.Equals(transf1));
}
}
}
}
8 changes: 4 additions & 4 deletions src/NHibernate/Criterion/CriteriaSpecification.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,10 @@ public static class CriteriaSpecification

static CriteriaSpecification()
{
AliasToEntityMap = new AliasToEntityMapResultTransformer();
RootEntity = new RootEntityResultTransformer();
DistinctRootEntity = new DistinctRootEntityResultTransformer();
Projection = new PassThroughResultTransformer();
AliasToEntityMap = AliasToEntityMapResultTransformer.Instance;
RootEntity = RootEntityResultTransformer.Instance;
DistinctRootEntity = DistinctRootEntityResultTransformer.Instance;
Projection = PassThroughResultTransformer.Instance;
InnerJoin = JoinType.InnerJoin;
FullJoin = JoinType.FullJoin;
LeftJoin = JoinType.LeftOuterJoin;
Expand Down
16 changes: 7 additions & 9 deletions src/NHibernate/Transform/AliasToEntityMapResultTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ namespace NHibernate.Transform
[Serializable]
public class AliasToEntityMapResultTransformer : AliasedTupleSubsetResultTransformer
{
private static readonly object Hasher = new object();
internal static readonly AliasToEntityMapResultTransformer Instance = new AliasToEntityMapResultTransformer();

public override object TransformTuple(object[] tuple, string[] aliases)
{
Expand Down Expand Up @@ -37,18 +37,16 @@ public override bool IsTransformedValueATupleElement(string[] aliases, int tuple

public override bool Equals(object obj)
{
if (obj == null || obj.GetHashCode() != Hasher.GetHashCode())
{
if (ReferenceEquals(obj, this))
return true;
if (obj == null)
return false;
}
// NH-3957: do not rely on hashcode alone.
// Must be the exact same type
return obj.GetType() == typeof(AliasToEntityMapResultTransformer);
return obj.GetType() == GetType();
}

public override int GetHashCode()
{
return Hasher.GetHashCode();
return System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(Instance);
}
}
}
}
4 changes: 2 additions & 2 deletions src/NHibernate/Transform/CacheableResultTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public class CacheableResultTransformer : IResultTransformer
// is private (as it should be for a singleton)
//private const PassThroughResultTransformer ACTUAL_TRANSFORMER =
// PassThroughResultTransformer.INSTANCE;
private readonly PassThroughResultTransformer _actualTransformer = new PassThroughResultTransformer();
private readonly PassThroughResultTransformer _actualTransformer = PassThroughResultTransformer.Instance;

public bool AutoDiscoverTypes { get; }

Expand Down Expand Up @@ -358,7 +358,7 @@ public override bool Equals(object o)
if (this == o)
return true;

if (o == null || typeof (CacheableResultTransformer) != o.GetType())
if (o == null || GetType() != o.GetType())
return false;

var that = (CacheableResultTransformer) o;
Expand Down
23 changes: 8 additions & 15 deletions src/NHibernate/Transform/DistinctRootEntityResultTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ namespace NHibernate.Transform
public class DistinctRootEntityResultTransformer : IResultTransformer, ITupleSubsetResultTransformer
{
private static readonly INHibernateLogger log = NHibernateLogger.For(typeof(DistinctRootEntityResultTransformer));
private static readonly object Hasher = new object();
internal static readonly DistinctRootEntityResultTransformer Instance = new DistinctRootEntityResultTransformer();

internal sealed class Identity
{
Expand Down Expand Up @@ -62,33 +62,26 @@ public IList TransformList(IList list)

public bool[] IncludeInTransform(String[] aliases, int tupleLength)
{
//return RootEntityResultTransformer.INSTANCE.includeInTransform(aliases, tupleLength);
var transformer = new RootEntityResultTransformer();
return transformer.IncludeInTransform(aliases, tupleLength);
return RootEntityResultTransformer.Instance.IncludeInTransform(aliases, tupleLength);
}


public bool IsTransformedValueATupleElement(String[] aliases, int tupleLength)
{
//return RootEntityResultTransformer.INSTANCE.isTransformedValueATupleElement(null, tupleLength);
var transformer = new RootEntityResultTransformer();
return transformer.IsTransformedValueATupleElement(null, tupleLength);
return RootEntityResultTransformer.Instance.IsTransformedValueATupleElement(null, tupleLength);
}

public override bool Equals(object obj)
{
if (obj == null || obj.GetHashCode() != Hasher.GetHashCode())
{
if (ReferenceEquals(obj, this))
return true;
if (obj == null)
return false;
}
// NH-3957: do not rely on hashcode alone.
// Must be the exact same type
return obj.GetType() == typeof(DistinctRootEntityResultTransformer);
return obj.GetType() == GetType();
}

public override int GetHashCode()
{
return Hasher.GetHashCode();
return RuntimeHelpers.GetHashCode(Instance);
}
}
}
17 changes: 7 additions & 10 deletions src/NHibernate/Transform/PassThroughResultTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace NHibernate.Transform
[Serializable]
public class PassThroughResultTransformer : IResultTransformer, ITupleSubsetResultTransformer
{
private static readonly object Hasher = new object();
internal static readonly PassThroughResultTransformer Instance = new PassThroughResultTransformer();

#region IResultTransformer Members

Expand Down Expand Up @@ -58,21 +58,18 @@ internal object[] UntransformToTuple(object transformed, bool isSingleResult)
return isSingleResult ? new[] {transformed} : (object[]) transformed;
}


public override bool Equals(object obj)
{
if (obj == null || obj.GetHashCode() != Hasher.GetHashCode())
{
if (ReferenceEquals(obj, this))
return true;
if (obj == null)
return false;
}
// NH-3957: do not rely on hashcode alone.
// Must be the exact same type
return obj.GetType() == typeof(PassThroughResultTransformer);
return obj.GetType() == GetType();
}

public override int GetHashCode()
{
return Hasher.GetHashCode();
return System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(Instance);
}
}
}
}
18 changes: 7 additions & 11 deletions src/NHibernate/Transform/RootEntityResultTransformer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace NHibernate.Transform
[Serializable]
public class RootEntityResultTransformer : IResultTransformer, ITupleSubsetResultTransformer
{
private static readonly object Hasher = new object();
internal static readonly RootEntityResultTransformer Instance = new RootEntityResultTransformer();

public object TransformTuple(object[] tuple, string[] aliases)
{
Expand All @@ -19,13 +19,11 @@ public IList TransformList(IList collection)
return collection;
}


public bool IsTransformedValueATupleElement(String[] aliases, int tupleLength)
{
return true;
}


public bool[] IncludeInTransform(String[] aliases, int tupleLength)
{
bool[] includeInTransform;
Expand All @@ -43,18 +41,16 @@ public bool[] IncludeInTransform(String[] aliases, int tupleLength)

public override bool Equals(object obj)
{
if (obj == null || obj.GetHashCode() != Hasher.GetHashCode())
{
if (ReferenceEquals(obj, this))
return true;
if (obj == null)
return false;
}
// NH-3957: do not rely on hashcode alone.
// Must be the exact same type
return obj.GetType() == typeof(RootEntityResultTransformer);
return obj.GetType() == GetType();
}

public override int GetHashCode()
{
return Hasher.GetHashCode();
return System.Runtime.CompilerServices.RuntimeHelpers.GetHashCode(Instance);
}
}
}
}
Loading