[Tarantool-patches] [PATCH v3 0/2] add an empty body to the unprepare response

[Tarantool-patches] [PATCH v3 0/2] add an empty body to the unprepare response

[Chris Sosnin]

issue: https://github.com/tarantool/tarantool/issues/4769
branch: https://github.com/tarantool/tarantool/tree/ksosnin/gh-4769-unprepare-response-body

Changes in v2:
 - Rewrite tests from Python to Lua.

Changes in v3:
 - As suggested, I created a helper function for
sending iproto requests (which can also be used
in future iproto.test.lua) and renamed the test files
using gh-4769- pattern.

Chris Sosnin (2):
  test: introduce iproto_request helper function
  iproto: add an empty body to the unprepare response

 src/box/iproto.cc                             | 19 +++---
 test/box/box.lua                              | 22 ++++++-
 .../gh-4769-unprepare-response-body.result    | 62 +++++++++++++++++++
 .../gh-4769-unprepare-response-body.test.lua  | 30 +++++++++
 4 files changed, 124 insertions(+), 9 deletions(-)
 create mode 100644 test/box/gh-4769-unprepare-response-body.result
 create mode 100644 test/box/gh-4769-unprepare-response-body.test.lua

-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v3 1/2] test: introduce iproto_request helper function

[Chris Sosnin]

It is needed for performing iproto tests in Lua.

Needed for #4769
---
 test/box/box.lua | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/test/box/box.lua b/test/box/box.lua
index 2a8e0e4fa..6fad07015 100644
--- a/test/box/box.lua
+++ b/test/box/box.lua
@@ -1,6 +1,8 @@
 #!/usr/bin/env tarantool
 os = require('os')
 
+local msgpack = require('msgpack')
+
 box.cfg{
     listen              = os.getenv("LISTEN"),
     memtx_memory        = 107374182,
@@ -38,4 +40,22 @@ function sorted(data)
     return data
 end
 
-_G.protected_globals = {'cfg_filter', 'sorted'}
+function iproto_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')
+    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)
+    body = msgpack.decode(response:sub(header_len))
+    return {
+        ['header'] = header,
+        ['body'] = body,
+    }
+end
+
+_G.protected_globals = {'cfg_filter', 'sorted', 'iproto_request'}
-- 
2.21.1 (Apple Git-122.3)

[Tarantool-patches] [PATCH v3 2/2] 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
---
 src/box/iproto.cc                             | 19 +++---
 .../gh-4769-unprepare-response-body.result    | 62 +++++++++++++++++++
 .../gh-4769-unprepare-response-body.test.lua  | 30 +++++++++
 3 files changed, 103 insertions(+), 8 deletions(-)
 create mode 100644 test/box/gh-4769-unprepare-response-body.result
 create mode 100644 test/box/gh-4769-unprepare-response-body.test.lua

diff --git a/src/box/iproto.cc b/src/box/iproto.cc
index f313af6ae..5a3fb5d0b 100644
--- a/src/box/iproto.cc
+++ b/src/box/iproto.cc
@@ -1780,21 +1780,24 @@ tx_process_sql(struct cmsg *m)
 	 * become out of date during yield.
 	 */
 	out = msg->connection->tx.p_obuf;
+	if (is_unprepare) {
+		if (iproto_reply_ok(out, msg->header.sync, schema_version) != 0)
+			goto error;
+		iproto_wpos_create(&msg->wpos, out);
+		return;
+	}
 	struct obuf_svp header_svp;
 	/* 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;
@@ -1871,7 +1874,7 @@ net_send_msg(struct cmsg *m)
 }
 
 /**
- * Complete sending an iproto error: 
+ * Complete sending an iproto error:
  * recycle the error object and flush output.
  */
 static void
diff --git a/test/box/gh-4769-unprepare-response-body.result b/test/box/gh-4769-unprepare-response-body.result
new file mode 100644
index 000000000..a5230fad1
--- /dev/null
+++ b/test/box/gh-4769-unprepare-response-body.result
@@ -0,0 +1,62 @@
+-- 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
+
+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)
+ | ---
+ | ...
+
+header = { [IPROTO_REQUEST_TYPE] = IPROTO_PREPARE }
+ | ---
+ | ...
+body = { [IPROTO_SQL_TEXT] = 'SELECT 1' }
+ | ---
+ | ...
+response = iproto_request(socket, header, body)
+ | ---
+ | ...
+
+body = { [IPROTO_STMT_ID] = response['body'][IPROTO_STMT_ID] }
+ | ---
+ | ...
+-- Decoding of the response will fail if there's no body.
+response = iproto_request(socket, header, body)
+ | ---
+ | ...
+response.body
+ | ---
+ | - {}
+ | ...
+
+box.schema.user.revoke('guest', 'read, write, execute', 'universe')
+ | ---
+ | ...
+socket:close()
+ | ---
+ | - true
+ | ...
+
diff --git a/test/box/gh-4769-unprepare-response-body.test.lua b/test/box/gh-4769-unprepare-response-body.test.lua
new file mode 100644
index 000000000..70e41b3e5
--- /dev/null
+++ b/test/box/gh-4769-unprepare-response-body.test.lua
@@ -0,0 +1,30 @@
+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
+
+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)
+
+header = { [IPROTO_REQUEST_TYPE] = IPROTO_PREPARE }
+body = { [IPROTO_SQL_TEXT] = 'SELECT 1' }
+response = iproto_request(socket, header, body)
+
+body = { [IPROTO_STMT_ID] = response['body'][IPROTO_STMT_ID] }
+-- Decoding of the response will fail if there's no body.
+response = iproto_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 v3 2/2] iproto: add an empty body to the unprepare response

