<p> Attention is currently required from: N A, Joshua Colp. </p>
<p>Patch set 4:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4; color: #000000;">Code-Review -1</span></p><p><a href="https://gerrit.asterisk.org/c/asterisk/+/18012">View Change</a></p><p>2 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="null">File funcs/func_json.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18012/comment/53d1a2f3_05169c4e">Patch Set #4, Line 90:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">static int parse_node(char **key, char *currentkey, char *nestchar, int count, struct ast_json *json, char *buf, size_t len)<br>{<br> const char *result = NULL;<br> char *previouskey;<br> struct ast_json *jsonval = json;<br><br> snprintf(buf, len, "%s", ""); /* clear the buffer from previous round if necessary */<br> if (!json) { /* no error or warning should be thrown */<br> ast_debug(1, "Could not find key '%s' in parsed JSON\n", currentkey);<br> return -1;<br> }<br><br> switch(ast_json_typeof(jsonval)) {<br> unsigned long int size;<br> int r;<br> char *result2;<br><br> case AST_JSON_STRING:<br> ast_debug(1, "Got JSON string\n");<br> result = ast_json_string_get(jsonval);<br> snprintf(buf, len, "%s", result);<br> break;<br> case AST_JSON_INTEGER:<br> ast_debug(1, "Got JSON integer\n");<br> r = ast_json_integer_get(jsonval);<br> snprintf(buf, len, "%d", r); /* the snprintf below is mutually exclusive with this one */<br> break;<br> case AST_JSON_ARRAY:<br> ast_debug(1, "Got JSON array\n");<br> previouskey = currentkey;<br> currentkey = strsep(key, nestchar); /* retrieve the desired index */<br> size = ast_json_array_size(jsonval);<br> ast_debug(1, "Parsed JSON array of size %lu, key: %s\n", size, currentkey);<br> if (!currentkey) { /* this is the end, so just dump the array */<br> if (count) {<br> ast_debug(1, "No key on which to index in the array, so returning count: %lu\n", size);<br> snprintf(buf, len, "%lu", size);<br> return 0;<br> } else {<br> char *jsonarray;<br> ast_debug(1, "No key on which to index in the array, so dumping '%s' array\n", previouskey);<br> jsonarray = ast_json_dump_string(jsonval);<br> ast_copy_string(buf, jsonarray, len);<br> ast_json_free(jsonarray);<br> }<br> } else if (ast_str_to_int(currentkey, &r) || r < 0) {<br> ast_debug(1, "Requested index '%s' is not numeric or is invalid\n", currentkey);<br> } else if (r >= size) {<br> ast_debug(1, "Requested index '%d' does not exist in parsed array\n", r);<br> } else {<br> struct ast_json *json2 = ast_json_array_get(jsonval, r);<br> if (!json2) {<br> ast_debug(1, "Array index %d contains empty item\n", r);<br> return -1;<br> }<br> previouskey = currentkey;<br> currentkey = strsep(key, nestchar); /* get the next subkey */<br> ast_debug(1, "Recursing on index %d in array (key was '%s' and is now '%s')\n", r, previouskey, currentkey);<br> /* json2 is a borrowed ref. That's fine, since json won't get freed until recursing is over */<br> /* If there are keys remaining, then parse the next object we can get. Otherwise, just dump the child */<br> if (parse_node(key, currentkey, nestchar, count, currentkey ? ast_json_object_get(json2, currentkey) : json2, buf, len)) { /* recurse on this node */<br> return -1;<br> }<br> }<br> break;<br> default:<br> ast_debug(1, "Got generic JSON object\n");<br> result2 = ast_json_dump_string(jsonval);<br> snprintf(buf, len, "%s", result2);<br> ast_json_free(result2);<br> }<br> return 0;<br>}<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this function can be simplified, made more generic, and then moved into json.c.</p><p style="white-space: pre-wrap; word-wrap: break-word;">For example here is some code that will recursively search a given json object, looking for the first given "key". If found it returns the associated value as a json object (NOTE, untested):</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">static struct ast_json *ast_json_find_by_key(struct ast_json *obj, const char *key)<br>{<br> struct ast_json *res;<br> struct ast_json_iter *iter;<br> size_t i;</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> switch(ast_json_typeof(obj)) {<br> case AST_JSON_OBJECT:<br> /* Breadth first */<br> res = ast_json_object_get(obj, key);<br> if (res) {<br> return res;<br> }</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> iter = ast_json_object_iter(obj);<br> while ((iter = ast_json_object_iter_next(obj, iter))) {<br> res = ast_json_find_by_key(ast_json_object_iter_value(iter), key);<br> if (res) {<br> return res;<br> }<br> }</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> break;<br> case AST_JSON_ARRAY:<br> for (i = 0; i < ast_json_array_size(obj); ++i) {<br> res = ast_json_find_by_key(ast_json_array_get(obj, i), key);<br> if (res) {<br> return res;<br> }<br> }<br> break;<br> default:<br> return obj;<br> }</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> return NULL;<br>}</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Then from your code you can just do something like this:</p><p style="white-space: pre-wrap; word-wrap: break-word;">struct ast_json *val;<br>char s_val;</p><p style="white-space: pre-wrap; word-wrap: break-word;">val = ast_json_find_by_key(json, "hello");</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">if (!val) {<br> /* cleanup, log, and return */<br>}</pre><pre style="font-family: monospace,monospace; white-space: pre-wrap;">s_val = ast_json_dump_string(val);<br>if (!s_val) {<br> /* cleanup, log, and return */<br>}</pre><p style="white-space: pre-wrap; word-wrap: break-word;">snprintf(buf, len, "%s", s_val);<br>/* cleanup and return */</p><p style="white-space: pre-wrap; word-wrap: break-word;">Note too it'd be easy to add another function that calls the above to search over multiple keys.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Things to consider with this approach and your current one. Any time you recursively call another function you need to be careful not to "blow the stack". Since the JSON is coming from an external source someone may be able to craft the JSON in a way to crash your system. I'd probably add a max depth to the search to alleviate that problem.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.asterisk.org/c/asterisk/+/18012/comment/690061eb_8a802efe">Patch Set #4, Line 235:</a> <code style="font-family:monospace,monospace"> json = ast_json_load_str(str, NULL);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">There is no reason you should have to load the json multiple times. You can craft things in a way to search, and return a json object and then search into that object if multiple keys are given. See above comment.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.asterisk.org/c/asterisk/+/18012">change 18012</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.asterisk.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.asterisk.org/c/asterisk/+/18012"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: asterisk </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I603940b216a3911b498fc6583b18934011ef5d5b </div>
<div style="display:none"> Gerrit-Change-Number: 18012 </div>
<div style="display:none"> Gerrit-PatchSet: 4 </div>
<div style="display:none"> Gerrit-Owner: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Reviewer: Friendly Automation </div>
<div style="display:none"> Gerrit-Reviewer: Kevin Harwell <kharwell@digium.com> </div>
<div style="display:none"> Gerrit-CC: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Attention: N A <mail@interlinked.x10host.com> </div>
<div style="display:none"> Gerrit-Attention: Joshua Colp <jcolp@sangoma.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 08 Apr 2022 21:27:24 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>