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

[Imeev Mergen <imeevma@tarantool.org>]

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