diff --git a/commit_message.txt b/commit_message.txt new file mode 100644 index 000000000000..ceb5ca8fb96e --- /dev/null +++ b/commit_message.txt @@ -0,0 +1,13 @@ +Restrict BytesType compatibility to scalar types only + +BytesType.isValueCompatibleWith() previously accepted all types, +including frozen collections and UDTs. This was incorrect because +converting collections or UDTs to raw bytes is nonsensical - it +allows reading raw serialized bytes which have no meaningful +interpretation. + +This patch restricts BytesType to only accept simple scalar types +by checking !isCollection() && !isUDT() in isValueCompatibleWithInternal(). + +patch by Dekrate for CASSANDRA-20982 + diff --git a/src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java b/src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java index c7a57139d129..240844ebd84a 100644 --- a/src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/schema/AlterTableStatement.java @@ -362,9 +362,10 @@ private void addColumn(KeyspaceMetadata keyspace, // columns is pushed deeper down the line. The latter would still be problematic in cases of schema races. if (!type.isSerializationCompatibleWith(droppedColumn.type)) { - throw ire("Cannot re-add previously dropped column '%s' of type %s, incompatible with previous type %s", + throw ire("Cannot add a column '%s' of type %s, incompatible with previously dropped column '%s' of type %s", name, type.asCQL3Type(), + name, droppedColumn.type.asCQL3Type()); } diff --git a/src/java/org/apache/cassandra/db/marshal/BytesType.java b/src/java/org/apache/cassandra/db/marshal/BytesType.java index 5ee02fcc6e92..1fe8a9e73fdb 100644 --- a/src/java/org/apache/cassandra/db/marshal/BytesType.java +++ b/src/java/org/apache/cassandra/db/marshal/BytesType.java @@ -94,6 +94,14 @@ public boolean isValueCompatibleWithInternal(AbstractType otherType) return true; } + @Override + public boolean isSerializationCompatibleWith(AbstractType previous) + { + // BytesType should only be compatible with simple scalar types, not with collections or UDTs + // because converting a collection or UDT to raw bytes is nonsensical + return !previous.isCollection() && !previous.isUDT() && super.isSerializationCompatibleWith(previous); + } + public CQL3Type asCQL3Type() { return CQL3Type.Native.BLOB; diff --git a/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java b/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java index d21d11bdd34b..150b31334c09 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java @@ -54,7 +54,7 @@ public void testNonFrozenCollectionsAreIncompatibleWithBlob() throws Throwable { createTable("CREATE TABLE %s (a int, b " + type + ", PRIMARY KEY (a));"); alterTable("ALTER TABLE %s DROP b;"); - assertInvalidMessage("Cannot re-add previously dropped column 'b' of type blob, incompatible with previous type " + type, + assertInvalidMessage("Cannot add a column 'b' of type blob, incompatible with previously dropped column 'b' of type " + type, "ALTER TABLE %s ADD b blob;"); } @@ -62,13 +62,13 @@ public void testNonFrozenCollectionsAreIncompatibleWithBlob() throws Throwable { createTable("CREATE TABLE %s (a int, b blob, PRIMARY KEY (a));"); alterTable("ALTER TABLE %s DROP b;"); - assertInvalidMessage("Cannot re-add previously dropped column 'b' of type " + type + ", incompatible with previous type blob", + assertInvalidMessage("Cannot add a column 'b' of type " + type + ", incompatible with previously dropped column 'b' of type blob", "ALTER TABLE %s ADD b " + type + ';'); } } @Test - public void testFrozenCollectionsAreCompatibleWithBlob() + public void testFrozenCollectionsAreNotCompatibleWithBlob() { String[] collectionTypes = new String[] {"frozen>", "frozen>", "frozen>"}; @@ -76,7 +76,8 @@ public void testFrozenCollectionsAreCompatibleWithBlob() { createTable("CREATE TABLE %s (a int, b " + type + ", PRIMARY KEY (a));"); alterTable("ALTER TABLE %s DROP b;"); - alterTable("ALTER TABLE %s ADD b blob;"); + assertInvalidMessage("Cannot add a column 'b' of type blob, incompatible with previously dropped column 'b' of type " + type, + "ALTER TABLE %s ADD b blob;"); } } diff --git a/test/unit/org/apache/cassandra/db/marshal/BytesTypeTest.java b/test/unit/org/apache/cassandra/db/marshal/BytesTypeTest.java index 789794044a7f..0e8a5fe92409 100644 --- a/test/unit/org/apache/cassandra/db/marshal/BytesTypeTest.java +++ b/test/unit/org/apache/cassandra/db/marshal/BytesTypeTest.java @@ -19,9 +19,16 @@ */ package org.apache.cassandra.db.marshal; +import java.util.Arrays; + +import org.apache.cassandra.cql3.FieldIdentifier; import org.apache.cassandra.serializers.MarshalException; +import org.apache.cassandra.utils.ByteBufferUtil; import org.junit.Test; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + public class BytesTypeTest { private static final String INVALID_HEX = "33AG45F"; // Invalid (has a G) @@ -38,4 +45,68 @@ public void testFromStringWithValidString() { BytesType.instance.fromString(VALID_HEX); } + + @Test + public void testValueCompatibilityWithScalarTypes() + { + // BytesType should be value-compatible with simple scalar types + assertTrue("BytesType should be value-compatible with AsciiType", + BytesType.instance.isValueCompatibleWith(AsciiType.instance)); + assertTrue("BytesType should be value-compatible with UTF8Type", + BytesType.instance.isValueCompatibleWith(UTF8Type.instance)); + assertTrue("BytesType should be value-compatible with Int32Type", + BytesType.instance.isValueCompatibleWith(Int32Type.instance)); + assertTrue("BytesType should be value-compatible with LongType", + BytesType.instance.isValueCompatibleWith(LongType.instance)); + assertTrue("BytesType should be value-compatible with UUIDType", + BytesType.instance.isValueCompatibleWith(UUIDType.instance)); + assertTrue("BytesType should be value-compatible with BooleanType", + BytesType.instance.isValueCompatibleWith(BooleanType.instance)); + assertTrue("BytesType should be value-compatible with itself", + BytesType.instance.isValueCompatibleWith(BytesType.instance)); + } + + @Test + public void testSerializationIncompatibilityWithCollections() + { + // BytesType should NOT be serialization-compatible with collections (even frozen ones) + // because converting a collection to raw bytes for schema changes is nonsensical + ListType frozenList = ListType.getInstance(Int32Type.instance, false); + SetType frozenSet = SetType.getInstance(Int32Type.instance, false); + MapType frozenMap = MapType.getInstance(Int32Type.instance, UTF8Type.instance, false); + + assertFalse("BytesType should NOT be serialization-compatible with frozen", + BytesType.instance.isSerializationCompatibleWith(frozenList)); + assertFalse("BytesType should NOT be serialization-compatible with frozen", + BytesType.instance.isSerializationCompatibleWith(frozenSet)); + assertFalse("BytesType should NOT be serialization-compatible with frozen", + BytesType.instance.isSerializationCompatibleWith(frozenMap)); + + // Also test with multi-cell collections + ListType multiCellList = ListType.getInstance(Int32Type.instance, true); + SetType multiCellSet = SetType.getInstance(Int32Type.instance, true); + MapType multiCellMap = MapType.getInstance(Int32Type.instance, UTF8Type.instance, true); + + assertFalse("BytesType should NOT be serialization-compatible with list", + BytesType.instance.isSerializationCompatibleWith(multiCellList)); + assertFalse("BytesType should NOT be serialization-compatible with set", + BytesType.instance.isSerializationCompatibleWith(multiCellSet)); + assertFalse("BytesType should NOT be serialization-compatible with map", + BytesType.instance.isSerializationCompatibleWith(multiCellMap)); + } + + @Test + public void testSerializationIncompatibilityWithUDT() + { + // BytesType should NOT be serialization-compatible with User Defined Types + // because converting a UDT to raw bytes for schema changes is nonsensical + UserType udt = new UserType("ks", + ByteBufferUtil.bytes("myType"), + Arrays.asList(FieldIdentifier.forQuoted("field1"), FieldIdentifier.forQuoted("field2")), + Arrays.asList(Int32Type.instance, UTF8Type.instance), + true); + + assertFalse("BytesType should NOT be serialization-compatible with UDT", + BytesType.instance.isSerializationCompatibleWith(udt)); + } }