[tarantool-patches] Re: [PATCH v2 2/4] msgpack: make msgpackffi use encode_max_depth option

[Vladislav Shpilevoy]

Thanks for the review!

On 13/09/2019 01:24, Alexander Turenko wrote:
> LGTM.
> 
> One small comment about the test case, see below.
> 
> WBR, Alexander Turenko.
> 
>> +    --
>> +    -- gh-4434: msgpackffi does not care about msgpack serializer
>> +    -- configuration, but it should.
>> +    --
>> +    local function check_depth(depth_to_try)
>> +        local t = nil
>> +        for i = 1, depth_to_try do t = {t} end
>> +        t = s.decode_unchecked(s.encode(t))
>> +        local level = 0
>> +        while t ~= nil do level = level + 1 t = t[1] end
>> +        return level
>> +    end
>> +    local msgpack = require('msgpack')
>> +    local max_depth = msgpack.cfg.encode_max_depth
>> +    local result_depth = check_depth(max_depth + 5)
>> +    test:is(result_depth, max_depth,
>> +            "msgpackffi uses msgpack.cfg.encode_max_depth")
>> +
>> +    msgpack.cfg({encode_max_depth = max_depth + 5})
>> +    result_depth = check_depth(max_depth + 5)
>> +    test:is(result_depth, max_depth + 5, "and uses it dynamically")
>> +
>> +    msgpack.cfg({encode_max_depth = max_depth})
> 
> Just in case: we recently close an [issue][1] when json module handles
> max depth correctly for maps, but non-correctly for arrays. I think it
> worth to ensure that those cases works good both for msgpackffi too.
> 
> [1]: https://github.com/tarantool/tarantool/issues/4366
> 

Ok:

diff --git a/test/app-tap/msgpackffi.test.lua b/test/app-tap/msgpackffi.test.lua
index e26247625..a1028e0ba 100755
--- a/test/app-tap/msgpackffi.test.lua
+++ b/test/app-tap/msgpackffi.test.lua
@@ -36,7 +36,7 @@ local function test_offsets(test, s)
 end
 
 local function test_other(test, s)
-    test:plan(21)
+    test:plan(23)
     local buf = string.char(0x93, 0x6e, 0xcb, 0x42, 0x2b, 0xed, 0x30, 0x47,
         0x6f, 0xff, 0xff, 0xac, 0x77, 0x6b, 0x61, 0x71, 0x66, 0x7a, 0x73,
         0x7a, 0x75, 0x71, 0x71, 0x78)
@@ -91,6 +91,20 @@ local function test_other(test, s)
     result_depth = check_depth(max_depth + 5)
     test:is(result_depth, max_depth + 5, "and uses it dynamically")
 
+    -- Recursive tables are handled correctly.
+    local level = 0
+    local t = {}
+    t[1] = t
+    t = s.decode(s.encode(t))
+    while t ~= nil do level = level + 1 t = t[1] end
+    test:is(level, max_depth + 5, "recursive array")
+    t = {}
+    t.key = t
+    t = s.decode(s.encode(t))
+    level = 0
+    while t ~= nil do level = level + 1 t = t.key end
+    test:is(level, max_depth + 5, "recursive map")
+
     msgpack.cfg({encode_max_depth = max_depth})
 end