Re: [Tarantool-patches] [PATCH] Msgpackffi considers msgpack.cfg options now

["Maria Khaydich" <maria.khaydich@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 14595 bytes --]


Thank you for the review.
 
I’ve made sure new functionality is tested in test/app-tap/lua/serializer_test.lua by adding ‘cfg’ and ‘new’ fields in msgpackffi module. 
 
 > Lua supports NaN/Inf values, .. I propose to implement them.
Also done.
 
> I think we also should implement encode_sparse_* options to be compatible with msgpack module.
Could you please explain how do you propose to implement them? It seems to me the purpose of those configs is to handle excessively sparse arrays as maps (as stated in src/lua/utils.h). Which does not seem relevant to lua module where the only container type is table. What do I miss here? 
 
> During the review I gathered the information what options are supported by each (de)serializer
That was very helpful, thank you!
 
The patch
 
Subject: [PATCH 1/2] Msgpackffi considers msgpack.cfg options now
Msgpack.cfg options changes did not affect msgpackffi serializer state.
This patch fixes it for options encode_load_metatables, decode_ and
encode_invalid_numbers, encode_use_tostring and encode_invalid_as_nil.
Closes #4499
---
Issue:
https://github.com/tarantool/tarantool/issues/4499
Branch:
https://github.com/tarantool/tarantool/compare/eljashm/gh-4499-msgpackffi-ignores-msgpack.cfg-options
 src/lua/msgpackffi.lua               | 46 +++++++++++++++++++++++++---
 test/app-tap/lua/serializer_test.lua |  7 ++++-
 2 files changed, 48 insertions(+), 5 deletions(-)
diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
index 5331dc75f..2689bb214 100644
--- a/src/lua/msgpackffi.lua
+++ b/src/lua/msgpackffi.lua
@@ -227,9 +227,11 @@ local function encode_r(buf, obj, level)
             return
         end
         local serialize = nil
-        local mt = getmetatable(obj)
-        if mt ~= nil then
-            serialize = mt.__serialize
+        if msgpack.cfg.encode_load_metatables then
+            local mt = getmetatable(obj)
+            if mt ~= nil then
+                serialize = mt.__serialize
+            end
         end
         -- calculate the number of array and map elements in the table
         -- TODO: pairs() aborts JIT
@@ -261,6 +263,19 @@ local function encode_r(buf, obj, level)
         else
             error("Invalid __serialize value")
         end
+    elseif type(obj) == math.huge or type(obj) == -math.huge or 
+        type(obj) == math.nan then
+        if msgpack.cfg.encode_invalid_numbers then
+            if obj == math.huge then
+                obj = 1/0
+            elseif obj == -math.huge then
+                obj = -1/0
+            else
+                obj = 0/0
+            end
+        else
+            encode_nil(buf)
+        end
     elseif obj == nil then
         encode_nil(buf)
     elseif type(obj) == "boolean" then
@@ -278,9 +293,21 @@ local function encode_r(buf, obj, level)
             error("can not encode FFI type: '"..ffi.typeof(obj).."'")
         end
     else
-        error("can not encode Lua type: '"..type(obj).."'")
+        if msgpack.cfg.encode_use_tostring then
+            obj = tostring(obj)
+            if obj then
+                encode_str(buf, obj)
+            else
+                error("can not encode Lua type: '"..type(obj).."'")
+            end
+        else if msgpack.cfg.encode_invalid_as_nil then
+            encode_nil(buf)
+        else
+            error("can not encode Lua type: '"..type(obj).."'")
+        end
     end
 end
+end
 
 local function encode(obj)
     local tmpbuf = buffer.IBUF_SHARED
@@ -562,6 +589,15 @@ decode_r = function(data)
     elseif c >= 0xe0 then
         return tonumber(ffi.cast('signed char',c)) -- negfixint
     elseif c == 0xc0 then
+        if msgpack.cfg.decode_invalid_numbers then
+            if c == 0/0 then
+                return math.nan
+            elseif c == 1/0 then
+                return math.huge
+            elseif c == -1/0 then
+                return -math.huge
+            end
+        end
         return msgpack.NULL
     elseif c == 0xc2 then
         return false
