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.

> 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!

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)

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.