[Tarantool-patches] [PATCH 0/2] tuple: fixes for update()

[Tarantool-patches] [PATCH 0/2] tuple: fixes for update()

[Chris Sosnin]

As Nikita suggested, I split the initial patch into the following two.
It already has LGTM from Vlad.

branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-3884-update-error
issue: https://github.com/tarantool/tarantool/issues/3884

Chris Sosnin (2):
  tuple: fix non-informative update() error message
  tuple: add argument length check for update()

 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 ++-
 ...1-access-settings-from-any-frontend.result |  9 ++++---
 test/box/update.result                        | 18 +++++++++++--
 test/box/update.test.lua                      |  3 +++
 test/engine/upsert.result                     | 13 ++++++----
 test/vinyl/gh.result                          |  3 ++-
 9 files changed, 57 insertions(+), 23 deletions(-)

-- 
2.21.0 (Apple Git-122.2)

[Tarantool-patches] [PATCH 1/2] tuple: fix non-informative update() error message

[Chris Sosnin]

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, or poiniting out at invalid operation code.

Fixes #3884
---
 src/box/errcode.h                              |  2 +-
 src/box/xrow_update.c                          |  4 ++--
 src/box/xrow_update_field.c                    | 18 ++++++++++++------
 src/box/xrow_update_field.h                    |  3 ++-
 ...11-access-settings-from-any-frontend.result |  9 ++++++---
 test/box/update.result                         |  6 ++++--
 test/engine/upsert.result                      | 13 ++++++++-----
 test/vinyl/gh.result                           |  3 ++-
 8 files changed, 37 insertions(+), 21 deletions(-)

diff --git a/src/box/errcode.h b/src/box/errcode.h
index 3065b1948..6f6e28c6c 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 0cee50017..e7a98073a 100644
--- a/src/box/xrow_update.c
+++ b/src/box/xrow_update.c
@@ -177,8 +177,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 7debc1930..deee91738 100644
--- a/src/box/xrow_update_field.c
+++ b/src/box/xrow_update_field.c
@@ -595,7 +595,7 @@ 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(char opcode, int op_num)
 {
 	switch (opcode) {
 	case '=':
@@ -614,9 +614,12 @@ xrow_update_op_by(char opcode)
 	case '!':
 		return &op_insert;
 	default:
-		diag_set(ClientError, ER_UNKNOWN_UPDATE_OP);
-		return NULL;
+		goto error;
 	}
+error:
+	diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num,
+		 tt_sprintf("\"%c\"", opcode));
+	return NULL;
 }
 
 int
