Re: [Tarantool-patches] [PATCH] box: make box.execute() immutable

[Igor Munkin <imun@tarantool.org>]

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
> 

<snipped>

[1]: https://www.tarantool.io/en/doc/2.2/dev_guide/developer_guidelines/
[2]: https://github.com/tarantool/doc/issues/1004

-- 
Best regards,
IM