Mailing-List: contact commits-help@cassandra.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cassandra.apache.org
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
Content-Transfer-Encoding: 7bit
From: slebresne@apache.org
To: commits@cassandra.apache.org
Date: Mon, 07 Nov 2016 13:11:15 -0000
Message-Id: <019487193d6d48b99cca28b224d60966@git.apache.org>
Subject: [1/6] cassandra git commit: Batch with multiple conditional updates
 for the same partition causes AssertionError
archived-at: Mon, 07 Nov 2016 13:11:18 -0000

Repository: cassandra
Updated Branches:
  refs/heads/cassandra-3.0 f7aa37142 -> 78fdfe233
  refs/heads/cassandra-3.X 9b4297465 -> 2b4567313
  refs/heads/trunk 0e132f4eb -> c47d395d8


Batch with multiple conditional updates for the same partition causes AssertionError

patch by Sylvain Lebresne; reviewed by Benjamin Lerer for CASSANDRA-12867


Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo
Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/78fdfe23
Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/78fdfe23
Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/78fdfe23

Branch: refs/heads/cassandra-3.0
Commit: 78fdfe2336048ba37a8b4c321ee4ab5d7bfb1357
Parents: f7aa371
Author: Sylvain Lebresne <sylvain@datastax.com>
Authored: Wed Nov 2 14:47:42 2016 +0100
Committer: Sylvain Lebresne <sylvain@datastax.com>
Committed: Mon Nov 7 14:09:22 2016 +0100

----------------------------------------------------------------------
 CHANGES.txt                                     |   1 +
 .../cql3/statements/CQL3CasRequest.java         |  27 ++++-
 .../operations/InsertUpdateIfConditionTest.java | 104 +++++++++++++++++++
 3 files changed, 127 insertions(+), 5 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cassandra/blob/78fdfe23/CHANGES.txt
----------------------------------------------------------------------
diff --git a/CHANGES.txt b/CHANGES.txt
index f708602..1d2c8f3 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -1,4 +1,5 @@
 3.0.10
+ * Batch with multiple conditional updates for the same partition causes AssertionError (CASSANDRA-12867)
  * Make AbstractReplicationStrategy extendable from outside its package (CASSANDRA-12788)
  * Fix CommitLogTest.testDeleteIfNotDirty (CASSANDRA-12854)
  * Don't tell users to turn off consistent rangemovements during rebuild. (CASSANDRA-12296)

http://git-wip-us.apache.org/repos/asf/cassandra/blob/78fdfe23/src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java
----------------------------------------------------------------------
diff --git a/src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java b/src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java
index cf4110c..d9e8796 100644
--- a/src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java
+++ b/src/java/org/apache/cassandra/cql3/statements/CQL3CasRequest.java
@@ -93,12 +93,29 @@ public class CQL3CasRequest implements CASRequest
     {
         assert condition instanceof ExistCondition || condition instanceof NotExistCondition;
         RowCondition previous = getConditionsForRow(clustering);
-        if (previous != null && !(previous.getClass().equals(condition.getClass())))
+        if (previous != null)
         {
-            // these should be prevented by the parser, but it doesn't hurt to check
-            throw (previous instanceof NotExistCondition || previous instanceof ExistCondition)
-                ? new InvalidRequestException("Cannot mix IF EXISTS and IF NOT EXISTS conditions for the same row")
-                : new InvalidRequestException("Cannot mix IF conditions and IF " + (isNotExist ? "NOT " : "") + "EXISTS for the same row");
+            if (previous.getClass().equals(condition.getClass()))
+            {
+                // We can get here if a BATCH has 2 different statements on the same row with the same "exist" condition.
+                // For instance (assuming 'k' is the full PK):
+                //   BEGIN BATCH
+                //      INSERT INTO t(k, v1) VALUES (0, 'foo') IF NOT EXISTS;
+                //      INSERT INTO t(k, v2) VALUES (0, 'bar') IF NOT EXISTS;
+                //   APPLY BATCH;
+                // Of course, those can be trivially rewritten by the user as a single INSERT statement, but we still don't
+                // want this to be a problem (see #12867 in particular), so we simply return (the condition itself has
+                // already be set).
+                assert hasExists; // We shouldn't have a previous condition unless hasExists has been set already.
+                return;
+            }
+            else
+            {
+                // these should be prevented by the parser, but it doesn't hurt to check
+                throw (previous instanceof NotExistCondition || previous instanceof ExistCondition)
+                    ? new InvalidRequestException("Cannot mix IF EXISTS and IF NOT EXISTS conditions for the same row")
+                    : new InvalidRequestException("Cannot mix IF conditions and IF " + (isNotExist ? "NOT " : "") + "EXISTS for the same row");
+            }
         }
 
         setConditionsForRow(clustering, condition);

http://git-wip-us.apache.org/repos/asf/cassandra/blob/78fdfe23/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java
----------------------------------------------------------------------
diff --git a/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java b/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java
index 352100e..40db977 100644
--- a/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java
+++ b/test/unit/org/apache/cassandra/cql3/validation/operations/InsertUpdateIfConditionTest.java
@@ -1421,4 +1421,108 @@ public class InsertUpdateIfConditionTest extends CQLTester
         assertRows(execute("SELECT * FROM %s WHERE a = 7"),
                    row(7, 7, null, null, 7));
     }
