Fixed: [Error when changing choice/case with different structure](https://github.com/clicon/clixon/issues/568)

CHANGELOG.md

@@ -67,6 +67,7 @@ Developers may need to change their code
 
 ### Corrected Busg
 
+* Fixed: [Error when changing choice/case with different structure](https://github.com/clicon/clixon/issues/568)
 * Fixed: [Clixon handle if-feature incorrectly](https://github.com/clicon/clixon/issues/555)
 * Fixed: [Clixon fails to load yang with extension](https://github.com/clicon/clixon/issues/554)
 * Fixed: Double top-levels in xmldb_get that could occur with xpath containing choice.

lib/src/clixon_datastore_write.c

@@ -109,13 +109,12 @@ struct xmldb_multi_write_arg {
  * @param[in]  x         XML node (where to look for attribute)
  * @param[in]  name      Attribute name
  * @param[in]  ns        (Expected) Namespace of attribute
- * @param[in]  peek      If 0 dont remove attribute
+ * @param[in]  peek      If 1 dont remove attribute
  * @param[out] cbret     Error message (if retval=0)
  * @param[out] valp      Malloced value (if retval=1)
  * @retval     1         OK
  * @retval     0         Failed (cbret set)
  * @retval    -1         Error
- * @note as a side.effect the attribute is removed
  */
 static int
 attr_ns_value(cxobj *x,
@@ -384,7 +383,7 @@ check_when_condition(cxobj     *x0p,
     goto done;
 }
 
-/*! Check if x0/y0 is part of other choice/case than y1 recursively , if so purge 
+/*! Check if y0 is part of other choice/case than y1 recursively
  *
  * @retval 1 yes, y0 is in other case than y1
  * @retval 0 No, y0 it is not in other case than y1
@@ -423,6 +422,56 @@ choice_is_other(yang_stmt *y0c,
     return 0;
 }
 
+/*! Find/peek operation recursively other than NONE
+ *
+ * @param[in]  x      Base tree node
+ * @param[in]  op     NETCONF operation
+ * @param[out] cbret  Initialized cligen buffer. Contains return XML if retval is 0. 
+ * @retval     1      OK
+ * @retval     0      Fail with cbret set
+ * @retval    -1      Error
+ */
+static int
+find_first_op_recurse(cxobj               *x,
+                      enum operation_type *op,
+                      cbuf                *cbret)
+{
+    int    retval = -1;
+    char  *opstr = NULL;
+    cxobj *xc;
+    int    ret;
+
+    if (*op != OP_NONE)
+        goto ok;
+    if ((ret = attr_ns_value(x, "operation", NETCONF_BASE_NAMESPACE, 1, cbret, &opstr)) < 0)
+        goto done;
+    if (ret == 0)
+        goto fail;
+    if (opstr != NULL)
+        if (xml_operation(opstr, op) < 0)
+            goto done;
+    if (*op == OP_NONE){
+        xc = NULL;
+        while ((xc = xml_child_each(x, xc, CX_ELMNT)) != NULL) {
+            if ((ret = find_first_op_recurse(xc, op, cbret)) < 0)
+                goto done;
+            if (ret == 0)
+                goto fail;
+            if (*op != OP_NONE)
+                break;
+        }
+    }
+ ok:
+    retval = 1;
+ done:
+    if (opstr)
+        free(opstr);
+    return retval;
+ fail:
+    retval = 0;
+    goto done;
+}
+
 /*! Check if choice nodes and if add implicitly remove all other cases, or fail if delete
  *
  * Special case is if yc parent (yp) is choice/case
@@ -431,6 +480,8 @@ choice_is_other(yang_stmt *y0c,
  * @param[in]  x0      Base tree node
  * @param[in]  x1c     Tree child
  * @param[in]  y1c     Yang spec of x1c
+ * @param[in]  op      NETCONF operation
+ * @param[out] cbret   Initialized cligen buffer. Contains return XML if retval is 0. 
  * @retval     1       OK
  * @retval     0       Fail with cbret set
  * @retval    -1       Error
@@ -441,6 +492,7 @@ choice_is_other(yang_stmt *y0c,
  * all nodes from all other cases.  If a request creates a node from a
  * case, the server will delete any existing nodes that are defined in
  * other cases inside the choice.
+ * XXX just looks for first op, handling could be improved if several
  */
 static int
 choice_other_match(cxobj              *x0,
@@ -457,16 +509,14 @@ choice_other_match(cxobj              *x0,
     yang_stmt *y0choice;
     yang_stmt *y1case = NULL;
     yang_stmt *y1choice = NULL;
-    char      *opstr = NULL;
     int        ret;
 
-    if ((ret = attr_ns_value(x1c, "operation", NETCONF_BASE_NAMESPACE, 1, cbret, &opstr)) < 0)
+    if ((ret = find_first_op_recurse(x1c, &op, cbret)) < 0)
         goto done;
     if (ret == 0)
         goto fail;
-    if (opstr != NULL)
+    if (op == OP_REMOVE || op == OP_NONE)
-        if (xml_operation(opstr, &op) < 0)
+        goto ok;
-            goto done;
     if (yang_choice_case_get(y1c, &y1case, &y1choice) == 0)
         goto ok;
     x0prev = NULL;
@@ -507,8 +557,6 @@ choice_other_match(cxobj              *x0,
  ok:
     retval = 1;
  done:
-    if (opstr)
-        free(opstr);
     return retval;
  fail:
     retval = 0;

test/test_choice.sh

@@ -37,7 +37,7 @@ cat <<EOF > $cfg
   <CLICON_CLISPEC_DIR>$clidir</CLICON_CLISPEC_DIR>
   <CLICON_SOCK>/usr/local/var/run/$APPNAME.sock</CLICON_SOCK>
   <CLICON_BACKEND_PIDFILE>/usr/local/var/run/$APPNAME.pidfile</CLICON_BACKEND_PIDFILE>
-  <CLICON_XMLDB_DIR>/usr/local/var/$APPNAME</CLICON_XMLDB_DIR>
+  <CLICON_XMLDB_DIR>$dir</CLICON_XMLDB_DIR>
   $RESTCONFIG
 </clixon-config>
 EOF

test/test_choice_implicit_delete.sh

@@ -0,0 +1,200 @@
+#!/usr/bin/env bash
+# test of choice implicit delete, see RFC7950 Sec 7.9 3rd paragraph:
+#   Since only one of the choice's cases can be valid at any time in the
+#   data tree, the creation of a node from one case implicitly deletes
+#   all nodes from all other cases.  If a request creates a node from a
+#   case, the server will delete any existing nodes that are defined in
+#   other cases inside the choice.
+
+# Magic line must be first in script (see README.md)
+s="$_" ; . ./lib.sh || if [ "$s" = $0 ]; then exit 0; else return 0; fi
+
+APPNAME=example
+
+cfg=$dir/choice.xml
+clidir=$dir/cli
+fyang=$dir/type.yang
+
+test -d ${clidir} || rm -rf ${clidir}
+mkdir $clidir
+
+cat <<EOF > $cfg
+<clixon-config xmlns="http://clicon.org/config">
+  <CLICON_CONFIGFILE>$cfg</CLICON_CONFIGFILE>
+  <CLICON_FEATURE>clixon-restconf:allow-auth-none</CLICON_FEATURE> <!-- Use auth-type=none -->
+  <CLICON_YANG_DIR>$dir</CLICON_YANG_DIR>
+  <CLICON_YANG_DIR>${YANG_INSTALLDIR}</CLICON_YANG_DIR>
+  <CLICON_YANG_MAIN_FILE>$fyang</CLICON_YANG_MAIN_FILE>
+  <CLICON_CLISPEC_DIR>/usr/local/lib/$APPNAME/clispec</CLICON_CLISPEC_DIR>
+  <CLICON_CLI_DIR>/usr/local/lib/$APPNAME/cli</CLICON_CLI_DIR>
+  <CLICON_CLI_MODE>$APPNAME</CLICON_CLI_MODE>
+  <CLICON_CLISPEC_DIR>$clidir</CLICON_CLISPEC_DIR>
+  <CLICON_SOCK>/usr/local/var/run/$APPNAME.sock</CLICON_SOCK>
+  <CLICON_BACKEND_PIDFILE>/usr/local/var/run/$APPNAME.pidfile</CLICON_BACKEND_PIDFILE>
+  <CLICON_XMLDB_DIR>$dir</CLICON_XMLDB_DIR>
+</clixon-config>
+EOF
+
+cat <<EOF > $fyang
+module system{
+  yang-version 1.1;
+  namespace "urn:example:config";
+  prefix ex;
+  /* Case with container vs leaf */
+  container cont {
+     description
+       "One case is leaf, the other container+leaf, issue when replacing one with the other";
+     choice d {
+        case d1 {
+           container d1c {
+              choice d11 {      
+                leaf d11x{
+                  type string;
+                }
+                leaf d11y{
+                  type string;
+                }
+              }
+           }
+        }
+        case d2 {
+           container d2c {
+              leaf d2x{
+                 type string;
+              }
+           }
+        }
+     }
+   }
+}
+EOF
+
+cat <<EOF > $clidir/ex.cli
+# Clixon example specification
+CLICON_MODE="example";
+CLICON_PROMPT="%U@%H %W> ";
+CLICON_PLUGIN="example_cli";
+
+# Autocli syntax tree operations
+set @datamodel, cli_auto_set();
+delete("Delete a configuration item") {
+      @datamodel, cli_auto_del(); 
+      all("Delete whole candidate configuration"), delete_all("candidate");
+}
+validate("Validate changes"), cli_validate();
+commit("Commit the changes"), cli_commit();
+quit("Quit"), cli_quit();
+discard("Discard edits (rollback 0)"), discard_changes();
+
+show("Show a particular state of the system"){
+    configuration("Show configuration"), cli_show_auto_mode("candidate", "text", true, false);{
+            cli("Show configuration as CLI commands"), cli_show_auto_mode("candidate", "cli", true, false, "report-all", "set ");
+            xml("Show configuration as XML"), cli_show_auto_mode("candidate", "xml", true, false, NULL);
+    }
+}
+EOF
+
+new "test params: -f $cfg"
+
+if [ $BE -ne 0 ]; then
+    new "kill old backend"
+    sudo clixon_backend -zf $cfg
+    if [ $? -ne 0 ]; then
+        err
+    fi
+    sudo pkill -f clixon_backend # to be sure
+    
+    new "start backend -s init -f $cfg"
+    start_backend -s init -f $cfg
+fi
+
+new "wait backend"
+wait_backend
+
+# replace
+
+new "set d1"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><edit-config><target><candidate/></target><config>
+<cont xmlns=\"urn:example:config\">
+<d1c>
+<d11x>aaa</d11x>
+</d1c>
+</cont>
+</config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
+new "replace d2 top"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><edit-config><target><candidate/></target><default-operation>none</default-operation><config>
+<cont xmlns=\"urn:example:config\" xmlns:nc=\"${BASENS}\">
+<d2c nc:operation='replace'><d2x>bbb</d2x></d2c>
+</cont>
+</config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
+new "check d2"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><get-config><source><candidate/></source></get-config></rpc>" "<cont xmlns=\"urn:example:config\"><d2c><d2x>bbb</d2x></d2c></cont>" ""
+
+new "replace d1 part"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><edit-config><target><candidate/></target><default-operation>none</default-operation><config>
+<cont xmlns=\"urn:example:config\" xmlns:nc=\"${BASENS}\">
+<d1c><d11x nc:operation='replace'>ccc</d11x></d1c>
+</cont>
+</config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
+new "check d1"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><get-config><source><candidate/></source></get-config></rpc>" "<cont xmlns=\"urn:example:config\"><d1c><d11x>ccc</d11x></d1c></cont>" ""
+
+# merge
+new "merge d2 part"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><edit-config><target><candidate/></target><default-operation>none</default-operation><config>
+<cont xmlns=\"urn:example:config\" xmlns:nc=\"${BASENS}\">
+<d2c><d2x nc:operation='merge'>ddd</d2x></d2c>
+</cont>
+</config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
+new "check d2"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><get-config><source><candidate/></source></get-config></rpc>" "<cont xmlns=\"urn:example:config\"><d2c><d2x>ddd</d2x></d2c></cont>" ""
+
+# create
+new "create d1 part"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><edit-config><target><candidate/></target><default-operation>none</default-operation><config>
+<cont xmlns=\"urn:example:config\" xmlns:nc=\"${BASENS}\">
+<d1c><d11x nc:operation='create'>eee</d11x></d1c>
+</cont>
+</config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
+new "check d1"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><get-config><source><candidate/></source></get-config></rpc>" "<cont xmlns=\"urn:example:config\"><d1c><d11x>eee</d11x></d1c></cont>" ""
+
+# remove
+new "remove d2 part"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><edit-config><target><candidate/></target><default-operation>none</default-operation><config>
+<cont xmlns=\"urn:example:config\" xmlns:nc=\"${BASENS}\">
+<d2c><d2x nc:operation='remove'>ddd</d2x></d2c>
+</cont>
+</config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
+new "check d1"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><get-config><source><candidate/></source></get-config></rpc>" "<cont xmlns=\"urn:example:config\"><d1c><d11x>eee</d11x></d1c></cont>" ""
+
+# delete
+new "delete d2 part"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><edit-config><target><candidate/></target><default-operation>none</default-operation><config>
+<cont xmlns=\"urn:example:config\" xmlns:nc=\"${BASENS}\">
+<d2c><d2x nc:operation='delete'>ddd</d2x></d2c>
+</cont>
+</config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><rpc-error><error-type>application</error-type><error-tag>data-missing</error-tag><error-severity>error</error-severity><error-message>Data does not exist; cannot delete resource</error-message></rpc-error></rpc-reply>"
+
+if [ $BE -ne 0 ]; then
+    new "Kill backend"
+    # Check if premature kill
+    pid=$(pgrep -u root -f clixon_backend)
+    if [ -z "$pid" ]; then
+        err "backend already dead"
+    fi
+    # kill backend
+    stop_backend -f $cfg
+fi
+
+rm -rf $dir
+
+new "endtest"
+endtest