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

[Tarantool-patches] [PATCH v2] 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/tree/ksosnin/gh-4769-unprepare-response-body
issue: https://github.com/tarantool/tarantool/issues/4769

As Nikita suggested, I created box/iproto.test.lua, and basically
inserted wrappers for requests testing from box-py for future usage.

 src/box/iproto.cc        | 17 ++++----
 test/box/iproto.result   | 86 ++++++++++++++++++++++++++++++++++++++++
 test/box/iproto.test.lua | 54 +++++++++++++++++++++++++
 3 files changed, 150 insertions(+), 7 deletions(-)
 create mode 100644 test/box/iproto.result
 create mode 100644 test/box/iproto.test.lua

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/iproto.result b/test/box/iproto.result
new file mode 100644
index 000000000..457f88493
--- /dev/null
+++ b/test/box/iproto.result
@@ -0,0 +1,86 @@
+-- test-run result file version 2
+test_run = require('test_run').new()
+ | ---
+ | ...
+test_run:cmd("setopt delimiter ';'")
+ | ---
+ | - true
+ | ...
+
+net_box = require('net.box')
+msgpack = require('msgpack')
+urilib = require('uri')
+
+IPROTO_REQUEST_TYPE   = 0x00
+IPROTO_PREPARE        = 13
+IPROTO_SQL_TEXT       = 0x40
+IPROTO_STMT_ID        = 0x43
+
+function recieve_response(socket)
+    local size = socket:read(5)
+    assert(size ~= nil, 'Failed to read response')
+    size = msgpack.decode(size)
+    local response = socket:read(size)
+    local header, header_len = msgpack.decode(response)
+    local body = msgpack.decode(response:sub(header_len))
+    return {
+        ['header'] = header,
+        ['body'] = body
+    }
+end
+
+function test_request(socket, query_header, query_body)
+    local header = msgpack.encode(query_header)
+    local body = msgpack.encode(query_body)
+    local size = msgpack.encode(header:len() + body:len())
+    assert(socket:write(size .. header .. body) ~= nil,
+           'Failed to send request')
+    return recieve_response(socket)
+end
+
+test_run:cmd("setopt delimiter ''");
+ | ---
+ | ...
+
+box.schema.user.grant('guest', 'read, write, execute', 'universe')
+ | ---
+ | ...
+uri = urilib.parse(box.cfg.listen)
+ | ---
+ | ...
+socket = net_box.establish_connection(uri.host, uri.service)
+ | ---
+ | ...
+
+--
+-- gh-4769: Unprepare response must have a body.
+--
+header = { [IPROTO_REQUEST_TYPE] = IPROTO_PREPARE }
+ | ---
+ | ...
+body = { [IPROTO_SQL_TEXT] = 'SELECT 1' }
+ | ---
+ | ...
+response = test_request(socket, header, body)
+ | ---
+ | ...
+
+body = { [IPROTO_STMT_ID] = response['body'][IPROTO_STMT_ID] }
+ | ---
+ | ...
+response = test_request(socket, header, body)
+ | ---
+ | ...
+response.body
+ | ---
+ | - {}
+ | ...
+
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+ | ---
+ | ...
+socket:close()
+ | ---
+ | - true
+ | ...
+
diff --git a/test/box/iproto.test.lua b/test/box/iproto.test.lua
new file mode 100644
index 000000000..5ade33339
--- /dev/null
+++ b/test/box/iproto.test.lua
@@ -0,0 +1,54 @@
+test_run = require('test_run').new()
+test_run:cmd("setopt delimiter ';'")
+
+net_box = require('net.box')
+msgpack = require('msgpack')
+urilib = require('uri')
+
+IPROTO_REQUEST_TYPE   = 0x00
+IPROTO_PREPARE        = 13
+IPROTO_SQL_TEXT       = 0x40
+IPROTO_STMT_ID        = 0x43
+
+function recieve_response(socket)
+    local size = socket:read(5)
+    assert(size ~= nil, 'Failed to read response')
+    size = msgpack.decode(size)
+    local response = socket:read(size)
+    local header, header_len = msgpack.decode(response)
+    local body = msgpack.decode(response:sub(header_len))
+    return {
+        ['header'] = header,
+        ['body'] = body
+    }
+end
+
+function test_request(socket, query_header, query_body)
+    local header = msgpack.encode(query_header)
+    local body = msgpack.encode(query_body)
+    local size = msgpack.encode(header:len() + body:len())
+    assert(socket:write(size .. header .. body) ~= nil,
+           'Failed to send request')
+    return recieve_response(socket)
+end
+
+test_run:cmd("setopt delimiter ''");
+
+box.schema.user.grant('guest', 'read, write, execute', 'universe')
+uri = urilib.parse(box.cfg.listen)
+socket = net_box.establish_connection(uri.host, uri.service)
+
+--
+-- gh-4769: Unprepare response must have a body.
+--
+header = { [IPROTO_REQUEST_TYPE] = IPROTO_PREPARE }
+body = { [IPROTO_SQL_TEXT] = 'SELECT 1' }
+response = test_request(socket, header, body)
+
+body = { [IPROTO_STMT_ID] = response['body'][IPROTO_STMT_ID] }
+response = test_request(socket, header, body)
+response.body
+
+box.schema.user.revoke('guest', 'read,write,execute', 'universe')
+socket:close()
+
-- 
2.21.1 (Apple Git-122.3)

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

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

