From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTP id 5093724CC7 for ; Sat, 27 Jul 2019 06:16:48 -0400 (EDT) Received: from turing.freelists.org ([127.0.0.1]) by localhost (turing.freelists.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id Wv5lUEAUH0mZ for ; Sat, 27 Jul 2019 06:16:48 -0400 (EDT) Received: from smtpng3.m.smailru.net (smtpng3.m.smailru.net [94.100.177.149]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by turing.freelists.org (Avenir Technologies Mail Multiplex) with ESMTPS id 8A1A424B31 for ; Sat, 27 Jul 2019 06:16:47 -0400 (EDT) Date: Sat, 27 Jul 2019 13:16:44 +0300 From: Mergen Imeev Subject: [tarantool-patches] Re: [PATCH v2 2/2] sql: fix error in case ARRAY/MAP converted to SCALAR Message-ID: <20190727101643.GA10982@tarantool.org> References: <67e721f4818f51861383cdf71cc88c63e45ab450.1563955619.git.imeevma@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: tarantool-patches-bounce@freelists.org Errors-to: tarantool-patches-bounce@freelists.org Reply-To: tarantool-patches@freelists.org List-Help: List-Unsubscribe: List-software: Ecartis version 1.0.0 List-Id: tarantool-patches List-Subscribe: List-Owner: List-post: List-Archive: To: "n.pettik" Cc: tarantool-patches@freelists.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 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;')