[PATCH v1 0/2] netbox: define formats for tuple from netbox

[PATCH v1 0/2] netbox: define formats for tuple from netbox

[imeevma]

This patch defines field names for the tuples obtained through
the netbox.

Mergen Imeev (2):
  netbox: store method_encoder args in request
  netbox: define formats for tuple from netbox

 src/box/lua/net_box.c     |  87 ++++++++++++++++-
 src/box/lua/net_box.lua   | 232 +++++++++++++++++++++++++++++++++-------------
 test/box/access.result    |  15 ++-
 test/box/access.test.lua  |   9 +-
 test/box/net.box.result   |  83 ++++++++++++++++-
 test/box/net.box.test.lua |  26 +++++-
 6 files changed, 369 insertions(+), 83 deletions(-)

-- 
2.7.4

[PATCH v1 1/2] netbox: store method_encoder args in request

[imeevma]

This patch changes the way arguments are passed to functions in
the array method_encoder, and saves these arguments in REQUEST.
This is necessary to establish a connection between netbox space
objects and the tuples obtained through the netbox

Needed for #2978
---
 src/box/lua/net_box.lua   | 179 +++++++++++++++++++++++++++++++---------------
 test/box/access.result    |  15 +++-
 test/box/access.test.lua  |   9 ++-
 test/box/net.box.result   |   6 +-
 test/box/net.box.test.lua |   6 +-
 5 files changed, 146 insertions(+), 69 deletions(-)

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 251ad40..8d42fb4 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -25,7 +25,6 @@ local check_primary_index = box.internal.check_primary_index
 
 local communicate     = internal.communicate
 local encode_auth     = internal.encode_auth
-local encode_select   = internal.encode_select
 local decode_greeting = internal.decode_greeting
 
 local TIMEOUT_INFINITY = 500 * 365 * 86400
@@ -84,22 +83,70 @@ local function decode_push(raw_data)
     return response[IPROTO_DATA_KEY][1], raw_end
 end
 