+
+    /**
+     * Test for CASSANDRA-12060, using a table without clustering.
+     */
+    @Test
+    public void testMultiExistConditionOnSameRowNoClustering() throws Throwable
+    {
+        createTable("CREATE TABLE %s (k int PRIMARY KEY, v1 text, v2 text)");
+
+        // Multiple inserts on the same row with not exist conditions
+        assertRows(execute("BEGIN BATCH "
+                           + "INSERT INTO %1$s (k, v1) values (0, 'foo') IF NOT EXISTS; "
+                           + "INSERT INTO %1$s (k, v2) values (0, 'bar') IF NOT EXISTS; "
+                           + "APPLY BATCH"),
+                   row(true));
+
+        assertRows(execute("SELECT * FROM %s WHERE k = 0"), row(0, "foo", "bar"));
+
+        // Same, but both insert on the same column: doing so would almost surely be a user error, but that's the
+        // original case reported in #12867, so being thorough.
+        assertRows(execute("BEGIN BATCH "
+                           + "INSERT INTO %1$s (k, v1) values (1, 'foo') IF NOT EXISTS; "
+                           + "INSERT INTO %1$s (k, v1) values (1, 'bar') IF NOT EXISTS; "
+                           + "APPLY BATCH"),
+                   row(true));
+
+        // As all statement gets the same timestamp, the biggest value ends up winning, so that's "foo"
+        assertRows(execute("SELECT * FROM %s WHERE k = 1"), row(1, "foo", null));
+
+        // Multiple deletes on the same row with exists conditions (note that this is somewhat non-sensical, one of the
+        // delete is redundant, we're just checking it doesn't break something)
+        assertRows(execute("BEGIN BATCH "
+                           + "DELETE FROM %1$s WHERE k = 0 IF EXISTS; "
+                           + "DELETE FROM %1$s WHERE k = 0 IF EXISTS; "
+                           + "APPLY BATCH"),
+                   row(true));
+
+        assertEmpty(execute("SELECT * FROM %s WHERE k = 0"));
+
+        // Validate we can't mix different type of conditions however
+        assertInvalidMessage("Cannot mix IF EXISTS and IF NOT EXISTS conditions for the same row",
+                             "BEGIN BATCH "
+                           + "INSERT INTO %1$s (k, v1) values (1, 'foo') IF NOT EXISTS; "
+                           + "DELETE FROM %1$s WHERE k = 1 IF EXISTS; "
+                           + "APPLY BATCH");
+
+        assertInvalidMessage("Cannot mix IF conditions and IF NOT EXISTS for the same row",
+                             "BEGIN BATCH "
+                             + "INSERT INTO %1$s (k, v1) values (1, 'foo') IF NOT EXISTS; "
+                             + "UPDATE %1$s SET v2 = 'bar' WHERE k = 1 IF v1 = 'foo'; "
+                             + "APPLY BATCH");
+    }
+
+    /**
+     * Test for CASSANDRA-12060, using a table with clustering.
+     */
+    @Test
+    public void testMultiExistConditionOnSameRowClustering() throws Throwable
+    {
+        createTable("CREATE TABLE %s (k int, t int, v1 text, v2 text, PRIMARY KEY (k, t))");
+
+        // Multiple inserts on the same row with not exist conditions
+        assertRows(execute("BEGIN BATCH "
+                           + "INSERT INTO %1$s (k, t, v1) values (0, 0, 'foo') IF NOT EXISTS; "
+                           + "INSERT INTO %1$s (k, t, v2) values (0, 0, 'bar') IF NOT EXISTS; "
+                           + "APPLY BATCH"),
+                   row(true));
+
+        assertRows(execute("SELECT * FROM %s WHERE k = 0"), row(0, 0, "foo", "bar"));
+
+        // Same, but both insert on the same column: doing so would almost surely be a user error, but that's the
+        // original case reported in #12867, so being thorough.
+        assertRows(execute("BEGIN BATCH "
+                           + "INSERT INTO %1$s (k, t, v1) values (1, 0, 'foo') IF NOT EXISTS; "
+                           + "INSERT INTO %1$s (k, t, v1) values (1, 0, 'bar') IF NOT EXISTS; "
+                           + "APPLY BATCH"),
+                   row(true));
+
+        // As all statement gets the same timestamp, the biggest value ends up winning, so that's "foo"
+        assertRows(execute("SELECT * FROM %s WHERE k = 1"), row(1, 0, "foo", null));
+
+        // Multiple deletes on the same row with exists conditions (note that this is somewhat non-sensical, one of the
+        // delete is redundant, we're just checking it doesn't break something)
+        assertRows(execute("BEGIN BATCH "
+                           + "DELETE FROM %1$s WHERE k = 0 AND t = 0 IF EXISTS; "
+                           + "DELETE FROM %1$s WHERE k = 0 AND t = 0 IF EXISTS; "
+                           + "APPLY BATCH"),
+                   row(true));
+
+        assertEmpty(execute("SELECT * FROM %s WHERE k = 0"));
+
+        // Validate we can't mix different type of conditions however
+        assertInvalidMessage("Cannot mix IF EXISTS and IF NOT EXISTS conditions for the same row",
+                             "BEGIN BATCH "
+                             + "INSERT INTO %1$s (k, t, v1) values (1, 0, 'foo') IF NOT EXISTS; "
+                             + "DELETE FROM %1$s WHERE k = 1 AND t = 0 IF EXISTS; "
+                             + "APPLY BATCH");
+
+        assertInvalidMessage("Cannot mix IF conditions and IF NOT EXISTS for the same row",
+                             "BEGIN BATCH "
+                             + "INSERT INTO %1$s (k, t, v1) values (1, 0, 'foo') IF NOT EXISTS; "
+                             + "UPDATE %1$s SET v2 = 'bar' WHERE k = 1 AND t = 0 IF v1 = 'foo'; "
+                             + "APPLY BATCH");
+    }
 }