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 AA7D428BFD for ; Sun, 9 Sep 2018 11:28:05 -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 ZIsS3j6ByfAM for ; Sun, 9 Sep 2018 11:28:05 -0400 (EDT) Received: from smtp32.i.mail.ru (smtp32.i.mail.ru [94.100.177.92]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 6393728B76 for ; Sun, 9 Sep 2018 11:28:05 -0400 (EDT) Date: Sun, 9 Sep 2018 18:28:04 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH v3] json: add options to json.encode() Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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: Roman Khabibov Cc: Vladislav Shpilevoy , tarantool-patches@freelists.org Hi, Roman! Sorry for the late response. Please, consider inline comments. WBR, Alexander Turenko. On Wed, Aug 15, 2018 at 01:29:38AM +0300, Vladislav Shpilevoy wrote: > Hi! Thanks for the fixes! I have force pushed one minor > fix for alignment in lua_cjson.c. > > Alexander, please, make a second review. > > On 14/08/2018 02:14, roman.habibov1@yandex.ru wrote: > > - /* > > - * 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. > > 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. > > 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}) BTW, I think test case with {encode_max_depth = 2} is redundant. > > + 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') The same comments are applicable to other test cases. > > + 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) > > + 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. Look also how gh-3514 block header is formatted. It would be good to have all headers in the same style. > > 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. > > + 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. > > return 1; > > } > > @@ -977,9 +980,17 @@ 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 is not used here, but used in encode. > > + 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; > >