[tarantool-patches] Re: [PATCH v3] json: add options to json.encode()

[Alexander Turenko]

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 at 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;
> >