ValueExpressionImpl#equals is likely not enough.
It's possible that there is the same hashCode for different objects. For some Java Classes we alwasy get 0 even.
For better tracking: this came up during a bug report against Apache OpenWebBeans as reported by Holger Sunke.
Given the current implementation, the odds of a hash collision look to be very small. The probably explains why the code has been like this since 6.0.x without any bugs reported. That said, the Javadoc for #hashCode() and equals() is clear that non-equal objects may have equal hash codes so checking equals in the event of equal hash codes looks like the safe thing to do.
I'm assuming the #hashCode() was used for performance reasons.
It's a little late, but I have a few comments about the patch.
1. There is no reason to check the hashcode if you are going to call .equals. Presumably, Node.equals() knows the best way to compare most efficiently.
2. I don't know enough about ValueExperssionImpl (etc.) to know whether this is true, but often using "instanceof" in an equals(Object) method is inappropriate. Is there any reason to use a concrete class here rather than an interface? Is there any reason two objects might have different runtime classes, yet still be considered "equals"?
> It's a little late, but I have a few comments about the patch.
My assumption was that hashCode() was used originally because it was faster. Therefore the patch uses hashCode() and only falls back to equals if the hash codes are the same. Whether that makes sense depends on the relative speed of hashCode() vs equals() and how often the tested objects are equal or not. I opted not to dig in to that and go with the patch as written. I've no objection to dropping the hashCode() if that is what folks prefer.
> classes, yet still be considered "equals"?
Not that I can think of. There would only ever be a single EL implementation in use.
Thanks for the feedback and applying. Have been sick over the weekend, so sorry for the delay.
Yes, you are both right. I first intended to ship a minimal impact patch.
But of course hashCode() will likely again be called in Node#equals(). That would make it being called twice - which makes no sense.
> would make it being called twice - which makes no sense.
I've rarely seen an equals() method use hashCode for anything.
It's not used in SimpleNode#equals, for instance.
> It's not used in SimpleNode#equals, for instance.
Oki, in that case please leave the hashCode() check!
Reason: the hashCode is internally cached as far as I've seen. So if you can quickly find NON matching nodes you are much faster. Expecially since the non-equals case is obviously much more often the case than equals.