@@ -628,6 +664,8 @@ return {
     on_encode = on_encode;
     decode_unchecked = decode_unchecked;
     decode = decode_unchecked; -- just for tests
+    cfg = msgpack.cfg;
+    new = msgpack.new; -- for serializer tests to work properly
     internal = {
         encode_fix = encode_fix;
         encode_array = encode_array;
diff --git a/test/app-tap/lua/serializer_test.lua b/test/app-tap/lua/serializer_test.lua
index 1609f768f..b29eb9da5 100644
--- a/test/app-tap/lua/serializer_test.lua
+++ b/test/app-tap/lua/serializer_test.lua
@@ -146,7 +146,7 @@ local function test_signed(test, s)
 end
 
 local function test_double(test, s)
-    test:plan(s.cfg and 15 or 9)
+    test:plan(s.cfg and 18 or 9)
     rt(test, s, -1.1)
 
     rt(test, s, 3.1415926535898)
@@ -168,23 +168,28 @@ local function test_double(test, s)
     --
     local nan = 0/0
     local inf = 1/0
+    local minf = -1/0
 
     local ss = s.new()
     ss.cfg{encode_invalid_numbers = false}
     test:ok(not pcall(ss.encode, nan), "encode exception on nan")
     test:ok(not pcall(ss.encode, inf), "encode exception on inf")
+    test:ok(not pcall(ss.encode, minf), "encode exception on minf")
 
     ss.cfg{encode_invalid_numbers = true}
     local xnan = ss.encode(nan)
     local xinf = ss.encode(inf)
+    local xminf = ss.encode(minf)
 
     ss.cfg{decode_invalid_numbers = false}
     test:ok(not pcall(ss.decode, xnan), "decode exception on nan")
     test:ok(not pcall(ss.decode, xinf), "decode exception on inf")
+    test:ok(not pcall(ss.decode, xminf), "decode exception on minf")
 
     ss.cfg{decode_invalid_numbers = true}
     rt(test, s, nan)
     rt(test, s, inf)
+    rt(test, s, minf)
 
     ss = nil
 end
-- 
2.20.1 (Apple Git-117)
 
 
Subject: [PATCH 2/2] Typos
---
 src/lua/msgpack.c | 1 -
 src/lua/msgpack.h | 2 +-
 src/lua/utils.h   | 2 +-
 3 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
index ef315b692..4c3fbfba2 100644
--- a/src/lua/msgpack.c
+++ b/src/lua/msgpack.c
@@ -95,7 +95,6 @@ luamp_decode_extension_default(struct lua_State *L, const char **data)
            (unsigned char) **data);
 }
 
