usingRecursiveComparison() returns false positive when comparing to a Spring Data JPA projection

[sapsucker58]

• spring-boot-starter-parent version: 3.3.2 (with managed dependency versions)
• assertj core version: 3.25.3
• java version: 17
• test framework version: junit-jupiter-api 5.10.3

Test case reproducing the bug

import jakarta.persistence.Entity;
import jakarta.persistence.Id;
import org.springframework.data.jpa.repository.JpaRepository;
import org.springframework.data.jpa.repository.Query;
import org.springframework.stereotype.Repository;

@Repository
public interface PersonRepo extends JpaRepository<PersonRepo.PersonEntity, String> {
    @Entity
    public static class PersonEntity {
        @Id
        private String name;
    }

    public interface Person {
        String getName();
    }

    @Query(value = "SELECT 'alice' as name")
    Person getPerson();
}

import org.junit.jupiter.api.Test;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabase;
import org.springframework.boot.test.autoconfigure.jdbc.AutoConfigureTestDatabase.Replace;
import org.springframework.boot.test.autoconfigure.orm.jpa.DataJpaTest;

import static org.assertj.core.api.Assertions.assertThat;

@DataJpaTest
@AutoConfigureTestDatabase(replace = Replace.NONE)
public class PersonRepoTest {
    @Autowired
    private PersonRepo personRepo;

    private record PersonImpl(String getName) implements PersonRepo.Person {
    }

    @Test
    public void test() {
        PersonRepo.Person alice = personRepo.getPerson();
        PersonRepo.Person bob = new PersonImpl("bob");
        // Recursive comparison incorrectly asserts that alice and bob are the same
        assertThat(alice).usingRecursiveComparison().isEqualTo(bob);
    }
}

[joel-costigliola]

@sapsucker58 I have tried to reproduce the issue but got it working, can you try with version 3.26.3 and see if it also succeeds on your side.

To set assertj version to 3.26.3 you likely have to exclude the version from spring boot - see #3533 (comment) on how to do that.

[scordio]

@sapsucker58 you can also tune the AssertJ version via the assertj.version build property – more at https://assertj.github.io/doc/#spring-boot

[sapsucker58]

Thanks for the tip. I tried using version 3.26.3 and I get the same result - the test posted above passes when it should fail.

Likewise, the opposite test fails when I am expecting it to pass:

assertThat(alice).usingRecursiveComparison().isNotEqualTo(bob);

Fails with the message:

Expecting actual: org.springframework.data.jpa.repository.query.AbstractJpaQuery$TupleConverter$TupleBackedMap@3a678e40 not to be equal to: PersonImpl[getName=bob] when recursively comparing field by field

But it sounds like you're not able to reproduce what I'm seeing? My os information is OS name: "mac os x", version: "14.5", arch: "aarch64", family: "mac"

[scordio]

I'll give it a try and get back to you

[joel-costigliola]

You are right, the test succeeds locally for me, I did some investigation, it turns out that the problem comes from the fact that the instance we want to compare is proxied by Spring, instead of introspecting the real class, AssertJ introspects the proxied class as shown below.

I can see 7 fields in the debugger but all of them are static, as the recursive comparison ignores static fields since we want to compare instances and not class level fieldls, it ends up comparing nothing which explains it succeeds.

What can we do about that, I'm not too sure, at least we could maybe warn when no fields are compared to give some feedback on what really happened.

Thoughts ?

[sapsucker58]

I can't think of a way around it either. I think your idea of adding a warning when no fields are compared would be very helpful. Then it would at least be harder to unwittingly have false positives in tests.

I can see the interface values buried several layers deep but I likewise don't know how to get around the proxied class.

[scordio]

The following can help bring out the underlying type:

    @Test
    public void test() {
        PersonRepo.Person alice = personRepo.getPerson();
        PersonRepo.Person bob = new PersonImpl("bob");

        assertThat(alice)
                .asInstanceOf(type(org.springframework.data.projection.TargetAware.class))
                .extracting(org.springframework.data.projection.TargetAware::getTarget)
                .usingRecursiveComparison()
                .isEqualTo(bob);
    }

However, this fails with:

java.lang.AssertionError: 
Expecting actual:
  {"name"="alice"}
to be equal to:
  PersonImpl[getName=bob]
when recursively comparing field by field, but found the following difference:

Top level actual and expected objects differ:
- actual value  : {"name"="alice"}
- expected value: PersonImpl[getName=bob]
org.springframework.data.jpa.repository.query.AbstractJpaQuery$TupleConverter$TupleBackedMap can't be compared to io.github.scordio.playground.PersonRepoTest$PersonImpl as PersonImpl does not declare all TupleBackedMap fields, it lacks these: [tuple]

It seems Spring Data stores the projection data into a Map<String, Object> backed by a Jakarta Persistence Tuple.

So, getting the proxy target is anyway a direction that leads to Spring Data implementation details and would be fragile.

[scordio]

@joel-costigliola I'm thinking about having some proxy-specific handling.

My rough idea is that, for the current example:

• alice instanceof java.lang.reflect.Proxy returns true --> decision point for special handling
• alice.getClass().getInterfaces() returns:
  1. interface io.github.scordio.playground.PersonRepo$Person
  2. interface org.springframework.data.projection.TargetAware
  3. interface org.springframework.aop.SpringProxy
  4. interface org.springframework.core.DecoratingProxy

Only the first interface is what we need, now the question is how to decide which one is the good one...

[scordio]

I couldn't find any straightforward way to identify the "good" interface that the proxy implements so I made it work with a custom introspection strategy that identifies the properties to check based on a given type:

@Test
public void test() {
    Person alice = personRepo.getPerson();
    Person bob = new PersonImpl("bob");

    assertThat(alice)
        .usingRecursiveComparison()
        .withIntrospectionStrategy(new ComparingTypeProperties(Person.class))
        .isEqualTo(bob);
}

static class ComparingTypeProperties extends org.assertj.core.api.recursive.comparison.ComparingProperties {

    private static final String GET_PREFIX = "get";
    private static final String IS_PREFIX = "is";

    private final Map<Class<?>, Set<String>> propertiesNamesPerClass = new ConcurrentHashMap<>();

    private final Class<?> type;

    ComparingTypeProperties(Class<?> type) {
        this.type = type;
    }

    @Override
    public Set<String> getChildrenNodeNamesOf(Object node) {
        if (node == null) {
            return new HashSet<>();
        }
        if (type.isInstance(node)) {
            return propertiesNamesPerClass.computeIfAbsent(type, ComparingTypeProperties::getPropertiesNamesOf);
        }
        throw new IllegalArgumentException();
    }

    // code below copy-pasted from ComparingProperties due to private/package-private access

    private static Set<String> getPropertiesNamesOf(Class<?> clazz) {
        return gettersIncludingInheritedOf(clazz).stream()
            .map(Method::getName)
            .map(ComparingTypeProperties::toPropertyName)
            .collect(toSet());
    }

    private static String toPropertyName(String methodName) {
        String propertyWithCapitalLetter = methodName.startsWith(GET_PREFIX)
                ? methodName.substring(GET_PREFIX.length()) : methodName.substring(IS_PREFIX.length());
        return propertyWithCapitalLetter.toLowerCase().charAt(0) + propertyWithCapitalLetter.substring(1);
    }

}

The example now fails with:

java.lang.AssertionError: 
Expecting actual:
  org.springframework.data.jpa.repository.query.AbstractJpaQuery$TupleConverter$TupleBackedMap@c7d173f
to be equal to:
  PersonImpl[getName=bob]
when recursively comparing field by field, but found the following difference:

field/property 'name' differ:
- actual value  : "alice"
- expected value: "bob"

This is just to spark further discussion, I'm not saying you should try this at home 😄

[joel-costigliola]

Thanks for the investigation @scordio !

I don't think we should support this use case out of the box with some introspection relyong on internal details of Spring, whenever they decide to change the internal mechanism, AssertJ would break.

We could though add ComparingTypeProperties to our codebase and document that this would work for this use case, probably need to rename it ComparingTypePropertiedProxiedBySpring to make it clear it only works for Spring.

WDYT ?

[scordio]

Although the trigger was Spring, the issue applies generically to any code using java.lang.reflect.Proxy, whether it's a framework, library, or user code. For example, I don't have concrete experience with Quarkus, but I would imagine something similar could happen also there.

For example, here's a reproducer in pure Java:

@Test
public void test() {
    Person alice = (Person) Proxy.newProxyInstance(Person.class.getClassLoader(),
            new Class[] { Person.class },
            new ProxyInvocationHandler("alice"));
    Person bob = new PersonImpl("bob");
    // Recursive comparison incorrectly asserts that alice and bob are the same
    assertThat(alice).usingRecursiveComparison()
        .isEqualTo(bob);
}

static class ProxyInvocationHandler implements InvocationHandler {

    private final String name;

    ProxyInvocationHandler(String name) {
        this.name = name;
    }

    @Override
    public Object invoke(Object proxy, Method method, Object[] args) {
        switch (method.getName()) {
            case "getName":
                return name;
            case "toString":
                return "Proxy for Person[getName=" + name + "]";
            default:
                throw new IllegalArgumentException(method.getName());
        }
    }

}

When using ComparingTypeProperties as the introspection strategy, it fails with:

java.lang.AssertionError: 
Expecting actual:
  Proxy for Person[getName=alice]
to be equal to:
  PersonImpl[getName=bob]
when recursively comparing field by field, but found the following difference:

field/property 'name' differ:
- actual value  : "alice"
- expected value: "bob"

I agree we could add ComparingTypeProperties with a better name. Proxied objects are one valid use case for such a strategy, which is worth mentioning in the Javadoc, but I would focus on the fact that the strategy allows partial property comparison. Maybe we should even allow a vararg of types in the constructor.

What about a withPropertyComparisonForTypes(Class<?>...) or withPartialPropertyComparison(Class<?>...) config method?

[scordio]

By the way, we should check how this approach behaves with a more complex object graph, e.g., when the given types also appear in nested fields.

It may be nothing to be worried about, but it's something I didn't try out.

[scordio]

I still wonder how to improve the user experience for the original example...

We could throw some misconfiguration exception in case the object under test is a proxy (detected via Proxy.isProxyClass()), no fields are found and expected is not a proxy. Kind of similar to how Mockito fails when stubbing is not done correctly.

WDYT?

[sapsucker58]

+1 From a user experience, I think throwing a misconfiguration exception of some sort in this case would be very helpful.