Fix mandatory enforcer failure on augmented nodes 47/96447/2
authorRobert Varga <robert.varga@pantheon.tech>
Tue, 18 May 2021 08:28:13 +0000 (10:28 +0200)
committerRobert Varga <robert.varga@pantheon.tech>
Tue, 8 Jun 2021 06:47:38 +0000 (08:47 +0200)
Changes the way mandatory nodes are extracted from the schema.
Previously all mandatory nodes were approached as direct children of
the parent node. Even if they came from an augmentation which caused
"missing mandatory descendant" exceptions.
This patch preserves both options of building data nodes containing
augmented mandatory nodes.
- setting the mandatory node as a direct child of the parent node
- setting the mandatory node as an AugmentationNode to the parent node

However the former option seems to be incorrect and might be removed
in the future major release.

JIRA: YANGTOOLS-1276
Change-Id: Ifcc2e0d141e18f74173f15e293b16cbd297a5af6
Signed-off-by: Robert Varga <robert.varga@pantheon.tech>
Signed-off-by: Tibor Král <tibor.kral@pantheon.tech>
(cherry picked from commit 7862271a33b14d21ebf1e72aacd05d3e4687c6b1)

yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/MandatoryDescendant.java [new file with mode: 0644]
yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/MandatoryLeafEnforcer.java
yang/yang-data-impl/src/test/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/YT1276Test.java [new file with mode: 0644]
yang/yang-data-impl/src/test/resources/yt1276.yang [new file with mode: 0644]

diff --git a/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/MandatoryDescendant.java b/yang/yang-data-impl/src/main/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/MandatoryDescendant.java
new file mode 100644 (file)
index 0000000..e5ff14a
--- /dev/null
@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2021 PANTHEON.tech, s.r.o. 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.yangtools.yang.data.impl.schema.tree;
+
+import static com.google.common.base.Verify.verify;
+import static java.util.Objects.requireNonNull;
+
+import org.eclipse.jdt.annotation.NonNull;
+import org.eclipse.jdt.annotation.Nullable;
+import org.opendaylight.yangtools.concepts.Immutable;
+import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
+import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifier;
+import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNode;
+import org.opendaylight.yangtools.yang.data.api.schema.NormalizedNodes;
+import org.opendaylight.yangtools.yang.data.util.DataSchemaContextNode;
+import org.opendaylight.yangtools.yang.model.api.AugmentationSchemaNode;
+import org.opendaylight.yangtools.yang.model.api.AugmentationTarget;
+import org.opendaylight.yangtools.yang.model.api.DataNodeContainer;
+import org.opendaylight.yangtools.yang.model.api.DataSchemaNode;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A path to a descendant which must exist. This really should be equivalent to YangInstanceIdentifier, but for now we
+ * need to deal with up to two possible paths -- one with AugmentationIdentifiers and one without.
+ */
+// FIXME: 8.0.0: remove this structure and just keep the 'path' YangInstanceIdentifier
+final class MandatoryDescendant implements Immutable {
+    private static final Logger LOG = LoggerFactory.getLogger(MandatoryDescendant.class);
+
+    // Correctly-nested path, mirroring Augmentation and choice/case structure
+    private final @NonNull YangInstanceIdentifier path;
+    // Legacy trivial path,
+    private final @Nullable YangInstanceIdentifier legacyPath;
+
+    private MandatoryDescendant(final YangInstanceIdentifier path, final YangInstanceIdentifier legacyPath) {
+        this.path = requireNonNull(path);
+        this.legacyPath = legacyPath;
+    }
+
+    static @NonNull MandatoryDescendant create(final YangInstanceIdentifier parentId,
+            final DataNodeContainer parentSchema, final DataSchemaNode childSchema) {
+        final NodeIdentifier childId = NodeIdentifier.create(childSchema.getQName());
+
+        if (childSchema.isAugmenting()) {
+            verify(parentSchema instanceof AugmentationTarget, "Unexpected augmenting child %s in non-target %s at %s",
+                childSchema, parentSchema, parentId);
+
+            final AugmentationSchemaNode aug = ((AugmentationTarget) parentSchema).getAvailableAugmentations().stream()
+                .filter(augment -> augment.findDataChildByName(childSchema.getQName()).isPresent())
+                .findFirst()
+                .orElseThrow(() -> new IllegalArgumentException(String.format(
+                    "Node %s is marked as augmenting but is not present in the schema of %s", childSchema.getQName(),
+                    parentSchema)));
+
+            return new MandatoryDescendant(
+                parentId.node(DataSchemaContextNode.augmentationIdentifierFrom(aug)).node(childId).toOptimized(),
+                parentId.node(childId).toOptimized());
+        }
+
+        return new MandatoryDescendant(parentId.node(childId).toOptimized(), null);
+    }
+
+    void enforceOnData(final NormalizedNode<?, ?> data) {
+        // Find try primary path first ...
+        if (NormalizedNodes.findNode(data, path).isPresent()) {
+            return;
+        }
+        // ... if we have a legacy path, try that as well ...
+        if (legacyPath != null) {
+            if (NormalizedNodes.findNode(data, legacyPath).isPresent()) {
+                // .. this should not really be happening ...
+                LOG.debug("Found {} at alternate path {}", path, legacyPath);
+                return;
+            }
+        }
+
+        // ... not found, report the error
+        throw new IllegalArgumentException(String.format("Node %s is missing mandatory descendant %s",
+            data.getIdentifier(), path));
+    }
+
+    @Override
+    public String toString() {
+        return legacyPath == null ? path.toString() : "(" + path + " || " + legacyPath + ")";
+    }
+}
index 90ae47c9865183f29534d5ffa733894c9e5616ee..6c99d3bb5890169f9cf41a96a2a866d3649287c8 100644 (file)
@@ -7,7 +7,6 @@
  */
 package org.opendaylight.yangtools.yang.data.impl.schema.tree;
 
