[Tarantool-patches] [PATCH] iproto: add an empty body to the unprepare response

[Tarantool-patches] [PATCH] iproto: add an empty body to the unprepare response

[Chris Sosnin]

Absence of the body in the unprepare response forces users to perform
additional checks to avoid errors. Adding an empty body fixes this problem.

Closes #4769
---
branch: https://github.com/tarantool/tarantool/commit/970bbec99903c9482b8d3b85040617c4f46a650e
issue: https://github.com/tarantool/tarantool/issues/4769

Yes, I was told that writing tests in python is kind of deprecated thing,
however this test fits here quite well and makes patch much more concise.

 src/box/iproto.cc          | 17 ++++++++++-------
 test/box-py/iproto.test.py | 14 ++++++++++++++
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index f313af6ae..d5891391d 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1781,20 +1781,23 @@ tx_process_sql(struct cmsg *m)
 	 */
 	out = msg->connection->tx.p_obuf;
 	struct obuf_svp header_svp;
+	if (is_unprepare) {
+		if (iproto_reply_ok(out, msg->header.sync, schema_version) != 0)
+			goto error;
+		iproto_wpos_create(&msg->wpos, out);
+		return;
+	}
 	/* Prepare memory for the iproto header. */
 	if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) {
 		port_destroy(&port);
 		goto error;
 	}
-	/* Nothing to dump in case of UNPREPARE request. */
-	if (!is_unprepare) {
-		if (port_dump_msgpack(&port, out) != 0) {
-			port_destroy(&port);
-			obuf_rollback_to_svp(out, &header_svp);
-			goto error;
-		}
+	if (port_dump_msgpack(&port, out) != 0) {
 		port_destroy(&port);
+		obuf_rollback_to_svp(out, &header_svp);
+		goto error;
 	}
+	port_destroy(&port);
 	iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version);
 	iproto_wpos_create(&msg->wpos, out);
 	return;
diff --git a/test/box-py/iproto.test.py b/test/box-py/iproto.test.py
index 77637d8ed..101e5e913 100644
--- a/test/box-py/iproto.test.py
+++ b/test/box-py/iproto.test.py
@@ -286,6 +286,20 @@ resp = test_request(header, body)
 print 'Schema changed -> error:', resp['header'][0] != 0
 print 'Got another schema_id:', resp['header'][5] != schema_id
 
+#
+# gh-4769 Unprepare response must have a body.
+#
+IPROTO_PREPARE = 13
+IPROTO_SQL_TEXT = 0x40
+IPROTO_STMT_ID = 0x43
+
+header = { IPROTO_CODE : IPROTO_PREPARE }
+body = { IPROTO_SQL_TEXT : 'SELECT 1' }
+resp = test_request(header, body)
+
+body = { IPROTO_STMT_ID : resp['body'][IPROTO_STMT_ID] }
+resp = test_request(header, body)
+
 #
 # gh-2334 Lost SYNC in JOIN response.
 #
-- 
2.21.1 (Apple Git-122.3)

Re: [Tarantool-patches] [PATCH] iproto: add an empty body to the unprepare response

[Nikita Pettik]

On 02 Mar 19:37, Chris Sosnin wrote:
> Yes, I was told that writing tests in python is kind of deprecated thing,
> however this test fits here quite well and makes patch much more concise.

Alternatively, you can introduce box/iproto.test.lua which will
be used for 'raw' iproto requests handling (e.g. to test raw iproto
requests containing new diagnostics area:
https://github.com/tarantool/tarantool/commit/5779ae4b562df18e4ec2e6c8821441d0d507f8f4#diff-af67795b74dc02a6ae1eab64b8fa920cR12).
As a reference you can look at gh-4077-iproto-execute-no-binds.test.lua

LGTM otherwise.
 
> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index f313af6ae..d5891391d 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1781,20 +1781,23 @@ tx_process_sql(struct cmsg *m)
>  	 */
>  	out = msg->connection->tx.p_obuf;
>  	struct obuf_svp header_svp;
> +	if (is_unprepare) {
> +		if (iproto_reply_ok(out, msg->header.sync, schema_version) != 0)
> +			goto error;
> +		iproto_wpos_create(&msg->wpos, out);
> +		return;
> +	}
>  	/* Prepare memory for the iproto header. */
>  	if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) {
>  		port_destroy(&port);
>  		goto error;
>  	}
> -	/* Nothing to dump in case of UNPREPARE request. */
> -	if (!is_unprepare) {
> -		if (port_dump_msgpack(&port, out) != 0) {
> -			port_destroy(&port);
> -			obuf_rollback_to_svp(out, &header_svp);
> -			goto error;
> -		}
> +	if (port_dump_msgpack(&port, out) != 0) {
>  		port_destroy(&port);
> +		obuf_rollback_to_svp(out, &header_svp);
> +		goto error;
>  	}
> +	port_destroy(&port);
>  	iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version);
>  	iproto_wpos_create(&msg->wpos, out);
>  	return;
> diff --git a/test/box-py/iproto.test.py b/test/box-py/iproto.test.py
> index 77637d8ed..101e5e913 100644
> --- a/test/box-py/iproto.test.py
> +++ b/test/box-py/iproto.test.py
> @@ -286,6 +286,20 @@ resp = test_request(header, body)
>  print 'Schema changed -> error:', resp['header'][0] != 0
>  print 'Got another schema_id:', resp['header'][5] != schema_id
>  
> +#
> +# gh-4769 Unprepare response must have a body.
> +#
> +IPROTO_PREPARE = 13
> +IPROTO_SQL_TEXT = 0x40
> +IPROTO_STMT_ID = 0x43
> +
> +header = { IPROTO_CODE : IPROTO_PREPARE }
> +body = { IPROTO_SQL_TEXT : 'SELECT 1' }
> +resp = test_request(header, body)
> +
> +body = { IPROTO_STMT_ID : resp['body'][IPROTO_STMT_ID] }
> +resp = test_request(header, body)
> +
>  #
>  # gh-2334 Lost SYNC in JOIN response.
>  #
> -- 
> 2.21.1 (Apple Git-122.3)
> 

