[tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR

[Mergen Imeev <imeevma@tarantool.org>]

Hi! Thank you for review. My answers and new patch below.
I didn't include diff since patch is quite obvious by
itself.

On Wed, Jul 24, 2019 at 03:24:32PM +0300, n.pettik wrote:
> 
> > ---
> > src/box/sql/vdbe.c      | 13 +++++++++--
> > test/sql/types.result   | 62 +++++++++++++++++++++++++++++++++++++++++++++++++
> > test/sql/types.test.lua | 22 ++++++++++++++++++
> > 3 files changed, 95 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/box/sql/vdbe.c b/src/box/sql/vdbe.c
> > index 1200ff4..f6aaaa6 100644
> > --- a/src/box/sql/vdbe.c
> > +++ b/src/box/sql/vdbe.c
> > @@ -2704,8 +2704,17 @@ case OP_ApplyType: {
> > 		assert(pIn1 <= &p->aMem[(p->nMem+1 - p->nCursor)]);
> > 		assert(memIsValid(pIn1));
> > 		if (mem_apply_type(pIn1, type) != 0) {
> > -			diag_set(ClientError, ER_SQL_TYPE_MISMATCH,
> > -				 sql_value_text(pIn1),
> > +			const char *value;
> > +			if ((pIn1->flags & MEM_Subtype) != 0 &&
> > +			    pIn1->subtype == SQL_SUBTYPE_MSGPACK) {
> > +				if (mp_typeof(*pIn1->z) == MP_MAP)
> > +					value = "map";
> > +				else
> > +					value = "array";
> > +			} else {
> > +				value = (const char *)sql_value_text(pIn1);
> 
> 
> Why not simply patch sql_value_text() to make it convert
> map/array to string representation? I’m afraid this is
> unlikely to be the only place where such error may occur.
> 
Done. I used region to create the result that is returned
by the sql_value_text() function in case the argument was
a BLOB with msgpack of type array or map.

> > +			}
> > +			diag_set(ClientError, ER_SQL_TYPE_MISMATCH, value,
> > 				 field_type_strs[type]);
> > 			goto abort_due_to_error;
> > 		}
> > diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
> > index 22cb105..a99cc9f 100644
> > --- a/test/sql/types.test.lua
> > +++ b/test/sql/types.test.lua
> > @@ -291,3 +291,25 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
> > box.execute('UPDATE t SET b = false WHERE i = 1;')
> > s:select()
> > s:drop()
> > +
> > +--
> > +-- Make sure that the array/map conversion to scalar error is
> > +-- displayed correctly.
> > +--
> > +box.execute('DROP TABLE IF EXISTS t1;’)
> 
> Why ‘IF EXISTS’? Btw I’d better use space:drop() to avoid
> SQL’s overhead.
> 
> 
I dropped this line since it was fixed after rebase.

New patch:

From 5dde6f6398ca9845c3a17173b9e9625bc1b20d32 Mon Sep 17 00:00:00 2001
From: Mergen Imeev <imeevma@gmail.com>
Date: Mon, 22 Jul 2019 12:54:34 +0300
Subject: [PATCH] sql: fix error in case ARRAY/MAP converted to SCALAR

Since ARRAY and MAP cannot be converted to a scalar, this
operation should throw an error. But when the error was throws
from SQL, the error was unreadable. The reason for this is that
the given array or map was not correctly converted to a string.
This patch fixes the problem by converting ARRAY or MAP to their
string representation.
For example:

box.execute('CREATE TABLE t1(i INT PRIMARY KEY, a SCALAR);')
format = {}
format[1] = {type = 'integer', name = 'I'}
format[2] = {type = 'array', name = 'A'}
s = box.schema.space.create('T2', {format=format})
i = s:create_index('ii')
s:insert({1, {1,2,3}})
box.execute('INSERT INTO t1 SELECT * FROM t2;')

Should return:
- error: 'Type mismatch: can not convert [1, 2, 3] to scalar'

Follow-up #4189

diff --git a/src/box/sql/vdbemem.c b/src/box/sql/vdbemem.c
index 847a6b0..8bea46b 100644
--- a/src/box/sql/vdbemem.c
+++ b/src/box/sql/vdbemem.c
@@ -1135,6 +1135,19 @@ valueToText(sql_value * pVal)
 {
 	assert(pVal != 0);
 	assert((pVal->flags & (MEM_Null)) == 0);
+	if ((pVal->flags & MEM_Subtype) != 0 &&
+	    pVal->subtype == SQL_SUBTYPE_MSGPACK) {
+		const char *value = mp_str(pVal->z);
+		size_t len = strlen(value) + 1;
+		char *result = region_alloc(&fiber()->gc, len);
+		if (result == NULL) {
+			diag_set(OutOfMemory, len, "region_alloc", "result");
+			sqlOomFault(sql_get());
+			return NULL;
+		}
+		memcpy(result, value, len);
+		return result;
+	}
 	if (pVal->flags & (MEM_Blob | MEM_Str)) {
 		if (ExpandBlob(pVal))
 			return 0;
diff --git a/test/sql/types.result b/test/sql/types.result
index 15e359d..95cbd7a 100644
--- a/test/sql/types.result
+++ b/test/sql/types.result
@@ -1794,3 +1794,61 @@ s:select()
 s:drop()
 ---
 ...
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+---
+- row_count: 1
+...
+format = {}
+---
+...
+format[1] = {type = 'integer', name = 'I'}
+---
+...
+format[2] = {type = 'array', name = 'A'}
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {1,2,3}})
+---
+- [1, [1, 2, 3]]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- error: 'Type mismatch: can not convert [1, 2, 3] to scalar'
+...
+s:drop()
+---
+...
+format[2].type = 'map'
+---
+...
+s = box.schema.space.create('T2', {format=format})
+---
+...
+i = s:create_index('ii')
+---
+...
+s:insert({1, {b = 1}})
+---
+- [1, {'b': 1}]
+...
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+---
+- error: 'Type mismatch: can not convert {"b": 1} to scalar'
+...
+s:drop()
+---
+...
+box.execute('DROP TABLE t1;')
+---
+- row_count: 1
+...
diff --git a/test/sql/types.test.lua b/test/sql/types.test.lua
index de1b7ac..190bdd2 100644
--- a/test/sql/types.test.lua
+++ b/test/sql/types.test.lua
@@ -416,3 +416,24 @@ s:insert({1, true, {1, 2}, {a = 3}, 'asd'})
 box.execute('UPDATE t SET b = false WHERE i = 1;')
 s:select()
 s:drop()
+
+--
+-- Make sure that the array/map conversion to scalar error is
+-- displayed correctly.
+--
+box.execute('CREATE TABLE t1(i INT PRIMARY KEY AUTOINCREMENT, a SCALAR);')
+format = {}
+format[1] = {type = 'integer', name = 'I'}
+format[2] = {type = 'array', name = 'A'}
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {1,2,3}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+format[2].type = 'map'
+s = box.schema.space.create('T2', {format=format})
+i = s:create_index('ii')
+s:insert({1, {b = 1}})
+box.execute('INSERT INTO t1(a) SELECT a FROM t2;')
+s:drop()
+box.execute('DROP TABLE t1;')