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

[Igor Munkin <imun@tarantool.org>]

Masha,

Thanks, the patch LGTM, except a couple minor comments below. Please
consider them.

Sasha, please proceed with the patch.

On 13.01.20, Maria Khaydich wrote:
>    Thank you for the review.
> 
>    I’ve changed the comment describing box.execute method to explain why
>    an extra flag is needed.
>    And also added polishing fixes proposed by Igor.
> 
>    Result:
> 
>    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 immutable.
> 
>    Closes #4231
>    ---
>    Branch:
>    [1]https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.e
>    xecute-immutable-function
>    Issue:
>    [2]https://github.com/tarantool/tarantool/issues/4231
>     src/box/lua/load_cfg.lua                      | 15 ++++++++-
>     .../gh-4231-immutable-box-execute.test.lua    | 32 +++++++++++++++++++
>     2 files changed, 46 insertions(+), 1 deletion(-)
>     create mode 100755 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 85617c8f0..1f9b4392f 100644
>    --- a/src/box/lua/load_cfg.lua
>    +++ b/src/box/lua/load_cfg.lua
>    @@ -517,10 +517,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
>    @@ -564,7 +568,16 @@ box.cfg = locked(load_cfg)
>     -- metatable.
>     --
>     function box.execute(...)
>    -    load_cfg()
>    +    --
>    +    -- Saving box.execute method before explicit calls to either
>    +    -- box.execute() or box.cfg{} leads to implicit invocation of
>    +    -- box.cfg nonetheless. The flag box_loaded makes sure execute
>    +    -- is an immutable function meaning it won't be overwritten by
>    +    -- any following attempts to reconfigure box.
>    +    --
>    +    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 100755
>    index 000000000..d8940a0b2
>    --- /dev/null
>    +++ b/test/box-tap/gh-4231-immutable-box-execute.test.lua
>    @@ -0,0 +1,32 @@
>    +#!/usr/bin/env tarantool
>    +local tap = require('tap')
>    +local test = tap.test('execute')
>    +test:plan(4)
>    +
>    +--
>    +-- gh-4231: box.execute should be immutable function meaning it's
>    +-- address doesn't change after first box.cfg implicit invocation
>    +--
>    +
>    +local function execute_is_immutable(execute, cmd, msg)
>    +    local status, err = pcall(function()
>    +        execute(cmd)
>    +    end)
>    +    test:ok(status and err == nil, msg)
>    +end
>    +
>    +local box_execute_stub = box.execute
>    +test:is(box_execute_stub, box.execute, "execute is not the same before
>    box.cfg")

Minor: this test is a trivial and excess one. I suggest to drop it.

>    +execute_is_immutable(box_execute_stub, "SELECT 1",
>    +    "execute does not work properly before box.cfg")
>    +
>    +local box_execute_actual = box.execute
>    +-- explicit call to load_cfg
>    +box.cfg{}
>    +--  checking  the function was not reconfigured, i.e. adress stays the
>    same
>    +test:is(box_execute_actual,  box.execute,  "execute  is  not  the same
>    after box.cfg")

Minor: out of 80 chars

>    +execute_is_immutable(box_execute_actual,
>    +    "CREATE TABLE t1 (s1 INTEGER, PRIMARY KEY (s1));",
>    +    "execute does not work properly after box.cfg")
>    +
>    +os.exit(test:check() and 0 or 1)
>    --
>    2.24.0
> 
>      Четверг, 26 декабря 2019, 17:08 +03:00 от Alexander Turenko
>      <alexander.turenko@tarantool.org>:
> 
>    >  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
>    >  +      --  result  -  to  an error when trying to use execute method
>    again.
>    > +    -- The check makes sure box.execute is an immutable function.
>    > +    --
>    I would describe all this hell with registering lbox_execute as
>    box.execute, moving it to box_cfg_guard_whitelist, adding this
>    box.execute function, then replacing back with lbox_execute at
>    load_cfg().
>    This  description  would  make  clear why everything is fine if we just
>    call
>    box.execute(), but extra flag is necessary if a user saved box.execute
>    before first box.execute() or box.cfg() call.
> 
> 
> 
>    --
>    Maria Khaydich
> 
> References
> 
>    1. https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function
>    2. https://github.com/tarantool/tarantool/issues/4231

-- 
Best regards,
IM