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 912FC270A7 for ; Thu, 12 Jul 2018 12:34:33 -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 EryZydwavmWU for ; Thu, 12 Jul 2018 12:34:33 -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 458DF2709F for ; Thu, 12 Jul 2018 12:34:33 -0400 (EDT) From: Kirill Shcherbatov Subject: [tarantool-patches] [PATCH v1 1/2] sql: restrict nullable action definitions Date: Thu, 12 Jul 2018 19:34:27 +0300 Message-Id: <3155108e339f80906e3d4a3bd3a0885ed08e8ffc.1531413185.git.kshcherbatov@tarantool.org> In-Reply-To: References: In-Reply-To: References: 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: tarantool-patches@freelists.org Cc: v.shpilevoy@tarantool.org, Kirill Shcherbatov This patch dissallows define multiple "NULL", "NOT NULL" options per column and fixes silent implicit behavior for invalid "NULL PRIMARY KEY" construction. Closes #3473. --- src/box/alter.cc | 1 + src/box/field_def.c | 1 + src/box/field_def.h | 3 ++- src/box/sql/build.c | 46 +++++++++++++++++++++++++++++++++++++++---- src/box/sql/parse.y | 2 +- test/sql/on-conflict.result | 13 ++++++++++++ test/sql/on-conflict.test.lua | 6 ++++++ 7 files changed, 66 insertions(+), 6 deletions(-) diff --git a/src/box/alter.cc b/src/box/alter.cc index 7b6bd1a..3791ac7 100644 --- a/src/box/alter.cc +++ b/src/box/alter.cc @@ -390,6 +390,7 @@ field_def_decode(struct field_def *field, const char **data, "nullable action", fieldno + TUPLE_INDEX_BASE)); } + assert(field->nullable_action != ON_CONFLICT_ACTION_UNDEFINED); if (!((field->is_nullable && field->nullable_action == ON_CONFLICT_ACTION_NONE) || (!field->is_nullable diff --git a/src/box/field_def.c b/src/box/field_def.c index 8dbead6..70a9e46 100644 --- a/src/box/field_def.c +++ b/src/box/field_def.c @@ -46,6 +46,7 @@ const char *field_type_strs[] = { }; const char *on_conflict_action_strs[] = { + /* [ON_CONFLICT_ACTION_UNDEFINED]= */ "undefined", /* [ON_CONFLICT_ACTION_NONE] = */ "none", /* [ON_CONFLICT_ACTION_ROLLBACK] = */ "rollback", /* [ON_CONFLICT_ACTION_ABORT] = */ "abort", diff --git a/src/box/field_def.h b/src/box/field_def.h index 05f80d4..c35e0b8 100644 --- a/src/box/field_def.h +++ b/src/box/field_def.h @@ -60,7 +60,8 @@ enum field_type { }; enum on_conflict_action { - ON_CONFLICT_ACTION_NONE = 0, + ON_CONFLICT_ACTION_UNDEFINED = 0, + ON_CONFLICT_ACTION_NONE, ON_CONFLICT_ACTION_ROLLBACK, ON_CONFLICT_ACTION_ABORT, ON_CONFLICT_ACTION_FAIL, diff --git a/src/box/sql/build.c b/src/box/sql/build.c index 53c20a6..946b10c 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -653,7 +653,7 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) struct field_def *column_def = &p->def->fields[p->def->field_count]; memcpy(column_def, &field_def_default, sizeof(field_def_default)); column_def->name = z; - column_def->nullable_action = ON_CONFLICT_ACTION_NONE; + column_def->nullable_action = ON_CONFLICT_ACTION_UNDEFINED; column_def->is_nullable = true; if (pType->n == 0) { @@ -701,9 +701,22 @@ sqlite3AddNotNull(Parse * pParse, int onError) p = pParse->pNewTable; if (p == 0 || NEVER(p->def->field_count < 1)) return; - p->def->fields[p->def->field_count - 1].nullable_action = (u8)onError; - p->def->fields[p->def->field_count - 1].is_nullable = - action_is_nullable(onError); + struct field_def *field = &p->def->fields[p->def->field_count - 1]; + if (field->nullable_action != ON_CONFLICT_ACTION_UNDEFINED) { + /* Prevent defining nullable_action many times. */ + const char *err_msg = + tt_sprintf("NULL declaration for column '%s' of table " + "'%s' has been already set to '%s'", + field->name, p->def->name, + on_conflict_action_strs[field-> + nullable_action]); + diag_set(ClientError, ER_SQL, err_msg); + pParse->rc = SQL_TARANTOOL_ERROR; + pParse->nErr++; + return; + } + field->nullable_action = (u8)onError; + field->is_nullable = action_is_nullable(onError); } /* @@ -1728,6 +1741,31 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ p->def->id = SQLITE_PAGENO_TO_SPACEID(p->tnum); } + /* Set default on_nullable action if required. */ + for (uint32_t i = 0; i < p->def->field_count; i++) { + if (p->def->fields[i].nullable_action == + ON_CONFLICT_ACTION_UNDEFINED) { + p->def->fields[i].nullable_action = + ON_CONFLICT_ACTION_NONE; + p->def->fields[i].is_nullable = true; + } else if (p->iPKey == (int)i && + p->def->fields[i].nullable_action == + ON_CONFLICT_ACTION_NONE) { + /* + * PRIMARY KEY can not be defined with + * ON_CONFLICT_ACTION_NONE. + */ + const char *err_str = + tt_sprintf("Cannot define PRIMARY KEY " + "constraint on nullable column in " + "table '%s'", p->def->name); + diag_set(ClientError, ER_SQL, err_str); + pParse->rc = SQL_TARANTOOL_ERROR; + pParse->nErr++; + return; + } + } + if (!p->def->opts.is_view) { if ((p->tabFlags & TF_HasPrimaryKey) == 0) { sqlite3ErrorMsg(pParse, diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index ac935fd..6463019 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -276,7 +276,7 @@ ccons ::= DEFAULT id(X). { // In addition to the type name, we also care about the primary key and // UNIQUE constraints. // -ccons ::= NULL onconf. +ccons ::= NULL onconf. {sqlite3AddNotNull(pParse, ON_CONFLICT_ACTION_NONE);} ccons ::= NOT NULL onconf(R). {sqlite3AddNotNull(pParse, R);} ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I). {sqlite3AddPrimaryKey(pParse,0,R,I,Z);} diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result index c0d0de0..bdc2c76 100644 --- a/test/sql/on-conflict.result +++ b/test/sql/on-conflict.result @@ -99,3 +99,16 @@ box.sql.execute('DROP TABLE t1') box.sql.execute('DROP TABLE t2') --- ... +-- +-- gh-3473: Primary key can be declared with NULL. +-- +box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY NOT NULL);') +--- +- error: 'SQL error: NULL declaration for column ''S1'' of table ''TE17'' has been + already set to ''none''' +... +box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY);') +--- +- error: 'SQL error: Cannot define PRIMARY KEY constraint on nullable column in table + ''TE17''' +... diff --git a/test/sql/on-conflict.test.lua b/test/sql/on-conflict.test.lua index b6d92f7..94eb84f 100644 --- a/test/sql/on-conflict.test.lua +++ b/test/sql/on-conflict.test.lua @@ -38,3 +38,9 @@ box.sql.execute('DROP TABLE p') box.sql.execute('DROP TABLE e') box.sql.execute('DROP TABLE t1') box.sql.execute('DROP TABLE t2') + +-- +-- gh-3473: Primary key can be declared with NULL. +-- +box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY NOT NULL);') +box.sql.execute('CREATE TABLE te17 (s1 INT NULL PRIMARY KEY);') -- 2.7.4