Avoid tracking visited values that can't lead to cycle / infinite recursion.

Fixes #1854

Author: joel-costigliola

src/main/java/org/assertj/core/api/recursive/comparison/DualValue.java

@@ -60,6 +60,8 @@ final class DualValue {
   public boolean equals(Object other) {
     if (!(other instanceof DualValue)) return false;
     DualValue that = (DualValue) other;
+    // it is critical to compare by reference when tracking visited dual values.
+    // see should_fix_1854_minimal_test for an explanation
     return actual == that.actual && expected == that.expected;
   }
 
@@ -212,4 +214,15 @@ private static List<String> fiedlPath(List<String> parentPath, String fieldName)
     return fieldPath;
   }
 
+  public boolean hasPotentialCyclingValues() {
+    return isPotentialCyclingValue(actual) && isPotentialCyclingValue(expected);
+  }
+
+  private static boolean isPotentialCyclingValue(Object object) {
+    if (object == null) return false;
+    // java.lang are base types that can't cycle to themselves of other types
+    // we could check more type, but that's a good start
+    return !object.getClass().getCanonicalName().startsWith("java.lang");
+  }
+
 }

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

@@ -97,7 +97,11 @@ public boolean hasDualValuesToCompare() {
 
     public DualValue pickDualValueToCompare() {
       final DualValue dualValue = dualValuesToCompare.removeFirst();
-      visitedDualValues.add(dualValue);
+      if (dualValue.hasPotentialCyclingValues()) {
+        // visited dual values are here to avoid cycle, java types don't have cycle, there is no need to track them.
+        // moreover this would make should_fix_1854_minimal_test to fail (see the test for a detailed explanation)
+        visitedDualValues.add(dualValue);
+      }
       return dualValue;
     }
 
@@ -136,15 +140,13 @@ private void initDualValuesToCompare(Object actual, Object expected, List<String
       // parent -> set{child} with child having a reference back to parent
       // it occurs to unordered collection where we compare all possible combination of the collection elements recursively
       // --
-      // Don't use removeAll which uses equals to compare values, comparison must be done by reference otherwise we could remove
-      // values too agressively, that occurs when we add a duplicate of a value already visited.
-      // Example:
-      // - a and a are duplicates but are not the same object, i.e. a equals a' but a' != a
-      // - visitedDualValues = {a , b , c}
-      // - dualValuesToCompare = {a'}
-      // dualValuesToCompare.removeAll(visitedDualValues) would remove it which is incorrect
-      // If we compare references then a' won't be removed from dualValuesToCompare
-      visitedDualValues.forEach(visited -> dualValuesToCompare.removeIf(toCompare -> toCompare == visited));
+      // remove visited values one by one, DualValue.equals correctly compare respective actual and expected fields by reference
+      visitedDualValues.forEach(visitedDualValue -> {
+        dualValuesToCompare.stream()
+                           .filter(dualValueToCompare -> dualValueToCompare.equals(visitedDualValue))
+                           .findFirst()
+                           .ifPresent(dualValuesToCompare::remove);
+      });
     }
 
     private boolean mustCompareFieldsRecursively(boolean isRootObject, DualValue dualValue) {

src/test/java/org/assertj/core/api/recursive/comparison/DualValue_hasPotentialCyclingValues_Test.java

@@ -0,0 +1,69 @@
+/*
+ * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on
+ * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations under the License.
+ *
+ * Copyright 2012-2020 the original author or authors.
+ */
+package org.assertj.core.api.recursive.comparison;
+
+import static org.assertj.core.api.BDDAssertions.then;
+import static org.assertj.core.util.Lists.list;
+
+import java.util.List;
+import java.util.stream.Stream;
+
+import org.assertj.core.internal.objects.data.FriendlyPerson;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+@DisplayName("DualValue hasPotentialCyclingValues")
+public class DualValue_hasPotentialCyclingValues_Test {
+
+  private static final List<String> PATH = list("foo", "bar");
+
+  @ParameterizedTest(name = "actual {0} / expected {1}")
+  @MethodSource("values")
+  void should_return_false_when_actual_or_expected_is_a_container_type_and_true_otherwise(Object actual, Object expected,
+                                                                                          boolean expectedResult) {
+    // GIVEN
+    DualValue dualValue = new DualValue(PATH, actual, expected);
+    // WHEN
+    boolean hasPotentialCyclingValuess = dualValue.hasPotentialCyclingValues();
+    // THEN
+    then(hasPotentialCyclingValuess).isEqualTo(expectedResult);
+  }
+
+  static Stream<Arguments> values() {
+    FriendlyPerson person1 = new FriendlyPerson();
+    FriendlyPerson person2 = new FriendlyPerson();
+    person1.otherFriends.add(person1);
+    person1.otherFriends.add(person2);
+    person2.otherFriends.add(person2);
+    person2.otherFriends.add(person1);
+
+    return Stream.of(Arguments.of(null, person2, false),
+                     Arguments.of(person1, null, false),
+                     Arguments.of(person1, "abc", false),
+                     Arguments.of("abc", person2, false),
+                     Arguments.of("abc", 2, false),
+                     Arguments.of((byte) 1, (byte) 2, false),
+                     Arguments.of((short) 1, (short) 2, false),
+                     Arguments.of(1, 2, false),
+                     Arguments.of(1.0, 2.0, false),
+                     Arguments.of(1.0f, 2.0f, false),
+                     Arguments.of('a', 'b', false),
+                     Arguments.of(person1, person1, true),
+                     Arguments.of(person1, person2, true),
+                     Arguments.of(list(person1), list(person1), true),
+                     Arguments.of(list(person1), list(person2), true),
+                     Arguments.of(list(person1, person2), list(person2, person1), true));
+  }
+}

src/test/java/org/assertj/core/api/recursive/comparison/RecursiveComparisonAssert_isEqualTo_ignoringCollectionOrder_Test.java

@@ -295,4 +295,62 @@ public void should_fix_1854() {
                                   .isEqualTo(list(listCReverse, listAReverse));
   }
 
+  /**
+   * This test shows that we can't track all visited values, only the one with potential cycles.
+   * <p>
+   * Let's run it step by step with tracking all visited values:<br>
+   * list(listA, listB) vs list(listAReverse, listBReverse) means trying to find<br>
+   * - listA in list(listAReverse, listBReverse) and then listB
+   * <p>
+   * After comparing possible pairs (listA element, listAReverse element) we conclude that listA matches listAReverse<br>
+   * - here are the pairs (1, 2), (1, 1), (2, 2), we add them to the visited ones<br>
+   * <p>
+   * We now try to find listB in list(listBReverse) - listAReverse must not be taken into account as it had already been matched<br>
+   * - we would like to try (1, 2), (1, 1), (2, 2) but they have already been visited so we skip them<br>
+   * at this point, we know listB won't be found because (1, 1), (2, 2) won't be considered.
+   * <p>
+   * Comparing dualValues actual and expected with == does not solve the issue because Java does not always create different objects
+   * for primitive wrapping the same basic value, i.e. {@code new Integer(1) == new Integer(1)}.
+   * <p>
+   * The solution is to avoid adding all pairs to visited values. <br>
+   * Visited values are here to track cycles, a pair of wrapped primitive types can't cycle back to itself, we thus can and must ignore them.
+   * <p>
+   * For good measure we don't track pair that include any java.lang values.
+   * <p>
+   * If listA and listB contained non wrapped basic types then == is enough to differentiate them.
+   */
+  @Test
+  public void should_fix_1854_minimal_test() {
+    // GIVEN
+    List<Integer> listA = list(1, 2);
+    List<Integer> listB = list(1, 2);
+    // Reversed lists
+    List<Integer> listAReverse = list(2, 1);
+    List<Integer> listBReverse = list(2, 1);
+    // WHEN - THEN
+    assertThat(list(listA, listB)).usingRecursiveComparison()
+                                  .ignoringCollectionOrder()
+                                  .isEqualTo(list(listAReverse, listBReverse));
+
+  }
+
+  @Test
+  public void should_fix_1854_with_non_wrapped_basic_types() {
+    // GIVEN
+    FriendlyPerson p1 = friend("Sherlock Holmes");
+    FriendlyPerson p2 = friend("Watson");
+    FriendlyPerson p3 = friend("Sherlock Holmes");
+    FriendlyPerson p4 = friend("Watson");
+    List<FriendlyPerson> listA = list(p1, p2);
+    List<FriendlyPerson> listB = list(p1, p2);
+    // Reversed lists
+    List<FriendlyPerson> listAReverse = list(p4, p3);
+    List<FriendlyPerson> listBReverse = list(p4, p3);
+    // WHEN - THEN
+    assertThat(list(listA, listB)).usingRecursiveComparison()
+                                  .ignoringCollectionOrder()
+                                  .isEqualTo(list(listAReverse, listBReverse));
+
+  }
+
 }