Re: [Tarantool-patches] [PATCH] box: fix non-informative update() error message

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

Thanks for the patch!

See 3 comments below.

On 24/11/2019 22:15, Chris Sosnin wrote:
> Calling tuple_object:update() with invalid argument number
> yields 'Unknown UPDATE operation' error. Instead, we replace this
> error with explicit "Wrong argument number".
> Taking an actual issue into account, the problem is about
> nil - box.NULL confusion, maybe we could consider giving some
> kind of warning in this case.
> 
> Fixes: #3884
> ---

1. All the same as for #4515 email.

> src/box/xrow_update_field.c | 4 +++-
> test/box/update.result | 4 ++--
> test/engine/upsert.result | 6 +++---
> test/vinyl/gh.result | 2 +-
> 4 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
> index c694e17e9..6d1334e02 100644
> --- a/src/box/xrow_update_field.c
> +++ b/src/box/xrow_update_field.c
> @@ -625,7 +625,9 @@ xrow_update_op_decode(struct xrow_update_op *op, int index_base,
> if (op->meta == NULL)
> return -1;
> if (arg_count != op->meta->arg_count) {
> - diag_set(ClientError, ER_UNKNOWN_UPDATE_OP);
> + char s[10];
> + sprintf(s, "%u", op->meta->arg_count);
> + diag_set(ClientError, ER_FUNC_WRONG_ARG_COUNT, "update", s, arg_count);

2. Instead of allocating buffers on the stack you can use
static allocator. See static.h and tt_static.h for details.
Here you could do


    const char* str = tt_sprintf(<your format>);
    <use str>

3. Error codes usually are divided into groups,
related to a certain subsystem. For example,
ER_FUNC_WRONG_ARG_COUNT error code is related to stored
functions (lua, SQL). As a result, it does not contain
an updated field name, or its number.

Tuple:update() is not a stored function. It is a
tuple method. For this method we have a group of
error codes ER_UPDATE_*. They show field name or
number on which the error has happened.

Concretely for this place we don't have a ready to
use error code. I think, you need to extend
ER_UNKNOWN_UPDATE_OP's error message. It should
contain a reason why it is unknown. An update operation
is unknown like in xrow_update_op_by(); or it contains
a wrong number of arguments like here in the patch.

In both cases it would be nice to have an ordinal number
of the operation in the message.