Fix AugmentationIdentifier.compareTo()

The implementation assumed stable Set iteration order, which was
not internally guaranteed, hence two AugmentationIdentifiers could
report as being equal via equals(), but still compare as having
different total ordering -- which could lead to strange results.

Fix this by doing some more work and compare them based on QName's
total ordering. The test suite is fixed up to use explicit iteration
order retained by ImmutableSet rather than relying on HashSet's
hashCode()-based iteration order non-determinism.

Also remove implicit boolean boxing by using assertTrue/assertFalse
in tests.

Change-Id: Iafd4ac8d7d54f0918b0579c6e6f8e76b35b56dd8
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit 77b8e16d3a2abfb368d2974a473154fe255f903e)

diff --git a/yang/yang-data-api/src/main/java/org/opendaylight/yangtools/yang/data/api/YangInstanceIdentifier.java b/yang/yang-data-api/src/main/java/org/opendaylight/yangtools/yang/data/api/YangInstanceIdentifier.java
index 561bc6ba98d401db43e8589e942ae29dc0b02c5b..392621936859bb79c302457fdd13f34492c2d706 100644 (file)
--- a/yang/yang-data-api/src/main/java/org/opendaylight/yangtools/yang/data/api/YangInstanceIdentifier.java
+++ b/yang/yang-data-api/src/main/java/org/opendaylight/yangtools/yang/data/api/YangInstanceIdentifier.java
@@ -7,6 +7,8 @@
  */
 package org.opendaylight.yangtools.yang.data.api;
 
+import static com.google.common.base.Verify.verify;
+
 import com.google.common.annotations.Beta;
 import com.google.common.base.Optional;
 import com.google.common.base.Preconditions;
@@ -15,8 +17,10 @@ import com.google.common.cache.CacheLoader;
 import com.google.common.cache.LoadingCache;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
+import com.google.common.collect.Sets;
 import java.io.Serializable;
 import java.lang.reflect.Array;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Collection;
 import java.util.Iterator;
