From d7afd8f9c3dd23433656663dbcf536b8a898f5d1 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Fri, 1 Apr 2022 23:37:22 +0200 Subject: [PATCH] Fix immutable NormalizedNode equality Equality has always been iffy in our immutable implementations, but we are now making a decent attempt to implement it. There are three gaps in that effort, where we only consider known subclasses for comparison of value part of NormalizedNode. Address this by providing fallback child comparison and add an explicit test suite. JIRA: YANGTOOLS-1417 Change-Id: I36e5bcfc6da06c789595b00b7ad86d973cb34703 Signed-off-by: Robert Varga --- .../impl/ImmutableLeafSetNodeBuilder.java | 13 +- .../impl/ImmutableUserLeafSetNodeBuilder.java | 6 +- .../AbstractImmutableDataContainerNode.java | 14 +- .../data/impl/schema/nodes/YT1417Test.java | 167 ++++++++++++++++++ 4 files changed, 196 insertions(+), 4 deletions(-) create mode 100644 data/yang-data-impl/src/test/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/YT1417Test.java diff --git a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableLeafSetNodeBuilder.java b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableLeafSetNodeBuilder.java index d034b7f942..057fcc0187 100644 --- a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableLeafSetNodeBuilder.java +++ b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableLeafSetNodeBuilder.java @@ -161,7 +161,18 @@ public class ImmutableLeafSetNodeBuilder implements ListNodeBuilder other) { - return children.equals(((ImmutableLeafSetNode) other).children); + if (other instanceof ImmutableLeafSetNode) { + return children.equals(((ImmutableLeafSetNode) other).children); + } + if (size() != other.size()) { + return false; + } + for (var child : children.values()) { + if (!child.equals(other.childByArg(child.getIdentifier()))) { + return false; + } + } + return true; } } } diff --git a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableUserLeafSetNodeBuilder.java b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableUserLeafSetNodeBuilder.java index 403722c419..017d822d6e 100644 --- a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableUserLeafSetNodeBuilder.java +++ b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableUserLeafSetNodeBuilder.java @@ -159,7 +159,11 @@ public class ImmutableUserLeafSetNodeBuilder implements ListNodeBuilder other) { - return children.equals(((ImmutableUserLeafSetNode) other).children); + if (other instanceof ImmutableUserLeafSetNode) { + return children.equals(((ImmutableUserLeafSetNode) other).children); + } + // Note: performs a size() check first + return Iterables.elementsEqual(children.values(), other.body()); } private Map> getChildren() { diff --git a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableDataContainerNode.java b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableDataContainerNode.java index 3d3556e1dc..8490d827d7 100644 --- a/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableDataContainerNode.java +++ b/data/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableDataContainerNode.java @@ -59,7 +59,17 @@ public abstract class AbstractImmutableDataContainerNode && children.equals( - ((AbstractImmutableDataContainerNode) other).children); + if (other instanceof AbstractImmutableDataContainerNode) { + return children.equals(((AbstractImmutableDataContainerNode) other).children); + } + if (size() != other.size()) { + return false; + } + for (var child : body()) { + if (!child.equals(other.childByArg(child.getIdentifier()))) { + return false; + } + } + return true; } } diff --git a/data/yang-data-impl/src/test/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/YT1417Test.java b/data/yang-data-impl/src/test/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/YT1417Test.java new file mode 100644 index 0000000000..797942ec91 --- /dev/null +++ b/data/yang-data-impl/src/test/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/YT1417Test.java @@ -0,0 +1,167 @@ +/* + * Copyright (c) 2022 PANTHEON.tech, s.r.o. and others. All rights reserved. + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Public License v1.0 which accompanies this distribution, + * and is available at http://www.eclipse.org/legal/epl-v10.html + */ +package org.opendaylight.yangtools.yang.data.impl.schema.nodes; + +import static org.junit.Assert.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.mockito.Mockito.doReturn; +import static org.mockito.Mockito.mock; + +import java.util.List; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.junit.MockitoJUnitRunner; +import org.opendaylight.yangtools.yang.common.QName; +import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifier; +import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeWithValue; +import org.opendaylight.yangtools.yang.data.api.schema.ContainerNode; +import org.opendaylight.yangtools.yang.data.api.schema.SystemLeafSetNode; +import org.opendaylight.yangtools.yang.data.api.schema.UserLeafSetNode; +import org.opendaylight.yangtools.yang.data.impl.schema.Builders; +import org.opendaylight.yangtools.yang.data.impl.schema.ImmutableNodes; + +@RunWith(MockitoJUnitRunner.StrictStubs.class) +public class YT1417Test { + public static final QName FOO = QName.create("foo", "foo"); + public static final QName BAR = QName.create("foo", "bar"); + + @Test + public void testContainerNodeEquality() { + final var mock = mock(ContainerNode.class); + doReturn(new NodeIdentifier(FOO)).when(mock).getIdentifier(); + doReturn(1).when(mock).size(); + doReturn(ImmutableNodes.leafNode(BAR, "abc")).when(mock).childByArg(new NodeIdentifier(BAR)); + + assertEquals(Builders.containerBuilder() + .withNodeIdentifier(new NodeIdentifier(FOO)) + .withChild(ImmutableNodes.leafNode(BAR, "abc")) + .build(), mock); + + // Mismatched identifier + assertNotEquals(Builders.containerBuilder() + .withNodeIdentifier(new NodeIdentifier(BAR)) + .withChild(ImmutableNodes.leafNode(BAR, "abc")) + .build(), mock); + + // Mismatched child size + assertNotEquals(Builders.containerBuilder() + .withNodeIdentifier(new NodeIdentifier(FOO)) + .withChild(ImmutableNodes.leafNode(FOO, "abc")) + .withChild(ImmutableNodes.leafNode(BAR, "abc")) + .build(), mock); + + // Mismatched child + assertNotEquals(Builders.containerBuilder() + .withNodeIdentifier(new NodeIdentifier(FOO)) + .withChild(ImmutableNodes.leafNode(FOO, "abc")) + .build(), mock); + } + + @Test + public void testSystemLeafSetNodeEquality() { + final var mock = mock(SystemLeafSetNode.class); + doReturn(new NodeIdentifier(FOO)).when(mock).getIdentifier(); + doReturn(1).when(mock).size(); + doReturn(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(FOO, "abc")) + .withValue("abc") + .build()).when(mock).childByArg(new NodeWithValue<>(FOO, "abc")); + + assertEquals(Builders.leafSetBuilder() + .withNodeIdentifier(new NodeIdentifier(FOO)) + .withChild(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(FOO, "abc")) + .withValue("abc") + .build()) + .build(), mock); + + // Mismatched identifier + assertNotEquals(Builders.leafSetBuilder() + .withNodeIdentifier(new NodeIdentifier(BAR)) + .withChild(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(BAR, "abc")) + .withValue("abc") + .build()) + .build(), mock); + + // Mismatched child size + assertNotEquals(Builders.leafSetBuilder() + .withNodeIdentifier(new NodeIdentifier(FOO)) + .withChild(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(FOO, "abc")) + .withValue("abc") + .build()) + .withChild(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(FOO, "def")) + .withValue("def") + .build()) + .build(), mock); + + // Mismatched child + assertNotEquals(Builders.leafSetBuilder() + .withNodeIdentifier(new NodeIdentifier(FOO)) + .withChild(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(FOO, "def")) + .withValue("def") + .build()) + .build(), mock); + } + + @Test + public void testUserLeafSetNodeEquality() { + final var mock = mock(UserLeafSetNode.class); + doReturn(new NodeIdentifier(FOO)).when(mock).getIdentifier(); + doReturn(List.of( + Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(FOO, "abc")) + .withValue("abc") + .build(), + Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(FOO, "def")) + .withValue("def") + .build())).when(mock).body(); + + assertEquals(Builders.orderedLeafSetBuilder() + .withNodeIdentifier(new NodeIdentifier(FOO)) + .withChild(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(FOO, "abc")) + .withValue("abc") + .build()) + .withChild(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(FOO, "def")) + .withValue("def") + .build()) + .build(), mock); + + // Mismatched identifier + assertNotEquals(Builders.orderedLeafSetBuilder() + .withNodeIdentifier(new NodeIdentifier(BAR)) + .withChild(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(BAR, "abc")) + .withValue("abc") + .build()) + .withChild(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(BAR, "def")) + .withValue("def") + .build()) + .build(), mock); + + // Mismatched child order + assertNotEquals(Builders.orderedLeafSetBuilder() + .withNodeIdentifier(new NodeIdentifier(FOO)) + .withChild(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(FOO, "def")) + .withValue("def") + .build()) + .withChild(Builders.leafSetEntryBuilder() + .withNodeIdentifier(new NodeWithValue<>(FOO, "abc")) + .withValue("abc") + .build()) + .build(), mock); + } +} -- 2.36.6