[Vladislav Shpilevoy]

Hi! Thanks for the patch!

See 2 comments below.

> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
> index f313af6ae..5a3fb5d0b 100644
> --- a/src/box/iproto.cc
> +++ b/src/box/iproto.cc
> @@ -1871,7 +1874,7 @@ net_send_msg(struct cmsg *m)
>  }
>  
>  /**
> - * Complete sending an iproto error: 
> + * Complete sending an iproto error:

1. Seems like a stray not related change.

>   * recycle the error object and flush output.
>   */
>  static void
> diff --git a/test/box/gh-4769-unprepare-response-body.result b/test/box/gh-4769-unprepare-response-body.result
> new file mode 100644
> index 000000000..a5230fad1
> --- /dev/null
> +++ b/test/box/gh-4769-unprepare-response-body.result
> @@ -0,0 +1,62 @@
> +-- test-run result file version 2
> +test_run = require('test_run').new()
> + | ---
> + | ...
> +test_run:cmd("setopt delimiter ';'")

2. Why? You never use the new delimiter until you
reset it back to normal.

You don't even need test_run = require('test_run').new(),
because without the needless delimiter change the
test_run object is also unused.

> + | ---
> + | - 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
> +
> +test_run:cmd("setopt delimiter ''");
> + | ---
> + | ...
> +

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

[Chris Sosnin]

Hi! Thank you for the review!

> On 15 Mar 2020, at 18:31, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
> 
> Hi! Thanks for the patch!
> 
> See 2 comments below.
> 
>> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
>> index f313af6ae..5a3fb5d0b 100644
>> --- a/src/box/iproto.cc
>> +++ b/src/box/iproto.cc
>> @@ -1871,7 +1874,7 @@ net_send_msg(struct cmsg *m)
>> }
>> 
>> /**
>> - * Complete sending an iproto error: 
>> + * Complete sending an iproto error:
> 
> 1. Seems like a stray not related change.

I didn’t notice that my editor trimmed this trailing whitespace. I’m sorry.
Reverted on branch.

> 
>>  * recycle the error object and flush output.
>>  */
>> static void
>> diff --git a/test/box/gh-4769-unprepare-response-body.result b/test/box/gh-4769-unprepare-response-body.result
>> new file mode 100644
>> index 000000000..a5230fad1
>> --- /dev/null
>> +++ b/test/box/gh-4769-unprepare-response-body.result
>> @@ -0,0 +1,62 @@
>> +-- test-run result file version 2
>> +test_run = require('test_run').new()
>> + | ---
>> + | ...
>> +test_run:cmd("setopt delimiter ';'")
> 
> 2. Why? You never use the new delimiter until you
> reset it back to normal.
> 
> You don't even need test_run = require('test_run').new(),
> because without the needless delimiter change the
> test_run object is also unused.

I did this to reduce output in the .result file. Fixed:

+-- test-run result file version 2
+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
+ | ---
+ | ...
+
+box.schema.user.grant('guest', 'read, write, execute', 'universe')

> 
>> + | ---
>> + | - 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
>> +
>> +test_run:cmd("setopt delimiter ''");
>> + | ---
>> + | ...
>> +

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

[Vladislav Shpilevoy]

LGTM.

On 16/03/2020 10:02, Chris Sosnin wrote:
> Hi! Thank you for the review!
> 
>> On 15 Mar 2020, at 18:31, Vladislav Shpilevoy <v.shpilevoy@tarantool.org> wrote:
>>
>> Hi! Thanks for the patch!
>>
>> See 2 comments below.
>>
>>> diff --git a/src/box/iproto.cc b/src/box/iproto.cc
>>> index f313af6ae..5a3fb5d0b 100644
>>> --- a/src/box/iproto.cc
>>> +++ b/src/box/iproto.cc
>>> @@ -1871,7 +1874,7 @@ net_send_msg(struct cmsg *m)
>>> }
>>>
>>> /**
>>> - * Complete sending an iproto error: 
>>> + * Complete sending an iproto error:
>>
>> 1. Seems like a stray not related change.
> 
> I didn’t notice that my editor trimmed this trailing whitespace. I’m sorry.
> Reverted on branch.
> 
>>
>>>  * recycle the error object and flush output.
>>>  */
>>> static void
>>> diff --git a/test/box/gh-4769-unprepare-response-body.result b/test/box/gh-4769-unprepare-response-body.result
>>> new file mode 100644
>>> index 000000000..a5230fad1
>>> --- /dev/null
>>> +++ b/test/box/gh-4769-unprepare-response-body.result
>>> @@ -0,0 +1,62 @@
>>> +-- test-run result file version 2
>>> +test_run = require('test_run').new()
>>> + | ---
>>> + | ...
>>> +test_run:cmd("setopt delimiter ';'")
>>
>> 2. Why? You never use the new delimiter until you
>> reset it back to normal.
>>
>> You don't even need test_run = require('test_run').new(),
>> because without the needless delimiter change the
>> test_run object is also unused.
> 
> I did this to reduce output in the .result file. Fixed:
> 
> +-- test-run result file version 2
> +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
> + | ---
> + | ...
> +
> +box.schema.user.grant('guest', 'read, write, execute', 'universe')
> 
>>
>>> + | ---
>>> + | - 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
>>> +
>>> +test_run:cmd("setopt delimiter ''");
>>> + | ---
>>> + | ...
>>> +
> 

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

[Nikita Pettik]

On 16 Mar 23:01, Vladislav Shpilevoy wrote:
> LGTM.
>

Pushed to master and 2.3. Changelogs are updated correspondingly.
Branch is dropped.