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

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

[Maria]

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
+    
+    return unpack(results)
 end
 
 this_module.self = {
diff --git a/test/app-tap/debug.result b/test/app-tap/debug.result
index 72934b076..cdc990307 100644
--- a/test/app-tap/debug.result
+++ b/test/app-tap/debug.result
@@ -15,15 +15,15 @@ nil
 Exec: tarantool -e "print(debug.__dir__); os.exit(0)"
 .
 Exec: tarantool -e "print(require('net.box').self:call('debug.sourcefile')); os.exit(0)"
-nil
+
 Exec: tarantool -e "print(require('net.box').self:call('debug.sourcedir')); os.exit(0)"
 .
 Exec: tarantool -e "fn = function() return debug.__file__ end; print(require('net.box').self:call('fn')); os.exit(0)"
-nil
+
 Exec: tarantool -e "fn = function() return debug.__dir__ end; print(require('net.box').self:call('fn')); os.exit(0)"
 .
 Exec: tarantool -e "fn = function() local res = debug.sourcefile(); return res end; print(require('net.box').self:call('fn')); os.exit(0)"
-nil
+
 Exec: tarantool -e "fn = function() local res = debug.sourcedir(); return res end; print(require('net.box').self:call('fn')); os.exit(0)"
 .
 Exec: tarantool -e "print(loadstring('return debug.sourcefile()')()); os.exit(0)"
@@ -52,7 +52,7 @@ debug/test.lua
 Source: print(debug.__dir__)
 debug
 Source: print(require('net.box').self:call('debug.sourcefile'))
-nil
+
 Source: print(require('net.box').self:call('debug.sourcedir'))
 .
 Source: fn = function() return debug.__file__ end; print(require('net.box').self:call('fn'))
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')
+}
+
+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)
-- 
2.24.0

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

[Vladislav Shpilevoy]

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)
> 

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

[Maria Khaydich]

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


Thank you for the review! All fixes below
 
----------------------------------------------------------------------
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                       |  8 +++-
 test/app-tap/debug.result                     |  8 ++--
 test/box-tap/net.box.test.lua                 |  3 +-
 test/box/engine.cfg                           |  6 +++
 ...ox-self-and-connect-interchangeable.result | 44 +++++++++++++++++++
 ...-self-and-connect-interchangeable.test.lua | 20 +++++++++
 test/box/suite.ini                            |  1 +
 7 files changed, 83 insertions(+), 7 deletions(-)
 create mode 100644 test/box/engine.cfg
 create mode 100644 test/box/gh-4513-netbox-self-and-connect-interchangeable.result
 create mode 100644 test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index b4811edfa..3f611c027 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -1550,7 +1550,13 @@ local function handle_eval_result(status, ...)
         rollback()
         return box.error(E_PROC_LUA, (...))
     end
-    return ...
+    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/app-tap/debug.result b/test/app-tap/debug.result
index 72934b076..cdc990307 100644
--- a/test/app-tap/debug.result
+++ b/test/app-tap/debug.result
@@ -15,15 +15,15 @@ nil
 Exec: tarantool -e "print(debug.__dir__); os.exit(0)"
 .
 Exec: tarantool -e "print(require('net.box').self:call('debug.sourcefile')); os.exit(0)"
-nil
+
 Exec: tarantool -e "print(require('net.box').self:call('debug.sourcedir')); os.exit(0)"
 .
 Exec: tarantool -e "fn = function() return debug.__file__ end; print(require('net.box').self:call('fn')); os.exit(0)"
-nil
+
 Exec: tarantool -e "fn = function() return debug.__dir__ end; print(require('net.box').self:call('fn')); os.exit(0)"
 .
 Exec: tarantool -e "fn = function() local res = debug.sourcefile(); return res end; print(require('net.box').self:call('fn')); os.exit(0)"
-nil
+
 Exec: tarantool -e "fn = function() local res = debug.sourcedir(); return res end; print(require('net.box').self:call('fn')); os.exit(0)"
 .
 Exec: tarantool -e "print(loadstring('return debug.sourcefile()')()); os.exit(0)"
