[tarantool-patches] Re: [PATCH] sql: prohibit duplication of FK action clause

[Kirill Yukhin <kyukhin@tarantool.org>]

Hello Nikita,

On 13 Feb 22:43, n.pettik wrote:
> 
> > On 13 Feb 2019, at 09:08, Kirill Yukhin <kyukhin@tarantool.org> wrote:
> > 
> > The patch prohibits duplication of action clause for foreign
> > key constraints. It is now syntax error to specify, say,
> > ON UPDATE clause for the same FK twice.
> > The patch also remove ON INSERT caluse at all, since it
> > ultimately is no-op and leads to nothing but confusion.
> > 
> > Closes #3475
> > ---
> > https://github.com/tarantool/tarantool/issues/3475
> > https://github.com/tarantool/tarantool/commits/kyukhin/gh-3475-fk-duplicate-action-clause
> 
> Travis status is completely negative. Please, make sure it is green
> before sending patch.

Travis is completely positive w/ this patch. Please, see [1].
Failing case is the branch after rebase.
Anyway, rebased version is fixed as well.

> > diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
> > index 32ef685..7938afd 100644
> > --- a/src/box/sql/parse.y
> > +++ b/src/box/sql/parse.y
> > @@ -283,10 +283,16 @@ autoinc(X) ::= AUTOINCR.  {X = 1;}
> > //
> > %type refargs {int}
> > refargs(A) ::= .                  { A = FKEY_NO_ACTION; }
> > -refargs(A) ::= refargs(A) refarg(Y). { A = (A & ~Y.mask) | Y.value; }
> > +refargs(A) ::= refargs(A) refarg(Y). { if ((A & Y.mask) == 0) {
> > +                                           A = (A & ~Y.mask) | Y.value;
> > +                                       } else {
> > +                                           sqlite3ErrorMsg(pParse,
> > +                                                           "near \"%T\": duplicate FK action clause.",
> > +                                                           &pParse->sLastToken);
> > +                                       }
> > +                                     }
> 
> Please, fix indentation, it looks extremely awful.
> Lets use 2 spaces as a default indent (as almost
> everywhere in parse.y).

I've replaced 4 spaces w/ 2 for tab.

> > diff --git a/test/sql/gh-3475-fk-duplicate-clause.test.lua b/test/sql/gh-3475-fk-duplicate-clause.test.lua
> > new file mode 100644
> > index 0000000..447c104
> > --- /dev/null
> > +++ b/test/sql/gh-3475-fk-duplicate-clause.test.lua
> > @@ -0,0 +1,13 @@
> > +test_run = require('test_run').new()
> > +engine = test_run:get_cfg('engine')
> > +box.sql.execute('pragma sql_default_engine=\''..engine..'\’')
> 
> Should this test be engine-dependent?

I'd test everything on boath engines. But you're reviewer, so removed.

> > +
> > +box.cfg{}
> > +
> > +box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY, b INT UNIQUE)")
> > +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON UPDATE CASCADE ON UPDATE CASCADE)")
> > +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE CASCADE)")
> > +box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE)”)
> 
> What about next cases:
> 
> box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE RESTRICT)”)
> box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE ON DELETE RESTRICT )”)
> 
> Sorry, can’t check myself since can’t build your branch.

I've added them as well.

[1] - https://travis-ci.org/tarantool/tarantool/builds/492546000

--
Regards, Kirill Yukhin

diff --git a/src/box/sql/parse.y b/src/box/sql/parse.y
index 8c8fdc2..7ee8dc2 100644
--- a/src/box/sql/parse.y
+++ b/src/box/sql/parse.y
@@ -284,11 +284,11 @@ autoinc(X) ::= AUTOINCR.  {X = 1;}
 %type refargs {int}
 refargs(A) ::= .                  { A = FKEY_NO_ACTION; }
 refargs(A) ::= refargs(A) refarg(Y). { if ((A & Y.mask) == 0) {
-                                           A = (A & ~Y.mask) | Y.value;
+                                         A = (A & ~Y.mask) | Y.value;
                                        } else {
-                                           sqlite3ErrorMsg(pParse,
-                                                           "near \"%T\": duplicate FK action clause.",
-                                                           &pParse->sLastToken);
+                                         sqlErrorMsg(pParse,
+                                                     "near \"%T\": duplicate FK action clause.",
+                                                     &pParse->sLastToken);
                                        }
                                      }
 %type refarg {struct {int value; int mask;}}
diff --git a/test/sql/engine.cfg b/test/sql/engine.cfg
index 0007d8d..4ec39ac 100644
--- a/test/sql/engine.cfg
+++ b/test/sql/engine.cfg
@@ -1,4 +1,5 @@
 {
+    "gh-3475-fk-duplicate-clause.test.lua": {},
     "*": {
         "memtx": {"engine": "memtx"},
         "vinyl": {"engine": "vinyl"}
diff --git a/test/sql/gh-3475-fk-duplicate-clause.result b/test/sql/gh-3475-fk-duplicate-clause.result
index 7bcd62b..5ce556f 100644
--- a/test/sql/gh-3475-fk-duplicate-clause.result
+++ b/test/sql/gh-3475-fk-duplicate-clause.result
@@ -1,12 +1,6 @@
 test_run = require('test_run').new()
 ---
 ...
-engine = test_run:get_cfg('engine')
----
-...
-box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
----
-...
 box.cfg{}
 ---
 ...
@@ -21,6 +15,14 @@ box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DE
 ---
 - error: 'near ")": duplicate FK action clause.'
 ...
+box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE RESTRICT)")
+---
+- error: 'near ")": duplicate FK action clause.'
+...
+box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE ON DELETE RESTRICT )")
+---
+- error: 'near ")": duplicate FK action clause.'
+...
 box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE)")
 ---
 ...
diff --git a/test/sql/gh-3475-fk-duplicate-clause.test.lua b/test/sql/gh-3475-fk-duplicate-clause.test.lua
index 447c104..2c2b55c 100644
--- a/test/sql/gh-3475-fk-duplicate-clause.test.lua
+++ b/test/sql/gh-3475-fk-duplicate-clause.test.lua
@@ -1,12 +1,12 @@
 test_run = require('test_run').new()
-engine = test_run:get_cfg('engine')
-box.sql.execute('pragma sql_default_engine=\''..engine..'\'')
 
 box.cfg{}
 
 box.sql.execute("CREATE TABLE t1(a INT PRIMARY KEY, b INT UNIQUE)")
 box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON UPDATE CASCADE ON UPDATE CASCADE)")
 box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE CASCADE)")
+box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON DELETE RESTRICT)")
+box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE ON DELETE RESTRICT )")
 box.sql.execute("CREATE TABLE t2(a INT PRIMARY KEY, b INT REFERENCES t1(b) ON DELETE CASCADE ON UPDATE CASCADE)")
 
 box.sql.execute("DROP TABLE t2")