<HTML><BODY><div><div>Thank you for the review! All fixes below</div><div> </div><hr><div>Branch:<br><a href="https://github.com/tarantool/tarantool/compare/eljashm/gh-4513-netbox.self-convert-tuples-to-table-type" target="_blank">https://github.com/tarantool/tarantool/compare/eljashm/gh-4513-netbox.self-convert-tuples-to-table-type</a><br>Issue:<br><a href="https://github.com/tarantool/tarantool/issues/4513" target="_blank">https://github.com/tarantool/tarantool/issues/4513</a></div><div> </div><div><div> src/box/lua/net_box.lua | 8 +++-<br> test/app-tap/debug.result | 8 ++--<br> test/box-tap/net.box.test.lua | 3 +-<br> test/box/engine.cfg | 6 +++<br> ...ox-self-and-connect-interchangeable.result | 44 +++++++++++++++++++<br> ...-self-and-connect-interchangeable.test.lua | 20 +++++++++<br> test/box/suite.ini | 1 +<br> 7 files changed, 83 insertions(+), 7 deletions(-)<br> create mode 100644 test/box/engine.cfg<br> create mode 100644 test/box/gh-4513-netbox-self-and-connect-interchangeable.result<br> create mode 100644 test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua</div><div>diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua<br>index b4811edfa..3f611c027 100644<br>--- a/src/box/lua/net_box.lua<br>+++ b/src/box/lua/net_box.lua<br>@@ -1550,7 +1550,13 @@ local function handle_eval_result(status, ...)<br> rollback()<br> return box.error(E_PROC_LUA, (...))<br> end<br>- return ...<br>+ local results = {...}<br>+ for i = 1, select('#', ...) do<br>+ if type(results[i]) == 'cdata' then<br>+ results[i] = msgpack.decode(msgpack.encode(results[i]))<br>+ end<br>+ end<br>+ return unpack(results)<br> end<br> <br> this_module.self = {<br>diff --git a/test/app-tap/debug.result b/test/app-tap/debug.result<br>index 72934b076..cdc990307 100644<br>--- a/test/app-tap/debug.result<br>+++ b/test/app-tap/debug.result<br>@@ -15,15 +15,15 @@ nil<br> Exec: tarantool -e "print(debug.__dir__); os.exit(0)"<br> .<br> Exec: tarantool -e "print(require('net.box').self:call('debug.sourcefile')); os.exit(0)"<br>-nil<br>+<br> Exec: tarantool -e "print(require('net.box').self:call('debug.sourcedir')); os.exit(0)"<br> .<br> Exec: tarantool -e "fn = function() return debug.__file__ end; print(require('net.box').self:call('fn')); os.exit(0)"<br>-nil<br>+<br> Exec: tarantool -e "fn = function() return debug.__dir__ end; print(require('net.box').self:call('fn')); os.exit(0)"<br> .<br> Exec: tarantool -e "fn = function() local res = debug.sourcefile(); return res end; print(require('net.box').self:call('fn')); os.exit(0)"<br>-nil<br>+<br> Exec: tarantool -e "fn = function() local res = debug.sourcedir(); return res end; print(require('net.box').self:call('fn')); os.exit(0)"<br> .<br> Exec: tarantool -e "print(loadstring('return debug.sourcefile()')()); os.exit(0)"<br>@@ -52,7 +52,7 @@ debug/test.lua<br> Source: print(debug.__dir__)<br> debug<br> Source: print(require('net.box').self:call('debug.sourcefile'))<br>-nil<br>+<br> Source: print(require('net.box').self:call('debug.sourcedir'))<br> .<br> Source: fn = function() return debug.__file__ end; print(require('net.box').self:call('fn'))<br>diff --git a/test/box-tap/net.box.test.lua b/test/box-tap/net.box.test.lua<br>index a46f28ad0..577e97d78 100755<br>--- a/test/box-tap/net.box.test.lua<br>+++ b/test/box-tap/net.box.test.lua<br>@@ -30,5 +30,4 @@ test:ok(not pcall(function() conn.space._vspace:insert() end), "error handling")<br> -- cleanup<br> conn:close()<br> inspector:cmd('stop server second with cleanup=1')<br>-test:check()<br>-os.exit(0)<br>+os.exit(test:check() and 0 or 1)<br>diff --git a/test/box/engine.cfg b/test/box/engine.cfg<br>new file mode 100644<br>index 000000000..2d1d450e3<br>--- /dev/null<br>+++ b/test/box/engine.cfg<br>@@ -0,0 +1,6 @@<br>+{<br>+ "gh-4513-netbox-self-and-connect-interchangeable.test.lua": {<br>+ "remote": {"remote": "true"},<br>+ "local": {"remote": "false"}<br>+ }<br>+}<br>diff --git a/test/box/gh-4513-netbox-self-and-connect-interchangeable.result b/test/box/gh-4513-netbox-self-and-connect-interchangeable.result<br>new file mode 100644<br>index 000000000..64bd129b3<br>--- /dev/null<br>+++ b/test/box/gh-4513-netbox-self-and-connect-interchangeable.result<br>@@ -0,0 +1,44 @@<br>+-- test-run result file version 2<br>+netbox = require('net.box')<br>+ | ---<br>+ | ...<br>+test_run = require('test_run').new()<br>+ | ---<br>+ | ...<br>+remote = test_run:get_cfg('remote') == 'true'<br>+ | ---<br>+ | ...<br>+test_run:cmd("setopt delimiter ';'")<br>+ | ---<br>+ | - true<br>+ | ...<br>+<br>+nb = nil<br>+if remote then<br>+ box.schema.user.grant('guest','super')<br>+ nb = netbox.connect(box.cfg.listen)<br>+else<br>+ nb = netbox.self<br>+end;<br>+ | ---<br>+ | ...<br>+<br>+--<br>+-- netbox:self and netbox:connect should work interchangeably<br>+--<br>+test_run:cmd("setopt delimiter ''");<br>+ | ---<br>+ | - true<br>+ | ...<br>+type(nb:eval('return box.tuple.new{1}')) -- table<br>+ | ---<br>+ | - table<br>+ | ...<br>+type(nb:eval('return box.error.new(1, "test error")')) -- string<br>+ | ---<br>+ | - string<br>+ | ...<br>+type(nb:eval('return box.NULL')) -- cdata<br>+ | ---<br>+ | - cdata<br>+ | ...<br>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<br>new file mode 100644<br>index 000000000..5e8ae7465<br>--- /dev/null<br>+++ b/test/box/gh-4513-netbox-self-and-connect-interchangeable.test.lua<br>@@ -0,0 +1,20 @@<br>+netbox = require('net.box')<br>+test_run = require('test_run').new()<br>+remote = test_run:get_cfg('remote') == 'true'<br>+test_run:cmd("setopt delimiter ';'")<br>+<br>+nb = nil<br>+if remote then<br>+ box.schema.user.grant('guest','super')<br>+ nb = netbox.connect(box.cfg.listen)<br>+else<br>+ nb = netbox.self<br>+end;<br>+<br>+--<br>+-- netbox:self and netbox:connect should work interchangeably<br>+--<br>+test_run:cmd("setopt delimiter ''");<br>+type(nb:eval('return box.tuple.new{1}')) -- table<br>+type(nb:eval('return box.error.new(1, "test error")')) -- string<br>+type(nb:eval('return box.NULL')) -- cdata<br>diff --git a/test/box/suite.ini b/test/box/suite.ini<br>index 9aa30ede6..01f07236b 100644<br>--- a/test/box/suite.ini<br>+++ b/test/box/suite.ini<br>@@ -3,6 +3,7 @@ core = tarantool<br> description = Database tests<br> script = box.lua<br> disabled = rtree_errinj.test.lua tuple_bench.test.lua<br>+config = engine.cfg<br> 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<br> lua_libs = lua/fifo.lua lua/utils.lua lua/bitset.lua lua/index_random_test.lua lua/push.lua lua/identifier.lua<br> use_unix_sockets = True<br>-- <br>2.24.0</div></div><blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Вторник, 21 января 2020, 0:22 +03:00 от Vladislav Shpilevoy <v.shpilevoy@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_15795553360310005819_BODY">Hi! Thanks for the patch!<br><br>See 3 comments below.<br><br>On 17/01/2020 18:08, Maria wrote:<br>> Despite what was stated in the documentation, netbox.connect was not always<br>> equivalent to netbox.self. In particular, they converted tuple to different<br>> types - table and cdata respectively.<br>><br>> The patch fixes the issue and covers all cases where netbox.self and connect<br>> perform conversion of types - e.g., for box.error.<br>><br>> Closes #4513<br>> ---<br>> Branch:<br>> <a href="https://github.com/tarantool/tarantool/compare/eljashm/gh-4513-netbox.self-convert-tuples-to-table-type" target="_blank">https://github.com/tarantool/tarantool/compare/eljashm/gh-4513-netbox.self-convert-tuples-to-table-type</a><br>> Issue:<br>> <a href="https://github.com/tarantool/tarantool/issues/4513" target="_blank">https://github.com/tarantool/tarantool/issues/4513</a><br>><br>> src/box/lua/net_box.lua | 10 ++++++++-<br>> test/app-tap/debug.result | 8 +++----<br>> test/box-tap/net.box.test.lua | 39 ++++++++++++++++++++++++++++++++---<br>> 3 files changed, 49 insertions(+), 8 deletions(-)<br>><br>> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua<br>> index b4811edfa..47ac72b80 100644<br>> --- a/src/box/lua/net_box.lua<br>> +++ b/src/box/lua/net_box.lua<br>> @@ -1550,7 +1550,15 @@ local function handle_eval_result(status, ...)<br>> rollback()<br>> return box.error(E_PROC_LUA, (...))<br>> end<br>> - return ...<br>> +<br>> + local results = {...}<br>> + for i = 0, select('#', results) do<br>> + if type(results[i]) == 'cdata' then<br>> + results[i] = msgpack.decode(msgpack.encode(results[i]))<br>> + end<br>> + end<br>> +<br><br>1. This line contains only whitespaces. Please, drop it.<br><br>> + return unpack(results)<br>> end<br><br>2. The code does not work, if I pass a cdata object in any<br>returned value except first.<br><br>tarantool> netbox = require('net.box')<br>---<br>...<br><br>tarantool> a, b = netbox.self:eval('return 1, box.tuple.new({2})')<br>---<br>...<br><br>tarantool> type(b)<br>---<br>- cdata<br>...<br><br>This is because<br><br>1) In Lua indexes start from 1. You start the loop from 0.<br><br>2) 'select('#', {...}) is always one. Because 'select('#', ...)'<br> returns number of the arguments starting from the second. In<br> your case you passed one argument - a table {...}.<br> You should use 'select('#', ...)' to get the element count.<br> Note, you can't use #results, because it will return an incorrect<br> value in case '...' contains a nil.<br><br>> diff --git a/test/box-tap/net.box.test.lua b/test/box-tap/net.box.test.lua<br>> index a46f28ad0..fefbe7ff7 100755<br>> --- a/test/box-tap/net.box.test.lua<br>> +++ b/test/box-tap/net.box.test.lua<br>> @@ -7,7 +7,7 @@ local net_box = require('net.box')<br>> local test_run = require('test_run')<br>> local inspector = test_run.new()<br>><br>> -test:plan(5)<br>> +test:plan(11)<br>><br>> -- create tarantool instance<br>> test:is(<br>> @@ -27,8 +27,41 @@ test:is(conn.space ~= nil, true, 'space exists')<br>> -- gh-1814: Segfault if using `net.box` before `box.cfg` start<br>> test:ok(not pcall(function() conn.space._vspace:insert() end), "error handling")<br>><br>> +--<br>> +-- gh-4513 netbox.connect and netbox.self should be interchangeable<br>> +--<br>> +box.cfg{<br>> + listen = os.getenv('LISTEN')<br>> +}<br><br>3. I don't think tap is a good engine for such a test. But up to<br>you. What is more important is that<br><br>1) If you use tap, then you should declare variables properly,<br>using 'local' prefix. Below you sometimes use local, sometimes<br>not. We need at least to be consistent. Either local is everywhere,<br>or nowhere.<br><br>2) You don't really need spaces to create and test return of a<br>tuple. There is 'box.tuple.new' function for that. JFYI, up to you<br>whether you want to use it and drop these space/index things.<br><br>3) To make the test simpler you can use the same what we did for<br>sql/bind.test.lua. It is run 2 times with different configuration<br>option 'remote' values: true and false. When the test is started<br>with remote true, we use netbox.connect(). When the test is started<br>with remote false, we use a local function. You can do the same<br>here. In the beginning you do something like this:<br><br>remote = test_run:get_cfg('remote') == 'true'<br>nb = nil<br>if remote then \<br>box.schema.user.grant('guest','super') \<br>nb = netbox.connect(box.cfg.listen) \<br>else \<br>nb = netbox.self \<br>end<br><br>Then you use and test 'nb.call/eval'. Test-run will run your test 2<br>times with remote true and remote false, so you will cover both remote<br>and local netbox instances in one test. Instead of duplicating each<br>test like you did below.<br><br>sql/bind.test.lua chooses 'remote' values in sql/engine.cfg file,<br>which is specified in sql/suite.ini. You need to do something similar.<br>And I guess in a separate file, since it is our policy, and because<br>there are no other common netbox test files which need to be run twice.<br><br>> +<br>> +space = box.schema.space.create('gh4513')<br>> +box.schema.user.grant('guest','read, write, execute','universe')<br>> +idx = box.space.gh4513:create_index('primary')<br>> +box.space.gh4513:insert({1})<br>> +<br>> +local dst = box.cfg.listen<br>> +local t1 = type(net_box.connect(dst):eval("return box.space.gh4513:get({1})"))<br>> +local t2 = type(net_box.self:eval("return box.space.gh4513:get{1}"))<br>> +<br>> +-- connect and self should convert tuples to tables<br>> +test:is(t1, "table", "connect converts to table")<br>> +test:is(t2, "table", "self convert to table")<br>> +<br>> +t1 = type(net_box.connect(dst):eval("return box.error.new(1, 'test error')"))<br>> +t2 = type(net_box.self:eval("return box.error.new(1, 'test error')"))<br>> +<br>> +-- check for converting strings<br>> +test:is(t1, "string", "connect converts to strings")<br>> +test:is(t2, "string", "self converts to strings")<br>> +<br>> +t1 = type(net_box.connect(dst):eval("return box.NULL"))<br>> +t2 = type(net_box.self:eval("return box.NULL"))<br>> +<br>> +-- check for converting cdata<br>> +test:is(t1, "cdata", "connect converts to cdata")<br>> +test:is(t2, "cdata", "self converts to cdata")<br>> +<br>> -- cleanup<br>> conn:close()<br>> inspector:cmd('stop server second with cleanup=1')<br>> -test:check()<br>> -os.exit(0)<br>> +os.exit(test:check() and 0 or 1)<br>></div></div></div></div></blockquote> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Maria Khaydich</div></div></div><div> </div></div></BODY></HTML>