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

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Hi! Thanks for the patch!

See 3 comments below.

On 17/01/2020 18:08, 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.
> 
> The patch fixes the issue and covers all cases where netbox.self and connect
> perform conversion of types - e.g., for box.error.
> 
> Closes #4513
> ---
> Branch:
> https://github.com/tarantool/tarantool/compare/eljashm/gh-4513-netbox.self-convert-tuples-to-table-type
> Issue:
> https://github.com/tarantool/tarantool/issues/4513
> 
>  src/box/lua/net_box.lua       | 10 ++++++++-
>  test/app-tap/debug.result     |  8 +++----
>  test/box-tap/net.box.test.lua | 39 ++++++++++++++++++++++++++++++++---
>  3 files changed, 49 insertions(+), 8 deletions(-)
> 
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index b4811edfa..47ac72b80 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -1550,7 +1550,15 @@ local function handle_eval_result(status, ...)
>          rollback()
>          return box.error(E_PROC_LUA, (...))
>      end
> -    return ...
> +
> +    local results = {...}
> +    for i = 0, select('#', results) do
> +        if type(results[i]) == 'cdata' then
> +            results[i] = msgpack.decode(msgpack.encode(results[i]))
> +        end
> +    end
> +    

1. This line contains only whitespaces. Please, drop it.

> +    return unpack(results)
>  end

2. The code does not work, if I pass a cdata object in any
returned value except first.

tarantool> netbox = require('net.box')
---
...

tarantool> a, b = netbox.self:eval('return 1, box.tuple.new({2})')
---
...

tarantool> type(b)
---
- cdata
...

This is because

1) In Lua indexes start from 1. You start the loop from 0.

2) 'select('#', {...}) is always one. Because 'select('#', ...)'
  returns number of the arguments starting from the second. In
  your case you passed one argument - a table {...}.
  You should use 'select('#', ...)' to get the element count.
  Note, you can't use #results, because it will return an incorrect
  value in case '...' contains a nil.

> diff --git a/test/box-tap/net.box.test.lua b/test/box-tap/net.box.test.lua
> index a46f28ad0..fefbe7ff7 100755
> --- a/test/box-tap/net.box.test.lua
> +++ b/test/box-tap/net.box.test.lua
> @@ -7,7 +7,7 @@ local net_box = require('net.box')
>  local test_run = require('test_run')
>  local inspector = test_run.new()
>  
> -test:plan(5)
> +test:plan(11)
>  
>  -- create tarantool instance
>  test:is(
> @@ -27,8 +27,41 @@ test:is(conn.space ~= nil, true, 'space exists')
>  -- gh-1814: Segfault if using `net.box` before `box.cfg` start
>  test:ok(not pcall(function() conn.space._vspace:insert() end), "error handling")
>  
> +--
> +-- gh-4513 netbox.connect and netbox.self should be interchangeable
> +--
> +box.cfg{
> +    listen = os.getenv('LISTEN')
> +}

3. I don't think tap is a good engine for such a test. But up to
you. What is more important is that

1) If you use tap, then you should declare variables properly,
using 'local' prefix. Below you sometimes use local, sometimes
not. We need at least to be consistent. Either local is everywhere,
or nowhere.

2) You don't really need spaces to create and test return of a
tuple. There is 'box.tuple.new' function for that. JFYI, up to you
whether you want to use it and drop these space/index things.

3) To make the test simpler you can use the same what we did for
sql/bind.test.lua. It is run 2 times with different configuration
option 'remote' values: true and false. When the test is started
with remote true, we use netbox.connect(). When the test is started
with remote false, we use a local function. You can do the same
here. In the beginning you do something like this:

remote = test_run:get_cfg('remote') == 'true'
nb = nil
if remote then					\
	box.schema.user.grant('guest','super')	\
	nb = netbox.connect(box.cfg.listen)	\
else						\
	nb = netbox.self			\
end

Then you use and test 'nb.call/eval'. Test-run will run your test 2
times with remote true and remote false, so you will cover both remote
and local netbox instances in one test. Instead of duplicating each
test like you did below.

sql/bind.test.lua chooses 'remote' values in sql/engine.cfg file,
which is specified in sql/suite.ini. You need to do something similar.
And I guess in a separate file, since it is our policy, and because
there are no other common netbox test files which need to be run twice.

> +
> +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})
> +
> +local dst = box.cfg.listen
> +local t1 = type(net_box.connect(dst):eval("return box.space.gh4513:get({1})"))
> +local t2 = type(net_box.self:eval("return box.space.gh4513:get{1}"))
> +
> +-- connect and self should convert tuples to tables
> +test:is(t1, "table", "connect converts to table")
> +test:is(t2, "table", "self convert to table")
> +
> +t1 = type(net_box.connect(dst):eval("return box.error.new(1, 'test error')"))
> +t2 = type(net_box.self:eval("return box.error.new(1, 'test error')"))
> +
> +-- check for converting strings
> +test:is(t1, "string", "connect converts to strings")
> +test:is(t2, "string", "self converts to strings")
> +
> +t1 = type(net_box.connect(dst):eval("return box.NULL"))
> +t2 = type(net_box.self:eval("return box.NULL"))
> +
> +-- check for converting cdata
> +test:is(t1, "cdata", "connect converts to cdata")
> +test:is(t2, "cdata", "self converts to cdata")
> +
>  -- cleanup
>  conn:close()
>  inspector:cmd('stop server second with cleanup=1')
> -test:check()
> -os.exit(0)
> +os.exit(test:check() and 0 or 1)
>