Re: [Tarantool-patches] [PATCH v2 10/10] iproto: support error stacked diagnostic area

[Nikita Pettik <korablev@tarantool.org>]

On 01 Apr 16:26, Nikita Pettik wrote:
> @@ -1169,7 +1166,7 @@ xrow_decode_error(struct xrow_header *row)
>  		} else if (key == IPROTO_ERROR_STACK ) {
>  			if (mp_check_array(pos, end) != 0)
>  				goto error;
> -			if (iproto_decode_error_stack(&pos, end) != 0)
> +			if (iproto_decode_error_stack(&pos) != 0)
>  				continue;
>  		} else {
>  			mp_next(&pos);
> 
> > I don't see any tests on that. Are there some? Maybe at least
> > unit tests, in xrow.cc? Although a proper test would try to
> > send an error stack through applier. Because xrow_decode_error()
> > is used only there.
> 
> I'm a bit new to replication tests. I've been working on it.
> Could you please review the rest of changes so far?

Well, I've come up with following unit tests (and a bit more
fixes of xrow_decode_error()):

diff --git a/src/box/xrow.c b/src/box/xrow.c
index 596c765b7..f33226dc5 100644
--- a/src/box/xrow.c
+++ b/src/box/xrow.c
@@ -1094,8 +1094,7 @@ iproto_decode_error_stack(const char **pos)
 	 * box_error_add() does not replace previous errors.
 	 */
 	box_error_clear();
-	if (mp_typeof(**pos) != MP_ARRAY)
-		return -1;
+	assert(mp_typeof(**pos) == MP_ARRAY);
 	uint32_t stack_sz = mp_decode_array(pos);
 	for (uint32_t i = 0; i < stack_sz; i++) {
 		uint32_t code = 0;
@@ -1163,11 +1162,11 @@ xrow_decode_error(struct xrow_header *row)
 			const char *str = mp_decode_str(&pos, &len);
 			snprintf(error, sizeof(error), "%.*s", len, str);
 			box_error_set(__FILE__, __LINE__, code, error);
-		} else if (key == IPROTO_ERROR_STACK ) {
-			if (mp_check_array(pos, end) != 0)
+		} else if (key == IPROTO_ERROR_STACK) {
+			if (mp_typeof(*pos) != MP_ARRAY)
 				goto error;
 			if (iproto_decode_error_stack(&pos) != 0)
-				continue;
+				goto error;
 		} else {
 			mp_next(&pos);
 			continue;
diff --git a/test/unit/xrow.cc b/test/unit/xrow.cc
index 68a334239..954f22f16 100644
--- a/test/unit/xrow.cc
+++ b/test/unit/xrow.cc
@@ -241,6 +241,140 @@ test_xrow_header_encode_decode()
 	check_plan();
 }
 
