BUG-7868: perform checks before starting modifications

The codepath for makeNormalPost() performs sub-optimal emulation
of create().

This patch changes the logic to first check for presence and then
perform modifications. This saves cycles as continuous modify/read
cycles incur heavy penalty on DataTree and also allows batching
of modification requests.

Change-Id: Ic8e18ada094334948170f2a147da806db7db1a16
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
(cherry picked from commit 3583194dd53ca09383fee9e50299a36a622895a1)

diff --git a/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java b/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java
index 2d72b78faef07daf6fcd092409b4502ffc7114f5..39b9b1b93f8ad2caa2397c0acab4b1b2f8033b68 100644 (file)
--- a/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java
+++ b/restconf/sal-rest-connector/src/main/java/org/opendaylight/netconf/sal/restconf/impl/BrokerFacade.java
@@ -17,6 +17,7 @@ import com.google.common.util.concurrent.CheckedFuture;
 import com.google.common.util.concurrent.FutureCallback;
 import com.google.common.util.concurrent.Futures;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.Iterator;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
@@ -562,13 +563,13 @@ public class BrokerFacade {
                     Builders.containerBuilder((ContainerSchemaNode) baseSchemaNode);
             buildCont(builder, (ContainerNode) result, baseSchemaCtxTree, path, trim);
             return builder.build();
-        } else {
-            final DataContainerNodeAttrBuilder<NodeIdentifierWithPredicates, MapEntryNode> builder =
-                    Builders.mapEntryBuilder((ListSchemaNode) baseSchemaNode);
-            buildMapEntryBuilder(builder, (MapEntryNode) result, baseSchemaCtxTree, path, trim,
-                    ((ListSchemaNode) baseSchemaNode).getKeyDefinition());
-            return builder.build();
         }
