From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp44.i.mail.ru (smtp44.i.mail.ru [94.100.177.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 3F49346970F for ; Tue, 26 Nov 2019 02:42:32 +0300 (MSK) References: <1574630112.698478354@f497.i.mail.ru> From: Vladislav Shpilevoy Message-ID: <04465189-1892-fb77-d1ae-f1ef32b55581@tarantool.org> Date: Tue, 26 Nov 2019 00:42:30 +0100 MIME-Version: 1.0 In-Reply-To: <1574630112.698478354@f497.i.mail.ru> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [Tarantool-patches] [PATCH] box: fix non-informative update() error message List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Chris Sosnin , tarantool-patches@dev.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(); 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.