Skip to content
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

AbstractPersistable.equals(…) always returns false if target entity is a proxy [DATAJPA-848] #1205

Closed
spring-projects-issues opened this issue Jan 15, 2016 · 4 comments
Assignees
Labels
in: core type: bug

Comments

@spring-projects-issues
Copy link

spring-projects-issues commented Jan 15, 2016

Ruslan Stelmachenko opened DATAJPA-848 and commented

I found a problem with org.springframework.data.jpa.domain.AbstractPersistable class equals method implementation.

It always return false if target entity for comparison is javassist proxy which hibernate returns for example if you do entityRepository.getOne(123) instead of findOne.

The problem is with line

if (!getClass().equals(obj.getClass())) {

The obj.getClass returns something like Employee_$$_jvst860_de while this already deproxyfied and has class Employee. So this two classes of course not equal.

I think we can do instead this:

if (!getClass().isInstance(obj)) {

Or this:

Class<?> otherClass = HibernateProxyHelper.getClassWithoutInitializingProxy(obj);
if (!getClass().equals(otherClass)) {

The second variant is more "right" because it excludes subclasses but using HibernateProxyHelper in AbstractPersistable is bad idea I think.

The first variant works but can't guarantee that obj is same class as this and not subclass (other than proxy). But I think even this is better then just return false for the same entity.

Simple code to reproduce this bug:

Employee e1 = employeeRepository.getOne(15);
Employee e2 = employeeRepository.getOne(15);
System.out.println(e1.getClass()); // Employee_$$_jvst860_de
System.out.println(e2.getClass()); // Employee_$$_jvst860_de
// The classes equal here but inside e1.equals method e1's 'this' has class Employee (not proxy)
System.out.println(e1.equals(e2)); // false

Affects: 1.9.2 (Gosling SR2)

Backported to: 1.9.4 (Gosling SR4), 1.8.3 (Fowler SR3)

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Jan 15, 2016

Oliver Drotbohm commented

That's fixed. We now inspect the user class instead of the one of the given object directly

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Feb 25, 2016

Neale Upstone commented

For those interested, there are code path changes in JDK1.8 which mean that equals gets called with arguments swapped (effectively return other.equals(this);.

It's easy to end up with generated equals() implementations that worked when adding to a HashMap in 1.7 (when 'this' is the proxied object and is assumed to be of the target type), but fail in 1.8 (because now 'this' is the target type, and other is the proxy)

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Feb 25, 2016

Oliver Drotbohm commented

Do you have pointers to read up on this, Neal?

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Feb 26, 2016

Neale Upstone commented

Hi,

I can't find anything specific, but we put the change of equals order down to JEP180 changes mentioned at https://docs.oracle.com/javase/8/docs/technotes/guides/collections/changes8.html.

We got caught out on the update not only on non-commutative equals, but also with hashCode implementations that had direct field access.

A typical fix was as follows:

     public boolean equals(Object o) {
         if (this == o) return true;
-        if (o == null || getClass() != o.getClass()) return false;
+        if (o == null || !(o instanceof Group)) return false;
 
         Group group = (Group) o;
 
-        if (name != null ? !name.equals(group.name) : group.name != null) return false;
+        if (getName() != null ? !getName().equals(group.getName()) : group.getName() != null) return false;
 
         return true;
     }
 
     @Override
     public int hashCode() {
-        return name != null ? name.hashCode() : 0;
+        return getName() != null ? getName().hashCode() : 0;

For us this was in specific cases where it had been working, really by luck, in that one of the objects was a new entity, and the other was an existing proxy-based entity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: core type: bug
Projects
None yet
Development

No branches or pull requests

2 participants