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 32E2824E03 for ; Thu, 12 Sep 2019 19:32:49 -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 k1KXpPLOno8V for ; Thu, 12 Sep 2019 19:32:49 -0400 (EDT) Received: from smtp16.mail.ru (smtp16.mail.ru [94.100.176.153]) (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 E133524DFB for ; Thu, 12 Sep 2019 19:32:48 -0400 (EDT) Date: Fri, 13 Sep 2019 02:32:32 +0300 From: Alexander Turenko Subject: [tarantool-patches] Re: [PATCH v2 4/4] app: allow to raise an error on too nested tables Message-ID: <20190912233231.zs3c6o2zv7pyl6lo@tkn_work_nb> References: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: 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: Vladislav Shpilevoy Cc: tarantool-patches@freelists.org > app: allow to raise an error on too nested tables The commit header and the description looks like we have 'crop' behaviour as default, however it is not more so. I agree that we should change the default behaviour and raise an error by default. This however should be clearly stated in the docbot request. See more comments below. WBR, Alexander Turenko. > msgpack.cfg({encode_crop_too_deep = false}) We have encode_invalid_as_nil with the similar meanings, so, maybe, encode_too_deep_as_nil? Or maybe rephrase 'too deep' as one word: foots, dregs, underside? Those variants don't look good, but maybe you know some word that would fit better. > + if (! cfg->encode_crop_too_deep) > + return luaL_error(L, "Too high nest level"); We can easily give more information: say, a current max depth level; so I think it worth to add to the error message. (The same for msgpackffi and json.) > + -- gh-4434 (yes, the same issue): let users choose whether > + -- they want to raise an error on tables with too high nest > + -- level. > + -- > + s.cfg({encode_crop_too_deep = false}) > + > + local t = nil > + for i = 1, max_depth + 1 do t = {t} end > + local ok, err = pcall(s.encode, t) > + test:ok(not ok, "too deep encode depth") > + > + s.cfg({encode_max_depth = max_depth + 1}) > + ok, err = pcall(s.encode, t) > + test:ok(ok, "no throw in a corner case") > + > + s.cfg({encode_crop_too_deep = true, encode_max_depth = max_depth}) I think we should save a default value of the option and set it here as we do with encode_max_depth. (The same for msgpackffi.test.lua.) BTW, the default is 'false' and we should set the default value here (now we set 'true'). Following test cases (e.g. gh-2888 test case in app-tap/json.test.lua) should set and restore the option locally. Maybe it also worth add a test case that will verify that the default behaviour is to raise an error?