[Tarantool-patches] [PATCH v2] tuple: fix non-informative update() error message
Chris Sosnin
k.sosnin at tarantool.org
Thu Nov 28 15:28:15 MSK 2019
Hi! Thank you for your suggestions.
I am sorry for being impatient.
> 1. To not duplicate code, you can add a label, and make a
> goto to there from 'default'. Or vice versa - go to default
> from there.
I labeled the first return as error:
+error:
+ diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num,
+ tt_sprintf("\"%.*s\"", len, opcode));
+ return NULL;
+ }
...
default:
- diag_set(ClientError, ER_UNKNOWN_UPDATE_OP);
- return NULL;
+ goto error;
}
> 2. I would better assign it after xrow_update_op_by()
> returned not NULL. Because MessagePack strings are not
> zero terminated. So 'opcode' after mp_decode_str() may
> actually point at invalid memory instead of zero
> terminator in case of an empty string.
In this case we would still return with an error, however
I changed it to be your way:
+ const char *opcode = mp_decode_str(expr, &len);
+ op->meta = xrow_update_op_by(opcode, len, op_num);
if (op->meta == NULL)
return -1;
+ op->opcode = *opcode;
Also I fixed coding style and extra white space.
And finally
> 5. You added an argument, but didn't update the comment.
+ * @param op_num Ordinal number of the opearion
Commit message and the patch itself are below.
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
---
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-3884-update-error
issue: https://github.com/tarantool/tarantool/issues/3884
src/box/errcode.h | 2 +-
src/box/xrow_update.c | 4 ++--
src/box/xrow_update_field.c | 25 +++++++++++++++++--------
src/box/xrow_update_field.h | 3 ++-
test/box/update.result | 6 ++++--
test/engine/upsert.result | 13 ++++++++-----
test/vinyl/gh.result | 3 ++-
7 files changed, 36 insertions(+), 20 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..b5a68d24c 100644
--- a/src/box/xrow_update_field.c
+++ b/src/box/xrow_update_field.c
@@ -576,9 +576,15 @@ 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) {
+error:
+ 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 +601,12 @@ xrow_update_op_by(char opcode)
case '!':
return &op_insert;
default:
- diag_set(ClientError, ER_UNKNOWN_UPDATE_OP);
- return NULL;
+ goto 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)
{
if (mp_typeof(**expr) != MP_ARRAY) {
@@ -620,12 +625,16 @@ 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->meta = xrow_update_op_by(opcode, len, op_num);
if (op->meta == NULL)
return -1;
+ op->opcode = *opcode;
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..e87bd6fab 100644
--- a/src/box/xrow_update_field.h
+++ b/src/box/xrow_update_field.h
@@ -189,6 +189,7 @@ struct xrow_update_op {
/**
* Decode an update operation from MessagePack.
* @param[out] op Update operation.
+ * @param op_num Ordinal number of the opearion
* @param index_base Field numbers base: 0 or 1.
* @param dict Dictionary to lookup field number by a name.
* @param expr MessagePack.
@@ -197,7 +198,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)
More information about the Tarantool-patches
mailing list