From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 6AC0C253FE for ; Fri, 13 Sep 2019 18:28:45 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id xV3XR6V6YNvz for ; Fri, 13 Sep 2019 18:28:45 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 8406925364 for ; Fri, 13 Sep 2019 18:28:44 -0400 (EDT) Subject: [tarantool-patches] Re: [PATCH v2 2/4] msgpack: make msgpackffi use encode_max_depth option References: <945d59a2cc72f44f66e51c29e14cff5b4acae8c0.1568055477.git.v.shpilevoy@tarantool.org> <20190912232405.qrig7tt5dj7uvf3j@tkn_work_nb> From: Vladislav Shpilevoy Message-ID: <75b097fc-9cef-345a-07ad-e328a55df819@tarantool.org> Date: Sat, 14 Sep 2019 00:32:34 +0200 MIME-Version: 1.0 In-Reply-To: <20190912232405.qrig7tt5dj7uvf3j@tkn_work_nb> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: Alexander Turenko Cc: tarantool-patches@freelists.org 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