Copilot commented on code in PR #4596:
URL: https://github.com/apache/cassandra/pull/4596#discussion_r2886100770
##########
test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java:
##########
@@ -781,6 +781,27 @@ public void testRowIndexSizeWarnEqAbort()
DatabaseDescriptor.applyReadThresholdsValidations(conf);
}
+ @Test
+ public void testPreparedStatementsRequireParametersEnabled() {
+ boolean originalValue =
DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled();
+ try
+ {
+ Assert.assertFalse("Default value of prepared_statement_enabled
must be false", originalValue);
Review Comment:
The assertion message says "prepared_statement_enabled" but the
config/property name is `prepared_statements_require_parameters_enabled`.
Updating the message will make failures easier to interpret.
```suggestion
Assert.assertFalse("Default value of
prepared_statements_require_parameters_enabled must be false", originalValue);
```
##########
test/unit/org/apache/cassandra/tools/nodetool/GuardrailsConfigCommandsTest.java:
##########
@@ -208,6 +208,7 @@ private Set<String> getConfigFieldNames()
"group_by_enabled true\n" +
"intersect_filtering_query_enabled true\n" +
"intersect_filtering_query_warned true\n" +
+ "prepared_statements_require_parameters_enabled false\n" +
"non_partition_restricted_index_query_enabled true\n" +
Review Comment:
`ALL_FLAGS_GETTER_OUTPUT` includes
`prepared_statements_require_parameters_enabled`, but the current
GuardrailsMBean accessor names would generate `misprepared_statements_enabled`
in nodetool output. Additionally, the inserted line is out of sort order
(nodetool output is sorted by guardrail name, so `non_partition...` should
appear before `prepared...`). Update the expected output (and/or rename the
MBean methods) so the printed flag names and ordering match what
`GuardrailsConfigCommand` produces.
```suggestion
"non_partition_restricted_index_query_enabled true\n" +
"misprepared_statements_enabled false\n" +
```
##########
conf/cassandra.yaml:
##########
@@ -2536,6 +2536,14 @@ drop_compact_storage_enabled: false
# This would also apply to system keyspaces.
# maximum_replication_factor_warn_threshold: -1
# maximum_replication_factor_fail_threshold: -1
+#
+# Guardrail to warn or fail when a statement is prepared without bind markers
(parameters).
+# This prevents the Prepared Statement Cache from being flooded with unique
entries caused
+# by hardcoded literals.
+# When true, the server rejects the preparation of any statement that does not
contain at least one parameter.
+# When false, the server only emits a warning.
+# Defaults to false.
+# prepared_statements_require_parameters_enabled: false
Review Comment:
PR description mentions the new flag defaults to true for backward
compatibility, but the added config field
(`prepared_statements_require_parameters_enabled`) defaults to `false` in both
Config and cassandra.yaml comments/tests. Please align the PR description (or
the default) so operators know the real default behavior.
##########
src/java/org/apache/cassandra/service/StorageServiceMBean.java:
##########
@@ -1097,6 +1097,16 @@ default int upgradeSSTables(String keyspaceName, boolean
excludeCurrentVersion,
@Deprecated(since = "5.0")
public void setColumnIndexCacheSize(int cacheSizeInKB);
+ /**
+ * Returns whether use of misprepared statements is enabled
+ */
+ public boolean getPreparedStatementsRequireParametersEnabled();
+
+ /**
+ * Sets whether use of misprepared statements is enabled
Review Comment:
These JMX method docs are ambiguous: the config controls whether misprepared
statements are *rejected* vs only *warned*, not simply whether they are
"enabled". Consider clarifying the Javadoc to reflect the true/false semantics
(reject vs warn) so operators understand what toggling does at runtime.
```suggestion
* Returns whether prepared statements that are missing required bound
parameter values
* are rejected instead of only generating a warning.
*
* @return {@code true} if misprepared statements are rejected, {@code
false} if they are only
* warned about and still executed.
*/
public boolean getPreparedStatementsRequireParametersEnabled();
/**
* Controls whether prepared statements that are missing required bound
parameter values
* are rejected instead of only generating a warning.
*
* @param enabled {@code true} to reject misprepared statements, {@code
false} to only log a
* warning and still execute them.
```
##########
test/unit/org/apache/cassandra/cql3/MispreparedStatementsTest.java:
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.cql3;
+
+import java.net.InetSocketAddress;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.auth.AuthenticatedUser;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.db.ConsistencyLevel;
+import org.apache.cassandra.db.guardrails.GuardrailViolatedException;
+import org.apache.cassandra.db.guardrails.Guardrails;
+import org.apache.cassandra.service.ClientState;
+import org.apache.cassandra.service.ClientWarn;
+
+import static org.junit.Assert.assertTrue;
+
+public class MispreparedStatementsTest extends CQLTester
+{
+ private static final ClientState state = ClientState.forExternalCalls(new
InetSocketAddress("127.0.0.1", 9042));
+ private static boolean originalValue;
+
+ @BeforeClass
+ public static void setupGlobalConfig()
+ {
+ CQLTester.requireAuthentication();
+ originalValue =
DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled();
+ DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true);
+ }
+
+ @AfterClass
+ public static void cleanUp()
+ {
+
DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(originalValue);
+ }
+
+ @Before
+ public void setUp()
+ {
+ AuthenticatedUser nonSuperUser = new AuthenticatedUser("regular-user")
+ {
+ @Override
+ public boolean isSuper()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean isSystem()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean isAnonymous()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean canLogin()
+ {
+ return true;
+ }
+ };
+
+ state.login(nonSuperUser);
+ state.setKeyspace(KEYSPACE);
+ ClientWarn.instance.captureWarnings();
+ createTable("CREATE TABLE %s (id int, description text, age int, name
text, PRIMARY KEY (id, name, age))");
+ }
+
+ @After
+ public void tearDown()
+ {
+ DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true);
+ }
+
+ @Test
+ public void testSelectWithPartitionKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithClusteringKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name =
'v1' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1
AND name = 'v1'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithWhereNonPrimaryKeyColumn()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE
description = 'foo' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+
+ @Test
+ public void testSelectWithCompositeRestriction()
+ {
+ assertGuardrailViolated(String.format("SELECT sum(id) from %s WHERE
(name, age) = ('a', 1) ALLOW FILTERING", currentTable()));
+ }
+
+ @Test
+ public void testSelectWithFunction()
+ {
+ assertGuardrailViolated(String.format("SELECT sum(id) from %s where
name = 'v1' ALLOW FILTERING", currentTable()));
+ }
+
+ @Test
+ public void testSelectWithRangeRestriction()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1
AND name > 'a'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnPartitionKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN
(1, 2, 3)", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnClusteringKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name IN
('a', 'b') ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN
(1, 2, 3) AND name in ('a', 'b')", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testDDLStatementsBypass()
+ {
+ assertGuardrailPassed("CREATE TABLE IF NOT EXISTS test_table (id INT
PRIMARY KEY)");
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testWhereLiteralWithLike()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name
LIKE 'prefix%%' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testInsertJsonGuardrail()
+ {
+ assertGuardrailViolated(String.format("INSERT INTO %s JSON '{\"id\":
1, \"name\": \"v1\"}'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testUpdateWithPartitionKey()
+ {
+ assertGuardrailViolated(String.format("UPDATE %s SET description =
'new' WHERE id = 1 AND name = 'name' AND age = 1", KEYSPACE + '.' +
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testUpdateWithIfCondition()
+ {
+ assertGuardrailViolated(String.format("UPDATE %s SET description =
'v2' WHERE id = 1 AND name = 'v1' AND age = 1 IF description = 'v0'",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testDeleteWithFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("DELETE FROM %s WHERE id = 1 AND
name = 'v1'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testMultiTableBatchGuardrailAllLiteral()
+ {
+ String table1 = currentTable();
+ String table2 = createTable("CREATE TABLE %s (id int PRIMARY KEY, val
text)");
+ String query = String.format("BEGIN BATCH " +
+ "UPDATE %s.%s SET description = 'v1'
WHERE id = 1 AND name = 'n1' AND age = 1; " +
+ "INSERT INTO %s.%s (id, val) VALUES (2,
'v2'); " +
+ "APPLY BATCH",
+ KEYSPACE, table1, KEYSPACE, table2);
+
+ assertGuardrailViolated(query);
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSucceedWithOnlyBindMarkers()
+ {
+ assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = ? AND
name = ?", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSucceedWithOneBindMarkerOneLiteral()
+ {
+
+ assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = ? AND
name = 'v1'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectAllPasses()
+ {
+ assertGuardrailPassed(String.format("SELECT * FROM %s",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testLiteralInProjectionIsAllowed()
+ {
+ assertGuardrailPassed(String.format("SELECT id, (text)'const_val' FROM
%s WHERE id = ?", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testInternalBypass()
+ {
+ assertGuardrailPassed("SELECT * FROM " + KEYSPACE + '.' +
currentTable() + " WHERE id = 1", ClientState.forInternalCalls());
+ }
+
+ @Test
+ public void testSuperUserBypass()
+ {
+ AuthenticatedUser superUser = new AuthenticatedUser("super-user")
+ {
+ @Override
+ public boolean isSuper()
+ {
+ return true;
+ }
+
+ @Override
+ public boolean isSystem()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean isAnonymous()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean canLogin()
+ {
+ return true;
+ }
+ };
+ ClientState superUserState = ClientState.forExternalCalls(new
InetSocketAddress("127.0.0.1", 9042));
+ superUserState.login(superUser);
+ assertGuardrailPassed("SELECT * FROM " + KEYSPACE + '.' +
currentTable() + " WHERE id = 1", superUserState);
+ }
+
+ @Test
+ public void testGuardrailBypassesNonPrepareStatements()
+ {
+ String tableName = createTable("CREATE TABLE %s (id int PRIMARY KEY,
name text)");
+
+ try
+ {
+ String query = String.format("SELECT * FROM %s.%s WHERE id = 1 and
name = 'v1' ALLOW FILTERING",
+ KEYSPACE, tableName);
+
+ QueryProcessor.process(query, ConsistencyLevel.ONE);
+ assertNoWarnings();
+ }
+ catch (GuardrailViolatedException e)
+ {
+ Assert.fail("Non Prepare statement also checked for prepared
constraints");
+ }
+ }
+
+
+ @Test
+ public void testGuardrailDoesNotAppliesToNonPreparableStatements()
+ {
+ assertGuardrailPassed(String.format("ALTER TABLE %s add mime text",
currentTable()));
+ assertNoWarnings();
+ }
Review Comment:
Method name grammar: `testGuardrailDoesNotAppliesToNonPreparableStatements`
should be `...DoesNotApply...` for readability and consistency.
##########
test/unit/org/apache/cassandra/cql3/MispreparedStatementsTest.java:
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.cql3;
+
+import java.net.InetSocketAddress;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.auth.AuthenticatedUser;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.db.ConsistencyLevel;
+import org.apache.cassandra.db.guardrails.GuardrailViolatedException;
+import org.apache.cassandra.db.guardrails.Guardrails;
+import org.apache.cassandra.service.ClientState;
+import org.apache.cassandra.service.ClientWarn;
+
+import static org.junit.Assert.assertTrue;
+
+public class MispreparedStatementsTest extends CQLTester
+{
+ private static final ClientState state = ClientState.forExternalCalls(new
InetSocketAddress("127.0.0.1", 9042));
+ private static boolean originalValue;
+
+ @BeforeClass
+ public static void setupGlobalConfig()
+ {
+ CQLTester.requireAuthentication();
+ originalValue =
DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled();
+ DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true);
+ }
+
+ @AfterClass
+ public static void cleanUp()
+ {
+
DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(originalValue);
+ }
+
+ @Before
+ public void setUp()
+ {
+ AuthenticatedUser nonSuperUser = new AuthenticatedUser("regular-user")
+ {
+ @Override
+ public boolean isSuper()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean isSystem()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean isAnonymous()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean canLogin()
+ {
+ return true;
+ }
+ };
+
+ state.login(nonSuperUser);
+ state.setKeyspace(KEYSPACE);
+ ClientWarn.instance.captureWarnings();
+ createTable("CREATE TABLE %s (id int, description text, age int, name
text, PRIMARY KEY (id, name, age))");
+ }
+
+ @After
+ public void tearDown()
+ {
+ DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true);
+ }
+
+ @Test
+ public void testSelectWithPartitionKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithClusteringKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name =
'v1' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1
AND name = 'v1'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithWhereNonPrimaryKeyColumn()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE
description = 'foo' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+
+ @Test
+ public void testSelectWithCompositeRestriction()
+ {
+ assertGuardrailViolated(String.format("SELECT sum(id) from %s WHERE
(name, age) = ('a', 1) ALLOW FILTERING", currentTable()));
+ }
+
+ @Test
+ public void testSelectWithFunction()
+ {
+ assertGuardrailViolated(String.format("SELECT sum(id) from %s where
name = 'v1' ALLOW FILTERING", currentTable()));
+ }
+
+ @Test
+ public void testSelectWithRangeRestriction()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1
AND name > 'a'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnPartitionKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN
(1, 2, 3)", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnClusteringKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name IN
('a', 'b') ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN
(1, 2, 3) AND name in ('a', 'b')", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testDDLStatementsBypass()
+ {
+ assertGuardrailPassed("CREATE TABLE IF NOT EXISTS test_table (id INT
PRIMARY KEY)");
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testWhereLiteralWithLike()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name
LIKE 'prefix%%' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testInsertJsonGuardrail()
+ {
+ assertGuardrailViolated(String.format("INSERT INTO %s JSON '{\"id\":
1, \"name\": \"v1\"}'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testUpdateWithPartitionKey()
+ {
+ assertGuardrailViolated(String.format("UPDATE %s SET description =
'new' WHERE id = 1 AND name = 'name' AND age = 1", KEYSPACE + '.' +
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testUpdateWithIfCondition()
+ {
+ assertGuardrailViolated(String.format("UPDATE %s SET description =
'v2' WHERE id = 1 AND name = 'v1' AND age = 1 IF description = 'v0'",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testDeleteWithFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("DELETE FROM %s WHERE id = 1 AND
name = 'v1'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testMultiTableBatchGuardrailAllLiteral()
+ {
+ String table1 = currentTable();
+ String table2 = createTable("CREATE TABLE %s (id int PRIMARY KEY, val
text)");
+ String query = String.format("BEGIN BATCH " +
+ "UPDATE %s.%s SET description = 'v1'
WHERE id = 1 AND name = 'n1' AND age = 1; " +
+ "INSERT INTO %s.%s (id, val) VALUES (2,
'v2'); " +
+ "APPLY BATCH",
+ KEYSPACE, table1, KEYSPACE, table2);
+
+ assertGuardrailViolated(query);
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSucceedWithOnlyBindMarkers()
+ {
+ assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = ? AND
name = ?", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSucceedWithOneBindMarkerOneLiteral()
+ {
+
+ assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = ? AND
name = 'v1'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectAllPasses()
+ {
+ assertGuardrailPassed(String.format("SELECT * FROM %s",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testLiteralInProjectionIsAllowed()
+ {
+ assertGuardrailPassed(String.format("SELECT id, (text)'const_val' FROM
%s WHERE id = ?", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testInternalBypass()
+ {
+ assertGuardrailPassed("SELECT * FROM " + KEYSPACE + '.' +
currentTable() + " WHERE id = 1", ClientState.forInternalCalls());
+ }
+
+ @Test
+ public void testSuperUserBypass()
+ {
+ AuthenticatedUser superUser = new AuthenticatedUser("super-user")
+ {
+ @Override
+ public boolean isSuper()
+ {
+ return true;
+ }
+
+ @Override
+ public boolean isSystem()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean isAnonymous()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean canLogin()
+ {
+ return true;
+ }
+ };
+ ClientState superUserState = ClientState.forExternalCalls(new
InetSocketAddress("127.0.0.1", 9042));
+ superUserState.login(superUser);
+ assertGuardrailPassed("SELECT * FROM " + KEYSPACE + '.' +
currentTable() + " WHERE id = 1", superUserState);
+ }
+
+ @Test
+ public void testGuardrailBypassesNonPrepareStatements()
+ {
+ String tableName = createTable("CREATE TABLE %s (id int PRIMARY KEY,
name text)");
+
+ try
+ {
+ String query = String.format("SELECT * FROM %s.%s WHERE id = 1 and
name = 'v1' ALLOW FILTERING",
+ KEYSPACE, tableName);
+
+ QueryProcessor.process(query, ConsistencyLevel.ONE);
+ assertNoWarnings();
+ }
+ catch (GuardrailViolatedException e)
+ {
+ Assert.fail("Non Prepare statement also checked for prepared
constraints");
+ }
+ }
+
+
+ @Test
+ public void testGuardrailDoesNotAppliesToNonPreparableStatements()
+ {
+ assertGuardrailPassed(String.format("ALTER TABLE %s add mime text",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSystemKeyspaceBypassForRegularUser()
+ {
+ assertGuardrailPassed("SELECT * FROM system.local WHERE key =
'local'");
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testGuardrailDisabledAllowsLiterals()
+ {
+
DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(false);
+ assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = 1",
currentTable()));
+ assertWarnings();
+ }
+
+ @Test
+ public void testWarningIsIssuedWhenGuardrailIsAllowed()
+ {
+
DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(false);
+ assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = 1",
currentTable()));
+ assertWarnings();
+
+ }
+
+ @Test
Review Comment:
`testGuardrailDisabledAllowsLiterals` and
`testWarningIsIssuedWhenGuardrailIsAllowed` are currently testing the same
behavior (both set the flag false, prepare the same literal query, and assert
warnings). Consider removing one or making them cover distinct scenarios; also,
the name "Disabled" is confusing because the flag=false path still emits
warnings (it isn't truly disabled).
```suggestion
```
##########
test/unit/org/apache/cassandra/cql3/MispreparedStatementsTest.java:
##########
@@ -0,0 +1,430 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.cassandra.cql3;
+
+import java.net.InetSocketAddress;
+import java.util.List;
+
+import org.junit.After;
+import org.junit.AfterClass;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import org.apache.cassandra.auth.AuthenticatedUser;
+import org.apache.cassandra.config.DatabaseDescriptor;
+import org.apache.cassandra.db.ConsistencyLevel;
+import org.apache.cassandra.db.guardrails.GuardrailViolatedException;
+import org.apache.cassandra.db.guardrails.Guardrails;
+import org.apache.cassandra.service.ClientState;
+import org.apache.cassandra.service.ClientWarn;
+
+import static org.junit.Assert.assertTrue;
+
+public class MispreparedStatementsTest extends CQLTester
+{
+ private static final ClientState state = ClientState.forExternalCalls(new
InetSocketAddress("127.0.0.1", 9042));
+ private static boolean originalValue;
+
+ @BeforeClass
+ public static void setupGlobalConfig()
+ {
+ CQLTester.requireAuthentication();
+ originalValue =
DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled();
+ DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true);
+ }
+
+ @AfterClass
+ public static void cleanUp()
+ {
+
DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(originalValue);
+ }
+
+ @Before
+ public void setUp()
+ {
+ AuthenticatedUser nonSuperUser = new AuthenticatedUser("regular-user")
+ {
+ @Override
+ public boolean isSuper()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean isSystem()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean isAnonymous()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean canLogin()
+ {
+ return true;
+ }
+ };
+
+ state.login(nonSuperUser);
+ state.setKeyspace(KEYSPACE);
+ ClientWarn.instance.captureWarnings();
+ createTable("CREATE TABLE %s (id int, description text, age int, name
text, PRIMARY KEY (id, name, age))");
+ }
+
+ @After
+ public void tearDown()
+ {
+ DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true);
+ }
+
+ @Test
+ public void testSelectWithPartitionKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithClusteringKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name =
'v1' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1
AND name = 'v1'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectWithWhereNonPrimaryKeyColumn()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE
description = 'foo' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+
+ @Test
+ public void testSelectWithCompositeRestriction()
+ {
+ assertGuardrailViolated(String.format("SELECT sum(id) from %s WHERE
(name, age) = ('a', 1) ALLOW FILTERING", currentTable()));
+ }
+
+ @Test
+ public void testSelectWithFunction()
+ {
+ assertGuardrailViolated(String.format("SELECT sum(id) from %s where
name = 'v1' ALLOW FILTERING", currentTable()));
+ }
+
+ @Test
+ public void testSelectWithRangeRestriction()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id = 1
AND name > 'a'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnPartitionKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN
(1, 2, 3)", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnClusteringKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name IN
('a', 'b') ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectInRestrictionOnFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE id IN
(1, 2, 3) AND name in ('a', 'b')", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testDDLStatementsBypass()
+ {
+ assertGuardrailPassed("CREATE TABLE IF NOT EXISTS test_table (id INT
PRIMARY KEY)");
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testWhereLiteralWithLike()
+ {
+ assertGuardrailViolated(String.format("SELECT * FROM %s WHERE name
LIKE 'prefix%%' ALLOW FILTERING", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testInsertJsonGuardrail()
+ {
+ assertGuardrailViolated(String.format("INSERT INTO %s JSON '{\"id\":
1, \"name\": \"v1\"}'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testUpdateWithPartitionKey()
+ {
+ assertGuardrailViolated(String.format("UPDATE %s SET description =
'new' WHERE id = 1 AND name = 'name' AND age = 1", KEYSPACE + '.' +
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testUpdateWithIfCondition()
+ {
+ assertGuardrailViolated(String.format("UPDATE %s SET description =
'v2' WHERE id = 1 AND name = 'v1' AND age = 1 IF description = 'v0'",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testDeleteWithFullPrimaryKey()
+ {
+ assertGuardrailViolated(String.format("DELETE FROM %s WHERE id = 1 AND
name = 'v1'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testMultiTableBatchGuardrailAllLiteral()
+ {
+ String table1 = currentTable();
+ String table2 = createTable("CREATE TABLE %s (id int PRIMARY KEY, val
text)");
+ String query = String.format("BEGIN BATCH " +
+ "UPDATE %s.%s SET description = 'v1'
WHERE id = 1 AND name = 'n1' AND age = 1; " +
+ "INSERT INTO %s.%s (id, val) VALUES (2,
'v2'); " +
+ "APPLY BATCH",
+ KEYSPACE, table1, KEYSPACE, table2);
+
+ assertGuardrailViolated(query);
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSucceedWithOnlyBindMarkers()
+ {
+ assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = ? AND
name = ?", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSucceedWithOneBindMarkerOneLiteral()
+ {
+
+ assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = ? AND
name = 'v1'", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSelectAllPasses()
+ {
+ assertGuardrailPassed(String.format("SELECT * FROM %s",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testLiteralInProjectionIsAllowed()
+ {
+ assertGuardrailPassed(String.format("SELECT id, (text)'const_val' FROM
%s WHERE id = ?", currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testInternalBypass()
+ {
+ assertGuardrailPassed("SELECT * FROM " + KEYSPACE + '.' +
currentTable() + " WHERE id = 1", ClientState.forInternalCalls());
+ }
+
+ @Test
+ public void testSuperUserBypass()
+ {
+ AuthenticatedUser superUser = new AuthenticatedUser("super-user")
+ {
+ @Override
+ public boolean isSuper()
+ {
+ return true;
+ }
+
+ @Override
+ public boolean isSystem()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean isAnonymous()
+ {
+ return false;
+ }
+
+ @Override
+ public boolean canLogin()
+ {
+ return true;
+ }
+ };
+ ClientState superUserState = ClientState.forExternalCalls(new
InetSocketAddress("127.0.0.1", 9042));
+ superUserState.login(superUser);
+ assertGuardrailPassed("SELECT * FROM " + KEYSPACE + '.' +
currentTable() + " WHERE id = 1", superUserState);
+ }
+
+ @Test
+ public void testGuardrailBypassesNonPrepareStatements()
+ {
+ String tableName = createTable("CREATE TABLE %s (id int PRIMARY KEY,
name text)");
+
+ try
+ {
+ String query = String.format("SELECT * FROM %s.%s WHERE id = 1 and
name = 'v1' ALLOW FILTERING",
+ KEYSPACE, tableName);
+
+ QueryProcessor.process(query, ConsistencyLevel.ONE);
+ assertNoWarnings();
+ }
+ catch (GuardrailViolatedException e)
+ {
+ Assert.fail("Non Prepare statement also checked for prepared
constraints");
+ }
+ }
+
+
+ @Test
+ public void testGuardrailDoesNotAppliesToNonPreparableStatements()
+ {
+ assertGuardrailPassed(String.format("ALTER TABLE %s add mime text",
currentTable()));
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testSystemKeyspaceBypassForRegularUser()
+ {
+ assertGuardrailPassed("SELECT * FROM system.local WHERE key =
'local'");
+ assertNoWarnings();
+ }
+
+ @Test
+ public void testGuardrailDisabledAllowsLiterals()
+ {
+
DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(false);
+ assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = 1",
currentTable()));
+ assertWarnings();
+ }
+
+ @Test
+ public void testWarningIsIssuedWhenGuardrailIsAllowed()
+ {
+
DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(false);
+ assertGuardrailPassed(String.format("SELECT * FROM %s WHERE id = 1",
currentTable()));
+ assertWarnings();
+
+ }
+
+ @Test
+ public void testGuardrailDisabledAllowsBatchLiterals()
+ {
+
DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(false);
+ String tableName = currentTable();
+ assertGuardrailPassed(String.format("BEGIN BATCH " +
+ "INSERT INTO %s.%s (id,
description, name, age) VALUES (1, 'v1', 'v1', 1); " +
+ "UPDATE %s.%s SET description =
'v2' WHERE id = 2 AND name = 'v1' AND age = 1; " +
+ "APPLY BATCH", KEYSPACE,
tableName, KEYSPACE, tableName));
+ assertWarnings();
+ }
+
+ @Test
+ public void testBlockedStatementsDoNotEnterCache()
+ {
+ QueryProcessor.clearPreparedStatementsCache();
+ int initialCacheSize = QueryProcessor.preparedStatementsCount();
+ for (int i = 0; i < 10; i++)
+ {
+ String query = String.format("SELECT * FROM %s WHERE id = %d",
currentTable(), i);
+ try
+ {
+ QueryProcessor.instance.prepare(query, state);
+ }
+ catch (GuardrailViolatedException e)
+ {
+ // Expected: Guardrail throws exception
+ }
+ }
+ Assert.assertEquals("Blocked misprepared statements should not be
added to the cache", initialCacheSize,
QueryProcessor.preparedStatementsCount());
+ }
+
+ private void assertGuardrailViolated(String query)
+ {
+ try
+ {
+ QueryProcessor.instance.prepare(query, state);
+ Assert.fail("Expected GuardrailViolatedException for query: " +
query);
+ }
+ catch (GuardrailViolatedException e)
+ {
+ assertTrue(e.getMessage().contains("Guardrail " +
Guardrails.preparedStatementsRequireParametersEnabled.name + " violated"));
+ }
+ catch (Exception e)
+ {
+ Assert.fail(e.getMessage());
+ }
+ }
+
+ private void assertNoWarnings()
+ {
+ List<String> warnings = ClientWarn.instance.getWarnings();
+ if (warnings != null)
+ {
+ assertTrue("Expected performance tip warning was found",
+ warnings.stream().noneMatch(w ->
w.contains(Guardrails.MISPREPARED_STATEMENT_WARN_MESSAGE)));
+ }
+ }
+
+ private void assertWarnings()
+ {
+ List<String> warnings = ClientWarn.instance.getWarnings();
+ assertTrue("Expected performance tip warning was not found",
+ warnings.stream().anyMatch(w ->
w.contains(Guardrails.MISPREPARED_STATEMENT_WARN_MESSAGE)));
+ }
Review Comment:
`assertWarnings()` can throw NPE when no warnings are present because
`ClientWarn.instance.getWarnings()` may return null. It would be more robust to
assert the list is non-null before streaming (so failures report a clear
assertion instead of NPE). Also, `assertNoWarnings()` uses the message
"Expected ... warning was found"; that reads backwards for the "no warnings"
assertion (it should indicate an *unexpected* warning).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
