* Added sorting of YANG statements

* Some openconfig specs seem to have use/when before a "config" which it depends on. This leads to XML encoding being in the "wrong order.
  * When parsing, clixon now sorts container/list statements so that sub-statements with WHEN are put last.
  * See [Statements given in "load set" are order dependent](https://github.com/clicon/clixon/issues/287)
* Fixed: [Statements given in "load set" are order dependent](https://github.com/clicon/clixon/issues/287)

CHANGELOG.md

@@ -1,4 +1,4 @@
-v# Clixon Changelog
+# Clixon Changelog
 
 * [5.4.0](#540) Expected: November
 * [5.3.0](#530) 27 September 2021
@@ -89,6 +89,10 @@ Developers may need to change their code
 
 ### Minor features
 
+* Added sorting of YANG statements
+  * Some openconfig specs seem to have use/when before a "config" which it depends on. This leads to XML encoding being in the "wrong order.
+  * When parsing, clixon now sorts container/list statements so that sub-statements with WHEN are put last.
+  * See [Statements given in "load set" are order dependent](https://github.com/clicon/clixon/issues/287)
 * Added: [Recursive search CLIXON_YANG_DIR](https://github.com/clicon/clixon/issues/284)
 * Plugin context check before and after all callbacks.
   * Check blocked signals and signal handlers
@@ -96,6 +100,7 @@ Developers may need to change their code
   * Any changes to context are logged at loglevel WARNING
 * [OpenConfig path compression](https://github.com/clicon/clixon/pull/276)
 * C API: Added set/get pointer API to clixon_data:
+   * Added json/cli support for cli save/load
    * clicon_ptr_get(), clicon_ptr_set(), 
 * Restconf YANG PATCH according to RFC 8072
   * Changed YANG PATCH enabling:
@@ -105,6 +110,8 @@ Developers may need to change their code
 
 ### Corrected Bugs
 
+* [Statements given in "load set" are order dependent](https://github.com/clicon/clixon/issues/287)
+  * Modify ordering of XML encoding to put sub-elements with YANG WHEN statements last
 * [RPC get-conf method returned some content not specified by select filter](https://github.com/clicon/clixon/issues/281)
   * Bug introduced when upgrading of list pagination
 * [type leafref in type union ineffective](https://github.com/clicon/clixon/issues/277)

apps/cli/cli_auto.c

@@ -142,7 +142,7 @@ tleaf(cxobj *x)
     return (xml_child_nr_notype(xc, CX_ATTR) == 0);
 }
 
-/*! Print an XML tree structure to an output stream and encode chars "<>&"
+/*! Print an XML tree structure from an auto-cli env to stdout and encode chars "<>&"
  *
  * @param[in]   xn          clicon xml tree
  * @param[in]   level       how many spaces to insert before each line
@@ -322,6 +322,7 @@ cli_xml2txt(cxobj            *xn,
  * @param[in] prepend  Print this text in front of all commands.
  * @param[in] gt       option to steer cli syntax
  * @param[in] fn       Callback to make print function
+ * @see xml2cli
  */
 int
 cli_xml2cli(cxobj             *xn,

apps/cli/cli_common.c

@@ -772,8 +772,11 @@ compare_dbs(clicon_handle h,
  * Utility function used by cligen spec file
  * @param[in] h     CLICON handle
  * @param[in] cvv   Vector of variables (where <varname> is found)
- * @param[in] argv  A string: "<varname> (merge|replace)" 
+ * @param[in] argv  A string: "<varname> <operation> [<format>]"
  *   <varname> is name of a variable occuring in "cvv" containing filename
+ *   <operation> : merge or replace
+ *   <format>  "text"|"xml"|"json"|"cli"|"netconf" (see format_enum)
+ * 
  * @note that "filename" is local on client filesystem not backend. 
  * @note file is assumed to have a dummy top-tag, eg <clicon></clicon>
  * @code
@@ -798,14 +801,21 @@ load_config_file(clicon_handle h,
     cxobj      *xt = NULL;
     cxobj      *x;
     cbuf       *cbxml;
+    char              *formatstr;
+    enum format_enum   format = FORMAT_XML;
 
-    if (cvec_len(argv) != 2){
+    if (cvec_len(argv) < 2 || cvec_len(argv) > 4){
-	if (cvec_len(argv)==1)
+	clicon_err(OE_PLUGIN, EINVAL, "Received %d arguments. Expected: <dbname>,<varname>[,<format>]",
-	    clicon_err(OE_PLUGIN, EINVAL, "Got single argument:\"%s\". Expected \"<varname>,<op>\"", cv_string_get(cvec_i(argv,0)));
+		   cvec_len(argv));
-	else
-	    clicon_err(OE_PLUGIN, EINVAL, "Got %d arguments. Expected: <varname>,<op>", cvec_len(argv));
 	goto done;
     }
+    if (cvec_len(argv) > 2){
+	formatstr = cv_string_get(cvec_i(argv, 2));
+	if ((int)(format = format_str2int(formatstr)) < 0){
+	    clicon_err(OE_PLUGIN, 0, "Not valid format: %s", formatstr);
+	    goto done;
+	}
+    }
     varstr = cv_string_get(cvec_i(argv, 0));
     opstr  = cv_string_get(cvec_i(argv, 1));
     if (strcmp(opstr, "merge") == 0) 
@@ -830,8 +840,20 @@ load_config_file(clicon_handle h,
 	clicon_err(OE_UNIX, errno, "open(%s)", filename);
 	goto done;
     }
+    switch (format){
+    case FORMAT_XML:
 	if (clixon_xml_parse_file(fp, YB_NONE, NULL, &xt, NULL) < 0)
 	    goto done;
+	break;
+    case FORMAT_JSON:
+	if (clixon_json_parse_file(fp, YB_NONE, NULL, &xt, NULL) < 0)
+	    goto done;
+	break;
+    default:
+	clicon_err(OE_PLUGIN, 0, "format: %s not implemented", formatstr);
+	goto done;
+	break;
+    }
     if (xt == NULL)
 	goto done;
     if ((cbxml = cbuf_new()) == NULL)
@@ -858,13 +880,15 @@ load_config_file(clicon_handle h,
     return ret;
 }
 
-/*! Copy database to local file 
+/*! Copy database to local file as XMLn
+ *
  * Utility function used by cligen spec file
  * @param[in] h     CLICON handle
  * @param[in] cvv  variable vector (containing <varname>)
- * @param[in] argv  a string: "<dbname> <varname>" 
+ * @param[in] argv  a string: "<dbname> <varname> [<format>]" 
  *   <dbname>  is running, candidate, or startup
  *   <varname> is name of cligen variable in the "cvv" vector containing file name
+ *   <format>  "text"|"xml"|"json"|"cli"|"netconf" (see format_enum)
  * Note that "filename" is local on client filesystem not backend.
  * The function can run without a local database
  * @note The file is saved with dummy top-tag: clicon: <clicon></clicon>
@@ -884,27 +908,35 @@ save_config_file(clicon_handle h,
     char              *dbstr;
     char              *varstr;
     cxobj             *xt = NULL;
+    cxobj             *xc;
     cxobj             *xerr;
     FILE              *f = NULL;
+    enum genmodel_type gt;
+    char              *formatstr;
+    enum format_enum   format = FORMAT_XML;
+    char              *prefix = "set "; /* XXX hardcoded */
+    int                pretty = 1;     /* XXX hardcoded */
 
-    if (cvec_len(argv) != 2){
+    if (cvec_len(argv) < 2 || cvec_len(argv) > 4){
-	if (cvec_len(argv)==1)
+	clicon_err(OE_PLUGIN, EINVAL, "Received %d arguments. Expected: <dbname>,<varname>[,<format>]",
-	    clicon_err(OE_PLUGIN, EINVAL, "Got single argument:\"%s\". Expected \"<dbname>,<varname>\"",
-		       cv_string_get(cvec_i(argv,0)));
-	else
-	    clicon_err(OE_PLUGIN, EINVAL, " Got %d arguments. Expected: <dbname>,<varname>",
 		   cvec_len(argv));
-
 	goto done;
     }
+    if (cvec_len(argv) > 2){
+	formatstr = cv_string_get(cvec_i(argv, 2));
+	if ((int)(format = format_str2int(formatstr)) < 0){
+	    clicon_err(OE_PLUGIN, 0, "Not valid format: %s", formatstr);
+	    goto done;
+	}
+    }
     dbstr = cv_string_get(cvec_i(argv, 0));
-    varstr  = cv_string_get(cvec_i(argv, 1));
     if (strcmp(dbstr, "running") != 0 && 
 	strcmp(dbstr, "candidate") != 0 &&
 	strcmp(dbstr, "startup") != 0) {
 	clicon_err(OE_PLUGIN, 0, "No such db name: %s", dbstr);	
 	goto done;
     }
+    varstr  = cv_string_get(cvec_i(argv, 1));
     if ((cv = cvec_find(cvv, varstr)) == NULL){
 	clicon_err(OE_PLUGIN, 0, "No such var name: %s", varstr);	
 	goto done;
@@ -929,8 +961,33 @@ save_config_file(clicon_handle h,
 	clicon_err(OE_CFG, errno, "Creating file %s", filename);
 	goto done;
     } 
-    if (clicon_xml2file(f, xt, 0, 1) < 0)
+    switch (format){
+    case FORMAT_XML:
+	if (clicon_xml2file(f, xt, 0, pretty) < 0)
 	    goto done;
+	break;
+    case FORMAT_JSON:
+	if (xml2json(f, xt, pretty) < 0)
+	    goto done;
+	break;
+    case FORMAT_TEXT:
+	cli_xml2txt(xt, cligen_output, 0); /* tree-formed text */
+	break;
+    case FORMAT_CLI:
+	if ((gt = clicon_cli_genmodel_type(h)) == GT_ERR)
+	    goto done;
+	if (xml2cli(f, xt, prefix, gt) < 0)
+	    goto done;
+	break;
+    case FORMAT_NETCONF:
+	fprintf(f, "<rpc xmlns=\"%s\"><edit-config><target><candidate/></target><config>",
+		NETCONF_BASE_NAMESPACE);
+	fprintf(f, "\n");
+	if (clicon_xml2file(f, xt, 0, pretty) < 0)
+	    goto done;
+	fprintf(f, "</config></edit-config></rpc>]]>]]>\n");
+	break;
+    } /* switch */
     retval = 0;
     /* Fall through */
   done:

apps/cli/cli_show.c

@@ -442,7 +442,7 @@ cli_show_config1(clicon_handle h,
     char            *prefix = NULL;
     
     if (cvec_len(argv) < 3 || cvec_len(argv) > 5){
-	clicon_err(OE_PLUGIN, EINVAL, "Got %d arguments. Expected: <dbname>,<format>,<xpath>[,<namespace>, [<prefix>]]", cvec_len(argv));
+	clicon_err(OE_PLUGIN, EINVAL, "Received %d arguments. Expected: <dbname>,<format>,<xpath>[,<namespace>, [<prefix>]]", cvec_len(argv));
 
 	goto done;
     }

example/main/example_cli.cli

@@ -95,10 +95,20 @@ show("Show a particular state of the system"){
     }
 }
 
-save("Save candidate configuration to XML file") <filename:string>("Filename (local filename)"), save_config_file("candidate","filename");
+save("Save candidate configuration to XML file") <filename:string>("Filename (local filename)"), save_config_file("candidate","filename", "xml");{
+    cli("Save configuration as CLI commands"), save_config_file("candidate","filename", "cli");
+    xml("Save configuration as XML"), save_config_file("candidate","filename", "xml");
+    json("Save configuration as JSON"), save_config_file("candidate","filename", "json");
+}
 load("Load configuration from XML file") <filename:string>("Filename (local filename)"),load_config_file("filename", "replace");{
-    replace("Replace candidate with file contents"), load_config_file("filename", "replace");
+    replace("Replace candidate with file contents"), load_config_file("filename", "replace");{
-    merge("Merge file with existent candidate"), load_config_file("filename", "merge");
+	xml("Replace candidate with file containing XML"), load_config_file("","filename", "replace", "xml");
+	json("Replace candidate with file containing JSON"), load_config_file("","filename", "replace", "json");
+    }
+    merge("Merge file with existent candidate"), load_config_file("filename", "merge");{
+	xml("Merge candidate with file containing XML"), load_config_file("","filename", "merge", "xml");
+	json("Merge candidate with file containing JSON"), load_config_file("","filename", "merge", "json");
+    }
 }
 example("This is a comment") <var:int32>("Just a random number"), mycallback("myarg");
 rpc("example rpc") <a:string>("routing instance"), example_client_rpc("");

include/clixon_custom.h

@@ -128,3 +128,14 @@
  * Consider removing this option after 5.4
  */
 #undef JSON_CDATA_STRIP
+
+/*! 
+ * RFC 7950 generally does not specify an XML/JSON encoding order of sub-elements of list or
+ * containers. See sections 7.5.7 and 7.8.5
+ * There are some exceptions, eg rpc/action input/output or list key elements
+ * Clixon by default encodes them in yang order.
+ * Set this option if you want sub-elements with WHEN sub-statements last
+ * See https://github.com/clicon/clixon/issues/287
+ * Consider enabling this option permanently after 5.4 
+ */
+#define YANG_ORDERING_WHEN_LAST

lib/clixon/clixon_yang.h

@@ -271,5 +271,6 @@ int        yang_type_cache_set(yang_stmt *ys, yang_stmt *resolved, int options,
 			       cvec *patterns, uint8_t fraction);
 yang_stmt *yang_anydata_add(yang_stmt *yp, char *name);
 int        yang_extension_value(yang_stmt *ys, char *name, char *ns, int *exist, char **value);
+int        yang_sort_subelements(yang_stmt *ys);
 
 #endif  /* _CLIXON_YANG_H_ */

lib/src/clixon_datastore_write.c

@@ -207,6 +207,12 @@ check_body_namespace(cxobj     *x0,
  *
  * Check if there is a when condition. First try it on the new request (x1), then on the
  * existing (x0). 
+ * This is according to RFC 7950 8.3.2 NETCONF <edit-config> Processing
+ *   During this processing [of edit-config] :
+ *   o  Modification requests for nodes tagged with "when", and the "when"
+ *      condition evaluates to "false".  In this case, the server MUST
+ *      reply with an "unknown-element" <error-tag> in the <rpc-error>.
+ * This is somewhat strange since this is different from checking "when" at validation
  * @param[in]  x0p      Parent of x0
  * @param[in]  x1       XML tree which modifies base
  * @param[in]  y0       Yang spec corresponding to xml-node x0. NULL if x0 is NULL

lib/src/clixon_yang.c

@@ -766,7 +766,6 @@ ys_dup(yang_stmt *old)
  *
  * @param[in] yorig  Existing yang statement
  * @param[in] yfrom   New empty (but created) yang statement
-
  * @retval    0     OK
  * @retval    -1    Error
  * @code
@@ -1373,6 +1372,7 @@ yang_order(yang_stmt *y)
 
     if (y == NULL)
 	return -1;
+
     /* Some special handling if yp is choice (or case)
      * if so, the real parent (from an xml point of view) is the parents
      * parent. 
@@ -3631,6 +3631,53 @@ yang_extension_value(yang_stmt *ys,
     return retval;
 }
 
+#ifdef YANG_ORDERING_WHEN_LAST
+/* Sort substatements with when:s last */
+static int
+yang_sort_subelements_fn(const void* arg1,
+			 const void* arg2)
+{
+    yang_stmt *ys1 = *(yang_stmt **)arg1;
+    yang_stmt *ys2 = *(yang_stmt **)arg2;
+    int        w1; /* ys1 has when substatement */
+    int        w2;
+    
+    w1 = yang_find(ys1, Y_WHEN, NULL) != NULL;
+    w2 = yang_find(ys2, Y_WHEN, NULL) != NULL;
+    if (w1 == w2)
+	return 0;
+    if (w1)
+	return 1;
+    if (w2)
+	return -1;
+    return 0;
+}
+#endif
+
+/*! Experimental code for sorting YANG children
+ *
+ * RFC 7950 7.5.7 and 7.8.5 says that containers and list sub elements are encoded in any order.
+ * with some exceptions, eg rpc/action input/output or list key elements
+ *
+ * Clixon by default encodes them in yang order. But this function can change that.
+ * For example, openconfig-network-instances.yang requires subelements with "when" statements last.
+ */
+int
+yang_sort_subelements(yang_stmt *ys)
+{
+    int retval = -1;
+
+#ifdef YANG_ORDERING_WHEN_LAST
+    if ((yang_keyword_get(ys) == Y_CONTAINER ||
+	 yang_keyword_get(ys) == Y_LIST)){
+	qsort(ys->ys_stmt, ys->ys_len, sizeof(ys), yang_sort_subelements_fn);
+    }
+#endif
+    retval = 0;
+    // done:
+    return retval;
+}
+
 #ifdef XML_EXPLICIT_INDEX
 /*! Mark element as search_index in list
  * @retval     0   OK

lib/src/clixon_yang_parse.y

@@ -255,6 +255,8 @@ ystack_pop(clixon_yang_yacc *yy)
 	clicon_err(OE_YANG, 0, "ystack is NULL");
 	return -1;
     }
+    if (yang_sort_subelements(ystack->ys_node) < 0)
+	return -1;
     yy->yy_stack = ystack->ys_next;
     free(ystack);
     return 0;

test/test_cli.sh

@@ -110,13 +110,13 @@ new "cli commit"
 expectpart "$($clixon_cli -1 -f $cfg -l o commit)" 0 "^$"
 
 new "cli save"
-expectpart "$($clixon_cli -1 -f $cfg -l o save /tmp/foo)" 0 "^$"
+expectpart "$($clixon_cli -1 -f $cfg -l o save $dir/foo)" 0 "^$"
 
 new "cli delete all"
 expectpart "$($clixon_cli -1 -f $cfg -l o delete all)" 0 "^$"
 
 new "cli load"
-expectpart "$($clixon_cli -1 -f $cfg -l o load /tmp/foo)" 0 "^$"
+expectpart "$($clixon_cli -1 -f $cfg -l o load $dir/foo)" 0 "^$"
 
 new "cli check load"
 expectpart "$($clixon_cli -1 -f $cfg -l o show conf cli)" 0 "interfaces interface eth/0/0 ipv4 enabled true"