Skip to content

Commit 7f786bd

Browse files
author
Joel Costigliola
committed
The recursive comparison should never use equals on root objects since it defeats the purpose of the recursive comparison.
This applies to objects in root containers like list, optional, array and maps. Fix #3320
1 parent e0f4542 commit 7f786bd

File tree

6 files changed

+162
-120
lines changed

6 files changed

+162
-120
lines changed

‎assertj-core/src/main/java/org/assertj/core/api/recursive/comparison/RecursiveComparisonConfiguration.java

+10-4
Original file line numberDiff line numberDiff line change
@@ -783,12 +783,17 @@ boolean hasCustomComparator(DualValue dualValue) {
783783
}
784784

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

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

905-
private boolean matchesAnIgnoredOverriddenEqualsField(FieldLocation fieldLocation) {
910+
private boolean matchesAnIgnoredOverriddenEqualsField(DualValue dualValue) {
911+
FieldLocation fieldLocation = dualValue.fieldLocation;
906912
return ignoredOverriddenEqualsForFields.stream().anyMatch(fieldLocation::exactlyMatches)
907913
|| matchesAnIgnoredOverriddenEqualsRegex(fieldLocation);
908914
}
@@ -1527,7 +1533,7 @@ public RecursiveComparisonConfiguration build() {
15271533
}
15281534
}
15291535

