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

["Maria Khaydich" <maria.khaydich@tarantool.org>]

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