-import static com.google.common.base.Preconditions.checkArgument;
 import static java.util.Objects.requireNonNull;
 
 import com.google.common.collect.ImmutableList;
@@ -17,7 +16,6 @@ import org.opendaylight.yangtools.concepts.Immutable;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
 import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifier;
 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.DataTreeConfiguration;
 import org.opendaylight.yangtools.yang.data.api.schema.tree.TreeType;
 import org.opendaylight.yangtools.yang.data.api.schema.tree.spi.TreeNode;
@@ -33,58 +31,56 @@ import org.slf4j.LoggerFactory;
 final class MandatoryLeafEnforcer implements Immutable {
     private static final Logger LOG = LoggerFactory.getLogger(MandatoryLeafEnforcer.class);
 
-    private final ImmutableList<YangInstanceIdentifier> mandatoryNodes;
+    private final ImmutableList<MandatoryDescendant> mandatoryNodes;
 
-    private MandatoryLeafEnforcer(final ImmutableList<YangInstanceIdentifier> mandatoryNodes) {
+    private MandatoryLeafEnforcer(final ImmutableList<MandatoryDescendant> mandatoryNodes) {
         this.mandatoryNodes = requireNonNull(mandatoryNodes);
     }
 
-
     static Optional<MandatoryLeafEnforcer> forContainer(final DataNodeContainer schema,
             final DataTreeConfiguration treeConfig) {
         if (!treeConfig.isMandatoryNodesValidationEnabled()) {
             return Optional.empty();
         }
 
-        final Builder<YangInstanceIdentifier> builder = ImmutableList.builder();
+        final Builder<MandatoryDescendant> builder = ImmutableList.builder();
         findMandatoryNodes(builder, YangInstanceIdentifier.empty(), schema, treeConfig.getTreeType());
-        final ImmutableList<YangInstanceIdentifier> mandatoryNodes = builder.build();
+        final ImmutableList<MandatoryDescendant> mandatoryNodes = builder.build();
         return mandatoryNodes.isEmpty() ? Optional.empty() : Optional.of(new MandatoryLeafEnforcer(mandatoryNodes));
     }
 
     void enforceOnData(final NormalizedNode<?, ?> data) {
-        for (final YangInstanceIdentifier id : mandatoryNodes) {
-            checkArgument(NormalizedNodes.findNode(data, id).isPresent(),
-                "Node %s is missing mandatory descendant %s", data.getIdentifier(), id);
-        }
+        mandatoryNodes.forEach(node -> node.enforceOnData(data));
     }
 
     void enforceOnTreeNode(final TreeNode tree) {
         enforceOnData(tree.getData());
     }
 
-    private static void findMandatoryNodes(final Builder<YangInstanceIdentifier> builder,
-            final YangInstanceIdentifier id, final DataNodeContainer schema, final TreeType type) {
+    private static void findMandatoryNodes(final Builder<MandatoryDescendant> builder,
+        final YangInstanceIdentifier id, final DataNodeContainer schema, final TreeType type) {
         for (final DataSchemaNode child : schema.getChildNodes()) {
             if (SchemaAwareApplyOperation.belongsToTree(type, child)) {
                 if (child instanceof ContainerSchemaNode) {
                     final ContainerSchemaNode container = (ContainerSchemaNode) child;
                     if (!container.isPresenceContainer()) {
-                        findMandatoryNodes(builder, id.node(NodeIdentifier.create(child.getQName())), container, type);
+                        findMandatoryNodes(builder,
+                            id.node(NodeIdentifier.create(container.getQName())), container, type);
                     }
                 } else {
                     boolean needEnforce = child instanceof MandatoryAware && ((MandatoryAware) child).isMandatory();
                     if (!needEnforce && child instanceof ElementCountConstraintAware) {
-                        needEnforce = ((ElementCountConstraintAware) child)
-                                .getElementCountConstraint().map(constraint -> {
-                                    final Integer min = constraint.getMinElements();
-                                    return min != null && min > 0;
-                                }).orElse(Boolean.FALSE).booleanValue();
+                        needEnforce = ((ElementCountConstraintAware) child).getElementCountConstraint()
+                            .map(constraint -> {
+                                final Integer min = constraint.getMinElements();
+                                return min != null && min > 0;
+                            })
+                            .orElse(Boolean.FALSE);
                     }
                     if (needEnforce) {
-                        final YangInstanceIdentifier childId = id.node(NodeIdentifier.create(child.getQName()));
-                        LOG.debug("Adding mandatory child {}", childId);
-                        builder.add(childId.toOptimized());
+                        final MandatoryDescendant desc = MandatoryDescendant.create(id, schema, child);
+                        LOG.debug("Adding mandatory child {}", desc);
+                        builder.add(desc);
                     }
                 }
             }
diff --git a/yang/yang-data-impl/src/test/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/YT1276Test.java b/yang/yang-data-impl/src/test/java/org/opendaylight/yangtools/yang/data/impl/schema/tree/YT1276Test.java
new file mode 100644 (file)
index 0000000..7e138cc
--- /dev/null
@@ -0,0 +1,162 @@
+/*
+ * Copyright (c) 2021 PANTHEON.tech, s.r.o. 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.yangtools.yang.data.impl.schema.tree;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
+
+import java.util.Set;
+import java.util.function.Consumer;
+import org.junit.BeforeClass;
+import org.junit.Test;
+import org.opendaylight.yangtools.yang.common.QName;
+import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier;
+import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.AugmentationIdentifier;
+import org.opendaylight.yangtools.yang.data.api.YangInstanceIdentifier.NodeIdentifier;
+import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTree;
+import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeConfiguration;
+import org.opendaylight.yangtools.yang.data.api.schema.tree.DataTreeModification;
+import org.opendaylight.yangtools.yang.data.api.schema.tree.DataValidationFailedException;
+import org.opendaylight.yangtools.yang.data.impl.schema.Builders;
+import org.opendaylight.yangtools.yang.data.impl.schema.ImmutableNodes;
+import org.opendaylight.yangtools.yang.model.api.EffectiveModelContext;
+import org.opendaylight.yangtools.yang.test.util.YangParserTestUtils;
+
+public class YT1276Test {
+    private static final QName FOO = QName.create("foo", "foo");
+    private static final QName BAR = QName.create(FOO, "bar");
+    private static final QName BAZ = QName.create(FOO, "baz");
+    private static final QName XYZZY_LEAF = QName.create(FOO, "xyzzy-leaf");
+    private static final QName XYZZY_AUGMENT = QName.create(FOO, "xyzzy-augment");
+
+    private static EffectiveModelContext MODEL;
+
+    private final DataTree tree = new InMemoryDataTreeFactory()
+        .create(DataTreeConfiguration.DEFAULT_CONFIGURATION, MODEL);
+
+    @BeforeClass
+    public static void beforeClass() {
+        MODEL = YangParserTestUtils.parseYangResource("/yt1276.yang");
+    }
+
+    @Test
+    public void testFooWithBar() throws DataValidationFailedException {
+        applyOperation(mod -> {
+            mod.write(YangInstanceIdentifier.of(FOO), Builders.containerBuilder()
+                .withNodeIdentifier(new NodeIdentifier(FOO))
+                .withChild(Builders.augmentationBuilder()
+                    .withNodeIdentifier(new AugmentationIdentifier(Set.of(BAR)))
+                    .withChild(ImmutableNodes.leafNode(BAR, "xyzzy"))
+                    .build())
+                .build());
+        });
+    }
+
+    @Test
+    @Deprecated
+    public void testFooWithBarLegacy() throws DataValidationFailedException {
+        applyOperation(mod -> {
+            mod.write(YangInstanceIdentifier.of(FOO), Builders.containerBuilder()
+                .withNodeIdentifier(new NodeIdentifier(FOO))
+                .withChild(ImmutableNodes.leafNode(BAR, "xyzzy"))
+                .build());
+        });
+    }
+
+    @Test
+    public void testFooWithoutBar() {
+        final IllegalArgumentException ex = assertFailsReady(mod -> {
+            mod.write(YangInstanceIdentifier.of(FOO), Builders.containerBuilder()
+                .withNodeIdentifier(new NodeIdentifier(FOO))
+                .build());
+        });
+        assertEquals(
+            "Node (foo)foo is missing mandatory descendant /AugmentationIdentifier{childNames=[(foo)bar]}/(foo)bar",
+            ex.getMessage());
+    }
+
+    @Test
+    public void testBarWithXyzzy() throws DataValidationFailedException {
+        applyOperation(mod -> {
+            mod.write(YangInstanceIdentifier.of(BAR), Builders.containerBuilder()
+                .withNodeIdentifier(new NodeIdentifier(BAR))
+                .withChild(Builders.choiceBuilder()
+                    .withNodeIdentifier(new NodeIdentifier(BAZ))
+                    .withChild(ImmutableNodes.leafNode(XYZZY_LEAF, "xyzzy"))
+                    .withChild(Builders.augmentationBuilder()
+                        .withNodeIdentifier(new AugmentationIdentifier(Set.of(XYZZY_AUGMENT)))
+                        .withChild(ImmutableNodes.leafNode(XYZZY_AUGMENT, "xyzzy"))
+                        .build())
+                    .build())
+                .build());
+        });
+    }
+
+    @Test
+    @Deprecated
+    public void testBarWithXyzzyLegacy() throws DataValidationFailedException {
+        applyOperation(mod -> {
+            mod.write(YangInstanceIdentifier.of(BAR), Builders.containerBuilder()
+                .withNodeIdentifier(new NodeIdentifier(BAR))
+                .withChild(Builders.choiceBuilder()
+                    .withNodeIdentifier(new NodeIdentifier(BAZ))
+                    .withChild(ImmutableNodes.leafNode(XYZZY_LEAF, "xyzzy"))
+                    .withChild(ImmutableNodes.leafNode(XYZZY_AUGMENT, "xyzzy"))
+                    .build())
+                .build());
+        });
+    }
+
+    @Test
+    public void testBarWithoutXyzzyLeaf() {
+        final IllegalArgumentException ex = assertFailsReady(mod -> {
+            mod.write(YangInstanceIdentifier.of(BAR), Builders.containerBuilder()
+                .withNodeIdentifier(new NodeIdentifier(BAR))
+                .withChild(Builders.choiceBuilder()
+                    .withNodeIdentifier(new NodeIdentifier(BAZ))
+                    .withChild(Builders.augmentationBuilder()
+                        .withNodeIdentifier(new AugmentationIdentifier(Set.of(XYZZY_AUGMENT)))
+                        .withChild(ImmutableNodes.leafNode(XYZZY_AUGMENT, "xyzzy"))
+                        .build())
+                    .build())
+                .build());
+        });
+        assertEquals(
+            "Node (foo)baz is missing mandatory descendant /(foo)xyzzy-leaf",
+            ex.getMessage());
+    }
+
+    @Test
+    public void testBarWithoutXyzzyAugment() {
+        final IllegalArgumentException ex = assertFailsReady(mod -> {
+            mod.write(YangInstanceIdentifier.of(BAR), Builders.containerBuilder()
+                .withNodeIdentifier(new NodeIdentifier(BAR))
+                .withChild(Builders.choiceBuilder()
+                    .withNodeIdentifier(new NodeIdentifier(BAZ))
+                    .withChild(ImmutableNodes.leafNode(XYZZY_LEAF, "xyzzy"))
+                    .build())
+                .build());
+        });
+        assertEquals("Node (foo)baz is missing mandatory descendant "
+            + "/AugmentationIdentifier{childNames=[(foo)xyzzy-augment]}/(foo)xyzzy-augment",
+            ex.getMessage());
+
+    }
+
+    private IllegalArgumentException assertFailsReady(final Consumer<DataTreeModification> operation) {
+        return assertThrows(IllegalArgumentException.class, () -> applyOperation(operation));
+    }
+
+    private void applyOperation(final Consumer<DataTreeModification> operation)
+            throws DataValidationFailedException {
+        final DataTreeModification mod = tree.takeSnapshot().newModification();
+        operation.accept(mod);
+        mod.ready();
+        tree.commit(tree.prepare(mod));
+    }
+}
diff --git a/yang/yang-data-impl/src/test/resources/yt1276.yang b/yang/yang-data-impl/src/test/resources/yt1276.yang
new file mode 100644 (file)
index 0000000..33c2979
--- /dev/null
@@ -0,0 +1,36 @@
+module foo {
+  namespace foo;
+  prefix foo;
+
+  container foo {
+    presence blah;
+  }
+
+  augment /foo {
+    leaf bar {
+      type string;
+      mandatory true;
+    }
+  }
+
+  container bar {
+    presence blah;
+
+    choice baz {
+      case xyzzy-case {
+        leaf xyzzy-leaf {
+          type string;
+          mandatory true;
+        }
+      }
+    }
+  }
+
+  augment /bar/baz/xyzzy-case {
+    leaf xyzzy-augment {
+      type string;
+      mandatory true;
+    }
+  }
+}
+