Re: [Tarantool-patches] [PATCH] box: frommap() bug fix

[Vladislav Shpilevoy <v.shpilevoy@tarantool.org>]

Thanks for the patch!

See 4 comments below.

On 24/11/2019 22:18, Chris Sosnin wrote:
> - If an optional argument is provided for
>   space_object:frommap() (which is {table = true|false}),
>   type match for first arguments is omitted, which is
>   incorrect. We should return the result only after making
>   sure it is possible to build a tuple.

1. This is a good catch. Please, provide a test. To ensure,
that it won't break again in the future.

I think you fit it in an existing test file, where other
frommap() cases are checked. It is not worth creating a new
test file just for this tiny ticket.

> 
> - If there is a type mismatch, however, frommap() does not
>   return nil, err as it is mentioned in the description, so we
>   change it to be this way.
> 
> Closes: #4262
> ---

2. All the same as for #4515 email.

>  src/box/lua/space.cc  | 11 +++++++----
>  test/box/tuple.result |  3 ++-
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index f6e96f0c0..a4ffa8240 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -580,14 +580,17 @@ lbox_space_frommap(struct lua_State *L)
>  		}
>  		lua_rawseti(L, -3, fieldno+1);
>  	}
> -	if (table)
> -		return 1;
> 
>  	lua_replace(L, 1);
>  	lua_settop(L, 1);
>  	tuple = luaT_tuple_new(L, -1, space->format);
> -	if (tuple == NULL)
> -		return luaT_error(L);
> +	if (tuple == NULL) {
> +		struct error *e = diag_last_error(&fiber()->diag);
> +		lua_pushnil(L);
> +		lua_pushstring(L, e->errmsg);
> +		return 2;
> +	} else if (table)
> +		return 1;

3. Nit: I think you may omit 'else' keyword here. Anyway
the previous condition makes a return.

4. When any if's or else's body has {}, all the others
in the same chain should have it as well. So 'return 1;'
should also be in {}.

>  	luaT_pushtuple(L, tuple);
>  	return 1;
>  usage_error: