Tuesday, July 10, 2007

Liskov's Substitution Principle, equals, and Hibernate Proxies

Hibernate's CGLIB dynamic proxy classes reared their ugly head today. I've been using the Eclipse-generated hashcode() and equals() methods in my domain objects for quite a while, with no problems. Then today I write a test that does a simple equality check and everything blows up!


The problem seems to be due to the way my equals() methods were written, and how Hibernate's default dynamic proxy strategy interacts with that. I was doing this:



if (getClass() != obj.getClass())

return false;
However, in my exploding test, one of my objects being compared was a regular old domain object, while the other was a proxied object. Since the proxied object that Hibernate creates is actually technically a subclass of my class (with additional Hibernate-specific methods and such), my getClass()-based equality test was choking badly; after all, com.foo.MyClass is most definitely not com.foo.MyClass$$EnhancerByCGLIB$$beb95050.

It turns out that this is due to something called the Liskov Substitution Principle, which basically formalizes the intuition behind the inheritance portion of the object-oriented programming model; if S is a subtype of T, then you can use an instance of S wherever an instance of T is called for and nothing breaks. The corollary would be that if something does break, then some of your assumptions might need re-examining.

In this case, specifying the equals() method in terms of getClass() is too restrictive; Hibernate proxies should be able to be used anywhere one of my domain objects is used (that's the whole point!). A way to solve this problem is offered by Josh Bloch in his Effective Java book: use an instanceof-based test instead:



if (!(obj instanceof MyClass))

return false;
Here, the proxied object, as a subclass of MyClass, is also an instanceof MyClass. Since the CGLIB proxy doesn't override equals(), the proxy inherits the same implementation of equals() as the base class, thus maintaining the symmetric property any valid equals() implementation must have. If the subclass did override equals(), then things would be different, but then you'd have a violation of the Liskov Substitution Principle. Also, as Bloch states in Effective Java, page 30:


It turns out that this is a fundamental problem of equivalence relations in object-oriented languages. There is simply no way to extend an instantiable class and add an aspect while preserving the equals contract. (emphasis in original)
If you're paranoid, you can declare your implementation of equals() to be final, so you can be sure that it is never overridden. Since the CGLIB proxy doesn't try to override it, you're safe.

The "downside" of this approach, if it can be called that, is that each "terminal" domain object needs its own implementation of equals() (note that it's (obj instanceof MyClass), not (obj instanceof getClass())); in other words, you can't define a general equals() in a superclass and let it do all the heavy lifting for inheriting classes. However, in the grand scheme of things, I don't really see that as much of a downside. Yeah, it's a bit of a pain to write equals() methods, but it has to be done anyway (it's your job as a designer), and it is arguably a more accurate approach to take. As a designer, you need to be aware of the implication of what you code. If the getClass() method works for you, fine; just be aware of what that implies. Ditto for the instanceof method. I'm convinced that in my particular case, the instanceof approach is the semantically correct one.


Update: I just checked my copy of Java Persistence with Hibernate and, sure enough, they use instanceof in their equals() implementations. Clearly, the interaction with the proxies is a driving reason to use this formulation. Apart from that, though, I still think that using instanceof is more semantically correct.

Helpful Links


3 comments:

Tim Azzopardi said...

When MyEclipse generates equals and hashcode it also generates

if (!name.equals(other.name))
return false;

if you are using hibernate this needs to be
if (!name.equals(other.getName()))
return false;

so that hibernate gets a chance to do its lazy loading if necessary.

John said...

I ran into this issue, and I certainly didn't want to override equals for thousands of domain objects. So I came up with this solution.. er workaround I guess

@MappedSuperclass
public abstract class AbstractEntity<PK extends Serializable> implements
Entity<PK> {



@Override
public boolean equals(final Object that) {
if (that == null
|| !(that instanceof AbstractEntity)
|| removeCgLibProxy(this.getClass()) != removeCgLibProxy(that
.getClass())) {
return false;
}
return this == that || this.idEquals((AbstractEntity<?>) that);
}


public boolean idEquals(AbstractEntity<? extends Serializable> that) {
if (this == null || that == null) {
return false;
}
PK thisId = this.getId();
Serializable thatId = that.getId();
return thisId != null && thatId != null && thisId.equals(thatId);
}


private Class<?> removeCgLibProxy(Class<?> proxiedClass) {
String name = proxiedClass.getName();
int i = name.indexOf("$$EnhancerByCGLIB$$");
if (i == -1) {
return proxiedClass;
}
try {
return Class.forName(name.substring(0, i));
} catch (ClassNotFoundException e) {
return proxiedClass;
}
}
}
}

Manuel Dominguez Sarmiento said...

I realize this post is old, but you can do an instanceof check in a base superclass. This is what our superclass equals method looks like:

public boolean equals(Object other) {
if (other == this) {
return true;
} else if (other == null) {
return false;
} else if (!getClass().isAssignableFrom(other.getClass())) {
return false;
} else if (id == null) {
return false;
} else {
return id.equals(((Entity) other).getId());
}

The secret is Class#isAssignableFrom() which is basically the same as an instanceof check.