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

[roman.habibov1@yandex.ru]

Addition.
New branch: https://github.com/tarantool/tarantool/tree/romankhabibov/gh-2888-options-encode

08.08.2018, 00:52, "roman.habibov1@yandex.ru" <roman.habibov1@yandex.ru>:
> Hi! Thanks for the review!
>
>>  1. Put the branch onto 1.10, not on 2.0. It does not
>>  require SQL, so 1.10 is ok.
>
> Done.
>
>>  2. As I said earlier, please, use separate @retval for each
>>  possible value. One @retval for not NULL pointer, one @retval
>>  for NULL.
>
> +/**
> + * 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)
>
>>  3. Both here and in luaL_serializer_parse_options indentation
>>  is broken after you changed the functions names.
>
> +luaL_serializer_parse_option(struct lua_State *L, int i,
> + struct luaL_serializer *cfg)
>
> +void
> +luaL_serializer_parse_options(struct lua_State *L,
> + struct luaL_serializer *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.
>
> 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);
> - }
> + /* Iterate over all available options and checks keys in passed table. */
> + for (int i = 0; OPTIONS[i].name != NULL; i++)
> + luaL_serializer_parse_option(L, i, cfg);
>          return 0;
>  }
>
>>  5. Please, align function's lines by the function's body offset. Including
>>  comments.
>
> Fixed.
>
>>  6. You've called the same cfg 2 lines above.
>
> + 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')
> + 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.
>
> Fixed.
>
>>  8. I believe, instead of 'boolean == false' you can just do
>>  'not boolean' (I've fixed it here. Do the same in other
>>  places).
>
> + test:ok(not pcall(serializer.encode, {a = nan}),
> + 'expected error with NaN ecoding with .cfg')
> + test:ok(not pcall(serializer.encode, {a = nan},
> + {encode_invalid_numbers = false}),
> + 'expected error with NaN ecoding with .encode')
>
> + test:ok(not pcall(serializer.decode, '{"a":inf}',
> + {decode_invalid_numbers = false}),
> + 'expected error with NaN decoding with .decode')
>
>>  9. Garbage empty comment.
>
> Deleted.
>
>>  10. 'len' parameter is ok to be NULL here.
>
> + char *json = strbuf_string(&encode_buf, NULL);
> + lua_pushlstring(l, json, encode_buf.length);
>
> commit f6d41bdfcec241733f98b7d26715ece6625539aa
> 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..3faeb416f 100644
> --- a/src/lua/utils.c
> +++ b/src/lua/utils.c
> @@ -214,6 +214,48 @@ 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
> + * @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;
> + }
> + 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,39 +267,14 @@ 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);
> - }
> + /* Iterate over all available options and checks keys in passed table. */
> + for (int i = 0; OPTIONS[i].name != NULL; i++)
> + luaL_serializer_parse_option(L, i, cfg);
>          return 0;
>  }
>
> diff --git a/src/lua/utils.h b/src/lua/utils.h
> index 6b057af3e..204bd8664 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.
> + * @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 42c79d6e9..f2c67ab0a 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(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}}}}
> + 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')
> + test:ok(serializer.encode(sub, {encode_max_depth = 1}) == '{"1":null,"a":1}',
> + 'depth of encoding is 1 with .encode')
> +
> + 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')
> +
> + 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(not pcall(serializer.decode, '{"a":inf}'),
> + 'expected error with NaN decoding with .cfg')
> + serializer.cfg({decode_invalid_numbers = true})
> + test:ok(not pcall(serializer.decode, '{"a":inf}',
> + {decode_invalid_numbers = false}),
> + 'expected error with NaN decoding with .decode')
> +
> + 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: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 11aa40225..b175b770c 100644
> --- a/third_party/lua-cjson/lua_cjson.c
> +++ b/third_party/lua-cjson/lua_cjson.c
> @@ -417,22 +417,24 @@ 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);
> + 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);
>      return 1;
>  }
>
> @@ -977,9 +979,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);
> + 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;