MDSAL-298: properly handle unkeyed lists 62/67662/4
authorRobert Varga <robert.varga@pantheon.tech>
Mon, 29 Jan 2018 13:22:35 +0000 (14:22 +0100)
committerRobert Varga <robert.varga@pantheon.tech>
Mon, 5 Feb 2018 12:08:54 +0000 (13:08 +0100)
Unkeyed lists are not representable in binding, which means they
cannot be reported as modified children, either. This has implications
for any data change which contains unrepresentable fields as child
modifications.

Previously we would report SUBTREE_MODIFIED for the container node,
but would fail to report any children, which is obviously wrong, as the
user is left guessing as to what exactly happened.

This patch modifies LazyDataObjectModification to report a WRITE event
if modifications to unrepresentable children are found in SUBTREE_MODIFIED
case.

It also reuses BindingStructuralType from md-sal, so that we do not have
two copies of this critical class lying around requiring maintenance.

Change-Id: I30599bc26d3c993c5b6c5e2f9e6f66144a530cb5
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
opendaylight/md-sal/sal-binding-broker/pom.xml
opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingStructuralType.java [deleted file]
opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/LazyDataObjectModification.java

index 20c56be..10e6fa6 100644 (file)
       <groupId>org.opendaylight.mdsal</groupId>
       <artifactId>mdsal-binding-dom-codec</artifactId>
     </dependency>
+    <dependency>
+      <groupId>org.opendaylight.mdsal</groupId>
+      <artifactId>mdsal-binding-dom-adapter</artifactId>
+    </dependency>
     <dependency>
       <groupId>org.opendaylight.yangtools</groupId>
       <artifactId>yang-data-impl</artifactId>
diff --git a/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingStructuralType.java b/opendaylight/md-sal/sal-binding-broker/src/main/java/org/opendaylight/controller/md/sal/binding/impl/BindingStructuralType.java
deleted file mode 100644 (file)
index 64d98ed..0000000
+++ /dev/null
@@ -1,133 +0,0 @@
-/*
- * Copyright (c) 2015 Cisco Systems, Inc. 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.controller.md.sal.binding.impl;
-
-import java.util.Optional;
-import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.AugmentationIdentifier;
-import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifierWithPredicates;
-import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeWithValue;
-import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument;
-import org.opendaylight.yangtools.yang.data.api.schema.AnyXmlNode;
-import org.opendaylight.yangtools.yang.data.api.schema.AugmentationNode;
-import org.opendaylight.yangtools.yang.data.api.schema.ChoiceNode;
-import org.opendaylight.yangtools.yang.data.api.schema.ContainerNode;
-import org.opendaylight.yangtools.yang.data.api.schema.LeafNode;
-import org.opendaylight.yangtools.yang.data.api.schema.LeafSetEntryNode;
-import org.opendaylight.yangtools.yang.data.api.schema.LeafSetNode;
-import org.opendaylight.yangtools.yang.data.api.schema.MapEntryNode;
-import org.opendaylight.yangtools.yang.data.api.schema.MapNode;
-import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
-import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeCandidateNode;
-
-/**
- *
- * Defines structural mapping of Normalized Node to Binding data
- * addressable by Instance Identifier.
- *
- * Not all binding data are addressable by instance identifier
- * and there are some differences.
- *
- * See {@link #NOT_ADDRESSABLE},{@link #INVISIBLE_CONTAINER},{@link #VISIBLE_CONTAINER}
- * for more details.
- *
- *
- */
-enum BindingStructuralType {
-
-    /**
-     * DOM Item is not addressable in Binding Instance Identifier,
-     * data is not lost, but are available only via parent object.
-     *
-     * Such types of data are leaf-lists, leafs, list without keys
-     * or anyxml.
-     *
-     */
-    NOT_ADDRESSABLE,
-    /**
-     * Data container is addressable in NormalizedNode format,
-     * but in Binding it is not represented in Instance Identifier.
-     *
-     * This are choice / case nodes.
-     *
-     * This data is still accessible using parent object and their
-     * children are addressable.
-     *
-     */
-    INVISIBLE_CONTAINER,
-    /**
-     * Data container is addressable in NormalizedNode format,
-     * but in Binding it is not represented in Instance Identifier.
-     *
-     * This are list nodes.
-     *
-     * This data is still accessible using parent object and their
-     * children are addressable.
-     *
-     */
-    INVISIBLE_LIST,
-    /**
-     * Data container is addressable in Binding Instance Identifier format
-     * and also YangInstanceIdentifier format.
-     *
-     */
-    VISIBLE_CONTAINER,
-    /**
-     * Mapping algorithm was unable to detect type or was not updated after introduction
-     * of new NormalizedNode type.
-     */
-    UNKNOWN;
-
-    static BindingStructuralType from(final DataTreeCandidateNode domChildNode) {
-        Optional<NormalizedNode<?, ?>> dataBased = domChildNode.getDataAfter();
-        if (!dataBased.isPresent()) {
-            dataBased = domChildNode.getDataBefore();
-        }
-        if (dataBased.isPresent()) {
-            return from(dataBased.get());
-        }
-        return from(domChildNode.getIdentifier());
-    }
-
-    private static BindingStructuralType from(final PathArgument identifier) {
-        if(identifier instanceof NodeIdentifierWithPredicates || identifier instanceof AugmentationIdentifier) {
-            return VISIBLE_CONTAINER;
-        }
-        if(identifier instanceof NodeWithValue) {
-            return NOT_ADDRESSABLE;
-        }
-        return UNKNOWN;
-    }
-
-    static BindingStructuralType from(final NormalizedNode<?, ?> data) {
-        if(isNotAddressable(data)) {
-            return NOT_ADDRESSABLE;
-        }
-        if(data instanceof MapNode) {
-            return INVISIBLE_LIST;
-        }
-        if(data instanceof ChoiceNode) {
-            return INVISIBLE_CONTAINER;
-        }
-        if(isVisibleContainer(data)) {
-            return VISIBLE_CONTAINER;
-        }
-        return UNKNOWN;
-    }
-
-    private static boolean isVisibleContainer(final NormalizedNode<?, ?> data) {
-        return data instanceof MapEntryNode || data instanceof ContainerNode || data instanceof AugmentationNode;
-    }
-
-    private static boolean isNotAddressable(final NormalizedNode<?, ?> d) {
-        return d instanceof LeafNode
-                || d instanceof AnyXmlNode
-                || d instanceof LeafSetNode
-                || d instanceof LeafSetEntryNode;
-    }
-
-}
index e92467d..525ea4e 100644 (file)
@@ -14,6 +14,7 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Optional;
 import org.opendaylight.controller.md.sal.binding.api.DataObjectModification;
