From ea25d431092eaa10677bc936c86ec9a4c4e401c6 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Mon, 5 Oct 2015 19:30:13 +0200 Subject: [PATCH] BUG-4359: use WRITE-implied metadata When we apply an operation under a WRITE node, we need to look up the metadata based on that write to correctly understand what the effect of this operation is. This is important when we modify data introduced by the WRITE node, as without this lookup a WRITE/DELETE combination on the child will result in a NONE logical operation, which means data introduced in the WRITE node will not be modified and seemingly reappear. Change-Id: I5a5a67470f1f8baafc7a71069341dead073ba84a Signed-off-by: Robert Varga --- .../tree/InMemoryDataTreeModification.java | 4 +- .../InMemoryDataTreeModificationCursor.java | 5 +- .../data/impl/schema/tree/ModifiedNode.java | 71 ++++++++++++++++--- .../tree/OperationWithModification.java | 17 ++--- 4 files changed, 77 insertions(+), 20 deletions(-) diff --git a/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/InMemoryDataTreeModification.java b/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/InMemoryDataTreeModification.java index a667bfd98e..48b6fe502a 100644 --- a/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/InMemoryDataTreeModification.java +++ b/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/InMemoryDataTreeModification.java @@ -75,7 +75,7 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements public void merge(final YangInstanceIdentifier path, final NormalizedNode data) { checkSealed(); checkIdentifierReferencesData(path, data); - resolveModificationFor(path).merge(data); + resolveModificationFor(path).merge(data, version); } @Override @@ -159,7 +159,7 @@ final class InMemoryDataTreeModification extends AbstractCursorAware implements operation = potential.get(); ++i; - modification = modification.modifyChild(pathArg, operation.getChildPolicy()); + modification = modification.modifyChild(pathArg, operation.getChildPolicy(), version); } return OperationWithModification.from(operation, modification); diff --git a/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/InMemoryDataTreeModificationCursor.java b/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/InMemoryDataTreeModificationCursor.java index 17216ee92f..e13073af02 100644 --- a/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/InMemoryDataTreeModificationCursor.java +++ b/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/InMemoryDataTreeModificationCursor.java @@ -34,7 +34,8 @@ final class InMemoryDataTreeModificationCursor extends AbstractCursor potential = op.getApplyOperation().getChild(child); if (potential.isPresent()) { final ModificationApplyOperation operation = potential.get(); - final ModifiedNode modification = op.getModification().modifyChild(child, operation.getChildPolicy()); + final ModifiedNode modification = op.getModification().modifyChild(child, operation.getChildPolicy(), + getParent().getVersion()); return OperationWithModification.from(operation, modification); } @@ -101,7 +102,7 @@ final class InMemoryDataTreeModificationCursor extends AbstractCursor data) { ensureNotClosed(); InMemoryDataTreeModification.checkIdentifierReferencesData(child, data); - resolveChildModification(child).merge(data); + resolveChildModification(child).merge(data, getParent().getVersion()); } @Override diff --git a/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/ModifiedNode.java b/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/ModifiedNode.java index 01e2299bc9..315464e999 100644 --- a/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/ModifiedNode.java +++ b/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/ModifiedNode.java @@ -16,9 +16,12 @@ import javax.annotation.Nonnull; import javax.annotation.concurrent.NotThreadSafe; import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.PathArgument; import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode; +import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNodes; import org.opendaylight.yangtools.yang.data.api.schema.tree.ModificationType; import org.opendaylight.yangtools.yang.data.api.schema.tree.StoreTreeNode; import org.opendaylight.yangtools.yang.data.api.schema.tree.spi.TreeNode; +import org.opendaylight.yangtools.yang.data.api.schema.tree.spi.TreeNodeFactory; +import org.opendaylight.yangtools.yang.data.api.schema.tree.spi.Version; /** * Node Modification Node and Tree @@ -57,6 +60,9 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode value; private ModificationType modType; + // Alternative history introduced in WRITE nodes. Instantiated when we touch any child underneath such a node. + private TreeNode writtenOriginal; + private ModifiedNode(final PathArgument identifier, final Optional original, final ChildTrackingPolicy childPolicy) { this.identifier = identifier; this.original = original; @@ -100,6 +106,54 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode fromNullable(children.get(child)); } + private Optional metadataFromSnapshot(@Nonnull final PathArgument child) { + return original.isPresent() ? original.get().getChild(child) : Optional.absent(); + } + + private Optional metadataFromData(@Nonnull final 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 + // modifications. + writtenOriginal = TreeNodeFactory.createTreeNode(value, modVersion); + } + + return writtenOriginal.getChild(child); + } + + /** + * Determine the base tree node we are going to apply the operation to. This is not entirely trivial because + * both DELETE and WRITE operations unconditionally detach their descendants from the original snapshot, so we need + * to take the current node's operation into account. + * + * @param child Child we are looking to modify + * @param modVersion Version allocated by the calling {@link InMemoryDataTreeModification} + * @return Before-image tree node as observed by that child. + */ + private Optional findOriginalMetadata(@Nonnull final PathArgument child, final Version modVersion) { + switch (operation) { + case DELETE: + // DELETE implies non-presence + return Optional.absent(); + case NONE: + case TOUCH: + return metadataFromSnapshot(child); + case MERGE: + // MERGE is half-way between TOUCH and WRITE. If the child exists in data, it behaves as a WRITE, otherwise + // it behaves as a TOUCH + if (NormalizedNodes.findNode(value, child).isPresent()) { + return metadataFromData(child, modVersion); + } else { + return metadataFromSnapshot(child); + } + case WRITE: + // WRITE implies presence based on written data + return metadataFromData(child, modVersion); + } + + throw new IllegalStateException("Unhandled node operation " + operation); + } + /** * * Returns child modification if child was modified, creates {@link ModifiedNode} @@ -110,10 +164,12 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode currentMetadata; - if (original.isPresent()) { - final TreeNode orig = original.get(); - currentMetadata = orig.getChild(child); - } else { - currentMetadata = Optional.absent(); - } + final Optional currentMetadata = findOriginalMetadata(child, modVersion); + final ModifiedNode newlyCreated = new ModifiedNode(child, currentMetadata, childPolicy); children.put(child, newlyCreated); @@ -227,6 +278,7 @@ final class ModifiedNode extends NodeModification implements StoreTreeNode data) { + private void recursiveMerge(final NormalizedNode data, final Version version) { if (data instanceof NormalizedNodeContainer) { @SuppressWarnings({ "rawtypes", "unchecked" }) final NormalizedNodeContainer> dataContainer = @@ -69,20 +69,20 @@ final class OperationWithModification { } } else { // Not present, issue a write - forChild(childId).write(c); + forChild(childId, version).write(c); } } } for (final NormalizedNode child : dataContainer.getValue()) { final PathArgument childId = child.getIdentifier(); - forChild(childId).recursiveMerge(child); + forChild(childId, version).recursiveMerge(child, version); } } modification.merge(data); } - void merge(final NormalizedNode data) { + void merge(final NormalizedNode data, final Version version) { /* * A merge operation will end up overwriting parts of the tree, retaining others. We want to * make sure we do not validate the complete resulting structure, but rather just what was @@ -93,7 +93,7 @@ final class OperationWithModification { * FIXME: Should be this moved to recursive merge and run for each node? */ applyOperation.verifyStructure(data, false); - recursiveMerge(data); + recursiveMerge(data, version); } void delete() { @@ -151,12 +151,13 @@ final class OperationWithModification { return new OperationWithModification(operation, modification); } - private OperationWithModification forChild(final PathArgument childId) { + private OperationWithModification forChild(final PathArgument childId, final Version version) { final Optional maybeChildOp = applyOperation.getChild(childId); - Preconditions.checkArgument(maybeChildOp.isPresent(), "Attempted to apply operation to non-existent child %s", childId); + Preconditions.checkArgument(maybeChildOp.isPresent(), + "Attempted to apply operation to non-existent child %s", childId); final ModificationApplyOperation childOp = maybeChildOp.get(); - final ModifiedNode childMod = modification.modifyChild(childId, childOp.getChildPolicy()); + final ModifiedNode childMod = modification.modifyChild(childId, childOp.getChildPolicy(), version); return from(childOp, childMod); } -- 2.36.6