From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp43.i.mail.ru (smtp43.i.mail.ru [94.100.177.103]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 0DAA546970E for ; Tue, 21 Jan 2020 00:22:16 +0300 (MSK) References: <20200117170801.34975-1-maria.khaydich@tarantool.org> From: Vladislav Shpilevoy Message-ID: <616330e3-950c-7e8a-d14b-ea81fb350611@tarantool.org> Date: Mon, 20 Jan 2020 22:22:14 +0100 MIME-Version: 1.0 In-Reply-To: <20200117170801.34975-1-maria.khaydich@tarantool.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] box: netbox.self and connect should work interchangeably List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Maria , tarantool-patches@dev.tarantool.org, imun@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) >