1530-
@SuppressWarnings({ "rawtypes", "unchecked" })
1536+
@SuppressWarnings({ "rawtypes", "unchecked", "ComparatorMethodParameterNotUsed" })
15311537
private static Comparator toComparator(BiPredicate equals) {
15321538
requireNonNull(equals, "Expecting a non null BiPredicate");
15331539
return (o1, o2) -> equals.test(o1, o2) ? 0 : 1;

‎assertj-core/src/main/java/org/assertj/core/api/recursive/comparison/RecursiveComparisonDifferenceCalculator.java

+2-2
Original file line numberDiff line numberDiff line change
@@ -475,8 +475,8 @@ private static boolean shouldHonorEquals(DualValue dualValue,
475475

476476
private static boolean shouldHonorOverriddenEquals(DualValue dualValue,
477477
RecursiveComparisonConfiguration recursiveComparisonConfiguration) {
478-
boolean shouldNotIgnoreOverriddenEqualsIfAny = !recursiveComparisonConfiguration.shouldIgnoreOverriddenEqualsOf(dualValue);
479-
return shouldNotIgnoreOverriddenEqualsIfAny && dualValue.actual != null && hasOverriddenEquals(dualValue.actual.getClass());
478+
boolean shouldHonorOverriddenEqualsIfAny = !recursiveComparisonConfiguration.shouldIgnoreOverriddenEqualsOf(dualValue);
479+
return shouldHonorOverriddenEqualsIfAny && dualValue.actual != null && hasOverriddenEquals(dualValue.actual.getClass());
480480
}
481481

482482
private static void compareArrays(DualValue dualValue, ComparisonState comparisonState) {

‎assertj-core/src/test/java/org/assertj/core/api/recursive/comparison/RecursiveComparisonAssert_isEqualTo_usingOverriddenEquals_Test.java

+139-76
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,13 @@
1212
*/
1313
package org.assertj.core.api.recursive.comparison;
1414

15+
import static java.lang.String.format;
1516
import static org.assertj.core.api.Assertions.assertThat;
1617
import static org.assertj.core.api.BDDAssertions.then;
18+
import static org.assertj.core.util.Arrays.array;
1719
import static org.assertj.core.util.AssertionsUtil.expectAssertionError;
20+
import static org.assertj.core.util.Lists.list;
21+
import static org.assertj.core.util.Maps.newHashMap;
1822
import static org.junit.jupiter.params.provider.Arguments.arguments;
1923

2024
import java.util.Objects;
@@ -33,6 +37,42 @@
3337
public class RecursiveComparisonAssert_isEqualTo_usingOverriddenEquals_Test
3438
extends RecursiveComparisonAssert_isEqualTo_BaseTest implements PersonData {
3539

40+
@ParameterizedTest(name = "{2}: actual={0} / expected={1}")
41+
@MethodSource
42+
void should_pass_when_using_overridden_equals(Object actual, Object expected, String testDescription) {
43+
then(actual).usingRecursiveComparison()
44+
.usingOverriddenEquals()
45+
.isEqualTo(expected);
46+
}
47+
48+
private static Stream<Arguments> should_pass_when_using_overridden_equals() {
49+
Person neighbourJohn = new Person();
50+
neighbourJohn.neighbour = new AlwaysEqualPerson("John");
51+
Person neighbourJack = new Person();
52+
neighbourJack.neighbour = new AlwaysEqualPerson("Jack");
53+
54+
WithObject withPerson1 = new WithObject(new PersonComparedByName("John", "green", new Pet("Ducky", "Duck")));
55+
WithObject withPerson2 = new WithObject(new PersonComparedByName("John", "blue", new Pet("Mia", "Duck")));
56+
57+
NeverEquals neverEquals1 = new NeverEquals(new PersonComparedByName("John", "green", null));
58+
NeverEquals neverEquals2 = new NeverEquals(new PersonComparedByName("John", "red", new Pet("Mia", "Duck")));
59+
60+
return Stream.of(arguments(withPerson1, withPerson2,
61+
"different pets and colors but equals ignore them"),
62+
arguments(list(withPerson1), list(withPerson2),
63+
"list: different pets and colors but equals ignore them"),
64+
arguments(array(withPerson1), array(withPerson2),
65+
"array: different pets and colors but equals ignore them"),
66+
arguments(newHashMap("person", withPerson1), newHashMap("person", withPerson2),
67+
"maps: different pets and colors but equals ignore them"),
68+
arguments(Optional.of(withPerson1), Optional.of(withPerson2),
69+
"Optional: different pets and colors but equals ignore them"),
70+
arguments(neighbourJohn, neighbourJack,
71+
"neighbour type equals is always true"),
72+
arguments(neverEquals1, neverEquals2,
73+
"root objects are never compared with equals, their fields are"));
74+
}
75+
3676
@ParameterizedTest(name = "{2}: actual={0} / expected={1}")
3777
@MethodSource
3878
void should_fail_when_using_overridden_equals(Object actual, Object expected, String testDescription) {
@@ -45,61 +85,38 @@ void should_fail_when_using_overridden_equals(Object actual, Object expected, St
4585
}
4686

4787
private static Stream<Arguments> should_fail_when_using_overridden_equals() {
48-
Person person1 = new AlwaysDifferentPerson();
49-
person1.neighbour = new Person("John");
50-
Person person2 = new AlwaysDifferentPerson();
51-
person2.neighbour = new Person("John");
52-
53-
Person person3 = new Person();
54-
person3.neighbour = new AlwaysDifferentPerson();
55-
person3.neighbour.name = "John";
56-
Person person4 = new Person();
57-
person4.neighbour = new AlwaysDifferentPerson();
58-
person4.neighbour.name = "John";
59-
60-
return Stream.of(arguments(person1, person2, "root Person is AlwaysDifferentPerson"),
61-
arguments(person3, person4, "neighbour Person is AlwaysDifferentPerson"));
62-
}
63-
64-
@ParameterizedTest(name = "{2}: actual={0} / expected={1}")
65-
@MethodSource
66-
void should_pass_when_using_overridden_equals(Object actual, Object expected, String testDescription) {
67-
then(actual).usingRecursiveComparison()
68-
.usingOverriddenEquals()
69-
.isEqualTo(expected);
88+
Person neighbourJohn = new Person();
89+
neighbourJohn.neighbour = new AlwaysDifferentPerson();
90+
neighbourJohn.neighbour.name = "John";
91+
Person differentNeighbourJohn = new Person();
92+
differentNeighbourJohn.neighbour = new AlwaysDifferentPerson();
93+
differentNeighbourJohn.neighbour.name = "John";
94+
return Stream.of(arguments(neighbourJohn, differentNeighbourJohn,
95+
"neighbour type is AlwaysDifferentPerson"),
96+
arguments(list(neighbourJohn), list(differentNeighbourJohn),
97+
"list: neighbour type is AlwaysDifferentPerson"),
98+
arguments(array(neighbourJohn), array(differentNeighbourJohn),
99+
"array: neighbour type is AlwaysDifferentPerson"),
100+
arguments(newHashMap("person", neighbourJohn), newHashMap("person", differentNeighbourJohn),
101+
"maps: neighbour type is AlwaysDifferentPerson"),
102+
arguments(Optional.of(neighbourJohn), Optional.of(differentNeighbourJohn),
103+
"Optional: neighbour type is AlwaysDifferentPerson"));
70104
}
71105

72-
private static Stream<Arguments> should_pass_when_using_overridden_equals() {
73-
Person person1 = new AlwaysEqualPerson();
74-
person1.neighbour = new Person("John");
75-
Person person2 = new AlwaysEqualPerson();
76-
person2.neighbour = new Person("Jack");
77-
78-
Person person3 = new Person();
79-
person3.neighbour = new AlwaysEqualPerson();
80-
person3.neighbour.name = "John";
81-
Person person4 = new Person();
82-
person4.neighbour = new AlwaysEqualPerson();
83-
person4.neighbour.name = "Jack";
84-
85-
return Stream.of(arguments(person1, person2, "root Person is AlwaysEqualPerson"),
86-
arguments(person3, person4, "neighbour Person is AlwaysEqualPerson"));
87-
}
88-
89-
static class PersonWithOverriddenEquals {
106+
static class PersonComparedByName {
90107
String name;
91108
String color;
92109
Pet pet;
93110

94-
public PersonWithOverriddenEquals(String name, String color, Pet pet) {
111+
public PersonComparedByName(String name, String color, Pet pet) {
95112
this.name = name; // only name is used in equals
96113
this.color = color;
97114
this.pet = pet;
98115
}
99116

100117
@Override
101118
public boolean equals(Object o) {
102-
PersonWithOverriddenEquals person = (PersonWithOverriddenEquals) o;
119+
PersonComparedByName person = (PersonComparedByName) o;
103120
return Objects.equals(name, person.name);
104121
}
105122

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

111128
@Override
112129
public String toString() {
113-
return String.format("Person [name=%s, color=%s]", name, color);
130+
return format("Person [name=%s, color=%s]", name, color);
114131
}
115132
}
116133

@@ -135,47 +152,93 @@ public int hashCode() {
135152
}
136153
}
137154

138-
static class PersonWrapper {
139-
PersonWithOverriddenEquals person;
140-
141-
public PersonWrapper(PersonWithOverriddenEquals person) {
142-
this.person = person;
143-
}
144-
145-
}
146-
147155
@Test
148-
void should_pass_when_comparison_using_overriden_equals_on_root_objects() {
156+
void should_use_equals_on_compared_field_only() {
149157
// GIVEN
150-
PersonWithOverriddenEquals person1 = new PersonWithOverriddenEquals("John", "green", new Pet("Ducky", "Duck"));
151-
PersonWithOverriddenEquals person2 = new PersonWithOverriddenEquals("John", "blue", new Pet("Mia", "Duck"));
158+
WithObject actual = new WithObject(new A("abc", new NeverEquals("never"), new AlwaysEquals("always")));
159+
WithObject expected = new WithObject(new A("abc", new NeverEquals("never"), new AlwaysEquals("always")));
152160
// WHEN/THEN
153-
then(person1).usingRecursiveComparison()
154-
.usingOverriddenEquals()
155-
.isEqualTo(person2);
161+
then(actual).usingRecursiveComparison()
162+
.usingOverriddenEquals()
163+
.comparingOnlyFields("group.name", "group.neverEquals.name")
164+
.isEqualTo(expected);
156165
}
157166

158167
@Test
159-
void should_pass_when_comparison_using_overriden_equals_on_fields() {
168+
void should_fail_since_the_compared_field_equals_returns_false_even_if_the_outer_field_equals_returns_true() {
160169
// GIVEN
161-
Optional<PersonWithOverriddenEquals> person1 = Optional.of(new PersonWithOverriddenEquals("John", "green",
162-
new Pet("Ducky", "Duck")));
163-
Optional<PersonWithOverriddenEquals> person2 = Optional.of(new PersonWithOverriddenEquals("John", "green",
164-
new Pet("Mia", "Duck")));
165-
// WHEN/THEN
166-
then(person1).usingRecursiveComparison()
167-
.usingOverriddenEquals()
168-
.isEqualTo(person2);
170+
WithObject actual = new WithObject(new A("abc", new NeverEquals("never"), new AlwaysEquals("always")));
171+
WithObject expected = new WithObject(new A("abc", new NeverEquals("never"), new AlwaysEquals("always")));
172+
// WHEN
173+
AssertionError assertionError = expectAssertionError(() -> assertThat(actual).usingRecursiveComparison()
174+
.usingOverriddenEquals()
175+
.comparingOnlyFields("group.name",
176+
"group.neverEquals")
177+
.isEqualTo(expected));
178+
// THEN
179+
then(assertionError).hasMessageContaining("- equals methods were used in the comparison");
169180
}
170181

171-
@Test
172-
void should_pass_when_comparison_using_overriden_equals_on_person_wrapper() {
173-
// GIVEN
174-
PersonWrapper person1 = new PersonWrapper(new PersonWithOverriddenEquals("John", "green", new Pet("Ducky", "Duck")));
175-
PersonWrapper person2 = new PersonWrapper(new PersonWithOverriddenEquals("John", "green", new Pet("Mia", "Duck")));
176-
// WHEN/THEN
177-
then(person1).usingRecursiveComparison()
178-
.usingOverriddenEquals()
179-
.isEqualTo(person2);
182+
private static class A {
183+
private final String name;
184+
private final NeverEquals neverEquals;
185+
private final AlwaysEquals alwaysEquals;
186+
187+
public A(String name, NeverEquals neverEquals, AlwaysEquals alwaysEquals) {
188+
this.name = name;
189+
this.neverEquals = neverEquals;
190+
this.alwaysEquals = alwaysEquals;
191+
}
192+
193+
@Override
194+
public boolean equals(Object o) {
195+
if (this == o) return true;
196+
if (!(o instanceof A)) return false;
197+
A a = (A) o;
198+
return Objects.equals(name, a.name)
199+
&& Objects.equals(neverEquals, a.neverEquals)
200+
&& Objects.equals(alwaysEquals, a.alwaysEquals);
201+
}
202+
203+
@Override
204+
public String toString() {
205+
return format("A[name=%s, neverEquals=%s, alwaysEquals=%s]", this.name, this.neverEquals, this.alwaysEquals);
206+
}
207+
}
208+
209+
private static class NeverEquals {
210+
final Object name;
211+
212+
public NeverEquals(Object name) {
213+
this.name = name;
214+
}
215+
216+
@Override
217+
public boolean equals(Object o) {
218+
return false;
219+
}
220+
221+
@Override
222+
public String toString() {
223+
return format("NeverEquals[name=%s]", this.name);
224+
}
225+
}
226+
227+
private static class AlwaysEquals {
228+
final Object name;
229+
230+
public AlwaysEquals(Object name) {
231+
this.name = name;
232+
}
233+
234+
@Override
235+
public boolean equals(Object o) {
236+
return true;
237+
}
238+
239+
@Override
240+
public String toString() {
241+
return format("AlwaysEquals[name=%s]", this.name);
242+
}
180243
}
181244
}

‎assertj-core/src/test/java/org/assertj/core/api/recursive/comparison/RecursiveComparisonAssert_isEqualTo_with_iterables_Test.java

-36
Original file line numberDiff line numberDiff line change
@@ -23,21 +23,16 @@
2323
import static org.assertj.core.util.Arrays.array;
2424
import static org.assertj.core.util.AssertionsUtil.expectAssertionError;
2525
import static org.assertj.core.util.Lists.list;
26-
import static org.assertj.core.util.Maps.newHashMap;
2726
import static org.assertj.core.util.Sets.newLinkedHashSet;
2827

2928
import java.util.Collection;
3029
import java.util.List;
31-
import java.util.Map;
32-
import java.util.Optional;
3330
import java.util.Set;
3431
import java.util.UnknownFormatConversionException;
3532
import java.util.stream.Stream;
3633

3734
import org.assertj.core.api.RecursiveComparisonAssert_isEqualTo_BaseTest;
3835
import org.assertj.core.groups.Tuple;
39-
import org.assertj.core.internal.objects.data.PersonDto;
40-
import org.assertj.core.test.Person;
4136
import org.junit.jupiter.api.Test;
4237
import org.junit.jupiter.params.ParameterizedTest;
4338
import org.junit.jupiter.params.provider.Arguments;
@@ -46,37 +41,6 @@
4641
class RecursiveComparisonAssert_isEqualTo_with_iterables_Test extends RecursiveComparisonAssert_isEqualTo_BaseTest
4742
implements PersonData {
4843

49-
@ParameterizedTest(name = "actual {0} / expected {1}")
50-
@MethodSource
51-
void should_fail_as_Person_overridden_equals_should_be_honored(Object actual, Object expected,
52-
ComparisonDifference difference) {
53-
// GIVEN
54-
recursiveComparisonConfiguration.useOverriddenEquals();
55-
// WHEN
56-
compareRecursivelyFailsAsExpected(actual, expected);
57-
// THEN
58-
verifyShouldBeEqualByComparingFieldByFieldRecursivelyCall(actual, expected, difference);
59-
}
60-
61-
static Stream<Arguments> should_fail_as_Person_overridden_equals_should_be_honored() {
62-
// sheldon type is Person which overrides equals!
63-
Iterable<Person> actualAsIterable = newHashSet(sheldon);
64-
Iterable<PersonDto> expectAsIterable = newHashSet(sheldonDto);
65-
Person[] actualAsArray = array(sheldon);
66-
PersonDto[] expectedAsArray = array(sheldonDto);
67-
Optional<Person> actualAsOptional = Optional.of(sheldon);
68-
Optional<PersonDto> expectedAsOptional = Optional.of(sheldonDto);
69-
Map<String, PersonDto> expectedAsMap = newHashMap("sheldon", sheldonDto);
70-
Map<String, Person> actualAsMap = newHashMap("sheldon", sheldon);
71-
return Stream.of(Arguments.of(actualAsIterable, expectAsIterable,
72-
diff("", actualAsIterable, expectAsIterable,
73-
format("The following expected elements were not matched in the actual HashSet:%n"
74-
+ " [PersonDto [name=Sheldon, home=HomeDto [address=AddressDto [number=1]]]]"))),
75-
Arguments.of(actualAsArray, expectedAsArray, diff("[0]", sheldon, sheldonDto)),
76-
Arguments.of(actualAsOptional, expectedAsOptional, diff("value", sheldon, sheldonDto)),
77-
Arguments.of(actualAsMap, expectedAsMap, diff("sheldon", sheldon, sheldonDto)));
78-
}
79-
8044
@ParameterizedTest(name = "author 1 {0} / author 2 {1}")
8145
@MethodSource
8246
void should_pass_when_comparing_same_collection_fields(Collection<Author> authors1, Collection<Author> authors2) {

‎assertj-core/src/test/java/org/assertj/core/api/recursive/comparison/WithObject.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -26,4 +26,4 @@ public String toString() {
2626
return format("WithObject group=%s", group);
2727
}
2828

29-
}
29+
}

0 commit comments

Comments
 (0)