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

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

Thanks for the patch!

When changes are minor, it is not worth sending a new
version. Just respond to my comments in the same thread,
and append a new patch version to the end of the response.

Also, please, when address my objections, answer to them
inline with a piece of diff for that concrete objection.

So your response email should look like this:

    > Cite of my comment 1

    Your answer 1.
    Your diff to that comment.

    > Cite of my comment 2

    Your answer 2.
    Your diff to that comment.

    Etc.

    New version of the patch.

In case you send a new version of the patch in a new thread,
you should respond to my comments in the old thread.

When you cite my comments, and respond to each of them, you
make the review simpler and faster.

This patch LGTM.

On 27/11/2019 22:30, Chris Sosnin wrote:
> Thank you for the review!
> Fixed version below.
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4262-frommap-error-handle
> issue: https://github.com/tarantool/tarantool/issues/4262
> 
> - 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.
> 
> - 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
> ---
>  src/box/lua/space.cc    | 13 +++++++++----
>  test/box/tuple.result   |  8 +++++++-
>  test/box/tuple.test.lua |  1 +
>  3 files changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/src/box/lua/space.cc b/src/box/lua/space.cc
> index f6e96f0c0..e772a924d 100644
> --- a/src/box/lua/space.cc
> +++ b/src/box/lua/space.cc
> @@ -580,14 +580,19 @@ 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;
> +	}
> +	if (table) {
> +		return 1;
> +	}
>  	luaT_pushtuple(L, tuple);
>  	return 1;
>  usage_error:
> diff --git a/test/box/tuple.result b/test/box/tuple.result
> index 9140211b7..78f919deb 100644
> --- a/test/box/tuple.result
> +++ b/test/box/tuple.result
> @@ -1121,7 +1121,13 @@ s:frommap()
>  ...
>  s:frommap({})
>  ---
> -- error: Tuple field 1 required by space format is missing
> +- null
> +- Tuple field 1 required by space format is missing
> +...
> +s:frommap({ddd = 'fail', aaa = 2, ccc = 3, bbb = 4}, {table=true})
> +---
> +- null
> +- 'Tuple field 4 type does not match one required by operation: expected unsigned'
>  ...
>  s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
>  ---
> diff --git a/test/box/tuple.test.lua b/test/box/tuple.test.lua
> index 3ac2902fe..baf2f22d5 100644
> --- a/test/box/tuple.test.lua
> +++ b/test/box/tuple.test.lua
> @@ -373,6 +373,7 @@ s:frommap({ddd = 1, aaa = 2, bbb = 3})
>  s:frommap({ddd = 1, aaa = 2, ccc = 3, eee = 4})
>  s:frommap()
>  s:frommap({})
> +s:frommap({ddd = 'fail', aaa = 2, ccc = 3, bbb = 4}, {table=true})
>  s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = true})
>  s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = 4}, {table = false})
>  s:frommap({ddd = 1, aaa = 2, ccc = 3, bbb = box.NULL})
> --
> 2.21.0 (Apple Git-122.2)
>