Re: [Tarantool-patches] [PATCH] box: netbox.self and connect should work interchangeably

[Oleg Babin <olegrok@tarantool.org>]

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

Hello, thanks for your patch!

> +    for i = 1, select('#', ...) do
> +        if type(results[i] == 'cdata') then
> +            results[i] = msgpack.decode(msgpack.encode(results[i]))
I suppose that it should be  "type(results[i]) == 'cdata'". Now this 
condition works always.

> +    if not ... then
> +        return ...
> +    end

What do you want to achieve with it?

This condition can't distinguish what it's a function without arguments 
or has nil as first argument.

I think this check should be dropped or replaced to something as 
"select('#', ...) == 0".

> +    local results = {...}

This transformation drops trailing nils that should be evaluated to 
box.NULL.

Please, add tests to check that all nils you passed will be converted to 
box.NULLs after transformation.


--
Oleg Babin


On 13/12/2019 00:28, Maria wrote:
> Despite what was stated in the documentation, netbox.connect was not always
> equivalent to netbox.self. In particular, they converted tuple to different
> types - table and cdata respectively.
> Netbox.self also allowed to modify source objects after transfer, which was
> not an example of expected behavior either.
>
> The patch fixes both those issues and covers all cases where netbox.self and
> connect perform conversion of types - e.g., for box.error.
>
> Closes #4513, #4602
> ---
> Issues:
> https://github.com/tarantool/tarantool/issues/4513
> https://github.com/tarantool/tarantool/issues/4602
> Branch:
> https://github.com/tarantool/tarantool/compare/eljashm/gh-4513-netbox.self-convert-tuples-to-table-type
>
>   src/box/lua/net_box.lua            | 11 +++-
>   test/box/net.box.result            |  4 +-
>   test/box/net_connect_self.result   | 81 ++++++++++++++++++++++++++++++
>   test/box/net_connect_self.test.lua | 33 ++++++++++++
>   4 files changed, 125 insertions(+), 4 deletions(-)
>   create mode 100644 test/box/net_connect_self.result
>   create mode 100644 test/box/net_connect_self.test.lua
>
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index c2e1bb9c4..aa36ba7d1 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -1523,7 +1523,16 @@ local function handle_eval_result(status, ...)
>           rollback()
>           return box.error(E_PROC_LUA, (...))
>       end
> -    return ...
> +    if not ... then
> +        return ...
> +    end
> +    local results = {...}
> +    for i = 1, select('#', ...) do
> +        if type(results[i] == 'cdata') then
> +            results[i] = msgpack.decode(msgpack.encode(results[i]))
> +        end
> +    end
> +    return unpack(results)
>   end
>   
>   this_module.self = {
> diff --git a/test/box/net.box.result b/test/box/net.box.result
> index e3dabf7d9..836a4fe8c 100644
> --- a/test/box/net.box.result
> +++ b/test/box/net.box.result
> @@ -626,9 +626,7 @@ box.schema.func.drop('pause')
>   -- call
>   remote.self:call('test_foo', {'a', 'b', 'c'})
>   ---
> -- - - a: 1
> -  - - b: 2
> -  - c
> +- [[{'a': 1}], [{'b': 2}], 'c']
>   ...
>   cn:call('test_foo', {'a', 'b', 'c'})
>   ---
> diff --git a/test/box/net_connect_self.result b/test/box/net_connect_self.result
> new file mode 100644
> index 000000000..b6ec38fb8
> --- /dev/null
> +++ b/test/box/net_connect_self.result
> @@ -0,0 +1,81 @@
> +-- test-run result file version 2
> +remote = require('net.box')
> + | ---
> + | ...
> +--
> +-- gh-4513 netbox.connect and netbox.self should be interchangeable
> +--
> +space = box.schema.space.create('gh4513')
> + | ---
> + | ...
> +box.schema.user.grant('guest','read, write, execute','universe')
> + | ---
> + | ...
> +idx = box.space.gh4513:create_index('primary')
> + | ---
> + | ...
> +box.space.gh4513:insert({1})
> + | ---
> + | - [1]
> + | ...
> +
> +type(remote.connect(box.cfg.listen):eval("return box.space.gh4513:get({1})"))
> + | ---
> + | - table
> + | ...
> +type(remote.self:eval("return box.space.gh4513:get{1}"))
> + | ---
> + | - table
> + | ...
> +
> +type(remote.connect(box.cfg.listen):eval('return box.error.new(1, "test error")'))
> + | ---
> + | - string
> + | ...
> +type(remote.self:eval('return box.error.new(1, "test error")'))
> + | ---
> + | - string
> + | ...
> +
> +type(remote.self:eval("return box.NULL"))
> + | ---
> + | - cdata
> + | ...
> +type(remote.connect(box.cfg.listen):eval("return box.NULL"))
> + | ---
> + | - cdata
> + | ...
> +
> +--
> +-- gh-4602 net.box:self allows to modify source object after transfer
> +--
> +x = {}
> + | ---
> + | ...
> +function test() return x end
> + | ---
> + | ...
> +box.schema.func.create('test')
> + | ---
> + | ...
> +box.schema.user.grant('guest', 'execute', 'function', 'test')
> + | ---
> + | ...
> +
> +y = remote.connect(box.cfg.listen):call('test')
> + | ---
> + | ...
> +-- should be false
> +y == x
> + | ---
> + | - false
> + | ...
> +
> +z = remote.self:call('test')
> + | ---
> + | ...
> +-- should be false as well
> +z == x
> + | ---
> + | - false
> + | ...
> diff --git a/test/box/net_connect_self.test.lua b/test/box/net_connect_self.test.lua
> new file mode 100644
> index 000000000..76b16704f
> --- /dev/null
> +++ b/test/box/net_connect_self.test.lua
> @@ -0,0 +1,33 @@
> +remote = require('net.box')
> +--
> +-- gh-4513 netbox.connect and netbox.self should be interchangeable
> +--
> +space = box.schema.space.create('gh4513')
> +box.schema.user.grant('guest','read, write, execute','universe')
> +idx = box.space.gh4513:create_index('primary')
> +box.space.gh4513:insert({1})
> +
> +type(remote.connect(box.cfg.listen):eval("return box.space.gh4513:get({1})"))
> +type(remote.self:eval("return box.space.gh4513:get{1}"))
> +
> +type(remote.connect(box.cfg.listen):eval('return box.error.new(1, "test error")'))
> +type(remote.self:eval('return box.error.new(1, "test error")'))
> +
> +type(remote.self:eval("return box.NULL"))
> +type(remote.connect(box.cfg.listen):eval("return box.NULL"))
> +
> +--
> +-- gh-4602 net.box:self allows to modify source object after transfer
> +--
> +x = {}
> +function test() return x end
> +box.schema.func.create('test')
> +box.schema.user.grant('guest', 'execute', 'function', 'test')
> +
> +y = remote.connect(box.cfg.listen):call('test')
> +-- should be false
> +y == x
> +
> +z = remote.self:call('test')
> +-- should be false as well
> +z == x

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