From 98c4a7cd02022c55f174da16d074e3fc62203aac Mon Sep 17 00:00:00 2001 From: Robert Stupp Date: Fri, 18 Sep 2015 09:48:17 +0200 Subject: [PATCH] Prevent ALTER TYPE from creating circular references patch by Robert Stupp; reviewed by Sylvain Lebresne for CASSANDRA-10339 --- CHANGES.txt | 1 + .../cql3/statements/AlterTypeStatement.java | 6 ++- .../cassandra/db/marshal/AbstractType.java | 8 +++ .../cassandra/db/marshal/CompositeType.java | 11 +++++ .../apache/cassandra/db/marshal/ListType.java | 6 +++ .../apache/cassandra/db/marshal/MapType.java | 6 +++ .../cassandra/db/marshal/ReversedType.java | 5 ++ .../apache/cassandra/db/marshal/SetType.java | 6 +++ .../cassandra/db/marshal/TupleType.java | 13 ++++- .../validation/entities/UserTypesTest.java | 49 +++++++++++++++++++ 10 files changed, 108 insertions(+), 3 deletions(-) diff --git a/CHANGES.txt b/CHANGES.txt index e2dd83ad31..166106d7ec 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 2.1.10 + * Prevent ALTER TYPE from creating circular references (CASSANDRA-10339) * Fix cache handling of 2i and base tables (CASSANDRA-10155, 10359) * Fix NPE in nodetool compactionhistory (CASSANDRA-9758) * (Pig) support BulkOutputFormat as a URL parameter (CASSANDRA-7410) diff --git a/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java b/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java index 74fafd67a8..6459e6bd11 100644 --- a/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java +++ b/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java @@ -294,9 +294,13 @@ public abstract class AlterTypeStatement extends SchemaAlteringStatement newNames.addAll(toUpdate.fieldNames()); newNames.add(fieldName.bytes); + AbstractType addType = type.prepare(keyspace()).getType(); + if (addType.references(toUpdate)) + throw new InvalidRequestException(String.format("Cannot add new field %s of type %s to type %s as this would create a circular reference", fieldName, type, name)); + List> newTypes = new ArrayList<>(toUpdate.size() + 1); newTypes.addAll(toUpdate.fieldTypes()); - newTypes.add(type.prepare(keyspace()).getType()); + newTypes.add(addType); return new UserType(toUpdate.keyspace, toUpdate.name, newNames, newTypes); } diff --git a/src/java/org/apache/cassandra/db/marshal/AbstractType.java b/src/java/org/apache/cassandra/db/marshal/AbstractType.java index 863cd47f6e..bcb33dc2c9 100644 --- a/src/java/org/apache/cassandra/db/marshal/AbstractType.java +++ b/src/java/org/apache/cassandra/db/marshal/AbstractType.java @@ -261,6 +261,14 @@ public abstract class AbstractType implements Comparator return Collections.>singletonList(this); } + /** + * Checks whether this type or any of the types this type contains references the given type. + */ + public boolean references(AbstractType check) + { + return this.equals(check); + } + /** * This must be overriden by subclasses if necessary so that for any * AbstractType, this == TypeParser.parse(toString()). diff --git a/src/java/org/apache/cassandra/db/marshal/CompositeType.java b/src/java/org/apache/cassandra/db/marshal/CompositeType.java index 9ee9fb3032..f8ac22deba 100644 --- a/src/java/org/apache/cassandra/db/marshal/CompositeType.java +++ b/src/java/org/apache/cassandra/db/marshal/CompositeType.java @@ -115,6 +115,17 @@ public class CompositeType extends AbstractCompositeType this.types = ImmutableList.copyOf(types); } + @Override + public boolean references(AbstractType check) + { + if (super.references(check)) + return true; + for (AbstractType type : types) + if (type.references(check)) + return true; + return false; + } + protected AbstractType getComparator(int i, ByteBuffer bb) { try diff --git a/src/java/org/apache/cassandra/db/marshal/ListType.java b/src/java/org/apache/cassandra/db/marshal/ListType.java index 510a526309..c2f36c010d 100644 --- a/src/java/org/apache/cassandra/db/marshal/ListType.java +++ b/src/java/org/apache/cassandra/db/marshal/ListType.java @@ -69,6 +69,12 @@ public class ListType extends CollectionType> this.isMultiCell = isMultiCell; } + @Override + public boolean references(AbstractType check) + { + return super.references(check) || elements.references(check); + } + public AbstractType getElementsType() { return elements; diff --git a/src/java/org/apache/cassandra/db/marshal/MapType.java b/src/java/org/apache/cassandra/db/marshal/MapType.java index 64f3e2a98b..0fb545d65f 100644 --- a/src/java/org/apache/cassandra/db/marshal/MapType.java +++ b/src/java/org/apache/cassandra/db/marshal/MapType.java @@ -70,6 +70,12 @@ public class MapType extends CollectionType> this.isMultiCell = isMultiCell; } + @Override + public boolean references(AbstractType check) + { + return super.references(check) || keys.references(check) || values.references(check); + } + public AbstractType getKeysType() { return keys; diff --git a/src/java/org/apache/cassandra/db/marshal/ReversedType.java b/src/java/org/apache/cassandra/db/marshal/ReversedType.java index 1323dc66bc..0389a32df2 100644 --- a/src/java/org/apache/cassandra/db/marshal/ReversedType.java +++ b/src/java/org/apache/cassandra/db/marshal/ReversedType.java @@ -109,6 +109,11 @@ public class ReversedType extends AbstractType return baseType.getSerializer(); } + public boolean references(AbstractType check) + { + return super.references(check) || baseType.references(check); + } + @Override public String toString() { diff --git a/src/java/org/apache/cassandra/db/marshal/SetType.java b/src/java/org/apache/cassandra/db/marshal/SetType.java index e10f2a16f3..b6352080d5 100644 --- a/src/java/org/apache/cassandra/db/marshal/SetType.java +++ b/src/java/org/apache/cassandra/db/marshal/SetType.java @@ -64,6 +64,12 @@ public class SetType extends CollectionType> this.isMultiCell = isMultiCell; } + @Override + public boolean references(AbstractType check) + { + return super.references(check) || elements.references(check); + } + public AbstractType getElementsType() { return elements; diff --git a/src/java/org/apache/cassandra/db/marshal/TupleType.java b/src/java/org/apache/cassandra/db/marshal/TupleType.java index e07319b69b..dbce0db82d 100644 --- a/src/java/org/apache/cassandra/db/marshal/TupleType.java +++ b/src/java/org/apache/cassandra/db/marshal/TupleType.java @@ -23,8 +23,6 @@ import java.util.List; import com.google.common.base.Objects; -import org.apache.cassandra.exceptions.InvalidRequestException; - import org.apache.cassandra.cql3.CQL3Type; import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.exceptions.SyntaxException; @@ -54,6 +52,17 @@ public class TupleType extends AbstractType return new TupleType(types); } + @Override + public boolean references(AbstractType check) + { + if (super.references(check)) + return true; + for (AbstractType type : types) + if (type.references(check)) + return true; + return false; + } + public AbstractType type(int i) { return types.get(i); diff --git a/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java b/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java index ee647d68ab..d96abdb7c7 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java @@ -386,4 +386,53 @@ public class UserTypesTest extends CQLTester execute("ALTER TYPE " + keyspace() + "." + typeName + " ADD foomap map "); execute("INSERT INTO %s (key, data) VALUES (1, {fooint: 1, fooset: {'2'}, foomap: {3 : 'bar'}})"); } + + @Test + public void testCircularReferences() throws Throwable + { + String type1 = createType("CREATE TYPE %s (foo int)"); + + String typeX = createType("CREATE TYPE %s (bar frozen<" + typeWithKs(type1) + ">)"); + assertInvalidMessage("would create a circular reference", "ALTER TYPE " + typeWithKs(type1) + " ADD needs_to_fail frozen<" + typeWithKs(typeX) + '>'); + + typeX = createType("CREATE TYPE %s (bar frozen>)"); + assertInvalidMessage("would create a circular reference", "ALTER TYPE " + typeWithKs(type1) + " ADD needs_to_fail frozen<" + typeWithKs(typeX) + '>'); + + typeX = createType("CREATE TYPE %s (bar frozen>)"); + assertInvalidMessage("would create a circular reference", "ALTER TYPE " + typeWithKs(type1) + " ADD needs_to_fail frozen<" + typeWithKs(typeX) + '>'); + + typeX = createType("CREATE TYPE %s (bar frozen>)"); + assertInvalidMessage("would create a circular reference", "ALTER TYPE " + typeWithKs(type1) + " ADD needs_to_fail frozen<" + typeWithKs(typeX) + '>'); + + typeX = createType("CREATE TYPE %s (bar frozen>)"); + assertInvalidMessage("would create a circular reference", "ALTER TYPE " + typeWithKs(type1) + " ADD needs_to_fail frozen<" + typeWithKs(typeX) + '>'); + + // + + String type2 = createType("CREATE TYPE %s (foo frozen<" + typeWithKs(type1) + ">)"); + + typeX = createType("CREATE TYPE %s (bar frozen<" + keyspace() + '.' + type2 + ">)"); + assertInvalidMessage("would create a circular reference", "ALTER TYPE " + typeWithKs(type1) + " ADD needs_to_fail frozen<" + typeWithKs(typeX) + '>'); + + typeX = createType("CREATE TYPE %s (bar frozen>)"); + assertInvalidMessage("would create a circular reference", "ALTER TYPE " + typeWithKs(type1) + " ADD needs_to_fail frozen<" + typeWithKs(typeX) + '>'); + + typeX = createType("CREATE TYPE %s (bar frozen>)"); + assertInvalidMessage("would create a circular reference", "ALTER TYPE " + typeWithKs(type1) + " ADD needs_to_fail frozen<" + typeWithKs(typeX) + '>'); + + typeX = createType("CREATE TYPE %s (bar frozen>)"); + assertInvalidMessage("would create a circular reference", "ALTER TYPE " + typeWithKs(type1) + " ADD needs_to_fail frozen<" + typeWithKs(typeX) + '>'); + + typeX = createType("CREATE TYPE %s (bar frozen>)"); + assertInvalidMessage("would create a circular reference", "ALTER TYPE " + typeWithKs(type1) + " ADD needs_to_fail frozen<" + typeWithKs(typeX) + '>'); + + // + + assertInvalidMessage("would create a circular reference", "ALTER TYPE " + typeWithKs(type1) + " ADD needs_to_fail frozen>"); + } + + private String typeWithKs(String type1) + { + return keyspace() + '.' + type1; + } }