BUG-648: Fixup hashCode/equals 71/5871/6
authorRobert Varga <rovarga@cisco.com>
Wed, 2 Apr 2014 20:15:56 +0000 (22:15 +0200)
committerRobert Varga <rovarga@cisco.com>
Mon, 7 Apr 2014 15:50:55 +0000 (17:50 +0200)
The baseline hashCode/equals implementation relied on the copied maps
for correctness. Iterable<?> in and of itself does not really have a
equals contract, so let's reimplement it and make it very explicit.

This patch is required for getting rid of the copied maps without
breaking users.

Change-Id: I2fcd2bc8775b60cf89bf03a5679f97a266cf1c1e
Signed-off-by: Robert Varga <rovarga@cisco.com>
yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableLeafNodeBuilder.java
yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableLeafSetEntryNodeBuilder.java
yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableLeafSetNodeBuilder.java
yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableMapEntryNodeBuilder.java
yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/builder/impl/ImmutableMapNodeBuilder.java
yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableDataContainerAttrNode.java
yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableDataContainerNode.java
yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableNormalizedAttrNode.java
yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableNormalizedNode.java

index deab671968b1ecedbbb679db20d7c3e1d6de10df..2ce6060f453e27e12c2ad235361d5cd2d0dbd948 100644 (file)
@@ -10,6 +10,7 @@ package org.opendaylight.yangtools.yang.data.impl.schema.builder.impl;
 import java.util.Map;
 
 import org.opendaylight.yangtools.yang.common.QName;
+import org.opendaylight.yangtools.yang.data.api.AttributesContainer;
 import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.schema.LeafNode;
 import org.opendaylight.yangtools.yang.data.impl.schema.builder.api.NormalizedNodeAttrBuilder;
@@ -28,9 +29,8 @@ public class ImmutableLeafNodeBuilder<T> extends AbstractImmutableNormalizedNode
 
     static final class ImmutableLeafNode<T> extends AbstractImmutableNormalizedAttrNode<InstanceIdentifier.NodeIdentifier, T> implements LeafNode<T> {
 
-        ImmutableLeafNode(InstanceIdentifier.NodeIdentifier nodeIdentifier, T value, Map<QName, String> attributes) {
+        ImmutableLeafNode(final InstanceIdentifier.NodeIdentifier nodeIdentifier, final T value, final Map<QName, String> attributes) {
             super(nodeIdentifier, value, attributes);
         }
-
     }
 }
index f81a12f97df7dbda20b1ac437bb80c4b3d733213..6cef0b40bba83beb6976ef0c9799e22823c7d3c9 100644 (file)
@@ -10,6 +10,7 @@ package org.opendaylight.yangtools.yang.data.impl.schema.builder.impl;
 import java.util.Map;
 
 import org.opendaylight.yangtools.yang.common.QName;
+import org.opendaylight.yangtools.yang.data.api.AttributesContainer;
 import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.schema.LeafSetEntryNode;
 import org.opendaylight.yangtools.yang.data.impl.schema.builder.api.NormalizedNodeAttrBuilder;
@@ -30,11 +31,10 @@ public class ImmutableLeafSetEntryNodeBuilder<T> extends AbstractImmutableNormal
 
     static final class ImmutableLeafSetEntryNode<T> extends AbstractImmutableNormalizedAttrNode<InstanceIdentifier.NodeWithValue, T> implements LeafSetEntryNode<T> {
 
-        ImmutableLeafSetEntryNode(InstanceIdentifier.NodeWithValue nodeIdentifier, T value, Map<QName, String> attributes) {
+        ImmutableLeafSetEntryNode(final InstanceIdentifier.NodeWithValue nodeIdentifier, final T value, final Map<QName, String> attributes) {
             super(nodeIdentifier, value, attributes);
             Preconditions.checkArgument(nodeIdentifier.getValue().equals(value),
                     "Node identifier contains different value: %s than value itself: %s", nodeIdentifier, value);
         }
-
     }
 }
