From 7e845da39aa0101df4338422fdd30724ec280b9c Mon Sep 17 00:00:00 2001
From: Peter Kajsa <pkajsa@cisco.com>
Date: Wed, 5 Apr 2017 15:55:08 +0200
Subject: [PATCH] Bug 8126 - Some augments are not being processed

If a node is marked as mandatory (e.g. "leaf" with "mandatory true;"),
yang statement parser does not perform augmentation of such node into
a target node in different module even though one of ancestors of the
target node is a non-mandatory choice or a non-mandatory list from the
same namespace as the mandatory node and so in fact it should not
be considered as augmenatation of mandatory node into different module
anymore and augmentation should be performed.

We also increase severity of the augment error log from debug to error.

Change-Id: Icb8554ba61ce0aa0fa25541dd5675f3aa8ba3d1b
Signed-off-by: Peter Kajsa <pkajsa@cisco.com>
---
 .../parser/spi/meta/StmtContextUtils.java     | 39 +++++-----
 .../stmt/rfc6020/AugmentStatementImpl.java    | 15 ++--
 .../yangtools/yang/stmt/Bug8126Test.java      | 53 ++++++++++++++
 .../src/test/resources/bugs/bug8126/bar.yang  | 72 +++++++++++++++++++
 .../src/test/resources/bugs/bug8126/foo.yang  |  7 ++
 5 files changed, 164 insertions(+), 22 deletions(-)
 create mode 100644 yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/Bug8126Test.java
 create mode 100644 yang/yang-parser-impl/src/test/resources/bugs/bug8126/bar.yang
 create mode 100644 yang/yang-parser-impl/src/test/resources/bugs/bug8126/foo.yang

diff --git a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/StmtContextUtils.java b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/StmtContextUtils.java
index ad1c7e78d2..7725c74366 100644
--- a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/StmtContextUtils.java
+++ b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/spi/meta/StmtContextUtils.java
@@ -304,18 +304,15 @@ public final class StmtContextUtils {
     }
 
     /**
-     * Checks whether statement context is a mandatory node according to RFC6020
-     * or not.
+     * Checks whether statement context is a mandatory leaf, choice, anyxml,
+     * list or leaf-list according to RFC6020 or not.
      *
      * @param stmtCtx
      *            statement context
-     * @return true if it is a mandatory node according to RFC6020
+     * @return true if it is a mandatory leaf, choice, anyxml, list or leaf-list
+     *         according to RFC6020.
      */
     public static boolean isMandatoryNode(final StatementContextBase<?, ?, ?> stmtCtx) {
-        return isMandatoryListOrLeafList(stmtCtx) || isMandatoryLeafChoiceOrAnyXML(stmtCtx);
-    }
-
-    private static boolean isMandatoryLeafChoiceOrAnyXML(final StatementContextBase<?, ?, ?> stmtCtx) {
         if (!(stmtCtx.getPublicDefinition() instanceof YangStmtMapping)) {
             return false;
         }
@@ -324,16 +321,6 @@ public final class StmtContextUtils {
         case CHOICE:
         case ANYXML:
             return Boolean.TRUE.equals(firstSubstatementAttributeOf(stmtCtx, MandatoryStatement.class));
-        default:
-            return false;
-        }
-    }
-
-    private static boolean isMandatoryListOrLeafList(final StatementContextBase<?, ?, ?> stmtCtx) {
-        if (!(stmtCtx.getPublicDefinition() instanceof YangStmtMapping)) {
-            return false;
-        }
-        switch ((YangStmtMapping) stmtCtx.getPublicDefinition()) {
         case LIST:
         case LEAF_LIST:
             final Integer minElements = firstSubstatementAttributeOf(stmtCtx, MinElementsStatement.class);
@@ -343,6 +330,24 @@ public final class StmtContextUtils {
         }
     }
 
+    /**
+     * Checks whether a statement context is a statement of supplied statement
+     * definition and whether it is not mandatory leaf, choice, anyxml, list or
+     * leaf-list according to RFC6020.
+     *
+     * @param stmtCtx
+     *            statement context
+     * @param stmtDef
+     *            statement definition
+     * @return true if supplied statement context is a statement of supplied
+     *         statement definition and if it is not mandatory leaf, choice,
+     *         anyxml, list or leaf-list according to RFC6020
+     */
+    public static boolean isNotMandatoryNodeOfType(final StatementContextBase<?, ?, ?> stmtCtx,
+            final StatementDefinition stmtDef) {
+        return stmtCtx.getPublicDefinition().equals(stmtDef) && !isMandatoryNode(stmtCtx);
+    }
+
     /**
      * Checks whether at least one ancestor of a StatementContext matches one
      * from collection of statement definitions.
diff --git a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/AugmentStatementImpl.java b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/AugmentStatementImpl.java
index d19232d87b..4069da268a 100644
--- a/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/AugmentStatementImpl.java
+++ b/yang/yang-parser-impl/src/main/java/org/opendaylight/yangtools/yang/parser/stmt/rfc6020/AugmentStatementImpl.java
@@ -148,7 +148,7 @@ public class AugmentStatementImpl extends AbstractDeclaredStatement<SchemaNodeId
                         augmentTargetCtx.addEffectiveSubstatement(augmentSourceCtx);
                         updateAugmentOrder(augmentSourceCtx);
                     } catch (final SourceException e) {
-                        LOG.debug("Failed to add augmentation {} defined at {}",
+                        LOG.warn("Failed to add augmentation {} defined at {}",
                             augmentTargetCtx.getStatementSourceReference(),
                                 augmentSourceCtx.getStatementSourceReference(), e);
                     }
@@ -329,11 +329,16 @@ public class AugmentStatementImpl extends AbstractDeclaredStatement<SchemaNodeId
                 }
 
                 /*
-                 * If target or one of its parent is a presence container from
-                 * the same module, return false and skip mandatory nodes
-                 * validation
+                 * If target or one of the target's ancestors from the same namespace
+                 * is a presence container
+                 * or is non-mandatory choice
+                 * or is non-mandatory list
+                 * return false and skip mandatory nodes validation, because these nodes
+                 * are not mandatory node containers according to RFC 6020 section 3.1.
                  */
-                if (StmtContextUtils.isPresenceContainer(targetCtx)) {
+                if (StmtContextUtils.isPresenceContainer(targetCtx)
+                        || StmtContextUtils.isNotMandatoryNodeOfType(targetCtx, YangStmtMapping.CHOICE)
+                        || StmtContextUtils.isNotMandatoryNodeOfType(targetCtx, YangStmtMapping.LIST)) {
                     return false;
                 }
             } while ((targetCtx = targetCtx.getParentContext()) != root);
diff --git a/yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/Bug8126Test.java b/yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/Bug8126Test.java
new file mode 100644
index 0000000000..1148994dcc
--- /dev/null
+++ b/yang/yang-parser-impl/src/test/java/org/opendaylight/yangtools/yang/stmt/Bug8126Test.java
@@ -0,0 +1,53 @@
+/*
+ * Copyright (c) 2016 Cisco Systems, Inc. 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.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+
+import com.google.common.collect.ImmutableList;
+import org.junit.Test;
+import org.opendaylight.yangtools.yang.common.QName;
+import org.opendaylight.yangtools.yang.model.api.LeafSchemaNode;
+import org.opendaylight.yangtools.yang.model.api.SchemaContext;
+import org.opendaylight.yangtools.yang.model.api.SchemaNode;
+import org.opendaylight.yangtools.yang.model.api.SchemaPath;
+import org.opendaylight.yangtools.yang.model.util.SchemaContextUtil;
+
+public class Bug8126Test {
+    private static final String FOO_NS = "foo";
+    private static final String BAR_NS = "bar";
+    private static final String REV = "1970-01-01";
+
+    @Test
+    public void test() throws Exception {
+        final SchemaContext context = StmtTestUtils.parseYangSources("/bugs/bug8126");
+        assertNotNull(context);
+        assertTrue(findNode(context, ImmutableList.of(foo("root"), bar("my-container"), bar("my-choice"), bar("one"),
+                bar("one"), bar("mandatory-leaf"))) instanceof LeafSchemaNode);
+        assertTrue(findNode(context, ImmutableList.of(foo("root"), bar("my-list"), bar("two"), bar("mandatory-leaf-2"))) instanceof LeafSchemaNode);
+
+        assertNull(findNode(context, ImmutableList.of(foo("root"), bar("mandatory-list"))));
+        assertNull(findNode(context, ImmutableList.of(foo("root"), bar("mandatory-container"), bar("mandatory-choice"))));
+        assertNull(findNode(context,
+                ImmutableList.of(foo("root"), bar("mandatory-container-2"), bar("one"), bar("mandatory-leaf-3"))));
+    }
+
+    private static SchemaNode findNode(final SchemaContext context, final Iterable<QName> qNames) {
+        return SchemaContextUtil.findDataSchemaNode(context, SchemaPath.create(qNames, true));
+    }
+
+    private static QName foo(final String localName) {
+        return QName.create(FOO_NS, REV, localName);
+    }
+
+    private static QName bar(final String localName) {
+        return QName.create(BAR_NS, REV, localName);
+    }
+}
diff --git a/yang/yang-parser-impl/src/test/resources/bugs/bug8126/bar.yang b/yang/yang-parser-impl/src/test/resources/bugs/bug8126/bar.yang
new file mode 100644
index 0000000000..c7db6443ae
--- /dev/null
+++ b/yang/yang-parser-impl/src/test/resources/bugs/bug8126/bar.yang
@@ -0,0 +1,72 @@
+module bar {
+    namespace bar;
+    prefix bar;
+
+    import foo { prefix foo; revision-date 1970-01-01; }
+
+    //valid augments (non-mandatory choice)
+    augment "/foo:root" {
+        container my-container {
+            choice my-choice {
+                case one {
+                }
+            }
+        }
+    }
+
+    augment "/foo:root/my-container/my-choice/one" {
+        container one {
+            leaf mandatory-leaf {
+                mandatory true;
+                type empty;
+            }
+        }
+    }
+
+    //valid augments (non-mandatory list)
+    augment "/foo:root" {
+        list my-list {
+            min-elements 0;
+        }
+    }
+
+    augment "/foo:root/my-list" {
+        container two {
+            leaf mandatory-leaf-2 {
+                mandatory true;
+                type empty;
+            }
+        }
+    }
+
+    //invalid augment (mandatory choice)
+    augment "/foo:root" {
+        container mandatory-container {
+            choice mandatory-choice {
+                mandatory true;
+            }
+        }
+    }
+
+    //invalid augment (mandatory list)    
+    augment "/foo:root" {
+        list mandatory-list {
+            min-elements 1;
+        }
+    }
+
+    //invalid augments (mandatory container)
+    augment "/foo:root" {
+        container mandatory-container-2 {
+        }
+    }
+
+    augment "/foo:root/mandatory-container-2" {
+        container one {
+            leaf mandatory-leaf-3 {
+                mandatory true;
+                type empty;
+            }
+        }
+    }
+}
diff --git a/yang/yang-parser-impl/src/test/resources/bugs/bug8126/foo.yang b/yang/yang-parser-impl/src/test/resources/bugs/bug8126/foo.yang
new file mode 100644
index 0000000000..b1b0df7885
--- /dev/null
+++ b/yang/yang-parser-impl/src/test/resources/bugs/bug8126/foo.yang
@@ -0,0 +1,7 @@
+module foo {
+    namespace foo;
+    prefix foo;
+
+    container root {
+    }
+}
-- 
2.36.6