Details
-
Type:
Bug
-
Status:
Closed
-
Priority:
Major
-
Resolution: Fixed
-
Affects Version/s: 2.4-RC0, 2.5-M0
-
Component/s: geometry, referencing
-
Labels:None
Description
A few months ago, we defined explicitly hashCode() and equals(Object) contracts in the DirectPosition interface:
The DirectPosition2D class was already existing prior the above addition. It is defined as below, in an attempt to act as a bridge between Java2D and GeoAPI:
public class DirectPosition2D extends Point2D implements DirectPosition
It was inheriting the Point2D.hashCode() implementation. But now that DirectPosition.hashCode() is explicitly defined, we have a clash between Point2D.hashCode() implementation and DirectPosition.hashCode() contract. There is no way to satisfy both in same time. Changing the DirectPosition.hashCode() contract is not a good option since it would make it more complicated, and even if we solved the Point2D case there is many other cases that could occur (JTS position, Java3D points, etc. - we can not satisfy them all).
This clash is a problem because Point2D.equals(Object) is defined as below:
if (other instanceof Point2D) { // do the comparaison }
The problem would not occurs if it has been defined as below instead (i.e. if we require the exact same class):
if (other!=null && other.getClass() == getClass()) { // do the comparaison }
Reason: if the object to compare are of the exact same class, we know that their hashCode() implementations are identical. If they are not equals, we don't have this garantee. "this" could use the Point2D.hashCode() implementation and "other" could use the DirectPosition.hashCode() contract, thuse violating the condition that this.equals(other) == true implies this.hashCode() == other.hashCode().
Conclusion: I suggest that in GeoTools implementation, most equals(Object) implementations require the exact same class. I know that some other GeoTools developpers asked for the opposite, but I believe that the opposite can be a cause of subtle bugs. The one described in this email could have been unoticed for long if we didn't had "assert" statements in {{DirectPosition2D}. We can replace some "same class" requirements by "instance of" on a case-by-case basis rather than as a general rule.
As far as the Point2D / DirectPosition clash is concerned, I had no other choice than writting a javadoc warning about it since we can't change Point2D implementation.
Issue Links
- is related to
-
GEO-119
Define equals and hashcode for DirectPosition
-