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

[roman.habibov1@yandex.ru]

Thanks for review.

> 1. Unnecessary diff. Please, avoid making additional
> non-functional changes which increase number of hunks
> in diff.
Fixed.


> 2. This function is not used out of utils.c and can be
> declared as static and removed from the header.
diff --git a/src/lua/utils.h b/src/lua/utils.h
index 6b057af3e..2faa966fe 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.
+ * parse_options.
+ * @param L lua stack
+ * @param cfg serializer to inherit configuration
+ */
+void
+parse_options(struct lua_State *l, struct luaL_serializer *cfg);
+
 /** A single value on the Lua stack. */
 struct luaL_field {
 	union {


> 3. Wrong style of luaL_serializer pointer declaration.
> Please, put '*' next to the name and after whitespace
> next to the type.
Fixed.


> 4. Please, do not omit 'struct' when declare struct
> variables. Here I am pointing out lua_State parameter.
Fixed in utils.c, but not in lua-cjson.c.


> 6. Why do you need to announce pval? Why not just
>
>      if (parse_option(L, i, cfg) != NULL)
>
> ?
>
+void
+parse_options(struct lua_State *L, struct luaL_serializer *cfg) {
+	for (int i = 0; OPTIONS[i].name != NULL; i++) {
+		if (parse_option(L, i, cfg) != NULL)
+			lua_pop(L, 1);
+	}
+	lua_pop(L, 1);
+}


> 7. Please, use 8-width tabs instead of spaces in this
> file.
Fixed.


> 8. Please, fix pointers declaration style. The previous
> was correct.
Fixed.


> 9. Broken indentation.
> 10. Same.
Fixed.


> 11. Please, consult other places how to write function comments.
> In a comment you should use imperative (like in the commit title)
> and it is not mandatory thing to either duplicate function name
> in the comment or write things like 'This function does blah-blah'.
+/**
+ * Configure one field in @a cfg. Add one lua table to the top of
+ * lua stack.
+ * @param L lua stack
+ * @param i index of option in OPTIONS[]
+ * @param cfg serializer to inherit configuration
+ * @return ponter to the value of option, NULL if option is not
+ * in the table
+ */
+static int *
+parse_option(struct lua_State *L, int i, struct luaL_serializer *cfg) {

+/**
+ * Parse configuration table into @a cfg. Remove the lua table
+ * from the top of lua stack.
+ * parse_options.
+ * @param L lua stack
+ * @param cfg serializer to inherit configuration
+ */
+void
+parse_options(struct lua_State *l, struct luaL_serializer *cfg);
+


> 12. Please, finish the sentence with the dot. Yes, even
> in the tests.
+-- gh-2888: Check the possibility of using options in encode()/decode().


commit ebef41bb68abbac75ed2d7cb4a5fa65d282f4ee3
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..12fa8ff0d 100644
--- a/src/lua/utils.c
+++ b/src/lua/utils.c
@@ -214,6 +214,50 @@ static struct {
 	{ NULL, 0, 0, 0},
 };
 
+/**
+ * Configure one field in @a cfg. Add one lua table to the top of
+ * lua stack.
+ * @param L lua stack
+ * @param i index of option in OPTIONS[]
+ * @param cfg serializer to inherit configuration
+ * @return ponter to the value of option, NULL if option is not
+ * in the table
+ */
+static int *
+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); /* key hasn't changed */
+		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);
+	/* Update struct luaL_serializer structure */
+	switch (OPTIONS[i].type) {
+	case LUA_TBOOLEAN:
+		*pval = lua_toboolean(L, -1);
+		break;
+	case LUA_TNUMBER:
+		*pval = lua_tointeger(L, -1);
+		break;
+	default:
+		unreachable();
+	}
+	return pval;
+}
+
+void
+parse_options(struct lua_State *L, struct luaL_serializer *cfg) {
+	for (int i = 0; OPTIONS[i].name != NULL; i++) {
+		if (parse_option(L, i, cfg) != NULL)
+			lua_pop(L, 1);
+	}
+	lua_pop(L, 1);
+}
+
 /**
  * @brief serializer.cfg{} Lua binding for serializers.
  * serializer.cfg is a table that contains current configuration values from
@@ -225,38 +269,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 */
 	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);
+		int *pval = parse_option(L, i, cfg);
 		/* 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();
+		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/src/lua/utils.h b/src/lua/utils.h
index 6b057af3e..2faa966fe 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.
+ * parse_options.
+ * @param L lua stack
+ * @param cfg serializer to inherit configuration
+ */
+void
+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 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().
+
+    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})
+    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(pcall(serializer.encode, {a = nan}) == false,
+        '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')
+
+--
     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..29553fc4d 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 */
     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;
+        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);
     lua_pushlstring(l, json, len);
-
     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);
+        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;