@@ -716,14 +720,19 @@ public abstract class YangInstanceIdentifier implements Path<YangInstanceIdentif
             int thisSize = childNames.size();
             int otherSize = otherChildNames.size();
             if (thisSize == otherSize) {
-                Iterator<QName> otherIterator = otherChildNames.iterator();
-                for (QName name : childNames) {
-                    int child = name.compareTo(otherIterator.next());
-                    if (child != 0) {
-                        return child;
-                    }
+                // Quick Set-based comparison
+                if (childNames.equals(otherChildNames)) {
+                    return 0;
                 }
-                return 0;
+
+                // We already know the sets are not equal, but have equal size, hence the sets differ in their elements,
+                // but potentially share a common set of elements. The most consistent way of comparing them is using
+                // total ordering defined by QName's compareTo. Hence convert both sets to lists ordered
+                // by QName.compareTo() and decide on the first differing element.
+                final List<QName> diff = new ArrayList<>(Sets.symmetricDifference(childNames, otherChildNames));
+                verify(!diff.isEmpty(), "Augmentation identifiers %s and %s report no difference", this, o);
+                diff.sort(QName::compareTo);
+                return childNames.contains(diff.get(0)) ? -1 : 1;
             } else if (thisSize < otherSize) {
                 return 1;
             } else {

diff --git a/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/PathArgumentListTest.java b/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/PathArgumentListTest.java
index 49beab7a334aa7aa9c7fc00de5b36edb15e0e5e9..26b28216a27847725d91746f8c306bafb44fdc9b 100644 (file)
--- a/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/PathArgumentListTest.java
+++ b/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/PathArgumentListTest.java
@@ -114,7 +114,7 @@ public class PathArgumentListTest {
         assertTrue(yangInstanceIdentifier.pathArgumentsEqual(yangInstanceIdentifier));
         assertEquals(pathArgumentToRoot, stackedPathArguments.get(0));
         assertEquals(4, stackedPathArguments.size());
-        assertEquals(true, stackedPathArguments.contains(pathArgumentToRoot));
+        assertTrue(stackedPathArguments.contains(pathArgumentToRoot));
         assertEquals(0, stackedPathArguments.indexOf(pathArgumentToRoot));
         assertEquals(0, stackedPathArguments.lastIndexOf(pathArgumentToRoot));
 
@@ -125,7 +125,7 @@ public class PathArgumentListTest {
         assertEquals(qNameRoot, rootQname);
         assertEquals(qNameLeaf, leafQname);
         assertEquals(4, stackedReversePathArguments.size());
-        assertEquals(true, stackedReversePathArguments.contains(pathArgumentToRoot));
+        assertTrue(stackedReversePathArguments.contains(pathArgumentToRoot));
         assertEquals(3, stackedReversePathArguments.indexOf(pathArgumentToRoot));
         assertEquals(3, stackedReversePathArguments.lastIndexOf(pathArgumentToRoot));
 

diff --git a/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/YangInstanceIdentifierTest.java b/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/YangInstanceIdentifierTest.java
index 8a02660aa374aaeef6f6a278ce728625eafa6ab0..5cd6864e06750dd266d3f83278e7c54d34bdcdb0 100644 (file)
--- a/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/YangInstanceIdentifierTest.java
+++ b/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/YangInstanceIdentifierTest.java
@@ -10,7 +10,6 @@ package org.opendaylight.yangtools.yang.data.api;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
-import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertSame;
@@ -18,8 +17,8 @@ import static org.junit.Assert.assertTrue;
 
 import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Lists;
-import com.google.common.collect.Sets;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
@@ -81,12 +80,12 @@ public class YangInstanceIdentifierTest {
                 new NodeIdentifier(NODENAME1));
         final YangInstanceIdentifier id4 = YangInstanceIdentifier.create(new NodeIdentifier(NODENAME1));
 
-        assertEquals("equals", false, id1.equals(null));
-        assertEquals("equals", true, id1.equals(id1));
-        assertEquals("equals", true, id1.equals(id2));
-        assertEquals("equals", false, id1.equals(id3));
-        assertEquals("equals", false, id1.equals(id4));
-        assertEquals("equals", false, id1.equals(new Object()));
+        assertFalse("equals", id1.equals(null));
+        assertTrue("equals", id1.equals(id1));
+        assertTrue("equals", id1.equals(id2));
+        assertFalse("equals", id1.equals(id3));
+        assertFalse("equals", id1.equals(id4));
+        assertFalse("equals", id1.equals(new Object()));
     }
 
     @Test
@@ -133,7 +132,7 @@ public class YangInstanceIdentifierTest {
                     new NodeIdentifier(NODENAME1), new NodeIdentifier(NODENAME2)));
 
         Optional<YangInstanceIdentifier> relative = id1.relativeTo(id2);
-        assertEquals("isPresent", true, relative.isPresent());
+        assertTrue("isPresent", relative.isPresent());
 
         List<PathArgument> path = relative.get().getPathArguments();
         assertEquals("Path size", 2, path.size());
@@ -141,11 +140,11 @@ public class YangInstanceIdentifierTest {
         assertEquals("PathArg 2 node type", NODENAME4, path.get(1).getNodeType());
 
         relative = id2.relativeTo(id3);
-        assertEquals("isPresent", true, relative.isPresent());
+        assertTrue("isPresent", relative.isPresent());
         assertEquals("Path size", 0, relative.get().getPathArguments().size());
 
         relative = id2.relativeTo(id1);
-        assertEquals("isPresent", false, relative.isPresent());
+        assertFalse("isPresent", relative.isPresent());
     }
 
     @Test(expected = IllegalArgumentException.class)
@@ -166,10 +165,10 @@ public class YangInstanceIdentifierTest {
         final YangInstanceIdentifier id4 = YangInstanceIdentifier.create(new NodeIdentifier(NODENAME1),
                 new NodeIdentifier(NODENAME3));
 
-        assertEquals("contains", true, id2.contains(id1));
-        assertEquals("contains", true, id2.contains(id3));
-        assertEquals("contains", false, id1.contains(id2));
-        assertEquals("contains", false, id2.contains(id4));
+        assertTrue("contains", id2.contains(id1));
+        assertTrue("contains", id2.contains(id3));
+        assertFalse("contains", id1.contains(id2));
+        assertFalse("contains", id2.contains(id4));
     }
 
     @Test
@@ -238,45 +237,38 @@ public class YangInstanceIdentifierTest {
         NodeIdentifierWithPredicates node2 = new NodeIdentifierWithPredicates(NODENAME1, KEY1, "foo");
 
         assertEquals("hashCode", node1.hashCode(), node2.hashCode());
-        assertEquals("equals", true, node1.equals(node2));
+        assertTrue("equals", node1.equals(node2));
 
-        assertEquals("equals", false,
-                node1.equals(new NodeIdentifierWithPredicates(NODENAME2, KEY1, "foo")));
-        assertEquals("equals", false,
-                node1.equals(new NodeIdentifierWithPredicates(NODENAME1, KEY2, "foo")));
-        assertEquals("equals", false,
-                node1.equals(new NodeIdentifierWithPredicates(NODENAME1, KEY1, "bar")));
-        assertEquals("equals", false, node1.equals(new Object()));
+        assertFalse("equals", node1.equals(new NodeIdentifierWithPredicates(NODENAME2, KEY1, "foo")));
+        assertFalse("equals", node1.equals(new NodeIdentifierWithPredicates(NODENAME1, KEY2, "foo")));
+        assertFalse("equals", node1.equals(new NodeIdentifierWithPredicates(NODENAME1, KEY1, "bar")));
+        assertFalse("equals", node1.equals(new Object()));
 
         assertNotNull(node1.toString()); // for code coverage
         assertNotNull(node1.toRelativeString(node2));
 
         NodeIdentifierWithPredicates node3 = new NodeIdentifierWithPredicates(NODENAME1,
-                ImmutableMap.<QName, Object>builder().put(KEY1, 10).put(KEY2, 20).build());
+                ImmutableMap.of(KEY1, 10, KEY2, 20));
 
         NodeIdentifierWithPredicates node4 = new NodeIdentifierWithPredicates(NODENAME1,
-                ImmutableMap.<QName, Object>builder().put(KEY1, 10).put(KEY2, 20).build());
+                ImmutableMap.of(KEY1, 10, KEY2, 20));
 
         assertEquals("hashCode", node3.hashCode(), node4.hashCode());
-        assertEquals("equals", true, node3.equals(node4));
+        assertTrue("equals", node3.equals(node4));
 
-        assertEquals("equals", false, node3.equals(node1));
-        assertEquals("equals", false,
-                node1.equals(new NodeIdentifierWithPredicates(NODENAME1,
-                        ImmutableMap.<QName, Object>builder().put(KEY1, 10).put(KEY3, 20).build())));
+        assertFalse("equals", node3.equals(node1));
+        assertFalse("equals", node1.equals(new NodeIdentifierWithPredicates(NODENAME1,
+            ImmutableMap.of(KEY1, 10, KEY3, 20))));
 
         node1 = new NodeIdentifierWithPredicates(NODENAME1, KEY1, new byte[]{1,2});
         node2 = new NodeIdentifierWithPredicates(NODENAME1, KEY1, new byte[]{1,2});
 
         assertEquals("hashCode", node1.hashCode(), node2.hashCode());
-        assertEquals("equals", true, node1.equals(node2));
-
-        assertEquals("equals", false,
-                node1.equals(new NodeIdentifierWithPredicates(NODENAME1, KEY1, new byte[]{1,3})));
-        assertEquals("equals", false,
-                node1.equals(new NodeIdentifierWithPredicates(NODENAME1, KEY1, new byte[]{1})));
-        assertEquals("equals", false,
-                node1.equals(new NodeIdentifierWithPredicates(NODENAME1, KEY1, new byte[]{1,2,3})));
+        assertTrue("equals", node1.equals(node2));
+
+        assertFalse("equals", node1.equals(new NodeIdentifierWithPredicates(NODENAME1, KEY1, new byte[]{1,3})));
+        assertFalse("equals", node1.equals(new NodeIdentifierWithPredicates(NODENAME1, KEY1, new byte[]{1})));
+        assertFalse("equals", node1.equals(new NodeIdentifierWithPredicates(NODENAME1, KEY1, new byte[]{1,2,3})));
     }
 
     @Test
