Copilot commented on code in PR #4596: URL: https://github.com/apache/cassandra/pull/4596#discussion_r2885634449
########## test/unit/org/apache/cassandra/cql3/MispreparedStatementsTest.java: ########## @@ -0,0 +1,405 @@ +/* + * 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.auth.CassandraAuthorizer; +import org.apache.cassandra.auth.PasswordAuthenticator; +import org.apache.cassandra.config.DatabaseDescriptor; +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() + { + DatabaseDescriptor.setAuthenticator(new PasswordAuthenticator()); + DatabaseDescriptor.setAuthorizer(new CassandraAuthorizer()); + 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())); Review Comment: `INSERT ... JSON` statements don’t have `restrictions` (the parsed insert-json path passes `null` restrictions), and the current guardrail check in `ModificationStatement.validate()` only triggers when `hasRestriction` is true. With the implementation in this PR, this test will not throw `GuardrailViolatedException` and is likely to fail (or it implies the guardrail scope should be expanded beyond WHERE/IF restrictions). Please align the test expectation with the actual guardrail behavior/scope. ```suggestion assertGuardrailPassed(String.format("INSERT INTO %s JSON '{\"id\": 1, \"name\": \"v1\"}'", currentTable())); ``` ########## test/unit/org/apache/cassandra/cql3/MispreparedStatementsTest.java: ########## @@ -0,0 +1,405 @@ +/* + * 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.auth.CassandraAuthorizer; +import org.apache.cassandra.auth.PasswordAuthenticator; +import org.apache.cassandra.config.DatabaseDescriptor; +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() + { + DatabaseDescriptor.setAuthenticator(new PasswordAuthenticator()); + DatabaseDescriptor.setAuthorizer(new CassandraAuthorizer()); + originalValue = DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled(); + DatabaseDescriptor.setPreparedStatementsRequireParametersEnabled(true); + } Review Comment: This test class mutates global auth configuration (`DatabaseDescriptor.setAuthenticator` / `setAuthorizer`) but doesn’t restore the previous authenticator/authorizer in `@AfterClass`. That can leak state into other unit tests in the same JVM. Prefer using `CQLTester.requireAuthentication(...)` helpers (which set up local test auth implementations) and/or save+restore the original authenticator/authorizer in `cleanUp()`. ########## test/unit/org/apache/cassandra/cql3/MispreparedStatementsTest.java: ########## @@ -0,0 +1,405 @@ +/* + * 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.auth.CassandraAuthorizer; +import org.apache.cassandra.auth.PasswordAuthenticator; +import org.apache.cassandra.config.DatabaseDescriptor; +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() + { + DatabaseDescriptor.setAuthenticator(new PasswordAuthenticator()); + DatabaseDescriptor.setAuthorizer(new CassandraAuthorizer()); + 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 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 identical (both disable the guardrail, prepare the same query, and assert warnings). Consider removing one or changing one of them to cover a distinct behavior to avoid redundant coverage and future maintenance burden. ```suggestion ``` ########## src/java/org/apache/cassandra/cql3/statements/ModificationStatement.java: ########## @@ -409,7 +409,21 @@ public void validate(ClientState state) throws InvalidRequestException checkFalse(isVirtual() && hasConditions(), "Conditional updates are not supported by virtual tables"); if (attrs.isTimestampSet()) + { Guardrails.userTimestampsEnabled.ensureEnabled(state); + } + // Misprepare guardrail shouldn't block system keyspaces + if (SchemaConstants.isSystemKeyspace(metadata.keyspace)) + return; + + boolean hasRestriction = restrictions != null && + (restrictions.hasPartitionKeyRestrictions() || + restrictions.hasClusteringColumnsRestrictions() || + restrictions.hasNonPrimaryKeyRestrictions()); + if (getBindVariables().isEmpty() && hasRestriction) + { + Guardrails.onMisprepared(state, metadata.keyspace, metadata.getTableName()); Review Comment: As with `SelectStatement`, adding the misprepared-statement guardrail to `validate()` means it will run for *executed* non-prepared statements too (since `QueryProcessor.processStatement` calls `statement.validate(clientState)`). That can unexpectedly reject/warn on ordinary literal UPDATE/DELETE statements that were never PREPAREd. Move this check to the prepare path (or otherwise gate it so it only runs during PREPARE). ```suggestion // Misprepare guardrail should only run during PREPARE and shouldn't block system keyspaces if (state.getSource() == StatementSource.PREPARE) { if (SchemaConstants.isSystemKeyspace(metadata.keyspace)) return; boolean hasRestriction = restrictions != null && (restrictions.hasPartitionKeyRestrictions() || restrictions.hasClusteringColumnsRestrictions() || restrictions.hasNonPrimaryKeyRestrictions()); if (getBindVariables().isEmpty() && hasRestriction) { Guardrails.onMisprepared(state, metadata.keyspace, metadata.getTableName()); } ``` ########## 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 getMispreparedStatementsEnabled(); + + /** + * Sets whether use of misprepared statements is enabled + */ + public void setMispreparedStatementsEnabled(boolean enabled); + Review Comment: These new StorageServiceMBean methods are named `get/setMispreparedStatementsEnabled`, but they simply proxy `DatabaseDescriptor.get/setPreparedStatementsRequireParametersEnabled()`. The naming/API here is inconsistent with the actual config option name (`prepared_statements_require_parameters_enabled`) and with the GuardrailsConfig/GuardrailsOptions method names, which will be confusing for JMX consumers. Consider renaming the JMX methods to match the config key, or introducing a clear aliasing scheme. ```suggestion * Returns whether use of misprepared statements is enabled. * * @deprecated use {@link #getPreparedStatementsRequireParametersEnabled()} instead. */ @Deprecated(since = "5.0") public boolean getMispreparedStatementsEnabled(); /** * Sets whether use of misprepared statements is enabled. * * @deprecated use {@link #setPreparedStatementsRequireParametersEnabled(boolean)} instead. */ @Deprecated(since = "5.0") public void setMispreparedStatementsEnabled(boolean enabled); /** * Returns whether the prepared_statements_require_parameters_enabled guardrail is enabled. */ public boolean getPreparedStatementsRequireParametersEnabled(); /** * Sets whether the prepared_statements_require_parameters_enabled guardrail is enabled. */ public void setPreparedStatementsRequireParametersEnabled(boolean enabled); ``` ########## test/unit/org/apache/cassandra/config/DatabaseDescriptorTest.java: ########## @@ -781,6 +781,27 @@ public void testRowIndexSizeWarnEqAbort() DatabaseDescriptor.applyReadThresholdsValidations(conf); } + @Test + public void testMisreparedStatementsRequireParametersEnabled() { + boolean originalValue = DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled(); + try + { + Assert.assertFalse("Default value of misprepared_statement_enabled must be false", originalValue); Review Comment: Typo in the test name: `testMisreparedStatementsRequireParametersEnabled` should be `testMisprepared...` (or preferably renamed to match the actual option name `prepared_statements_require_parameters_enabled`). The assertion message also refers to `misprepared_statement_enabled`, which doesn’t match the config key used elsewhere in this PR. ```suggestion public void testPreparedStatementsRequireParametersEnabled() { boolean originalValue = DatabaseDescriptor.getPreparedStatementsRequireParametersEnabled(); try { Assert.assertFalse("Default value of prepared_statements_require_parameters_enabled must be false", originalValue); ``` ########## src/java/org/apache/cassandra/cql3/statements/SelectStatement.java: ########## @@ -339,8 +339,19 @@ public void authorize(ClientState state) throws InvalidRequestException, Unautho public void validate(ClientState state) throws InvalidRequestException { - if (parameters.allowFiltering && !SchemaConstants.isSystemKeyspace(table.keyspace)) + if (SchemaConstants.isSystemKeyspace(table.keyspace)) + return; + if (parameters.allowFiltering) Guardrails.allowFilteringEnabled.ensureEnabled(state); + boolean hasRestrictions = restrictions != null && + (restrictions.hasPartitionKeyRestrictions() || + restrictions.hasClusteringColumnsRestrictions() || + restrictions.hasNonPrimaryKeyRestrictions()); + if (getBindVariables().isEmpty() + && hasRestrictions) + { + Guardrails.onMisprepared(state, table.keyspace, table.name); + } Review Comment: This misprepared-statement guardrail is implemented in `validate()`, but `validate()` is invoked for both prepared statements (`parseAndPrepare`) and for executing non-prepared CQL (`processStatement`). As a result, normal literal SELECT queries (not PREPAREd) can now be rejected/warned, which is a behavioral change beyond the PR’s stated scope. The guardrail should run only on the prepare path (e.g., in `QueryProcessor.parseAndPrepare` or a prepare-specific hook), not during general statement validation/execution. ```suggestion ``` ########## src/java/org/apache/cassandra/db/guardrails/Guardrails.java: ########## @@ -615,6 +618,17 @@ public final class Guardrails implements GuardrailsMBean format("The keyspace %s has a replication factor of %s, above the %s threshold of %s.", what, value, isWarning ? "warning" : "failure", threshold)); + /** + * Prevents cache overflow and eviction caused by the anti-pattern of preparing queries with + * hardcoded literals instead of bind markers. This prevents filling the statement cache with non-reusable entries. + */ + + public static final EnableFlag preparedStatementsRequireParametersEnabled = + new EnableFlag("prepare_statements_require_parameters_enabled", Review Comment: The new guardrail is named `"prepare_statements_require_parameters_enabled"` but the yaml/config field is `prepared_statements_require_parameters_enabled` (note the missing 'd'). This inconsistency makes messages and guardrail naming confusing and will likely break expectations around configuration naming; please align the guardrail name with the config key. ```suggestion new EnableFlag("prepared_statements_require_parameters_enabled", ``` ########## 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. Review Comment: The yaml comment says that when enabled the server rejects preparation of “any statement that does not contain at least one parameter”. With the current implementation, the guardrail only triggers when `restrictions` exist (i.e., WHERE-style restrictions) and won’t apply to statements without restrictions (e.g., INSERT). Please adjust the documentation here to match the actual behavior/scope (or expand the implementation to match the documented behavior). ```suggestion # Guardrail to warn or fail when a statement with restrictions (for example, a WHERE clause) # is prepared without bind markers (parameters) in those restrictions. # This helps prevent the Prepared Statement Cache from being flooded with unique entries # caused by hardcoded literals in query restrictions. # When true, the server rejects the preparation of any such statement whose restrictions do not # contain at least one parameter; statements without restrictions are not affected. ``` -- 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]
