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

[roman.habibov1@yandex.ru]

Hi! Thanks for review.

>>  > - /*
>>  > - * 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.
Returned.

>>  > 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.
Yes. Sentence deleted.

>>  > 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})
Redone.

> BTW, I think test case with {encode_max_depth = 2} is redundant.
Removed.

>>  > + 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')
Added.

> The same comments are applicable to other test cases.
Redone.

>>  > + 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)
Fixed.

>>  > + 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.
Block is moved downwards.

> Look also how gh-3514 block header is formatted. It would be good to
> have all headers in the same style.
Done.

>>  > 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.
Added to 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.
Fixed.

commit 0d9351dac8312a6fab4a80c2167b75a36589d031
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.
    
    @TarantoolBot document
    Title: json.encode() json.decode()
    Add an ability to pass options to
    json.encode() and json.decode().
    These are the same options that
    are used globally in json.cfg().

diff --git a/src/lua/utils.c b/src/lua/utils.c
index 2f0f4dc..653ed1c 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -215,6 +215,50 @@ static struct {
 };
 
 /**
+ * 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;
+	}
+	/*
+	 * 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);
+	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);
+}
+
+/**
  * @brief serializer.cfg{} Lua binding for serializers.
  * serializer.cfg is a table that contains current configuration values from
  * luaL_serializer structure. serializer.cfg has overriden __call() method
@@ -225,39 +269,11 @@ 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);
-	}
+	luaL_serializer_parse_options(L, luaL_checkserializer(L));
 	return 0;
 }
 
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 6b057af..f7980f4 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -240,6 +240,15 @@ luaL_checkserializer(struct lua_State *L) {
 		luaL_checkudata(L, lua_upvalueindex(1), LUAL_SERIALIZER);
 }
 
+/**
+ * Parse configuration table into @a cfg.
+ * @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 42c79d6..7ec3b11 100755
--- a/test/app-tap/json.test.lua
+++ b/test/app-tap/json.test.lua
@@ -22,7 +22,8 @@ end
 
 tap.test("json", function(test)
     local serializer = require('json')
-    test:plan(13)
+    test:plan(28)
+
     test:test("unsigned", common.test_unsigned, serializer)
     test:test("signed", common.test_signed, serializer)
     test:test("double", common.test_double, serializer)
@@ -34,6 +35,65 @@ tap.test("json", function(test)
     test:test("misc", test_misc, serializer)
 
     --
+    -- gh-2888: Check the possibility of using options in encode()/decode().
+    --
+    local orig_encode_max_depth = serializer.cfg.encode_max_depth
+    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 = orig_encode_max_depth})
+    test:ok(serializer.encode(sub, {encode_max_depth = 1}) == '{"1":null,"a":1}',
+            'depth of encoding is 1 with .encode')
+    test:is(serializer.cfg.encode_max_depth, orig_encode_max_depth,
+            'global option remains unchanged')
+
+    local orig_encode_invalid_numbers = serializer.cfg.encode_invalid_numbers
+    local nan = 1/0
+    serializer.cfg({encode_invalid_numbers = false})
+    test:ok(not pcall(serializer.encode, {a = nan}),
+            'expected error with NaN encoding with .cfg')
+    serializer.cfg({encode_invalid_numbers = orig_encode_invalid_numbers})
+    test:ok(not pcall(serializer.encode, {a = nan},
+                      {encode_invalid_numbers = false}),
+            'expected error with NaN encoding with .encode')
+    test:is(serializer.cfg.encode_invalid_numbers, orig_encode_invalid_numbers,
+            'global option remains unchanged')
+
+    local orig_encode_number_precision = serializer.cfg.encode_number_precision
+    local number = 0.12345
+    serializer.cfg({encode_number_precision = 3})
+    test:ok(serializer.encode({a = number}) == '{"a":0.123}',
+            'precision is 3')
+    serializer.cfg({encode_number_precision = orig_encode_number_precision})
+    test:ok(serializer.encode({a = number}, {encode_number_precision = 3}) ==
+            '{"a":0.123}', 'precision is 3')
+    test:is(serializer.cfg.encode_number_precision, orig_encode_number_precision,
+            'global option remains unchanged')
+
+    local orig_decode_invalid_numbers = serializer.cfg.decode_invalid_numbers
+    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 = orig_decode_invalid_numbers})
+    test:ok(not pcall(serializer.decode, '{"a":inf}',
+                      {decode_invalid_numbers = false}),
+            'expected error with NaN decoding with .decode')
+    test:is(serializer.cfg.decode_invalid_numbers, orig_decode_invalid_numbers,
+            'global option remains unchanged')
+
+    local orig_decode_max_depth = serializer.cfg.decode_max_depth
+    serializer.cfg({decode_max_depth = 2})
+    test:ok(not pcall(serializer.decode, '{"1":{"b":{"c":1,"d":null}},"a":1}'),
+            'error: too many nested data structures')
+    serializer.cfg({decode_max_depth = orig_decode_max_depth})
+    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:is(serializer.cfg.decode_max_depth, orig_decode_max_depth,
+            'global option remains unchanged')
+
+    --
     -- gh-3514: fix parsing integers with exponent in json
     --
     test:is(serializer.decode('{"var":2.0e+3}')["var"], 2000)
diff --git a/third_party/lua-cjson/lua_cjson.c b/third_party/lua-cjson/lua_cjson.c
index 11aa402..631c53e 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;
-
-    luaL_argcheck(l, lua_gettop(l) == 1, 1, "expected 1 argument");
+static int json_encode(lua_State *l) {
+    luaL_argcheck(l, lua_gettop(l) == 2 || lua_gettop(l) == 1, 1,
+                  "expected 1 or 2 arguments");
 
-    /* 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);
+        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, strbuf_length(&encode_buf));
     return 1;
 }
 
@@ -977,9 +980,18 @@ 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(l, 1);
+        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;