+
+        final DataContainerNodeAttrBuilder<NodeIdentifierWithPredicates, MapEntryNode> builder =
+                Builders.mapEntryBuilder((ListSchemaNode) baseSchemaNode);
+        buildMapEntryBuilder(builder, (MapEntryNode) result, baseSchemaCtxTree, path, trim,
+            ((ListSchemaNode) baseSchemaNode).getKeyDefinition());
+        return builder.build();
     }
 
     private void buildMapEntryBuilder(final DataContainerNodeAttrBuilder<NodeIdentifierWithPredicates, MapEntryNode> builder,
@@ -690,91 +691,92 @@ public class BrokerFacade {
             final SchemaContext schemaContext, final String insert, final String point) {
         if (insert == null) {
             makeNormalPost(rWTransaction, datastore, path, payload, schemaContext);
-        } else {
-            final DataSchemaNode schemaNode = checkListAndOrderedType(schemaContext, path);
-            checkItemDoesNotExists(rWTransaction, datastore, path);
-            switch (insert) {
-                case "first":
-                    if(schemaNode instanceof ListSchemaNode){
-                        final OrderedMapNode readList =
-                                (OrderedMapNode) this.readConfigurationData(path.getParent().getParent());
-                        if (readList == null || readList.getValue().isEmpty()) {
-                            simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
-                        } else {
-                            rWTransaction.delete(datastore, path.getParent().getParent());
-                            simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
-                            makeNormalPost(rWTransaction, datastore, path.getParent().getParent(), readList,
-                                    schemaContext);
-                        }
+            return;
+        }
+
+        final DataSchemaNode schemaNode = checkListAndOrderedType(schemaContext, path);
+        checkItemDoesNotExists(rWTransaction, datastore, path);
+        switch (insert) {
+            case "first":
+                if(schemaNode instanceof ListSchemaNode){
+                    final OrderedMapNode readList =
+                            (OrderedMapNode) this.readConfigurationData(path.getParent().getParent());
+                    if (readList == null || readList.getValue().isEmpty()) {
+                        simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
                     } else {
-                        final OrderedLeafSetNode readLeafList =
-                                (OrderedLeafSetNode) readConfigurationData(path.getParent());
-                        if (readLeafList == null || readLeafList.getValue().isEmpty()) {
-                            simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
-                        } else {
-                            rWTransaction.delete(datastore, path.getParent());
-                            simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
-                            makeNormalPost(rWTransaction, datastore, path.getParent().getParent(), readLeafList,
-                                    schemaContext);
-                        }
+                        rWTransaction.delete(datastore, path.getParent().getParent());
+                        simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
+                        makeNormalPost(rWTransaction, datastore, path.getParent().getParent(), readList,
+                            schemaContext);
                     }
-                    break;
-                case "last":
-                    simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
-                    break;
-                case "before":
-                    if(schemaNode instanceof ListSchemaNode){
-                        final OrderedMapNode readList =
-                                (OrderedMapNode) this.readConfigurationData(path.getParent().getParent());
-                        if (readList == null || readList.getValue().isEmpty()) {
-                            simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
-                        } else {
-                            insertWithPointListPost(rWTransaction, datastore, path, payload, schemaContext, point,
-                                    readList,
-                                    true);
-                        }
+                } else {
+                    final OrderedLeafSetNode<?> readLeafList =
+                            (OrderedLeafSetNode<?>) readConfigurationData(path.getParent());
+                    if (readLeafList == null || readLeafList.getValue().isEmpty()) {
+                        simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
                     } else {
-                        final OrderedLeafSetNode<?> readLeafList =
-                                (OrderedLeafSetNode<?>) readConfigurationData(path.getParent());
-                        if (readLeafList == null || readLeafList.getValue().isEmpty()) {
-                            simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
-                        } else {
-                            insertWithPointLeafListPost(rWTransaction, datastore, path, payload, schemaContext, point,
-                                    readLeafList, true);
-                        }
+                        rWTransaction.delete(datastore, path.getParent());
+                        simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
+                        makeNormalPost(rWTransaction, datastore, path.getParent().getParent(), readLeafList,
+                            schemaContext);
                     }
-                    break;
-                case "after":
-                    if (schemaNode instanceof ListSchemaNode) {
-                        final OrderedMapNode readList =
-                                (OrderedMapNode) this.readConfigurationData(path.getParent().getParent());
-                        if (readList == null || readList.getValue().isEmpty()) {
-                            simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
-                        } else {
-                            insertWithPointListPost(rWTransaction, datastore, path, payload, schemaContext, point,
-                                    readList,
-                                    false);
-                        }
+                }
+                break;
+            case "last":
+                simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
+                break;
+            case "before":
+                if(schemaNode instanceof ListSchemaNode){
+                    final OrderedMapNode readList =
+                            (OrderedMapNode) this.readConfigurationData(path.getParent().getParent());
+                    if (readList == null || readList.getValue().isEmpty()) {
+                        simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
                     } else {
-                        final OrderedLeafSetNode<?> readLeafList =
-                                (OrderedLeafSetNode<?>) readConfigurationData(path.getParent());
-                        if (readLeafList == null || readLeafList.getValue().isEmpty()) {
-                            simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
-                        } else {
-                            insertWithPointLeafListPost(rWTransaction, datastore, path, payload, schemaContext, point,
-                                    readLeafList, false);
-                        }
+                        insertWithPointListPost(rWTransaction, datastore, path, payload, schemaContext, point,
+                            readList,
+                            true);
                     }
-                    break;
-                default:
-                    throw new RestconfDocumentedException(
-                            "Used bad value of insert parameter. Possible values are first, last, before or after, "
-                                    + "but was: " + insert);
-            }
+                } else {
+                    final OrderedLeafSetNode<?> readLeafList =
+                            (OrderedLeafSetNode<?>) readConfigurationData(path.getParent());
+                    if (readLeafList == null || readLeafList.getValue().isEmpty()) {
+                        simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
+                    } else {
+                        insertWithPointLeafListPost(rWTransaction, datastore, path, payload, schemaContext, point,
+                            readLeafList, true);
+                    }
+                }
+                break;
+            case "after":
+                if (schemaNode instanceof ListSchemaNode) {
+                    final OrderedMapNode readList =
+                            (OrderedMapNode) this.readConfigurationData(path.getParent().getParent());
+                    if (readList == null || readList.getValue().isEmpty()) {
+                        simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
+                    } else {
+                        insertWithPointListPost(rWTransaction, datastore, path, payload, schemaContext, point,
+                            readList,
+                            false);
+                    }
+                } else {
+                    final OrderedLeafSetNode<?> readLeafList =
+                            (OrderedLeafSetNode<?>) readConfigurationData(path.getParent());
+                    if (readLeafList == null || readLeafList.getValue().isEmpty()) {
+                        simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
+                    } else {
+                        insertWithPointLeafListPost(rWTransaction, datastore, path, payload, schemaContext, point,
+                            readLeafList, false);
+                    }
+                }
+                break;
+            default:
+                throw new RestconfDocumentedException(
+                    "Used bad value of insert parameter. Possible values are first, last, before or after, "
+                            + "but was: " + insert);
         }
     }
 
-    private void insertWithPointLeafListPost(final DOMDataReadWriteTransaction rWTransaction,
+    private static void insertWithPointLeafListPost(final DOMDataReadWriteTransaction rWTransaction,
             final LogicalDatastoreType datastore, final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload,
             final SchemaContext schemaContext, final String point, final OrderedLeafSetNode<?> readLeafList,
             final boolean before) {
@@ -807,7 +809,7 @@ public class BrokerFacade {
         }
     }
 
-    private void insertWithPointListPost(final DOMDataReadWriteTransaction rWTransaction,
+    private static void insertWithPointListPost(final DOMDataReadWriteTransaction rWTransaction,
             final LogicalDatastoreType datastore,
             final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload, final SchemaContext schemaContext,
             final String point, final MapNode readList, final boolean before) {
@@ -861,28 +863,30 @@ public class BrokerFacade {
         throw new RestconfDocumentedException("Insert parameter can be used only with list or leaf-list");
     }
 
-    private void makeNormalPost(final DOMDataReadWriteTransaction rWTransaction, final LogicalDatastoreType datastore,
-            final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload, final SchemaContext schemaContext) {
+    private static void makeNormalPost(final DOMDataReadWriteTransaction rWTransaction,
+            final LogicalDatastoreType datastore, final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload,
+            final SchemaContext schemaContext) {
+        final Collection<? extends NormalizedNode<?, ?>> children;
         if (payload instanceof MapNode) {
-            final NormalizedNode<?, ?> emptySubtree = ImmutableNodes.fromInstanceId(schemaContext, path);
-            rWTransaction.merge(datastore, YangInstanceIdentifier.create(emptySubtree.getIdentifier()), emptySubtree);
-            ensureParentsByMerge(datastore, path, rWTransaction, schemaContext);
-            for (final MapEntryNode child : ((MapNode) payload).getValue()) {
-                final YangInstanceIdentifier childPath = path.node(child.getIdentifier());
-                checkItemDoesNotExists(rWTransaction, datastore, childPath);
-                rWTransaction.put(datastore, childPath, child);
-            }
+            children = ((MapNode) payload).getValue();
         } else if (payload instanceof LeafSetNode) {
-            final NormalizedNode<?, ?> emptySubtree = ImmutableNodes.fromInstanceId(schemaContext, path);
-            rWTransaction.merge(datastore, YangInstanceIdentifier.create(emptySubtree.getIdentifier()), emptySubtree);
-            ensureParentsByMerge(datastore, path, rWTransaction, schemaContext);
-            for (final LeafSetEntryNode<?> child : ((LeafSetNode<?>) payload).getValue()) {
-                final YangInstanceIdentifier childPath = path.node(child.getIdentifier());
-                checkItemDoesNotExists(rWTransaction, datastore, childPath);
-                rWTransaction.put(datastore, childPath, child);
-            }
+            children = ((LeafSetNode<?>) payload).getValue();
         } else {
             simplePostPut(rWTransaction, datastore, path, payload, schemaContext);
+            return;
+        }
+
+        // We are putting multiple children, we really need a create() operation, but until we have that we make do
+        // with a two-step process of verifying if the children exist and then putting them in.
+        for (final NormalizedNode<?, ?> child : children) {
+            checkItemDoesNotExists(rWTransaction, datastore, path.node(child.getIdentifier()));
+        }
+
+        final NormalizedNode<?, ?> emptySubtree = ImmutableNodes.fromInstanceId(schemaContext, path);
+        rWTransaction.merge(datastore, YangInstanceIdentifier.create(emptySubtree.getIdentifier()), emptySubtree);
+        ensureParentsByMerge(datastore, path, rWTransaction, schemaContext);
+        for (final NormalizedNode<?, ?> child : children) {
+            rWTransaction.put(datastore, path.node(child.getIdentifier()), child);
         }
     }
 
@@ -1054,7 +1058,7 @@ public class BrokerFacade {
         }
     }
 
-    private void insertWithPointLeafListPut(final DOMDataReadWriteTransaction rWTransaction,
+    private static void insertWithPointLeafListPut(final DOMDataReadWriteTransaction rWTransaction,
             final LogicalDatastoreType datastore, final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload,
             final SchemaContext schemaContext, final String point, final OrderedLeafSetNode<?> readLeafList,
             final boolean before) {
@@ -1085,7 +1089,7 @@ public class BrokerFacade {
         }
     }
 
-    private void insertWithPointListPut(final DOMDataReadWriteTransaction rWTransaction,
+    private static void insertWithPointListPut(final DOMDataReadWriteTransaction rWTransaction,
             final LogicalDatastoreType datastore, final YangInstanceIdentifier path, final NormalizedNode<?, ?> payload,
             final SchemaContext schemaContext, final String point, final OrderedMapNode readList,
             final boolean before) {