[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