[tarantool-patches] [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields

[tarantool-patches] [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields

[imeevma]

Tuple method 'tomap' in some cases worked impropertly if tuple
length less than it should be according to space format. Fixed
in this patch.

Closes #3631
---
Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3631-wrong-tomap-work-with-nullable-fields
Issue: https://github.com/tarantool/tarantool/issues/3631

 src/box/lua/tuple.c        |  4 +--
 test/engine/tuple.result   | 63 ++++++++++++++++++++++++++++++++++++++++++++++
 test/engine/tuple.test.lua | 23 +++++++++++++++++
 3 files changed, 88 insertions(+), 2 deletions(-)

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 7ca4299..856042c 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -284,12 +284,12 @@ lbox_tuple_to_map(struct lua_State *L)
 		luaL_error(L, "Usage: tuple:tomap()");
 	const struct tuple *tuple = lua_checktuple(L, 1);
 	const struct tuple_format *format = tuple_format(tuple);
-	const struct tuple_field *field = &format->fields[0];
 	const char *pos = tuple_data(tuple);
 	int field_count = (int)mp_decode_array(&pos);
 	int n_named = format->dict->name_count;
 	lua_createtable(L, field_count, n_named);
-	for (int i = 0; i < n_named; ++i, ++field) {
+	int fields_number = n_named > field_count ? field_count : n_named;
+	for (int i = 0; i < fields_number; ++i) {
 		/* Access by name. */
 		const char *name = format->dict->names[i];
 		lua_pushstring(L, name);
diff --git a/test/engine/tuple.result b/test/engine/tuple.result
index b3b23b2..b9f42b9 100644
--- a/test/engine/tuple.result
+++ b/test/engine/tuple.result
@@ -590,6 +590,69 @@ maplen(t1map), t1map[1], t1map[2], t1map[3]
 s:drop()
 ---
 ...
+--
+-- gh-3631: Wrong 'tomap' work with nullable fields
+--
+format = {}
+---
+...
+format[1] = {'first', 'unsigned'}
+---
+...
+format[2] = {'second', 'unsigned'}
+---
+...
+format[3] = {'third', 'unsigned'}
+---
+...
+format[4] = {'fourth', 'string', is_nullable = true}
+---
+...
+s = box.schema.space.create('test', {format = format, engine = engine})
+---
+...
+pk = s:create_index('primary', {parts = { 1, 'unsigned'}})
+---
+...
+s:insert({1, 2, 3})
+---
+- [1, 2, 3]
+...
+tuple = s:get(1)
+---
+...
+tuple
+---
+- [1, 2, 3]
+...
+-- Should be NULL
+tuple.fourth
+---
+- null
+...
+-- Should have only three named fields
+tuple:tomap()
+---
+- 1: 1
+  2: 2
+  3: 3
+  third: 3
+  second: 2
+  first: 1
+...
+-- Should be NULL
+tuple:tomap().fourth
+---
+- null
+...
+-- Should be nil
+type(tuple:tomap().fourth)
+---
+- nil
+...
+s:drop()
+---
+...
 engine = nil
 ---
 ...
diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua
index 6d7d254..10b888e 100644
--- a/test/engine/tuple.test.lua
+++ b/test/engine/tuple.test.lua
@@ -200,5 +200,28 @@ t1map = t1:tomap()
 maplen(t1map), t1map[1], t1map[2], t1map[3]
 s:drop()
 
+--
+-- gh-3631: Wrong 'tomap' work with nullable fields
+--
+format = {}
+format[1] = {'first', 'unsigned'}
+format[2] = {'second', 'unsigned'}
+format[3] = {'third', 'unsigned'}
+format[4] = {'fourth', 'string', is_nullable = true}
+s = box.schema.space.create('test', {format = format, engine = engine})
+pk = s:create_index('primary', {parts = { 1, 'unsigned'}})
+s:insert({1, 2, 3})
+tuple = s:get(1)
+tuple
+-- Should be NULL
+tuple.fourth
+-- Should have only three named fields
+tuple:tomap()
+-- Should be NULL
+tuple:tomap().fourth
+-- Should be nil
+type(tuple:tomap().fourth)
+s:drop()
+
 engine = nil
 test_run = nil
-- 
2.7.4

[tarantool-patches] Re: [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields

[Vladislav Shpilevoy]

Hi! Thanks for the patch! See 3 comments below.

On 22/08/2018 13:43, imeevma@tarantool.org wrote:
> Tuple method 'tomap' in some cases worked impropertly if tuple

1. Typo: 'impropertly'.

> length less than it should be according to space format. Fixed
> in this patch.
> 
> Closes #3631
> ---
> Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-3631-wrong-tomap-work-with-nullable-fields
> Issue: https://github.com/tarantool/tarantool/issues/3631
> 
>   src/box/lua/tuple.c        |  4 +--
>   test/engine/tuple.result   | 63 ++++++++++++++++++++++++++++++++++++++++++++++
>   test/engine/tuple.test.lua | 23 +++++++++++++++++
>   3 files changed, 88 insertions(+), 2 deletions(-)
> 
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 7ca4299..856042c 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -284,12 +284,12 @@ lbox_tuple_to_map(struct lua_State *L)
>   		luaL_error(L, "Usage: tuple:tomap()");
>   	const struct tuple *tuple = lua_checktuple(L, 1);
>   	const struct tuple_format *format = tuple_format(tuple);
> -	const struct tuple_field *field = &format->fields[0];
>   	const char *pos = tuple_data(tuple);
>   	int field_count = (int)mp_decode_array(&pos);
>   	int n_named = format->dict->name_count;
>   	lua_createtable(L, field_count, n_named);
> -	for (int i = 0; i < n_named; ++i, ++field) {
> +	int fields_number = n_named > field_count ? field_count : n_named;

2. Please, use MIN macros. It is available in this file as I
remember.

> +	for (int i = 0; i < fields_number; ++i) {
>   		/* Access by name. */
>   		const char *name = format->dict->names[i];
>   		lua_pushstring(L, name);
> diff --git a/test/engine/tuple.result b/test/engine/tuple.result
> index b3b23b2..b9f42b9 100644
> --- a/test/engine/tuple.result
> +++ b/test/engine/tuple.result
> @@ -590,6 +590,69 @@ maplen(t1map), t1map[1], t1map[2], t1map[3]
>   s:drop()
>   ---
>   ...
> +--
> +-- gh-3631: Wrong 'tomap' work with nullable fields
> +--
> +format = {}
> +---
> +...
> +format[1] = {'first', 'unsigned'}
> +---
> +...
> +format[2] = {'second', 'unsigned'}
> +---
> +...
> +format[3] = {'third', 'unsigned'}
> +---
> +...
> +format[4] = {'fourth', 'string', is_nullable = true}
> +---
> +...
> +s = box.schema.space.create('test', {format = format, engine = engine})
> +---
> +...
> +pk = s:create_index('primary', {parts = { 1, 'unsigned'}})

3. Odd white space before '1'.

> +---
> +...
> +s:insert({1, 2, 3})
> +---
> +- [1, 2, 3]
> +...
> +tuple = s:get(1)
> +---
> +...

[tarantool-patches] Re: [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields

[Imeev Mergen]

Hello! Thank you for the review!


On 08/22/2018 05:16 PM, Vladislav Shpilevoy wrote:
> Hi! Thanks for the patch! See 3 comments below.
>
> On 22/08/2018 13:43, imeevma@tarantool.org wrote:
>> Tuple method 'tomap' in some cases worked impropertly if tuple
>
> 1. Typo: 'impropertly'.
Fixed.
>
>> length less than it should be according to space format. Fixed
>> in this patch.
>>
>> Closes #3631
>> ---
>> Branch: 
>> https://github.com/tarantool/tarantool/tree/imeevma/gh-3631-wrong-tomap-work-with-nullable-fields
>> Issue: https://github.com/tarantool/tarantool/issues/3631
>>
>>   src/box/lua/tuple.c        |  4 +--
>>   test/engine/tuple.result   | 63 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>   test/engine/tuple.test.lua | 23 +++++++++++++++++
>>   3 files changed, 88 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
>> index 7ca4299..856042c 100644
>> --- a/src/box/lua/tuple.c
>> +++ b/src/box/lua/tuple.c
>> @@ -284,12 +284,12 @@ lbox_tuple_to_map(struct lua_State *L)
>>           luaL_error(L, "Usage: tuple:tomap()");
>>       const struct tuple *tuple = lua_checktuple(L, 1);
>>       const struct tuple_format *format = tuple_format(tuple);
>> -    const struct tuple_field *field = &format->fields[0];
>>       const char *pos = tuple_data(tuple);
>>       int field_count = (int)mp_decode_array(&pos);
>>       int n_named = format->dict->name_count;
>>       lua_createtable(L, field_count, n_named);
>> -    for (int i = 0; i < n_named; ++i, ++field) {
>> +    int fields_number = n_named > field_count ? field_count : n_named;
>
> 2. Please, use MIN macros. It is available in this file as I
> remember.
Fixed.
>
>> +    for (int i = 0; i < fields_number; ++i) {
>>           /* Access by name. */
>>           const char *name = format->dict->names[i];
>>           lua_pushstring(L, name);
>> diff --git a/test/engine/tuple.result b/test/engine/tuple.result
>> index b3b23b2..b9f42b9 100644
>> --- a/test/engine/tuple.result
>> +++ b/test/engine/tuple.result
>> @@ -590,6 +590,69 @@ maplen(t1map), t1map[1], t1map[2], t1map[3]
>>   s:drop()
>>   ---
>>   ...
>> +--
>> +-- gh-3631: Wrong 'tomap' work with nullable fields
>> +--
>> +format = {}
>> +---
>> +...
>> +format[1] = {'first', 'unsigned'}
>> +---
>> +...
>> +format[2] = {'second', 'unsigned'}
>> +---
>> +...
>> +format[3] = {'third', 'unsigned'}
>> +---
>> +...
>> +format[4] = {'fourth', 'string', is_nullable = true}
>> +---
>> +...
>> +s = box.schema.space.create('test', {format = format, engine = engine})
>> +---
>> +...
>> +pk = s:create_index('primary', {parts = { 1, 'unsigned'}})
>
> 3. Odd white space before '1'.
Fixed.
>
>> +---
>> +...
>> +s:insert({1, 2, 3})
>> +---
>> +- [1, 2, 3]
>> +...
>> +tuple = s:get(1)
>> +---
>> +...

commit 066db38393316c486b9edef0d71e854c3e4fdc78
Author: Mergen Imeev <imeevma@gmail.com>
Date:   Wed Aug 22 13:33:22 2018 +0300

     lua: wrong 'tomap' work with nullable fields

     Tuple method 'tomap' in some cases worked improperly if tuple
     length less than it should be according to space format. Fixed
     in this patch.

     Closes #3631

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index 7ca4299..cd96152 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -284,12 +284,11 @@ lbox_tuple_to_map(struct lua_State *L)
          luaL_error(L, "Usage: tuple:tomap()");
      const struct tuple *tuple = lua_checktuple(L, 1);
      const struct tuple_format *format = tuple_format(tuple);
-    const struct tuple_field *field = &format->fields[0];
      const char *pos = tuple_data(tuple);
      int field_count = (int)mp_decode_array(&pos);
      int n_named = format->dict->name_count;
      lua_createtable(L, field_count, n_named);
-    for (int i = 0; i < n_named; ++i, ++field) {
+    for (int i = 0; i < MIN(field_count, n_named); ++i) {
          /* Access by name. */
          const char *name = format->dict->names[i];
          lua_pushstring(L, name);
diff --git a/test/engine/tuple.result b/test/engine/tuple.result
index b3b23b2..88bb3b4 100644
--- a/test/engine/tuple.result
+++ b/test/engine/tuple.result
@@ -590,6 +590,69 @@ maplen(t1map), t1map[1], t1map[2], t1map[3]
  s:drop()
  ---
  ...
+--
+-- gh-3631: Wrong 'tomap' work with nullable fields
+--
+format = {}
+---
+...
+format[1] = {'first', 'unsigned'}
+---
+...
+format[2] = {'second', 'unsigned'}
+---
+...
+format[3] = {'third', 'unsigned'}
+---
+...
+format[4] = {'fourth', 'string', is_nullable = true}
+---
+...
+s = box.schema.space.create('test', {format = format, engine = engine})
+---
+...
+pk = s:create_index('primary', {parts = {1, 'unsigned'}})
+---
+...
+s:insert({1, 2, 3})
+---
+- [1, 2, 3]
+...
+tuple = s:get(1)
+---
+...
+tuple
+---
+- [1, 2, 3]
+...
+-- Should be NULL
+tuple.fourth
+---
+- null
+...
+-- Should have only three named fields
+tuple:tomap()
+---
+- 1: 1
+  2: 2
+  3: 3
+  third: 3
+  second: 2
+  first: 1
+...
+-- Should be NULL
+tuple:tomap().fourth
+---
+- null
+...
+-- Should be nil
+type(tuple:tomap().fourth)
+---
+- nil
+...
+s:drop()
+---
+...
  engine = nil
  ---
  ...
diff --git a/test/engine/tuple.test.lua b/test/engine/tuple.test.lua
index 6d7d254..723dde4 100644
--- a/test/engine/tuple.test.lua
+++ b/test/engine/tuple.test.lua
@@ -200,5 +200,28 @@ t1map = t1:tomap()
  maplen(t1map), t1map[1], t1map[2], t1map[3]
  s:drop()

+--
+-- gh-3631: Wrong 'tomap' work with nullable fields
+--
+format = {}
+format[1] = {'first', 'unsigned'}
+format[2] = {'second', 'unsigned'}
+format[3] = {'third', 'unsigned'}
+format[4] = {'fourth', 'string', is_nullable = true}
+s = box.schema.space.create('test', {format = format, engine = engine})
+pk = s:create_index('primary', {parts = {1, 'unsigned'}})
+s:insert({1, 2, 3})
+tuple = s:get(1)
+tuple
+-- Should be NULL
+tuple.fourth
+-- Should have only three named fields
+tuple:tomap()
+-- Should be NULL
+tuple:tomap().fourth
+-- Should be nil
+type(tuple:tomap().fourth)
+s:drop()
+
  engine = nil
  test_run = nil

[tarantool-patches] Re: [PATCH v1 1/1] lua: wrong 'tomap' work with nullable fields

[Vladislav Shpilevoy]

Hi! Thanks for the fixes!

> commit 066db38393316c486b9edef0d71e854c3e4fdc78
> Author: Mergen Imeev <imeevma@gmail.com>
> Date:   Wed Aug 22 13:33:22 2018 +0300
> 
>      lua: wrong 'tomap' work with nullable fields
> 
>      Tuple method 'tomap' in some cases worked improperly if tuple
>      length less than it should be according to space format. Fixed
>      in this patch.
> 
>      Closes #3631
> 
> diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
> index 7ca4299..cd96152 100644
> --- a/src/box/lua/tuple.c
> +++ b/src/box/lua/tuple.c
> @@ -284,12 +284,11 @@ lbox_tuple_to_map(struct lua_State *L)
>           luaL_error(L, "Usage: tuple:tomap()");
>       const struct tuple *tuple = lua_checktuple(L, 1);
>       const struct tuple_format *format = tuple_format(tuple);
> -    const struct tuple_field *field = &format->fields[0];
>       const char *pos = tuple_data(tuple);
>       int field_count = (int)mp_decode_array(&pos);
>       int n_named = format->dict->name_count;
>       lua_createtable(L, field_count, n_named);
> -    for (int i = 0; i < n_named; ++i, ++field) {
> +    for (int i = 0; i < MIN(field_count, n_named); ++i) {

It is not ok to calculate MIN on each iteration.
Either N_named and field_count can not change during
iteration. I fixed it in a separate commit on the
branch. Please, look and squash. And send the patch
as v2 to Vova D.

>           /* Access by name. */
>           const char *name = format->dict->names[i];
>           lua_pushstring(L, name);

My diff (so far it is required by SOP since recently):

diff --git a/src/box/lua/tuple.c b/src/box/lua/tuple.c
index cd961526c..5acbecb23 100644
--- a/src/box/lua/tuple.c
+++ b/src/box/lua/tuple.c
@@ -288,7 +288,8 @@ lbox_tuple_to_map(struct lua_State *L)
  	int field_count = (int)mp_decode_array(&pos);
  	int n_named = format->dict->name_count;
  	lua_createtable(L, field_count, n_named);
-	for (int i = 0; i < MIN(field_count, n_named); ++i) {
+	int named_and_presented = MIN(field_count, n_named);
+	for (int i = 0; i < named_and_presented; ++i) {
  		/* Access by name. */
  		const char *name = format->dict->names[i];
  		lua_pushstring(L, name);