On 03/03/2020 17:16, Chris Sosnin wrote:
> 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/tree/ksosnin/gh-4769-unprepare-response-body
> issue: https://github.com/tarantool/tarantool/issues/4769
> 
> As Nikita suggested, I created box/iproto.test.lua, and basically
> inserted wrappers for requests testing from box-py for future usage.
> 
>  src/box/iproto.cc        | 17 ++++----
>  test/box/iproto.result   | 86 ++++++++++++++++++++++++++++++++++++++++
>  test/box/iproto.test.lua | 54 +++++++++++++++++++++++++
>  3 files changed, 150 insertions(+), 7 deletions(-)
>  create mode 100644 test/box/iproto.result
>  create mode 100644 test/box/iproto.test.lua
> 
> 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) {

Looks like you don't need that flag anymore. Consider the diff:

====================
diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index d5891391d..28095f5b8 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1716,10 +1716,9 @@ tx_process_sql(struct cmsg *m)
 	struct obuf *out;
 	struct port port;
 	struct sql_bind *bind = NULL;
-	int bind_count = 0;
+	int rc, bind_count = 0;
 	const char *sql;
 	uint32_t len;
-	bool is_unprepare = false;
 
 	if (tx_check_schema(msg->header.schema_version))
 		goto error;
@@ -1771,7 +1770,12 @@ tx_process_sql(struct cmsg *m)
 			uint32_t stmt_id = mp_decode_uint(&sql);
 			if (sql_unprepare(stmt_id) != 0)
 				goto error;
-			is_unprepare = true;
+			out = msg->connection->tx.p_obuf;
+			if (iproto_reply_ok(out, msg->header.sync,
+					    schema_version) != 0)
+				goto error;
+			iproto_wpos_create(&msg->wpos, out);
+			return;
 		}
 	}
 
@@ -1781,23 +1785,17 @@ 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;
 	}