@@ -52,7 +52,7 @@ debug/test.lua
 Source: print(debug.__dir__)
 debug
 Source: print(require('net.box').self:call('debug.sourcefile'))
-nil
+
 Source: print(require('net.box').self:call('debug.sourcedir'))
 .
 Source: fn = function() return debug.__file__ end; print(require('net.box').self:call('fn'))
diff --git a/test/box-tap/net.box.test.lua b/test/box-tap/net.box.test.lua
index a46f28ad0..577e97d78 100755
--- a/test/box-tap/net.box.test.lua
+++ b/test/box-tap/net.box.test.lua
@@ -30,5 +30,4 @@ test:ok(not pcall(function() conn.space._vspace:insert() end), "error handling")
 -- cleanup
 conn:close()
 inspector:cmd('stop server second with cleanup=1')
-test:check()
-os.exit(0)
+os.exit(test:check() and 0 or 1)
diff --git a/test/box/engine.cfg b/test/box/engine.cfg
new file mode 100644
index 000000000..2d1d450e3
--- /dev/null
+++ b/test/box/engine.cfg
@@ -0,0 +1,6 @@
+{
+    "gh-4513-netbox-self-and-connect-interchangeable.test.lua": {
+        "remote": {"remote": "true"},
+        "local": {"remote": "false"}
+    }
+}
diff --git a/test/box/gh-4513-netbox-self-and-connect-interchangeable.result b/test/box/gh-4513-netbox-self-and-connect-interchangeable.result
new file mode 100644
index 000000000..64bd129b3
--- /dev/null
+++ b/test/box/gh-4513-netbox-self-and-connect-interchangeable.result
@@ -0,0 +1,44 @@
+-- test-run result file version 2
+netbox = require('net.box')
+ | ---
+ | ...
+test_run = require('test_run').new()
+ | ---
+ | ...
+remote = test_run:get_cfg('remote') == 'true'
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+
+nb = nil
+if remote then
+    box.schema.user.grant('guest','super')
+    nb = netbox.connect(box.cfg.listen)
+else
+    nb = netbox.self
+end;
+ | ---
+ | ...
+
+--
+-- netbox:self and netbox:connect should work interchangeably
+--
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | - true
+ | ...
+type(nb:eval('return box.tuple.new{1}')) -- table
+ | ---
+ | - table
+ | ...
+type(nb:eval('return box.error.new(1, "test error")')) -- string
+ | ---
+ | - string
+ | ...
+type(nb:eval('return box.NULL')) -- cdata
+ | ---
+ | - cdata
+ | ...
diff --git a/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua b/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua
new file mode 100644
index 000000000..5e8ae7465
--- /dev/null
+++ b/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua
@@ -0,0 +1,20 @@
+netbox = require('net.box')
+test_run = require('test_run').new()
+remote = test_run:get_cfg('remote') == 'true'
+test_run:cmd("setopt delimiter ';'")
+
+nb = nil
+if remote then
+    box.schema.user.grant('guest','super')
+    nb = netbox.connect(box.cfg.listen)
+else
+    nb = netbox.self
+end;
+
+--
+-- netbox:self and netbox:connect should work interchangeably
+--
+test_run:cmd("setopt delimiter ''");
+type(nb:eval('return box.tuple.new{1}')) -- table
+type(nb:eval('return box.error.new(1, "test error")')) -- string
+type(nb:eval('return box.NULL')) -- cdata
diff --git a/test/box/suite.ini b/test/box/suite.ini
index 9aa30ede6..01f07236b 100644
--- a/test/box/suite.ini
+++ b/test/box/suite.ini
@@ -3,6 +3,7 @@ core = tarantool
 description = Database tests
 script = box.lua
 disabled = rtree_errinj.test.lua tuple_bench.test.lua
+config = engine.cfg
 release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua gh-4648-func-load-unload.test.lua
 lua_libs = lua/fifo.lua lua/utils.lua lua/bitset.lua lua/index_random_test.lua lua/push.lua lua/identifier.lua
 use_unix_sockets = True
