From: Robert Varga <robert.varga@pantheon.tech>
Date: Mon, 4 Sep 2023 18:15:35 +0000 (+0200)
Subject: Do not store Optional in ModifiedNode
X-Git-Tag: v11.0.4~8
X-Git-Url: https://git.opendaylight.org/gerrit/gitweb?a=commitdiff_plain;h=ac45ed859db4d9365bae01412e955bff00769732;p=yangtools.git

Do not store Optional in ModifiedNode

Storing an Optional is a bit superfluous and an anti-pattern. Use a
@Nullable type instead. Also migrate users which can easily use a
nullable instead of Optional.

JIRA: YANGTOOLS-1538
Change-Id: I70bdea652ee0186aae6109db1ae756780eaa54b7
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
---

diff --git a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/AbstractNodeContainerModificationStrategy.java b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/AbstractNodeContainerModificationStrategy.java
index 3c5879228f..30240ce098 100644
--- a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/AbstractNodeContainerModificationStrategy.java
+++ b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/AbstractNodeContainerModificationStrategy.java
@@ -16,7 +16,6 @@ import java.util.Collection;
 import java.util.Optional;
 import org.eclipse.jdt.annotation.NonNull;
 import org.eclipse.jdt.annotation.Nullable;
-import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument;
 import org.opendaylight.yangtools.yang.data.api.schema.DistinctNodeContainer;
 import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