-	if (port_dump_msgpack(&port, out) != 0) {
-		port_destroy(&port);
+	rc = port_dump_msgpack(&port, out);
+	port_destroy(&port);
+	if (rc != 0) {
 		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;

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

[Chris Sosnin]

[-- Attachment #1: Type: text/plain, Size: 4147 bytes --]

Thank you for the review!

> 4 марта 2020 г., в 01:33, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> написал(а):
> 
> Hi! Thanks for the patch!
> 
> On 03/03/2020 17:16, Chris Sosnin wrote:
>> 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/tree/ksosnin/gh-4769-unprepare-response-body
>> issue: https://github.com/tarantool/tarantool/issues/4769
>> 
>> As Nikita suggested, I created box/iproto.test.lua, and basically
>> inserted wrappers for requests testing from box-py for future usage.
>> 
>> src/box/iproto.cc        | 17 ++++----
>> test/box/iproto.result   | 86 ++++++++++++++++++++++++++++++++++++++++
>> test/box/iproto.test.lua | 54 +++++++++++++++++++++++++
>> 3 files changed, 150 insertions(+), 7 deletions(-)
>> create mode 100644 test/box/iproto.result
>> create mode 100644 test/box/iproto.test.lua
>> 
>> 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) {
> 
> Looks like you don't need that flag anymore. Consider the diff:
> 
> ====================
> diff --git a/src/box/iproto.cc <http://iproto.cc/> b/src/box/iproto.cc <http://iproto.cc/>
> index d5891391d..28095f5b8 100644
> --- a/src/box/iproto.cc <http://iproto.cc/>
> +++ b/src/box/iproto.cc <http://iproto.cc/>
> @@ -1716,10 +1716,9 @@ tx_process_sql(struct cmsg *m)
> 	struct obuf *out;
> 	struct port port;
> 	struct sql_bind *bind = NULL;
> -	int bind_count = 0;
> +	int rc, bind_count = 0;
> 	const char *sql;
> 	uint32_t len;
> -	bool is_unprepare = false;
> 
> 	if (tx_check_schema(msg->header.schema_version))
> 		goto error;
> @@ -1771,7 +1770,12 @@ tx_process_sql(struct cmsg *m)
> 			uint32_t stmt_id = mp_decode_uint(&sql);
> 			if (sql_unprepare(stmt_id) != 0)
> 				goto error;
> -			is_unprepare = true;
> +			out = msg->connection->tx.p_obuf;
> +			if (iproto_reply_ok(out, msg->header.sync,
> +					    schema_version) != 0)
> +				goto error;
> +			iproto_wpos_create(&msg->wpos, out);
> +			return;
> 		}
> 	}
> 
> @@ -1781,23 +1785,17 @@ 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;
> 	}
> -	if (port_dump_msgpack(&port, out) != 0) {
> -		port_destroy(&port);
> +	rc = port_dump_msgpack(&port, out);
> +	port_destroy(&port);
> +	if (rc != 0) {
> 		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;

The change with rc and avoiding code duplication is more about refactoring, should we apply
it here? As for the flag, the current code is well separated, all switch-cases perform the execution, 
everything after is about preparing the message to send.

Maybe it’s only me, but it looks cleaner without this diff, not to mention it produces the same code.
If you (or someone else) insist, however, I will apply it and resend.

I added one small change to the branch:
        out = msg->connection->tx.p_obuf;
-       struct obuf_svp header_svp;
        if (is_unprepare) {
	...
	}
+       struct obuf_svp header_svp;
        /* Prepare memory for the iproto header. */
        if (iproto_prepare_header(out, &header_svp, IPROTO_HEADER_LEN) != 0) {

This variable is not needed in case of unprepare, so we could declare it later.


[-- Attachment #2: Type: text/html, Size: 92978 bytes --]

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

[Vladislav Shpilevoy]

>>> diff --git a/src/box/iproto.cc <http://iproto.cc> b/src/box/iproto.cc <http://iproto.cc>
>>> index f313af6ae..d5891391d 100644
>>> --- a/src/box/iproto.cc <http://iproto.cc>
>>> +++ b/src/box/iproto.cc <http://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) {
>>
>> Looks like you don't need that flag anymore. Consider the diff:
>>
>> ====================
>> diff --git a/src/box/iproto.cc <http://iproto.cc/> b/src/box/iproto.cc <http://iproto.cc/>
>> index d5891391d..28095f5b8 100644
>> --- a/src/box/iproto.cc <http://iproto.cc/>
>> +++ b/src/box/iproto.cc <http://iproto.cc/>
>> @@ -1716,10 +1716,9 @@ tx_process_sql(struct cmsg *m)
>> struct obuf *out;
>> struct port port;
>> struct sql_bind *bind = NULL;
>> -int bind_count = 0;
>> +int rc, bind_count = 0;
>> const char *sql;
>> uint32_t len;
>> -bool is_unprepare = false;
>>
>> if (tx_check_schema(msg->header.schema_version))
>> goto error;
>> @@ -1771,7 +1770,12 @@ tx_process_sql(struct cmsg *m)
>> uint32_t stmt_id = mp_decode_uint(&sql);
>> if (sql_unprepare(stmt_id) != 0)
>> goto error;
>> -is_unprepare = true;
>> +out = msg->connection->tx.p_obuf;
>> +if (iproto_reply_ok(out, msg->header.sync,
>> +    schema_version) != 0)
>> +goto error;
>> +iproto_wpos_create(&msg->wpos, out);
>> +return;
>> }
>> }
>>
>> @@ -1781,23 +1785,17 @@ 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;
>> }
>> -if (port_dump_msgpack(&port, out) != 0) {
>> -port_destroy(&port);
>> +rc = port_dump_msgpack(&port, out);
>> +port_destroy(&port);
>> +if (rc != 0) {
>> 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;
> 
> The change with rc and avoiding code duplication is more about refactoring, should we apply
> it here?

I changed 'rc' and port_destroy(), because you changed the same lines. So
my diff does not make the final patch bigger and touches only lines which
would be changed anyway, except 'rc' declaration. I couldn't declare it in
place, because the compiler complained about 'rc' not declared for 'end'
label, if jumped from unprepare execution.

> As for the flag, the current code is well separated, all switch-cases perform the execution, 
> everything after is about preparing the message to send.
> 
> Maybe it’s only me, but it looks cleaner without this diff, not to mention it produces the same code.

Did you check assembler?

> If you (or someone else) insist, however, I will apply it and resend.

I don't insist. You can keep it as is.

LGTM.

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

[Kirill Yukhin]

Hello,

On 03 мар 19:16, Chris Sosnin wrote:
> 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/tree/ksosnin/gh-4769-unprepare-response-body
> issue: https://github.com/tarantool/tarantool/issues/4769
> 
> As Nikita suggested, I created box/iproto.test.lua, and basically
> inserted wrappers for requests testing from box-py for future usage.

Could you please rename the test to be not so generic?
Like box/gh-4769-iproto-unprep-body or whatever.

--
Regards, Kirill Yukhin

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

[Nikita Pettik]

On 05 Mar 08:41, Kirill Yukhin wrote:
> Hello,
> 
> On 03 мар 19:16, Chris Sosnin wrote:
> > 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/tree/ksosnin/gh-4769-unprepare-response-body
> > issue: https://github.com/tarantool/tarantool/issues/4769
> > 
> > As Nikita suggested, I created box/iproto.test.lua, and basically
> > inserted wrappers for requests testing from box-py for future usage.
> 
> Could you please rename the test to be not so generic?
> Like box/gh-4769-iproto-unprep-body or whatever.

Kirill, this test is going to assemble all iproto-related tests
which don't rely on net.box module. Setting up all preparations
required for raw iproto communication results in duplicating ~30-40
lines of code in each test file.
 
> --
> Regards, Kirill Yukhin

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

[Alexander Turenko]

On Thu, Mar 05, 2020 at 08:41:35AM +0000, Nikita Pettik wrote:
> On 05 Mar 08:41, Kirill Yukhin wrote:
> > Hello,
> > 
> > On 03 мар 19:16, Chris Sosnin wrote:
> > > 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/tree/ksosnin/gh-4769-unprepare-response-body
> > > issue: https://github.com/tarantool/tarantool/issues/4769
> > > 
> > > As Nikita suggested, I created box/iproto.test.lua, and basically
> > > inserted wrappers for requests testing from box-py for future usage.
> > 
> > Could you please rename the test to be not so generic?
> > Like box/gh-4769-iproto-unprep-body or whatever.
> 
> Kirill, this test is going to assemble all iproto-related tests
> which don't rely on net.box module. Setting up all preparations
> required for raw iproto communication results in duplicating ~30-40
> lines of code in each test file.

Technically there are two ways to extract helpers from a 'core =
tarantool' test:

* Add it to, say, test/box/box.lua and to _G.protected_globals.
* Add it to a separate Lua file in test/box/lua and to 'lua_libs' field
  in test/box/suite.ini. After this you can use `require` for this
  module in a test.

So technically you're not blocked here. Both ways are available and
don't lead to much code duplication, but the process (SOP) requires to
add a test for a bug to a separate file. (Personally I still don't sure
it is good, but anyway.)

NB: 'receive', not 'recieve'. Very often typo.

WBR, Alexander Turenko.

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

[Chris Sosnin]

[-- Attachment #1: Type: text/plain, Size: 2225 bytes --]


> On 6 Mar 2020, at 20:27, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> 
> On Thu, Mar 05, 2020 at 08:41:35AM +0000, Nikita Pettik wrote:
>> On 05 Mar 08:41, Kirill Yukhin wrote:
>>> Hello,
>>> 
>>> On 03 мар 19:16, Chris Sosnin wrote:
>>>> 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/tree/ksosnin/gh-4769-unprepare-response-body
>>>> issue: https://github.com/tarantool/tarantool/issues/4769
>>>> 
>>>> As Nikita suggested, I created box/iproto.test.lua, and basically
>>>> inserted wrappers for requests testing from box-py for future usage.
>>> 
>>> Could you please rename the test to be not so generic?
>>> Like box/gh-4769-iproto-unprep-body or whatever.
>> 
>> Kirill, this test is going to assemble all iproto-related tests
>> which don't rely on net.box module. Setting up all preparations
>> required for raw iproto communication results in duplicating ~30-40
>> lines of code in each test file.
> 
> Technically there are two ways to extract helpers from a 'core =
> tarantool' test:
> 
> * Add it to, say, test/box/box.lua and to _G.protected_globals.
> * Add it to a separate Lua file in test/box/lua and to 'lua_libs' field
>  in test/box/suite.ini. After this you can use `require` for this
>  module in a test.

This also seems like a fine solution, if we are to stick to the SOP, I will do this.

However, I’m not sure whether this patch fixes a bug, it is stated in the code
that there’s nothing to send in case of unprepare, perhaps it is a feature?

I will resend v3 if no one gives other proposals.
Thank you for participating in discussion!

> 
> So technically you're not blocked here. Both ways are available and
> don't lead to much code duplication, but the process (SOP) requires to
> add a test for a bug to a separate file. (Personally I still don't sure
> it is good, but anyway.)
> 
> NB: 'receive', not 'recieve'. Very often typo.

Thanks for the catch here too, I fixed it for the future.

> 
> WBR, Alexander Turenko.


[-- Attachment #2: Type: text/html, Size: 14409 bytes --]

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

[Alexander Turenko]

> > Technically there are two ways to extract helpers from a 'core =
> > tarantool' test:
> > 
> > * Add it to, say, test/box/box.lua and to _G.protected_globals.
> > * Add it to a separate Lua file in test/box/lua and to 'lua_libs' field
> >  in test/box/suite.ini. After this you can use `require` for this
> >  module in a test.
> 
> This also seems like a fine solution, if we are to stick to the SOP, I will do this.

It is our process. We should follow it or at least provide strong
reasoning why we shouldn't in a particular case.

> However, I’m not sure whether this patch fixes a bug, it is stated in the code
> that there’s nothing to send in case of unprepare, perhaps it is a feature?

The issue is marked with 'bug' label.

'unprepare' is the only request, which does not contain a body. In
1.6.4-128-gf433f4b0a (see [1]) it was decided to always send a body in a
response and 'unprepare' does not fit this property. It is unexpected
behaviour and it is what we usually call a bug.

[1]: https://github.com/tarantool/tarantool/commit/f433f4b0aeab48be9b5259b6c5065091dbf07553

WBR, Alexander Turenko.

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

[Nikita Pettik]

On 06 Mar 22:58, Chris Sosnin wrote:
> 
> > On 6 Mar 2020, at 20:27, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> > 
> > On Thu, Mar 05, 2020 at 08:41:35AM +0000, Nikita Pettik wrote:
> >> On 05 Mar 08:41, Kirill Yukhin wrote:
> >>> Hello,
> >>> 
> >>> On 03 мар 19:16, Chris Sosnin wrote:
> >>>> 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/tree/ksosnin/gh-4769-unprepare-response-body
> >>>> issue: https://github.com/tarantool/tarantool/issues/4769
> >>>> 
> >>>> As Nikita suggested, I created box/iproto.test.lua, and basically
> >>>> inserted wrappers for requests testing from box-py for future usage.
> >>> 
> >>> Could you please rename the test to be not so generic?
> >>> Like box/gh-4769-iproto-unprep-body or whatever.
> >> 
> >> Kirill, this test is going to assemble all iproto-related tests
> >> which don't rely on net.box module. Setting up all preparations
> >> required for raw iproto communication results in duplicating ~30-40
> >> lines of code in each test file.
> > 
> > Technically there are two ways to extract helpers from a 'core =
> > tarantool' test:
> > 
> > * Add it to, say, test/box/box.lua and to _G.protected_globals.
> > * Add it to a separate Lua file in test/box/lua and to 'lua_libs' field
> >  in test/box/suite.ini. After this you can use `require` for this
> >  module in a test.
> 
> This also seems like a fine solution, if we are to stick to the SOP, I will do this.
> 
> However, I’m not sure whether this patch fixes a bug, it is stated in the code
> that there’s nothing to send in case of unprepare, perhaps it is a feature?

It's neither bug nor feature. I didn't realize that all other
requests have empty body (what is more, docs say that nop hasn't body
but in fact nop does have body tho) while was writing this code.
As it is stated in the issue, to unify raw statement processing it is
better to add empty body to unprepare response - just refactoring that
may simplify smb's life. That's it.

Anyway, I will introduce box/iproto.test.lua since stacked diag is
a feature. (It is so ironic that developers straggle from decision
which was taken solely by Kirill.)
 
> I will resend v3 if no one gives other proposals.
> Thank you for participating in discussion!
> 
> > 
> > So technically you're not blocked here. Both ways are available and
> > don't lead to much code duplication, but the process (SOP) requires to
> > add a test for a bug to a separate file. (Personally I still don't sure
> > it is good, but anyway.)
> > 
> > NB: 'receive', not 'recieve'. Very often typo.
> 
> Thanks for the catch here too, I fixed it for the future.
> 
> > 
> > WBR, Alexander Turenko.
> 

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

[Alexander Turenko]

On Sat, Mar 07, 2020 at 10:02:28PM +0000, Nikita Pettik wrote:
> On 06 Mar 22:58, Chris Sosnin wrote:
> > 
> > > On 6 Mar 2020, at 20:27, Alexander Turenko <alexander.turenko@tarantool.org> wrote:
> > > 
> > > On Thu, Mar 05, 2020 at 08:41:35AM +0000, Nikita Pettik wrote:
> > >> On 05 Mar 08:41, Kirill Yukhin wrote:
> > >>> Hello,
> > >>> 
> > >>> On 03 мар 19:16, Chris Sosnin wrote:
> > >>>> 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/tree/ksosnin/gh-4769-unprepare-response-body
> > >>>> issue: https://github.com/tarantool/tarantool/issues/4769
> > >>>> 
> > >>>> As Nikita suggested, I created box/iproto.test.lua, and basically
> > >>>> inserted wrappers for requests testing from box-py for future usage.
> > >>> 
> > >>> Could you please rename the test to be not so generic?
> > >>> Like box/gh-4769-iproto-unprep-body or whatever.
> > >> 
> > >> Kirill, this test is going to assemble all iproto-related tests
> > >> which don't rely on net.box module. Setting up all preparations
> > >> required for raw iproto communication results in duplicating ~30-40
> > >> lines of code in each test file.
> > > 
> > > Technically there are two ways to extract helpers from a 'core =
> > > tarantool' test:
> > > 
> > > * Add it to, say, test/box/box.lua and to _G.protected_globals.
> > > * Add it to a separate Lua file in test/box/lua and to 'lua_libs' field
> > >  in test/box/suite.ini. After this you can use `require` for this
> > >  module in a test.
> > 
> > This also seems like a fine solution, if we are to stick to the SOP, I will do this.
> > 
> > However, I’m not sure whether this patch fixes a bug, it is stated in the code
> > that there’s nothing to send in case of unprepare, perhaps it is a feature?
> 
> It's neither bug nor feature. I didn't realize that all other
> requests have empty body (what is more, docs say that nop hasn't body
> but in fact nop does have body tho) while was writing this code.
> As it is stated in the issue, to unify raw statement processing it is
> better to add empty body to unprepare response - just refactoring that
> may simplify smb's life. That's it.
> 
> Anyway, I will introduce box/iproto.test.lua since stacked diag is
> a feature. (It is so ironic that developers straggle from decision
> which was taken solely by Kirill.)

If you want my personal opinion: pick up any way and go ahead. I was
asked by Kirill to answer how to extract helpers from a test, that is
why I'm here.

I still think that a fix of a certain property should be considered as a
bugfix, but it does not worth to spend time to discuss it.

WBR, Alexander Turenko.

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

[Vladislav Shpilevoy]

On 06/03/2020 18:27, Alexander Turenko wrote:
> On Thu, Mar 05, 2020 at 08:41:35AM +0000, Nikita Pettik wrote:
>> On 05 Mar 08:41, Kirill Yukhin wrote:
>>> Hello,
>>>
>>> On 03 мар 19:16, Chris Sosnin wrote:
>>>> 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/tree/ksosnin/gh-4769-unprepare-response-body
>>>> issue: https://github.com/tarantool/tarantool/issues/4769
>>>>
>>>> As Nikita suggested, I created box/iproto.test.lua, and basically
>>>> inserted wrappers for requests testing from box-py for future usage.
>>>
>>> Could you please rename the test to be not so generic?
>>> Like box/gh-4769-iproto-unprep-body or whatever.
>>
>> Kirill, this test is going to assemble all iproto-related tests
>> which don't rely on net.box module. Setting up all preparations
>> required for raw iproto communication results in duplicating ~30-40
>> lines of code in each test file.
> 
> Technically there are two ways to extract helpers from a 'core =
> tarantool' test:
> 
> * Add it to, say, test/box/box.lua and to _G.protected_globals.
> * Add it to a separate Lua file in test/box/lua and to 'lua_libs' field
>   in test/box/suite.ini. After this you can use `require` for this
>   module in a test.
> 
> So technically you're not blocked here. Both ways are available and
> don't lead to much code duplication, but the process (SOP) requires to
> add a test for a bug to a separate file. (Personally I still don't sure
> it is good, but anyway.)
> 
> NB: 'receive', not 'recieve'. Very often typo.
> 
> WBR, Alexander Turenko.

The whole purpose of the 'one issue - one file' was to simplify
reproducibility in a console. When you need to extract some helpers
into a second file, the idea does not work anymore, but just complicates
life, when you need to invent how to make resuable and abstract
something, which is not needed to be reusable and abstract really.

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

[Konstantin Osipov]

* Chris Sosnin <k.sosnin@tarantool.org> [20/03/03 19:18]:
> Absence of the body in the unprepare response forces users to perform
> additional checks to avoid errors. Adding an empty body fixes this problem.

There is no word unprepare. 

The standard term is "deallocate prepare".


-- 
Konstantin Osipov, Moscow, Russia

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

[Kirill Yukhin]

Hello,

On 15 мар 16:34, Vladislav Shpilevoy wrote:
> 
> 
> On 06/03/2020 18:27, Alexander Turenko wrote:
> > On Thu, Mar 05, 2020 at 08:41:35AM +0000, Nikita Pettik wrote:
> >> On 05 Mar 08:41, Kirill Yukhin wrote:
> >>> Hello,
> >>>
> >>> On 03 мар 19:16, Chris Sosnin wrote:
> >>>> 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/tree/ksosnin/gh-4769-unprepare-response-body
> >>>> issue: https://github.com/tarantool/tarantool/issues/4769
> >>>>
> >>>> As Nikita suggested, I created box/iproto.test.lua, and basically
> >>>> inserted wrappers for requests testing from box-py for future usage.
> >>>
> >>> Could you please rename the test to be not so generic?
> >>> Like box/gh-4769-iproto-unprep-body or whatever.
> >>
> >> Kirill, this test is going to assemble all iproto-related tests
> >> which don't rely on net.box module. Setting up all preparations
> >> required for raw iproto communication results in duplicating ~30-40
> >> lines of code in each test file.
> > 
> > Technically there are two ways to extract helpers from a 'core =
> > tarantool' test:
> > 
> > * Add it to, say, test/box/box.lua and to _G.protected_globals.
> > * Add it to a separate Lua file in test/box/lua and to 'lua_libs' field
> >   in test/box/suite.ini. After this you can use `require` for this
> >   module in a test.
> > 
> > So technically you're not blocked here. Both ways are available and
> > don't lead to much code duplication, but the process (SOP) requires to
> > add a test for a bug to a separate file. (Personally I still don't sure
> > it is good, but anyway.)
> > 
> > NB: 'receive', not 'recieve'. Very often typo.
> > 
> > WBR, Alexander Turenko.
> 
> The whole purpose of the 'one issue - one file' was to simplify
> reproducibility in a console. When you need to extract some helpers
> into a second file, the idea does not work anymore, but just complicates
> life, when you need to invent how to make resuable and abstract
> something, which is not needed to be reusable and abstract really.

Also, the purpose was to separate cases so they're not interfere.
I guess, we might have something like 'gcc -E' to preprocess
the cases.

--
Regards, Kirill Yukhin