Author: xedin Date: Wed Aug 17 22:17:03 2011 New Revision: 1158939 URL: http://svn.apache.org/viewvc?rev=1158939&view=rev Log: Validate that column names in column_metadata does not equal to key_alias on create/update of the ColumnFamily and CQL 'ALTER' statement. patch by Pavel Yaskevich; reviewed by Jonathan Ellis for CASSANDRA-3036
Modified: cassandra/branches/cassandra-0.8/CHANGES.txt cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/AlterTableStatement.java cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/CreateColumnFamilyStatement.java cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/QueryProcessor.java cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/thrift/ThriftValidation.java cassandra/branches/cassandra-0.8/test/system/test_cql.py cassandra/branches/cassandra-0.8/test/unit/org/apache/cassandra/thrift/ThriftValidationTest.java Modified: cassandra/branches/cassandra-0.8/CHANGES.txt URL: http://svn.apache.org/viewvc/cassandra/branches/cassandra-0.8/CHANGES.txt?rev=1158939&r1=1158938&r2=1158939&view=diff ============================================================================== --- cassandra/branches/cassandra-0.8/CHANGES.txt (original) +++ cassandra/branches/cassandra-0.8/CHANGES.txt Wed Aug 17 22:17:03 2011 @@ -13,7 +13,8 @@ (rows containing nothing but expired tombstones) (CASSANDRA-3039) * work around native memory leak in com.sun.management.GarbageCollectorMXBean (CASSANDRA-2868) - + * validate that column names in column_metadata does not equal to key_alias + on create/update of the ColumnFamily and CQL 'ALTER' statement (CASSANDRA-3036) 0.8.4 * include files-to-be-streamed in StreamInSession.getSources (CASSANDRA-2972) Modified: cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/AlterTableStatement.java URL: http://svn.apache.org/viewvc/cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/AlterTableStatement.java?rev=1158939&r1=1158938&r2=1158939&view=diff ============================================================================== --- cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/AlterTableStatement.java (original) +++ cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/AlterTableStatement.java Wed Aug 17 22:17:03 2011 @@ -65,10 +65,15 @@ public class AlterTableStatement switch (oType) { case ADD: + if (cfDef.key_alias != null && cfDef.key_alias.equals(columnName)) + throw new InvalidRequestException("Invalid column name: " + + this.columnName + + ", because it equals to key_alias."); + cfDef.column_metadata.add(new ColumnDefinition(columnName, - TypeParser.parse(validator), - null, - null).deflate()); + TypeParser.parse(validator), + null, + null).deflate()); break; case ALTER: Modified: cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/CreateColumnFamilyStatement.java URL: http://svn.apache.org/viewvc/cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/CreateColumnFamilyStatement.java?rev=1158939&r1=1158938&r2=1158939&view=diff ============================================================================== --- cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/CreateColumnFamilyStatement.java (original) +++ cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/CreateColumnFamilyStatement.java Wed Aug 17 22:17:03 2011 @@ -169,6 +169,28 @@ public class CreateColumnFamilyStatement throw new InvalidRequestException("You must specify a PRIMARY KEY"); else if (keyValidator.size() > 1) throw new InvalidRequestException("You may only specify one PRIMARY KEY"); + + AbstractType<?> comparator; + + try + { + comparator = getComparator(); + } + catch (ConfigurationException e) + { + throw new InvalidRequestException(e.toString()); + } + + for (Map.Entry<Term, String> column : columns.entrySet()) + { + ByteBuffer name = column.getKey().getByteBuffer(comparator); + + if (keyAlias != null && keyAlias.equals(name)) + throw new InvalidRequestException("Invalid column name: " + + column.getKey().getText() + + ", because it equals to the key_alias."); + + } } /** Map a column name to a validator for its value */ @@ -215,7 +237,7 @@ public class CreateColumnFamilyStatement { try { - ByteBuffer columnName = col.getKey().getByteBuffer(comparator); + ByteBuffer columnName = comparator.fromString(col.getKey().getText()); String validatorClassName = comparators.containsKey(col.getValue()) ? comparators.get(col.getValue()) : col.getValue(); AbstractType<?> validator = TypeParser.parse(validatorClassName); columnDefs.put(columnName, new ColumnDefinition(columnName, validator, null, null)); @@ -230,7 +252,26 @@ public class CreateColumnFamilyStatement return columnDefs; } - + + /* If not comparator/validator is not specified, default to text (BytesType is the wrong default for CQL + * since it uses hex terms). If the value specified is not found in the comparators map, assume the user + * knows what they are doing (a custom comparator/validator for example), and pass it on as-is. + */ + + private AbstractType<?> getComparator() throws ConfigurationException + { + return TypeParser.parse((comparators.get(getPropertyString(KW_COMPARATOR, "text")) != null) + ? comparators.get(getPropertyString(KW_COMPARATOR, "text")) + : getPropertyString(KW_COMPARATOR, "text")); + } + + private AbstractType<?> getValidator() throws ConfigurationException + { + return TypeParser.parse((comparators.get(getPropertyString(KW_DEFAULTVALIDATION, "text")) != null) + ? comparators.get(getPropertyString(KW_DEFAULTVALIDATION, "text")) + : getPropertyString(KW_DEFAULTVALIDATION, "text")); + } + /** * Returns a CFMetaData instance based on the parameters parsed from this * <code>CREATE</code> statement, or defaults where applicable. @@ -246,17 +287,7 @@ public class CreateColumnFamilyStatement CFMetaData newCFMD; try { - /* If not comparator/validator is not specified, default to text (BytesType is the wrong default for CQL - * since it uses hex terms). If the value specified is not found in the comparators map, assume the user - * knows what they are doing (a custom comparator/validator for example), and pass it on as-is. - */ - String comparatorString = (comparators.get(getPropertyString(KW_COMPARATOR, "text")) != null) - ? comparators.get(getPropertyString(KW_COMPARATOR, "text")) - : getPropertyString(KW_COMPARATOR, "text"); - String validatorString = (comparators.get(getPropertyString(KW_DEFAULTVALIDATION, "text")) != null) - ? comparators.get(getPropertyString(KW_DEFAULTVALIDATION, "text")) - : getPropertyString(KW_DEFAULTVALIDATION, "text"); - AbstractType<?> comparator = TypeParser.parse(comparatorString); + AbstractType<?> comparator = getComparator(); newCFMD = new CFMetaData(keyspace, name, @@ -270,7 +301,7 @@ public class CreateColumnFamilyStatement .readRepairChance(getPropertyDouble(KW_READREPAIRCHANCE, CFMetaData.DEFAULT_READ_REPAIR_CHANCE)) .replicateOnWrite(getPropertyBoolean(KW_REPLICATEONWRITE, CFMetaData.DEFAULT_REPLICATE_ON_WRITE)) .gcGraceSeconds(getPropertyInt(KW_GCGRACESECONDS, CFMetaData.DEFAULT_GC_GRACE_SECONDS)) - .defaultValidator(TypeParser.parse(validatorString)) + .defaultValidator(getValidator()) .minCompactionThreshold(getPropertyInt(KW_MINCOMPACTIONTHRESHOLD, CFMetaData.DEFAULT_MIN_COMPACTION_THRESHOLD)) .maxCompactionThreshold(getPropertyInt(KW_MAXCOMPACTIONTHRESHOLD, CFMetaData.DEFAULT_MAX_COMPACTION_THRESHOLD)) .rowCacheSavePeriod(getPropertyInt(KW_ROWCACHESAVEPERIODSECS, CFMetaData.DEFAULT_ROW_CACHE_SAVE_PERIOD_IN_SECONDS)) Modified: cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/QueryProcessor.java URL: http://svn.apache.org/viewvc/cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/QueryProcessor.java?rev=1158939&r1=1158938&r2=1158939&view=diff ============================================================================== --- cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/QueryProcessor.java (original) +++ cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/cql/QueryProcessor.java Wed Aug 17 22:17:03 2011 @@ -845,8 +845,6 @@ public class QueryProcessor case ALTER_TABLE: AlterTableStatement alterTable = (AlterTableStatement) statement.statement; - System.out.println(alterTable); - validateColumnFamily(keyspace, alterTable.columnFamily); clientState.hasColumnFamilyAccess(alterTable.columnFamily, Permission.WRITE); validateSchemaAgreement(); Modified: cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/thrift/ThriftValidation.java URL: http://svn.apache.org/viewvc/cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/thrift/ThriftValidation.java?rev=1158939&r1=1158938&r2=1158939&view=diff ============================================================================== --- cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/thrift/ThriftValidation.java (original) +++ cassandra/branches/cassandra-0.8/src/java/org/apache/cassandra/thrift/ThriftValidation.java Wed Aug 17 22:17:03 2011 @@ -553,22 +553,6 @@ public class ThriftValidation } } - if (cf_def.key_alias != null) - { - if (!cf_def.key_alias.hasRemaining()) - throw new InvalidRequestException("key_alias may not be empty"); - try - { - // it's hard to use a key in a select statement if we can't type it. - // for now let's keep it simple and require ascii. - AsciiType.instance.validate(cf_def.key_alias); - } - catch (MarshalException e) - { - throw new InvalidRequestException("Key aliases must be ascii"); - } - } - ColumnFamilyType cfType = ColumnFamilyType.create(cf_def.column_type); if (cfType == null) throw new InvalidRequestException("invalid column type " + cf_def.column_type); @@ -586,6 +570,18 @@ public class ThriftValidation ? TypeParser.parse(cf_def.comparator_type) : TypeParser.parse(cf_def.subcomparator_type); + if (cf_def.key_alias != null) + { + // check if any of the columns has name equal to the cf.key_alias + for (ColumnDef columnDef : cf_def.column_metadata) + { + if (cf_def.key_alias.equals(columnDef.name)) + throw new InvalidRequestException("Invalid column name: " + + AsciiType.instance.compose(cf_def.key_alias) + + ", because it equals to the key_alias."); + } + } + // initialize a set of names NOT in the CF under consideration Set<String> indexNames = new HashSet<String>(); for (ColumnFamilyStore cfs : ColumnFamilyStore.all()) Modified: cassandra/branches/cassandra-0.8/test/system/test_cql.py URL: http://svn.apache.org/viewvc/cassandra/branches/cassandra-0.8/test/system/test_cql.py?rev=1158939&r1=1158938&r2=1158939&view=diff ============================================================================== --- cassandra/branches/cassandra-0.8/test/system/test_cql.py (original) +++ cassandra/branches/cassandra-0.8/test/system/test_cql.py Wed Aug 17 22:17:03 2011 @@ -477,6 +477,9 @@ class TestCql(ThriftTester): # Missing primary key assert_raises(cql.ProgrammingError, cursor.execute, "CREATE COLUMNFAMILY NewCf2") + # column name should not match key alias + assert_raises(cql.ProgrammingError, cursor.execute, "CREATE COLUMNFAMILY NewCf2 (id 'utf8' primary key, id int)") + # Too many primary keys assert_raises(cql.ProgrammingError, cursor.execute, @@ -1140,7 +1143,7 @@ class TestCql(ThriftTester): cursor.execute("USE AlterTableKS;") cursor.execute(""" - CREATE COLUMNFAMILY NewCf1 (KEY varint PRIMARY KEY) WITH default_validation = ascii; + CREATE COLUMNFAMILY NewCf1 (id_key varint PRIMARY KEY) WITH default_validation = ascii; """) # TODO: temporary (until this can be done with CQL). @@ -1204,6 +1207,12 @@ class TestCql(ThriftTester): assert_raises(cql.ProgrammingError, cursor.execute, "ALTER COLUMNFAMILY NewCf1 DROP name") + + # should raise error when column name equals key alias + assert_raises(cql.ProgrammingError, + cursor.execute, + "ALTER COLUMNFAMILY NewCf1 ADD id_key utf8") + def test_counter_column_support(self): "update statement should be able to work with counter columns" Modified: cassandra/branches/cassandra-0.8/test/unit/org/apache/cassandra/thrift/ThriftValidationTest.java URL: http://svn.apache.org/viewvc/cassandra/branches/cassandra-0.8/test/unit/org/apache/cassandra/thrift/ThriftValidationTest.java?rev=1158939&r1=1158938&r2=1158939&view=diff ============================================================================== --- cassandra/branches/cassandra-0.8/test/unit/org/apache/cassandra/thrift/ThriftValidationTest.java (original) +++ cassandra/branches/cassandra-0.8/test/unit/org/apache/cassandra/thrift/ThriftValidationTest.java Wed Aug 17 22:17:03 2011 @@ -21,10 +21,17 @@ package org.apache.cassandra.thrift; */ +import org.apache.cassandra.config.CFMetaData; +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.db.marshal.AsciiType; +import org.apache.cassandra.db.marshal.UTF8Type; + import org.junit.Test; import org.apache.cassandra.CleanupHelper; +import java.util.concurrent.Callable; + public class ThriftValidationTest extends CleanupHelper { @Test(expected=InvalidRequestException.class) @@ -38,4 +45,46 @@ public class ThriftValidationTest extend { ThriftValidation.validateColumnFamily("Keyspace1", "Counter1", true); } + + @Test + public void testColumnNameEqualToKeyAlias() + { + CFMetaData metaData = DatabaseDescriptor.getCFMetaData("Keyspace1", "Standard1"); + CfDef newMetadata = CFMetaData.convertToThrift(metaData); + + boolean gotException = false; + + // add a key_alias = "id" + newMetadata.setKey_alias(AsciiType.instance.decompose("id")); + + // should not throw IRE here + try + { + ThriftValidation.validateCfDef(newMetadata, metaData); + } + catch (InvalidRequestException e) + { + gotException = true; + } + + assert !gotException : "got unexpected InvalidRequestException"; + + // add a column with name = "id" + newMetadata.addToColumn_metadata(new ColumnDef(UTF8Type.instance.decompose("id"), + "org.apache.cassandra.db.marshal.UTF8Type")); + + + gotException = false; + + try + { + ThriftValidation.validateCfDef(newMetadata, metaData); + } + catch (InvalidRequestException e) + { + gotException = true; + } + + assert gotException : "expected InvalidRequestException but not received."; + } }