CASSANDRA-20982: Restrict BytesType value compatibility to scalar types, exclude collections and UDTs

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
+

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());
                 }
 

src/java/org/apache/cassandra/db/marshal/BytesType.java

@@ -90,8 +90,9 @@ public boolean isCompatibleWith(AbstractType<?> previous)
     @Override
     public boolean isValueCompatibleWithInternal(AbstractType<?> otherType)
     {
-        // BytesType can read anything
-        return true;
+        // 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 !otherType.isMultiCell();
     }
 
     public CQL3Type asCQL3Type()

test/unit/org/apache/cassandra/cql3/validation/operations/AlterTest.java

@@ -54,29 +54,30 @@ 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;");
         }
 
         for (String type : collectionTypes)
         {
             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<map<int, int>>", "frozen<set<int>>", "frozen<list<int>>"};
 
         for (String type : collectionTypes)
         {
             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;");
         }
     }
 

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 compatible with simple scalar types
+        assertTrue("BytesType should be compatible with AsciiType",
+                   BytesType.instance.isValueCompatibleWith(AsciiType.instance));
+        assertTrue("BytesType should be compatible with UTF8Type",
+                   BytesType.instance.isValueCompatibleWith(UTF8Type.instance));
+        assertTrue("BytesType should be compatible with Int32Type",
+                   BytesType.instance.isValueCompatibleWith(Int32Type.instance));
+        assertTrue("BytesType should be compatible with LongType",
+                   BytesType.instance.isValueCompatibleWith(LongType.instance));
+        assertTrue("BytesType should be compatible with UUIDType",
+                   BytesType.instance.isValueCompatibleWith(UUIDType.instance));
+        assertTrue("BytesType should be compatible with BooleanType",
+                   BytesType.instance.isValueCompatibleWith(BooleanType.instance));
+        assertTrue("BytesType should be compatible with itself",
+                   BytesType.instance.isValueCompatibleWith(BytesType.instance));
+    }
+
+    @Test
+    public void testValueIncompatibilityWithCollections()
+    {
+        // BytesType should NOT be compatible with collections (even frozen ones)
+        // because converting a collection to raw bytes 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 compatible with frozen<list>",
+                    BytesType.instance.isValueCompatibleWith(frozenList));
+        assertFalse("BytesType should NOT be compatible with frozen<set>",
+                    BytesType.instance.isValueCompatibleWith(frozenSet));
+        assertFalse("BytesType should NOT be compatible with frozen<map>",
+                    BytesType.instance.isValueCompatibleWith(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 compatible with list",
+                    BytesType.instance.isValueCompatibleWith(multiCellList));
+        assertFalse("BytesType should NOT be compatible with set",
+                    BytesType.instance.isValueCompatibleWith(multiCellSet));
+        assertFalse("BytesType should NOT be compatible with map",
+                    BytesType.instance.isValueCompatibleWith(multiCellMap));
+    }
+
+    @Test
+    public void testValueIncompatibilityWithUDT()
+    {
+        // BytesType should NOT be compatible with User Defined Types
+        // because converting a UDT to raw bytes 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 compatible with UDT",
+                    BytesType.instance.isValueCompatibleWith(udt));
+    }
 }