From 146f7b5421a2a8f5e7e7f531db78ca53c2cb4eec Mon Sep 17 00:00:00 2001 From: Dejan Mircevski Date: Fri, 3 Sep 2021 21:45:55 -0400 Subject: [PATCH] cql3: Reject updates with NULL key values We were silently ignoring INSERTs with NULL values for primary-key columns, which Cassandra rejects. Fix it by rejecting any modification_statement that would operate on empty partition or clustering range. This is the most direct fix, because range and slice are calculated in one place for all modification statements. It covers not only NULL cases, but also impossible restrictions like c>0 AND c<0. Unfortunately, Cassandra doesn't treat all modification statements consistently, so this fix cannot fully match its behavior. We err on the side of tolerance, accepting some DELETE statements that Cassandra rejects. We add a TODO for rejecting such DELETEs later. Fixes #7852. Tests: unit (dev), cql-pytest against Cassandra 4.0 Signed-off-by: Dejan Mircevski Closes #9286 (cherry picked from commit 1fdaeca7d0406371d51a0f956ee05e698e264552) --- cql3/statements/modification_statement.cc | 6 +++++ .../entities/frozen_collections_test.py | 3 --- test/cql-pytest/test_null.py | 24 +++++++++++++++---- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/cql3/statements/modification_statement.cc b/cql3/statements/modification_statement.cc index 65e3cf8395..bfef68c4c9 100644 --- a/cql3/statements/modification_statement.cc +++ b/cql3/statements/modification_statement.cc @@ -139,7 +139,13 @@ modification_statement::get_mutations(service::storage_proxy& proxy, const query auto cl = options.get_consistency(); auto json_cache = maybe_prepare_json_cache(options); auto keys = build_partition_keys(options, json_cache); + if (keys.empty()) { + throw exceptions::invalid_request_exception("Invalid partition key in a modification statement"); + } auto ranges = create_clustering_ranges(options, json_cache); + if (ranges.empty() && !type.is_delete() /* DELETE is allowed on empty ranges (TODO: but not on WHERE ck=NULL).*/) { + throw exceptions::invalid_request_exception("Invalid clustering key in a modification statement"); + } auto f = make_ready_future(s); if (is_counter()) { diff --git a/test/cql-pytest/cassandra_tests/validation/entities/frozen_collections_test.py b/test/cql-pytest/cassandra_tests/validation/entities/frozen_collections_test.py index a98aeab606..a64f8ea0e9 100644 --- a/test/cql-pytest/cassandra_tests/validation/entities/frozen_collections_test.py +++ b/test/cql-pytest/cassandra_tests/validation/entities/frozen_collections_test.py @@ -86,15 +86,12 @@ def do_test_partition_key_usage(cql, test_keyspace, typ, v1, v2, v3, v4): [v2, 0], [v4, 0]) -@pytest.mark.xfail(reason="fails just because of null key, issue #7852") def test_partition_key_usage_set(cql, test_keyspace): do_test_partition_key_usage(cql, test_keyspace, "set", set(), {1, 2, 3}, {4, 5, 6}, {7, 8, 9}) -@pytest.mark.xfail(reason="fails just because of null key, issue #7852") def test_partition_key_usage_list(cql, test_keyspace): do_test_partition_key_usage(cql, test_keyspace, "list", [], [1, 2, 3], [4, 5, 6], [7, 8, 9]) -@pytest.mark.xfail(reason="fails just because of null key, issue #7852") def test_partition_key_usage_map(cql, test_keyspace): do_test_partition_key_usage(cql, test_keyspace, "map", {}, {1: 10, 2: 20, 3: 30}, {4: 40, 5: 50, 6: 60}, {7: 70, 8: 80, 9: 90}) diff --git a/test/cql-pytest/test_null.py b/test/cql-pytest/test_null.py index 8d2ab8f7da..5bcc1f0922 100644 --- a/test/cql-pytest/test_null.py +++ b/test/cql-pytest/test_null.py @@ -48,20 +48,19 @@ def test_insert_missing_key(cql, table1): # A null key, like a missing one, is also not allowed. # This reproduces issue #7852. -@pytest.mark.xfail(reason="issue #7852") def test_insert_null_key(cql, table1): s = random_string() - with pytest.raises(InvalidRequest, match='null value'): + with pytest.raises(InvalidRequest, match=re.compile('Invalid (null|clustering)')): cql.execute(f"INSERT INTO {table1} (p,c) VALUES ('{s}', null)") - with pytest.raises(InvalidRequest, match='null value'): + with pytest.raises(InvalidRequest, match=re.compile('Invalid (null|partition)')): cql.execute(f"INSERT INTO {table1} (p,c) VALUES (null, '{s}')") # Try the same thing with prepared statement, where a "None" stands for # a null. Note that this is completely different from UNSET_VALUE - only # with the latter should the insertion be ignored. stmt = cql.prepare(f"INSERT INTO {table1} (p,c) VALUES (?, ?)") - with pytest.raises(InvalidRequest, match='null value'): + with pytest.raises(InvalidRequest, match=re.compile('Invalid (null|clustering)')): cql.execute(stmt, [s, None]) - with pytest.raises(InvalidRequest, match='null value'): + with pytest.raises(InvalidRequest, match=re.compile('Invalid (null|partition)')): cql.execute(stmt, [None, s]) def test_primary_key_in_null(cql, table1): @@ -80,3 +79,18 @@ def test_regular_column_in_null(scylla_only, cql, table1): cql.execute(f"INSERT INTO {table1} (p,c) VALUES ('p', 'c')") with pytest.raises(InvalidRequest, match='null value'): cql.execute(cql.prepare(f"SELECT v FROM {table1} WHERE v IN ? ALLOW FILTERING"), [None]) + +# Though nonsensical, this operation is allowed by Cassandra. Ensure we allow it, too. +def test_delete_impossible_clustering_range(cql, table1): + cql.execute(f"DELETE FROM {table1} WHERE p='p' and c<'a' and c>'a'") + +@pytest.mark.xfail(reason="Unimplemented for clustering key") +def test_delete_null_key(cql, table1): + with pytest.raises(InvalidRequest, match=re.compile('Invalid (null|partition)')): + cql.execute(f"DELETE FROM {table1} WHERE p=null") + with pytest.raises(InvalidRequest, match=re.compile('Invalid (null|partition)')): + cql.execute(cql.prepare(f"DELETE FROM {table1} WHERE p=?"), [None]) + with pytest.raises(InvalidRequest, match=re.compile('Invalid (null|clustering)')): + cql.execute(f"DELETE FROM {table1} WHERE p='p' AND c=null") + with pytest.raises(InvalidRequest, match=re.compile('Invalid (null|clustering)')): + cql.execute(cql.prepare(f"DELETE FROM {table1} WHERE p='p' AND c=?"), [None])