[tarantool-patches] Re: [PATCH v1 1/1] sql: rework error handling in box.execute()

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

Hi! Thanks for the patch!

On 30/07/2019 16:04, imeevma@tarantool.org wrote:
> In accordance with the Lua coding style in Tarantool, all errors
> returned in Lua should be returned using 'return nil, error'.
> However, box.execute() throws an exception in case of an error.
> This patch causes box.execute() to return an error, as described
> in the coding style.
> 
> Closes #4390
> ---
> https://github.com/tarantool/tarantool/issues/4390
> https://github.com/tarantool/tarantool/tree/imeevma/gh-4390-box_execute-should-not-throw
> 
> diff --git a/src/box/lua/execute.c b/src/box/lua/execute.c
> index 7b7c575..85c5738 100644
> --- a/src/box/lua/execute.c
> +++ b/src/box/lua/execute.c
> @@ -254,13 +254,25 @@ lbox_execute(struct lua_State *L)
>  		if (! lua_istable(L, 2))
>  			return luaL_error(L, "Second argument must be a table");
>  		bind_count = lua_sql_bind_list_decode(L, &bind, 2);
> -		if (bind_count < 0)
> -			return luaT_error(L);
> +		if (bind_count < 0) {
> +			lua_pushnil(L);
> +			struct error *e = box_error_last();
> +			if (e == NULL)
> +				return 1;
> +			luaT_pusherror(L, e);

First, how is it possible, that e == NULL? It should not be
so. I deleted that check, and the tests passed. So please,
drop it.

Secondly, there are already 4 places, which push exactly the same
nil + diag error. And 13 more intricate places in lua/fio.c.
I propose you to introduce a new function to push nil + last box
error.

Another place is lua_swim_new, which pushes nil implicitly,
when in the expression:

    *(struct swim **) luaL_pushcdata(L, ctid_swim_ptr) = s;

's' is NULL. But in fact it is the same case, and here you can
use than new function too.

Please, introduce and use that new function in a separate commit
before this one.