From 801a53da5a59219883bb1141d2e8191c11b65f47 Mon Sep 17 00:00:00 2001 From: Robert Varga Date: Tue, 22 Oct 2019 22:20:00 +0200 Subject: [PATCH] Reinstate "Check for nested augmentations" This reverts commit 02fa7531709e3c4599bc9ec23dc605a21a711bc1, effectively re-applying 759e69d85991edb987a760165adb83fa3f2ab67b. We also add the mechanics needed to walk the copy history backwards, as needed to understand the copy process. JIRA: YANGTOOLS-956 Change-Id: I96b3afb30fdcd326ef9b780ff02ccc7f1c94e07e Signed-off-by: Robert Varga --- .../stmt/reactor/StatementContextBase.java | 10 ++++ .../AbstractAugmentStatementSupport.java | 47 +++++++++++++++---- .../yangtools/yang/stmt/YT956Test.java | 38 +++++++++++++++ .../resources/bugs/YT956/another-module.yang | 12 +++++ .../test/resources/bugs/YT956/mainmodule.yang | 8 ++++ .../resources/bugs/YT956/sub-module-1.yang | 18 +++++++ .../resources/bugs/YT956/sub-module-2.yang | 43 +++++++++++++++++ .../yang/parser/spi/meta/StmtContext.java | 22 ++++++++- 8 files changed, 187 insertions(+), 11 deletions(-) create mode 100644 yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT956Test.java create mode 100644 yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/another-module.yang create mode 100644 yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/mainmodule.yang create mode 100644 yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/sub-module-1.yang create mode 100644 yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/sub-module-2.yang diff --git a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java index a29e11708e..02443a920d 100644 --- a/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java +++ b/yang/yang-parser-reactor/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/reactor/StatementContextBase.java @@ -101,6 +101,7 @@ public abstract class StatementContextBase, E private final @NonNull StatementDefinitionContext definition; private final @NonNull StatementSourceReference statementDeclSource; private final StmtContext originalCtx; + private final StmtContext prevCopyCtx; private final CopyHistory copyHistory; private final String rawArgument; @@ -127,6 +128,7 @@ public abstract class StatementContextBase, E this.rawArgument = def.internArgument(rawArgument); this.copyHistory = CopyHistory.original(); this.originalCtx = null; + this.prevCopyCtx = null; } StatementContextBase(final StatementDefinitionContext def, final StatementSourceReference ref, @@ -136,6 +138,7 @@ public abstract class StatementContextBase, E this.rawArgument = rawArgument; this.copyHistory = CopyHistory.of(copyType, CopyHistory.original()); this.originalCtx = null; + this.prevCopyCtx = null; } StatementContextBase(final StatementContextBase original, final CopyType copyType) { @@ -144,6 +147,7 @@ public abstract class StatementContextBase, E this.rawArgument = original.rawArgument; this.copyHistory = CopyHistory.of(copyType, original.getCopyHistory()); this.originalCtx = original.getOriginalCtx().orElse(original); + this.prevCopyCtx = original; } StatementContextBase(final StatementContextBase original) { @@ -152,6 +156,7 @@ public abstract class StatementContextBase, E this.rawArgument = original.rawArgument; this.copyHistory = original.getCopyHistory(); this.originalCtx = original.getOriginalCtx().orElse(original); + this.prevCopyCtx = original; this.substatements = original.substatements; this.effective = original.effective; } @@ -241,6 +246,11 @@ public abstract class StatementContextBase, E return Optional.ofNullable(originalCtx); } + @Override + public Optional> getPreviousCopyCtx() { + return Optional.ofNullable(prevCopyCtx); + } + @Override public ModelProcessingPhase getCompletedPhase() { return completedPhase; diff --git a/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/augment/AbstractAugmentStatementSupport.java b/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/augment/AbstractAugmentStatementSupport.java index fdf68e03c4..dff3296dd5 100644 --- a/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/augment/AbstractAugmentStatementSupport.java +++ b/yang/yang-parser-rfc7950/src/main/java/org/opendaylight/yangtools/yang/parser/rfc7950/stmt/augment/AbstractAugmentStatementSupport.java @@ -212,8 +212,11 @@ abstract class AbstractAugmentStatementSupport extends AbstractStatementSupport< * statement, otherwise false */ private static boolean isConditionalAugmentStmt(final StmtContext ctx) { - return ctx.getPublicDefinition() == YangStmtMapping.AUGMENT - && StmtContextUtils.findFirstSubstatement(ctx, WhenStatement.class) != null; + return ctx.getPublicDefinition() == YangStmtMapping.AUGMENT && hasWhenSubstatement(ctx); + } + + private static boolean hasWhenSubstatement(final StmtContext ctx) { + return StmtContextUtils.findFirstSubstatement(ctx, WhenStatement.class) != null; } private static void copyStatement(final Mutable original, final StatementContextBase target, @@ -237,7 +240,7 @@ abstract class AbstractAugmentStatementSupport extends AbstractStatementSupport< } if (!skipCheckOfMandatoryNodes && typeOfCopy == CopyType.ADDED_BY_AUGMENTATION - && reguiredCheckOfMandatoryNodes(sourceCtx, targetCtx)) { + && requireCheckOfMandatoryNodes(sourceCtx, targetCtx)) { checkForMandatoryNodes(sourceCtx); } @@ -276,23 +279,25 @@ abstract class AbstractAugmentStatementSupport extends AbstractStatementSupport< sourceCtx.rawStatementArgument()); } - private static boolean reguiredCheckOfMandatoryNodes(final StmtContext sourceCtx, + private static boolean requireCheckOfMandatoryNodes(final StmtContext sourceCtx, Mutable targetCtx) { /* * If the statement argument is not QName, it cannot be mandatory * statement, therefore return false and skip mandatory nodes validation */ - if (!(sourceCtx.getStatementArgument() instanceof QName)) { + final Object arg = sourceCtx.getStatementArgument(); + if (!(arg instanceof QName)) { return false; } - final QName sourceStmtQName = (QName) sourceCtx.getStatementArgument(); + final QName sourceStmtQName = (QName) arg; // RootStatementContext, for example final Mutable root = targetCtx.getRoot(); do { - Verify.verify(targetCtx.getStatementArgument() instanceof QName, - "Argument of augment target statement must be QName."); - final QName targetStmtQName = (QName) targetCtx.getStatementArgument(); + final Object targetArg = targetCtx.getStatementArgument(); + Verify.verify(targetArg instanceof QName, "Argument of augment target statement must be QName, not %s", + targetArg); + final QName targetStmtQName = (QName) targetArg; /* * If target is from another module, return true and perform mandatory nodes validation */ @@ -313,6 +318,22 @@ abstract class AbstractAugmentStatementSupport extends AbstractStatementSupport< || StmtContextUtils.isNotMandatoryNodeOfType(targetCtx, YangStmtMapping.LIST)) { return false; } + + // This could be an augmentation stacked on top of a previous augmentation from the same module, which is + // conditional -- in which case we do not run further checks + if (targetCtx.getCopyHistory().getLastOperation() == CopyType.ADDED_BY_AUGMENTATION) { + final Optional> optPrevCopy = targetCtx.getPreviousCopyCtx(); + if (optPrevCopy.isPresent()) { + final StmtContext original = optPrevCopy.get(); + final Object origArg = original.coerceStatementArgument(); + Verify.verify(origArg instanceof QName, "Unexpected statement argument %s", origArg); + + if (sourceStmtQName.getModule().equals(((QName) origArg).getModule()) + && hasWhenSubstatement(getParentAugmentation(original))) { + return false; + } + } + } } while ((targetCtx = targetCtx.getParentContext()) != root); /* @@ -322,6 +343,14 @@ abstract class AbstractAugmentStatementSupport extends AbstractStatementSupport< return false; } + private static StmtContext getParentAugmentation(final StmtContext child) { + StmtContext parent = Verify.verifyNotNull(child.getParentContext(), "Child %s has not parent", child); + while (parent.getPublicDefinition() != YangStmtMapping.AUGMENT) { + parent = Verify.verifyNotNull(parent.getParentContext(), "Failed to find augmentation parent of %s", child); + } + return parent; + } + private static final ImmutableSet NOCOPY_DEF_SET = ImmutableSet.of(YangStmtMapping.USES, YangStmtMapping.WHEN, YangStmtMapping.DESCRIPTION, YangStmtMapping.REFERENCE, YangStmtMapping.STATUS); diff --git a/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT956Test.java b/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT956Test.java new file mode 100644 index 0000000000..9b232b5c3c --- /dev/null +++ b/yang/yang-parser-rfc7950/src/test/java/org/opendaylight/yangtools/yang/stmt/YT956Test.java @@ -0,0 +1,38 @@ +/* + * Copyright (c) 2018 Pantheon Technologies, 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.stmt; + +import static org.hamcrest.Matchers.isA; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThat; + +import org.junit.Test; +import org.opendaylight.yangtools.yang.common.QName; +import org.opendaylight.yangtools.yang.model.api.ContainerSchemaNode; +import org.opendaylight.yangtools.yang.model.api.DataSchemaNode; + +public class YT956Test { + private static final QName ANOTHER_CONTAINER = QName.create("http://www.example.com/anothermodule", + "another-container"); + private static final QName FIRST_AUGMENT = QName.create("http://www.example.com/mainmodule", "first-augment"); + + @Test + public void testAugmentationConditional() throws Exception { + final DataSchemaNode another = StmtTestUtils.parseYangSources("/bugs/YT956/") + .findDataChildByName(ANOTHER_CONTAINER).get(); + assertThat(another, isA(ContainerSchemaNode.class)); + final ContainerSchemaNode anotherContainer = (ContainerSchemaNode) another; + + final DataSchemaNode first = anotherContainer.findDataChildByName(FIRST_AUGMENT).get(); + assertThat(first, isA(ContainerSchemaNode.class)); + final ContainerSchemaNode firstAugment = (ContainerSchemaNode) first; + + // Augmentation needs to be added + assertEquals(3, firstAugment.getChildNodes().size()); + } +} diff --git a/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/another-module.yang b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/another-module.yang new file mode 100644 index 0000000000..da47533ff9 --- /dev/null +++ b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/another-module.yang @@ -0,0 +1,12 @@ +module another-module { + yang-version 1.1; + namespace "http://www.example.com/anothermodule"; + prefix am; + + container another-container { + leaf type { + type string; + mandatory true; + } + } +} diff --git a/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/mainmodule.yang b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/mainmodule.yang new file mode 100644 index 0000000000..4fa41660aa --- /dev/null +++ b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/mainmodule.yang @@ -0,0 +1,8 @@ +module mainmodule { + yang-version 1.1; + namespace "http://www.example.com/mainmodule"; + prefix mm; + + include sub-module-1; + include sub-module-2; +} diff --git a/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/sub-module-1.yang b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/sub-module-1.yang new file mode 100644 index 0000000000..6e401c7a01 --- /dev/null +++ b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/sub-module-1.yang @@ -0,0 +1,18 @@ +submodule sub-module-1 { + yang-version 1.1; + + belongs-to mainmodule { + prefix mm; + } + + import another-module { + prefix am; + } + + augment '/am:another-container' { + when "am:type = 'test'"; + container first-augment { + + } + } +} diff --git a/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/sub-module-2.yang b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/sub-module-2.yang new file mode 100644 index 0000000000..3e6f0369c6 --- /dev/null +++ b/yang/yang-parser-rfc7950/src/test/resources/bugs/YT956/sub-module-2.yang @@ -0,0 +1,43 @@ +submodule sub-module-2 { + yang-version 1.1; + + belongs-to mainmodule { + prefix mm; + } + + import another-module { + prefix am; + } + + include sub-module-1; + + grouping dummygrouping { + leaf dummyleaf { + type string; + mandatory true; + } + } + + grouping first { + leaf first-leaf { + type string; + mandatory true; + } + } + + grouping second { + uses first; + leaf second-leaf { + type string; + mandatory true; + } + } + + augment '/am:another-container/mm:first-augment' { + uses dummygrouping; + } + + augment '/am:another-container/mm:first-augment' { + uses second; + } +} diff --git a/yang/yang-parser-spi/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/StmtContext.java b/yang/yang-parser-spi/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/StmtContext.java index 084785149c..9e99f0d996 100644 --- a/yang/yang-parser-spi/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/StmtContext.java +++ b/yang/yang-parser-spi/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/StmtContext.java @@ -155,14 +155,32 @@ public interface StmtContext, E extends Effect boolean isSupportedToBuildEffective(); + boolean isSupportedByFeatures(); + Collection> getEffectOfStatement(); + /** + * Return the executive summary of the copy process that has produced this context. + * + * @return A simplified summary of the copy process. + */ CopyHistory getCopyHistory(); - boolean isSupportedByFeatures(); - + /** + * Return the statement context of the original definition, if this statement is an instantiated copy. + * + * @return Original definition, if this statement was copied. + */ Optional> getOriginalCtx(); + /** + * Return the context of the previous copy of this statement -- effectively walking towards the source origin + * of this statement. + * + * @return Context of the previous copy of this statement, if this statement has been copied. + */ + Optional> getPreviousCopyCtx(); + ModelProcessingPhase getCompletedPhase(); /** -- 2.36.6