-- 
2.24.0 
>Вторник, 21 января 2020, 0:22 +03:00 от 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)
>> 
 
 
--
Maria Khaydich
 

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

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

[Igor Munkin]

Masha,

Thanks for the patch! I left some nits below, please consider them.
Otherwise, LGTM.

On 27.01.20, Maria Khaydich wrote:
> 
> Thank you for the review! All fixes below
>  
> ----------------------------------------------------------------------
> 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                       |  8 +++-
>  test/app-tap/debug.result                     |  8 ++--
>  test/box-tap/net.box.test.lua                 |  3 +-
>  test/box/engine.cfg                           |  6 +++
>  ...ox-self-and-connect-interchangeable.result | 44 +++++++++++++++++++
>  ...-self-and-connect-interchangeable.test.lua | 20 +++++++++
>  test/box/suite.ini                            |  1 +
>  7 files changed, 83 insertions(+), 7 deletions(-)
>  create mode 100644 test/box/engine.cfg
>  create mode 100644 test/box/gh-4513-netbox-self-and-connect-interchangeable.result
>  create mode 100644 test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua

<snipped>

> diff --git a/test/box-tap/net.box.test.lua b/test/box-tap/net.box.test.lua
> index a46f28ad0..577e97d78 100755
> --- a/test/box-tap/net.box.test.lua
> +++ b/test/box-tap/net.box.test.lua
> @@ -30,5 +30,4 @@ test:ok(not pcall(function() conn.space._vspace:insert() end), "error handling")
>  -- cleanup
>  conn:close()
>  inspector:cmd('stop server second with cleanup=1')
> -test:check()
> -os.exit(0)
> +os.exit(test:check() and 0 or 1)

The changes are fine but look like not related to the subj.

> diff --git a/test/box/engine.cfg b/test/box/engine.cfg

<snipped>

> diff --git a/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua b/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua
> new file mode 100644
> index 000000000..5e8ae7465
> --- /dev/null
> +++ b/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua
> @@ -0,0 +1,20 @@
> +netbox = require('net.box')
> +test_run = require('test_run').new()
> +remote = test_run:get_cfg('remote') == 'true'
> +test_run:cmd("setopt delimiter ';'")
> +
> +nb = nil
> +if remote then
> +    box.schema.user.grant('guest','super')
> +    nb = netbox.connect(box.cfg.listen)
> +else
> +    nb = netbox.self
> +end;
> +
> +--
> +-- netbox:self and netbox:connect should work interchangeably
> +--
> +test_run:cmd("setopt delimiter ''");

Minor: The line above relates to the nb initialization, so it's better
been placed before the comment. It's totally minor, feel free to ignore.

> +type(nb:eval('return box.tuple.new{1}')) -- table
> +type(nb:eval('return box.error.new(1, "test error")')) -- string
> +type(nb:eval('return box.NULL')) -- cdata

<snipped>

> -- 
> 2.24.0 

<snipped>

-- 
Best regards,
IM

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

[Maria Khaydich]

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


