[tarantool-patches] Re: [PATCH v3] json: add options to json.encode()
- From: roman.habibov1@xxxxxxxxx
- To: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>, "tarantool-patches@xxxxxxxxxxxxx" <tarantool-patches@xxxxxxxxxxxxx>
- Date: Tue, 31 Jul 2018 18:29:06 +0300
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@xxxxxxxxx>
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;
Other related posts:
