[tarantool-patches] Re: [PATCH v1 1/2] sql: restrict nullable action definitions

[Kirill Shcherbatov <kshcherbatov@tarantool.org>]

Hi! Thank you for review.

On 13.07.2018 13:26, Vladislav Shpilevoy wrote:
> Hello. See 5 comments below.
> 
> 1. Assertion.
non actual already.

> 2. Please, remove this new action, because
> * it is not really action, but just a flag;
> * it can be replaced with on_conflict_action_MAX or
>    ON_CONFLICT_ACTION_DEFAULT.
Implemented with on_conflict_action_MAX 
> 3. Why do you cast to u8 though nullable_action is enum?
field->nullable_action = onError;

> 4. This still works:
> 5. I do this:
> 
>      box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE)")
> 
> But ON CONFLICT makes no sense for nullable columns.
Included in tests.

=========================================


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           | 83 ++++++++++++++++++++++++++++++++++++++-----
 src/box/sql/parse.y           |  2 +-
 test/sql/on-conflict.result   | 21 +++++++++++
 test/sql/on-conflict.test.lua |  8 +++++
 4 files changed, 105 insertions(+), 9 deletions(-)

diff --git a/src/box/sql/build.c b/src/box/sql/build.c
index 8fba373..b00f4ff 100644
--- a/src/box/sql/build.c
+++ b/src/box/sql/build.c
@@ -653,7 +653,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) {
@@ -701,9 +706,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_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);
+		pParse->rc = SQL_TARANTOOL_ERROR;
+		pParse->nErr++;
+		return;
+	}
+	field->nullable_action = onError;
+	field->is_nullable = action_is_nullable(onError);
 }
 
 /*
@@ -1306,11 +1324,31 @@ convertToWithoutRowidTable(Parse * pParse, Table * pTab)
 	 */
 	if (!db->init.imposterTable) {
 		for (i = 0; i < (int)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;
+			if (pTab->aCol[i].is_primkey == 0)
+				continue;
+			enum on_conflict_action action =
+				pTab->def->fields[i].nullable_action;
+			if (action != on_conflict_action_MAX &&
+			    action != ON_CONFLICT_ACTION_ABORT &&
+			    action != ON_CONFLICT_ACTION_DEFAULT) {
+				const char *action_str =
+					on_conflict_action_strs[
+						pTab->def->fields[i].
+						nullable_action];
+				const char *err_str =
+					tt_sprintf("Cannot define PRIMARY KEY "
+						   "constraint on column %s "
+						   "with on conflict action %s",
+						   pTab->def->fields[i].name,
+						   action_str);
+				diag_set(ClientError, ER_SQL, err_str);
+				pParse->rc = SQL_TARANTOOL_ERROR;
+				pParse->nErr++;
+				return;
 			}
+			pTab->def->fields[i].nullable_action =
+				ON_CONFLICT_ACTION_ABORT;
+			pTab->def->fields[i].is_nullable = false;
 		}
 	}
 
@@ -1739,6 +1777,35 @@ sqlite3EndTable(Parse * pParse,	/* Parse context */
 		}
 	}
 
+	/* 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_MAX) {
+			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 *action_str =
+				on_conflict_action_strs[p->def->fields[i].
+							nullable_action];
+			const char *err_str =
+				tt_sprintf("Cannot define PRIMARY KEY "
+					   "constraint on nullable column %s",
+					   p->def->fields[i].name,
+					   action_str);
+			diag_set(ClientError, ER_SQL, err_str);
+			pParse->rc = SQL_TARANTOOL_ERROR;
+			pParse->nErr++;
+			return;
+		}
+	}
+
 	if (check_on_conflict_replace_entries(p)) {
 		sqlite3ErrorMsg(pParse,
 				"only PRIMARY KEY constraint can "
diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index ac935fd..4894b8a 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.                  {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..5c8131a 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: 'SQL error: Cannot define PRIMARY KEY constraint on nullable column S1'
+...
+box.sql.execute("CREATE TABLE test (a int PRIMARY KEY, b int NULL ON CONFLICT IGNORE);")
+---
+- error: keyword "ON" is reserved
+...
+box.sql.execute("CREATE TABLE test (a int, b int NULL, c int, PRIMARY KEY(a, b, c))")
+---
+- error: 'SQL error: Cannot define PRIMARY KEY constraint on column B with on conflict
+    action none'
+...
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