Re: [Tarantool-patches] [PATCH] iproto: add an empty body to the unprepare response

[Chris Sosnin]

Hi! Thank you for the review!

> 2 марта 2020 г., в 22:18, Nikita Pettik <korablev@tarantool.org> написал(а):
> 
> On 02 Mar 19:37, Chris Sosnin wrote:
>> Yes, I was told that writing tests in python is kind of deprecated thing,
>> however this test fits here quite well and makes patch much more concise.
> 
> Alternatively, you can introduce box/iproto.test.lua which will
> be used for 'raw' iproto requests handling (e.g. to test raw iproto
> requests containing new diagnostics area:
> https://github.com/tarantool/tarantool/commit/5779ae4b562df18e4ec2e6c8821441d0d507f8f4#diff-af67795b74dc02a6ae1eab64b8fa920cR12).
> As a reference you can look at gh-4077-iproto-execute-no-binds.test.lua

This seems reasonable. Thanks, I will rework it and send a new version.


> LGTM otherwise.
> 
>> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
>> index f313af6ae..d5891391d 100644
>> --- a/src/box/iproto.cc
>> +++ b/src/box/iproto.cc
>> @@ -1781,20 +1781,23 @@ tx_process_sql(struct cmsg *m)
>> 	 */
>> 	out = msg->connection->tx.p_obuf;
>> 	struct obuf_svp header_svp;
>> +	if (is_unprepare) {
>> +		if (iproto_reply_ok(out, msg->header.sync, schema_version) != 0)
>> +			goto error;
>> +		iproto_wpos_create(&msg->wpos, out);
>> +		return;
>> +	}
>> 	/* Prepare memory for the iproto header. */
>> 	if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) {
>> 		port_destroy(&port);
>> 		goto error;
>> 	}
>> -	/* Nothing to dump in case of UNPREPARE request. */
>> -	if (!is_unprepare) {
>> -		if (port_dump_msgpack(&port, out) != 0) {
>> -			port_destroy(&port);
>> -			obuf_rollback_to_svp(out, &header_svp);
>> -			goto error;
>> -		}
>> +	if (port_dump_msgpack(&port, out) != 0) {
>> 		port_destroy(&port);
>> +		obuf_rollback_to_svp(out, &header_svp);
>> +		goto error;
>> 	}
>> +	port_destroy(&port);
>> 	iproto_reply_sql(out, &header_svp, msg->header.sync, schema_version);
>> 	iproto_wpos_create(&msg->wpos, out);
>> 	return;
>> diff --git a/test/box-py/iproto.test.py b/test/box-py/iproto.test.py
>> index 77637d8ed..101e5e913 100644
>> --- a/test/box-py/iproto.test.py
>> +++ b/test/box-py/iproto.test.py
>> @@ -286,6 +286,20 @@ resp = test_request(header, body)
>> print 'Schema changed -> error:', resp['header'][0] != 0
>> print 'Got another schema_id:', resp['header'][5] != schema_id
>> 
>> +#
>> +# gh-4769 Unprepare response must have a body.
>> +#
>> +IPROTO_PREPARE = 13
>> +IPROTO_SQL_TEXT = 0x40
>> +IPROTO_STMT_ID = 0x43
>> +
>> +header = { IPROTO_CODE : IPROTO_PREPARE }
>> +body = { IPROTO_SQL_TEXT : 'SELECT 1' }
>> +resp = test_request(header, body)
>> +
>> +body = { IPROTO_STMT_ID : resp['body'][IPROTO_STMT_ID] }
>> +resp = test_request(header, body)
>> +
>> #
>> # gh-2334 Lost SYNC in JOIN response.
>> #
>> -- 
>> 2.21.1 (Apple Git-122.3)
>>