+local function encode_call(send_buf, id, method_args)
+    return internal.encode_call(send_buf, id, method_args.func_name,
+                                method_args.args)
+end
+local function encode_call_16(send_buf, id, method_args)
+    return internal.encode_call_16(send_buf, id, method_args.func_name,
+                                   method_args.args)
+end
+local function encode_ping(send_buf, id, ...)
+    return internal.encode_ping(send_buf, id, ...)
+end
+local function encode_eval(send_buf, id, method_args)
+    return internal.encode_eval(send_buf, id, method_args.code,
+                                method_args.args)
+end
+local function encode_insert(send_buf, id, method_args)
+    return internal.encode_insert(send_buf, id, method_args.space_id,
+                                  method_args.tuple)
+end
+local function encode_replace(send_buf, id, method_args)
+    return internal.encode_replace(send_buf, id, method_args.space_id,
+                                   method_args.tuple)
+end
+local function encode_delete(send_buf, id, method_args)
+    return internal.encode_delete(send_buf, id, method_args.space_id,
+                                  method_args.index_id, method_args.key)
+end
+local function encode_update(send_buf, id, method_args)
+    return internal.encode_update(send_buf, id, method_args.space_id,
+                                  method_args.index_id, method_args.key,
+                                  method_args.oplist)
+end
+local function encode_upsert(send_buf, id, method_args)
+    return internal.encode_upsert(send_buf, id, method_args.space_id,
+                                  method_args.key, method_args.oplist)
+end
+local function encode_select(send_buf, id, method_args)
+    return internal.encode_select(send_buf, id, method_args.space_id,
+                                  method_args.index_id, method_args.iterator,
+                                  method_args.offset, method_args.limit,
+                                  method_args.key)
+end
+local function encode_execute(send_buf, id, method_args)
+    return internal.encode_execute(send_buf, id, method_args.query,
+                                   method_args.parameters, method_args.sql_opts)
+end
+
+
 local method_encoder = {
-    ping    = internal.encode_ping,
-    call_16 = internal.encode_call_16,
-    call_17 = internal.encode_call,
-    eval    = internal.encode_eval,
-    insert  = internal.encode_insert,
-    replace = internal.encode_replace,
-    delete  = internal.encode_delete,
-    update  = internal.encode_update,
-    upsert  = internal.encode_upsert,
-    select  = internal.encode_select,
-    execute = internal.encode_execute,
-    get     = internal.encode_select,
-    min     = internal.encode_select,
-    max     = internal.encode_select,
-    count   = internal.encode_call,
+    ping    = encode_ping,
+    call_16 = encode_call_16,
+    call_17 = encode_call,
+    eval    = encode_eval,
+    insert  = encode_insert,
+    replace = encode_replace,
+    delete  = encode_delete,
+    update  = encode_update,
+    upsert  = encode_upsert,
+    select  = encode_select,
+    execute = encode_execute,
+    get     = encode_select,
+    min     = encode_select,
+    max     = encode_select,
+    count   = encode_call,
     -- inject raw data into connection, used by console and tests
     inject = function(buf, id, bytes)
         local ptr = buf:reserve(#bytes)
@@ -486,7 +533,7 @@ local function create_transport(host, port, user, password, callback,
     -- @retval not nil Future object.
     --
     local function perform_async_request(buffer, skip_header, method, on_push,
-                                         on_push_ctx, ...)
+                                         on_push_ctx, method_args)
         if state ~= 'active' and state ~= 'fetch_schema' then
             return nil, box.error.new({code = last_errno or E_NO_CONNECTION,
                                        reason = last_error})
@@ -497,12 +544,12 @@ local function create_transport(host, port, user, password, callback,
             worker_fiber:wakeup()
         end
         local id = next_request_id
-        method_encoder[method](send_buf, id, ...)
+        method_encoder[method](send_buf, id, method_args)
         next_request_id = next_id(id)
-        -- Request in most cases has maximum 9 members:
+        -- Request in most cases has maximum 10 members:
         -- method, buffer, skip_header, id, cond, errno, response,
-        -- on_push, on_push_ctx.
-        local request = setmetatable(table_new(0, 9), request_mt)
+        -- on_push, on_push_ctx and method_args.
+        local request = setmetatable(table_new(0, 10), request_mt)
         request.method = method
         request.buffer = buffer
         request.skip_header = skip_header
@@ -511,6 +558,7 @@ local function create_transport(host, port, user, password, callback,
         requests[id] = request
         request.on_push = on_push
         request.on_push_ctx = on_push_ctx
+        request.method_args = method_args
         return request
     end
 
@@ -520,10 +568,10 @@ local function create_transport(host, port, user, password, callback,
     -- @retval not nil Response object.
     --
     local function perform_request(timeout, buffer, skip_header, method,
-                                   on_push, on_push_ctx, ...)
+                                   on_push, on_push_ctx, method_args)
         local request, err =
             perform_async_request(buffer, skip_header, method, on_push,
-                                  on_push_ctx, ...)
+                                  on_push_ctx, method_args)
         if not request then
             return nil, err
         end
@@ -739,13 +787,14 @@ local function create_transport(host, port, user, password, callback,
         local select3_id = new_request_id()
         local response = {}
         -- fetch everything from space _vspace, 2 = ITER_ALL
-        encode_select(send_buf, select1_id, VSPACE_ID, 0, 2, 0, 0xFFFFFFFF, nil)
+        internal.encode_select(send_buf, select1_id, VSPACE_ID, 0, 2, 0,
+                               0xFFFFFFFF, nil)
         -- fetch everything from space _vindex, 2 = ITER_ALL
-        encode_select(send_buf, select2_id, VINDEX_ID, 0, 2, 0, 0xFFFFFFFF, nil)
+        internal.encode_select(send_buf, select2_id, VINDEX_ID, 0, 2, 0,
+                               0xFFFFFFFF, nil)
         -- fetch everything from space _vcollation, 2 = ITER_ALL
-        encode_select(send_buf, select3_id, VCOLLATION_ID, 0, 2, 0, 0xFFFFFFFF,
-                      nil)
-
+        internal.encode_select(send_buf, select3_id, VCOLLATION_ID, 0, 2, 0,
+                               0xFFFFFFFF, nil)
         schema_version = nil -- any schema_version will do provided that
                              -- it is consistent across responses
         repeat
@@ -1064,7 +1113,7 @@ function remote_methods:wait_connected(timeout)
     return self._transport.wait_state('active', timeout)
 end
 
-function remote_methods:_request(method, opts, ...)
+function remote_methods:_request(method, opts, method_args)
     local transport = self._transport
     local on_push, on_push_ctx, buffer, skip_header, deadline
     -- Extract options, set defaults, check if the request is
@@ -1078,7 +1127,7 @@ function remote_methods:_request(method, opts, ...)
             end
             local res, err =
                 transport.perform_async_request(buffer, skip_header, method,
-                                                table.insert, {}, ...)
+                                                table.insert, {}, method_args)
             if err then
                 box.error(err)
             end
@@ -1106,7 +1155,7 @@ function remote_methods:_request(method, opts, ...)
     end
     local res, err = transport.perform_request(timeout, buffer, skip_header,
                                                method, on_push, on_push_ctx,
-                                               ...)
+                                               method_args)
     if err then
         box.error(err)
     end
@@ -1133,14 +1182,16 @@ end
 -- @deprecated since 1.7.4
 function remote_methods:call_16(func_name, ...)
     check_remote_arg(self, 'call')
-    return (self:_request('call_16', nil, tostring(func_name), {...}))
+    local method_args = {func_name=tostring(func_name), args={...}}
+    return (self:_request('call_16', nil, method_args))
 end
 
 function remote_methods:call(func_name, args, opts)
     check_remote_arg(self, 'call')
     check_call_args(args)
     args = args or {}
-    local res = self:_request('call_17', opts, tostring(func_name), args)
+    local method_args = {func_name=tostring(func_name), args=args}
+    local res = self:_request('call_17', opts, method_args)
     if type(res) ~= 'table' or opts and opts.is_async then
         return res
     end
@@ -1150,14 +1201,16 @@ end
 -- @deprecated since 1.7.4
 function remote_methods:eval_16(code, ...)
     check_remote_arg(self, 'eval')
-    return unpack((self:_request('eval', nil, code, {...})))
+    local method_args = {code=code, args={...}}
+    return unpack((self:_request('eval', nil, method_args)))
 end
 
 function remote_methods:eval(code, args, opts)
     check_remote_arg(self, 'eval')
     check_eval_args(args)
     args = args or {}
-    local res = self:_request('eval', opts, code, args)
+    local method_args = {code=code, args=args}
+    local res = self:_request('eval', opts, method_args)
     if type(res) ~= 'table' or opts and opts.is_async then
         return res
     end
@@ -1169,8 +1222,9 @@ function remote_methods:execute(query, parameters, sql_opts, netbox_opts)
     if sql_opts ~= nil then
         box.error(box.error.UNSUPPORTED, "execute", "options")
     end
-    return self:_request('execute', netbox_opts, query, parameters or {},
-                         sql_opts or {})
+    local method_args = {query=query, parameters=parameters or {},
+                         sql_opts=sql_opts or {}}
+    return self:_request('execute', netbox_opts, method_args)
 end
 
 function remote_methods:wait_state(state, timeout)
@@ -1314,7 +1368,8 @@ function console_methods:eval(line, timeout)
     end
     if self.protocol == 'Binary' then
         local loader = 'return require("console").eval(...)'
-        res, err = pr(timeout, nil, false, 'eval', nil, nil, loader, {line})
+        local method_args = {code=loader, args={line}}
+        res, err = pr(timeout, nil, false, 'eval', nil, nil, method_args)
     else
         assert(self.protocol == 'Lua console')
         res, err = pr(timeout, nil, false, 'inject', nil, nil, line..'$EOF$\n')
@@ -1336,33 +1391,38 @@ space_metatable = function(remote)
 
     function methods:insert(tuple, opts)
         check_space_arg(self, 'insert')
-        return remote:_request('insert', opts, self.id, tuple)
+        local method_args = {space_id=self.id, tuple=tuple}
+        return remote:_request('insert', opts, method_args)
     end
 
     function methods:replace(tuple, opts)
         check_space_arg(self, 'replace')
-        return remote:_request('replace', opts, self.id, tuple)
+        local method_args = {space_id=self.id, tuple=tuple}
+        return remote:_request('replace', opts, method_args)
     end
 
     function methods:select(key, opts)
         check_space_arg(self, 'select')
+        local method_args = {key=key, opts=opts}
         return check_primary_index(self):select(key, opts)
     end
 
     function methods:delete(key, opts)
         check_space_arg(self, 'delete')
+        local method_args = {key=key, opts=opts}
         return check_primary_index(self):delete(key, opts)
     end
 
     function methods:update(key, oplist, opts)
         check_space_arg(self, 'update')
+        local method_args = {oplist=oplist, opts=opts}
         return check_primary_index(self):update(key, oplist, opts)
     end
 
     function methods:upsert(key, oplist, opts)
         check_space_arg(self, 'upsert')
-        return nothing_or_data(remote:_request('upsert', opts, self.id, key,
-                                               oplist))
+        local method_args = {space_id=self.id, key=key, oplist=oplist}
+        return nothing_or_data(remote:_request('upsert', opts, method_args))
     end
 
     function methods:get(key, opts)
@@ -1391,8 +1451,10 @@ index_metatable = function(remote)
         local iterator = check_iterator_type(opts, key_is_nil)
         local offset = tonumber(opts and opts.offset) or 0
         local limit = tonumber(opts and opts.limit) or 0xFFFFFFFF
-        return (remote:_request('select', opts, self.space.id, self.id,
-                                iterator, offset, limit, key))
+        local method_args = {space_id=self.space.id, index_id=self.id,
+                             iterator=iterator, offset=offset, limit=limit,
+                             key=key}
+        return (remote:_request('select', opts, method_args))
     end
 
     function methods:get(key, opts)
@@ -1400,8 +1462,9 @@ index_metatable = function(remote)
         if opts and opts.buffer then
             error("index:get() doesn't support `buffer` argument")
         end
-        return nothing_or_data(remote:_request('get', opts, self.space.id,
-                               self.id, box.index.EQ, 0, 2, key))
+        local method_args = {space_id=self.space.id, index_id=self.id,
+                             iterator=box.index.EQ, offset=0, limit=2, key=key}
+        return nothing_or_data(remote:_request('get', opts, method_args))
     end
 
     function methods:min(key, opts)
@@ -1409,9 +1472,9 @@ index_metatable = function(remote)
         if opts and opts.buffer then
             error("index:min() doesn't support `buffer` argument")
         end
-        return nothing_or_data(remote:_request('min', opts, self.space.id,
-                                               self.id, box.index.GE, 0, 1,
-                                               key))
+        local method_args = {space_id=self.space.id, index_id=self.id,
+                             iterator=box.index.GE, offset=0, limit=1, key=key}
+        return nothing_or_data(remote:_request('min', opts, method_args))
     end
 
     function methods:max(key, opts)
@@ -1419,9 +1482,9 @@ index_metatable = function(remote)
         if opts and opts.buffer then
             error("index:max() doesn't support `buffer` argument")
         end
-        return nothing_or_data(remote:_request('max', opts, self.space.id,
-                                               self.id, box.index.LE, 0, 1,
-                                               key))
+        local method_args = {space_id=self.space.id, index_id=self.id,
+                             iterator=box.index.LE, offset=0, limit=1, key=key}
+        return nothing_or_data(remote:_request('max', opts, method_args))
     end
 
     function methods:count(key, opts)
@@ -1431,19 +1494,21 @@ index_metatable = function(remote)
         end
         local code = string.format('box.space.%s.index.%s:count',
                                    self.space.name, self.name)
-        return remote:_request('count', opts, code, { key, opts })
+        local method_args = {func_name=code, args={ key, opts }}
+        return remote:_request('count', opts, method_args)
     end
 
     function methods:delete(key, opts)
         check_index_arg(self, 'delete')
-        return nothing_or_data(remote:_request('delete', opts, self.space.id,
-                                               self.id, key))
+        local method_args = {space_id=self.space.id, index_id=self.id, key=key}
+        return nothing_or_data(remote:_request('delete', opts, method_args))
     end
 
     function methods:update(key, oplist, opts)
         check_index_arg(self, 'update')
-        return nothing_or_data(remote:_request('update', opts, self.space.id,
-                                               self.id, key, oplist))
+        local method_args = {space_id=self.space.id, index_id=self.id, key=key,
+                             oplist=oplist}
+        return nothing_or_data(remote:_request('update', opts, method_args))
     end
 
     return { __index = methods, __metatable = false }
diff --git a/test/box/access.result b/test/box/access.result
index 0ebc6a3..e3d4036 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -890,15 +890,24 @@ LISTEN = require('uri').parse(box.cfg.listen)
 c = (require 'net.box').connect(LISTEN.host, LISTEN.service)
 ---
 ...
-c:_request("select", nil, 1, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
+args = {space_id=1, index_id=0, iterator=box.index.EQ, offset=0, limit=0xFFFFFFFF, key={}}
+---
+...
+c:_request("select", nil, args)
 ---
 - error: Space '1' does not exist
 ...
-c:_request("select", nil, 65537, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
+args.space_id = 65537
+---
+...
+c:_request("select", nil, args)
 ---
 - error: Space '65537' does not exist
 ...
-c:_request("select", nil, 4294967295, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
+args.space_id = 4294967295
+---
+...
+c:_request("select", nil, args)
 ---
 - error: Space '4294967295' does not exist
 ...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index ad63a10..c63152b 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -343,9 +343,12 @@ box.schema.func.drop(name)
 -- very large space id, no crash occurs.
 LISTEN = require('uri').parse(box.cfg.listen)
 c = (require 'net.box').connect(LISTEN.host, LISTEN.service)
-c:_request("select", nil, 1, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
-c:_request("select", nil, 65537, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
-c:_request("select", nil, 4294967295, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
+args = {space_id=1, index_id=0, iterator=box.index.EQ, offset=0, limit=0xFFFFFFFF, key={}}
+c:_request("select", nil, args)
+args.space_id = 65537
+c:_request("select", nil, args)
+args.space_id = 4294967295
+c:_request("select", nil, args)
 c:close()
 
 session = box.session
diff --git a/test/box/net.box.result b/test/box/net.box.result
index 474297a..451c31d 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -25,9 +25,9 @@ test_run:cmd("setopt delimiter ';'")
 - true
 ...
 function x_select(cn, space_id, index_id, iterator, offset, limit, key, opts)
-    local ret = cn:_request('select', opts, space_id, index_id, iterator,
-                            offset, limit, key)
-    return ret
+    local args = {space_id=space_id, index_id=index_id, iterator=iterator,
+                  offset=offset, limit=limit, key=key}
+    return cn:_request('select', opts, args)
 end
 function x_fatal(cn) cn._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') end
 test_run:cmd("setopt delimiter ''");
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index bea4300..6651b58 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -8,9 +8,9 @@ test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
 
 test_run:cmd("setopt delimiter ';'")
 function x_select(cn, space_id, index_id, iterator, offset, limit, key, opts)
-    local ret = cn:_request('select', opts, space_id, index_id, iterator,
-                            offset, limit, key)
-    return ret
+    local args = {space_id=space_id, index_id=index_id, iterator=iterator,
+                  offset=offset, limit=limit, key=key}
+    return cn:_request('select', opts, args)
 end
 function x_fatal(cn) cn._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') end
 test_run:cmd("setopt delimiter ''");
-- 
2.7.4

Re: [PATCH v1 1/2] netbox: store method_encoder args in request

[Vladimir Davydov]

On Mon, Jun 10, 2019 at 01:02:23PM +0300, imeevma@tarantool.org wrote:
> This patch changes the way arguments are passed to functions in
> the array method_encoder, and saves these arguments in REQUEST.
> This is necessary to establish a connection between netbox space
> objects and the tuples obtained through the netbox
> 
> Needed for #2978
> ---
>  src/box/lua/net_box.lua   | 179 +++++++++++++++++++++++++++++++---------------
>  test/box/access.result    |  15 +++-
>  test/box/access.test.lua  |   9 ++-
>  test/box/net.box.result   |   6 +-
>  test/box/net.box.test.lua |   6 +-
>  5 files changed, 146 insertions(+), 69 deletions(-)
> 
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 251ad40..8d42fb4 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -25,7 +25,6 @@ local check_primary_index = box.internal.check_primary_index
>  
>  local communicate     = internal.communicate
>  local encode_auth     = internal.encode_auth
> -local encode_select   = internal.encode_select
>  local decode_greeting = internal.decode_greeting
>  
>  local TIMEOUT_INFINITY = 500 * 365 * 86400
> @@ -84,22 +83,70 @@ local function decode_push(raw_data)
>      return response[IPROTO_DATA_KEY][1], raw_end
>  end
>  
> +local function encode_call(send_buf, id, method_args)
> +    return internal.encode_call(send_buf, id, method_args.func_name,
> +                                method_args.args)
> +end
> +local function encode_call_16(send_buf, id, method_args)
> +    return internal.encode_call_16(send_buf, id, method_args.func_name,
> +                                   method_args.args)
> +end
> +local function encode_ping(send_buf, id, ...)
> +    return internal.encode_ping(send_buf, id, ...)
> +end
> +local function encode_eval(send_buf, id, method_args)
> +    return internal.encode_eval(send_buf, id, method_args.code,
> +                                method_args.args)
> +end
> +local function encode_insert(send_buf, id, method_args)
> +    return internal.encode_insert(send_buf, id, method_args.space_id,
> +                                  method_args.tuple)
> +end
> +local function encode_replace(send_buf, id, method_args)
> +    return internal.encode_replace(send_buf, id, method_args.space_id,
> +                                   method_args.tuple)
> +end
> +local function encode_delete(send_buf, id, method_args)
> +    return internal.encode_delete(send_buf, id, method_args.space_id,
> +                                  method_args.index_id, method_args.key)
> +end
> +local function encode_update(send_buf, id, method_args)
> +    return internal.encode_update(send_buf, id, method_args.space_id,
> +                                  method_args.index_id, method_args.key,
> +                                  method_args.oplist)
> +end
> +local function encode_upsert(send_buf, id, method_args)
> +    return internal.encode_upsert(send_buf, id, method_args.space_id,
> +                                  method_args.key, method_args.oplist)
> +end
> +local function encode_select(send_buf, id, method_args)
> +    return internal.encode_select(send_buf, id, method_args.space_id,
> +                                  method_args.index_id, method_args.iterator,
> +                                  method_args.offset, method_args.limit,
> +                                  method_args.key)
> +end
> +local function encode_execute(send_buf, id, method_args)
> +    return internal.encode_execute(send_buf, id, method_args.query,
> +                                   method_args.parameters, method_args.sql_opts)
> +end

Why not call it simply 'args'?

> @@ -1150,14 +1201,16 @@ end
>  -- @deprecated since 1.7.4
>  function remote_methods:eval_16(code, ...)
>      check_remote_arg(self, 'eval')
> -    return unpack((self:_request('eval', nil, code, {...})))
> +    local method_args = {code=code, args={...}}
> +    return unpack((self:_request('eval', nil, method_args)))
>  end
>  
>  function remote_methods:eval(code, args, opts)
>      check_remote_arg(self, 'eval')
>      check_eval_args(args)
>      args = args or {}
> -    local res = self:_request('eval', opts, code, args)
> +    local method_args = {code=code, args=args}

Coding style: we add a space before and after '=':

	local method_args = {code = code, args = args}

Please fix here and everywhere else.

Other than that this patch looks okay to me.

Re: [PATCH v1 1/2] netbox: store method_encoder args in request

[Mergen Imeev]

On Tue, Jun 11, 2019 at 11:15:06AM +0300, Vladimir Davydov wrote:
> On Mon, Jun 10, 2019 at 01:02:23PM +0300, imeevma@tarantool.org wrote:
> > This patch changes the way arguments are passed to functions in
> > the array method_encoder, and saves these arguments in REQUEST.
> > This is necessary to establish a connection between netbox space
> > objects and the tuples obtained through the netbox
> > 
> > Needed for #2978
> > ---
> >  src/box/lua/net_box.lua   | 179 +++++++++++++++++++++++++++++++---------------
> >  test/box/access.result    |  15 +++-
> >  test/box/access.test.lua  |   9 ++-
> >  test/box/net.box.result   |   6 +-
> >  test/box/net.box.test.lua |   6 +-
> >  5 files changed, 146 insertions(+), 69 deletions(-)
> > 
> > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > index 251ad40..8d42fb4 100644
> > --- a/src/box/lua/net_box.lua
> > +++ b/src/box/lua/net_box.lua
> > @@ -25,7 +25,6 @@ local check_primary_index = box.internal.check_primary_index
> >  
> >  local communicate     = internal.communicate
> >  local encode_auth     = internal.encode_auth
> > -local encode_select   = internal.encode_select
> >  local decode_greeting = internal.decode_greeting
> >  
> >  local TIMEOUT_INFINITY = 500 * 365 * 86400
> > @@ -84,22 +83,70 @@ local function decode_push(raw_data)
> >      return response[IPROTO_DATA_KEY][1], raw_end
> >  end
> >  
> > +local function encode_call(send_buf, id, method_args)
> > +    return internal.encode_call(send_buf, id, method_args.func_name,
> > +                                method_args.args)
> > +end
> > +local function encode_call_16(send_buf, id, method_args)
> > +    return internal.encode_call_16(send_buf, id, method_args.func_name,
> > +                                   method_args.args)
> > +end
> > +local function encode_ping(send_buf, id, ...)
> > +    return internal.encode_ping(send_buf, id, ...)
> > +end
> > +local function encode_eval(send_buf, id, method_args)
> > +    return internal.encode_eval(send_buf, id, method_args.code,
> > +                                method_args.args)
> > +end
> > +local function encode_insert(send_buf, id, method_args)
> > +    return internal.encode_insert(send_buf, id, method_args.space_id,
> > +                                  method_args.tuple)
> > +end
> > +local function encode_replace(send_buf, id, method_args)
> > +    return internal.encode_replace(send_buf, id, method_args.space_id,
> > +                                   method_args.tuple)
> > +end
> > +local function encode_delete(send_buf, id, method_args)
> > +    return internal.encode_delete(send_buf, id, method_args.space_id,
> > +                                  method_args.index_id, method_args.key)
> > +end
> > +local function encode_update(send_buf, id, method_args)
> > +    return internal.encode_update(send_buf, id, method_args.space_id,
> > +                                  method_args.index_id, method_args.key,
> > +                                  method_args.oplist)
> > +end
> > +local function encode_upsert(send_buf, id, method_args)
> > +    return internal.encode_upsert(send_buf, id, method_args.space_id,
> > +                                  method_args.key, method_args.oplist)
> > +end
> > +local function encode_select(send_buf, id, method_args)
> > +    return internal.encode_select(send_buf, id, method_args.space_id,
> > +                                  method_args.index_id, method_args.iterator,
> > +                                  method_args.offset, method_args.limit,
> > +                                  method_args.key)
> > +end
> > +local function encode_execute(send_buf, id, method_args)
> > +    return internal.encode_execute(send_buf, id, method_args.query,
> > +                                   method_args.parameters, method_args.sql_opts)
> > +end
> 
> Why not call it simply 'args'?
> 
Fixed. I used 'method_args' since 'args' was used in call
and eval.

> > @@ -1150,14 +1201,16 @@ end
> >  -- @deprecated since 1.7.4
> >  function remote_methods:eval_16(code, ...)
> >      check_remote_arg(self, 'eval')
> > -    return unpack((self:_request('eval', nil, code, {...})))
> > +    local method_args = {code=code, args={...}}
> > +    return unpack((self:_request('eval', nil, method_args)))
> >  end
> >  
> >  function remote_methods:eval(code, args, opts)
> >      check_remote_arg(self, 'eval')
> >      check_eval_args(args)
> >      args = args or {}
> > -    local res = self:_request('eval', opts, code, args)
> > +    local method_args = {code=code, args=args}
> 
> Coding style: we add a space before and after '=':
> 
> 	local method_args = {code = code, args = args}
> 
> Please fix here and everywhere else.
> 
> Other than that this patch looks okay to me.

Fixed.


New patch:

From 8be1560eb5ed57d397c5f2da97112b448505080c Mon Sep 17 00:00:00 2001
Date: Wed, 5 Jun 2019 13:31:07 +0300
Subject: [PATCH] netbox: store method_encoder args in request

This patch changes the way arguments are passed to functions in
the array method_encoder, and saves these arguments in REQUEST.
This is necessary to establish a connection between netbox space
objects and the tuples obtained through the netbox

Needed for #2978

diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 251ad40..d9838f8 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -25,7 +25,6 @@ local check_primary_index = box.internal.check_primary_index
 
 local communicate     = internal.communicate
 local encode_auth     = internal.encode_auth
-local encode_select   = internal.encode_select
 local decode_greeting = internal.decode_greeting
 
 local TIMEOUT_INFINITY = 500 * 365 * 86400
@@ -84,22 +83,63 @@ local function decode_push(raw_data)
     return response[IPROTO_DATA_KEY][1], raw_end
 end
 
+local function encode_call(send_buf, id, args)
+    return internal.encode_call(send_buf, id, args.func_name, args.args)
+end
+local function encode_call_16(send_buf, id, args)
+    return internal.encode_call_16(send_buf, id, args.func_name, args.args)
+end
+local function encode_ping(send_buf, id, ...)
+    return internal.encode_ping(send_buf, id, ...)
+end
+local function encode_eval(send_buf, id, args)
+    return internal.encode_eval(send_buf, id, args.code, args.args)
+end
+local function encode_insert(send_buf, id, args)
+    return internal.encode_insert(send_buf, id, args.space_id, args.tuple)
+end
+local function encode_replace(send_buf, id, args)
+    return internal.encode_replace(send_buf, id, args.space_id, args.tuple)
+end
+local function encode_delete(send_buf, id, args)
+    return internal.encode_delete(send_buf, id, args.space_id, args.index_id,
+                                  args.key)
+end
+local function encode_update(send_buf, id, args)
+    return internal.encode_update(send_buf, id, args.space_id, args.index_id,
+                                  args.key, args.oplist)
+end
+local function encode_upsert(send_buf, id, args)
+    return internal.encode_upsert(send_buf, id, args.space_id, args.key,
+                                  args.oplist)
+end
+local function encode_select(send_buf, id, args)
+    return internal.encode_select(send_buf, id, args.space_id, args.index_id,
+                                  args.iterator, args.offset, args.limit,
+                                  args.key)
+end
+local function encode_execute(send_buf, id, args)
+    return internal.encode_execute(send_buf, id, args.query, args.parameters,
+                                   args.sql_opts)
+end
+
+
 local method_encoder = {
-    ping    = internal.encode_ping,
-    call_16 = internal.encode_call_16,
-    call_17 = internal.encode_call,
-    eval    = internal.encode_eval,
-    insert  = internal.encode_insert,
-    replace = internal.encode_replace,
-    delete  = internal.encode_delete,
-    update  = internal.encode_update,
-    upsert  = internal.encode_upsert,
-    select  = internal.encode_select,
-    execute = internal.encode_execute,
-    get     = internal.encode_select,
-    min     = internal.encode_select,
-    max     = internal.encode_select,
-    count   = internal.encode_call,
+    ping    = encode_ping,
+    call_16 = encode_call_16,
+    call_17 = encode_call,
+    eval    = encode_eval,
+    insert  = encode_insert,
+    replace = encode_replace,
+    delete  = encode_delete,
+    update  = encode_update,
+    upsert  = encode_upsert,
+    select  = encode_select,
+    execute = encode_execute,
+    get     = encode_select,
+    min     = encode_select,
+    max     = encode_select,
+    count   = encode_call,
     -- inject raw data into connection, used by console and tests
     inject = function(buf, id, bytes)
         local ptr = buf:reserve(#bytes)
@@ -486,7 +526,7 @@ local function create_transport(host, port, user, password, callback,
     -- @retval not nil Future object.
     --
     local function perform_async_request(buffer, skip_header, method, on_push,
-                                         on_push_ctx, ...)
+                                         on_push_ctx, args)
         if state ~= 'active' and state ~= 'fetch_schema' then
             return nil, box.error.new({code = last_errno or E_NO_CONNECTION,
                                        reason = last_error})
@@ -497,12 +537,12 @@ local function create_transport(host, port, user, password, callback,
             worker_fiber:wakeup()
         end
         local id = next_request_id
-        method_encoder[method](send_buf, id, ...)
+        method_encoder[method](send_buf, id, args)
         next_request_id = next_id(id)
-        -- Request in most cases has maximum 9 members:
+        -- Request in most cases has maximum 10 members:
         -- method, buffer, skip_header, id, cond, errno, response,
-        -- on_push, on_push_ctx.
-        local request = setmetatable(table_new(0, 9), request_mt)
+        -- on_push, on_push_ctx and args.
+        local request = setmetatable(table_new(0, 10), request_mt)
         request.method = method
         request.buffer = buffer
         request.skip_header = skip_header
@@ -511,6 +551,7 @@ local function create_transport(host, port, user, password, callback,
         requests[id] = request
         request.on_push = on_push
         request.on_push_ctx = on_push_ctx
+        request.args = args
         return request
     end
 
@@ -520,10 +561,10 @@ local function create_transport(host, port, user, password, callback,
     -- @retval not nil Response object.
     --
     local function perform_request(timeout, buffer, skip_header, method,
-                                   on_push, on_push_ctx, ...)
+                                   on_push, on_push_ctx, args)
         local request, err =
             perform_async_request(buffer, skip_header, method, on_push,
-                                  on_push_ctx, ...)
+                                  on_push_ctx, args)
         if not request then
             return nil, err
         end
@@ -739,13 +780,14 @@ local function create_transport(host, port, user, password, callback,
         local select3_id = new_request_id()
         local response = {}
         -- fetch everything from space _vspace, 2 = ITER_ALL
-        encode_select(send_buf, select1_id, VSPACE_ID, 0, 2, 0, 0xFFFFFFFF, nil)
+        internal.encode_select(send_buf, select1_id, VSPACE_ID, 0, 2, 0,
+                               0xFFFFFFFF, nil)
         -- fetch everything from space _vindex, 2 = ITER_ALL
-        encode_select(send_buf, select2_id, VINDEX_ID, 0, 2, 0, 0xFFFFFFFF, nil)
+        internal.encode_select(send_buf, select2_id, VINDEX_ID, 0, 2, 0,
+                               0xFFFFFFFF, nil)
         -- fetch everything from space _vcollation, 2 = ITER_ALL
-        encode_select(send_buf, select3_id, VCOLLATION_ID, 0, 2, 0, 0xFFFFFFFF,
-                      nil)
-
+        internal.encode_select(send_buf, select3_id, VCOLLATION_ID, 0, 2, 0,
+                               0xFFFFFFFF, nil)
         schema_version = nil -- any schema_version will do provided that
                              -- it is consistent across responses
         repeat
@@ -1064,7 +1106,7 @@ function remote_methods:wait_connected(timeout)
     return self._transport.wait_state('active', timeout)
 end
 
-function remote_methods:_request(method, opts, ...)
+function remote_methods:_request(method, opts, args)
     local transport = self._transport
     local on_push, on_push_ctx, buffer, skip_header, deadline
     -- Extract options, set defaults, check if the request is
@@ -1078,7 +1120,7 @@ function remote_methods:_request(method, opts, ...)
             end
             local res, err =
                 transport.perform_async_request(buffer, skip_header, method,
-                                                table.insert, {}, ...)
+                                                table.insert, {}, args)
             if err then
                 box.error(err)
             end
@@ -1106,7 +1148,7 @@ function remote_methods:_request(method, opts, ...)
     end
     local res, err = transport.perform_request(timeout, buffer, skip_header,
                                                method, on_push, on_push_ctx,
-                                               ...)
+                                               args)
     if err then
         box.error(err)
     end
@@ -1133,14 +1175,16 @@ end
 -- @deprecated since 1.7.4
 function remote_methods:call_16(func_name, ...)
     check_remote_arg(self, 'call')
-    return (self:_request('call_16', nil, tostring(func_name), {...}))
+    local args = {func_name = tostring(func_name), args = {...}}
+    return (self:_request('call_16', nil, args))
 end
 
-function remote_methods:call(func_name, args, opts)
+function remote_methods:call(func_name, call_args, opts)
     check_remote_arg(self, 'call')
-    check_call_args(args)
-    args = args or {}
-    local res = self:_request('call_17', opts, tostring(func_name), args)
+    check_call_args(call_args)
+    call_args = call_args or {}
+    local args = {func_name = tostring(func_name), args = call_args}
+    local res = self:_request('call_17', opts, args)
     if type(res) ~= 'table' or opts and opts.is_async then
         return res
     end
@@ -1150,14 +1194,16 @@ end
 -- @deprecated since 1.7.4
 function remote_methods:eval_16(code, ...)
     check_remote_arg(self, 'eval')
-    return unpack((self:_request('eval', nil, code, {...})))
+    local args = {code = code, args = {...}}
+    return unpack((self:_request('eval', nil, args)))
 end
 
-function remote_methods:eval(code, args, opts)
+function remote_methods:eval(code, eval_args, opts)
     check_remote_arg(self, 'eval')
-    check_eval_args(args)
-    args = args or {}
-    local res = self:_request('eval', opts, code, args)
+    check_eval_args(eval_args)
+    eval_args = eval_args or {}
+    local args = {code = code, args = eval_args}
+    local res = self:_request('eval', opts, args)
     if type(res) ~= 'table' or opts and opts.is_async then
         return res
     end
@@ -1169,8 +1215,9 @@ function remote_methods:execute(query, parameters, sql_opts, netbox_opts)
     if sql_opts ~= nil then
         box.error(box.error.UNSUPPORTED, "execute", "options")
     end
-    return self:_request('execute', netbox_opts, query, parameters or {},
-                         sql_opts or {})
+    local args = {query =query, parameters = parameters or {},
+                  sql_opts = sql_opts or {}}
+    return self:_request('execute', netbox_opts, args)
 end
 
 function remote_methods:wait_state(state, timeout)
@@ -1314,7 +1361,8 @@ function console_methods:eval(line, timeout)
     end
     if self.protocol == 'Binary' then
         local loader = 'return require("console").eval(...)'
-        res, err = pr(timeout, nil, false, 'eval', nil, nil, loader, {line})
+        local args = {code = loader, args = {line}}
+        res, err = pr(timeout, nil, false, 'eval', nil, nil, args)
     else
         assert(self.protocol == 'Lua console')
         res, err = pr(timeout, nil, false, 'inject', nil, nil, line..'$EOF$\n')
@@ -1336,12 +1384,14 @@ space_metatable = function(remote)
 
     function methods:insert(tuple, opts)
         check_space_arg(self, 'insert')
-        return remote:_request('insert', opts, self.id, tuple)
+        local args = {space_id = self.id, tuple = tuple}
+        return remote:_request('insert', opts, args)
     end
 
     function methods:replace(tuple, opts)
         check_space_arg(self, 'replace')
-        return remote:_request('replace', opts, self.id, tuple)
+        local args = {space_id = self.id, tuple = tuple}
+        return remote:_request('replace', opts, args)
     end
 
     function methods:select(key, opts)
@@ -1361,8 +1411,8 @@ space_metatable = function(remote)
 
     function methods:upsert(key, oplist, opts)
         check_space_arg(self, 'upsert')
-        return nothing_or_data(remote:_request('upsert', opts, self.id, key,
-                                               oplist))
+        local args = {space_id = self.id, key = key, oplist = oplist}
+        return nothing_or_data(remote:_request('upsert', opts, args))
     end
 
     function methods:get(key, opts)
@@ -1391,8 +1441,10 @@ index_metatable = function(remote)
         local iterator = check_iterator_type(opts, key_is_nil)
         local offset = tonumber(opts and opts.offset) or 0
         local limit = tonumber(opts and opts.limit) or 0xFFFFFFFF
-        return (remote:_request('select', opts, self.space.id, self.id,
-                                iterator, offset, limit, key))
+        local args = {space_id = self.space.id, index_id = self.id,
+                      iterator = iterator, offset = offset, limit = limit,
+                      key = key}
+        return (remote:_request('select', opts, args))
     end
 
     function methods:get(key, opts)
@@ -1400,8 +1452,9 @@ index_metatable = function(remote)
         if opts and opts.buffer then
             error("index:get() doesn't support `buffer` argument")
         end
-        return nothing_or_data(remote:_request('get', opts, self.space.id,
-                               self.id, box.index.EQ, 0, 2, key))
+        local args = {space_id = self.space.id, index_id = self.id,
+                      iterator = box.index.EQ, offset = 0, limit = 2, key = key}
+        return nothing_or_data(remote:_request('get', opts, args))
     end
 
     function methods:min(key, opts)
@@ -1409,9 +1462,9 @@ index_metatable = function(remote)
         if opts and opts.buffer then
             error("index:min() doesn't support `buffer` argument")
         end
-        return nothing_or_data(remote:_request('min', opts, self.space.id,
-                                               self.id, box.index.GE, 0, 1,
-                                               key))
+        local args = {space_id = self.space.id, index_id = self.id,
+                      iterator = box.index.GE, offset = 0, limit = 1, key = key}
+        return nothing_or_data(remote:_request('min', opts, args))
     end
 
     function methods:max(key, opts)
@@ -1419,9 +1472,9 @@ index_metatable = function(remote)
         if opts and opts.buffer then
             error("index:max() doesn't support `buffer` argument")
         end
-        return nothing_or_data(remote:_request('max', opts, self.space.id,
-                                               self.id, box.index.LE, 0, 1,
-                                               key))
+        local args = {space_id = self.space.id, index_id = self.id,
+                      iterator = box.index.LE, offset = 0, limit = 1, key = key}
+        return nothing_or_data(remote:_request('max', opts, args))
     end
 
     function methods:count(key, opts)
@@ -1431,19 +1484,21 @@ index_metatable = function(remote)
         end
         local code = string.format('box.space.%s.index.%s:count',
                                    self.space.name, self.name)
-        return remote:_request('count', opts, code, { key, opts })
+        local args = {func_name = code, args = { key, opts }}
+        return remote:_request('count', opts, args)
     end
 
     function methods:delete(key, opts)
         check_index_arg(self, 'delete')
-        return nothing_or_data(remote:_request('delete', opts, self.space.id,
-                                               self.id, key))
+        local args = {space_id = self.space.id, index_id = self.id, key = key}
+        return nothing_or_data(remote:_request('delete', opts, args))
     end
 
     function methods:update(key, oplist, opts)
         check_index_arg(self, 'update')
-        return nothing_or_data(remote:_request('update', opts, self.space.id,
-                                               self.id, key, oplist))
+        local args = {space_id = self.space.id, index_id = self.id, key = key,
+                      oplist = oplist}
+        return nothing_or_data(remote:_request('update', opts, args))
     end
 
     return { __index = methods, __metatable = false }
diff --git a/test/box/access.result b/test/box/access.result
index 0ebc6a3..e3d4036 100644
--- a/test/box/access.result
+++ b/test/box/access.result
@@ -890,15 +890,24 @@ LISTEN = require('uri').parse(box.cfg.listen)
 c = (require 'net.box').connect(LISTEN.host, LISTEN.service)
 ---
 ...
-c:_request("select", nil, 1, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
+args = {space_id=1, index_id=0, iterator=box.index.EQ, offset=0, limit=0xFFFFFFFF, key={}}
+---
+...
+c:_request("select", nil, args)
 ---
 - error: Space '1' does not exist
 ...
-c:_request("select", nil, 65537, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
+args.space_id = 65537
+---
+...
+c:_request("select", nil, args)
 ---
 - error: Space '65537' does not exist
 ...
-c:_request("select", nil, 4294967295, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
+args.space_id = 4294967295
+---
+...
+c:_request("select", nil, args)
 ---
 - error: Space '4294967295' does not exist
 ...
diff --git a/test/box/access.test.lua b/test/box/access.test.lua
index ad63a10..c63152b 100644
--- a/test/box/access.test.lua
+++ b/test/box/access.test.lua
@@ -343,9 +343,12 @@ box.schema.func.drop(name)
 -- very large space id, no crash occurs.
 LISTEN = require('uri').parse(box.cfg.listen)
 c = (require 'net.box').connect(LISTEN.host, LISTEN.service)
-c:_request("select", nil, 1, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
-c:_request("select", nil, 65537, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
-c:_request("select", nil, 4294967295, box.index.EQ, 0, 0, 0xFFFFFFFF, {})
+args = {space_id=1, index_id=0, iterator=box.index.EQ, offset=0, limit=0xFFFFFFFF, key={}}
+c:_request("select", nil, args)
+args.space_id = 65537
+c:_request("select", nil, args)
+args.space_id = 4294967295
+c:_request("select", nil, args)
 c:close()
 
 session = box.session
diff --git a/test/box/net.box.result b/test/box/net.box.result
index 474297a..e87a60b 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -25,9 +25,9 @@ test_run:cmd("setopt delimiter ';'")
 - true
 ...
 function x_select(cn, space_id, index_id, iterator, offset, limit, key, opts)
-    local ret = cn:_request('select', opts, space_id, index_id, iterator,
-                            offset, limit, key)
-    return ret
+    local args = {space_id = space_id, index_id = index_id, iterator = iterator,
+                  offset = offset, limit = limit, key = key}
+    return cn:_request('select', opts, args)
 end
 function x_fatal(cn) cn._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') end
 test_run:cmd("setopt delimiter ''");
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index bea4300..6e58da8 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -8,9 +8,9 @@ test_run:cmd("push filter ".."'\\.lua.*:[0-9]+: ' to '.lua...\"]:<line>: '")
 
 test_run:cmd("setopt delimiter ';'")
 function x_select(cn, space_id, index_id, iterator, offset, limit, key, opts)
-    local ret = cn:_request('select', opts, space_id, index_id, iterator,
-                            offset, limit, key)
-    return ret
+    local args = {space_id = space_id, index_id = index_id, iterator = iterator,
+                  offset = offset, limit = limit, key = key}
+    return cn:_request('select', opts, args)
 end
 function x_fatal(cn) cn._transport.perform_request(nil, nil, false, 'inject', nil, nil, '\x80') end
 test_run:cmd("setopt delimiter ''");

Re: [PATCH v1 1/2] netbox: store method_encoder args in request

[Vladimir Davydov]

On Fri, Jun 14, 2019 at 02:50:23PM +0300, Mergen Imeev wrote:
> New patch:
> 
> From 8be1560eb5ed57d397c5f2da97112b448505080c Mon Sep 17 00:00:00 2001
> Date: Wed, 5 Jun 2019 13:31:07 +0300
> Subject: [PATCH] netbox: store method_encoder args in request
> 
> This patch changes the way arguments are passed to functions in
> the array method_encoder, and saves these arguments in REQUEST.
> This is necessary to establish a connection between netbox space
> objects and the tuples obtained through the netbox
> 
> Needed for #2978

Looks good to me now, thanks.

[PATCH v1 2/2] netbox: define formats for tuple from netbox

[imeevma]

This patch creates tuple_formats for the tuples obtained through
the netbox.

Closes #2978
---
 src/box/lua/net_box.c     | 87 ++++++++++++++++++++++++++++++++++++++++++++---
 src/box/lua/net_box.lua   | 69 ++++++++++++++++++++++++++++---------
 test/box/net.box.result   | 77 +++++++++++++++++++++++++++++++++++++++++
 test/box/net.box.test.lua | 20 +++++++++++
 4 files changed, 231 insertions(+), 22 deletions(-)

diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 7484a86..fab4a8b 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -40,6 +40,7 @@
 #include "box/xrow.h"
 #include "box/tuple.h"
 #include "box/execute.h"
+#include "box/schema.h"
 
 #include "lua/msgpack.h"
 #include "third_party/base64.h"
@@ -590,12 +591,12 @@ netbox_encode_execute(lua_State *L)
  * @param data MessagePack.
  */
 static void
-netbox_decode_data(struct lua_State *L, const char **data)
+netbox_decode_data(struct lua_State *L, const char **data, uint32_t format_id)
 {
+	(void)format_id;
 	uint32_t count = mp_decode_array(data);
 	lua_createtable(L, count, 0);
-	struct tuple_format *format =
-		box_tuple_format_default();
+	struct tuple_format *format = tuple_format_by_id(format_id);
 	for (uint32_t j = 0; j < count; ++j) {
 		const char *begin = *data;
 		mp_next(data);
@@ -608,6 +609,74 @@ netbox_decode_data(struct lua_State *L, const char **data)
 	}
 }
 
+static int
+netbox_format_new(struct lua_State *L)
+{
+	if (lua_gettop(L) != 1 || lua_istable(L, 1) != 1)
+		return luaL_error(L, "Bad params!");
+
+	uint32_t count = lua_objlen(L, 1);
+	if (count == 0) {
+		lua_pushinteger(L, box_tuple_format_default()->id);
+		return 1;
+	}
+	size_t size = count * sizeof(struct field_def);
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+	struct field_def *fields = region_alloc(region, size);
+	if (fields == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc", "fields");
+		return luaT_error(L);
+	}
+	memset(fields, 0, size);
+	for (uint32_t i = 0; i < count; ++i) {
+		lua_pushinteger(L, i + 1);
+		lua_gettable(L, 1);
+
+		lua_pushstring(L, "type");
+		lua_gettable(L, -2);
+		size_t len;
+		const char *type_name = lua_tolstring(L, -1, &len);
+		lua_pop(L, 1);
+		fields[i].type = field_type_by_name(type_name, len);
+
+		lua_pushstring(L, "name");
+		lua_gettable(L, -2);
+		const char *name = lua_tolstring(L, -1, &len);
+		fields[i].name = region_alloc(region, len + 1);
+		memcpy(fields[i].name, name, len);
+		fields[i].name[len] = '\0';
+		lua_pop(L, 1);
+		lua_pop(L, 1);
+	}
+	struct tuple_dictionary *dict = tuple_dictionary_new(fields, count);
+	if (dict == NULL) {
+		region_truncate(region, region_svp);
+		return luaT_error(L);
+	}
+
+	struct tuple_format *format =
+		tuple_format_new(&box_tuple_format_default()->vtab, NULL, NULL,
+				 0, fields, count, 0, dict, false, false);
+	assert(format != NULL);
+	tuple_format_ref(format);
+	lua_pushinteger(L, format->id);
+	region_truncate(region, region_svp);
+	return 1;
+}
+
+static int
+netbox_format_delete(struct lua_State *L)
+{
+	int32_t format_id = luaL_checkinteger(L, 1);
+	if (format_id == box_tuple_format_default()->id)
+		return 0;
+	struct tuple_format *format = tuple_format_by_id(format_id);
+	assert(format != NULL);
+	tuple_format_unref(format);
+	return 0;
+}
+
 /**
  * Decode Tarantool response body consisting of single
  * IPROTO_DATA key into array of tuples.
@@ -619,6 +688,11 @@ netbox_decode_select(struct lua_State *L)
 {
 	uint32_t ctypeid;
 	const char *data = *(const char **)luaL_checkcdata(L, 1, &ctypeid);
+	uint32_t format_id;
+	if (lua_gettop(L) == 2)
+		format_id = luaL_checkinteger(L, 2);
+	else
+		format_id = box_tuple_format_default()->id;
 	assert(mp_typeof(*data) == MP_MAP);
 	uint32_t map_size = mp_decode_map(&data);
 	/* Until 2.0 body has no keys except DATA. */
@@ -627,7 +701,7 @@ netbox_decode_select(struct lua_State *L)
 	uint32_t key = mp_decode_uint(&data);
 	assert(key == IPROTO_DATA);
 	(void) key;
-	netbox_decode_data(L, &data);
+	netbox_decode_data(L, &data, format_id);
 	*(const char **)luaL_pushcdata(L, ctypeid) = data;
 	return 2;
 }
@@ -716,7 +790,8 @@ netbox_decode_execute(struct lua_State *L)
 		uint32_t key = mp_decode_uint(&data);
 		switch(key) {
 		case IPROTO_DATA:
-			netbox_decode_data(L, &data);
+			netbox_decode_data(L, &data,
+					   box_tuple_format_default()->id);
 			rows_index = i - map_size;
 			break;
 		case IPROTO_METADATA:
@@ -766,6 +841,8 @@ luaopen_net_box(struct lua_State *L)
 		{ "communicate",    netbox_communicate },
 		{ "decode_select",  netbox_decode_select },
 		{ "decode_execute", netbox_decode_execute },
+		{ "_format_new", netbox_format_new },
+		{ "_format_delete", netbox_format_delete },
 		{ NULL, NULL}
 	};
 	/* luaL_register_module polutes _G */
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index 8d42fb4..26ff7ff 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -63,12 +63,22 @@ local function decode_data(raw_data)
     local response, raw_end = decode(raw_data)
     return response[IPROTO_DATA_KEY], raw_end
 end
-local function decode_tuple(raw_data)
-    local response, raw_end = internal.decode_select(raw_data)
+local function decode_tuple(raw_data, raw_data_end, opts)
+    local response, raw_end
+    if opts ~= nil and opts.format_id ~= nil then
+        response, raw_end = internal.decode_select(raw_data, opts.format_id)
+    else
+        response, raw_end = internal.decode_select(raw_data)
+    end
     return response[1], raw_end
 end
-local function decode_get(raw_data)
-    local body, raw_end = internal.decode_select(raw_data)
+local function decode_get(raw_data, raw_data_end, opts)
+    local body, raw_end
+    if opts ~= nil and opts.format_id ~= nil then
+        body, raw_end = internal.decode_select(raw_data, opts.format_id)
+    else
+        body, raw_end = internal.decode_select(raw_data)
+    end
     if body[2] then
         return nil, raw_end, box.error.MORE_THAN_ONE_TUPLE
     end
@@ -82,6 +92,15 @@ local function decode_push(raw_data)
     local response, raw_end = decode(raw_data)
     return response[IPROTO_DATA_KEY][1], raw_end
 end
+local function decode_select(raw_data, raw_data_end, opts)
+    if opts ~= nil and opts.format_id ~= nil then
+        return internal.decode_select(raw_data, opts.format_id)
+    end
+    return internal.decode_select(raw_data)
+end
+local function decode_execute(raw_data, raw_data_end)
+    return internal.decode_execute(raw_data)
+end
 
 local function encode_call(send_buf, id, method_args)
     return internal.encode_call(send_buf, id, method_args.func_name,
@@ -157,7 +176,7 @@ local method_encoder = {
 
 local method_decoder = {
     ping    = decode_nil,
-    call_16 = internal.decode_select,
+    call_16 = decode_select,
     call_17 = decode_data,
     eval    = decode_data,
     insert  = decode_tuple,
@@ -165,8 +184,8 @@ local method_decoder = {
     delete  = decode_tuple,
     update  = decode_tuple,
     upsert  = decode_nil,
-    select  = internal.decode_select,
-    execute = internal.decode_execute,
+    select  = decode_select,
+    execute = decode_execute,
     get     = decode_get,
     min     = decode_get,
     max     = decode_get,
@@ -630,14 +649,15 @@ local function create_transport(host, port, user, password, callback,
         -- Decode xrow.body[DATA] to Lua objects
         if status == IPROTO_OK_KEY then
             request.response, real_end, request.errno =
-                method_decoder[request.method](body_rpos, body_end)
+                method_decoder[request.method](body_rpos, body_end,
+                                               request.method_args)
             assert(real_end == body_end, "invalid body length")
             requests[id] = nil
             request.id = nil
         else
             local msg
             msg, real_end, request.errno =
-                method_decoder.push(body_rpos, body_end)
+                method_decoder.push(body_rpos, body_end, request.method_args)
             assert(real_end == body_end, "invalid body length")
             request.on_push(request.on_push_ctx, msg)
         end
@@ -1085,6 +1105,14 @@ end
 
 function remote_methods:close()
     check_remote_arg(self, 'close')
+    if (self.space ~= nil and type(self.space) == 'table') then
+        for _,space in pairs(self.space) do
+            if space.format_id ~= nil then
+                internal._format_delete(space._format_id)
+                space.format_id = nil
+            end
+        end
+    end
     self._transport.stop()
 end
 
@@ -1274,6 +1302,7 @@ function remote_methods:_install_schema(schema_version, spaces, indices,
         s.index = {}
         s.temporary = false
         s._format = format
+        s._format_id = internal._format_new(format)
         s.connection = self
         if #space > 5 then
             local opts = space[6]
@@ -1391,13 +1420,15 @@ space_metatable = function(remote)
 
     function methods:insert(tuple, opts)
         check_space_arg(self, 'insert')
-        local method_args = {space_id=self.id, tuple=tuple}
+        local method_args = {space_id=self.id, tuple=tuple,
+                             format_id=self._format_id}
         return remote:_request('insert', opts, method_args)
     end
 
     function methods:replace(tuple, opts)
         check_space_arg(self, 'replace')
-        local method_args = {space_id=self.id, tuple=tuple}
+        local method_args = {space_id=self.id, tuple=tuple,
+                             format_id=self._format_id}
         return remote:_request('replace', opts, method_args)
     end
 
@@ -1453,7 +1484,7 @@ index_metatable = function(remote)
         local limit = tonumber(opts and opts.limit) or 0xFFFFFFFF
         local method_args = {space_id=self.space.id, index_id=self.id,
                              iterator=iterator, offset=offset, limit=limit,
-                             key=key}
+                             key=key, format_id=self.space._format_id}
         return (remote:_request('select', opts, method_args))
     end
 
@@ -1463,7 +1494,8 @@ index_metatable = function(remote)
             error("index:get() doesn't support `buffer` argument")
         end
         local method_args = {space_id=self.space.id, index_id=self.id,
-                             iterator=box.index.EQ, offset=0, limit=2, key=key}
+                             iterator=box.index.EQ, offset=0, limit=2, key=key,
+                             format_id=self.space._format_id}
         return nothing_or_data(remote:_request('get', opts, method_args))
     end
 
@@ -1473,7 +1505,8 @@ index_metatable = function(remote)
             error("index:min() doesn't support `buffer` argument")
         end
         local method_args = {space_id=self.space.id, index_id=self.id,
-                             iterator=box.index.GE, offset=0, limit=1, key=key}
+                             iterator=box.index.GE, offset=0, limit=1, key=key,
+                             format_id=self.space._format_id}
         return nothing_or_data(remote:_request('min', opts, method_args))
     end
 
@@ -1483,7 +1516,8 @@ index_metatable = function(remote)
             error("index:max() doesn't support `buffer` argument")
         end
         local method_args = {space_id=self.space.id, index_id=self.id,
-                             iterator=box.index.LE, offset=0, limit=1, key=key}
+                             iterator=box.index.LE, offset=0, limit=1, key=key,
+                             format_id=self.space._format_id}
         return nothing_or_data(remote:_request('max', opts, method_args))
     end
 
@@ -1500,14 +1534,15 @@ index_metatable = function(remote)
 
     function methods:delete(key, opts)
         check_index_arg(self, 'delete')
-        local method_args = {space_id=self.space.id, index_id=self.id, key=key}
+        local method_args = {space_id=self.space.id, index_id=self.id, key=key,
+                             format_id=self.space._format_id}
         return nothing_or_data(remote:_request('delete', opts, method_args))
     end
 
     function methods:update(key, oplist, opts)
         check_index_arg(self, 'update')
         local method_args = {space_id=self.space.id, index_id=self.id, key=key,
-                             oplist=oplist}
+                             oplist=oplist, format_id=self.space._format_id}
         return nothing_or_data(remote:_request('update', opts, method_args))
     end
 
diff --git a/test/box/net.box.result b/test/box/net.box.result
index 451c31d..da40a3d 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -3572,6 +3572,83 @@ box.schema.func.drop('change_schema')
 ---
 ...
 --
+-- gh-2978: field names for tuples received from netbox.
+--
+_ = box.schema.create_space("named", {format = {{name = "id"}, {name="abc"}}})
+---
+...
+_ = box.space.named:create_index('id', {parts = {{1, 'unsigned'}}})
+---
+...
+box.space.named:insert({1, 1})
+---
+- [1, 1]
+...
+box.schema.user.grant('guest', 'read, write, execute', 'universe')
+---
+...
+cn = net.connect(box.cfg.listen)
+---
+...
+s = cn.space.named
+---
+...
+s:get{1}.id
+---
+- 1
+...
+s:get{1}:tomap()
+---
+- 1: 1
+  2: 1
+  abc: 1
+  id: 1
+...
+s:insert{2,3}:tomap()
+---
+- 1: 2
+  2: 3
+  abc: 3
+  id: 2
+...
+s:replace{2,14}:tomap()
+---
+- 1: 2
+  2: 14
+  abc: 14
+  id: 2
+...
+s:update(1, {{'+', 2, 10}}):tomap()
+---
+- 1: 1
+  2: 11
+  abc: 11
+  id: 1
+...
+s:select()[1]:tomap()
+---
+- 1: 1
+  2: 11
+  abc: 11
+  id: 1
+...
+s:delete({2}):tomap()
+---
+- 1: 2
+  2: 14
+  abc: 14
+  id: 2
+...
+cn:close()
+---
+...
+box.space.named:drop()
+---
+...
+box.schema.user.revoke('guest', 'read, write, execute', 'universe')
+---
+...
+--
 -- gh-3400: long-poll input discard must not touch event loop of
 -- a closed connection.
 --
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 6651b58..bba4eb5 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1433,6 +1433,26 @@ box.space.test3:drop()
 box.schema.func.drop('change_schema')
 
 --
+-- gh-2978: field names for tuples received from netbox.
+--
+_ = box.schema.create_space("named", {format = {{name = "id"}, {name="abc"}}})
+_ = box.space.named:create_index('id', {parts = {{1, 'unsigned'}}})
+box.space.named:insert({1, 1})
+box.schema.user.grant('guest', 'read, write, execute', 'universe')
+cn = net.connect(box.cfg.listen)
+s = cn.space.named
+s:get{1}.id
+s:get{1}:tomap()
+s:insert{2,3}:tomap()
+s:replace{2,14}:tomap()
+s:update(1, {{'+', 2, 10}}):tomap()
+s:select()[1]:tomap()
+s:delete({2}):tomap()
+cn:close()
+box.space.named:drop()
+box.schema.user.revoke('guest', 'read, write, execute', 'universe')
+
+--
 -- gh-3400: long-poll input discard must not touch event loop of
 -- a closed connection.
 --
-- 
2.7.4

Re: [PATCH v1 2/2] netbox: define formats for tuple from netbox

[Vladimir Davydov]

On Mon, Jun 10, 2019 at 01:02:24PM +0300, imeevma@tarantool.org wrote:
> This patch creates tuple_formats for the tuples obtained through
> the netbox.
> 
> Closes #2978

Please write a docbot request.

> ---
>  src/box/lua/net_box.c     | 87 ++++++++++++++++++++++++++++++++++++++++++++---
>  src/box/lua/net_box.lua   | 69 ++++++++++++++++++++++++++++---------
>  test/box/net.box.result   | 77 +++++++++++++++++++++++++++++++++++++++++
>  test/box/net.box.test.lua | 20 +++++++++++
>  4 files changed, 231 insertions(+), 22 deletions(-)
> 
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 7484a86..fab4a8b 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -40,6 +40,7 @@
>  #include "box/xrow.h"
>  #include "box/tuple.h"
>  #include "box/execute.h"
> +#include "box/schema.h"
>  
>  #include "lua/msgpack.h"
>  #include "third_party/base64.h"
> @@ -590,12 +591,12 @@ netbox_encode_execute(lua_State *L)
>   * @param data MessagePack.
>   */
>  static void
> -netbox_decode_data(struct lua_State *L, const char **data)
> +netbox_decode_data(struct lua_State *L, const char **data, uint32_t format_id)

I'd pass a pointer to tuple_format struct instead of format_id to
this function.

>  {
> +	(void)format_id;

This is clearly useless.

>  	uint32_t count = mp_decode_array(data);
>  	lua_createtable(L, count, 0);
> -	struct tuple_format *format =
> -		box_tuple_format_default();
> +	struct tuple_format *format = tuple_format_by_id(format_id);
>  	for (uint32_t j = 0; j < count; ++j) {
>  		const char *begin = *data;
>  		mp_next(data);
> @@ -608,6 +609,74 @@ netbox_decode_data(struct lua_State *L, const char **data)
>  	}
>  }
>  
> +static int
> +netbox_format_new(struct lua_State *L)
> +{
> +	if (lua_gettop(L) != 1 || lua_istable(L, 1) != 1)
> +		return luaL_error(L, "Bad params!");

Please print a "Usage: ..." message, like other functions do.

> +
> +	uint32_t count = lua_objlen(L, 1);
> +	if (count == 0) {
> +		lua_pushinteger(L, box_tuple_format_default()->id);

Please use tuple_format_runtime instead of box_tuple_format_default(),
here and everywhere else. The latter is, after all, for modules.

> +		return 1;
> +	}
> +	size_t size = count * sizeof(struct field_def);
> +	struct region *region = &fiber()->gc;
> +	size_t region_svp = region_used(region);
> +	struct field_def *fields = region_alloc(region, size);
> +	if (fields == NULL) {
> +		diag_set(OutOfMemory, size, "region_alloc", "fields");
> +		return luaT_error(L);
> +	}
> +	memset(fields, 0, size);

Please init each field with field_def_default in the loop below
instead of zeroing it.

> +	for (uint32_t i = 0; i < count; ++i) {
> +		lua_pushinteger(L, i + 1);
> +		lua_gettable(L, 1);
> +
> +		lua_pushstring(L, "type");
> +		lua_gettable(L, -2);
> +		size_t len;
> +		const char *type_name = lua_tolstring(L, -1, &len);
> +		lua_pop(L, 1);
> +		fields[i].type = field_type_by_name(type_name, len);
> +
> +		lua_pushstring(L, "name");
> +		lua_gettable(L, -2);
> +		const char *name = lua_tolstring(L, -1, &len);

Strictly speaking, the name or type can be absent. The type can also be
invalid. We should handle those situations gracefully.

> +		fields[i].name = region_alloc(region, len + 1);
> +		memcpy(fields[i].name, name, len);
> +		fields[i].name[len] = '\0';
> +		lua_pop(L, 1);
> +		lua_pop(L, 1);
> +	}
> +	struct tuple_dictionary *dict = tuple_dictionary_new(fields, count);
> +	if (dict == NULL) {
> +		region_truncate(region, region_svp);
> +		return luaT_error(L);
> +	}
> +
> +	struct tuple_format *format =
> +		tuple_format_new(&box_tuple_format_default()->vtab, NULL, NULL,
> +				 0, fields, count, 0, dict, false, false);

Do we need really need to pass fields to this function? AFAIU tuple
dictionary would be enough since we only need names. What happens if
a field is nullable?

> +	assert(format != NULL);

Strictly speaking tuple_format_new() may fail with OOM.
Please handle it.

> +	tuple_format_ref(format);
> +	lua_pushinteger(L, format->id);

Returning a cdata with gc set would look robuster to me - with an id
it's pretty easy to leak an object.

> +	region_truncate(region, region_svp);
> +	return 1;
> +}
> +
> +static int
> +netbox_format_delete(struct lua_State *L)
> +{
> +	int32_t format_id = luaL_checkinteger(L, 1);
> +	if (format_id == box_tuple_format_default()->id)
> +		return 0;
> +	struct tuple_format *format = tuple_format_by_id(format_id);
> +	assert(format != NULL);
> +	tuple_format_unref(format);
> +	return 0;
> +}
> +
>  /**
>   * Decode Tarantool response body consisting of single
>   * IPROTO_DATA key into array of tuples.
> @@ -619,6 +688,11 @@ netbox_decode_select(struct lua_State *L)
>  {
>  	uint32_t ctypeid;
>  	const char *data = *(const char **)luaL_checkcdata(L, 1, &ctypeid);
> +	uint32_t format_id;
> +	if (lua_gettop(L) == 2)
> +		format_id = luaL_checkinteger(L, 2);
> +	else
> +		format_id = box_tuple_format_default()->id;
>  	assert(mp_typeof(*data) == MP_MAP);
>  	uint32_t map_size = mp_decode_map(&data);
>  	/* Until 2.0 body has no keys except DATA. */
> @@ -627,7 +701,7 @@ netbox_decode_select(struct lua_State *L)
>  	uint32_t key = mp_decode_uint(&data);
>  	assert(key == IPROTO_DATA);
>  	(void) key;
> -	netbox_decode_data(L, &data);
> +	netbox_decode_data(L, &data, format_id);
>  	*(const char **)luaL_pushcdata(L, ctypeid) = data;
>  	return 2;
>  }
> @@ -716,7 +790,8 @@ netbox_decode_execute(struct lua_State *L)
>  		uint32_t key = mp_decode_uint(&data);
>  		switch(key) {
>  		case IPROTO_DATA:
> -			netbox_decode_data(L, &data);
> +			netbox_decode_data(L, &data,
> +					   box_tuple_format_default()->id);
>  			rows_index = i - map_size;
>  			break;
>  		case IPROTO_METADATA:
> @@ -766,6 +841,8 @@ luaopen_net_box(struct lua_State *L)
>  		{ "communicate",    netbox_communicate },
>  		{ "decode_select",  netbox_decode_select },
>  		{ "decode_execute", netbox_decode_execute },
> +		{ "_format_new", netbox_format_new },
> +		{ "_format_delete", netbox_format_delete },

I think we better place these helpers in src/box/lua/misc.cc - we might
need them somewhere else. Also, this is 'internal' namespace so no need
to prefix function names with an underscore.

Actually, I think we should add these functions in a separate patch and
cover them with some basic tests involving argument checking (like
passing an invalid field type or a field without a name).

>  		{ NULL, NULL}
>  	};
>  	/* luaL_register_module polutes _G */
> diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> index 8d42fb4..26ff7ff 100644
> --- a/src/box/lua/net_box.lua
> +++ b/src/box/lua/net_box.lua
> @@ -63,12 +63,22 @@ local function decode_data(raw_data)
>      local response, raw_end = decode(raw_data)
>      return response[IPROTO_DATA_KEY], raw_end
>  end
> -local function decode_tuple(raw_data)
> -    local response, raw_end = internal.decode_select(raw_data)
> +local function decode_tuple(raw_data, raw_data_end, opts)

You use name 'args' when passing 'opts' to the encoder. Let's use name
'args' everywhere to avoid confusion.

> +    local response, raw_end
> +    if opts ~= nil and opts.format_id ~= nil then
> +        response, raw_end = internal.decode_select(raw_data, opts.format_id)
> +    else
> +        response, raw_end = internal.decode_select(raw_data)
> +    end
>      return response[1], raw_end
>  end
> -local function decode_get(raw_data)
> -    local body, raw_end = internal.decode_select(raw_data)
> +local function decode_get(raw_data, raw_data_end, opts)
> +    local body, raw_end
> +    if opts ~= nil and opts.format_id ~= nil then
> +        body, raw_end = internal.decode_select(raw_data, opts.format_id)
> +    else
> +        body, raw_end = internal.decode_select(raw_data)
> +    end

Can the 'else' branch be executed at all? Shouldn't 'format' always be
available for decode_tuple and decode_get?

>      if body[2] then
>          return nil, raw_end, box.error.MORE_THAN_ONE_TUPLE
>      end
> @@ -82,6 +92,15 @@ local function decode_push(raw_data)
>      local response, raw_end = decode(raw_data)
>      return response[IPROTO_DATA_KEY][1], raw_end
>  end
> +local function decode_select(raw_data, raw_data_end, opts)
> +    if opts ~= nil and opts.format_id ~= nil then
> +        return internal.decode_select(raw_data, opts.format_id)
> +    end
> +    return internal.decode_select(raw_data)
> +end
> +local function decode_execute(raw_data, raw_data_end)
> +    return internal.decode_execute(raw_data)
> +end
>  
>  local function encode_call(send_buf, id, method_args)
>      return internal.encode_call(send_buf, id, method_args.func_name,
> @@ -157,7 +176,7 @@ local method_encoder = {
>  
>  local method_decoder = {
>      ping    = decode_nil,
> -    call_16 = internal.decode_select,
> +    call_16 = decode_select,

I'd rather add decode_call_16 to avoid branching in decode_select
(branching in a virtual method looks ugly IMO).

>      call_17 = decode_data,
>      eval    = decode_data,
>      insert  = decode_tuple,
> @@ -165,8 +184,8 @@ local method_decoder = {
>      delete  = decode_tuple,
>      update  = decode_tuple,
>      upsert  = decode_nil,
> -    select  = internal.decode_select,
> -    execute = internal.decode_execute,
> +    select  = decode_select,
> +    execute = decode_execute,
>      get     = decode_get,
>      min     = decode_get,
>      max     = decode_get,
> @@ -630,14 +649,15 @@ local function create_transport(host, port, user, password, callback,
>          -- Decode xrow.body[DATA] to Lua objects
>          if status == IPROTO_OK_KEY then
>              request.response, real_end, request.errno =
> -                method_decoder[request.method](body_rpos, body_end)
> +                method_decoder[request.method](body_rpos, body_end,
> +                                               request.method_args)
>              assert(real_end == body_end, "invalid body length")
>              requests[id] = nil
>              request.id = nil
>          else
>              local msg
>              msg, real_end, request.errno =
> -                method_decoder.push(body_rpos, body_end)
> +                method_decoder.push(body_rpos, body_end, request.method_args)

Looks unnecessary.

>              assert(real_end == body_end, "invalid body length")
>              request.on_push(request.on_push_ctx, msg)
>          end
> @@ -1085,6 +1105,14 @@ end
>  
>  function remote_methods:close()
>      check_remote_arg(self, 'close')
> +    if (self.space ~= nil and type(self.space) == 'table') then
> +        for _,space in pairs(self.space) do
> +            if space.format_id ~= nil then
> +                internal._format_delete(space._format_id)
> +                space.format_id = nil
> +            end
> +        end
> +    end
>      self._transport.stop()
>  end
>  
> @@ -1274,6 +1302,7 @@ function remote_methods:_install_schema(schema_version, spaces, indices,
>          s.index = {}
>          s.temporary = false
>          s._format = format
> +        s._format_id = internal._format_new(format)

Shouldn't you unref the old format before setting the new one?

>          s.connection = self
>          if #space > 5 then
>              local opts = space[6]
> @@ -1391,13 +1420,15 @@ space_metatable = function(remote)
>  
>      function methods:insert(tuple, opts)
>          check_space_arg(self, 'insert')
> -        local method_args = {space_id=self.id, tuple=tuple}
> +        local method_args = {space_id=self.id, tuple=tuple,
> +                             format_id=self._format_id}
>          return remote:_request('insert', opts, method_args)
>      end
>  
>      function methods:replace(tuple, opts)
>          check_space_arg(self, 'replace')
> -        local method_args = {space_id=self.id, tuple=tuple}
> +        local method_args = {space_id=self.id, tuple=tuple,
> +                             format_id=self._format_id}
>          return remote:_request('replace', opts, method_args)
>      end
>  
> @@ -1453,7 +1484,7 @@ index_metatable = function(remote)
>          local limit = tonumber(opts and opts.limit) or 0xFFFFFFFF
>          local method_args = {space_id=self.space.id, index_id=self.id,
>                               iterator=iterator, offset=offset, limit=limit,
> -                             key=key}
> +                             key=key, format_id=self.space._format_id}
>          return (remote:_request('select', opts, method_args))
>      end
>  
> @@ -1463,7 +1494,8 @@ index_metatable = function(remote)
>              error("index:get() doesn't support `buffer` argument")
>          end
>          local method_args = {space_id=self.space.id, index_id=self.id,
> -                             iterator=box.index.EQ, offset=0, limit=2, key=key}
> +                             iterator=box.index.EQ, offset=0, limit=2, key=key,
> +                             format_id=self.space._format_id}
>          return nothing_or_data(remote:_request('get', opts, method_args))
>      end
>  
> @@ -1473,7 +1505,8 @@ index_metatable = function(remote)
>              error("index:min() doesn't support `buffer` argument")
>          end
>          local method_args = {space_id=self.space.id, index_id=self.id,
> -                             iterator=box.index.GE, offset=0, limit=1, key=key}
> +                             iterator=box.index.GE, offset=0, limit=1, key=key,
> +                             format_id=self.space._format_id}
>          return nothing_or_data(remote:_request('min', opts, method_args))
>      end
>  
> @@ -1483,7 +1516,8 @@ index_metatable = function(remote)
>              error("index:max() doesn't support `buffer` argument")
>          end
>          local method_args = {space_id=self.space.id, index_id=self.id,
> -                             iterator=box.index.LE, offset=0, limit=1, key=key}
> +                             iterator=box.index.LE, offset=0, limit=1, key=key,
> +                             format_id=self.space._format_id}
>          return nothing_or_data(remote:_request('max', opts, method_args))
>      end
>  
> @@ -1500,14 +1534,15 @@ index_metatable = function(remote)
>  
>      function methods:delete(key, opts)
>          check_index_arg(self, 'delete')
> -        local method_args = {space_id=self.space.id, index_id=self.id, key=key}
> +        local method_args = {space_id=self.space.id, index_id=self.id, key=key,
> +                             format_id=self.space._format_id}
>          return nothing_or_data(remote:_request('delete', opts, method_args))
>      end
>  
>      function methods:update(key, oplist, opts)
>          check_index_arg(self, 'update')
>          local method_args = {space_id=self.space.id, index_id=self.id, key=key,
> -                             oplist=oplist}
> +                             oplist=oplist, format_id=self.space._format_id}
>          return nothing_or_data(remote:_request('update', opts, method_args))
>      end
>  
> diff --git a/test/box/net.box.result b/test/box/net.box.result
> index 451c31d..da40a3d 100644
> --- a/test/box/net.box.result
> +++ b/test/box/net.box.result
> @@ -3572,6 +3572,83 @@ box.schema.func.drop('change_schema')
>  ---
>  ...
>  --
> +-- gh-2978: field names for tuples received from netbox.
> +--
> +_ = box.schema.create_space("named", {format = {{name = "id"}, {name="abc"}}})
> +---
> +...
> +_ = box.space.named:create_index('id', {parts = {{1, 'unsigned'}}})
> +---
> +...
> +box.space.named:insert({1, 1})
> +---
> +- [1, 1]
> +...
> +box.schema.user.grant('guest', 'read, write, execute', 'universe')

Please don't use 'universe' in tests - we're trying to deprecate it
AFAIR. Grant specific permissions instead.

It would be nice to check that schema reload does update the format,
i.e. update the space format, call remote methods again and check
that they do use new names.

> +---
> +...
> +cn = net.connect(box.cfg.listen)
> +---
> +...
> +s = cn.space.named
> +---
> +...
> +s:get{1}.id
> +---
> +- 1
> +...
> +s:get{1}:tomap()
> +---
> +- 1: 1
> +  2: 1
> +  abc: 1
> +  id: 1
> +...
> +s:insert{2,3}:tomap()
> +---
> +- 1: 2
> +  2: 3
> +  abc: 3
> +  id: 2
> +...
> +s:replace{2,14}:tomap()
> +---
> +- 1: 2
> +  2: 14
> +  abc: 14
> +  id: 2
> +...
> +s:update(1, {{'+', 2, 10}}):tomap()
> +---
> +- 1: 1
> +  2: 11
> +  abc: 11
> +  id: 1
> +...
> +s:select()[1]:tomap()
> +---
> +- 1: 1
> +  2: 11
> +  abc: 11
> +  id: 1
> +...
> +s:delete({2}):tomap()
> +---
> +- 1: 2
> +  2: 14
> +  abc: 14
> +  id: 2
> +...
> +cn:close()
> +---
> +...
> +box.space.named:drop()
> +---
> +...
> +box.schema.user.revoke('guest', 'read, write, execute', 'universe')
> +---
> +...
> +--

Re: [PATCH v1 2/2] netbox: define formats for tuple from netbox

[Mergen Imeev]

Thank you for review! There will be two new patches below.


On Tue, Jun 11, 2019 at 11:55:11AM +0300, Vladimir Davydov wrote:
> On Mon, Jun 10, 2019 at 01:02:24PM +0300, imeevma@tarantool.org wrote:
> > This patch creates tuple_formats for the tuples obtained through
> > the netbox.
> > 
> > Closes #2978
> 
> Please write a docbot request.
> 
Done:

@TarantoolBot document
Title: Field names for tuples received from net.box

It is possible now to access by field name for tuples received
from net.box. For example:

box.cfg{listen = 3302}
box.schema.user.grant('guest','read, write, execute', 'space')
box.schema.user.grant('guest', 'create', 'space')

box.schema.create_space("named", {format = {{name = "id"}}})
box.space.named:create_index('id', {parts = {{1, 'unsigned'}}})
box.space.named:insert({1})
require('net.box').connect('localhost', 3302).space.named:get(1).id

Result:

tarantool> require('net.box').connect('localhost', 3302).space.named:get(1).id
---
- 1
...

> >  static void
> > -netbox_decode_data(struct lua_State *L, const char **data)
> > +netbox_decode_data(struct lua_State *L, const char **data, uint32_t format_id)
> 
> I'd pass a pointer to tuple_format struct instead of format_id to
> this function.
> 
Done.

> >  {
> > +	(void)format_id;
> 
> This is clearly useless.
> 
Fixed.

> > +	if (lua_gettop(L) != 1 || lua_istable(L, 1) != 1)
> > +		return luaL_error(L, "Bad params!");
> 
> Please print a "Usage: ..." message, like other functions do.
> 
Fixed.

> > +
> > +	uint32_t count = lua_objlen(L, 1);
> > +	if (count == 0) {
> > +		lua_pushinteger(L, box_tuple_format_default()->id);
> 
> Please use tuple_format_runtime instead of box_tuple_format_default(),
> here and everywhere else. The latter is, after all, for modules.
> 
Fixed.

> > +	struct field_def *fields = region_alloc(region, size);
> > +	if (fields == NULL) {
> > +		diag_set(OutOfMemory, size, "region_alloc", "fields");
> > +		return luaT_error(L);
> > +	}
> > +	memset(fields, 0, size);
> 
> Please init each field with field_def_default in the loop below
> instead of zeroing it.
> 
Fixed.

> > +
> > +		lua_pushstring(L, "name");
> > +		lua_gettable(L, -2);
> > +		const char *name = lua_tolstring(L, -1, &len);
> 
> Strictly speaking, the name or type can be absent. The type can also be
> invalid. We should handle those situations gracefully.
> 
Fixed.

> > +		fields[i].name = region_alloc(region, len + 1);
> > +		memcpy(fields[i].name, name, len);
> > +		fields[i].name[len] = '\0';
> > +		lua_pop(L, 1);
> > +		lua_pop(L, 1);
> > +	}
> > +	struct tuple_dictionary *dict = tuple_dictionary_new(fields, count);
> > +	if (dict == NULL) {
> > +		region_truncate(region, region_svp);
> > +		return luaT_error(L);
> > +	}
> > +
> > +	struct tuple_format *format =
> > +		tuple_format_new(&box_tuple_format_default()->vtab, NULL, NULL,
> > +				 0, fields, count, 0, dict, false, false);
> 
> Do we need really need to pass fields to this function? AFAIU tuple
> dictionary would be enough since we only need names.
You are right.

> What happens if a field is nullable?
> 
It works fine, I think:

box.cfg{listen = 3302}
box.schema.user.grant('guest','read, write, execute', 'space')
box.schema.user.grant('guest', 'create', 'space')

box.schema.create_space("named", {format = {{name = "id"}, {name = "nf", is_nullable = true}}})
box.space.named:create_index('id', {parts = {{1, 'unsigned'}}})
box.space.named:insert({1})
box.space.named:insert({2,3})
box.space.named:get(1):tomap()
box.space.named:get(2):tomap()
require('net.box').connect('localhost', 3302).space.named:get(1):tomap()
require('net.box').connect('localhost', 3302).space.named:get(2):tomap()

tarantool> box.space.named:get(1):tomap()
---
- 1: 1
  id: 1
...

tarantool> box.space.named:get(2):tomap()
---
- 1: 2
  2: 3
  nf: 3
  id: 2
...

tarantool> require('net.box').connect('localhost', 3302).space.named:get(1):tomap()
---
- 1: 1
  id: 1
...

tarantool> require('net.box').connect('localhost', 3302).space.named:get(2):tomap()
---
- 1: 2
  2: 3
  nf: 3
  id: 2
...

> > +	assert(format != NULL);
> 
> Strictly speaking tuple_format_new() may fail with OOM.
> Please handle it.
>
Fixed.
 
> > +	tuple_format_ref(format);
> > +	lua_pushinteger(L, format->id);
> 
> Returning a cdata with gc set would look robuster to me - with an id
> it's pretty easy to leak an object.
> 
Fixed.

> > +	region_truncate(region, region_svp);
> > +	return 1;
> > +}
> > +
> > +static int
> > +netbox_format_delete(struct lua_State *L)
> > +{
> > +	int32_t format_id = luaL_checkinteger(L, 1);
> > +	if (format_id == box_tuple_format_default()->id)
> > +		return 0;
> > +	struct tuple_format *format = tuple_format_by_id(format_id);
> > +	assert(format != NULL);
> > +	tuple_format_unref(format);
> > +	return 0;
> > +}
> > +
> >  /**
> >   * Decode Tarantool response body consisting of single
> >   * IPROTO_DATA key into array of tuples.
> > @@ -619,6 +688,11 @@ netbox_decode_select(struct lua_State *L)
> >  {
> >  	uint32_t ctypeid;
> >  	const char *data = *(const char **)luaL_checkcdata(L, 1, &ctypeid);
> > +	uint32_t format_id;
> > +	if (lua_gettop(L) == 2)
> > +		format_id = luaL_checkinteger(L, 2);
> > +	else
> > +		format_id = box_tuple_format_default()->id;
> >  	assert(mp_typeof(*data) == MP_MAP);
> >  	uint32_t map_size = mp_decode_map(&data);
> >  	/* Until 2.0 body has no keys except DATA. */
> > @@ -627,7 +701,7 @@ netbox_decode_select(struct lua_State *L)
> >  	uint32_t key = mp_decode_uint(&data);
> >  	assert(key == IPROTO_DATA);
> >  	(void) key;
> > -	netbox_decode_data(L, &data);
> > +	netbox_decode_data(L, &data, format_id);
> >  	*(const char **)luaL_pushcdata(L, ctypeid) = data;
> >  	return 2;
> >  }
> > @@ -716,7 +790,8 @@ netbox_decode_execute(struct lua_State *L)
> >  		uint32_t key = mp_decode_uint(&data);
> >  		switch(key) {
> >  		case IPROTO_DATA:
> > -			netbox_decode_data(L, &data);
> > +			netbox_decode_data(L, &data,
> > +					   box_tuple_format_default()->id);
> >  			rows_index = i - map_size;
> >  			break;
> >  		case IPROTO_METADATA:
> > @@ -766,6 +841,8 @@ luaopen_net_box(struct lua_State *L)
> >  		{ "communicate",    netbox_communicate },
> >  		{ "decode_select",  netbox_decode_select },
> >  		{ "decode_execute", netbox_decode_execute },
> > +		{ "_format_new", netbox_format_new },
> > +		{ "_format_delete", netbox_format_delete },
> 
> I think we better place these helpers in src/box/lua/misc.cc - we might
> need them somewhere else. Also, this is 'internal' namespace so no need
> to prefix function names with an underscore.
> 
> Actually, I think we should add these functions in a separate patch and
> cover them with some basic tests involving argument checking (like
> passing an invalid field type or a field without a name).
> 
Done. Patch below.

> >  		{ NULL, NULL}
> >  	};
> >  	/* luaL_register_module polutes _G */
> > diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
> > index 8d42fb4..26ff7ff 100644
> > --- a/src/box/lua/net_box.lua
> > +++ b/src/box/lua/net_box.lua
> > @@ -63,12 +63,22 @@ local function decode_data(raw_data)
> >      local response, raw_end = decode(raw_data)
> >      return response[IPROTO_DATA_KEY], raw_end
> >  end
> > -local function decode_tuple(raw_data)
> > -    local response, raw_end = internal.decode_select(raw_data)
> > +local function decode_tuple(raw_data, raw_data_end, opts)
> 
> You use name 'args' when passing 'opts' to the encoder. Let's use name
> 'args' everywhere to avoid confusion.
> 
Fixed.

> > +    local response, raw_end
> > +    if opts ~= nil and opts.format_id ~= nil then
> > +        response, raw_end = internal.decode_select(raw_data, opts.format_id)
> > +    else
> > +        response, raw_end = internal.decode_select(raw_data)
> > +    end
> >      return response[1], raw_end
> >  end
> > -local function decode_get(raw_data)
> > -    local body, raw_end = internal.decode_select(raw_data)
> > +local function decode_get(raw_data, raw_data_end, opts)
> > +    local body, raw_end
> > +    if opts ~= nil and opts.format_id ~= nil then
> > +        body, raw_end = internal.decode_select(raw_data, opts.format_id)
> > +    else
> > +        body, raw_end = internal.decode_select(raw_data)
> > +    end
> 
> Can the 'else' branch be executed at all? Shouldn't 'format' always be
> available for decode_tuple and decode_get?
> 
You are right, fixed.

> >      if body[2] then
> >          return nil, raw_end, box.error.MORE_THAN_ONE_TUPLE
> >      end
> > @@ -82,6 +92,15 @@ local function decode_push(raw_data)
> >      local response, raw_end = decode(raw_data)
> >      return response[IPROTO_DATA_KEY][1], raw_end
> >  end
> > +local function decode_select(raw_data, raw_data_end, opts)
> > +    if opts ~= nil and opts.format_id ~= nil then
> > +        return internal.decode_select(raw_data, opts.format_id)
> > +    end
> > +    return internal.decode_select(raw_data)
> > +end
> > +local function decode_execute(raw_data, raw_data_end)
> > +    return internal.decode_execute(raw_data)
> > +end
> >  
> >  local function encode_call(send_buf, id, method_args)
> >      return internal.encode_call(send_buf, id, method_args.func_name,
> > @@ -157,7 +176,7 @@ local method_encoder = {
> >  
> >  local method_decoder = {
> >      ping    = decode_nil,
> > -    call_16 = internal.decode_select,
> > +    call_16 = decode_select,
> 
> I'd rather add decode_call_16 to avoid branching in decode_select
> (branching in a virtual method looks ugly IMO).
> 
Fixed.

> >      call_17 = decode_data,
> >      eval    = decode_data,
> >      insert  = decode_tuple,
> > @@ -165,8 +184,8 @@ local method_decoder = {
> >      delete  = decode_tuple,
> >      update  = decode_tuple,
> >      upsert  = decode_nil,
> > -    select  = internal.decode_select,
> > -    execute = internal.decode_execute,
> > +    select  = decode_select,
> > +    execute = decode_execute,
> >      get     = decode_get,
> >      min     = decode_get,
> >      max     = decode_get,
> > @@ -630,14 +649,15 @@ local function create_transport(host, port, user, password, callback,
> >          -- Decode xrow.body[DATA] to Lua objects
> >          if status == IPROTO_OK_KEY then
> >              request.response, real_end, request.errno =
> > -                method_decoder[request.method](body_rpos, body_end)
> > +                method_decoder[request.method](body_rpos, body_end,
> > +                                               request.method_args)
> >              assert(real_end == body_end, "invalid body length")
> >              requests[id] = nil
> >              request.id = nil
> >          else
> >              local msg
> >              msg, real_end, request.errno =
> > -                method_decoder.push(body_rpos, body_end)
> > +                method_decoder.push(body_rpos, body_end, request.method_args)
> 
> Looks unnecessary.
> 
Fixed.

> >              assert(real_end == body_end, "invalid body length")
> >              request.on_push(request.on_push_ctx, msg)
> >          end
> > @@ -1085,6 +1105,14 @@ end
> >  
> >  function remote_methods:close()
> >      check_remote_arg(self, 'close')
> > +    if (self.space ~= nil and type(self.space) == 'table') then
> > +        for _,space in pairs(self.space) do
> > +            if space.format_id ~= nil then
> > +                internal._format_delete(space._format_id)
> > +                space.format_id = nil
> > +            end
> > +        end
> > +    end
> >      self._transport.stop()
> >  end
> >  
> > @@ -1274,6 +1302,7 @@ function remote_methods:_install_schema(schema_version, spaces, indices,
> >          s.index = {}
> >          s.temporary = false
> >          s._format = format
> > +        s._format_id = internal._format_new(format)
> 
> Shouldn't you unref the old format before setting the new one?
> 
Fixed since __gc is set now.

> > +-- gh-2978: field names for tuples received from netbox.
> > +--
> > +_ = box.schema.create_space("named", {format = {{name = "id"}, {name="abc"}}})
> > +---
> > +...
> > +_ = box.space.named:create_index('id', {parts = {{1, 'unsigned'}}})
> > +---
> > +...
> > +box.space.named:insert({1, 1})
> > +---
> > +- [1, 1]
> > +...
> > +box.schema.user.grant('guest', 'read, write, execute', 'universe')
> 
> Please don't use 'universe' in tests - we're trying to deprecate it
> AFAIR. Grant specific permissions instead.
> 
Fixed.

> It would be nice to check that schema reload does update the format,
> i.e. update the space format, call remote methods again and check
> that they do use new names.
> 
Done.


First new patch:

From 63075deb8411643d4af0cf27be2c024b5a20222c Mon Sep 17 00:00:00 2001
Date: Tue, 11 Jun 2019 15:21:39 +0300
Subject: [PATCH] lua: internal function to parse space format

This patch defines a new function that parses space format and
returns it to Lua as cdata. For this cdata destructor is included.

Needed for #2978

diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
index 8de7401..e62e2ba 100644
--- a/src/box/lua/misc.cc
+++ b/src/box/lua/misc.cc
@@ -37,9 +37,13 @@
 
 #include "box/box.h"
 #include "box/port.h"
+#include "box/tuple.h"
+#include "box/tuple_format.h"
 #include "box/lua/tuple.h"
 #include "mpstream.h"
 
+uint32_t CTID_STRUCT_TUPLE_FORMAT_PTR;
+
 /** {{{ Miscellaneous utils **/
 
 char *
@@ -116,14 +120,117 @@ lbox_select(lua_State *L)
 
 /* }}} */
 
+static int
+lbox_tuple_format_gc(struct lua_State *L)
+{
+	uint32_t ctypeid;
+	struct tuple_format *format =
+		*(struct tuple_format **)luaL_checkcdata(L, 1, &ctypeid);
+	if (ctypeid != CTID_STRUCT_TUPLE_FORMAT_PTR) {
+		luaL_error(L, "Invalid argument: 'struct tuple_format *' "\
+			   "expected, got %s)",
+			   lua_typename(L, lua_type(L, 1)));
+	}
+	box_tuple_format_unref(format);
+	return 0;
+}
+
+static int
+lbox_push_tuple_format(struct lua_State *L, struct tuple_format *format)
+{
+	struct tuple_format **ptr = (struct tuple_format **)
+		luaL_pushcdata(L, CTID_STRUCT_TUPLE_FORMAT_PTR);
+	*ptr = format;
+	box_tuple_format_ref(format);
+	lua_pushcfunction(L, lbox_tuple_format_gc);
+	luaL_setcdatagc(L, -2);
+	return 1;
+}
+
+static int
+lbox_format_new(struct lua_State *L)
+{
+	assert(CTID_STRUCT_TUPLE_FORMAT_PTR != 0);
+	if (lua_gettop(L) != 1 || ! lua_istable(L, 1))
+		return luaL_error(L, "Usage: box.internal.format_new(format)");
+	uint32_t count = lua_objlen(L, 1);
+	if (count == 0)
+		return lbox_push_tuple_format(L, tuple_format_runtime);
+	size_t size = count * sizeof(struct field_def);
+	struct region *region = &fiber()->gc;
+	size_t region_svp = region_used(region);
+	struct field_def *fields =
+		(struct field_def *)region_alloc(region, size);
+	if (fields == NULL) {
+		diag_set(OutOfMemory, size, "region_alloc", "fields");
+		return luaT_error(L);
+	}
+	for (uint32_t i = 0; i < count; ++i) {
+		size_t len;
+
+		fields[i] = field_def_default;
+
+		lua_pushinteger(L, i + 1);
+		lua_gettable(L, 1);
+
+		lua_pushstring(L, "type");
+		lua_gettable(L, -2);
+		if (! lua_isnil(L, -1)) {
+			const char *type_name = lua_tolstring(L, -1, &len);
+			fields[i].type = field_type_by_name(type_name, len);
+			if (fields[i].type == field_type_MAX) {
+				const char *err =
+					"field %d has unknown field type";
+				return luaL_error(L, tt_sprintf(err, i + 1));
+			}
+		}
+		lua_pop(L, 1);
+
+		lua_pushstring(L, "name");
+		lua_gettable(L, -2);
+		if (lua_isnil(L, -1)) {
+			return luaL_error(L, tt_sprintf("field %d name is not "\
+							"specified", i + 1));
+		}
+		const char *name = lua_tolstring(L, -1, &len);
+		fields[i].name = (char *)region_alloc(region, len + 1);
+		if (fields == NULL) {
+			diag_set(OutOfMemory, size, "region_alloc",
+				 "fields[i].name");
+			return luaT_error(L);
+		}
+		memcpy(fields[i].name, name, len);
+		fields[i].name[len] = '\0';
+		lua_pop(L, 1);
+		lua_pop(L, 1);
+	}
+	struct tuple_dictionary *dict = tuple_dictionary_new(fields, count);
+	region_truncate(region, region_svp);
+	if (dict == NULL)
+		return luaT_error(L);
+	struct tuple_format *format =
+		tuple_format_new(&tuple_format_runtime->vtab, NULL, NULL, 0,
+				 NULL, 0, 0, dict, false, false);
+	if (format == NULL)
+		return luaT_error(L);
+	return lbox_push_tuple_format(L, format);
+}
+
 void
 box_lua_misc_init(struct lua_State *L)
 {
 	static const struct luaL_Reg boxlib_internal[] = {
 		{"select", lbox_select},
+		{"format_new", lbox_format_new},
 		{NULL, NULL}
 	};
 
 	luaL_register(L, "box.internal", boxlib_internal);
 	lua_pop(L, 1);
+
+	int rc = luaL_cdef(L, "struct tuple_format;");
+	assert(rc == 0);
+	(void) rc;
+	CTID_STRUCT_TUPLE_FORMAT_PTR = luaL_ctypeid(L, "struct tuple_format *");
+	assert(CTID_STRUCT_TUPLE_FORMAT_PTR != 0);
 }
diff --git a/test/box/misc.result b/test/box/misc.result
index 43b5a4a..ce39bbb 100644
--- a/test/box/misc.result
+++ b/test/box/misc.result
@@ -1271,3 +1271,51 @@ box.cfg{too_long_threshold = too_long_threshold}
 s:drop()
 ---
 ...
+--
+-- gh-2978: Function to parse space format.
+--
+format = {}
+---
+...
+format[1] = {}
+---
+...
+format[1].type = 'unsigned'
+---
+...
+box.internal.format_new(format)
+---
+- error: field 1 name is not specified
+...
+format[1].name = 'aaa'
+---
+...
+format[2] = {}
+---
+...
+format[2].name = 'aaa'
+---
+...
+format[2].type = 'any'
+---
+...
+box.internal.format_new(format)
+---
+- error: Space field 'aaa' is duplicate
+...
+format[2].name = 'bbb'
+---
+...
+format[3] = {}
+---
+...
+format[3].name = 'ccc'
+---
+...
+format[3].type = 'something'
+---
+...
+box.internal.format_new(format)
+---
+- error: field 3 has unknown field type
+...
diff --git a/test/box/misc.test.lua b/test/box/misc.test.lua
index 75d937e..2d13b99 100644
--- a/test/box/misc.test.lua
+++ b/test/box/misc.test.lua
@@ -353,3 +353,23 @@ lsn == expected_lsn
 box.cfg{too_long_threshold = too_long_threshold}
 s:drop()
 
+
+--
+-- gh-2978: Function to parse space format.
+--
+format = {}
+format[1] = {}
+format[1].type = 'unsigned'
+box.internal.format_new(format)
+
+format[1].name = 'aaa'
+format[2] = {}
+format[2].name = 'aaa'
+format[2].type = 'any'
+box.internal.format_new(format)
+
+format[2].name = 'bbb'
+format[3] = {}
+format[3].name = 'ccc'
+format[3].type = 'something'
+box.internal.format_new(format)




Second new patch:

From f9e959e3f0f38e8f6b8f1294ba29c28d41f30968 Mon Sep 17 00:00:00 2001
Date: Tue, 11 Jun 2019 16:36:39 +0300
Subject: [PATCH] netbox: define formats for tuple from netbox

This patch creates tuple_formats for the tuples obtained through
the netbox.

Closes #2978

@TarantoolBot document
Title: Field names for tuples received from net.box

It is possible now to access by field name for tuples received
from net.box. For example:

box.cfg{listen = 3302}
box.schema.user.grant('guest','read, write, execute', 'space')
box.schema.user.grant('guest', 'create', 'space')

box.schema.create_space("named", {format = {{name = "id"}}})
box.space.named:create_index('id', {parts = {{1, 'unsigned'}}})
box.space.named:insert({1})
require('net.box').connect('localhost', 3302).space.named:get(1).id

Result:

tarantool> require('net.box').connect('localhost', 3302).space.named:get(1).id
---
- 1
...

diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
index 7484a86..946d397 100644
--- a/src/box/lua/net_box.c
+++ b/src/box/lua/net_box.c
@@ -590,12 +590,11 @@ netbox_encode_execute(lua_State *L)
  * @param data MessagePack.
  */
 static void
-netbox_decode_data(struct lua_State *L, const char **data)
+netbox_decode_data(struct lua_State *L, const char **data,
+		   struct tuple_format *format)
 {
 	uint32_t count = mp_decode_array(data);
 	lua_createtable(L, count, 0);
-	struct tuple_format *format =
-		box_tuple_format_default();
 	for (uint32_t j = 0; j < count; ++j) {
 		const char *begin = *data;
 		mp_next(data);
@@ -618,6 +617,15 @@ static int
 netbox_decode_select(struct lua_State *L)
 {
 	uint32_t ctypeid;
+	int top = lua_gettop(L);
+	assert(top == 1 || top == 2);
+	struct tuple_format *format;
+	if (top == 2 && lua_type(L, 2) == LUA_TCDATA) {
+		format = *(struct tuple_format **)luaL_checkcdata(L, 2,
+								  &ctypeid);
+	} else {
+		format = tuple_format_runtime;
+	}
 	const char *data = *(const char **)luaL_checkcdata(L, 1, &ctypeid);
 	assert(mp_typeof(*data) == MP_MAP);
 	uint32_t map_size = mp_decode_map(&data);
@@ -627,7 +635,7 @@ netbox_decode_select(struct lua_State *L)
 	uint32_t key = mp_decode_uint(&data);
 	assert(key == IPROTO_DATA);
 	(void) key;
-	netbox_decode_data(L, &data);
+	netbox_decode_data(L, &data, format);
 	*(const char **)luaL_pushcdata(L, ctypeid) = data;
 	return 2;
 }
@@ -716,7 +724,7 @@ netbox_decode_execute(struct lua_State *L)
 		uint32_t key = mp_decode_uint(&data);
 		switch(key) {
 		case IPROTO_DATA:
-			netbox_decode_data(L, &data);
+			netbox_decode_data(L, &data, tuple_format_runtime);
 			rows_index = i - map_size;
 			break;
 		case IPROTO_METADATA:
diff --git a/src/box/lua/net_box.lua b/src/box/lua/net_box.lua
index d9838f8..30cf227 100644
--- a/src/box/lua/net_box.lua
+++ b/src/box/lua/net_box.lua
@@ -63,12 +63,12 @@ local function decode_data(raw_data)
     local response, raw_end = decode(raw_data)
     return response[IPROTO_DATA_KEY], raw_end
 end
-local function decode_tuple(raw_data)
-    local response, raw_end = internal.decode_select(raw_data)
+local function decode_tuple(raw_data, raw_data_end, args)
+    local response, raw_end = internal.decode_select(raw_data, args.format)
     return response[1], raw_end
 end
-local function decode_get(raw_data)
-    local body, raw_end = internal.decode_select(raw_data)
+local function decode_get(raw_data, raw_data_end, args)
+    local body, raw_end = internal.decode_select(raw_data, args.format)
     if body[2] then
         return nil, raw_end, box.error.MORE_THAN_ONE_TUPLE
     end
@@ -82,6 +82,12 @@ local function decode_push(raw_data)
     local response, raw_end = decode(raw_data)
     return response[IPROTO_DATA_KEY][1], raw_end
 end
+local function decode_call_16(raw_data)
+    return internal.decode_select(raw_data)
+end
+local function decode_select(raw_data, raw_data_end, args)
+    return internal.decode_select(raw_data, args.format)
+end
 
 local function encode_call(send_buf, id, args)
     return internal.encode_call(send_buf, id, args.func_name, args.args)
@@ -150,7 +156,7 @@ local method_encoder = {
 
 local method_decoder = {
     ping    = decode_nil,
-    call_16 = internal.decode_select,
+    call_16 = decode_call_16,
     call_17 = decode_data,
     eval    = decode_data,
     insert  = decode_tuple,
@@ -158,7 +164,7 @@ local method_decoder = {
     delete  = decode_tuple,
     update  = decode_tuple,
     upsert  = decode_nil,
-    select  = internal.decode_select,
+    select  = decode_select,
     execute = internal.decode_execute,
     get     = decode_get,
     min     = decode_get,
@@ -623,7 +629,8 @@ local function create_transport(host, port, user, password, callback,
         -- Decode xrow.body[DATA] to Lua objects
         if status == IPROTO_OK_KEY then
             request.response, real_end, request.errno =
-                method_decoder[request.method](body_rpos, body_end)
+                method_decoder[request.method](body_rpos, body_end,
+                                               request.args)
             assert(real_end == body_end, "invalid body length")
             requests[id] = nil
             request.id = nil
@@ -1267,6 +1274,7 @@ function remote_methods:_install_schema(schema_version, spaces, indices,
         s.index = {}
         s.temporary = false
         s._format = format
+        s._format_cdata = box.internal.format_new(format)
         s.connection = self
         if #space > 5 then
             local opts = space[6]
@@ -1384,13 +1392,15 @@ space_metatable = function(remote)
 
     function methods:insert(tuple, opts)
         check_space_arg(self, 'insert')
-        local args = {space_id = self.id, tuple = tuple}
+        local args = {space_id = self.id, tuple = tuple,
+                      format = self._format_cdata}
         return remote:_request('insert', opts, args)
     end
 
     function methods:replace(tuple, opts)
         check_space_arg(self, 'replace')
-        local args = {space_id = self.id, tuple = tuple}
+        local args = {space_id = self.id, tuple = tuple,
+                      format = self._format_cdata}
         return remote:_request('replace', opts, args)
     end
 
@@ -1443,7 +1453,7 @@ index_metatable = function(remote)
         local limit = tonumber(opts and opts.limit) or 0xFFFFFFFF
         local args = {space_id = self.space.id, index_id = self.id,
                       iterator = iterator, offset = offset, limit = limit,
-                      key = key}
+                      key = key, format = self.space._format_cdata}
         return (remote:_request('select', opts, args))
     end
 
@@ -1453,7 +1463,8 @@ index_metatable = function(remote)
             error("index:get() doesn't support `buffer` argument")
         end
         local args = {space_id = self.space.id, index_id = self.id,
-                      iterator = box.index.EQ, offset = 0, limit = 2, key = key}
+                      iterator = box.index.EQ, offset = 0, limit = 2, key = key,
+                      format = self.space._format_cdata}
         return nothing_or_data(remote:_request('get', opts, args))
     end
 
@@ -1463,7 +1474,8 @@ index_metatable = function(remote)
             error("index:min() doesn't support `buffer` argument")
         end
         local args = {space_id = self.space.id, index_id = self.id,
-                      iterator = box.index.GE, offset = 0, limit = 1, key = key}
+                      iterator = box.index.GE, offset = 0, limit = 1, key = key,
+                      format = self.space._format_cdata}
         return nothing_or_data(remote:_request('min', opts, args))
     end
 
@@ -1473,7 +1485,8 @@ index_metatable = function(remote)
             error("index:max() doesn't support `buffer` argument")
         end
         local args = {space_id = self.space.id, index_id = self.id,
-                      iterator = box.index.LE, offset = 0, limit = 1, key = key}
+                      iterator = box.index.LE, offset = 0, limit = 1, key = key,
+                      format = self.space._format_cdata}
         return nothing_or_data(remote:_request('max', opts, args))
     end
 
@@ -1490,14 +1503,15 @@ index_metatable = function(remote)
 
     function methods:delete(key, opts)
         check_index_arg(self, 'delete')
-        local args = {space_id = self.space.id, index_id = self.id, key = key}
+        local args = {space_id = self.space.id, index_id = self.id, key = key,
+                      format = self.space._format_cdata}
         return nothing_or_data(remote:_request('delete', opts, args))
     end
 
     function methods:update(key, oplist, opts)
         check_index_arg(self, 'update')
         local args = {space_id = self.space.id, index_id = self.id, key = key,
-                      oplist = oplist}
+                      oplist = oplist, format = self.space._format_cdata}
         return nothing_or_data(remote:_request('update', opts, args))
     end
 
diff --git a/test/box/net.box.result b/test/box/net.box.result
index e87a60b..413bdec 100644
--- a/test/box/net.box.result
+++ b/test/box/net.box.result
@@ -3572,6 +3572,104 @@ box.schema.func.drop('change_schema')
 ---
 ...
 --
+-- gh-2978: field names for tuples received from netbox.
+--
+_ = box.schema.create_space("named", {format = {{name = "id"}, {name="abc"}}})
+---
+...
+_ = box.space.named:create_index('id', {parts = {{1, 'unsigned'}}})
+---
+...
+box.space.named:insert({1, 1})
+---
+- [1, 1]
+...
+box.schema.user.grant('guest', 'read, write, execute', 'space')
+---
+...
+cn = net.connect(box.cfg.listen)
+---
+...
+s = cn.space.named
+---
+...
+s:get{1}.id
+---
+- 1
+...
+s:get{1}:tomap()
+---
+- 1: 1
+  2: 1
+  abc: 1
+  id: 1
+...
+s:insert{2,3}:tomap()
+---
+- 1: 2
+  2: 3
+  abc: 3
+  id: 2
+...
+s:replace{2,14}:tomap()
+---
+- 1: 2
+  2: 14
+  abc: 14
+  id: 2
+...
+s:update(1, {{'+', 2, 10}}):tomap()
+---
+- 1: 1
+  2: 11
+  abc: 11
+  id: 1
+...
+s:select()[1]:tomap()
+---
+- 1: 1
+  2: 11
+  abc: 11
+  id: 1
+...
+s:delete({2}):tomap()
+---
+- 1: 2
+  2: 14
+  abc: 14
+  id: 2
+...
+-- Check that formats changes after reload.
+box.space.named:format({{name = "id2"}, {name="abc2"}})
+---
+...
+s:select()[1]:tomap()
+---
+- 1: 1
+  2: 11
+  abc: 11
+  id: 1
+...
+cn:reload_schema()
+---
+...
+s:select()[1]:tomap()
+---
+- 1: 1
+  2: 11
+  id2: 1
+  abc2: 11
+...
+cn:close()
+---
+...
+box.space.named:drop()
+---
+...
+box.schema.user.revoke('guest', 'read, write, execute', 'space')
+---
+...
+--
 -- gh-3400: long-poll input discard must not touch event loop of
 -- a closed connection.
 --
diff --git a/test/box/net.box.test.lua b/test/box/net.box.test.lua
index 6e58da8..759cc4f 100644
--- a/test/box/net.box.test.lua
+++ b/test/box/net.box.test.lua
@@ -1433,6 +1433,34 @@ box.space.test3:drop()
 box.schema.func.drop('change_schema')
 
 --
+-- gh-2978: field names for tuples received from netbox.
+--
+_ = box.schema.create_space("named", {format = {{name = "id"}, {name="abc"}}})
+_ = box.space.named:create_index('id', {parts = {{1, 'unsigned'}}})
+box.space.named:insert({1, 1})
+box.schema.user.grant('guest', 'read, write, execute', 'space')
+cn = net.connect(box.cfg.listen)
+
+s = cn.space.named
+s:get{1}.id
+s:get{1}:tomap()
+s:insert{2,3}:tomap()
+s:replace{2,14}:tomap()
+s:update(1, {{'+', 2, 10}}):tomap()
+s:select()[1]:tomap()
+s:delete({2}):tomap()
+
+-- Check that formats changes after reload.
+box.space.named:format({{name = "id2"}, {name="abc2"}})
+s:select()[1]:tomap()
+cn:reload_schema()
+s:select()[1]:tomap()
+
+cn:close()
+box.space.named:drop()
+box.schema.user.revoke('guest', 'read, write, execute', 'space')
+
+--
 -- gh-3400: long-poll input discard must not touch event loop of
 -- a closed connection.
 --

Re: [PATCH v1 2/2] netbox: define formats for tuple from netbox

[Vladimir Davydov]

On Fri, Jun 14, 2019 at 03:29:21PM +0300, Mergen Imeev wrote:
> First new patch:

It looks like it's time to send v2 in a separate thread.

See a few minor comments inline.

> 
> From 63075deb8411643d4af0cf27be2c024b5a20222c Mon Sep 17 00:00:00 2001
> Date: Tue, 11 Jun 2019 15:21:39 +0300
> Subject: [PATCH] lua: internal function to parse space format
> 
> This patch defines a new function that parses space format and
> returns it to Lua as cdata. For this cdata destructor is included.
> 
> Needed for #2978
> 
> diff --git a/src/box/lua/misc.cc b/src/box/lua/misc.cc
> index 8de7401..e62e2ba 100644
> --- a/src/box/lua/misc.cc
> +++ b/src/box/lua/misc.cc
> @@ -37,9 +37,13 @@
>  
>  #include "box/box.h"
>  #include "box/port.h"
> +#include "box/tuple.h"
> +#include "box/tuple_format.h"
>  #include "box/lua/tuple.h"
>  #include "mpstream.h"
>  
> +uint32_t CTID_STRUCT_TUPLE_FORMAT_PTR;
> +

Should be static.

>  /** {{{ Miscellaneous utils **/
>  
>  char *
> @@ -116,14 +120,117 @@ lbox_select(lua_State *L)
>  
>  /* }}} */
>  
> +static int
> +lbox_tuple_format_gc(struct lua_State *L)
> +{
> +	uint32_t ctypeid;
> +	struct tuple_format *format =
> +		*(struct tuple_format **)luaL_checkcdata(L, 1, &ctypeid);
> +	if (ctypeid != CTID_STRUCT_TUPLE_FORMAT_PTR) {
> +		luaL_error(L, "Invalid argument: 'struct tuple_format *' "\

Backslash (\) is not necessary.

Anyway, I think we better factor this check out into a separate
function, like lboxk_check_tuple_format, so that we could easily
reuse it in case we introduce some new functions taking a format.

> +			   "expected, got %s)",
> +			   lua_typename(L, lua_type(L, 1)));
> +	}
> +	box_tuple_format_unref(format);

Please use tuple_format_ref/tuple_format_unref.

box_* functions exist for external modules. No need to use them here.

> +	return 0;
> +}
> +
> +static int
> +lbox_push_tuple_format(struct lua_State *L, struct tuple_format *format)
> +{
> +	struct tuple_format **ptr = (struct tuple_format **)
> +		luaL_pushcdata(L, CTID_STRUCT_TUPLE_FORMAT_PTR);
> +	*ptr = format;
> +	box_tuple_format_ref(format);
> +	lua_pushcfunction(L, lbox_tuple_format_gc);
> +	luaL_setcdatagc(L, -2);
> +	return 1;
> +}
> +
> +static int
> +lbox_format_new(struct lua_State *L)

Please be consistent and use tuple_format prefix everywhere.

> +{
> +	assert(CTID_STRUCT_TUPLE_FORMAT_PTR != 0);
> +	if (lua_gettop(L) != 1 || ! lua_istable(L, 1))
> +		return luaL_error(L, "Usage: box.internal.format_new(format)");

I would rather name this function box.internal.new_tuple_format.

Also, I think we should allow calling it without any arguments, in which
case it should be equivalent to box.internal.new_tuple_format({}).

> +	uint32_t count = lua_objlen(L, 1);
> +	if (count == 0)
> +		return lbox_push_tuple_format(L, tuple_format_runtime);
> +	size_t size = count * sizeof(struct field_def);
> +	struct region *region = &fiber()->gc;
> +	size_t region_svp = region_used(region);
> +	struct field_def *fields =
> +		(struct field_def *)region_alloc(region, size);
> +	if (fields == NULL) {
> +		diag_set(OutOfMemory, size, "region_alloc", "fields");
> +		return luaT_error(L);
> +	}
> +	for (uint32_t i = 0; i < count; ++i) {
> +		size_t len;
> +
> +		fields[i] = field_def_default;
> +
> +		lua_pushinteger(L, i + 1);
> +		lua_gettable(L, 1);
> +
> +		lua_pushstring(L, "type");
> +		lua_gettable(L, -2);
> +		if (! lua_isnil(L, -1)) {
> +			const char *type_name = lua_tolstring(L, -1, &len);
> +			fields[i].type = field_type_by_name(type_name, len);
> +			if (fields[i].type == field_type_MAX) {
> +				const char *err =
> +					"field %d has unknown field type";
> +				return luaL_error(L, tt_sprintf(err, i + 1));
> +			}
> +		}
> +		lua_pop(L, 1);
> +
> +		lua_pushstring(L, "name");
> +		lua_gettable(L, -2);
> +		if (lua_isnil(L, -1)) {
> +			return luaL_error(L, tt_sprintf("field %d name is not "\

Useless backslash (\).

> +							"specified", i + 1));
> +		}
> +		const char *name = lua_tolstring(L, -1, &len);
> +		fields[i].name = (char *)region_alloc(region, len + 1);
> +		if (fields == NULL) {
> +			diag_set(OutOfMemory, size, "region_alloc",
> +				 "fields[i].name");
> +			return luaT_error(L);
> +		}
> +		memcpy(fields[i].name, name, len);
> +		fields[i].name[len] = '\0';
> +		lua_pop(L, 1);
> +		lua_pop(L, 1);
> +	}
> +	struct tuple_dictionary *dict = tuple_dictionary_new(fields, count);
> +	region_truncate(region, region_svp);
> +	if (dict == NULL)
> +		return luaT_error(L);
> +	struct tuple_format *format =
> +		tuple_format_new(&tuple_format_runtime->vtab, NULL, NULL, 0,
> +				 NULL, 0, 0, dict, false, false);
> +	if (format == NULL)
> +		return luaT_error(L);
> +	return lbox_push_tuple_format(L, format);
> +}
> +
>  void
>  box_lua_misc_init(struct lua_State *L)
>  {
>  	static const struct luaL_Reg boxlib_internal[] = {
>  		{"select", lbox_select},
> +		{"format_new", lbox_format_new},
>  		{NULL, NULL}
>  	};
>  
>  	luaL_register(L, "box.internal", boxlib_internal);
>  	lua_pop(L, 1);
> +
> +	int rc = luaL_cdef(L, "struct tuple_format;");
> +	assert(rc == 0);
> +	(void) rc;
> +	CTID_STRUCT_TUPLE_FORMAT_PTR = luaL_ctypeid(L, "struct tuple_format *");
> +	assert(CTID_STRUCT_TUPLE_FORMAT_PTR != 0);
>  }
> diff --git a/test/box/misc.result b/test/box/misc.result
> index 43b5a4a..ce39bbb 100644
> --- a/test/box/misc.result
> +++ b/test/box/misc.result
> @@ -1271,3 +1271,51 @@ box.cfg{too_long_threshold = too_long_threshold}
>  s:drop()
>  ---
>  ...
> +--
> +-- gh-2978: Function to parse space format.
> +--
> +format = {}
> +---
> +...
> +format[1] = {}
> +---
> +...
> +format[1].type = 'unsigned'
> +---
> +...
> +box.internal.format_new(format)
> +---
> +- error: field 1 name is not specified
> +...
> +format[1].name = 'aaa'
> +---
> +...
> +format[2] = {}
> +---
> +...
> +format[2].name = 'aaa'
> +---
> +...
> +format[2].type = 'any'
> +---
> +...
> +box.internal.format_new(format)
> +---
> +- error: Space field 'aaa' is duplicate
> +...
> +format[2].name = 'bbb'
> +---
> +...
> +format[3] = {}
> +---
> +...
> +format[3].name = 'ccc'
> +---
> +...
> +format[3].type = 'something'
> +---
> +...
> +box.internal.format_new(format)
> +---
> +- error: field 3 has unknown field type
> +...

Please write more tests:
 - setting 'format' without 'type'
 - passing something different from a table to the function
 - passing the value returned by space:format() to this function
 - setting is_nullable in the input table

Re: [PATCH v1 2/2] netbox: define formats for tuple from netbox

[Vladimir Davydov]

On Fri, Jun 14, 2019 at 03:29:21PM +0300, Mergen Imeev wrote:
> Second new patch:
> 
> From f9e959e3f0f38e8f6b8f1294ba29c28d41f30968 Mon Sep 17 00:00:00 2001
> Date: Tue, 11 Jun 2019 16:36:39 +0300
> Subject: [PATCH] netbox: define formats for tuple from netbox
> 
> This patch creates tuple_formats for the tuples obtained through
> the netbox.
> 
> Closes #2978
> 
> @TarantoolBot document
> Title: Field names for tuples received from net.box
> 
> It is possible now to access by field name for tuples received
> from net.box. For example:
> 
> box.cfg{listen = 3302}
> box.schema.user.grant('guest','read, write, execute', 'space')
> box.schema.user.grant('guest', 'create', 'space')
> 
> box.schema.create_space("named", {format = {{name = "id"}}})
> box.space.named:create_index('id', {parts = {{1, 'unsigned'}}})
> box.space.named:insert({1})
> require('net.box').connect('localhost', 3302).space.named:get(1).id
> 
> Result:
> 
> tarantool> require('net.box').connect('localhost', 3302).space.named:get(1).id
> ---
> - 1
> ...
> 
> diff --git a/src/box/lua/net_box.c b/src/box/lua/net_box.c
> index 7484a86..946d397 100644
> --- a/src/box/lua/net_box.c
> +++ b/src/box/lua/net_box.c
> @@ -590,12 +590,11 @@ netbox_encode_execute(lua_State *L)
>   * @param data MessagePack.
>   */
>  static void
> -netbox_decode_data(struct lua_State *L, const char **data)
> +netbox_decode_data(struct lua_State *L, const char **data,
> +		   struct tuple_format *format)
>  {
>  	uint32_t count = mp_decode_array(data);
>  	lua_createtable(L, count, 0);
> -	struct tuple_format *format =
> -		box_tuple_format_default();
>  	for (uint32_t j = 0; j < count; ++j) {
>  		const char *begin = *data;
>  		mp_next(data);
> @@ -618,6 +617,15 @@ static int
>  netbox_decode_select(struct lua_State *L)
>  {
>  	uint32_t ctypeid;
> +	int top = lua_gettop(L);
> +	assert(top == 1 || top == 2);
> +	struct tuple_format *format;
> +	if (top == 2 && lua_type(L, 2) == LUA_TCDATA) {
> +		format = *(struct tuple_format **)luaL_checkcdata(L, 2,
> +								  &ctypeid);

I think we should use lbox_check_tuple_format helper from the previous
patch here.

Other than that, this patch looks good to me.

> +	} else {
> +		format = tuple_format_runtime;
> +	}
>  	const char *data = *(const char **)luaL_checkcdata(L, 1, &ctypeid);
>  	assert(mp_typeof(*data) == MP_MAP);
>  	uint32_t map_size = mp_decode_map(&data);

Re: [PATCH v1 0/2] netbox: define formats for tuple from netbox

[Vladimir Davydov]

On Mon, Jun 10, 2019 at 01:02:21PM +0300, imeevma@tarantool.org wrote:
> This patch defines field names for the tuples obtained through
> the netbox.

Links to the branch and the ticket are missing.

> 
> Mergen Imeev (2):
>   netbox: store method_encoder args in request
>   netbox: define formats for tuple from netbox
> 
>  src/box/lua/net_box.c     |  87 ++++++++++++++++-
>  src/box/lua/net_box.lua   | 232 +++++++++++++++++++++++++++++++++-------------
>  test/box/access.result    |  15 ++-
>  test/box/access.test.lua  |   9 +-
>  test/box/net.box.result   |  83 ++++++++++++++++-
>  test/box/net.box.test.lua |  26 +++++-
>  6 files changed, 369 insertions(+), 83 deletions(-)

Re: [PATCH v1 0/2] netbox: define formats for tuple from netbox

[Mergen Imeev]

Hi! Thank you for review. I'm sorry, I haven't looked at my
patch properly before sending it last time.

https://github.com/tarantool/tarantool/issues/2978
https://github.com/tarantool/tarantool/tree/imeevma/gh-2978-formats-for-tuples-from-net-box

On Tue, Jun 11, 2019 at 11:55:52AM +0300, Vladimir Davydov wrote:
> On Mon, Jun 10, 2019 at 01:02:21PM +0300, imeevma@tarantool.org wrote:
> > This patch defines field names for the tuples obtained through
> > the netbox.
> 
> Links to the branch and the ticket are missing.
> 
> > 
> > Mergen Imeev (2):
> >   netbox: store method_encoder args in request
> >   netbox: define formats for tuple from netbox
> > 
> >  src/box/lua/net_box.c     |  87 ++++++++++++++++-
> >  src/box/lua/net_box.lua   | 232 +++++++++++++++++++++++++++++++++-------------
> >  test/box/access.result    |  15 ++-
> >  test/box/access.test.lua  |   9 +-
> >  test/box/net.box.result   |  83 ++++++++++++++++-
> >  test/box/net.box.test.lua |  26 +++++-
> >  6 files changed, 369 insertions(+), 83 deletions(-)