Tarantool development patches archive
 help / color / mirror / Atom feed
From: Alexander Turenko <alexander.turenko@tarantool.org>
To: Roman Khabibov <roman.habibov1@yandex.ru>
Cc: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>,
	tarantool-patches@freelists.org
Subject: [tarantool-patches] Re: [PATCH v3] json: add options to json.encode()
Date: Sun, 9 Sep 2018 18:28:04 +0300	[thread overview]
Message-ID: <20180909152804.4lagt5drkymriv5x@tkn_work_nb> (raw)
In-Reply-To: <a4b26695-9388-0d69-ce81-3e4216846d22@tarantool.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;
> > 

  parent reply	other threads:[~2018-09-09 15:28 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <cover.1531010828.git.roman.habibov1@yandex.ru>
2018-07-08  0:57 ` [tarantool-patches] [PATCH] json: added " Roman Khabibov
2018-07-09 10:33   ` [tarantool-patches] " Vladislav Shpilevoy
2018-07-17 18:19     ` roman.habibov1
2018-07-19 10:18       ` Vladislav Shpilevoy
2018-07-23 22:38         ` [tarantool-patches] Re: [PATCH v3] json: add " roman.habibov1
2018-07-25 21:35           ` Vladislav Shpilevoy
2018-07-26  9:40             ` roman.habibov1
2018-07-26 10:07               ` Vladislav Shpilevoy
2018-07-26 12:29                 ` roman.habibov1
2018-07-26 12:33                   ` Vladislav Shpilevoy
2018-07-26 13:19                     ` roman.habibov1
2018-07-26 21:45                       ` Vladislav Shpilevoy
2018-07-31 15:29                         ` roman.habibov1
2018-08-01 10:37                           ` Vladislav Shpilevoy
2018-08-01 20:41                             ` roman.habibov1
2018-08-02 12:59                               ` Vladislav Shpilevoy
2018-08-07 21:52                                 ` roman.habibov1
2018-08-07 21:53                                   ` roman.habibov1
2018-08-08 19:07                                   ` Vladislav Shpilevoy
2018-08-13 23:14                                     ` roman.habibov1
2018-08-14 22:29                                       ` Vladislav Shpilevoy
2018-08-23 21:03                                         ` Alexander Turenko
2018-09-09 15:28                                         ` Alexander Turenko [this message]
2018-09-09 23:42                                           ` roman.habibov1
2018-09-10 13:12                                             ` Alexander Turenko
2018-08-08 19:08                                 ` Vladislav Shpilevoy
2018-07-11  7:57   ` [tarantool-patches] Re: [PATCH] json: added " Kirill Yukhin
2018-07-19 10:24   ` Vladislav Shpilevoy
2018-09-13 15:23   ` Kirill Yukhin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180909152804.4lagt5drkymriv5x@tkn_work_nb \
    --to=alexander.turenko@tarantool.org \
    --cc=roman.habibov1@yandex.ru \
    --cc=tarantool-patches@freelists.org \
    --cc=v.shpilevoy@tarantool.org \
    --subject='[tarantool-patches] Re: [PATCH v3] json: add options to json.encode()' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox