Skip to content

Commit

Permalink
The recursive comparison should never use equals on root objects sinc…
Browse files Browse the repository at this point in the history
…e it defeats the purpose of the recursive comparison.

This applies to objects in root containers like list, optional, array and maps.
Fix #3320
  • Loading branch information
Joel Costigliola committed Jan 15, 2024
1 parent e0f4542 commit 7f786bd
Show file tree
Hide file tree
Showing 6 changed files with 162 additions and 120 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -783,12 +783,17 @@ boolean hasCustomComparator(DualValue dualValue) {
}

boolean shouldIgnoreOverriddenEqualsOf(DualValue dualValue) {
// root objects are not compared with equals as it makes the recursive comparison pointless (use isEqualsTo instead)
if (dualValue.fieldLocation.isRoot()) return true;
// we must compare java basic types otherwise the recursive comparison loops infinitely!
if (dualValue.isActualJavaType()) return false;
// enums don't have fields, comparing them field by field makes no sense, we need to use equals which is overridden and final
// enums don't have fields, comparing them field by field makes no sense; we need to use equals, which is overridden and final
if (dualValue.isActualAnEnum()) return false;
// if there are some compared fields, we must only honor overridden equals on them, if the value is not a compared
// field then we treat as usual and ignore its equals method and introspects it
if (someComparedFieldsHaveBeenSpecified() && !exactlyMatchesAnyComparedFields(dualValue)) return true;
return ignoreAllOverriddenEquals
|| matchesAnIgnoredOverriddenEqualsField(dualValue.fieldLocation)
|| matchesAnIgnoredOverriddenEqualsField(dualValue)
|| (dualValue.actual != null && shouldIgnoreOverriddenEqualsOf(dualValue.actual.getClass()));
}

Expand Down Expand Up @@ -902,7 +907,8 @@ private boolean matchesAnIgnoredOverriddenEqualsType(Class<?> clazz) {
return ignoredOverriddenEqualsForTypes.contains(clazz);
}

private boolean matchesAnIgnoredOverriddenEqualsField(FieldLocation fieldLocation) {
private boolean matchesAnIgnoredOverriddenEqualsField(DualValue dualValue) {
FieldLocation fieldLocation = dualValue.fieldLocation;
return ignoredOverriddenEqualsForFields.stream().anyMatch(fieldLocation::exactlyMatches)
|| matchesAnIgnoredOverriddenEqualsRegex(fieldLocation);
}
Expand Down Expand Up @@ -1527,7 +1533,7 @@ public RecursiveComparisonConfiguration build() {
}
}