Both minor comments are fixed. 

  
>Среда, 26 февраля 2020, 16:56 +03:00 от Igor Munkin <imun@tarantool.org>:
> 
>Masha,
>
>Thanks for the patch! I left some nits below, please consider them.
>Otherwise, LGTM.
>
>On 27.01.20, Maria Khaydich wrote:
>>
>> Thank you for the review! All fixes below
>>  
>> ----------------------------------------------------------------------
>> 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                       |  8 +++-
>>  test/app-tap/debug.result                     |  8 ++--
>>  test/box-tap/net.box.test.lua                 |  3 +-
>>  test/box/engine.cfg                           |  6 +++
>>  ...ox-self-and-connect-interchangeable.result | 44 +++++++++++++++++++
>>  ...-self-and-connect-interchangeable.test.lua | 20 +++++++++
>>  test/box/suite.ini                            |  1 +
>>  7 files changed, 83 insertions(+), 7 deletions(-)
>>  create mode 100644 test/box/engine.cfg
>>  create mode 100644 test/box/gh-4513-netbox-self-and-connect-interchangeable.result
>>  create mode 100644 test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua
>
><snipped>
>
>> diff --git a/test/box-tap/net.box.test.lua b/test/box-tap/net.box.test.lua
>> index a46f28ad0..577e97d78 100755
>> --- a/test/box-tap/net.box.test.lua
>> +++ b/test/box-tap/net.box.test.lua
>> @@ -30,5 +30,4 @@ test:ok(not pcall(function() conn.space._vspace:insert() end), "error handling")
>>  -- cleanup
>>  conn:close()
>>  inspector:cmd('stop server second with cleanup=1')
>> -test:check()
>> -os.exit(0)
>> +os.exit(test:check() and 0 or 1)
>
>The changes are fine but look like not related to the subj.
>
>> diff --git a/test/box/engine.cfg b/test/box/engine.cfg
>
><snipped>
>
>> diff --git a/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua b/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua
>> new file mode 100644
>> index 000000000..5e8ae7465
>> --- /dev/null
>> +++ b/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua
>> @@ -0,0 +1,20 @@
>> +netbox = require('net.box')
>> +test_run = require('test_run').new()
>> +remote = test_run:get_cfg('remote') == 'true'
>> +test_run:cmd("setopt delimiter ';'")
>> +
>> +nb = nil
>> +if remote then
>> +    box.schema.user.grant('guest','super')
>> +    nb = netbox.connect(box.cfg.listen)
>> +else
>> +    nb = netbox.self
>> +end;
>> +
>> +--
>> +-- netbox:self and netbox:connect should work interchangeably
>> +--
>> +test_run:cmd("setopt delimiter ''");
>
>Minor: The line above relates to the nb initialization, so it's better
>been placed before the comment. It's totally minor, feel free to ignore.
>
>> +type(nb:eval('return box.tuple.new{1}')) -- table
>> +type(nb:eval('return box.error.new(1, "test error")')) -- string
>> +type(nb:eval('return box.NULL')) -- cdata
>
><snipped>
>
>> -- 
>> 2.24.0
>
><snipped>
>
>--
>Best regards,
>IM 
 
 
--
Maria Khaydich
 

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

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

[Vladislav Shpilevoy]

Thanks for the patch!

See 2 comments below.

> diff --git a/test/box/gh-4513-netbox-self-and-connect-interchangeable.result b/test/box/gh-4513-netbox-self-and-connect-interchangeable.result
> new file mode 100644
> index 000000000..fa41b2442
> --- /dev/null
> +++ b/test/box/gh-4513-netbox-self-and-connect-interchangeable.result
> @@ -0,0 +1,44 @@
> +-- test-run result file version 2
> +netbox = require('net.box')
> + | ---
> + | ...
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +remote = test_run:get_cfg('remote') == 'true'
> + | ---
> + | ...
> +test_run:cmd("setopt delimiter ';'")
> + | ---
> + | - true
> + | ...
> +
> +nb = nil
> +if remote then
> +    box.schema.user.grant('guest','super')

1. Grant was given, but was not revoked. This can affect next
tests running on the same instance.

> +    nb = netbox.connect(box.cfg.listen)
> +else
> +    nb = netbox.self
> +end;
> + | ---
> + | ...
> +test_run:cmd("setopt delimiter ''");

2. We have a nice syntax of '\' to create multi-line expressions
like this. For such a small piece of code this would look better
than 2 'setopt delimiter', IMO. Up to you.

> + | ---
> + | - true
> + | ...
> +
> +--
> +-- netbox:self and netbox:connect should work interchangeably
> +--
> +type(nb:eval('return box.tuple.new{1}')) -- table
> + | ---
> + | - table
> + | ...
> +type(nb:eval('return box.error.new(1, "test error")')) -- string
> + | ---
> + | - string
> + | ...
> +type(nb:eval('return box.NULL')) -- cdata
> + | ---
> + | - cdata
> + | ...

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

[Maria Khaydich]

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