@@ -289,11 +281,11 @@ public class YangInstanceIdentifierTest {
         NodeWithValue<?> node2 = new NodeWithValue<>(NODENAME1, "foo");
 
         assertEquals("hashCode", node1.hashCode(), node2.hashCode());
-        assertEquals("equals", true, node1.equals(node2));
+        assertTrue("equals", node1.equals(node2));
 
-        assertEquals("equals", false, node1.equals(new NodeWithValue<>(NODENAME1, "bar")));
-        assertEquals("equals", false, node1.equals(new NodeWithValue<>(NODENAME2, "foo")));
-        assertEquals("equals", false, node1.equals(new Object()));
+        assertFalse("equals", node1.equals(new NodeWithValue<>(NODENAME1, "bar")));
+        assertFalse("equals", node1.equals(new NodeWithValue<>(NODENAME2, "foo")));
+        assertFalse("equals", node1.equals(new Object()));
 
         assertNotNull(node1.toString()); // for code coverage
         assertNotNull(node1.toRelativeString(node2));
@@ -302,10 +294,10 @@ public class YangInstanceIdentifierTest {
         NodeWithValue<?> node4 = new NodeWithValue<>(NODENAME1, new byte[]{1,2});
 
         assertEquals("hashCode", node3.hashCode(), node4.hashCode());
-        assertEquals("equals", true, node3.equals(node4));
+        assertTrue("equals", node3.equals(node4));
 
-        assertEquals("equals", false, node3.equals(new NodeWithValue<>(NODENAME1, new byte[]{1,3})));
-        assertEquals("equals", false, node3.equals(node1));
+        assertFalse("equals", node3.equals(new NodeWithValue<>(NODENAME1, new byte[]{1,3})));
+        assertFalse("equals", node3.equals(node1));
     }
 
     @Test
@@ -314,49 +306,49 @@ public class YangInstanceIdentifierTest {
         final NodeIdentifier node1 = new NodeIdentifier(NODENAME1);
         assertEquals("getNodeType", NODENAME1, node1.getNodeType());
         final NodeIdentifier node2 = new NodeIdentifier(NODENAME1);
-        final AugmentationIdentifier node3 = new AugmentationIdentifier(Sets.newHashSet(NODENAME1, NODENAME2));
+        final AugmentationIdentifier node3 = new AugmentationIdentifier(ImmutableSet.of(NODENAME1, NODENAME2));
 
         assertEquals("hashCode", node1.hashCode(), node2.hashCode());
         assertEquals("compareTo", 0, node1.compareTo(node2));
-        assertEquals("compareTo", true, node1.compareTo(new NodeIdentifier(NODENAME3)) != 0);
+        assertTrue("compareTo", node1.compareTo(new NodeIdentifier(NODENAME3)) != 0);
 
-        assertEquals("equals", false, node1.equals(null));
-        assertEquals("equals", false, node1.equals(node3));
-        assertEquals("equals", true, node1.equals(node1));
-        assertEquals("equals", true, node1.equals(node2));
-        assertEquals("equals", false, node1.equals(new NodeIdentifier(NODENAME3)));
-        assertEquals("equals", false, node1.equals(new Object()));
+        assertFalse("equals", node1.equals(null));
+        assertFalse("equals", node1.equals(node3));
+        assertTrue("equals", node1.equals(node1));
+        assertTrue("equals", node1.equals(node2));
+        assertFalse("equals", node1.equals(new NodeIdentifier(NODENAME3)));
+        assertFalse("equals", node1.equals(new Object()));
 
         assertNotNull(node1.toString()); // for code coverage
     }
 
     @Test(expected = UnsupportedOperationException.class)
     public void testAugmentationIdentifierNodeType() {
-        AugmentationIdentifier node1 = new AugmentationIdentifier(Sets.newHashSet(NODENAME1, NODENAME2));
+        AugmentationIdentifier node1 = new AugmentationIdentifier(ImmutableSet.of(NODENAME1, NODENAME2));
         node1.getNodeType();
     }
 
     @Test
     public void testAugmentationIdentifier() {
 
-        final AugmentationIdentifier node1 = new AugmentationIdentifier(Sets.newHashSet(NODENAME1, NODENAME2));
-        assertEquals("getPossibleChildNames", Sets.newHashSet(NODENAME1, NODENAME2), node1.getPossibleChildNames());
-        final AugmentationIdentifier node2 = new AugmentationIdentifier(Sets.newHashSet(NODENAME2, NODENAME1));
-        final AugmentationIdentifier node3 = new AugmentationIdentifier(Sets.newHashSet(NODENAME1, NODENAME3));
-        final AugmentationIdentifier node4 = new AugmentationIdentifier(Sets.newHashSet(NODENAME1, NODENAME2,
+        final AugmentationIdentifier node1 = new AugmentationIdentifier(ImmutableSet.of(NODENAME1, NODENAME2));
+        assertEquals("getPossibleChildNames", ImmutableSet.of(NODENAME1, NODENAME2), node1.getPossibleChildNames());
+        final AugmentationIdentifier node2 = new AugmentationIdentifier(ImmutableSet.of(NODENAME2, NODENAME1));
+        final AugmentationIdentifier node3 = new AugmentationIdentifier(ImmutableSet.of(NODENAME1, NODENAME3));
+        final AugmentationIdentifier node4 = new AugmentationIdentifier(ImmutableSet.of(NODENAME1, NODENAME2,
                     NODENAME3));
         final NodeIdentifier node5 = new NodeIdentifier(NODENAME3);
 
         assertEquals("hashCode", node1.hashCode(), node2.hashCode());
 
-        assertEquals("equals", true, node1.equals(node1));
-        assertEquals("equals", true, node1.equals(node2));
-        assertEquals("equals", false, node1.equals(node3));
-        assertEquals("equals", false, node1.equals(new AugmentationIdentifier(Sets.newHashSet(NODENAME1))));
-        assertEquals("equals", false, node1.equals(new Object()));
+        assertTrue("equals", node1.equals(node1));
+        assertTrue("equals", node1.equals(node2));
+        assertFalse("equals", node1.equals(node3));
+        assertFalse("equals", node1.equals(new AugmentationIdentifier(ImmutableSet.of(NODENAME1))));
+        assertFalse("equals", node1.equals(new Object()));
 
         assertEquals("compareTo", -1, node1.compareTo(node5));
-        assertNotEquals("compareTo", -1, node1.compareTo(node2));
+        assertEquals("compareTo", 0, node1.compareTo(node2));
         assertEquals("compareTo", 0, node1.compareTo(node2));
         assertEquals("compareTo", 1, node1.compareTo(node4));
         assertEquals("compareTo", -1, node4.compareTo(node1));

diff --git a/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/schema/tree/DataTreeCandidatesTest.java b/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/schema/tree/DataTreeCandidatesTest.java
index a42b6994750e99c0da189ff9e04817d6189f9065..f486edd04cf920835525fe4241e2084ea96f614d 100644 (file)
--- a/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/schema/tree/DataTreeCandidatesTest.java
+++ b/yang/yang-data-api/src/test/java/org/opendaylight/yangtools/yang/data/api/schema/tree/DataTreeCandidatesTest.java
@@ -86,7 +86,7 @@ public class DataTreeCandidatesTest {
         verify(mockedModification, times(1)).createCursor(any(YangInstanceIdentifier.class));
         verify(mockedCursor, times(1)).delete(any(PathArgument.class));
 
-        doReturn(true).when(mockedRootPath).isEmpty();
+        doReturn(Boolean.TRUE).when(mockedRootPath).isEmpty();
         try {
             DataTreeCandidates.applyToModification(mockedModification, mockedDataTreeCandidate);
             fail("An IllegalArgumentException should have been thrown!");