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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the fixes! See 10 comments below.

1. Put the branch onto 1.10, not on 2.0. It does not
require SQL, so 1.10 is ok.

> commit 1ecdfa05d8aa0037da79eeeba19cecb06bab2103
> Author: Roman Khabibov <roman.habibov1@yandex.ru>
> 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.
> 
> diff --git a/src/lua/utils.c b/src/lua/utils.c
> index 2f0f4dcf8..e200fee0a 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -214,6 +214,47 @@ static struct {
>   	{ NULL, 0, 0, 0},
>   };
>   
> +/**
> + * 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, NULL if option is not
> + * in the table

2. As I said earlier, please, use separate @retval for each
possible value. One @retval for not NULL pointer, one @retval
for NULL.

> + */
> +static int *
> +luaL_serializer_parse_option(struct lua_State *L,
> +	int i, struct luaL_serializer *cfg)

3. Both here and in luaL_serializer_parse_options indentation
is broken after you changed the functions names.

> +{
> +	lua_getfield(L, 2, OPTIONS[i].name);
> +	if (lua_isnil(L, -1)) {
> +		lua_pop(L, 1);
> +		return NULL;
> +	}
> +	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);
> +	lua_pop(L, 1);
> +}
> +
>   /**
>    * @brief serializer.cfg{} Lua binding for serializers.
>    * serializer.cfg is a table that contains current configuration values from
> @@ -225,38 +266,29 @@ 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 */
> +	/* 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();
> +		int *pval = luaL_serializer_parse_option(L, i, cfg);

4. I've noticed that all the code below (push, setfield) is useless.
parse_option gets the value from cfg[name] and sets it to struct
luaL_cfg_serializer.name. Then the same value is pushed back onto
the stack and set into cfg[name].

So actually it looks like cfg[name] = cfg[name]. I've removed the
code below and all works ok. Please, do it.

> +		/* Update struct luaL_serializer structure. */
> +		if (pval != NULL) {
> +			switch (OPTIONS[i].type) {
> +			case LUA_TBOOLEAN:
> +				lua_pushboolean(L, *pval);
> +				break;
> +			case LUA_TNUMBER:
> +				lua_pushinteger(L, *pval);
> +				break;
> +			default:
> +				unreachable();
> +			}
> +			/* Save normalized value to serializer.cfg table. */
> +			lua_setfield(L, 1, OPTIONS[i].name);
>   		}
> -		/* Save normalized value to serializer.cfg table */
> -		lua_setfield(L, 1, OPTIONS[i].name);
>   	}
>   	return 0;
>   }
> diff --git a/test/app-tap/json.test.lua b/test/app-tap/json.test.lua
> index 3884b41e7..2a219ec24 100755
> --- a/test/app-tap/json.test.lua
> +++ b/test/app-tap/json.test.lua
> @@ -22,7 +22,55 @@ end
>   
>   tap.test("json", function(test)
>       local serializer = require('json')
> -    test:plan(9)
> +    test:plan(21)
> +
> +-- gh-2888: Check the possibility of using options in encode()/decode().

5. Please, align function's lines by the function's body offset. Including
comments.

> +
> +    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 = 2})
> +    test:ok(serializer.encode(sub) == '{"1":{"b":null},"a":1}',
> +        'depth of encoding is 2 with .cfg')
> +    serializer.cfg({encode_max_depth = 2})

6. You've called the same cfg 2 lines above.

> +    test:ok(serializer.encode(sub, {encode_max_depth = 1}) == '{"1":null,"a":1}',
> +        'depth of encoding is 1 with .encode')

7. Please, align function arguments under the function beginning.
I've pushed several fixes of this violation on a separate commit on
your branch. Please, look at them, squash, and do the same alignment
in other places in this file and other files.

> +
> +    local nan = 1/0
> +    test:ok(serializer.encode({a = nan}) == '{"a":inf}',
> +        'default "encode_invalid_numbers"')
> +    serializer.cfg({encode_invalid_numbers = false})
> +    test:ok(pcall(serializer.encode, {a = nan}) == false,

8. I believe, instead of 'boolean == false' you can just do
'not boolean' (I've fixed it here. Do the same in other
places).

> +        'expected error with NaN ecoding with .cfg')
> +    serializer.cfg({encode_invalid_numbers = true})
> +    test:ok(pcall(serializer.encode, {a = nan},
> +        {encode_invalid_numbers = false}) == false,
> +        'expected error with NaN ecoding with .encode')
> +
> +    local number = 0.12345
> +    test:ok(serializer.encode({a = number}) == '{"a":0.12345}',
> +        'precision more than 5')
> +    serializer.cfg({encode_number_precision = 3})
> +    test:ok(serializer.encode({a = number}) == '{"a":0.123}',
> +        'precision is 3')
> +    serializer.cfg({encode_number_precision = 14})
> +    test:ok(serializer.encode({a = number},
> +        {encode_number_precision = 3}) == '{"a":0.123}', 'precision is 3')
> +
> +    serializer.cfg({decode_invalid_numbers = false})
> +    test:ok(pcall(serializer.decode, '{"a":inf}') == false,
> +        'expected error with NaN decoding with .cfg')
> +    serializer.cfg({decode_invalid_numbers = true})
> +    test:ok(pcall(serializer.decode, '{"a":inf}',
> +        {decode_invalid_numbers = false}) == false,
> +        'expected error with NaN decoding with .decode')
> +
> +    test:ok(pcall(serializer.decode, '{"1":{"b":{"c":1,"d":null}},"a":1}',
> +        {decode_max_depth = 2}) == false,
> +        'error: too many nested data structures')
> +
> +--

9. Garbage empty comment.

>       test:test("unsigned", common.test_unsigned, serializer)
>       test:test("signed", common.test_signed, serializer)
>       test:test("double", common.test_double, serializer)
> diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
> index aa8217dfb..1a1bb3180 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);
> +    if (lua_gettop(l) == 2) {
> +        struct luaL_serializer user_cfg = *cfg;
> +        luaL_serializer_parse_options(l, &user_cfg);
> +        json_append_data(l, &user_cfg, 0, &encode_buf);
> +    } else {
> +        json_append_data(l, cfg, 0, &encode_buf);
> +    }
>   
> +    int len;
> +    char *json = strbuf_string(&encode_buf, &len);

10. 'len' parameter is ok to be NULL here.

>       lua_pushlstring(l, json, len);
> -
>       return 1;
>   }