Fixed: [YANG ordering fails for nested choice and action](YANG ordering fails for nested choice and action)

CHANGELOG.md

@@ -52,6 +52,7 @@ Expected: September 2022
 
 ### Corrected Bugs
 
+* Fixed: [YANG ordering fails for nested choice and action](YANG ordering fails for nested choice and action)
 * Fixed: [YANG min-elements within non-presence container does not work](https://github.com/clicon/clixon/issues/355)
 * Fixed: [Issues with ietf-snmp modules](https://github.com/clicon/clixon/issues/353)
 * Fixed: [Missing/no namespace error in YANG augments with default values](https://github.com/clicon/clixon/issues/354)

lib/src/clixon_xml_sort.c

@@ -183,18 +183,17 @@ xml_cv_cache_clear(cxobj *xt)
  * This is useful in searching for indexes in trees, where x1 is a search index and not a 
  * complete tree.
  *
- * @note empty value/NULL is smallest value
+ * "Comparing" x1 and x2 here determines equality from a structural (model)
- * @note some error cases return as -1 (qsort cant handle errors)
- * @note some error cases return as -1 (qsort cant handle errors)
- * 
- * @note "comparing" x1 and x2 here judges equality from a structural (model)
  * perspective, ie both have the same yang spec, if they are lists, they have the
  * the same keys. NOT that the values are equal!
- * In other words, if x is a leaf with the same yang spec, <x>1</x> and <x>2</x> are
+ * In other words, if x is a leaf with the same yang spec, <x>1</x> and <x>2</x> are "equal". 
- * "equal". 
  * If x is a list element (with key "k"), 
  *    <x><k>42</42><y>foo</y></x> and <x><k>42</42><y>bar</y></x> are equal, 
  * but is not equal to <x><k>71</42><y>bar</y></x>
+ * 
+ * @note empty value/NULL is smallest value
+ * @note some error cases return as -1 (qsort cant handle errors) (which?)
+ *     - yang_order() is one
  */
 int
 xml_cmp(cxobj  *x1,
@@ -256,6 +255,7 @@ xml_cmp(cxobj  *x1,
         goto done;
     }
     if (y1 != y2){ 
+	/* XXX handle errors */
 	yi1 = yang_order(y1);
 	yi2 = yang_order(y2);
 	if ((equal = yi1-yi2) != 0)
@@ -388,7 +388,7 @@ xml_cmp(cxobj  *x1,
 }
 
 /*!
- * @note args are pointer ot pointers, to fit into qsort cmp function
+ * @note args are pointer to pointers, to fit into qsort cmp function
  */
 static int
 xml_cmp_qsort(const void* arg1, 
@@ -476,11 +476,14 @@ xml_find_keys_notsorted(cxobj   *xp,
     int        i;
     cxobj     *xc;
     yang_stmt *yc;
+    int        yi;
     
     for (i=mid+1; i<xml_child_nr(xp); i++){ /* First increment */
 	xc = xml_child_i(xp, i);
 	yc = xml_spec(xc);
-	if (yangi != yang_order(yc)) /* wrong yang */
+	if ((yi = yang_order(yc)) < -1)
+	    goto done;
+	if (yangi != yi) /* wrong yang */
 	    break;
 	if (xml_cmp(xc, x1, 0, skip1, NULL) == 0){
 	    if (clixon_xvec_append(xvec, xc) < 0)
@@ -491,7 +494,9 @@ xml_find_keys_notsorted(cxobj   *xp,
     for (i=mid-1; i>=0; i--){ /* Then decrement */
 	xc = xml_child_i(xp, i);
 	yc = xml_spec(xc);
-	if (yangi != yang_order(yc)) /* wrong yang */
+	if ((yi = yang_order(yc)) < -1)
+	    goto done;
+	if (yangi != yi) /* wrong yang */
 	    break;
 	if (xml_cmp(xc, x1, 0, skip1, NULL) == 0){
 	    if (clixon_xvec_append(xvec, xc) < 0)
@@ -528,11 +533,14 @@ search_multi_equals(cxobj  **childvec,
     int        i;
     cxobj     *xc;
     yang_stmt *yc;
+    int        yi;
     
     for (i=mid-1; i>=0; i--){ /* First decrement */
 	xc = childvec[i];
 	yc = xml_spec(xc);
-	if (yangi != yang_order(yc)) /* wrong yang */
+	if ((yi = yang_order(yc)) < -1)
+	    goto done;
+	if (yangi != yi) /* wrong yang */
 	    break;
 	if (xml_cmp(x1, xc, 0, skip1, NULL) != 0)
 	    break;
@@ -542,7 +550,9 @@ search_multi_equals(cxobj  **childvec,
     for (i=mid+1; i<childlen; i++){ /* Then increment */
 	xc = childvec[i];
 	yc = xml_spec(xc);
-	if (yangi != yang_order(yc)) /* wrong yang */
+	if ((yi = yang_order(yc)) < -1)
+	    goto done;
+	if (yangi != yi) /* wrong yang */
 	    break;
 	if (xml_cmp(x1, xc, 0, skip1, NULL) != 0)
 	    break;
@@ -569,11 +579,14 @@ search_multi_equals_xvec(clixon_xvec  *childvec,
     int        i;
     cxobj     *xc;
     yang_stmt *yc;
+    int        yi;
     
     for (i=mid-1; i>=0; i--){ /* First decrement */
 	xc = clixon_xvec_i(childvec, i);
 	yc = xml_spec(xc);
-	if (yangi != yang_order(yc)) /* wrong yang */
+	if ((yi = yang_order(yc)) < -1)
+	    goto done;
+	if (yangi != yi) /* wrong yang */
 	    break;
 	if (xml_cmp(x1, xc, 0, skip1, NULL) != 0)
 	    break;
@@ -583,7 +596,9 @@ search_multi_equals_xvec(clixon_xvec  *childvec,
     for (i=mid+1; i<clixon_xvec_len(childvec); i++){ /* Then increment */
 	xc = clixon_xvec_i(childvec, i);
 	yc = xml_spec(xc);
-	if (yangi != yang_order(yc)) /* wrong yang */
+	if ((yi = yang_order(yc)) < -1)
+	    goto done;
+	if (yangi != yi) /* wrong yang */
 	    break;
 	if (xml_cmp(x1, xc, 0, skip1, NULL) != 0)
 	    break;
@@ -734,6 +749,7 @@ xml_search_binary(cxobj   *xp,
     int        cmp;
     cxobj     *xc;
     yang_stmt *y;
+    int        yi;
 
     if (upper < low)
     	goto ok;
@@ -743,7 +759,9 @@ xml_search_binary(cxobj   *xp,
     xc = xml_child_i(xp, mid);
     if ((y = xml_spec(xc)) == NULL)
 	goto ok;
-    cmp = yangi-yang_order(y);
+    if ((yi = yang_order(y)) < -1)
+	goto done;
+    cmp = yangi - yi;
     /* Here is right yang order == same yang? */
     if (cmp == 0){
 #ifdef XML_EXPLICIT_INDEX
@@ -827,8 +845,8 @@ xml_search_yang(cxobj       *xp,
 #endif
 	if (yang_keyword_get(yc) == Y_LIST || yang_keyword_get(yc) == Y_LEAF_LIST)
 	    sorted = (yang_find(yc, Y_ORDERED_BY, "user") == NULL);
-    yangi = yang_order(yc);
+    if ((yangi = yang_order(yc)) < -1)
-    
+	goto done;
     if (xml_search_binary(xp, x1, sorted, yangi, low, upper, skip1, indexvar, xvec) < 0)
 	goto done;
     retval = 0;
@@ -962,6 +980,7 @@ xml_insert2(cxobj           *xp,
     int        cmp;
     cxobj     *xc;
     yang_stmt *yc;
+    int        yi;
     
     if (low > upper){  /* beyond range */
 	clicon_err(OE_XML, 0, "low>upper %d %d", low, upper);	
@@ -994,7 +1013,9 @@ xml_insert2(cxobj           *xp,
 	    cmp = xml_cmp(xn, xc, 0, 0, NULL);
     }
     else{ /* Not equal yang - compute diff */
-	cmp = yni - yang_order(yc);
+	if ((yi = yang_order(yc)) < -1)
+	    goto done;
+	cmp = yni - yi;
 	/* One case is a choice where 
 	 * xc = <tcp/>, xn = <udp/>
 	 * same order but different yang spec
@@ -1072,7 +1093,8 @@ xml_insert(cxobj           *xp,
 #endif
 	if (yang_keyword_get(y) == Y_LIST || yang_keyword_get(y) == Y_LEAF_LIST)
 	    userorder = (yang_find(y, Y_ORDERED_BY, "user") != NULL);
-    yi = yang_order(y);
+    if ((yi = yang_order(y)) < -1)
+	goto done;
     if ((i = xml_insert2(xp, xi, y, yi,
 			 userorder, ins, key_val, nsc_key,
 			 low, upper)) < 0)

lib/src/clixon_yang.c

@@ -1466,7 +1466,7 @@ yang_choice(yang_stmt *y)
  * In (2) we increment with only 1.
  */
 static int
-order1_choice(yang_stmt *yp,
+yang_order1_choice(yang_stmt *yp,
 		   yang_stmt *y,
 		   int       *index)
 {
@@ -1474,31 +1474,37 @@ order1_choice(yang_stmt *yp,
     yang_stmt  *yc;
     int         i;
     int         j;
-    int         shortcut=0;
     int         max=0;
+    int         index0;
 
+    index0 = *index;
     for (i=0; i<yp->ys_len; i++){ /* Loop through choice */
 	ys = yp->ys_stmt[i];
 	if (ys->ys_keyword == Y_CASE){ /* Loop through case */
+	    *index = index0;
 	    for (j=0; j<ys->ys_len; j++){
 		yc = ys->ys_stmt[j];
-		if (yang_datanode(yc) && yc == y){
+		if (yc->ys_keyword == Y_CHOICE){
-		    *index += j;
+		    if (yang_order1_choice(yc, y, index) == 1) /* If one of the choices is "y" */
 			return 1;
 		}
+		else {
+		    if (yang_datanode(yc) && yc == y){
+			//			*index = index0 + j;
+			return 1;
 		    }
-	    if (j>max)
+		    (*index)++;
-		max = j;
+		}
+	    }
+	    if (*index-index0 > max)
+		max = *index-index0;
 	}
 	else {
-	    shortcut = 1;   /* Shortcut, no case */
+	    max = 1;   /* Shortcut, no case */
 	    if (yang_datanode(ys) && ys == y) 
 		return 1;
 	}
     }
-    if (shortcut)
-	(*index)++;
-    else
     *index += max;
     return 0;
 }
@@ -1507,44 +1513,52 @@ order1_choice(yang_stmt *yp,
  * @param[in]  yp     Parent
  * @param[in]  y      Yang datanode to find
  * @param[out] index  Index of y in yp:s list of children
- * @retval     0      not found (must be datanode)
+ * @retval     0      OK: found
- * @retval     1      found
+ * @retval     -1     Error: not found (must be datanode)
  */
 static int
-order1(yang_stmt *yp,
+yang_order1(yang_stmt *yp,
 	    yang_stmt *y,
 	    int       *index)
 {
+    int         retval = -1;
     yang_stmt  *ys;
     int         i;
     
     for (i=0; i<yp->ys_len; i++){
 	ys = yp->ys_stmt[i];
 	if (ys->ys_keyword == Y_CHOICE){
-	    if (order1_choice(ys, y, index) == 1) /* If one of the choices is "y" */
+	    if (yang_order1_choice(ys, y, index) == 1) /* If one of the choices is "y" */
-		return 1;
+		break;
 	}
 	else {
-	    if (!yang_datanode(ys))
+	    if (!yang_datanode(ys) &&
+		yang_keyword_get(ys) != Y_ACTION) /* action is special case */
 		continue;
 	    if (ys == y)
-		return 1;
+		break;
 	    (*index)++; 
 	}
     }
-    return 0;
+    if (i < yp->ys_len) /* break -> found */
+	retval = 0; 
+    else
+	assert(0); // XXX
+    return retval;
 }
 
 /*! Return order of yang statement y in parents child vector
  * @param[in]  y      Find position of this data-node
  * @param[out] index  Index of y in yp:s list of children
  * @retval   >=0      Order of child with specified argument
- * @retval    -1      Not found
+ * @retval    -1      No spec, y is NULL, which applies to eg attributes and are placed first
+ * @retval    -2      Error: Not found
  * @note special handling if y is child of (sub)module
  */
 int
 yang_order(yang_stmt *y)
 {
+    int         retval = -2;
     yang_stmt  *yp;
     yang_stmt  *ypp;
     yang_stmt  *ym;
@@ -1552,9 +1566,10 @@ yang_order(yang_stmt *y)
     int         j=0;
     int         tot = 0;
 
-    if (y == NULL)
+    if (y == NULL){
-	return -1;
+	retval = -1; 
-
+	goto done;
+    }
     /* Some special handling if yp is choice (or case)
      * if so, the real parent (from an xml point of view) is the parents
      * parent. 
@@ -1578,9 +1593,13 @@ yang_order(yang_stmt *y)
 	    tot += ym->ys_len;
 	}
     }
-    if (order1(yp, y, &j) == 1)
+    if (yang_order1(yp, y, &j) < 0){
-	return tot + j;
+	clicon_err(OE_YANG, 0, "YANG node %s not ordered: not found", yang_argument_get(y));
-    return -1;
+	goto done;
+    }
+    retval = tot + j;
+ done:
+    return retval;
 }
 
 char *

test/test_augment_default.sh

@@ -89,21 +89,18 @@ new "wait backend"
 wait_backend
 
 new "cli show config startup"
-#expectpart "$($clixon_cli -1 -f $cfg -l o show config xml)" 0 '<table xmlns="urn:example:clixon">' '<map xmlns="urn:example:augment">' '<aug:enable xmlns:aug="urn:example:augment">true</aug:enable>'
 expectpart "$($clixon_cli -1 -f $cfg -l o show config xml)" 0 '<table xmlns="urn:example:clixon">' '<map xmlns="urn:example:augment">' '<enable>true</enable>'
 
 new "cli delete map name"
 expectpart "$($clixon_cli -1 -f $cfg -l o delete table map name me)" 0 ""
 
 new "cli show config deleted"
-#expectpart "$($clixon_cli -1 -f $cfg -l o show config xml)" 0 '<table xmlns="urn:example:clixon">' '<aug:map xmlns:aug="urn:example:augment">' '<aug:enable>true</aug:enable>'
 expectpart "$($clixon_cli -1 -f $cfg -l o show config xml)" 0 '<table xmlns="urn:example:clixon">' '<map xmlns="urn:example:augment">' '<enable>true</enable>'
 
 new "cli set map name"
 expectpart "$($clixon_cli -1 -f $cfg -l o set table map name x)" 0 ""
 
 new "cli show config set"
-#expectpart "$($clixon_cli -1 -f $cfg -l o show config xml)" 0 '<table xmlns="urn:example:clixon">' '<map xmlns="urn:example:augment">' '<aug:enable xmlns:aug="urn:example:augment">true</aug:enable>'
 expectpart "$($clixon_cli -1 -f $cfg -l o show config xml)" 0 '<table xmlns="urn:example:clixon">' '<map xmlns="urn:example:augment">' '<enable>true</enable>'
 
 if [ $BE -ne 0 ]; then

test/test_choice.sh

@@ -158,6 +158,27 @@ module system{
         }
      }
   }
+  list mylist {
+     key k;
+     leaf k{
+        type string;
+     }
+     choice c {
+       case c1 {
+         leaf c1 {
+	    mandatory true;
+	    type string;
+	 }
+	 choice c1c {
+	    case c1c1 {
+	       leaf c1c1l {
+                 type string;
+ 	       }
+            }
+         }
+      }
+    }
+  }
 }
 EOF
 
@@ -181,6 +202,7 @@ discard("Discard edits (rollback 0)"), discard_changes();
 show("Show a particular state of the system"){
     configuration("Show configuration"), cli_auto_show("datamodel", "candidate", "text", true, false);{
 	    cli("Show configuration as CLI commands"), cli_auto_show("datamodel", "candidate", "cli", true, false, "set ");
+	    xml("Show configuration as XML"), cli_auto_show("datamodel", "candidate", "xml", true, false, NULL);
     }
 }
 EOF
@@ -438,6 +460,21 @@ expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS>
 new "netconf discard-changes"
 expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><discard-changes/></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
 
+new "set case mandatory + non"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><edit-config><target><candidate/></target><config><manleaf xmlns=\"urn:example:config\"><c1>a</c1><ccc1>b</ccc1></manleaf></config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
+new "netconf validate ok"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><validate><source><candidate/></source></validate></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
+new "netconf discard-changes"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><discard-changes/></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
+new "set list+mandatory case"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><edit-config><target><candidate/></target><config><mylist xmlns=\"urn:example:config\"><k>0</k><c1>x</c1><c1c1l>y</c1c1l></mylist></config></edit-config></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
+new "netconf validate ok"
+expecteof_netconf "$clixon_netconf -qf $cfg" 0 "$DEFAULTHELLO" "<rpc $DEFAULTNS><validate><source><candidate/></source></validate></rpc>" "" "<rpc-reply $DEFAULTNS><ok/></rpc-reply>"
+
 if [ $RC -ne 0 ]; then
     new "Kill restconf daemon"
     stop_restconf