From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 9ED2D2860B for ; Sun, 9 Sep 2018 19:42:58 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id 0tTdV7lis7rZ for ; Sun, 9 Sep 2018 19:42:58 -0400 (EDT) Received: from forward4j.cmail.yandex.net (forward4j.cmail.yandex.net [5.255.227.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id E271D2852C for ; Sun, 9 Sep 2018 19:42:57 -0400 (EDT) From: roman.habibov1@yandex.ru In-Reply-To: <20180909152804.4lagt5drkymriv5x@tkn_work_nb> References: <2952461532611162@iva5-08e20335b0d9.qloud-c.yandex.net> <33ec689b-9aca-97e9-e56f-c0aee649ad87@tarantool.org> <28658371533050946@myt4-74a8acfc13eb.qloud-c.yandex.net> <898281533156060@myt5-cf6d29327892.qloud-c.yandex.net> <26831531533678724@myt5-f9d71769b752.qloud-c.yandex.net> <6304221534202083@iva8-37fc2ad204cd.qloud-c.yandex.net> <20180909152804.4lagt5drkymriv5x@tkn_work_nb> Subject: [tarantool-patches] Re: [PATCH v3] json: add options to json.encode() MIME-Version: 1.0 Date: Mon, 10 Sep 2018 02:42:54 +0300 Message-Id: <31669101536536574@iva1-77b79d681cd9.qloud-c.yandex.net> Content-Transfer-Encoding: 7bit Content-Type: text/plain Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-subscribe: List-owner: List-post: List-archive: To: Alexander Turenko Cc: Vladislav Shpilevoy , "tarantool-patches@freelists.org" Hi! Thanks for review. >> > - /* >> > - * Update struct luaL_serializer using pointer to a >> > - * configuration value (all values must be `int` for that). >> > - */ > > Why this comment was stripped away? It seems to be still relevant to the > new code. Returned. >> > diff --git a/src/lua/utils.h b/src/lua/utils.h >> > index 6b057af3e..9e2353511 100644 >> > --- a/src/lua/utils.h >> > +++ b/src/lua/utils.h >> > @@ -240,6 +240,16 @@ luaL_checkserializer(struct lua_State *L) { >> > luaL_checkudata(L, lua_upvalueindex(1), LUAL_SERIALIZER); >> > } >> > +/** >> > + * Parse configuration table into @a cfg. Remove the lua table >> > + * from the top of lua stack. > > It seems it does not remove the table from a stack. Yes. Sentence deleted. >> > diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua >> > index 42c79d6e9..ce21bbfcf 100755 >> > --- a/test/app-tap/json.test.lua >> > +++ b/test/app-tap/json.test.lua >> > @@ -22,7 +22,53 @@ end >> > tap.test("json", function(test) >> > local serializer = require('json') >> > - test:plan(13) >> > + test:plan(25) >> > + >> > + -- gh-2888: Check the possibility of using options in encode()/decode(). >> > + >> > + local sub = {a = 1, { b = {c = 1, d = {e = 1}}}} > > Proposed to save encode_max_depth default value here, like so: > > local orig_encode_max_depth = serializer.cfg.encode_max_depth > >> > + serializer.cfg({encode_max_depth = 1}) >> > + test:ok(serializer.encode(sub) == '{"1":null,"a":1}', >> > + 'depth of encoding is 1 with .cfg') >> > + serializer.cfg({encode_max_depth = 2}) >> > + test:ok(serializer.encode(sub) == '{"1":{"b":null},"a":1}', >> > + 'depth of encoding is 2 with .cfg') > > And restore it here: > > serializer.cfg({encode_max_depth = orig_encode_max_depth}) Redone. > BTW, I think test case with {encode_max_depth = 2} is redundant. Removed. >> > + test:ok(serializer.encode(sub, {encode_max_depth = 1}) == '{"1":null,"a":1}', >> > + 'depth of encoding is 1 with .encode') >> > + > > Here we can check that json.encode(data, opts) did not change the global > configuration: > > test:is(serializer.cfg.encode_max_depth, orig_encode_max_depth, > 'global option remains unchanged') Added. > The same comments are applicable to other test cases. Redone. >> > + local nan = 1/0 >> > + test:ok(serializer.encode({a = nan}) == '{"a":inf}', >> > + 'default "encode_invalid_numbers"') >> > + serializer.cfg({encode_invalid_numbers = false}) >> > + test:ok(not pcall(serializer.encode, {a = nan}), >> > + 'expected error with NaN ecoding with .cfg') >> > + serializer.cfg({encode_invalid_numbers = true}) >> > + test:ok(not pcall(serializer.encode, {a = nan}, >> > + {encode_invalid_numbers = false}), >> > + 'expected error with NaN ecoding with .encode') >> > + > > ecoding -> encoding (two occurences) Fixed. >> > + test:ok(not pcall(serializer.decode, '{"1":{"b":{"c":1,"d":null}},"a":1}', >> > + {decode_max_depth = 2}), >> > + 'error: too many nested data structures') >> > + > > It is not obvious that gh-2888 block ends here. Please, add appropriate > name for the block below or move gh-2888 block below the unnamed block. Block is moved downwards. > Look also how gh-3514 block header is formatted. It would be good to > have all headers in the same style. Done. >> > diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c >> > index 11aa40225..e431c3fb0 100644 >> > --- a/third_party/lua-cjson/lua_cjson.c >> > +++ b/third_party/lua-cjson/lua_cjson.c >> > @@ -417,22 +417,25 @@ static void json_append_data(lua_State *l, struct luaL_serializer *cfg, >> > } >> > } >> > -static int json_encode(lua_State *l) >> > -{ >> > - struct luaL_serializer *cfg = luaL_checkserializer(l); >> > - char *json; >> > - int len; >> > +static int json_encode(lua_State *l) { >> > + luaL_argcheck(l, (lua_gettop(l) == 2) || (lua_gettop(l) == 1), >> > + 1, "expected 1 or 2 arguments"); >> > - luaL_argcheck(l, lua_gettop(l) == 1, 1, "expected 1 argument"); >> > - >> > - /* Reuse existing buffer */ >> > + /* Reuse existing buffer. */ >> > strbuf_reset(&encode_buf); >> > + struct luaL_serializer *cfg = luaL_checkserializer(l); >> > - json_append_data(l, cfg, 0, &encode_buf); >> > - json = strbuf_string(&encode_buf, &len); >> > - >> > - lua_pushlstring(l, json, len); >> > + if (lua_gettop(l) == 2) { >> > + struct luaL_serializer user_cfg = *cfg; >> > + luaL_serializer_parse_options(l, &user_cfg); >> > + lua_pop(l, 1); > > lua_pop is used here (I guessto remove the table with options from a > stack), but don't used in decode. Added to decode. >> > + json_append_data(l, &user_cfg, 0, &encode_buf); >> > + } else { >> > + json_append_data(l, cfg, 0, &encode_buf); >> > + } >> > + char *json = strbuf_string(&encode_buf, NULL); >> > + lua_pushlstring(l, json, encode_buf.length); > > encode_buf.length breaks incapsulation of strbuf 'object'. Please, use > strbuf_length function instead. Fixed. commit 0d9351dac8312a6fab4a80c2167b75a36589d031 Author: Roman Khabibov Date: Sun Jul 8 02:21:08 2018 +0300 json: add options to json.encode() Add an ability to pass options to json.encode()/decode(). Closes: #2888. @TarantoolBot document Title: json.encode() json.decode() Add an ability to pass options to json.encode() and json.decode(). These are the same options that are used globally in json.cfg(). diff --git a/src/lua/utils.c b/src/lua/utils.c index 2f0f4dc..653ed1c 100644 --- a/src/lua/utils.c +++ b/src/lua/utils.c @@ -215,6 +215,50 @@ static struct { }; /** + * Configure one field in @a cfg. + * @param L Lua stack. + * @param i Index of option in OPTIONS[]. + * @param cfg Serializer to inherit configuration. + * @retval Pointer to the value of option. + * @retval NULL if option is not in the table. + */ +static int * +luaL_serializer_parse_option(struct lua_State *L, int i, + struct luaL_serializer *cfg) +{ + lua_getfield(L, 2, OPTIONS[i].name); + if (lua_isnil(L, -1)) { + lua_pop(L, 1); + return NULL; + } + /* + * Update struct luaL_serializer using pointer to a + * configuration value (all values must be `int` for that). + */ + int *pval = (int *) ((char *) cfg + OPTIONS[i].offset); + switch (OPTIONS[i].type) { + case LUA_TBOOLEAN: + *pval = lua_toboolean(L, -1); + break; + case LUA_TNUMBER: + *pval = lua_tointeger(L, -1); + break; + default: + unreachable(); + } + lua_pop(L, 1); + return pval; +} + +void +luaL_serializer_parse_options(struct lua_State *L, + struct luaL_serializer *cfg) +{ + for (int i = 0; OPTIONS[i].name != NULL; i++) + luaL_serializer_parse_option(L, i, cfg); +} + +/** * @brief serializer.cfg{} Lua binding for serializers. * serializer.cfg is a table that contains current configuration values from * luaL_serializer structure. serializer.cfg has overriden __call() method @@ -225,39 +269,11 @@ static struct { * @return 0 */ static int -luaL_serializer_cfg(lua_State *L) +luaL_serializer_cfg(struct lua_State *L) { luaL_checktype(L, 1, LUA_TTABLE); /* serializer */ luaL_checktype(L, 2, LUA_TTABLE); /* serializer.cfg */ - struct luaL_serializer *cfg = luaL_checkserializer(L); - /* Iterate over all available options and checks keys in passed table */ - for (int i = 0; OPTIONS[i].name != NULL; i++) { - lua_getfield(L, 2, OPTIONS[i].name); - if (lua_isnil(L, -1)) { - lua_pop(L, 1); /* key hasn't changed */ - continue; - } - /* - * Update struct luaL_serializer using pointer to a - * configuration value (all values must be `int` for that). - */ - int *pval = (int *) ((char *) cfg + OPTIONS[i].offset); - /* Update struct luaL_serializer structure */ - switch (OPTIONS[i].type) { - case LUA_TBOOLEAN: - *pval = lua_toboolean(L, -1); - lua_pushboolean(L, *pval); - break; - case LUA_TNUMBER: - *pval = lua_tointeger(L, -1); - lua_pushinteger(L, *pval); - break; - default: - unreachable(); - } - /* Save normalized value to serializer.cfg table */ - lua_setfield(L, 1, OPTIONS[i].name); - } + luaL_serializer_parse_options(L, luaL_checkserializer(L)); return 0; } diff --git a/src/lua/utils.h b/src/lua/utils.h index 6b057af..f7980f4 100644 --- a/src/lua/utils.h +++ b/src/lua/utils.h @@ -240,6 +240,15 @@ luaL_checkserializer(struct lua_State *L) { luaL_checkudata(L, lua_upvalueindex(1), LUAL_SERIALIZER); } +/** + * Parse configuration table into @a cfg. + * @param L Lua stack. + * @param cfg Serializer to inherit configuration. + */ +void +luaL_serializer_parse_options(struct lua_State *l, + struct luaL_serializer *cfg); + /** A single value on the Lua stack. */ struct luaL_field { union { diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua index 42c79d6..7ec3b11 100755 --- a/test/app-tap/json.test.lua +++ b/test/app-tap/json.test.lua @@ -22,7 +22,8 @@ end tap.test("json", function(test) local serializer = require('json') - test:plan(13) + test:plan(28) + test:test("unsigned", common.test_unsigned, serializer) test:test("signed", common.test_signed, serializer) test:test("double", common.test_double, serializer) @@ -34,6 +35,65 @@ tap.test("json", function(test) test:test("misc", test_misc, serializer) -- + -- gh-2888: Check the possibility of using options in encode()/decode(). + -- + local orig_encode_max_depth = serializer.cfg.encode_max_depth + local sub = {a = 1, { b = {c = 1, d = {e = 1}}}} + serializer.cfg({encode_max_depth = 1}) + test:ok(serializer.encode(sub) == '{"1":null,"a":1}', + 'depth of encoding is 1 with .cfg') + serializer.cfg({encode_max_depth = orig_encode_max_depth}) + test:ok(serializer.encode(sub, {encode_max_depth = 1}) == '{"1":null,"a":1}', + 'depth of encoding is 1 with .encode') + test:is(serializer.cfg.encode_max_depth, orig_encode_max_depth, + 'global option remains unchanged') + + local orig_encode_invalid_numbers = serializer.cfg.encode_invalid_numbers + local nan = 1/0 + serializer.cfg({encode_invalid_numbers = false}) + test:ok(not pcall(serializer.encode, {a = nan}), + 'expected error with NaN encoding with .cfg') + serializer.cfg({encode_invalid_numbers = orig_encode_invalid_numbers}) + test:ok(not pcall(serializer.encode, {a = nan}, + {encode_invalid_numbers = false}), + 'expected error with NaN encoding with .encode') + test:is(serializer.cfg.encode_invalid_numbers, orig_encode_invalid_numbers, + 'global option remains unchanged') + + local orig_encode_number_precision = serializer.cfg.encode_number_precision + local number = 0.12345 + serializer.cfg({encode_number_precision = 3}) + test:ok(serializer.encode({a = number}) == '{"a":0.123}', + 'precision is 3') + serializer.cfg({encode_number_precision = orig_encode_number_precision}) + test:ok(serializer.encode({a = number}, {encode_number_precision = 3}) == + '{"a":0.123}', 'precision is 3') + test:is(serializer.cfg.encode_number_precision, orig_encode_number_precision, + 'global option remains unchanged') + + local orig_decode_invalid_numbers = serializer.cfg.decode_invalid_numbers + serializer.cfg({decode_invalid_numbers = false}) + test:ok(not pcall(serializer.decode, '{"a":inf}'), + 'expected error with NaN decoding with .cfg') + serializer.cfg({decode_invalid_numbers = orig_decode_invalid_numbers}) + test:ok(not pcall(serializer.decode, '{"a":inf}', + {decode_invalid_numbers = false}), + 'expected error with NaN decoding with .decode') + test:is(serializer.cfg.decode_invalid_numbers, orig_decode_invalid_numbers, + 'global option remains unchanged') + + local orig_decode_max_depth = serializer.cfg.decode_max_depth + serializer.cfg({decode_max_depth = 2}) + test:ok(not pcall(serializer.decode, '{"1":{"b":{"c":1,"d":null}},"a":1}'), + 'error: too many nested data structures') + serializer.cfg({decode_max_depth = orig_decode_max_depth}) + test:ok(not pcall(serializer.decode, '{"1":{"b":{"c":1,"d":null}},"a":1}', + {decode_max_depth = 2}), + 'error: too many nested data structures') + test:is(serializer.cfg.decode_max_depth, orig_decode_max_depth, + 'global option remains unchanged') + + -- -- gh-3514: fix parsing integers with exponent in json -- test:is(serializer.decode('{"var":2.0e+3}')["var"], 2000) diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c index 11aa402..631c53e 100644 --- a/third_party/lua-cjson/lua_cjson.c +++ b/third_party/lua-cjson/lua_cjson.c @@ -417,22 +417,25 @@ static void json_append_data(lua_State *l, struct luaL_serializer *cfg, } } -static int json_encode(lua_State *l) -{ - struct luaL_serializer *cfg = luaL_checkserializer(l); - char *json; - int len; - - luaL_argcheck(l, lua_gettop(l) == 1, 1, "expected 1 argument"); +static int json_encode(lua_State *l) { + luaL_argcheck(l, lua_gettop(l) == 2 || lua_gettop(l) == 1, 1, + "expected 1 or 2 arguments"); - /* Reuse existing buffer */ + /* Reuse existing buffer. */ strbuf_reset(&encode_buf); + struct luaL_serializer *cfg = luaL_checkserializer(l); - json_append_data(l, cfg, 0, &encode_buf); - json = strbuf_string(&encode_buf, &len); - - lua_pushlstring(l, json, len); + if (lua_gettop(l) == 2) { + struct luaL_serializer user_cfg = *cfg; + luaL_serializer_parse_options(l, &user_cfg); + lua_pop(l, 1); + json_append_data(l, &user_cfg, 0, &encode_buf); + } else { + json_append_data(l, cfg, 0, &encode_buf); + } + char *json = strbuf_string(&encode_buf, NULL); + lua_pushlstring(l, json, strbuf_length(&encode_buf)); return 1; } @@ -977,9 +980,18 @@ static int json_decode(lua_State *l) json_token_t token; size_t json_len; - luaL_argcheck(l, lua_gettop(l) == 1, 1, "expected 1 argument"); + luaL_argcheck(l, lua_gettop(l) == 2 || lua_gettop(l) == 1, 1, + "expected 1 or 2 arguments"); + + if (lua_gettop(l) == 2) { + struct luaL_serializer user_cfg = *luaL_checkserializer(l); + luaL_serializer_parse_options(l, &user_cfg); + lua_pop(l, 1); + json.cfg = &user_cfg; + } else { + json.cfg = luaL_checkserializer(l); + } - json.cfg = luaL_checkserializer(l); json.data = luaL_checklstring(l, 1, &json_len); json.current_depth = 0; json.ptr = json.data;