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

[Maria Khaydich]

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:
https://github.com/tarantool/tarantool/compare/eljashm/gh-4231-box.execute-immutable-function
Issue:
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")
+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")
+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 at 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
 
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.tarantool.org/pipermail/tarantool-patches/attachments/20200113/917aca77/attachment.html>