+import org.opendaylight.mdsal.binding.dom.adapter.BindingStructuralType;
 import org.opendaylight.mdsal.binding.dom.codec.api.BindingCodecTreeNode;
 import org.opendaylight.yangtools.yang.binding.Augmentation;
 import org.opendaylight.yangtools.yang.binding.ChildOf;
@@ -44,7 +45,9 @@ final class LazyDataObjectModification<T extends DataObject> implements DataObje
     private final BindingCodecTreeNode<T> codec;
     private final DataTreeCandidateNode domData;
     private final PathArgument identifier;
-    private Collection<DataObjectModification<? extends DataObject>> childNodesCache;
+
+    private volatile Collection<DataObjectModification<? extends DataObject>> childNodesCache;
+    private volatile ModificationType modificationType;
 
     private LazyDataObjectModification(final BindingCodecTreeNode<T> codec, final DataTreeCandidateNode domData) {
         this.codec = Preconditions.checkNotNull(codec);
@@ -136,28 +139,68 @@ final class LazyDataObjectModification<T extends DataObject> implements DataObje
     }
 
     @Override
-    public DataObjectModification.ModificationType getModificationType() {
-        switch(domData.getModificationType()) {
+    public ModificationType getModificationType() {
+        ModificationType localType = modificationType;
+        if (localType != null) {
+            return localType;
+        }
+
+        switch (domData.getModificationType()) {
             case APPEARED:
             case WRITE:
-                return DataObjectModification.ModificationType.WRITE;
-            case SUBTREE_MODIFIED:
-                return DataObjectModification.ModificationType.SUBTREE_MODIFIED;
+                localType = ModificationType.WRITE;
+                break;
             case DISAPPEARED:
             case DELETE:
-                return DataObjectModification.ModificationType.DELETE;
+                localType = ModificationType.DELETE;
+                break;
+            case SUBTREE_MODIFIED:
+                localType = resolveSubtreeModificationType();
+                break;
             default:
                 // TODO: Should we lie about modification type instead of exception?
                 throw new IllegalStateException("Unsupported DOM Modification type " + domData.getModificationType());
         }
+
+        modificationType = localType;
+        return localType;
+    }
+
+    private ModificationType resolveSubtreeModificationType() {
+        switch (codec.getChildAddressabilitySummary()) {
+            case ADDRESSABLE:
+                // All children are addressable, it is safe to report SUBTREE_MODIFIED
+                return ModificationType.SUBTREE_MODIFIED;
+            case UNADDRESSABLE:
+                // All children are non-addressable, report WRITE
+                return ModificationType.WRITE;
+            case MIXED:
+                // This case is not completely trivial, as we may have NOT_ADDRESSABLE nodes underneath us. If that
+                // is the case, we need to turn this modification into a WRITE operation, so that the user is able
+                // to observe those nodes being introduced. This is not efficient, but unfortunately unavoidable,
+                // as we cannot accurately represent such changes.
+                for (DataTreeCandidateNode child : domData.getChildNodes()) {
+                    if (BindingStructuralType.recursiveFrom(child) == BindingStructuralType.NOT_ADDRESSABLE) {
+                        // We have a non-addressable child, turn this modification into a write
+                        return ModificationType.WRITE;
+                    }
+                }
+
+                // No unaddressable children found, proceed in addressed mode
+                return ModificationType.SUBTREE_MODIFIED;
+            default:
+                throw new IllegalStateException("Unsupported child addressability summary "
+                        + codec.getChildAddressabilitySummary());
+        }
     }
 
     @Override
     public Collection<DataObjectModification<? extends DataObject>> getModifiedChildren() {
-        if (childNodesCache == null) {
-            childNodesCache = from(codec, domData.getChildNodes());
+        Collection<DataObjectModification<? extends DataObject>> local = childNodesCache;
+        if (local == null) {
+            childNodesCache = local = from(codec, domData.getChildNodes());
         }
-        return childNodesCache;
+        return local;
     }
 
     @Override

©2013 OpenDaylight, A Linux Foundation Collaborative Project. All Rights Reserved.
OpenDaylight is a registered trademark of The OpenDaylight Project, Inc.
Linux Foundation and OpenDaylight are registered trademarks of the Linux Foundation.
Linux is a registered trademark of Linus Torvalds.