-
 void
 luamp_set_decode_extension(luamp_decode_extension_f handler)
 {
diff --git a/src/lua/msgpack.h b/src/lua/msgpack.h
index d0ade303f..1e60f0a5d 100644
--- a/src/lua/msgpack.h
+++ b/src/lua/msgpack.h
@@ -47,7 +47,7 @@ struct mpstream;
 /**
  * Default instance of msgpack serializer (msgpack = require('msgpack')).
  * This instance is used by all box's Lua/C API bindings (e.g. space:replace()).
- * All changes made by msgpack.cfg{} function are also affect box's bindings
+ * All changes made by msgpack.cfg{} function also affect box's bindings
  * (this is a feature).
  */
 extern struct luaL_serializer *luaL_msgpack_default;
diff --git a/src/lua/utils.h b/src/lua/utils.h
index d9fb0704f..1e586295a 100644
--- a/src/lua/utils.h
+++ b/src/lua/utils.h
@@ -186,7 +186,7 @@ struct luaL_serializer {
      *  + map - at least one table index is not unsigned integer.
      *  + regular array - all array indexes are available.
      *  + sparse array - at least one array index is missing.
-     *  + excessively sparse arrat - the number of values missing
+     *  + excessively sparse array - the number of values missing
      * exceeds the configured ratio.
      *
      * An array is excessively sparse when **all** the following
-- 
2.20.1 (Apple Git-117)
 
  
>Вторник, 12 ноября 2019, 2:21 +03:00 от Alexander Turenko <alexander.turenko@tarantool.org>:
> 
>We should test the new code. Look, test/app-tap/lua/serializer_test.lua
>(common code to test serializers) contains `if not s.cfg then return
>end` blocks to exclude test cases, which are not supported by
>msgpackffi. Please, ensure that all new functionality is tested.
>
>You can also try to use luacov (test-run even has --luacov option),
>however I didn't tried it with tarantool's build-in code, only with
>tarantool/graphql. Don't sure whether it is hard to setup, so I don't
>insist on this at all.
>
>Igor (I CCed him) recently wonder whether it worth to use ffi on modern
>LuaJIT with implemented traces stitching. Maybe it worth to perform some
>microbenchmarks of msgpack and msgpackffi to ensure that there is still
>need to work on msgpackffi. (Technically it is out of scope of your
>issue, however it is interesting to know. Please, ask Georgy whether it
>is worth to spend your time on that now.)
>
>See other notes below.
>
>WBR, Alexander Turenko.
>
>On Thu, Nov 07, 2019 at 01:36:42PM +0300, Maria wrote:
>> Lua serializers have common configuration options
>> that mostly were not respected in msgpackffi module.
>> This patch fixes that for several configurations:
>> encode_use_tostring, encode_invalid_as_nil and
>> encode_load_metatables.
>> Options encode/decode_invalid_numbers are suggested
>> to not be considered in msgpackffi module since lua
>> processed them properly without this check so adding
>> it would mean cutting functionality provided by the
>> language in case this field was set to be false.
>
>Lua supports NaN/Inf values, MsgPack (I mean, the format) supports them
>too -- here you are right. But a user logic can lean on the fact that
>all floating point numbers are not NaN/Inf. Also, we intend to be
>compatible with msgpack module, which supports those options. I propose
>to implement them.
>
>I think we also should implement encode_sparse_* options to be
>compatible with msgpack module.
>
>During the review I gathered the information what options are supported
>by each (de)serializer:  https://github.com/tarantool/doc/issues/976
>Maybe this will be interesting for you.
>
>I'll paste notes I wrote while looking around serializers (I was not
>quite aware of the code), they can be useful to verify the table from
>the issue above.
>
>----
>
>The following encode_* options are handled in src/lua/utils.c (in
>luaL_tofield() and luaL_convertfield(), which are used in msgpack, json
>and yaml modules; sometimes via luaL_checkfield() helper):
>
>- encode_sparse_convert
>- encode_sparse_ratio
>- encode_sparse_safe
>- encode_invalid_numbers
>- encode_load_metatables
>- encode_use_tostring
>- encode_invalid_as_nil
>
>The following encode_* options are handled separately by each
>serializer:
>
>- encode_max_depth: msgpack, json
>- encode_deep_as_nil: msgpack, json
>- encode_number_precision: json, yaml
>
>(msgpackffi does not use those helpers, because they are for Lua-C API.)
>
>All decode_* options are handled separately by each deserializer (but
>sometimes using luaL_checkfinite() helper from src/lua/utils.c):
>
>- decode_invalid_numbers: msgpack, json, yaml
>- decode_save_metatables: msgpack, json, yaml
>- decode_max_depth: json
>
>----
>
>FYI: I also filed  https://github.com/tarantool/tarantool/issues/4622
>('lua: forbid options that are not applicable for certain
>(de)serializer').
>
>> Closes #4499
>
>I think it worth to add a docbot comment (see tarantool/docbot) and
>state that msgpackffi now supports all msgpack options (and gather those
>ones that are set with msgpack.cfg()). It worth also mention that while
>the API is mostly the same, msgpackffi does not check a buffer
>boundaries and lacks of .decode_{array,map}_header(), .cfg() and .new()
>functions (maybe there are other differences; I would expect that you
>will refine them during the work on tests).
>
>However, to be honest, I'm a bit tentative about making the module
>public. I would discuss this with the rest of the team. Igor, Georgy,
>what do you think about it?
>
>> Issue:
>>  https://github.com/tarantool/tarantool/issues/4499
>> Branch:
>>  https://github.com/tarantool/tarantool/compare/eljashm/gh-4499-msgpackffi-ignores-msgpack.cfg-options
>
>> diff --git a/src/lua/msgpack.c b/src/lua/msgpack.c
>> index c2be0b3e8..d3df1a8c1 100644
>> --- a/src/lua/msgpack.c
>> +++ b/src/lua/msgpack.c
>> @@ -95,7 +95,6 @@ luamp_decode_extension_default(struct lua_State *L, const char **data)
>> (unsigned char) **data);
>> }
>>
>> -
>> void
>> luamp_set_decode_extension(luamp_decode_extension_f handler)
>> {
>
>Please, move all those changes into a separate commit.
>
>> diff --git a/src/lua/msgpack.h b/src/lua/msgpack.h
>> index d0ade303f..1e60f0a5d 100644
>> --- a/src/lua/msgpack.h
>> +++ b/src/lua/msgpack.h
>> @@ -47,7 +47,7 @@ struct mpstream;
>> /**
>> * Default instance of msgpack serializer (msgpack = require('msgpack')).
>> * This instance is used by all box's Lua/C API bindings (e.g. space:replace()).
>> - * All changes made by msgpack.cfg{} function are also affect box's bindings
>> + * All changes made by msgpack.cfg{} function also affect box's bindings
>> * (this is a feature).
>> */
>> extern struct luaL_serializer *luaL_msgpack_default;
>> diff --git a/src/lua/msgpackffi.lua b/src/lua/msgpackffi.lua
>> index f7ee44291..43f5d6df1 100644
>> --- a/src/lua/msgpackffi.lua
>> +++ b/src/lua/msgpackffi.lua
>> @@ -224,9 +224,11 @@ local function encode_r(buf, obj, level)
>> return
>> end
>> local serialize = nil
>> - local mt = getmetatable(obj)
>> - if mt ~= nil then
>> - serialize = mt.__serialize
>> + if msgpack.cfg.encode_load_metatables then
>> + local mt = getmetatable(obj)
>> + if mt ~= nil then
>> + serialize = mt.__serialize
>> + end
>> end
>> -- calculate the number of array and map elements in the table
>> -- TODO: pairs() aborts JIT
>> @@ -275,7 +277,19 @@ local function encode_r(buf, obj, level)
>> error("can not encode FFI type: '"..ffi.typeof(obj).."'")
>> end
>> else
>> - error("can not encode Lua type: '"..type(obj).."'")
>> + if msgpack.cfg.encode_use_tostring then
>> + obj = tostring(obj)
>> + if obj then
>> + goto restart
>> + else
>> + error("can not encode Lua type: '"..type(obj).."'")
>> + end
>> + else if msgpack.cfg.encode_invalid_as_nil then
>> + encode_nil(buf)
>> + else
>> + error("can not encode Lua type: '"..type(obj).."'")
>> + end
>> + end
>> end
>> end
>
>The code itself looks correct, however we cannot be sure w/o proper
>testing.
 
 
--
Maria Khaydich
 

[-- Attachment #2: Type: text/html, Size: 19504 bytes --]