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

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

[Chris Sosnin]

[-- Attachment #1: Type: text/plain, Size: 2793 bytes --]


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
---
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);
return -1;
}
int32_t field_no = 0;
diff --git a/test/box/update.result b/test/box/update.result
index c6a5a25a7..b89b7124b 100644
--- a/test/box/update.result
+++ b/test/box/update.result
@@ -826,11 +826,11 @@ s:update({0}, {{}})
...
s:update({0}, {{'+'}})
---
-- error: Unknown UPDATE operation
+- error: 'Wrong number of arguments is passed to update(): expected 3, got 1'
...
s:update({0}, {{'+', 0}})
---
-- error: Unknown UPDATE operation
+- error: 'Wrong number of arguments is passed to update(): expected 3, got 2'
...
s:update({0}, {{'+', '+', '+'}})
---
diff --git a/test/engine/upsert.result b/test/engine/upsert.result
index 47da307fa..1e1441237 100644
--- a/test/engine/upsert.result
+++ b/test/engine/upsert.result
@@ -2117,15 +2117,15 @@ test(t, {{'+', 3, 3}, {'&', 3, 'a'}}, {{1, '1', 1, '1'}})
...
test(t, {{'+', 3}, {'&', 3, 'a'}}, {{1, '1', 1, '1'}})
---
-- error: Unknown UPDATE operation
+- error: 'Wrong number of arguments is passed to update(): expected 3, got 2'
...
test(t, {{':', 3, 3}}, {{1, '1', 1, '1'}})
---
-- error: Unknown UPDATE operation
+- error: 'Wrong number of arguments is passed to update(): expected 5, got 3'
...
test(t, {{':', 3, 3, 3}}, {{1, '1', 1, '1'}})
---
-- error: Unknown UPDATE operation
+- error: 'Wrong number of arguments is passed to update(): expected 5, got 4'
...
test(t, {{'?', 3, 3}}, {{1, '1', 1, '1'}})
---
diff --git a/test/vinyl/gh.result b/test/vinyl/gh.result
index 78cb2a28d..7985ac903 100644
--- a/test/vinyl/gh.result
+++ b/test/vinyl/gh.result
@@ -369,7 +369,7 @@ s:select()
...
s:upsert({1, 'test', 'failed'}, {{'=', 3, 33}, {'=', 4, nil}})
---
-- error: Unknown UPDATE operation
+- error: 'Wrong number of arguments is passed to update(): expected 3, got 2'
...
s:select()
---
-- 
2.24.0
 

[-- Attachment #2: Type: text/html, Size: 3153 bytes --]

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

[Vladislav Shpilevoy]

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.