index e0f016f8f313dd36951c8b328e93a7c10bfaa96b..aa6dc1ed474b1f9efbc590705dceafe924fa980f 100644 (file)
@@ -24,6 +24,7 @@ import org.opendaylight.yangtools.yang.data.impl.schema.nodes.AbstractImmutableN
 
 import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 
 public class ImmutableLeafSetNodeBuilder<T> implements ListNodeBuilder<T, LeafSetEntryNode<T>> {
@@ -55,7 +56,7 @@ public class ImmutableLeafSetNodeBuilder<T> implements ListNodeBuilder<T, LeafSe
 
     @Override
     public ListNodeBuilder<T, LeafSetEntryNode<T>> withValue(final List<LeafSetEntryNode<T>> value) {
-        for (LeafSetEntryNode<T> leafSetEntry : value) {
+        for (final LeafSetEntryNode<T> leafSetEntry : value) {
             withChild(leafSetEntry);
         }
 
@@ -64,14 +65,14 @@ public class ImmutableLeafSetNodeBuilder<T> implements ListNodeBuilder<T, LeafSe
 
 
     @Override
-    public ListNodeBuilder<T, LeafSetEntryNode<T>> withChildValue(final T value, Map<QName, String> attributes) {
+    public ListNodeBuilder<T, LeafSetEntryNode<T>> withChildValue(final T value, final Map<QName, String> attributes) {
         return withChild(new ImmutableLeafSetEntryNodeBuilder.ImmutableLeafSetEntryNode<>(
                 new InstanceIdentifier.NodeWithValue(nodeIdentifier.getNodeType(), value), value, attributes));
 
     }
 
     @Override
-    public ListNodeBuilder<T, LeafSetEntryNode<T>> withChildValue(T value) {
+    public ListNodeBuilder<T, LeafSetEntryNode<T>> withChildValue(final T value) {
         return withChildValue(value, Collections.<QName,String>emptyMap());
     }
 
@@ -93,6 +94,15 @@ public class ImmutableLeafSetNodeBuilder<T> implements ListNodeBuilder<T, LeafSe
             return Optional.fromNullable(mappedChildren.get(child));
         }
 
+        @Override
+        protected int valueHashCode() {
+            return mappedChildren.hashCode();
+        }
+
+        @Override
+        protected boolean valueEquals(final AbstractImmutableNormalizedNode<?, ?> other) {
+            return mappedChildren.equals(((ImmutableLeafSetNode<?>) other).mappedChildren);
+        }
     }
 
     @Override
index 63d91fb3e38975359683350929fffcd2a0e7315a..0d493b3ccbc4c9145a68f1b15e8e3513a7870a50 100644 (file)
@@ -37,9 +37,9 @@ public class ImmutableMapEntryNodeBuilder
     // FIXME, find better solution than 2 maps (map from QName to Child ?)
 
     @Override
-    public DataContainerNodeAttrBuilder<InstanceIdentifier.NodeIdentifierWithPredicates, MapEntryNode> withValue(List<DataContainerChild<? extends InstanceIdentifier.PathArgument, ?>> value) {
-        for (DataContainerChild<? extends InstanceIdentifier.PathArgument, ?> childId : value) {
-            InstanceIdentifier.PathArgument identifier = childId.getIdentifier();
+    public DataContainerNodeAttrBuilder<InstanceIdentifier.NodeIdentifierWithPredicates, MapEntryNode> withValue(final List<DataContainerChild<? extends InstanceIdentifier.PathArgument, ?>> value) {
+        for (final DataContainerChild<? extends InstanceIdentifier.PathArgument, ?> childId : value) {
+            final InstanceIdentifier.PathArgument identifier = childId.getIdentifier();
 
             // Augmentation nodes cannot be keys, and do not have to be present in childrenQNamesToPaths map
             if(identifier instanceof InstanceIdentifier.AugmentationIdentifier) {
@@ -67,15 +67,15 @@ public class ImmutableMapEntryNodeBuilder
     }
 
     private void checkKeys() {
-        for (QName keyQName : nodeIdentifier.getKeyValues().keySet()) {
+        for (final QName keyQName : nodeIdentifier.getKeyValues().keySet()) {
 
-            InstanceIdentifier.PathArgument childNodePath = childrenQNamesToPaths.get(keyQName);
-            DataContainerChild<?, ?> childNode = value.get(childNodePath);
+            final InstanceIdentifier.PathArgument childNodePath = childrenQNamesToPaths.get(keyQName);
+            final DataContainerChild<?, ?> childNode = value.get(childNodePath);
 
             Preconditions.checkNotNull(childNode, "Key child node: %s, not present", keyQName);
 
-            Object actualValue = nodeIdentifier.getKeyValues().get(keyQName);
-            Object expectedValue = childNode.getValue();
+            final Object actualValue = nodeIdentifier.getKeyValues().get(keyQName);
+            final Object expectedValue = childNode.getValue();
             Preconditions.checkArgument(expectedValue.equals(actualValue),
                     "Key child node with unexpected value, is: %s, should be: %s", actualValue, expectedValue);
         }
index a4b3a493d9c5435c66b015f0120d231255dff403..683020db27aac586eb9a346f27cdca0932b65c2b 100644 (file)
@@ -19,6 +19,7 @@ import org.opendaylight.yangtools.yang.data.impl.schema.nodes.AbstractImmutableN
 
 import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Maps;
 
 public class ImmutableMapNodeBuilder
@@ -40,7 +41,7 @@ public class ImmutableMapNodeBuilder
     @Override
     public CollectionNodeBuilder<MapEntryNode, MapNode> withValue(final List<MapEntryNode> value) {
         // TODO replace or putAll ?
-        for (MapEntryNode mapEntryNode : value) {
+        for (final MapEntryNode mapEntryNode : value) {
             withChild(mapEntryNode);
         }
 
@@ -79,5 +80,14 @@ public class ImmutableMapNodeBuilder
             return Optional.fromNullable(mappedChildren.get(child));
         }
 
+        @Override
+        protected int valueHashCode() {
+            return mappedChildren.hashCode();
+        }
+
+        @Override
+        protected boolean valueEquals(final AbstractImmutableNormalizedNode<?, ?> other) {
+            return mappedChildren.equals(((ImmutableMapNode) other).mappedChildren);
+        }
     }
 }
index 68024d6ea292dc1c7635139dded6dc9e29f3a605..85d4a8ed7a2f5525c4212c494a6727a64bdd316d 100644 (file)
@@ -14,6 +14,8 @@ import org.opendaylight.yangtools.yang.data.api.AttributesContainer;
 import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.schema.DataContainerChild;
 
+import com.google.common.base.Objects.ToStringHelper;
+
 public abstract class AbstractImmutableDataContainerAttrNode<K extends InstanceIdentifier.PathArgument>
         extends AbstractImmutableDataContainerNode<K>
     implements AttributesContainer {
@@ -21,8 +23,8 @@ public abstract class AbstractImmutableDataContainerAttrNode<K extends InstanceI
     private final Map<QName, String> attributes;
 
     public AbstractImmutableDataContainerAttrNode(
-            Map<InstanceIdentifier.PathArgument, DataContainerChild<? extends InstanceIdentifier.PathArgument, ?>> children,
-            K nodeIdentifier, Map<QName, String> attributes) {
+            final Map<InstanceIdentifier.PathArgument, DataContainerChild<? extends InstanceIdentifier.PathArgument, ?>> children,
+            final K nodeIdentifier, final Map<QName, String> attributes) {
         super(children, nodeIdentifier);
         this.attributes = attributes;
     }
@@ -33,7 +35,35 @@ public abstract class AbstractImmutableDataContainerAttrNode<K extends InstanceI
     }
 
     @Override
-    public final Object getAttributeValue(QName value) {
+    public final Object getAttributeValue(final QName value) {
         return attributes.get(value);
     }
+
+    @Override
+    protected ToStringHelper addToStringAttributes(final ToStringHelper toStringHelper) {
+        return super.addToStringAttributes(toStringHelper).add("attributes", attributes);
+    }
+
+// FIXME: are attributes part of hashCode/equals?
+//    @Override
+//    protected int valueHashCode() {
+//        int result = super.valueHashCode();
+//        for (final Entry<?, ?> a : attributes.entrySet()) {
+//            result = 31 * result + a.hashCode();
+//        }
+//        return result;
+//    }
+
+ // FIXME: are attributes part of hashCode/equals?
+//    @Override
+//    protected boolean valueEquals(final NormalizedNode<?, ?> other) {
+//        if (!super.valueEquals(other)) {
+//            return false;
+//        }
+//        final Set<Entry<QName, String>> tas = getAttributes().entrySet();
+//        final Set<Entry<QName, String>> oas = container.getAttributes().entrySet();
+//
+//        return tas.containsAll(oas) && oas.containsAll(tas);
+//        return true;
+//    }
 }
index 020c364fe52967088c80eab05476dd7182b740f5..9f32daa9ae144004cea81e8a649abe379b501ce3 100644 (file)
@@ -9,14 +9,14 @@ package org.opendaylight.yangtools.yang.data.impl.schema.nodes;
 
 import java.util.Map;
 
-import com.google.common.collect.ImmutableSet;
 import org.opendaylight.yangtools.concepts.Immutable;
 import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier.PathArgument;
 import org.opendaylight.yangtools.yang.data.api.schema.DataContainerChild;
 import org.opendaylight.yangtools.yang.data.api.schema.DataContainerNode;
 
 import com.google.common.base.Optional;
-import com.google.common.collect.ImmutableList;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 
 public abstract class AbstractImmutableDataContainerNode<K extends PathArgument> //
         extends AbstractImmutableNormalizedNode<K, Iterable<DataContainerChild<? extends PathArgument, ?>>> //
@@ -36,4 +36,17 @@ public abstract class AbstractImmutableDataContainerNode<K extends PathArgument>
         return Optional.<DataContainerChild<? extends PathArgument, ?>> fromNullable(children.get(child));
     }
 
+    @Override
+    protected int valueHashCode() {
+        return children.hashCode();
+    }
+
+    @Override
+    protected boolean valueEquals(final AbstractImmutableNormalizedNode<?, ?> other) {
+        if (!(other instanceof AbstractImmutableDataContainerNode<?>)) {
+            return false;
+        }
+
+        return children.equals(((AbstractImmutableDataContainerNode<?>)other).children);
+    }
 }
index cb992ea61f8fb61bcfb4d27178f558bf4b127561..a08506934db8138054a8ffb0df3564d9aa74f330 100644 (file)
@@ -13,6 +13,7 @@ import org.opendaylight.yangtools.yang.common.QName;
 import org.opendaylight.yangtools.yang.data.api.AttributesContainer;
 import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier;
 
+import com.google.common.base.Objects.ToStringHelper;
 import com.google.common.collect.ImmutableMap;
 
 public abstract class AbstractImmutableNormalizedAttrNode<K extends InstanceIdentifier.PathArgument,V>
@@ -21,7 +22,7 @@ public abstract class AbstractImmutableNormalizedAttrNode<K extends InstanceIden
 
     private final Map<QName, String> attributes;
 
-    protected AbstractImmutableNormalizedAttrNode(K nodeIdentifier, V value, Map<QName, String> attributes) {
+    protected AbstractImmutableNormalizedAttrNode(final K nodeIdentifier, final V value, final Map<QName, String> attributes) {
         super(nodeIdentifier, value);
         this.attributes = ImmutableMap.copyOf(attributes);
     }
@@ -32,8 +33,36 @@ public abstract class AbstractImmutableNormalizedAttrNode<K extends InstanceIden
     }
 
     @Override
-    public final Object getAttributeValue(QName value) {
+    public final Object getAttributeValue(final QName value) {
         return attributes.get(value);
     }
 
+    @Override
+    protected ToStringHelper addToStringAttributes(final ToStringHelper toStringHelper) {
+        return super.addToStringAttributes(toStringHelper).add("attributes", attributes);
+    }
+
+    @Override
+    protected int valueHashCode() {
+        final int result = getValue().hashCode();
+// FIXME: are attributes part of hashCode/equals?
+//        for (final Entry<?, ?> a : attributes.entrySet()) {
+//            result = 31 * result + a.hashCode();
+//        }
+        return result;
+    }
+
+    @Override
+    protected boolean valueEquals(final AbstractImmutableNormalizedNode<?, ?> other) {
+        if (!getValue().equals(other.getValue())) {
+            return false;
+        }
+
+// FIXME: are attributes part of hashCode/equals?
+//        final Set<Entry<QName, String>> tas = getAttributes().entrySet();
+//        final Set<Entry<QName, String>> oas = container.getAttributes().entrySet();
+//
+//        return tas.containsAll(oas) && oas.containsAll(tas);
+        return true;
+    }
 }
index 96ff9e5f22676d49143a753712d95e2ae9080807..708f9225c2f29c640db725b4550b2d5386d2a6cf 100644 (file)
@@ -14,15 +14,16 @@ import org.opendaylight.yangtools.yang.data.api.InstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
 
 import com.google.common.base.Objects;
+import com.google.common.base.Objects.ToStringHelper;
 import com.google.common.base.Preconditions;
 
 public abstract class AbstractImmutableNormalizedNode<K extends InstanceIdentifier.PathArgument,V>
         implements NormalizedNode<K, V>, Immutable {
 
-    protected final K nodeIdentifier;
-    protected final V value;
+    private final K nodeIdentifier;
+    private final V value;
 
-    protected AbstractImmutableNormalizedNode(K nodeIdentifier, V value) {
+    protected AbstractImmutableNormalizedNode(final K nodeIdentifier, final V value) {
         this.nodeIdentifier = Preconditions.checkNotNull(nodeIdentifier, "nodeIdentifier");
         this.value = Preconditions.checkNotNull(value, "value");
     }
@@ -53,35 +54,46 @@ public abstract class AbstractImmutableNormalizedNode<K extends InstanceIdentifi
     }
 
     @Override
-    public final V setValue(V value) {
+    public final V setValue(final V value) {
         throw new UnsupportedOperationException("Immutable");
     }
 
     @Override
     public final String toString() {
-        return Objects.toStringHelper(this)
-                .add("nodeIdentifier", nodeIdentifier)
-                .add("value", value)
-                .toString();
+        return addToStringAttributes(Objects.toStringHelper(this)).toString();
     }
 
-    @Override
-    public boolean equals(Object o) {
-        if (this == o) return true;
-        if (!(o instanceof AbstractImmutableNormalizedNode)) return false;
-
-        AbstractImmutableNormalizedNode<?, ?> that = (AbstractImmutableNormalizedNode<?, ?>) o;
+    protected ToStringHelper addToStringAttributes(final ToStringHelper toStringHelper) {
+        return toStringHelper.add("nodeIdentifier", nodeIdentifier).add("value", getValue());
+    }
 
-        if (!nodeIdentifier.equals(that.nodeIdentifier)) return false;
-        if (!value.equals(that.value)) return false;
+    protected abstract boolean valueEquals(AbstractImmutableNormalizedNode<?, ?> other);
+    protected abstract int valueHashCode();
 
-        return true;
+    @Override
+    public final boolean equals(final Object obj) {
+        if (this == obj) {
+            return true;
+        }
+        if (obj == null) {
+            return false;
+        }
+        if (this.getClass() != obj.getClass()) {
+            return false;
+        }
+
+        final AbstractImmutableNormalizedNode<?, ?> other = (AbstractImmutableNormalizedNode<?, ?>)obj;
+        if (!nodeIdentifier.equals(other.nodeIdentifier)) {
+            return false;
+        }
+
+        return valueEquals(other);
     }
 
     @Override
-    public int hashCode() {
+    public final int hashCode() {
         int result = nodeIdentifier.hashCode();
-        result = 31 * result + value.hashCode();
+        result = 31 * result + valueHashCode();
         return result;
     }
 }