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

[Vladimir Davydov <vdavydov.dev@gmail.com>]

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')
> +---
> +...
> +--