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

["Maria Khaydich" <maria.khaydich@tarantool.org>]

[-- Attachment #1: Type: text/plain, Size: 5257 bytes --]


Fixed minor comments.
 
Sasha, you can proceed with the review. 
  
>Понедельник, 13 января 2020, 18:50 +03:00 от 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 
 
 
--
Maria Khaydich
 

[-- Attachment #2: Type: text/html, Size: 7115 bytes --]