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.

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

Conversation

bahusoid
Copy link
Member

Fixed possible equality issue:

public class MyTransformer: DistinctRootEntityResultTransformer {}
var myTransformer = new MyTransformer();
var distinctTransformer = new DistinctRootEntityResultTransformer();
myTransformer.Equals(distinctTransformer); // true. Should be false
distinctTransformer.Equals(myTransformer); // false

Also reuse instances when possible

}
}

public class CustomAliasToEntityMapResultTransformer : AliasToEntityMapResultTransformer { }
Copy link
Member Author

@bahusoid bahusoid Jan 31, 2019

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

@bahusoid bahusoid changed the title Fixed Equals method for transformers WIP Fixed Equals method for transformers Feb 1, 2019
@bahusoid bahusoid force-pushed the transEqual branch 2 times, most recently from 0a364ec to 1af577a Compare February 1, 2019 03:02
@bahusoid bahusoid changed the title WIP Fixed Equals method for transformers Fixed Equals method for transformers Feb 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants