-
Notifications
You must be signed in to change notification settings - Fork 933
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.
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
Conversation
} | ||
} | ||
|
||
public class CustomAliasToEntityMapResultTransformer : AliasToEntityMapResultTransformer { } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
0a364ec
to
1af577a
Compare
Fixed possible equality issue:
Also reuse instances when possible