Thank you for the review! Fixes are done:
----------------------------------------------------------------------
Branch:
https://github.com/tarantool/tarantool/compare/eljashm/gh-4513-netbox.self-convert-tuples-to-table-type  
 
 src/box/lua/net_box.lua                       |  8 +++-
 test/app-tap/debug.result                     |  8 ++--
 test/box/engine.cfg                           |  6 +++
 ...ox-self-and-connect-interchangeable.result | 45 +++++++++++++++++++
 ...-self-and-connect-interchangeable.test.lua | 23 ++++++++++
 test/box/suite.ini                            |  1 +
 6 files changed, 86 insertions(+), 5 deletions(-)
 create mode 100644 test/box/engine.cfg
 create mode 100644 test/box/gh-4513-netbox-self-and-connect-interchangeable.result
 create mode 100644 test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua
 
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index b4811edfa..3f611c027 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -1550,7 +1550,13 @@ local function handle_eval_result(status, ...)
         rollback()
         return box.error(E_PROC_LUA, (...))
     end
-    return ...
+    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/app-tap/debug.result b/test/app-tap/debug.result
index 72934b076..cdc990307 100644
--- a/test/app-tap/debug.result
+++ b/test/app-tap/debug.result
@@ -15,15 +15,15 @@ nil
 Exec: tarantool -e "print(debug.__dir__); os.exit(0)"
 .
 Exec: tarantool -e "print(require('net.box').self:call('debug.sourcefile')); os.exit(0)"
-nil
+
 Exec: tarantool -e "print(require('net.box').self:call('debug.sourcedir')); os.exit(0)"
 .
 Exec: tarantool -e "fn = function() return debug.__file__ end; print(require('net.box').self:call('fn')); os.exit(0)"
-nil
+
 Exec: tarantool -e "fn = function() return debug.__dir__ end; print(require('net.box').self:call('fn')); os.exit(0)"
 .
 Exec: tarantool -e "fn = function() local res = debug.sourcefile(); return res end; print(require('net.box').self:call('fn')); os.exit(0)"
-nil
+
 Exec: tarantool -e "fn = function() local res = debug.sourcedir(); return res end; print(require('net.box').self:call('fn')); os.exit(0)"
 .
 Exec: tarantool -e "print(loadstring('return debug.sourcefile()')()); os.exit(0)"
@@ -52,7 +52,7 @@ debug/test.lua
 Source: print(debug.__dir__)
 debug
 Source: print(require('net.box').self:call('debug.sourcefile'))
-nil
+
 Source: print(require('net.box').self:call('debug.sourcedir'))
 .
 Source: fn = function() return debug.__file__ end; print(require('net.box').self:call('fn'))
diff --git a/test/box/engine.cfg b/test/box/engine.cfg
new file mode 100644
index 000000000..2d1d450e3
--- /dev/null
+++ b/test/box/engine.cfg
@@ -0,0 +1,6 @@
+{
+    "gh-4513-netbox-self-and-connect-interchangeable.test.lua": {
+        "remote": {"remote": "true"},
+        "local": {"remote": "false"}
+    }
+}
 
diff --git a/test/box/gh-4513-netbox-self-and-connect-interchangeable.result b/test/box/gh-4513-netbox-self-and-connect-interchangeable.result
new file mode 100644
index 000000000..67000f9c8
--- /dev/null
+++ b/test/box/gh-4513-netbox-self-and-connect-interchangeable.result
@@ -0,0 +1,45 @@
+-- test-run result file version 2
+netbox = require('net.box')
+ | ---
+ | ...
+test_run = require('test_run').new()
+ | ---
+ | ...
+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
+ | ---
+ | ...
+
+--
+-- netbox:self and netbox:connect should work interchangeably
+--
+type(nb:eval('return box.tuple.new{1}')) -- table
+ | ---
+ | - table
+ | ...
+type(nb:eval('return box.error.new(1, "test error")')) -- string
+ | ---
+ | - string
+ | ...
+type(nb:eval('return box.NULL')) -- cdata
+ | ---
+ | - cdata
+ | ...
+
+if remote then \
+    box.schema.user.revoke('guest', 'super') \
+    nb:close() \
+end
+ | ---
+ | ...
 
