From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 dev.tarantool.org (Postfix) with ESMTPS id 93A5B46970E for ; Wed, 25 Dec 2019 04:32:49 +0300 (MSK) Date: Wed, 25 Dec 2019 04:30:38 +0300 From: Igor Munkin Message-ID: <20191225013038.GC31304@tarantool.org> References: <20191114115020.21091-1-maria.khaydich@tarantool.org> <20191217143940.GQ1214@tarantool.org> <1577201550.829136486@f483.i.mail.ru> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1577201550.829136486@f483.i.mail.ru> Subject: Re: [Tarantool-patches] [PATCH] box: make box.execute() immutable List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Maria Khaydich Cc: tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy Masha, Thanks for the patch! It LGTM in general, but please consider a couple polishing-aimed comments below. On 24.12.19, Maria Khaydich wrote: > Igor, thank you for the review! > Proposed fixes are done: > At first, please adjust the commit message subject considering our contribution guide[1]. I see the component prefix is missing. > > Using box.execute method before explicitly configuring box > automatically invoked box.cfg nonetheless. Any further calls > to box.cfg caused its reconfiguration which then led to an > error when trying to use execute method again. > > The patch introduces a fix making box.execute method immutable. > > Closes #4231 > --- > Issue: > https://github.com/tarantool/tarantool/issues/4231 > Branch: > https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.exec > ute-immutable-function > src/box/lua/load_cfg.lua | 15 +++++++++++- > .../gh-4231_immutable_box.execute.test.lua | 24 +++++++++++++++++++ > 2 files changed, 38 insertions(+), 1 deletion(-) > create mode 100644 test/box-tap/gh-4231_immutable_box.execute.test.lua > diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua > index e7f62cf4e..ad416e578 100644 > --- a/src/box/lua/load_cfg.lua > +++ b/src/box/lua/load_cfg.lua > @@ -481,10 +481,14 @@ setmetatable(box, { > end > }) > > +-- Flag needed to keep immutable functions after box reconfiguration > +local box_loaded = false > + > local function load_cfg(cfg) > cfg = upgrade_cfg(cfg, translate_cfg) > cfg = prepare_cfg(cfg, default_cfg, template_cfg, modify_cfg) > apply_default_cfg(cfg, default_cfg); > + box_loaded = true > -- Save new box.cfg > box.cfg = cfg > if not pcall(private.cfg_check) then > @@ -528,7 +532,16 @@ box.cfg = locked(load_cfg) > -- metatable. > -- > function box.execute(...) > - load_cfg() > + -- > + -- Calling box.execute before explicitly configuring box led to > + -- automatic box.cfg invocation nonetheless. Following explicit > + -- attempts to configure box led to its reconfiguratin and as a Typo: s/reconfiguratin/reconfiguration/. > + -- result - to an error when trying to use execute method again. > + -- The check makes sure box.execute is an immutable function. There is a trailing whitespace within this comment (I pulled your branch and checked it). Please adjust it. > + -- > + if not box_loaded then > + load_cfg() > + end > return box.execute(...) > end > > diff --git a/test/box-tap/gh-4231_immutable_box.execute.test.lua > b/test/box-tap/gh-4231_immutable_box.execute.test.lua > new file mode 100644 > index 000000000..e2469630e > --- /dev/null > +++ b/test/box-tap/gh-4231_immutable_box.execute.test.lua As Nikita mentioned before (and we have the issue for it[2]), we're going to use dashes instead of underscores. Please adjust the test chunk name regarding this policy. > @@ -0,0 +1,24 @@ > +#!/usr/bin/env tarantool > +local tap = require('tap') > +local test = tap.test('execute') > +test:plan(2) > + > +-- > +-- gh-4231: box.execute should be immutable function meaning it > doesn't > +-- change after first box.cfg implicit invocation > +-- > + > +local box_execute = box.execute > +local status, err = pcall(function() > + box_execute("SELECT 1") > +end) > +test:ok(status and err == nil, "box.execute does not work properly > before box.cfg") > + load_cfg has been already called here, so you can check that box.execute function prior to this call is the same as the one after it (as Nikita suggested before). > +box.cfg{} > + > +local status2, err2 = pcall(function() > + box_execute("CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));") > +end) > +test:ok(status2 and err2 == nil, "box.execute is changed after box.cfg > invocation") Minor: I guess you can move these lines to a separate function, since they are quite similar to the ones prior to box.cfg call. > + > +os.exit(test:check() and 0 or 1) > -- > 2.24.0 > [1]: https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/ [2]: https://github.com/tarantool/doc/issues/1004 -- Best regards, IM