[PATCH 1/2] tuple format: don't allow null where array/map is expected

[PATCH 1/2] tuple format: don't allow null where array/map is expected

[Vladimir Davydov]

If an indexed field expects array/map, it shouldn't be allowed to insert
null instead, because this might break expectations of field accessors.
For unikey indexes inserting null instead of array/map works fine though
somewhat confusing: for a non-nullable field you get a wrong error
message ("field is missing" instead of "array/map expected, got nil");
for a nullable field, this silently works, just looks weird as there's a
clear type mismatch here. However, for a multikey field you get a crash
as tuple_multikey_count() doesn't expect to see null where an array
should be according to the format:

  tuple_raw_multikey_count: Assertion `mp_typeof(*array_raw) == MP_ARRAY' failed.

This issue exists, because we assume all fields are nullable by default
for some reason. Fix that and add some tests.

Note, you can still omit nullable fields, e.g. if field "[2].a[1]" is
nullable you may insert tuple [1, {a = {}}] or [1, {b = 1}] or even [1],
you just can't pass box.NULL instead of an array/map.
---
https://github.com/tarantool/tarantool/commits/dv/tuple-format-check-fixes

This patch should be backported to 2.1 if accepted.

 src/box/tuple_format.c        |  2 +-
 test/engine/json.result       | 83 +++++++++++++++++++++++++++++++++++++++++--
 test/engine/json.test.lua     | 27 ++++++++++++--
 test/engine/multikey.result   | 58 ++++++++++++++++++++++++++++++
 test/engine/multikey.test.lua | 19 ++++++++++
 5 files changed, 184 insertions(+), 5 deletions(-)

diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 9cdeb051..9b913306 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -152,7 +152,7 @@ tuple_field_new(void)
 	field->type = FIELD_TYPE_ANY;
 	field->offset_slot = TUPLE_OFFSET_SLOT_NIL;
 	field->coll_id = COLL_NONE;
-	field->nullable_action = ON_CONFLICT_ACTION_NONE;
+	field->nullable_action = ON_CONFLICT_ACTION_DEFAULT;
 	field->multikey_required_fields = NULL;
 	return field;
 }
diff --git a/test/engine/json.result b/test/engine/json.result
index e5a36a8f..9f0c8968 100644
--- a/test/engine/json.result
+++ b/test/engine/json.result
@@ -596,6 +596,9 @@ box.snapshot()
 - ok
 ...
 test_run:cmd("restart server default")
+engine = test_run:get_cfg('engine')
+---
+...
 s = box.space["withdata"]
 ---
 ...
@@ -685,7 +688,7 @@ s:drop()
 ...
 -- Check replace with tuple with map having numeric keys that
 -- cannot be included in JSON index.
-s = box.schema.space.create('withdata', {engine='vinyl'})
+s = box.schema.space.create('withdata', {engine = engine})
 ---
 ...
 pk = s:create_index('pk', {parts={{1, 'int'}}})
@@ -716,9 +719,85 @@ s:insert({5, {1, 1, 1}, {{fname='A', sname='B'}, {fname='C', sname='D'}, {fname=
 - [5, [1, 1, 1], [{'fname': 'A', 'sname': 'B'}, {'fname': 'C', 'sname': 'D'}, {'fname': 'A',
       'sname': 'B'}]]
 ...
-s:delete(5)
+_ = s:delete(5)
 ---
 ...
 s:drop()
 ---
 ...
+-- Check that null isn't allowed in case array/map is expected
+-- according to json document format.
+s = box.schema.space.create('test', {engine = engine})
+---
+...
+_ = s:create_index('pk')
+---
+...
+_ = s:create_index('sk', {parts = {{'[2][1].a', 'unsigned'}}})
+---
+...
+s:insert{1, box.NULL} -- error
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected array'
+...
+s:insert{2, {box.NULL}} -- error
+---
+- error: 'Tuple field [2][1] type does not match one required by operation: expected
+    map'
+...
+s:insert{3} -- error
+---
+- error: Tuple field [2][1]["a"] required by space format is missing
+...
+s:insert{4, {}} -- error
+---
+- error: Tuple field [2][1]["a"] required by space format is missing
+...
+s:insert{5, {{b = 1}}} -- error
+---
+- error: Tuple field [2][1]["a"] required by space format is missing
+...
+s:insert{6, {{a = 1}}} -- ok
+---
+- [6, [{'a': 1}]]
+...
+s.index.sk:alter{parts = {{'[2][1].a', 'unsigned', is_nullable = true}}}
+---
+...
+s:insert{7, box.NULL} -- error
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected array'
+...
+s:insert{8, {box.NULL}} -- error
+---
+- error: 'Tuple field [2][1] type does not match one required by operation: expected
+    map'
+...
+-- Skipping nullable fields is okay though.
+s:insert{9} -- ok
+---
+- [9]
+...
+s:insert{10, {}} -- ok
+---
+- [10, []]
+...
+s:insert{11, {{b = 1}}} -- ok
+---
+- [11, [{'b': 1}]]
+...
+s:insert{12, {{a = box.NULL}}} -- ok
+---
+- [12, [{'a': null}]]
+...
+s.index.sk:select()
+---
+- - [9]
+  - [10, []]
+  - [11, [{'b': 1}]]
+  - [12, [{'a': null}]]
+  - [6, [{'a': 1}]]
+...
+s:drop()
+---
+...
diff --git a/test/engine/json.test.lua b/test/engine/json.test.lua
index 592706e0..c6c207dc 100644
--- a/test/engine/json.test.lua
+++ b/test/engine/json.test.lua
@@ -171,6 +171,7 @@ name:select({'Max'})
 name:get({'Max', 'Stierlitz', 'Otto'})
 box.snapshot()
 test_run:cmd("restart server default")
+engine = test_run:get_cfg('engine')
 s = box.space["withdata"]
 pk = s.index["pk"]
 name = s.index["name"]
@@ -195,7 +196,7 @@ s:drop()
 
 -- Check replace with tuple with map having numeric keys that
 -- cannot be included in JSON index.
-s = box.schema.space.create('withdata', {engine='vinyl'})
+s = box.schema.space.create('withdata', {engine = engine})
 pk = s:create_index('pk', {parts={{1, 'int'}}})
 idx0 = s:create_index('idx0', {parts = {{2, 'str', path = 'name'}, {3, "str"}}})
 s:insert({4, {"d", name='D'}, "test"})
@@ -204,5 +205,27 @@ idx0:drop()
 s:truncate()
 idx0 = s:create_index('idx2', {parts = {{3, 'str', path = '[1].fname'}, {3, 'str', path = '[1].sname'}}})
 s:insert({5, {1, 1, 1}, {{fname='A', sname='B'}, {fname='C', sname='D'}, {fname='A', sname='B'}}})
-s:delete(5)
+_ = s:delete(5)
+s:drop()
+
+-- Check that null isn't allowed in case array/map is expected
+-- according to json document format.
+s = box.schema.space.create('test', {engine = engine})
+_ = s:create_index('pk')
+_ = s:create_index('sk', {parts = {{'[2][1].a', 'unsigned'}}})
+s:insert{1, box.NULL} -- error
+s:insert{2, {box.NULL}} -- error
+s:insert{3} -- error
+s:insert{4, {}} -- error
+s:insert{5, {{b = 1}}} -- error
+s:insert{6, {{a = 1}}} -- ok
+s.index.sk:alter{parts = {{'[2][1].a', 'unsigned', is_nullable = true}}}
+s:insert{7, box.NULL} -- error
+s:insert{8, {box.NULL}} -- error
+-- Skipping nullable fields is okay though.
+s:insert{9} -- ok
+s:insert{10, {}} -- ok
+s:insert{11, {{b = 1}}} -- ok
+s:insert{12, {{a = box.NULL}}} -- ok
+s.index.sk:select()
 s:drop()
diff --git a/test/engine/multikey.result b/test/engine/multikey.result
index 38c108eb..e7326cb9 100644
--- a/test/engine/multikey.result
+++ b/test/engine/multikey.result
@@ -777,3 +777,61 @@ s:insert({"Jorge", {"911", "89457609234"}, 'a'})
 s:drop()
 ---
 ...
+--
+-- Inserting box.NULL where a multikey array is expected is
+-- handled gracefully: no crashes, just an error message.
+--
+s = box.schema.space.create('test', {engine = engine})
+---
+...
+_ = s:create_index('pk')
+---
+...
+_ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}})
+---
+...
+s:insert{1, box.NULL} -- error
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected array'
+...
+s:insert{2, {box.NULL}} -- error
+---
+- error: 'Tuple field [2][*] type does not match one required by operation: expected
+    unsigned'
+...
+s:insert{3, {}} -- ok
+---
+- [3, []]
+...
+s:insert{4, {1}} -- ok
+---
+- [4, [1]]
+...
+s.index.sk:alter{parts = {{'[2][*]', 'unsigned', is_nullable = true}}}
+---
+...
+s:insert{5, box.NULL} -- still error
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected array'
+...
+s:insert{6, {box.NULL}} -- ok
+---
+- [6, [null]]
+...
+s:insert{7, {}} -- ok
+---
+- [7, []]
+...
+s:insert{8, {2}} -- ok
+---
+- [8, [2]]
+...
+s.index.sk:select()
+---
+- - [6, [null]]
+  - [4, [1]]
+  - [8, [2]]
+...
+s:drop()
+---
+...
diff --git a/test/engine/multikey.test.lua b/test/engine/multikey.test.lua
index a2bef989..48eed2a3 100644
--- a/test/engine/multikey.test.lua
+++ b/test/engine/multikey.test.lua
@@ -205,3 +205,22 @@ phone_idx = s:create_index('phone_idx', {parts = {{'[2][*]', 'string'}, {3, 'str
 s:insert({"Genadiy", {"911"}, 'b'})
 s:insert({"Jorge", {"911", "89457609234"}, 'a'})
 s:drop()
+
+--
+-- Inserting box.NULL where a multikey array is expected is
+-- handled gracefully: no crashes, just an error message.
+--
+s = box.schema.space.create('test', {engine = engine})
+_ = s:create_index('pk')
+_ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}})
+s:insert{1, box.NULL} -- error
+s:insert{2, {box.NULL}} -- error
+s:insert{3, {}} -- ok
+s:insert{4, {1}} -- ok
+s.index.sk:alter{parts = {{'[2][*]', 'unsigned', is_nullable = true}}}
+s:insert{5, box.NULL} -- still error
+s:insert{6, {box.NULL}} -- ok
+s:insert{7, {}} -- ok
+s:insert{8, {2}} -- ok
+s.index.sk:select()
+s:drop()
-- 
2.11.0

[PATCH 2/2] tuple format: remove invalid assertion from tuple_format_iterator_next

[Vladimir Davydov]

It's too early to assert msgpack type as an array when a multikey field
is encountered - we haven't checked the field type yet so the type might
as well be a map, in which case we will raise an error just a few lines
below. Remove the assertion and add a test case.
---
https://github.com/tarantool/tarantool/commits/dv/tuple-format-check-fixes

 src/box/tuple_format.c        |  1 -
 test/engine/multikey.result   | 28 ++++++++++++++++++++++++++++
 test/engine/multikey.test.lua | 12 ++++++++++++
 3 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/src/box/tuple_format.c b/src/box/tuple_format.c
index 9b913306..02fadf1c 100644
--- a/src/box/tuple_format.c
+++ b/src/box/tuple_format.c
@@ -1138,7 +1138,6 @@ tuple_format_iterator_next(struct tuple_format_iterator *it,
 			 * and is equal to multikey index
 			 * comparison hint.
 			 */
-			assert(type == MP_ARRAY);
 			it->multikey_frame = mp_stack_top(&it->stack);
 		}
 		it->parent = &field->token;
diff --git a/test/engine/multikey.result b/test/engine/multikey.result
index e7326cb9..30208227 100644
--- a/test/engine/multikey.result
+++ b/test/engine/multikey.result
@@ -835,3 +835,31 @@ s.index.sk:select()
 s:drop()
 ---
 ...
+--
+-- Inserting a map where a multikey array is expected is
+-- handled gracefully: no crashes, just an error message.
+--
+s = box.schema.space.create('test', {engine = engine})
+---
+...
+_ = s:create_index('pk')
+---
+...
+_ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}})
+---
+...
+s:insert{1, {a = 1}} -- error
+---
+- error: 'Tuple field 2 type does not match one required by operation: expected array'
+...
+s:insert{2, {1}} -- ok
+---
+- [2, [1]]
+...
+s.index.sk:select()
+---
+- - [2, [1]]
+...
+s:drop()
+---
+...
diff --git a/test/engine/multikey.test.lua b/test/engine/multikey.test.lua
index 48eed2a3..6d974716 100644
--- a/test/engine/multikey.test.lua
+++ b/test/engine/multikey.test.lua
@@ -224,3 +224,15 @@ s:insert{7, {}} -- ok
 s:insert{8, {2}} -- ok
 s.index.sk:select()
 s:drop()
+
+--
+-- Inserting a map where a multikey array is expected is
+-- handled gracefully: no crashes, just an error message.
+--
+s = box.schema.space.create('test', {engine = engine})
+_ = s:create_index('pk')
+_ = s:create_index('sk', {parts = {{'[2][*]', 'unsigned'}}})
+s:insert{1, {a = 1}} -- error
+s:insert{2, {1}} -- ok
+s.index.sk:select()
+s:drop()
-- 
2.11.0

[tarantool-patches] Re: [PATCH 1/2] tuple format: don't allow null where array/map is expected

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/22 18:46]:

I'm not sure users will like it, but since the fix is so easy,
lgtm, we can always allow nulls in the future.


-- 
Konstantin Osipov, Moscow, Russia

[tarantool-patches] Re: [PATCH 2/2] tuple format: remove invalid assertion from tuple_format_iterator_next

[Konstantin Osipov]

* Vladimir Davydov <vdavydov.dev@gmail.com> [19/05/22 18:46]:
> It's too early to assert msgpack type as an array when a multikey field
> is encountered - we haven't checked the field type yet so the type might
> as well be a map, in which case we will raise an error just a few lines
> below. Remove the assertion and add a test case.

I assume you pushed as obvious already, lgtm anywya.


-- 
Konstantin Osipov, Moscow, Russia

Re: [PATCH 1/2] tuple format: don't allow null where array/map is expected

[Vladimir Davydov]

Pushed to master and 2.1.

Re: [PATCH 2/2] tuple format: remove invalid assertion from tuple_format_iterator_next

[Vladimir Davydov]

Pushed to master.