bbotella commented on code in PR #4008: URL: https://github.com/apache/cassandra/pull/4008#discussion_r2018710550
########## test/unit/org/apache/cassandra/cql3/ColumnSpecificationTest.java: ########## @@ -0,0 +1,206 @@ +/* + * 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.util.Map; + +import org.junit.Before; +import org.junit.Test; + +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.schema.CompactionParams; + +/** + * Test various "extensions" to a column spec when altering / creating a table + */ +public class ColumnSpecificationTest extends CQLTester +{ + @Before + public void before() + { + DatabaseDescriptor.setDynamicDataMaskingEnabled(true); + } + + @Test + public void testCreateTableWithColumnHavingMaskBeforeCheck() + { + createTable("CREATE TABLE %s (pk text primary key, name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1);"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAlterTableAlterColumnWithMaskAndCheckStandalone() + { + createTable("CREATE TABLE %s (pk text, name text, primary key (pk));"); + execute("ALTER TABLE %s ALTER name MASKED WITH system.mask_default()"); + execute("ALTER TABLE %s ALTER name CHECK NOT_NULL(name) AND LENGTH(name) > 1;"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAlterTableAlterColumnWithMask() + { + createTable("CREATE TABLE %s (pk text, name text, primary key (pk));"); + execute("ALTER TABLE %s ALTER name MASKED WITH system.mask_default()"); + verifyColumnSpec("name text MASKED WITH system.mask_default()"); + } + + @Test + public void testAlterTableAlterColumnWithCheck() + { + createTable("CREATE TABLE %s (pk text, name text, primary key (pk));"); + execute("ALTER TABLE %s ALTER name CHECK NOT_NULL(name) AND LENGTH(name) > 1;"); + verifyColumnSpec("name text CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAddingCheckToColumnWithMask() + { + createTable("CREATE TABLE %s (pk text primary key, name text MASKED WITH system.mask_default());"); + execute("ALTER TABLE %s ALTER name CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAddingMaskToColumnWithCheck() + { + createTable("CREATE TABLE %s (pk text primary key, name text CHECK NOT_NULL(name) AND LENGTH(name) > 1);"); + execute("ALTER TABLE %s ALTER name MASKED WITH system.mask_default()"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testDroppingCheckKeepsMask() + { + createTable("CREATE TABLE %s (pk text primary key, name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1);"); + execute("ALTER TABLE %s ALTER name DROP CHECK"); + verifyColumnSpec("name text MASKED WITH system.mask_default()"); + } + + @Test + public void droppingMaskKeepsCheck() + { + createTable("CREATE TABLE %s (pk text primary key, name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1);"); + execute("ALTER TABLE %s ALTER name DROP MASKED"); + verifyColumnSpec("name text CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAlterTableAddColumnWithCheck() + { + createTable("CREATE TABLE %s (pk text primary key);"); + execute("ALTER TABLE %s ADD name text CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + verifyColumnSpec("name text CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAlterTableAddColumnWithMask() + { + createTable("CREATE TABLE %s (pk text primary key);"); + execute("ALTER TABLE %s ADD name text MASKED WITH system.mask_default()"); + verifyColumnSpec("name text MASKED WITH system.mask_default()"); + } + + @Test + public void testAlterTableAddColumnWithMaskAndCheck() + { + createTable("CREATE TABLE %s (pk text primary key);"); + execute("ALTER TABLE %s ADD name text MASKED WITH system.mask_default() CHECK NOT_NULL(name)"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name)"); + } + + @Test + public void testAlterTableAddColumnWithMaskAndMultipleChecks() + { + createTable("CREATE TABLE %s (pk text primary key);"); + execute("ALTER TABLE %s ADD name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + /** + * TODO - investigate if it is possible to specify checks before mask when creating a table + */ + @Test(expected = RuntimeException.class) + public void testFailingCreateTableWithColumnHavingMaskAfterCheck() + { + createTable("CREATE TABLE %s (pk text primary key, name text CHECK NOT_NULL(name) AND LENGTH(name) > 1 MASKED WITH system.mask_default());"); + } + + /** + * TODO - investigate if it is possible to specify both check and mask, check being first + */ + @Test(expected = RuntimeException.class) + public void testFailingAlterTableAlterColumnWithCheckAndMask() + { + createTable("CREATE TABLE %s (pk text, name text, primary key (pk));"); + execute("ALTER TABLE %s ALTER name CHECK NOT_NULL(name) AND LENGTH(name) > 1 MASKED WITH system.mask_default();"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + /** + * TODO - investigate if it is possible to specify both check and mask, mask being first + */ + @Test(expected = RuntimeException.class) + public void testFailingAlterTableAlterColumnWithMaskAndCheck() + { + createTable("CREATE TABLE %s (pk text, name text, primary key (pk));"); + execute("ALTER TABLE %s ALTER name MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); Review Comment: I don't think we are never reaching this `verifyColumnSpec` right? The runtime exception is thrown by the execute. ########## test/unit/org/apache/cassandra/cql3/ColumnSpecificationTest.java: ########## @@ -0,0 +1,206 @@ +/* + * 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.util.Map; + +import org.junit.Before; +import org.junit.Test; + +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.schema.CompactionParams; + +/** + * Test various "extensions" to a column spec when altering / creating a table + */ +public class ColumnSpecificationTest extends CQLTester +{ + @Before + public void before() + { + DatabaseDescriptor.setDynamicDataMaskingEnabled(true); + } + + @Test + public void testCreateTableWithColumnHavingMaskBeforeCheck() + { + createTable("CREATE TABLE %s (pk text primary key, name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1);"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAlterTableAlterColumnWithMaskAndCheckStandalone() + { + createTable("CREATE TABLE %s (pk text, name text, primary key (pk));"); + execute("ALTER TABLE %s ALTER name MASKED WITH system.mask_default()"); + execute("ALTER TABLE %s ALTER name CHECK NOT_NULL(name) AND LENGTH(name) > 1;"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAlterTableAlterColumnWithMask() + { + createTable("CREATE TABLE %s (pk text, name text, primary key (pk));"); + execute("ALTER TABLE %s ALTER name MASKED WITH system.mask_default()"); + verifyColumnSpec("name text MASKED WITH system.mask_default()"); + } + + @Test + public void testAlterTableAlterColumnWithCheck() + { + createTable("CREATE TABLE %s (pk text, name text, primary key (pk));"); + execute("ALTER TABLE %s ALTER name CHECK NOT_NULL(name) AND LENGTH(name) > 1;"); + verifyColumnSpec("name text CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAddingCheckToColumnWithMask() + { + createTable("CREATE TABLE %s (pk text primary key, name text MASKED WITH system.mask_default());"); + execute("ALTER TABLE %s ALTER name CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAddingMaskToColumnWithCheck() + { + createTable("CREATE TABLE %s (pk text primary key, name text CHECK NOT_NULL(name) AND LENGTH(name) > 1);"); + execute("ALTER TABLE %s ALTER name MASKED WITH system.mask_default()"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testDroppingCheckKeepsMask() + { + createTable("CREATE TABLE %s (pk text primary key, name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1);"); + execute("ALTER TABLE %s ALTER name DROP CHECK"); + verifyColumnSpec("name text MASKED WITH system.mask_default()"); + } + + @Test + public void droppingMaskKeepsCheck() + { + createTable("CREATE TABLE %s (pk text primary key, name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1);"); + execute("ALTER TABLE %s ALTER name DROP MASKED"); + verifyColumnSpec("name text CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAlterTableAddColumnWithCheck() + { + createTable("CREATE TABLE %s (pk text primary key);"); + execute("ALTER TABLE %s ADD name text CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + verifyColumnSpec("name text CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + @Test + public void testAlterTableAddColumnWithMask() + { + createTable("CREATE TABLE %s (pk text primary key);"); + execute("ALTER TABLE %s ADD name text MASKED WITH system.mask_default()"); + verifyColumnSpec("name text MASKED WITH system.mask_default()"); + } + + @Test + public void testAlterTableAddColumnWithMaskAndCheck() + { + createTable("CREATE TABLE %s (pk text primary key);"); + execute("ALTER TABLE %s ADD name text MASKED WITH system.mask_default() CHECK NOT_NULL(name)"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name)"); + } + + @Test + public void testAlterTableAddColumnWithMaskAndMultipleChecks() + { + createTable("CREATE TABLE %s (pk text primary key);"); + execute("ALTER TABLE %s ADD name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); + } + + /** + * TODO - investigate if it is possible to specify checks before mask when creating a table + */ + @Test(expected = RuntimeException.class) + public void testFailingCreateTableWithColumnHavingMaskAfterCheck() + { + createTable("CREATE TABLE %s (pk text primary key, name text CHECK NOT_NULL(name) AND LENGTH(name) > 1 MASKED WITH system.mask_default());"); + } + + /** + * TODO - investigate if it is possible to specify both check and mask, check being first + */ + @Test(expected = RuntimeException.class) + public void testFailingAlterTableAlterColumnWithCheckAndMask() + { + createTable("CREATE TABLE %s (pk text, name text, primary key (pk));"); + execute("ALTER TABLE %s ALTER name CHECK NOT_NULL(name) AND LENGTH(name) > 1 MASKED WITH system.mask_default();"); + verifyColumnSpec("name text MASKED WITH system.mask_default() CHECK NOT_NULL(name) AND LENGTH(name) > 1"); Review Comment: I don't think we are never reaching this `verifyColumnSpec` right? The runtime exception is thrown by the execute. ########## src/java/org/apache/cassandra/cql3/constraints/ColumnConstraints.java: ########## @@ -120,18 +120,21 @@ public boolean hasRelevantConstraints() public void validate(ColumnMetadata columnMetadata) throws InvalidConstraintDefinitionException { if (!columnMetadata.type.isConstrainable()) + { throw new InvalidConstraintDefinitionException("Constraint cannot be defined on the column " + columnMetadata.name + " of type " + columnMetadata.type.asCQL3Type() - + " for the table " + columnMetadata.ksName + "." + columnMetadata.cfName); + + " for the table " + columnMetadata.ksName + '.' + columnMetadata.cfName + '.' + + (columnMetadata.type.isCollection() ? " When using collections, constraints can be used only of frozen collections." : "")); + } + + // this validation will check whether it makes sense to execute such constraint on a given column + for (ColumnConstraint<?> constraint : constraints) + constraint.validate(columnMetadata); Review Comment: Why checking the actual constraints before the satisfiability check? I think this may be potentially more expensive (thinking about the regex constraint) -- 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]