diff --git a/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua b/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua
new file mode 100644
index 000000000..010d6cf34
--- /dev/null
+++ b/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua
@@ -0,0 +1,23 @@
+netbox = require('net.box')
+test_run = require('test_run').new()
+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
+
+--
+-- netbox:self and netbox:connect should work interchangeably
+--
+type(nb:eval('return box.tuple.new{1}')) -- table
+type(nb:eval('return box.error.new(1, "test error")')) -- string
+type(nb:eval('return box.NULL')) -- cdata
+
+if remote then \
+    box.schema.user.revoke('guest', 'super') \
+    nb:close() \
+end
 
diff --git a/test/box/suite.ini b/test/box/suite.ini
index 9aa30ede6..01f07236b 100644
--- a/test/box/suite.ini
+++ b/test/box/suite.ini
@@ -3,6 +3,7 @@ core = tarantool
 description = Database tests
 script = box.lua
 disabled = rtree_errinj.test.lua tuple_bench.test.lua
+config = engine.cfg
 release_disabled = errinj.test.lua errinj_index.test.lua rtree_errinj.test.lua upsert_errinj.test.lua iproto_stress.test.lua gh-4648-func-load-unload.test.lua
 lua_libs = lua/fifo.lua lua/utils.lua lua/bitset.lua lua/index_random_test.lua lua/push.lua lua/identifier.lua
 use_unix_sockets = True
-- 
2.24.0 
>Среда, 4 марта 2020, 1:49 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:
> 
>Thanks for the patch!
>
>See 2 comments below.
>
>> diff --git a/test/box/gh-4513-netbox-self-and-connect-interchangeable.result b/test/box/gh-4513-netbox-self-and-connect-interchangeable.result
>> new file mode 100644
>> index 000000000..fa41b2442
>> --- /dev/null
>> +++ b/test/box/gh-4513-netbox-self-and-connect-interchangeable.result
>> @@ -0,0 +1,44 @@
>> +-- test-run result file version 2
>> +netbox = require('net.box')
>> + | ---
>> + | ...
>> +test_run = require('test_run').new()
>> + | ---
>> + | ...
>> +remote = test_run:get_cfg('remote') == 'true'
>> + | ---
>> + | ...
>> +test_run:cmd("setopt delimiter ';'")
>> + | ---
>> + | - true
>> + | ...
>> +
>> +nb = nil
>> +if remote then
>> + box.schema.user.grant('guest','super')
>
>1. Grant was given, but was not revoked. This can affect next
>tests running on the same instance.
>
>> + nb = netbox.connect(box.cfg.listen)
>> +else
>> + nb = netbox.self
>> +end;
>> + | ---
>> + | ...
>> +test_run:cmd("setopt delimiter ''");
>
>2. We have a nice syntax of '\' to create multi-line expressions
>like this. For such a small piece of code this would look better
>than 2 'setopt delimiter', IMO. Up to you.
>
>> + | ---
>> + | - true
>> + | ...
>> +
>> +--
>> +-- netbox:self and netbox:connect should work interchangeably
>> +--
>> +type(nb:eval('return box.tuple.new{1}')) -- table
>> + | ---
>> + | - table
>> + | ...
>> +type(nb:eval('return box.error.new(1, "test error")')) -- string
>> + | ---
>> + | - string
>> + | ...
>> +type(nb:eval('return box.NULL')) -- cdata
>> + | ---
>> + | - cdata
>> + | ...
>  
 
 
--
Maria Khaydich
 

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

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

[Vladislav Shpilevoy]

Thanks for working on this, LGTM.

Here is @ChangeLog for Kirill:
- netbox.self:call/eval() now returns all the same types, as
  netbox_connection:call/eval. Previously it could return
  tuple or box.error cdata (gh-4513).

Since we got 2 LGTMs, I committed the patch to master, 2.3, 2.2,
1.10. The changelog is added to release notes.

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

[Maria]

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
-- 
2.20.1 (Apple Git-117)

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

[Oleg Babin]

[-- 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 --]