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 DCFD42172C for ; Wed, 18 Jul 2018 12:52:26 -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 HqNQfCBB8i7l for ; Wed, 18 Jul 2018 12:52:26 -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 6560221469 for ; Wed, 18 Jul 2018 12:52:26 -0400 (EDT) From: Kirill Shcherbatov Subject: [tarantool-patches] [PATCH v1 1/3] sql: restrict nullable action definitions Date: Wed, 18 Jul 2018 19:52:20 +0300 Message-Id: <33314a00ce2e0602e0a0fa7e30257ff03e3eed16.1531932662.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: korablev@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/sql/build.c | 98 +++++++++++++++++++++++++++++++------------ src/box/sql/parse.y | 9 +++- src/box/sql/sqliteInt.h | 17 +++++++- test/sql/on-conflict.result | 21 ++++++++++ test/sql/on-conflict.test.lua | 8 ++++ 5 files changed, 123 insertions(+), 30 deletions(-) diff --git a/src/box/sql/build.c b/src/box/sql/build.c index a64d723..a023b1e 100644 --- a/src/box/sql/build.c +++ b/src/box/sql/build.c @@ -656,7 +656,12 @@ 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; + /* + * Marker on_conflict_action_MAX is used to detect + * attempts to define NULL multiple time or to detect + * invalid primary key definition. + */ + column_def->nullable_action = on_conflict_action_MAX; column_def->is_nullable = true; if (pType->n == 0) { @@ -691,22 +696,29 @@ sqlite3AddColumn(Parse * pParse, Token * pName, Token * pType) pParse->constraintName.n = 0; } -/* - * This routine is called by the parser while in the middle of - * parsing a CREATE TABLE statement. A "NOT NULL" constraint has - * been seen on a column. This routine sets the notNull flag on - * the column currently under construction. - */ void -sqlite3AddNotNull(Parse * pParse, int onError) +sql_column_nullable_action_add(struct Parse *parser, + enum on_conflict_action nullable_action) { - Table *p; - p = pParse->pNewTable; - if (p == 0 || NEVER(p->def->field_count < 1)) + struct Table *p = parser->pNewTable; + if (p == NULL || 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_MAX) { + /* 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); + parser->rc = SQL_TARANTOOL_ERROR; + parser->nErr++; + return; + } + field->nullable_action = nullable_action; + field->is_nullable = action_is_nullable(nullable_action); } /* @@ -1215,6 +1227,24 @@ createTableStmt(sqlite3 * db, Table * p) return zStmt; } +static int +field_def_create_for_pk(struct Parse *parser, struct field_def *field, + const char *space_name) +{ + if (field->nullable_action != ON_CONFLICT_ACTION_ABORT && + field->nullable_action != ON_CONFLICT_ACTION_DEFAULT && + field->nullable_action != on_conflict_action_MAX) { + diag_set(ClientError, ER_NULLABLE_PRIMARY, space_name); + parser->rc = SQL_TARANTOOL_ERROR; + parser->nErr++; + return -1; + } else if (field->nullable_action == on_conflict_action_MAX) { + field->nullable_action = ON_CONFLICT_ACTION_ABORT; + field->is_nullable = false; + } + return 0; +} + /* * This routine runs at the end of parsing a CREATE TABLE statement. * The job of this routine is to convert both @@ -1232,18 +1262,8 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab) { Index *pPk; sqlite3 *db = pParse->db; - - /* Mark every PRIMARY KEY column as NOT NULL (except for imposter tables) - */ - if (!db->init.imposterTable) { - for (uint32_t i = 0; i < pTab->def->field_count; i++) { - if (pTab->aCol[i].is_primkey) { - pTab->def->fields[i].nullable_action - = ON_CONFLICT_ACTION_ABORT; - pTab->def->fields[i].is_nullable = false; - } - } - } + struct field_def *fields = pTab->def->fields; + const char *space_name = pTab->def->name; /* Locate the PRIMARY KEY index. Or, if this table was originally * an INTEGER PRIMARY KEY table, create a new PRIMARY KEY index. @@ -1251,7 +1271,10 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab) if (pTab->iPKey >= 0) { ExprList *pList; Token ipkToken; - sqlite3TokenInit(&ipkToken, pTab->def->fields[pTab->iPKey].name); + struct field_def *field = &fields[pTab->iPKey]; + if (field_def_create_for_pk(pParse, field, space_name) != 0) + return; + sqlite3TokenInit(&ipkToken, field->name); pList = sql_expr_list_append(pParse->db, NULL, sqlite3ExprAlloc(db, TK_ID, &ipkToken, 0)); @@ -1268,6 +1291,16 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab) pTab->iPKey = -1; } else { pPk = sqlite3PrimaryKeyIndex(pTab); + assert(pPk != NULL); + struct key_def *key_def = pPk->def->key_def; + assert(key_def != NULL); + for (uint32_t i = 0; i < key_def->part_count; i++) { + struct field_def *field = + &fields[key_def->parts[i].fieldno]; + if (field_def_create_for_pk(pParse, field, + space_name) != 0) + return; + } } assert(pPk != 0); } @@ -1654,6 +1687,17 @@ sqlite3EndTable(Parse * pParse, /* Parse context */ return; } else { convertToWithoutRowidTable(pParse, p); + if (pParse->nErr > 0) + return; + } + } + + /* Set default on_nullable action if required. */ + struct field_def *field = p->def->fields; + for (uint32_t i = 0; i < p->def->field_count; ++i, ++field) { + if (field->nullable_action == on_conflict_action_MAX) { + field->nullable_action = ON_CONFLICT_ACTION_NONE; + field->is_nullable = true; } } diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y index e8a36e4..45321a7 100644 --- a/src/box/sql/parse.y +++ b/src/box/sql/parse.y @@ -276,8 +276,13 @@ ccons ::= DEFAULT id(X). { // In addition to the type name, we also care about the primary key and // UNIQUE constraints. // -ccons ::= NULL onconf. -ccons ::= NOT NULL onconf(R). {sqlite3AddNotNull(pParse, R);} +ccons ::= NULL onconf(R). { + sql_column_nullable_action_add(pParse, ON_CONFLICT_ACTION_NONE); + /* Trigger nullability mismatch error if required. */ + if (R != ON_CONFLICT_ACTION_DEFAULT) + sql_column_nullable_action_add(pParse, R); +} +ccons ::= NOT NULL onconf(R). {sql_column_nullable_action_add(pParse, R);} ccons ::= PRIMARY KEY sortorder(Z) onconf(R) autoinc(I). {sqlite3AddPrimaryKey(pParse,0,R,I,Z);} ccons ::= UNIQUE onconf(R). {sql_create_index(pParse,0,0,0,R,0,0, diff --git a/src/box/sql/sqliteInt.h b/src/box/sql/sqliteInt.h index 54661cb..9f6e97e 100644 --- a/src/box/sql/sqliteInt.h +++ b/src/box/sql/sqliteInt.h @@ -3506,7 +3506,22 @@ Table *sqlite3ResultSetOfSelect(Parse *, Select *); Index *sqlite3PrimaryKeyIndex(Table *); void sqlite3StartTable(Parse *, Token *, int); void sqlite3AddColumn(Parse *, Token *, Token *); -void sqlite3AddNotNull(Parse *, int); + +/** + * This routine is called by the parser while in the middle of + * parsing a CREATE TABLE statement. A "NOT NULL" constraint has + * been seen on a column. This routine sets the is_nullable flag + * on the column currently under construction. + * If nullable_action has been already set, this function raises + * an error. + * + * @param parser SQL Parser object. + * @param nullable_action on_conflict_action value. + */ +void +sql_column_nullable_action_add(struct Parse *parser, + enum on_conflict_action nullable_action); + void sqlite3AddPrimaryKey(Parse *, ExprList *, int, int, enum sort_order); /** diff --git a/test/sql/on-conflict.result b/test/sql/on-conflict.result index 4080648..a15acca 100644 --- a/test/sql/on-conflict.result +++ b/test/sql/on-conflict.result @@ -99,3 +99,24 @@ 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: Primary index of the space 'TE17' can not contain nullable parts +... +box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE);") +--- +- error: 'SQL error: NULL declaration for column ''B'' of table ''TEST'' has been + already set to ''none''' +... +box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))") +--- +- error: Primary index of the space 'TEST' can not contain nullable parts +... diff --git a/test/sql/on-conflict.test.lua b/test/sql/on-conflict.test.lua index b6d92f7..5ae6b0c 100644 --- a/test/sql/on-conflict.test.lua +++ b/test/sql/on-conflict.test.lua @@ -38,3 +38,11 @@ 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);') +box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE);") +box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))") -- 2.7.4