From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (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 8330E46970F for ; Wed, 27 Nov 2019 11:53:26 +0300 (MSK) Received: by smtpng3.m.smailru.net with esmtpa (envelope-from ) id 1iZt4f-0004tl-Sc for tarantool-patches@dev.tarantool.org; Wed, 27 Nov 2019 11:53:26 +0300 From: Chris Sosnin Date: Wed, 27 Nov 2019 11:53:25 +0300 Message-Id: <20191127085325.66144-1-k.sosnin@tarantool.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Subject: [Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: tarantool-patches@dev.tarantool.org Thank you for the review! See second version below. branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-3884-update-error issue: https://github.com/tarantool/tarantool/issues/3884 Calling tuple_object:update() with invalid argument number yields 'Unknown UPDATE operation' error. Instead, we replace this error with explicit "wrong argument number", mentioning which operation failed. Also add checking for correct opcode. Fixes #3884 --- src/box/errcode.h | 2 +- src/box/xrow_update.c | 4 ++-- src/box/xrow_update_field.c | 23 ++++++++++++++++------- src/box/xrow_update_field.h | 2 +- test/box/update.result | 6 ++++-- test/engine/upsert.result | 13 ++++++++----- test/vinyl/gh.result | 3 ++- 7 files changed, 34 insertions(+), 19 deletions(-) diff --git a/src/box/errcode.h b/src/box/errcode.h index c660b1c70..97603259b 100644 --- a/src/box/errcode.h +++ b/src/box/errcode.h @@ -80,7 +80,7 @@ struct errcode_record { /* 25 */_(ER_UPDATE_SPLICE, "SPLICE error on field %s: %s") \ /* 26 */_(ER_UPDATE_ARG_TYPE, "Argument type in operation '%c' on field %s does not match field type: expected %s") \ /* 27 */_(ER_FORMAT_MISMATCH_INDEX_PART, "Field %s has type '%s' in space format, but type '%s' in index definition") \ - /* 28 */_(ER_UNKNOWN_UPDATE_OP, "Unknown UPDATE operation") \ + /* 28 */_(ER_UNKNOWN_UPDATE_OP, "Unknown UPDATE operation #%d: %s") \ /* 29 */_(ER_UPDATE_FIELD, "Field %s UPDATE error: %s") \ /* 30 */_(ER_FUNCTION_TX_ACTIVE, "Transaction is active at return from function") \ /* 31 */_(ER_KEY_PART_COUNT, "Invalid key part count (expected [0..%u], got %u)") \ diff --git a/src/box/xrow_update.c b/src/box/xrow_update.c index 123db081a..db215aada 100644 --- a/src/box/xrow_update.c +++ b/src/box/xrow_update.c @@ -172,8 +172,8 @@ xrow_update_read_ops(struct xrow_update *update, const char *expr, } struct xrow_update_op *op = update->ops; struct xrow_update_op *ops_end = op + update->op_count; - for (; op < ops_end; op++) { - if (xrow_update_op_decode(op, update->index_base, dict, + for (int i = 1; op < ops_end; op++, i++) { + if (xrow_update_op_decode(op, i, update->index_base, dict, &expr) != 0) return -1; /* diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c index c694e17e9..47c263945 100644 --- a/src/box/xrow_update_field.c +++ b/src/box/xrow_update_field.c @@ -576,9 +576,14 @@ static const struct xrow_update_op_meta op_delete = { }; static inline const struct xrow_update_op_meta * -xrow_update_op_by(char opcode) +xrow_update_op_by(const char *opcode, uint32_t len, int op_num) { - switch (opcode) { + if (len != 1) { + diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num, + tt_sprintf("\"%.*s\"", len, opcode)); + return NULL; + } + switch (*opcode) { case '=': return &op_set; case '+': @@ -595,13 +600,14 @@ xrow_update_op_by(char opcode) case '!': return &op_insert; default: - diag_set(ClientError, ER_UNKNOWN_UPDATE_OP); + diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num, + tt_sprintf("\"%.*s\"", len, opcode)); return NULL; } } int -xrow_update_op_decode(struct xrow_update_op *op, int index_base, +xrow_update_op_decode(struct xrow_update_op *op, int op_num, int index_base, struct tuple_dictionary *dict, const char **expr) { if (mp_typeof(**expr) != MP_ARRAY) { @@ -620,12 +626,15 @@ xrow_update_op_decode(struct xrow_update_op *op, int index_base, "update operation name must be a string"); return -1; } - op->opcode = *mp_decode_str(expr, &len); - op->meta = xrow_update_op_by(op->opcode); + const char *opcode = mp_decode_str(expr, &len); + op->opcode = *opcode; + op->meta = xrow_update_op_by(opcode, len, op_num); if (op->meta == NULL) return -1; if (arg_count != op->meta->arg_count) { - diag_set(ClientError, ER_UNKNOWN_UPDATE_OP); + const char *str = tt_sprintf("wrong number of arguments, "\ + "expected %u, got %u", op->meta->arg_count, arg_count); + diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num, str); return -1; } int32_t field_no = 0; diff --git a/src/box/xrow_update_field.h b/src/box/xrow_update_field.h index e90095b9e..23a4ae517 100644 --- a/src/box/xrow_update_field.h +++ b/src/box/xrow_update_field.h @@ -197,7 +197,7 @@ struct xrow_update_op { * @retval -1 Client error. */ int -xrow_update_op_decode(struct xrow_update_op *op, int index_base, +xrow_update_op_decode(struct xrow_update_op *op, int op_num, int index_base, struct tuple_dictionary *dict, const char **expr); /* }}} xrow_update_op */ diff --git a/test/box/update.result b/test/box/update.result index c6a5a25a7..317a51aca 100644 --- a/test/box/update.result +++ b/test/box/update.result @@ -826,11 +826,13 @@ s:update({0}, {{}}) ... s:update({0}, {{'+'}}) --- -- error: Unknown UPDATE operation +- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got + 1' ... s:update({0}, {{'+', 0}}) --- -- error: Unknown UPDATE operation +- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got + 2' ... s:update({0}, {{'+', '+', '+'}}) --- diff --git a/test/engine/upsert.result b/test/engine/upsert.result index 47da307fa..5610c675d 100644 --- a/test/engine/upsert.result +++ b/test/engine/upsert.result @@ -1809,7 +1809,7 @@ space:upsert({1}, {{'!', 2, 100}}) -- must fail on checking tuple ... space:upsert({'a'}, {{'a', 2, 100}}) -- must fail on checking ops --- -- error: Unknown UPDATE operation +- error: 'Unknown UPDATE operation #1: "a"' ... space:upsert({'a'}, {{'!', 2, 'ups1'}}) -- 'fast' upsert via insert in one index --- @@ -2117,19 +2117,22 @@ test(t, {{'+', 3, 3}, {'&', 3, 'a'}}, {{1, '1', 1, '1'}}) ... test(t, {{'+', 3}, {'&', 3, 'a'}}, {{1, '1', 1, '1'}}) --- -- error: Unknown UPDATE operation +- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got + 2' ... test(t, {{':', 3, 3}}, {{1, '1', 1, '1'}}) --- -- error: Unknown UPDATE operation +- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 5, got + 3' ... test(t, {{':', 3, 3, 3}}, {{1, '1', 1, '1'}}) --- -- error: Unknown UPDATE operation +- error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 5, got + 4' ... test(t, {{'?', 3, 3}}, {{1, '1', 1, '1'}}) --- -- error: Unknown UPDATE operation +- error: 'Unknown UPDATE operation #1: "?"' ... 'dump ' .. anything_to_string(box.space.s:select{}) --- diff --git a/test/vinyl/gh.result b/test/vinyl/gh.result index 78cb2a28d..59578ebdd 100644 --- a/test/vinyl/gh.result +++ b/test/vinyl/gh.result @@ -369,7 +369,8 @@ s:select() ... s:upsert({1, 'test', 'failed'}, {{'=', 3, 33}, {'=', 4, nil}}) --- -- error: Unknown UPDATE operation +- error: 'Unknown UPDATE operation #2: wrong number of arguments, expected 3, got + 2' ... s:select() --- -- 2.21.0 (Apple Git-122.2)