@@ -357,8 +356,8 @@ abstract sealed class AbstractNodeContainerModificationStrategy<T extends DataSc
         if (current.isEmpty()) {
             currentNode = defaultTreeNode();
             if (currentNode == null) {
-                if (modification.getOriginal().isEmpty()) {
-                    final YangInstanceIdentifier id = path.toInstanceIdentifier();
+                if (modification.original() == null) {
+                    final var id = path.toInstanceIdentifier();
                     throw new ModifiedNodeDoesNotExistException(id,
                         "Node " + id + " does not exist. Cannot apply modification to its children.");
                 }
diff --git a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java
index 0201cd21e2..73b25bd693 100644
--- a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java
+++ b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/ModifiedNode.java
@@ -41,7 +41,7 @@ import org.opendaylight.yangtools.yang.data.tree.impl.node.Version;
  */
 final class ModifiedNode extends NodeModification implements StoreTreeNode<ModifiedNode> {
     private final Map<PathArgument, ModifiedNode> children;
-    private final Optional<? extends TreeNode> original;
+    private final @Nullable TreeNode original;
     private final @NonNull PathArgument identifier;
 
     private LogicalOperation operation = LogicalOperation.NONE;
@@ -57,7 +57,7 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
     private Optional<? extends TreeNode> validatedCurrent;
     private ValidatedTreeNode validatedNode;
 
-    private ModifiedNode(final PathArgument identifier, final Optional<? extends TreeNode> original,
+    private ModifiedNode(final PathArgument identifier, final @Nullable TreeNode original,
             final ChildTrackingPolicy childPolicy) {
         this.identifier = requireNonNull(identifier);
         this.original = original;
@@ -75,7 +75,7 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
     }
 
     @Override
-    Optional<? extends TreeNode> getOriginal() {
+    TreeNode original() {
         return original;
     }
 
@@ -101,11 +101,12 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
         return children.get(arg);
     }
 
-    private Optional<? extends TreeNode> metadataFromSnapshot(final @NonNull PathArgument child) {
-        return original.isPresent() ? original.orElseThrow().findChildByArg(child) : Optional.empty();
+    private @Nullable TreeNode metadataFromSnapshot(final @NonNull PathArgument child) {
+        final var local = original;
+        return local != null ? local.childByArg(child) : null;
     }
 
-    private Optional<? extends TreeNode> metadataFromData(final @NonNull PathArgument child, final Version modVersion) {
+    private @Nullable TreeNode metadataFromData(final @NonNull PathArgument child, final Version modVersion) {
         if (writtenOriginal == null) {
             // Lazy instantiation, as we do not want do this for all writes. We are using the modification's version
             // here, as that version is what the SchemaAwareApplyOperation will see when dealing with the resulting
@@ -113,7 +114,7 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
             writtenOriginal = TreeNode.of(value, modVersion);
         }
 
-        return writtenOriginal.findChildByArg(child);
+        return writtenOriginal.childByArg(child);
     }
 
     /**
@@ -125,12 +126,11 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
      * @param modVersion Version allocated by the calling {@link InMemoryDataTreeModification}
      * @return Before-image tree node as observed by that child.
      */
-    private Optional<? extends TreeNode> findOriginalMetadata(final @NonNull PathArgument child,
-            final Version modVersion) {
+    private @Nullable TreeNode originalMetadata(final @NonNull PathArgument child, final Version modVersion) {
         return switch (operation) {
             case DELETE ->
                 // DELETE implies non-presence
-                Optional.empty();
+                null;
             case NONE, TOUCH, MERGE -> metadataFromSnapshot(child);
             case WRITE ->
                 // WRITE implies presence based on written data
@@ -155,20 +155,20 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
         if (operation == LogicalOperation.NONE) {
             updateOperationType(LogicalOperation.TOUCH);
         }
-        final ModifiedNode potential = children.get(child);
+        final var potential = children.get(child);
         if (potential != null) {
             return potential;
         }
 
-        final Optional<? extends TreeNode> currentMetadata = findOriginalMetadata(child, modVersion);
-        final ModifiedNode newlyCreated = new ModifiedNode(child, currentMetadata, childOper.getChildPolicy());
+        final var newlyCreated = new ModifiedNode(child, originalMetadata(child, modVersion),
+            childOper.getChildPolicy());
         if (operation == LogicalOperation.MERGE && value != null) {
             /*
              * We are attempting to modify a previously-unmodified part of a MERGE node. If the
              * value contains this component, we need to materialize it as a MERGE modification.
              */
             @SuppressWarnings({ "rawtypes", "unchecked" })
-            final NormalizedNode childData = ((DistinctNodeContainer)value).childByArg(child);
+            final var childData = ((DistinctNodeContainer) value).childByArg(child);
             if (childData != null) {
                 childOper.mergeIntoModifiedNode(newlyCreated, childData, modVersion);
             }
@@ -211,7 +211,7 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
                 //
                 // As documented in BUG-2470, a delete of data introduced in this transaction needs to be turned into
                 // a no-op.
-                original.isPresent() ? LogicalOperation.DELETE : LogicalOperation.NONE;
+                original != null ? LogicalOperation.DELETE : LogicalOperation.NONE;
         };
 
         clearSnapshot();
@@ -327,7 +327,7 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode<Modif
     }
 
     public static ModifiedNode createUnmodified(final TreeNode metadataTree, final ChildTrackingPolicy childPolicy) {
-        return new ModifiedNode(metadataTree.getIdentifier(), Optional.of(metadataTree), childPolicy);
+        return new ModifiedNode(metadataTree.getIdentifier(), requireNonNull(metadataTree), childPolicy);
     }
 
     void setValidatedNode(final ModificationApplyOperation op, final Optional<? extends TreeNode> current,
diff --git a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/NodeModification.java b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/NodeModification.java
index 4b4005db6b..5d24b2a2e5 100644
--- a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/NodeModification.java
+++ b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/NodeModification.java
@@ -9,6 +9,7 @@ package org.opendaylight.yangtools.yang.data.tree.impl;
 
 import java.util.Collection;
 import java.util.Optional;
+import org.eclipse.jdt.annotation.Nullable;
 import org.opendaylight.yangtools.concepts.Identifiable;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument;
 import org.opendaylight.yangtools.yang.data.tree.impl.node.TreeNode;
@@ -28,10 +29,19 @@ abstract sealed class NodeModification implements Identifiable<PathArgument> per
     /**
      * Get the original tree node to which the modification is to be applied.
      *
-     * @return The original node, or {@link Optional#absent()} if the node is
-     *         a new node.
+     * @return The original node, or {@link Optional#absent()} if the node is a new node.
      */
-    abstract Optional<? extends TreeNode> getOriginal();
+    // FIXME: we should not need this method
+    final Optional<? extends TreeNode> getOriginal() {
+        return Optional.ofNullable(original());
+    }
+
+    /**
+     * Get the original tree node to which the modification is to be applied.
+     *
+     * @return The original node, or {@code null} if the node is a new node.
+     */
+    abstract @Nullable TreeNode original();
 
     /**
      * Get a read-only view of children nodes.
diff --git a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/SchemaAwareApplyOperation.java b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/SchemaAwareApplyOperation.java
index e884f300c2..8cd04e29ab 100644
--- a/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/SchemaAwareApplyOperation.java
+++ b/data/yang-data-tree-ri/src/main/java/org/opendaylight/yangtools/yang/data/tree/impl/SchemaAwareApplyOperation.java
@@ -148,16 +148,14 @@ abstract sealed class SchemaAwareApplyOperation<T extends DataSchemaNode> extend
 
     protected void checkMergeApplicable(final ModificationPath path, final NodeModification modification,
             final Optional<? extends TreeNode> current, final Version version) throws DataValidationFailedException {
-        final Optional<? extends TreeNode> original = modification.getOriginal();
-        if (original.isPresent() && current.isPresent()) {
+        final var orig = modification.original();
+        if (orig != null && current.isPresent()) {
             /*
-             * We need to do conflict detection only and only if the value of leaf changed
-             * before two transactions. If value of leaf is unchanged between two transactions
-             * it should not cause transaction to fail, since result of this merge
-             * leads to same data.
+             * We need to do conflict detection only and only if the value of leaf changed before two transactions. If
+             * value of leaf is unchanged between two transactions it should not cause transaction to fail, since result
+             * of this merge leads to same data.
              */
-            final TreeNode orig = original.orElseThrow();
-            final TreeNode cur = current.orElseThrow();
+            final var cur = current.orElseThrow();
             if (!orig.getData().equals(cur.getData())) {
                 checkNotConflicting(path, orig, cur);
             }
@@ -176,11 +174,11 @@ abstract sealed class SchemaAwareApplyOperation<T extends DataSchemaNode> extend
      */
     private static void checkWriteApplicable(final ModificationPath path, final NodeModification modification,
             final Optional<? extends TreeNode> current, final Version version) throws DataValidationFailedException {
-        final var original = modification.getOriginal();
-        if (original.isPresent() && current.isPresent()) {
-            checkNotConflicting(path, original.orElseThrow(), current.orElseThrow());
+        final var original = modification.original();
+        if (original != null && current.isPresent()) {
+            checkNotConflicting(path, original, current.orElseThrow());
         } else {
-            checkConflicting(path, original.isEmpty(), "Node was deleted by other transaction.");
+            checkConflicting(path, original == null, "Node was deleted by other transaction.");
             checkConflicting(path, current.isEmpty(), "Node was created by other transaction.");
         }
     }