@@ -637,7 +640,7 @@ xrow_update_op_next_token(struct xrow_update_op *op)
 }
 
 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) {
@@ -657,11 +660,14 @@ xrow_update_op_decode(struct xrow_update_op *op, int index_base,
 		return -1;
 	}
 	op->opcode = *mp_decode_str(expr, &len);
-	op->meta = xrow_update_op_by(op->opcode);
+	op->meta = xrow_update_op_by(op->opcode, 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;
 	}
 	/*
diff --git a/src/box/xrow_update_field.h b/src/box/xrow_update_field.h
index 387c26566..451a23024 100644
--- a/src/box/xrow_update_field.h
+++ b/src/box/xrow_update_field.h
@@ -223,6 +223,7 @@ xrow_update_op_next_token(struct xrow_update_op *op);
 /**
  * Decode an update operation from MessagePack.
  * @param[out] op Update operation.
+ * @param op_num Ordinal number of the operation.
  * @param index_base Field numbers base: 0 or 1.
  * @param dict Dictionary to lookup field number by a name.
  * @param expr MessagePack.
@@ -231,7 +232,7 @@ xrow_update_op_next_token(struct xrow_update_op *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);
 
 /**
diff --git a/test/box/gh-4511-access-settings-from-any-frontend.result b/test/box/gh-4511-access-settings-from-any-frontend.result
index 64532a1ba..ed340d053 100644
--- a/test/box/gh-4511-access-settings-from-any-frontend.result
+++ b/test/box/gh-4511-access-settings-from-any-frontend.result
@@ -196,15 +196,18 @@ s:update('sql_defer_foreign_keys', {{}})
  | ...
 s:update('sql_defer_foreign_keys', {{'='}})
  | ---
- | - error: Unknown UPDATE operation
+ | - error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got
+ |     1'
  | ...
 s:update('sql_defer_foreign_keys', {{'=', 'value'}})
  | ---
- | - error: Unknown UPDATE operation
+ | - error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got
+ |     2'
  | ...
 s:update('sql_defer_foreign_keys', {{'=', 'value', true, 1}})
  | ---
- | - error: Unknown UPDATE operation
+ | - error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got
+ |     4'
  | ...
 
 s:update('sql_defer_foreign_keys', {{'+', 'value', 2}})
diff --git a/test/box/update.result b/test/box/update.result
index d1dc06eae..7fc0b02c6 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)

[Tarantool-patches] [PATCH 2/2] tuple: add argument length check for update()

[Chris Sosnin]

Currently tuple_object:update() does not check the length
of operation string and just takes the first character
after decoding. This patch fixes this problem.

Follow-up #3884
---
 src/box/xrow_update_field.c | 13 ++++++++-----
 test/box/update.result      | 12 ++++++++++++
 test/box/update.test.lua    |  3 +++
 3 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/src/box/xrow_update_field.c b/src/box/xrow_update_field.c
index deee91738..7c0f5fb5e 100644
--- a/src/box/xrow_update_field.c
+++ b/src/box/xrow_update_field.c
@@ -595,9 +595,11 @@ static const struct xrow_update_op_meta op_delete = {
 };
 
 static inline const struct xrow_update_op_meta *
-xrow_update_op_by(char opcode, int op_num)
+xrow_update_op_by(const char *opcode, uint32_t len, int op_num)
 {
-	switch (opcode) {
+	if (len != 1)
+		goto error;
+	switch (*opcode) {
 	case '=':
 		return &op_set;
 	case '+':
@@ -618,7 +620,7 @@ xrow_update_op_by(char opcode, int op_num)
 	}
 error:
 	diag_set(ClientError, ER_UNKNOWN_UPDATE_OP, op_num,
-		 tt_sprintf("\"%c\"", opcode));
+		 tt_sprintf("\"%.*s\"", len, opcode));
 	return NULL;
 }
 
@@ -659,10 +661,11 @@ xrow_update_op_decode(struct xrow_update_op *op, int op_num, 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, op_num);
+	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) {
 		const char *str = tt_sprintf("wrong number of arguments, "\
 					     "expected %u, got %u",
diff --git a/test/box/update.result b/test/box/update.result
index 7fc0b02c6..28ba47831 100644
--- a/test/box/update.result
+++ b/test/box/update.result
@@ -834,6 +834,18 @@ s:update({0}, {{'+', 0}})
 - error: 'Unknown UPDATE operation #1: wrong number of arguments, expected 3, got
     2'
 ...
+s:update({0}, {{'', 2, 1}})
+---
+- error: 'Unknown UPDATE operation #1: ""'
+...
+s:update({0}, {{'more than 1 character', 2, 1}})
+---
+- error: 'Unknown UPDATE operation #1: "more than 1 character"'
+...
+s:update({0}, {{'same as previous'}})
+---
+- error: 'Unknown UPDATE operation #1: "same as previous"'
+...
 s:update({0}, {{'+', '+', '+'}})
 ---
 - error: 'Field ''+'' UPDATE error: invalid JSON in position 1'
diff --git a/test/box/update.test.lua b/test/box/update.test.lua
index 6e8dbd642..314ebef05 100644
--- a/test/box/update.test.lua
+++ b/test/box/update.test.lua
@@ -252,6 +252,9 @@ s:update({0}, {'+', 2, 2})
 s:update({0}, {{}})
 s:update({0}, {{'+'}})
 s:update({0}, {{'+', 0}})
+s:update({0}, {{'', 2, 1}})
+s:update({0}, {{'more than 1 character', 2, 1}})
+s:update({0}, {{'same as previous'}})
 s:update({0}, {{'+', '+', '+'}})
 s:update({0}, {{0, 0, 0}})
 
-- 
2.21.0 (Apple Git-122.2)

Re: [Tarantool-patches] [PATCH 0/2] tuple: fixes for update()

[Kirill Yukhin]

Hello,

On 10 янв 10:28, Chris Sosnin wrote:
> As Nikita suggested, I split the initial patch into the following two.
> It already has LGTM from Vlad.
> 
> branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-3884-update-error
> issue: https://github.com/tarantool/tarantool/issues/3884
> 
> Chris Sosnin (2):
>   tuple: fix non-informative update() error message
>   tuple: add argument length check for update()

I've checked your patch into master.

--
Regards, Kirill Yukhin