From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Date: Tue, 16 Oct 2018 11:39:06 +0300 From: Vladimir Davydov Subject: Re: [PATCH v4 09/14] lib: introduce json_path_normalize routine Message-ID: <20181016083906.vcapdiopyg2k6oqk@esperanza> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: To: Kirill Shcherbatov Cc: tarantool-patches@freelists.org List-ID: On Thu, Oct 11, 2018 at 10:58:54AM +0300, Kirill Shcherbatov wrote: > Introduced a new routine json_path_normalize that makes a > conversion of JSON path to the 'canonical' form: > - all maps keys are specified with operator ["key"] form > - all array indexes are specified with operator [i] form. > This notation is preferable because in the general case it can > be uniquely parsed. > We need such API in JSON indexes patch to store all paths in > 'canonical' form to commit the path uniqueness checks and > to tune access with JSON path hashtable. > > Need for #1012. Nit: Needed for #1012 > --- > src/lib/json/path.c | 25 +++++++++++++++++++++++++ > src/lib/json/path.h | 18 ++++++++++++++++++ > test/unit/json_path.c | 41 ++++++++++++++++++++++++++++++++++++++++- > test/unit/json_path.result | 14 +++++++++++++- > 4 files changed, 96 insertions(+), 2 deletions(-) > > diff --git a/src/lib/json/path.c b/src/lib/json/path.c > index 2e72930..0eb5d49 100644 > --- a/src/lib/json/path.c > +++ b/src/lib/json/path.c > @@ -242,3 +242,28 @@ json_path_next(struct json_path_parser *parser, struct json_path_node *node) > return json_parse_identifier(parser, node); > } > } > + > +int > +json_path_normalize(const char *path, uint32_t path_len, char *out) > +{ > + struct json_path_parser parser; > + struct json_path_node node; > + json_path_parser_create(&parser, path, path_len); > + int rc; > + while ((rc = json_path_next(&parser, &node)) == 0 && > + node.type != JSON_PATH_END) { > + if (node.type == JSON_PATH_NUM) { > + out += sprintf(out, "[%llu]", > + (unsigned long long)node.num); > + } else if (node.type == JSON_PATH_STR) { > + out += sprintf(out, "[\"%.*s\"]", node.len, node.str); Using sprintf here is unsafe, because it can write beyond the buffer boundaries. The API should be reworked so that the output buffer length is passed explicitly and the function returns the number of printed characters, in an snprintf-like manner. You might want to use SNPRINT helper macro. BTW, it may be worth renaming json_path_node to json_path_token and json_path_parser to json_path_tokenizer so as not to confuse path tokens with tree nodes. > + } else { > + unreachable(); > + } > + }; > + if (rc != 0) > + return rc; > + *out = '\0'; > + assert(node.type == JSON_PATH_END); > + return 0; > +} > diff --git a/src/lib/json/path.h b/src/lib/json/path.h > index c3c381a..f6b2ee2 100644 > --- a/src/lib/json/path.h > +++ b/src/lib/json/path.h > @@ -105,6 +105,24 @@ json_path_parser_create(struct json_path_parser *parser, const char *src, > int > json_path_next(struct json_path_parser *parser, struct json_path_node *node); > > +/** > + * Convert path to the 'canonical' form: > + * - all maps keys are specified with operator ["key"] form > + * - all array indexes are specified with operator [i] form. > + * This notation is preferable because in the general case it can > + * be uniquely parsed. > + * @param path Source path string to be converted. > + * @param path_len The length of the @path. > + * @param[out] out Memory to store normalized string. > + * The worst-case scenario require > + * 2.5 * path_len + 1 buffer. > + * @retval 0 On success. > + * @retval > 0 Position of a syntax error. A position is 1-based > + * and starts from a beginning of a source string. > + */ > +int > +json_path_normalize(const char *path, uint32_t path_len, char *out); > + > #ifdef __cplusplus > } > #endif > diff --git a/test/unit/json_path.c b/test/unit/json_path.c > index 9a1de06..775b0b1 100644 > --- a/test/unit/json_path.c > +++ b/test/unit/json_path.c > @@ -360,15 +360,54 @@ test_tree() > footer(); > } > > +void > +test_normalize_path() > +{ > + header(); > + plan(8); > + > + const char *path_normalized = "[\"FIO\"][3][\"fname\"]"; > + const char *path1 = "FIO[3].fname"; > + const char *path2 = "[\"FIO\"][3].fname"; > + const char *path3 = "FIO[3][\"fname\"]"; > + char buff[strlen(path_normalized) + 1]; > + int rc; > + > + rc = json_path_normalize(path_normalized, strlen(path_normalized), > + buff); > + is(rc, 0, "normalize '%s' path status", path_normalized); > + is(strcmp(buff, path_normalized), 0, "normalize '%s' path compare", > + path_normalized); > + > + rc = json_path_normalize(path1, strlen(path1), buff); > + is(rc, 0, "normalize '%s' path status", path1); > + is(strcmp(buff, path_normalized), 0, "normalize '%s' path compare", > + path1); > + > + rc = json_path_normalize(path2, strlen(path2), buff); > + is(rc, 0, "normalize '%s' path status", path2); > + is(strcmp(buff, path_normalized), 0, "normalize '%s' path compare", > + path2); > + > + rc = json_path_normalize(path3, strlen(path3), buff); > + is(rc, 0, "normalize '%s' path status", path3); > + is(strcmp(buff, path_normalized), 0, "normalize '%s' path compare", > + path3); I guess, you should test invalid paths as well so as to cover error paths.