+static char *
+error_stack_entry_encode(char *pos, const char *err_str)
+{
+	pos = mp_encode_map(pos, 2);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_CODE);
+	pos = mp_encode_uint(pos, 159);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
+	pos = mp_encode_str(pos, err_str, strlen(err_str));
+	return pos;
+}
+
+void
+test_xrow_error_stack_decode()
+{
+	plan(14);
+	char buffer[2048];
+	/*
+	 * To start with, let's test the simplest and obsolete
+	 * way of setting errors.
+	 */
+	char *pos = mp_encode_map(buffer, 1);
+	pos = mp_encode_uint(pos, IPROTO_ERROR);
+	pos = mp_encode_str(pos, "e1", strlen("e1"));
+
+	struct xrow_header header;
+	header.bodycnt = 666;
+	header.type = 159 | IPROTO_TYPE_ERROR;
+	header.body[0].iov_base = buffer;
+	header.body[0].iov_len = pos - buffer;
+
+	xrow_decode_error(&header);
+	struct error *last = diag_last_error(diag_get());
+	isnt(last, NULL, "xrow_decode succeed: diag has been set");
+	is(strcmp(last->errmsg, "e1"), 0,
+	   "xrow_decode succeed: error is parsed");
+
+	pos = mp_encode_map(buffer, 1);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
+	pos = mp_encode_array(pos, 2);
+	pos = error_stack_entry_encode(pos, "e1");
+	pos = error_stack_entry_encode(pos, "e2");
+	header.body[0].iov_base = buffer;
+	header.body[0].iov_len = pos - buffer;
+
+	diag_clear(diag_get());
+	xrow_decode_error(&header);
+	last = diag_last_error(diag_get());
+	isnt(last, NULL, "xrow_decode succeed: diag has been set");
+	is(strcmp(last->errmsg, "e2"), 0, "xrow_decode error stack suceed: "
+	   "e2 at the top of stack");
+	last = last->cause;
+	isnt(last, NULL, "xrow_decode succeed: 'cause' is present in stack")
+	is(strcmp(last->errmsg, "e1"), 0, "xrow_decode succeed: "
+	   "stack has been parsed");
+	/*
+	 * Let's try decode broken stack. Variations:
+	 * 1. Stack is not encoded as an array;
+	 * 2. Stack doesn't contain maps;
+	 * 3. Stack's map keys are not uints;
+	 * 4. Stack's map values have wrong types;
+	 * In all these cases diag_last should contain emty err.
+	 */
+	/* Stack is encoded as map. */
+	pos = mp_encode_map(buffer, 1);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
+	pos = mp_encode_map(pos, 1);
+	pos = error_stack_entry_encode(pos, "e1");
+	header.body[0].iov_base = buffer;
+	header.body[0].iov_len = pos - buffer;
+
+	diag_clear(diag_get());
+	xrow_decode_error(&header);
+	last = diag_last_error(diag_get());
+	isnt(last, NULL, "xrow_decode succeed: diag has been set");
+	is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: "
+	   "stack contains map instead of array");
+
+	/* Stack doesn't containt map(s) - array instead. */
+	pos = mp_encode_map(buffer, 1);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
+	pos = mp_encode_array(pos, 2);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_CODE);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
+	header.body[0].iov_base = buffer;
+	header.body[0].iov_len = pos - buffer;
+
+	diag_clear(diag_get());
+	xrow_decode_error(&header);
+	last = diag_last_error(diag_get());
+	isnt(last, NULL, "xrow_decode succeed: diag has been set");
+	is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: "
+	   "stack contains array values instead of maps");
+
+	/* Stack's map keys are not uints. */
+	pos = mp_encode_map(buffer, 1);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
+	pos = mp_encode_array(pos, 1);
+	pos = mp_encode_map(pos, 2);
+	pos = mp_encode_str(pos, "string instead of uint",
+			    strlen("string instead of uint"));
+	pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_CODE);
+	pos = mp_encode_uint(pos, 159);
+	header.body[0].iov_base = buffer;
+	header.body[0].iov_len = pos - buffer;
+
+	diag_clear(diag_get());
+	xrow_decode_error(&header);
+	last = diag_last_error(diag_get());
+	isnt(last, NULL, "xrow_decode succeed: diag has been set");
+	is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: "
+	   "stack's map keys are not uints");
+
+	/* Stack's map values have wrong types. */
+	pos = mp_encode_map(buffer, 1);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_STACK);
+	pos = mp_encode_array(pos, 1);
+	pos = mp_encode_map(pos, 2);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_CODE);
+	pos = mp_encode_uint(pos, 159);
+	pos = mp_encode_uint(pos, IPROTO_ERROR_MESSAGE);
+	pos = mp_encode_uint(pos, 666);
+	header.body[0].iov_base = buffer;
+	header.body[0].iov_len = pos - buffer;
+
+	diag_clear(diag_get());
+	xrow_decode_error(&header);
+	last = diag_last_error(diag_get());
+	isnt(last, NULL, "xrow_decode succeed: diag has been set");
+	is(strcmp(last->errmsg, ""), 0, "xrow_decode corrupted stack: "
+	   "stack's map wrong value type");
+	check_plan();
+}
+
 void
 test_request_str()
 {
@@ -279,13 +413,14 @@ main(void)
 {
 	memory_init();
 	fiber_init(fiber_c_invoke);
-	plan(3);
+	plan(4);
 
 	random_init();
 
 	test_iproto_constants();
 	test_greeting();
 	test_xrow_header_encode_decode();
+	test_xrow_error_stack_decode();
 	test_request_str();
 
 	random_free();
diff --git a/test/unit/xrow.result b/test/unit/xrow.result
index 5ee92ad7b..1d07915a4 100644
--- a/test/unit/xrow.result
+++ b/test/unit/xrow.result
@@ -1,4 +1,4 @@
-1..3
+1..4
     1..40
     ok 1 - round trip
     ok 2 - roundtrip.version_id
@@ -53,6 +53,22 @@ ok 1 - subtests
     ok 9 - decoded sync
     ok 10 - decoded bodycnt
 ok 2 - subtests
+    1..14
+    ok 1 - xrow_decode failed: no diag has been set
+    ok 2 - xrow_decode failed: wrong error is set
+    ok 3 - xrow_decode error stack failed: no diag has been set
+    ok 4 - xrow_decode error stack failed: e2 should come first
+    ok 5 - xrow_decode error stack failed: 'cause' is absent
+    ok 6 - xrow_decode error stack failed: wrong error is set
+    ok 7 - xrow_decode error stack failed: no diag has been set
+    ok 8 - xrow_decode corrupted stack: stack contains map instead of array
+    ok 9 - xrow_decode error stack failed: no diag has been set
+    ok 10 - xrow_decode corrupted stack: stack contains array values instead of maps
+    ok 11 - xrow_decode error stack failed: no diag has been set
+    ok 12 - xrow_decode corrupted stack: stack's map keys are not uints
+    ok 13 - xrow_decode error stack failed: no diag has been set
+    ok 14 - xrow_decode corrupted stack: stack's map wrong value type
+ok 3 - subtests
     1..1
     ok 1 - request_str
-ok 3 - subtests
+ok 4 - subtests