From 0d71cb8296652be2da49b50d0ac38e376abb87b2 Mon Sep 17 00:00:00 2001
From: Robert Varga <rovarga@cisco.com>
Date: Wed, 10 Sep 2014 16:22:03 +0200
Subject: [PATCH] BUG-1790: detect existing unmodifiableMap encapsulation

Commit 9bcc82d107075da35c8cd01422481eaea395ba29 introduced a way to
reuse children map by builders. Unfortunately there is a codepath which
could see us receiving the returned collection in our constructor, and
the subsequent operation would wrap it again. If you stack these
operations, at some point the nesting becomes unmanageable.

The fix is to detect a previously-encapsulated collection and reusing
it. Analysis also shows that the children field is not needed, so remove
it as well.

Change-Id: I42d604d290326afca4564565760cbcf4586c2908
Signed-off-by: Robert Varga <rovarga@cisco.com>
---
 .../AbstractImmutableDataContainerNode.java   | 41 +++++++++++++++----
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableDataContainerNode.java b/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableDataContainerNode.java
index cd43988642..01bfe32b28 100644
--- a/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableDataContainerNode.java
+++ b/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/nodes/AbstractImmutableDataContainerNode.java
@@ -16,18 +16,41 @@ import org.opendaylight.yangtools.concepts.Immutable;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument;
 import org.opendaylight.yangtools.yang.data.api.schema.DataContainerChild;
 import org.opendaylight.yangtools.yang.data.api.schema.DataContainerNode;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
-public abstract class AbstractImmutableDataContainerNode<K extends PathArgument> extends AbstractImmutableNormalizedNode<K, Iterable<DataContainerChild<? extends PathArgument, ?>>>
-implements Immutable, DataContainerNode<K> {
-
-    protected final Map<PathArgument, DataContainerChild<? extends PathArgument, ?>> children;
-    private final Map<PathArgument, DataContainerChild<? extends PathArgument, ?>> publicChildren;
+public abstract class AbstractImmutableDataContainerNode<K extends PathArgument> extends AbstractImmutableNormalizedNode<K, Iterable<DataContainerChild<? extends PathArgument, ?>>> implements Immutable, DataContainerNode<K> {
+    private static final Logger LOG = LoggerFactory.getLogger(AbstractImmutableDataContainerNode.class);
+    private final Map<PathArgument, DataContainerChild<? extends PathArgument, ?>> children;
 
     public AbstractImmutableDataContainerNode(
             final Map<PathArgument, DataContainerChild<? extends PathArgument, ?>> children, final K nodeIdentifier) {
         super(nodeIdentifier);
-        this.children = children;
-        this.publicChildren = Collections.unmodifiableMap(children);
+
+        /*
+         * There is a code path where AbstractImmutableDataContainerNodeBuilder can reflect
+         * the collection acquired via getChildren() back to us. This is typically the case
+         * in the datastore where transactions cancel each other out, leaving an unmodified
+         * node. In that case we want to skip wrapping the map again (and again and again).
+         *
+         * In a perfect world, Collection.unmodifiableMap() would be doing the instanceof
+         * check which would stop the proliferation. Unfortunately this not the case and the
+         * 'unmodifiable' trait is not exposed by anything we can query. Furthermore the API
+         * contract there is sufficiently vague so an implementation may actually return a
+         * different implementation based on input map -- for example
+         * Collections.unmodifiableMap(Collections.emptyMap()) returning the same thing as
+         * Collections.emptyMap().
+         *
+         * This means that we have to perform the instantiation here (as opposed to once at
+         * class load time) and then compare the classes.
+         */
+        final Map<PathArgument, DataContainerChild<? extends PathArgument, ?>> pub = Collections.unmodifiableMap(children);
+        if (children.getClass().equals(pub.getClass())) {
+            LOG.trace("Reusing already-unmodifiable children {}", children);
+            this.children = children;
+        } else {
+            this.children = pub;
+        }
     }
 
     @Override
@@ -37,7 +60,7 @@ implements Immutable, DataContainerNode<K> {
 
     @Override
     public final Iterable<DataContainerChild<? extends PathArgument, ?>> getValue() {
-        return publicChildren.values();
+        return children.values();
     }
 
     @Override
@@ -46,7 +69,7 @@ implements Immutable, DataContainerNode<K> {
     }
 
     public final Map<PathArgument, DataContainerChild<? extends PathArgument, ?>> getChildren() {
-        return publicChildren;
+        return children;
     }
 
     @Override
-- 
2.36.6