You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/03/10 16:34:55 UTC
[GitHub] [cassandra] adelapena commented on a change in pull request #1453: added enable for ALLOW FILTERING property
adelapena commented on a change in pull request #1453:
URL: https://github.com/apache/cassandra/pull/1453#discussion_r823858908
##########
File path: src/java/org/apache/cassandra/db/guardrails/Guardrails.java
##########
@@ -148,6 +148,13 @@
new DisableFlag(state -> !CONFIG_PROVIDER.getOrCreate(state).getReadBeforeWriteListOperationsEnabled(),
"List operation requiring read before write");
+ /**
+ * Guardrail disabling ALLOW FILTERING statement within a query
+ */
+ public static final DisableFlag allowFilteringEnabled =
+ new DisableFlag(state -> !CONFIG_PROVIDER.getOrCreate(state).getAllowFilteringEnabled(),
Review comment:
I'm to blame. That's how IntelliJ with the project code style formats, at least for me. I don't have a clue about what's the guideline on this, and it seems that the codebase contains examples of both types on alignments, and also with two-block indent. If we change to something different, I'd do it for all the guardrails in the file.
##########
File path: src/java/org/apache/cassandra/cql3/statements/SelectStatement.java
##########
@@ -31,9 +31,11 @@
import org.apache.cassandra.audit.AuditLogContext;
import org.apache.cassandra.audit.AuditLogEntryType;
import org.apache.cassandra.auth.Permission;
+import org.apache.cassandra.config.DatabaseDescriptor;
Review comment:
Nit: unused import
##########
File path: test/unit/org/apache/cassandra/db/guardrails/GuardrailAllowFilteringTest.java
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.db.guardrails;
+
+import org.junit.Before;
+import org.junit.Test;
+
+public class GuardrailAllowFilteringTest extends GuardrailTester
+{
+ @Before
+ public void setupTest()
+ {
+ createTable("CREATE TABLE %s (k int PRIMARY KEY, a int, b int)");
+ }
+
+ private void setGuardrail(boolean allowFilteringEnabled)
+ {
+ guardrails().setAllowFilteringEnabled(allowFilteringEnabled);
+ }
+
+ private boolean getGuardrial()
+ {
+ return guardrails().getAllowFilteringEnabled();
+ }
+
+ @Test
+ public void testAllowFilteringDisabled() throws Throwable
+ {
+ boolean originalState = getGuardrial();
+ setGuardrail(false);
+ assertFails("ALLOW FILTERING statement is prohibited", "SELECT * from %s WHERE a = 5 ALLOW FILTERING");
Review comment:
We could also test with `ALLOW FILTERING` enabled:
```java
setGuardrail(true);
assertValid("SELECT * FROM %s WHERE a = 5 ALLOW FILTERING");
```
##########
File path: test/unit/org/apache/cassandra/cql3/validation/operations/SelectTest.java
##########
@@ -34,13 +35,11 @@
import static org.apache.cassandra.utils.ByteBufferUtil.EMPTY_BYTE_BUFFER;
import static org.apache.cassandra.utils.ByteBufferUtil.bytes;
-
Review comment:
These changes seem unrelated to the patch, maybe they are survivors from the previous version of the new tests.
##########
File path: test/unit/org/apache/cassandra/cql3/CQLTester.java
##########
@@ -1219,7 +1219,7 @@ protected TableMetadata currentTableMetadata()
return Schema.instance.getTableMetadata(KEYSPACE, currentTable());
}
- protected com.datastax.driver.core.ResultSet executeNet(ProtocolVersion protocolVersion, String query, Object... values) throws Throwable
+ protected com.datastax.driver.core.ResultSet executeNet(ProtocolVersion protocolVersion, String query, Object... values)
Review comment:
I guess this might come from a previous version where this was called from a lambda, could it be the reason? Not against cleaning up though, otherwise these unnecessary "throws X" can remain here forever.
##########
File path: src/java/org/apache/cassandra/db/guardrails/Guardrails.java
##########
@@ -148,6 +148,13 @@
new DisableFlag(state -> !CONFIG_PROVIDER.getOrCreate(state).getReadBeforeWriteListOperationsEnabled(),
"List operation requiring read before write");
+ /**
+ * Guardrail disabling ALLOW FILTERING statement within a query
+ */
+ public static final DisableFlag allowFilteringEnabled =
+ new DisableFlag(state -> !CONFIG_PROVIDER.getOrCreate(state).getAllowFilteringEnabled(),
+ "ALLOW FILTERING statement is prohibited");
Review comment:
We still have pending adding a specific exception type for guardrails, although it seems problematic with drivers. We could also add a common prefix to all the exception messages produced by guardrails.
Another problem is that we are providing here is parameter in the [`what` argument](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/db/guardrails/DisableFlag.java#L43-L46) of the constructor for `DisableFlag`, which will be [concatenated](https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/db/guardrails/DisableFlag.java#L79) with `" is not allowed"` in the error message. So the final error message will be `ALLOW FILTERING statement is prohibited is not allowed`.
##########
File path: src/java/org/apache/cassandra/config/GuardrailsOptions.java
##########
@@ -322,6 +322,20 @@ public void setReadBeforeWriteListOperationsEnabled(boolean enabled)
x -> config.read_before_write_list_operations_enabled = x);
}
+ @Override
+ public boolean getAllowFilteringEnabled()
+ {
+ return config.allow_filtering_enabled;
+ }
+
+ public void setAllowFilteringEnabled(boolean enabled)
+ {
+ updatePropertyWithLogging("allow_filtering_enabled",
+ enabled,
+ () -> config.allow_filtering_enabled,
+ x -> config.allow_filtering_enabled = x);
+ }
+
Review comment:
Nit: misaligned
##########
File path: test/unit/org/apache/cassandra/db/guardrails/GuardrailAllowFilteringTest.java
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.db.guardrails;
+
+import org.junit.Before;
+import org.junit.Test;
+
+public class GuardrailAllowFilteringTest extends GuardrailTester
+{
+ @Before
+ public void setupTest()
+ {
+ createTable("CREATE TABLE %s (k int PRIMARY KEY, a int, b int)");
+ }
+
+ private void setGuardrail(boolean allowFilteringEnabled)
+ {
+ guardrails().setAllowFilteringEnabled(allowFilteringEnabled);
+ }
+
+ private boolean getGuardrial()
+ {
+ return guardrails().getAllowFilteringEnabled();
+ }
+
+ @Test
+ public void testAllowFilteringDisabled() throws Throwable
+ {
+ boolean originalState = getGuardrial();
+ setGuardrail(false);
+ assertFails("ALLOW FILTERING statement is prohibited", "SELECT * from %s WHERE a = 5 ALLOW FILTERING");
Review comment:
Last changes in `GuardrailTester` have changed the order of the parameters (so we can specify multiple warning messages). It would be:
```suggestion
assertFails("SELECT * from %s WHERE a = 5 ALLOW FILTERING", "ALLOW FILTERING statement is prohibited");
```
##########
File path: test/unit/org/apache/cassandra/db/guardrails/GuardrailAllowFilteringTest.java
##########
@@ -0,0 +1,62 @@
+/*
+ * 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.db.guardrails;
+
+import org.junit.Before;
+import org.junit.Test;
+
+public class GuardrailAllowFilteringTest extends GuardrailTester
+{
+ @Before
+ public void setupTest()
+ {
+ createTable("CREATE TABLE %s (k int PRIMARY KEY, a int, b int)");
+ }
+
+ private void setGuardrail(boolean allowFilteringEnabled)
+ {
+ guardrails().setAllowFilteringEnabled(allowFilteringEnabled);
+ }
+
+ private boolean getGuardrial()
+ {
+ return guardrails().getAllowFilteringEnabled();
+ }
+
+ @Test
+ public void testAllowFilteringDisabled() throws Throwable
+ {
+ boolean originalState = getGuardrial();
Review comment:
Not sure we need to save the state but, if we do, we can do it in `@Before`/`@After` or `@BeforeClass`/`@AfterClass` methods so we don't need to do it on every test.
--
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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org