[Tarantool-patches] [PATCH] box: make box.execute() immutable
Igor Munkin
imun at tarantool.org
Wed Dec 25 04:30:38 MSK 2019
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
More information about the Tarantool-patches
mailing list