<HTML><BODY><div>Thank you for the review! All done now:</div><div> </div><div>Msgpack.cfg reconfiguration did not affect ffi module behavior. This<br>patch fixes it for all the options that were not yet covered elsewhere:<br>encode_sparse_convert, encode_sparse_ratio, encode_sparse_safe,<br>encode_invalid_numbers & decode_invalid_numbers,<br>encode_load_metatables & decode_save_metatables,<br>encode_use_tostring, encode_invalid_as_nil.</div><div>Most of common serializers' test were not applicable to msgpackffi since<br>it has no cfg field of its own. This issue was also solved.</div><div>Closes #4499<br>---<br>Issue:<br>https://github.com/tarantool/tarantool/issues/4499<br>Branch:<br>https://github.com/tarantool/tarantool/compare/eljashm/gh-4499-msgpackffi-ignores-msgpack.cfg-options</div><div> src/lua/msgpack.c | 1 -<br> src/lua/msgpack.h | 2 +-<br> src/lua/msgpackffi.lua | 70 +++++++++++++++++++----<br> src/lua/utils.h | 2 +-<br> test/app-tap/lua/serializer_test.lua | 7 ++-<br> test/app-tap/msgpackffi.test.lua | 85 +++++++++++++++++++++++++++-<br> 6 files changed, 149 insertions(+), 18 deletions(-)</div><div>diff --git a/src/lua/msgpack.h b/src/lua/msgpack.h<br>index d0ade303f..1e60f0a5d 100644<br>--- a/src/lua/msgpack.h<br>+++ b/src/lua/msgpack.h<br>@@ -47,7 +47,7 @@ struct mpstream;<br> /**<br> * Default instance of msgpack serializer (msgpack = require('msgpack')).<br> * This instance is used by all box's Lua/C API bindings (e.g. space:replace()).<br>- * All changes made by msgpack.cfg{} function are also affect box's bindings<br>+ * All changes made by msgpack.cfg{} function also affect box's bindings<br> * (this is a feature).<br> */<br> extern struct luaL_serializer *luaL_msgpack_default;<br>diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua<br>index f775f2d41..159a652b9 100644<br>--- a/src/lua/msgpackffi.lua<br>+++ b/src/lua/msgpackffi.lua<br>@@ -206,6 +206,10 @@ local function encode_nil(buf)<br> p[0] = 0xc0<br> end<br> <br>+local inf = 1/0<br>+local minf = -1/0<br>+local nan = 0/0<br>+<br> local function encode_r(buf, obj, level)<br> ::restart::<br> if type(obj) == "number" then<br>@@ -213,6 +217,14 @@ local function encode_r(buf, obj, level)<br> if obj % 1 == 0 and obj > -1e63 and obj < 1e64 then<br> encode_int(buf, obj)<br> else<br>+ if (obj == inf or obj == minf or tostring(obj) == "nan") and<br>+ not msgpack.cfg.encode_invalid_numbers then<br>+ if msgpack.cfg.encode_invalid_as_nil then<br>+ encode_nil(buf)<br>+ else<br>+ error("Invalid numbers encoding is manually prohibited")<br>+ end<br>+ end<br> encode_double(buf, obj)<br> end<br> elseif type(obj) == "string" then<br>@@ -227,28 +239,46 @@ local function encode_r(buf, obj, level)<br> return<br> end<br> local serialize = nil<br>- local mt = getmetatable(obj)<br>- if mt ~= nil then<br>- serialize = mt.__serialize<br>+ if msgpack.cfg.encode_load_metatables then<br>+ local mt = getmetatable(obj)<br>+ if mt ~= nil then<br>+ serialize = mt.__serialize<br>+ end<br> end<br> -- calculate the number of array and map elements in the table<br> -- TODO: pairs() aborts JIT<br>- local array_count, map_count = 0, 0<br>- for key in pairs(obj) do<br>- if type(key) == 'number' and key >= 1 and<br>- key == math.floor(key) and key == array_count + 1 then<br>+ local array_count, map_count, max_idx = 0, 0, 0<br>+ for key, el in pairs(obj) do<br>+ if type(key) == 'number' and key >= 1 and <br>+ key == math.floor(key) then<br> array_count = array_count + 1<br>+ if key > max_idx then<br>+ max_idx = key<br>+ end<br> else<br> map_count = map_count + 1<br> end<br> end<br>- if (serialize == nil and map_count == 0) or serialize == 'array' or<br>- serialize == 'seq' or serialize == 'sequence' then<br>+ -- excessively sparse tables should be encoded as maps<br>+ local is_excessively_sparse = false<br>+ if msgpack.cfg.encode_sparse_ratio and<br>+ max_idx > msgpack.cfg.encode_sparse_safe and<br>+ max_idx > array_count * msgpack.cfg.encode_sparse_ratio then<br>+ if not msgpack.cfg.encode_sparse_convert then<br>+ error(string.format("Can\'t encode excessively "..<br>+ "sparse tables when encode_sparse_convert "..<br>+ "configuration option is set to false"))<br>+ end<br>+ is_excessively_sparse = true<br>+ end<br>+ if (not is_excessively_sparse) and ( serialize == 'array' or<br>+ serialize == 'seq' or serialize == 'sequence' or<br>+ (serialize == nil and map_count == 0) ) then<br> encode_array(buf, array_count)<br> for i=1,array_count,1 do<br> encode_r(buf, obj[i], level + 1)<br> end<br>- elseif (serialize == nil and map_count > 0) or<br>+ elseif serialize == nil or is_excessively_sparse or<br> serialize == 'map' or serialize == 'mapping' then<br> encode_map(buf, array_count + map_count)<br> for key, val in pairs(obj) do<br>@@ -278,7 +308,18 @@ local function encode_r(buf, obj, level)<br> error("can not encode FFI type: '"..ffi.typeof(obj).."'")<br> end<br> else<br>- error("can not encode Lua type: '"..type(obj).."'")<br>+ if msgpack.cfg.encode_use_tostring then<br>+ obj = tostring(obj)<br>+ if obj then<br>+ encode_str(buf, obj)<br>+ else<br>+ error("can not encode Lua type: '"..type(obj).."'")<br>+ end<br>+ elseif msgpack.cfg.encode_invalid_as_nil then<br>+ encode_nil(buf)<br>+ else<br>+ error("can not encode Lua type: '"..type(obj).."'")<br>+ end<br> end<br> end<br> <br>@@ -453,7 +494,12 @@ end<br> <br> local function decode_double(data)<br> data[0] = data[0] - 1 -- mp_decode_double need type code<br>- return tonumber(builtin.mp_decode_double(data))<br>+ local res = tonumber(builtin.mp_decode_double(data))<br>+ if (res == inf or res == minf or tostring(res) == "nan") and<br>+ not msgpack.cfg.decode_invalid_numbers then<br>+ error("Invalid numbers decoding is manually prohibited")<br>+ end<br>+ return res<br> end<br> <br> local function decode_str(data, size)<br>diff --git a/src/lua/utils.h b/src/lua/utils.h<br>index 0b3672769..fed83cf8d 100644<br>--- a/src/lua/utils.h<br>+++ b/src/lua/utils.h<br>@@ -183,7 +183,7 @@ struct luaL_serializer {<br> * + map - at least one table index is not unsigned integer.<br> * + regular array - all array indexes are available.<br> * + sparse array - at least one array index is missing.<br>- * + excessively sparse arrat - the number of values missing<br>+ * + excessively sparse array - the number of values missing<br> * exceeds the configured ratio.<br> *<br> * An array is excessively sparse when **all** the following<br>diff --git a/test/app-tap/lua/serializer_test.lua b/test/app-tap/lua/serializer_test.lua<br>index 47edac621..860f4dceb 100644<br>--- a/test/app-tap/lua/serializer_test.lua<br>+++ b/test/app-tap/lua/serializer_test.lua<br>@@ -150,7 +150,7 @@ local function test_signed(test, s)<br> end<br> <br> local function test_double(test, s)<br>- test:plan(s.cfg and 15 or 9)<br>+ test:plan(s.cfg and 18 or 9)<br> rt(test, s, -1.1)<br> <br> rt(test, s, 3.1415926535898)<br>@@ -172,23 +172,28 @@ local function test_double(test, s)<br> --<br> local nan = 0/0<br> local inf = 1/0<br>+ local minf = -1/0<br> <br> local ss = s.new()<br> ss.cfg{encode_invalid_numbers = false}<br> test:ok(not pcall(ss.encode, nan), "encode exception on nan")<br> test:ok(not pcall(ss.encode, inf), "encode exception on inf")<br>+ test:ok(not pcall(ss.encode, minf), "encode exception on minf")<br> <br> ss.cfg{encode_invalid_numbers = true}<br> local xnan = ss.encode(nan)<br> local xinf = ss.encode(inf)<br>+ local xminf = ss.encode(minf)<br> <br> ss.cfg{decode_invalid_numbers = false}<br> test:ok(not pcall(ss.decode, xnan), "decode exception on nan")<br> test:ok(not pcall(ss.decode, xinf), "decode exception on inf")<br>+ test:ok(not pcall(ss.decode, xminf), "decode exception on minf")<br> <br> ss.cfg{decode_invalid_numbers = true}<br> rt(test, s, nan)<br> rt(test, s, inf)<br>+ rt(test, s, minf)<br> <br> ss = nil<br> end<br>diff --git a/test/app-tap/msgpackffi.test.lua b/test/app-tap/msgpackffi.test.lua<br>index 994402d15..6304559f0 100755<br>--- a/test/app-tap/msgpackffi.test.lua<br>+++ b/test/app-tap/msgpackffi.test.lua<br>@@ -5,6 +5,7 @@ package.path = "lua/?.lua;"..package.path<br> local tap = require('tap')<br> local common = require('serializer_test')<br> local ffi = require('ffi')<br>+local msgpack = require('msgpack')<br> <br> local function is_map(s)<br> local b = string.byte(string.sub(s, 1, 1))<br>@@ -36,6 +37,86 @@ local function test_offsets(test, s)<br> test:ok(not pcall(s.decode, dump, offset), "invalid offset")<br> end<br> <br>+local function test_configs(test, s)<br>+ test:plan(26)<br>+ --<br>+ -- gh-4499: ffi module should consider msgpack.cfg options<br>+ --<br>+<br>+ -- only excessively sparse arrays should be encoded as maps,<br>+ -- i.e. encode_sparse_ratio > 0 && max_idx > encode_sparse_safe<br>+ -- && max_idx > num(elements) * encode_sparse_ratio<br>+ test:ok(is_array(s.encode({ [1] = 1, [3] = 3, [4] = 3 })),<br>+ "not excessively sparse")<br>+ test:ok(is_array(s.encode({ [1] = 1, [2] = 2, [3] = 3, [4] = 4, [5] = 5,<br>+ [11] = 6 })), "still not excessively sparse")<br>+ test:ok(is_map(s.encode({ [1] = 1, [2] = 2, [100] = 3 })),<br>+ "is excessively sparse")<br>+<br>+ msgpack.cfg({encode_sparse_convert = false})<br>+ local ok, _ = pcall(s.encode(), { [1] = 1, [2] = 2, [100] = 3 })<br>+ test:ok(not ok, "conversion of sparse tables is manually prohibited")<br>+<br>+ -- testing (en/de)coding of invalid numbers<br>+ local inf = 1/0<br>+ local minf = -1/0<br>+ local nan = 0/0<br>+<br>+ test:ok(s.decode(s.encode(inf)), "decode & encode for inf")<br>+ test:ok(s.decode(s.encode(minf)), "decode & encode for minf")<br>+ test:ok(s.decode(s.encode(nan)), "decode & encode for nan")<br>+<br>+ msgpack.cfg({encode_invalid_numbers = false})<br>+ test:ok(not pcall(s.encode, inf), "prohibited encode for inf")<br>+ test:ok(not pcall(s.encode, minf), "prohibited encode for minf")<br>+ test:ok(not pcall(s.encode, nan), "prohibited encode for nan")<br>+<br>+ msgpack.cfg({decode_invalid_numbers = false})<br>+ test:ok(not pcall(s.decode, inf), "prohibited decode for inf")<br>+ test:ok(not pcall(s.decode, minf), "prohibited decode for minf")<br>+ test:ok(not pcall(s.decode, nan), "prohibited decode for nan")<br>+<br>+ -- invalid numbers can also be encoded as nil values<br>+ msgpack.cfg({decode_invalid_numbers = true, encode_invalid_as_nil = true})<br>+ test:is(s.decode(s.encode(inf)), nil, "invalid_as_nil for inf")<br>+ test:is(s.decode(s.encode(minf)), nil, "invalid_as_nil for minf")<br>+ test:is(s.decode(s.encode(nan)), nil, "invalid_as_nil for nan")<br>+<br>+ -- whether __serialize meta-value checking is enabled<br>+ local arr = setmetatable({1, 2, 3, k1 = 'v1', k2 = 'v2', 4, 5},<br>+ { __serialize = 'seq'})<br>+ local map = setmetatable({1, 2, 3, 4, 5}, { __serialize = 'map'})<br>+ local obj = setmetatable({}, {<br>+ __serialize = function(x) return 'serialize' end<br>+ })<br>+<br>+ -- testing encode metatables<br>+ msgpack.cfg{encode_load_metatables = false}<br>+ test:ok(is_map(s.encode(arr)), "array ignore __serialize")<br>+ test:ok(is_array(s.encode(map)), "map ignore __serialize")<br>+ test:ok(is_array(s.encode(obj)), "object ignore __serialize")<br>+<br>+ msgpack.cfg{encode_load_metatables = true}<br>+ test:ok(is_array(s.encode(arr)), "array load __serialize")<br>+ test:ok(is_map(s.encode(map)), "map load __serialize")<br>+ test:is(s.decode(s.encode(obj)), "serialize", "object load __serialize")<br>+<br>+ -- testing decode metatables<br>+ msgpack.cfg{decode_save_metatables = false}<br>+ test:isnil(getmetatable(s.decode(s.encode(arr))), "array __serialize")<br>+ test:isnil(getmetatable(s.decode(s.encode(map))), "map __serialize")<br>+<br>+ msgpack.cfg{decode_save_metatables = true}<br>+ test:is(getmetatable(s.decode(s.encode(arr))).__serialize, "seq",<br>+ "array save __serialize")<br>+ test:is(getmetatable(s.decode(s.encode(map))).__serialize, "map",<br>+ "map save __serialize")<br>+<br>+ -- applying default configs<br>+ msgpack.cfg({encode_sparse_convert = true, encode_invalid_numbers = true,<br>+ encode_invalid_as_nil = false})<br>+end<br>+<br> local function test_other(test, s)<br> test:plan(24)<br> local buf = string.char(0x93, 0x6e, 0xcb, 0x42, 0x2b, 0xed, 0x30, 0x47,<br>@@ -82,7 +163,6 @@ local function test_other(test, s)<br> while t ~= nil do level = level + 1 t = t[1] end<br> return level<br> end<br>- local msgpack = require('msgpack')<br> local deep_as_nil = msgpack.cfg.encode_deep_as_nil<br> msgpack.cfg({encode_deep_as_nil = true})<br> local max_depth = msgpack.cfg.encode_max_depth<br>@@ -118,7 +198,7 @@ end<br> <br> tap.test("msgpackffi", function(test)<br> local serializer = require('msgpackffi')<br>- test:plan(11)<br>+ test:plan(12)<br> test:test("unsigned", common.test_unsigned, serializer)<br> test:test("signed", common.test_signed, serializer)<br> test:test("double", common.test_double, serializer)<br>@@ -130,6 +210,7 @@ tap.test("msgpackffi", function(test)<br> -- udata/cdata hooks are not implemented<br> --test:test("ucdata", common.test_ucdata, serializer)<br> test:test("offsets", test_offsets, serializer)<br>+ test:test("configs", test_configs, serializer)<br> test:test("other", test_other, serializer)<br> test:test("decode_buffer", common.test_decode_buffer, serializer)<br> end)<br>diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c<br>index edbc15b72..133b2b527 100644<br>--- a/src/lua/msgpack.c<br>+++ b/src/lua/msgpack.c<br>@@ -95,7 +95,6 @@ luamp_decode_extension_default(struct lua_State *L, const char **data)<br> (unsigned char) **data);<br> }<br> <br>-<br> void<br> luamp_set_decode_extension(luamp_decode_extension_f handler)<br> {<br>-- <br>2.24.0</div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;"><div><br>Среда, 18 декабря 2019, 2:26 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_15766251980010426531_BODY">Sorry again for the late reply.<br><br>> > I think we also should implement encode_sparse_* options to<br>> > be compatible with msgpack module.<br>><br>> Could you please explain how do you propose to implement them? It<br>> seems to me the purpose of those configs is to handle excessively<br>> sparse arrays as maps (as stated in src/lua/utils.h). Which does not<br>> seem relevant to lua module where the only container type is table.<br>> What do I miss here? <br><br>It is about encoding, not decoding: encode a Lua table with numeric keys<br>as MP_ARRAY, MP_MAP or give an error. It seems that nothing prevent us<br>from implementing this logic on Lua.<br><br>> Subject: [PATCH 1/2] Msgpackffi considers msgpack.cfg options now<br>> Msgpack.cfg options changes did not affect msgpackffi serializer state.<br>> This patch fixes it for options encode_load_metatables, decode_ and<br>> encode_invalid_numbers, encode_use_tostring and encode_invalid_as_nil.<br>> Closes #4499<br>> ---<br>> Issue:<br>> <a href="https://github.com/tarantool/tarantool/issues/4499" target="_blank">https://github.com/tarantool/tarantool/issues/4499</a><br>> Branch:<br>> <a href="https://github.com/tarantool/tarantool/compare/eljashm/gh-4499-msgpackffi-ignores-msgpack.cfg-options" target="_blank">https://github.com/tarantool/tarantool/compare/eljashm/gh-4499-msgpackffi-ignores-msgpack.cfg-options</a><br><br>> @@ -261,6 +263,19 @@ local function encode_r(buf, obj, level)<br>> else<br>> error("Invalid __serialize value")<br>> end<br>> + elseif type(obj) == math.huge or type(obj) == -math.huge or <br>> + type(obj) == math.nan then<br>> + if msgpack.cfg.encode_invalid_numbers then<br>> + if obj == math.huge then<br>> + obj = 1/0<br>> + elseif obj == -math.huge then<br>> + obj = -1/0<br>> + else<br>> + obj = 0/0<br>> + end<br>> + else<br>> + encode_nil(buf)<br>> + end<br><br>I would save 1/0, -1/0, 0/0 as module local variables for readability,<br>like as it is done in test_double().<br><br>I also found that math.huge is 1/0 (inf) and there is no math.nan (it<br>just nil).<br><br>> @@ -562,6 +589,15 @@ decode_r = function(data)<br>> elseif c >= 0xe0 then<br>> return tonumber(ffi.cast('signed char',c)) -- negfixint<br>> elseif c == 0xc0 then<br>> + if msgpack.cfg.decode_invalid_numbers then<br>> + if c == 0/0 then<br>> + return math.nan<br>> + elseif c == 1/0 then<br>> + return math.huge<br>> + elseif c == -1/0 then<br>> + return -math.huge<br>> + end<br>> + end<br><br>Same as above.<br><br>> @@ -628,6 +664,8 @@ return {<br>> on_encode = on_encode;<br>> decode_unchecked = decode_unchecked;<br>> decode = decode_unchecked; -- just for tests<br>> + cfg = msgpack.cfg;<br>> + new = msgpack.new; -- for serializer tests to work properly<br><br>So they'll test msgpack, not msgpackffi.</div></div></div></div></div></blockquote> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Maria Khaydich</div></div></div><div> </div></div></blockquote></BODY></HTML>