@SuppressWarnings({ "rawtypes", "unchecked" })
@SuppressWarnings({ "rawtypes", "unchecked", "ComparatorMethodParameterNotUsed" })
private static Comparator toComparator(BiPredicate equals) {
requireNonNull(equals, "Expecting a non null BiPredicate");
return (o1, o2) -> equals.test(o1, o2) ? 0 : 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -475,8 +475,8 @@ private static boolean shouldHonorEquals(DualValue dualValue,

private static boolean shouldHonorOverriddenEquals(DualValue dualValue,
RecursiveComparisonConfiguration recursiveComparisonConfiguration) {
boolean shouldNotIgnoreOverriddenEqualsIfAny = !recursiveComparisonConfiguration.shouldIgnoreOverriddenEqualsOf(dualValue);
return shouldNotIgnoreOverriddenEqualsIfAny && dualValue.actual != null && hasOverriddenEquals(dualValue.actual.getClass());
boolean shouldHonorOverriddenEqualsIfAny = !recursiveComparisonConfiguration.shouldIgnoreOverriddenEqualsOf(dualValue);
return shouldHonorOverriddenEqualsIfAny && dualValue.actual != null && hasOverriddenEquals(dualValue.actual.getClass());
}

private static void compareArrays(DualValue dualValue, ComparisonState comparisonState) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,13 @@
*/
package org.assertj.core.api.recursive.comparison;

import static java.lang.String.format;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.BDDAssertions.then;
import static org.assertj.core.util.Arrays.array;
import static org.assertj.core.util.AssertionsUtil.expectAssertionError;
import static org.assertj.core.util.Lists.list;
import static org.assertj.core.util.Maps.newHashMap;
import static org.junit.jupiter.params.provider.Arguments.arguments;

import java.util.Objects;
Expand All @@ -33,6 +37,42 @@
public class RecursiveComparisonAssert_isEqualTo_usingOverriddenEquals_Test
extends RecursiveComparisonAssert_isEqualTo_BaseTest implements PersonData {

@ParameterizedTest(name = "{2}: actual={0} / expected={1}")
@MethodSource
void should_pass_when_using_overridden_equals(Object actual, Object expected, String testDescription) {
then(actual).usingRecursiveComparison()
.usingOverriddenEquals()
.isEqualTo(expected);
}

private static Stream<Arguments> should_pass_when_using_overridden_equals() {
Person neighbourJohn = new Person();
neighbourJohn.neighbour = new AlwaysEqualPerson("John");
Person neighbourJack = new Person();
neighbourJack.neighbour = new AlwaysEqualPerson("Jack");

WithObject withPerson1 = new WithObject(new PersonComparedByName("John", "green", new Pet("Ducky", "Duck")));
WithObject withPerson2 = new WithObject(new PersonComparedByName("John", "blue", new Pet("Mia", "Duck")));

NeverEquals neverEquals1 = new NeverEquals(new PersonComparedByName("John", "green", null));
NeverEquals neverEquals2 = new NeverEquals(new PersonComparedByName("John", "red", new Pet("Mia", "Duck")));

return Stream.of(arguments(withPerson1, withPerson2,
"different pets and colors but equals ignore them"),
arguments(list(withPerson1), list(withPerson2),
"list: different pets and colors but equals ignore them"),
arguments(array(withPerson1), array(withPerson2),
"array: different pets and colors but equals ignore them"),
arguments(newHashMap("person", withPerson1), newHashMap("person", withPerson2),
"maps: different pets and colors but equals ignore them"),
arguments(Optional.of(withPerson1), Optional.of(withPerson2),
"Optional: different pets and colors but equals ignore them"),
arguments(neighbourJohn, neighbourJack,
"neighbour type equals is always true"),
arguments(neverEquals1, neverEquals2,
"root objects are never compared with equals, their fields are"));
}

@ParameterizedTest(name = "{2}: actual={0} / expected={1}")
@MethodSource
void should_fail_when_using_overridden_equals(Object actual, Object expected, String testDescription) {
Expand All @@ -45,61 +85,38 @@ void should_fail_when_using_overridden_equals(Object actual, Object expected, St
}

private static Stream<Arguments> should_fail_when_using_overridden_equals() {
Person person1 = new AlwaysDifferentPerson();
person1.neighbour = new Person("John");
Person person2 = new AlwaysDifferentPerson();
person2.neighbour = new Person("John");

Person person3 = new Person();
person3.neighbour = new AlwaysDifferentPerson();
person3.neighbour.name = "John";
Person person4 = new Person();
person4.neighbour = new AlwaysDifferentPerson();
person4.neighbour.name = "John";

return Stream.of(arguments(person1, person2, "root Person is AlwaysDifferentPerson"),
arguments(person3, person4, "neighbour Person is AlwaysDifferentPerson"));
}

@ParameterizedTest(name = "{2}: actual={0} / expected={1}")
@MethodSource
void should_pass_when_using_overridden_equals(Object actual, Object expected, String testDescription) {
then(actual).usingRecursiveComparison()
.usingOverriddenEquals()
.isEqualTo(expected);
Person neighbourJohn = new Person();
neighbourJohn.neighbour = new AlwaysDifferentPerson();
neighbourJohn.neighbour.name = "John";
Person differentNeighbourJohn = new Person();
differentNeighbourJohn.neighbour = new AlwaysDifferentPerson();
differentNeighbourJohn.neighbour.name = "John";
return Stream.of(arguments(neighbourJohn, differentNeighbourJohn,
"neighbour type is AlwaysDifferentPerson"),
arguments(list(neighbourJohn), list(differentNeighbourJohn),
"list: neighbour type is AlwaysDifferentPerson"),
arguments(array(neighbourJohn), array(differentNeighbourJohn),
"array: neighbour type is AlwaysDifferentPerson"),
arguments(newHashMap("person", neighbourJohn), newHashMap("person", differentNeighbourJohn),
"maps: neighbour type is AlwaysDifferentPerson"),
arguments(Optional.of(neighbourJohn), Optional.of(differentNeighbourJohn),
"Optional: neighbour type is AlwaysDifferentPerson"));
}

private static Stream<Arguments> should_pass_when_using_overridden_equals() {
Person person1 = new AlwaysEqualPerson();
person1.neighbour = new Person("John");
Person person2 = new AlwaysEqualPerson();
person2.neighbour = new Person("Jack");

Person person3 = new Person();
person3.neighbour = new AlwaysEqualPerson();
person3.neighbour.name = "John";
Person person4 = new Person();
person4.neighbour = new AlwaysEqualPerson();
person4.neighbour.name = "Jack";

return Stream.of(arguments(person1, person2, "root Person is AlwaysEqualPerson"),
arguments(person3, person4, "neighbour Person is AlwaysEqualPerson"));
}

static class PersonWithOverriddenEquals {
static class PersonComparedByName {
String name;
String color;
Pet pet;

public PersonWithOverriddenEquals(String name, String color, Pet pet) {
public PersonComparedByName(String name, String color, Pet pet) {
this.name = name; // only name is used in equals
this.color = color;
this.pet = pet;
}

@Override
public boolean equals(Object o) {
PersonWithOverriddenEquals person = (PersonWithOverriddenEquals) o;
PersonComparedByName person = (PersonComparedByName) o;
return Objects.equals(name, person.name);
}

Expand All @@ -110,7 +127,7 @@ public int hashCode() {

@Override
public String toString() {
return String.format("Person [name=%s, color=%s]", name, color);
return format("Person [name=%s, color=%s]", name, color);
}
}

Expand All @@ -135,47 +152,93 @@ public int hashCode() {
}
}

static class PersonWrapper {
PersonWithOverriddenEquals person;

public PersonWrapper(PersonWithOverriddenEquals person) {
this.person = person;
}

}

@Test
void should_pass_when_comparison_using_overriden_equals_on_root_objects() {
void should_use_equals_on_compared_field_only() {
// GIVEN
PersonWithOverriddenEquals person1 = new PersonWithOverriddenEquals("John", "green", new Pet("Ducky", "Duck"));
PersonWithOverriddenEquals person2 = new PersonWithOverriddenEquals("John", "blue", new Pet("Mia", "Duck"));
WithObject actual = new WithObject(new A("abc", new NeverEquals("never"), new AlwaysEquals("always")));
WithObject expected = new WithObject(new A("abc", new NeverEquals("never"), new AlwaysEquals("always")));
// WHEN/THEN
then(person1).usingRecursiveComparison()
.usingOverriddenEquals()
.isEqualTo(person2);
then(actual).usingRecursiveComparison()
.usingOverriddenEquals()
.comparingOnlyFields("group.name", "group.neverEquals.name")
.isEqualTo(expected);
}

@Test
void should_pass_when_comparison_using_overriden_equals_on_fields() {
void should_fail_since_the_compared_field_equals_returns_false_even_if_the_outer_field_equals_returns_true() {
// GIVEN
Optional<PersonWithOverriddenEquals> person1 = Optional.of(new PersonWithOverriddenEquals("John", "green",
new Pet("Ducky", "Duck")));
Optional<PersonWithOverriddenEquals> person2 = Optional.of(new PersonWithOverriddenEquals("John", "green",
new Pet("Mia", "Duck")));
// WHEN/THEN
then(person1).usingRecursiveComparison()
.usingOverriddenEquals()
.isEqualTo(person2);
WithObject actual = new WithObject(new A("abc", new NeverEquals("never"), new AlwaysEquals("always")));
WithObject expected = new WithObject(new A("abc", new NeverEquals("never"), new AlwaysEquals("always")));
// WHEN
AssertionError assertionError = expectAssertionError(() -> assertThat(actual).usingRecursiveComparison()
.usingOverriddenEquals()
.comparingOnlyFields("group.name",
"group.neverEquals")
.isEqualTo(expected));
// THEN
then(assertionError).hasMessageContaining("- equals methods were used in the comparison");
}

@Test
void should_pass_when_comparison_using_overriden_equals_on_person_wrapper() {
// GIVEN
PersonWrapper person1 = new PersonWrapper(new PersonWithOverriddenEquals("John", "green", new Pet("Ducky", "Duck")));
PersonWrapper person2 = new PersonWrapper(new PersonWithOverriddenEquals("John", "green", new Pet("Mia", "Duck")));
// WHEN/THEN
then(person1).usingRecursiveComparison()
.usingOverriddenEquals()
.isEqualTo(person2);
private static class A {
private final String name;
private final NeverEquals neverEquals;
private final AlwaysEquals alwaysEquals;

public A(String name, NeverEquals neverEquals, AlwaysEquals alwaysEquals) {
this.name = name;
this.neverEquals = neverEquals;
this.alwaysEquals = alwaysEquals;
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof A)) return false;
A a = (A) o;
return Objects.equals(name, a.name)
&& Objects.equals(neverEquals, a.neverEquals)
&& Objects.equals(alwaysEquals, a.alwaysEquals);
}

@Override
public String toString() {
return format("A[name=%s, neverEquals=%s, alwaysEquals=%s]", this.name, this.neverEquals, this.alwaysEquals);
}
}

private static class NeverEquals {
final Object name;

public NeverEquals(Object name) {
this.name = name;
}

@Override
public boolean equals(Object o) {
return false;
}

@Override
public String toString() {
return format("NeverEquals[name=%s]", this.name);
}
}

private static class AlwaysEquals {
final Object name;

public AlwaysEquals(Object name) {
this.name = name;
}

@Override
public boolean equals(Object o) {
return true;
}

@Override
public String toString() {
return format("AlwaysEquals[name=%s]", this.name);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,21 +23,16 @@
import static org.assertj.core.util.Arrays.array;
import static org.assertj.core.util.AssertionsUtil.expectAssertionError;
import static org.assertj.core.util.Lists.list;
import static org.assertj.core.util.Maps.newHashMap;
import static org.assertj.core.util.Sets.newLinkedHashSet;

import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Set;
import java.util.UnknownFormatConversionException;
import java.util.stream.Stream;

import org.assertj.core.api.RecursiveComparisonAssert_isEqualTo_BaseTest;
import org.assertj.core.groups.Tuple;
import org.assertj.core.internal.objects.data.PersonDto;
import org.assertj.core.test.Person;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
Expand All @@ -46,37 +41,6 @@
class RecursiveComparisonAssert_isEqualTo_with_iterables_Test extends RecursiveComparisonAssert_isEqualTo_BaseTest
implements PersonData {

@ParameterizedTest(name = "actual {0} / expected {1}")
@MethodSource
void should_fail_as_Person_overridden_equals_should_be_honored(Object actual, Object expected,
ComparisonDifference difference) {
// GIVEN
recursiveComparisonConfiguration.useOverriddenEquals();
// WHEN
compareRecursivelyFailsAsExpected(actual, expected);
// THEN
verifyShouldBeEqualByComparingFieldByFieldRecursivelyCall(actual, expected, difference);
}

static Stream<Arguments> should_fail_as_Person_overridden_equals_should_be_honored() {
// sheldon type is Person which overrides equals!
Iterable<Person> actualAsIterable = newHashSet(sheldon);
Iterable<PersonDto> expectAsIterable = newHashSet(sheldonDto);
Person[] actualAsArray = array(sheldon);
PersonDto[] expectedAsArray = array(sheldonDto);
Optional<Person> actualAsOptional = Optional.of(sheldon);
Optional<PersonDto> expectedAsOptional = Optional.of(sheldonDto);
Map<String, PersonDto> expectedAsMap = newHashMap("sheldon", sheldonDto);
Map<String, Person> actualAsMap = newHashMap("sheldon", sheldon);
return Stream.of(Arguments.of(actualAsIterable, expectAsIterable,
diff("", actualAsIterable, expectAsIterable,
format("The following expected elements were not matched in the actual HashSet:%n"
+ " [PersonDto [name=Sheldon, home=HomeDto [address=AddressDto [number=1]]]]"))),
Arguments.of(actualAsArray, expectedAsArray, diff("[0]", sheldon, sheldonDto)),
Arguments.of(actualAsOptional, expectedAsOptional, diff("value", sheldon, sheldonDto)),
Arguments.of(actualAsMap, expectedAsMap, diff("sheldon", sheldon, sheldonDto)));
}

@ParameterizedTest(name = "author 1 {0} / author 2 {1}")
@MethodSource
void should_pass_when_comparing_same_collection_fields(Collection<Author> authors1, Collection<Author> authors2) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,4 @@ public String toString() {
return format("WithObject group=%s", group);
}

}
}
Loading

0 comments on commit 7f786bd

Please sign in to comment.