Repository: cassandra Updated Branches: refs/heads/trunk e75ebc4f6 -> 21b134c2e
Prevent ALTER TYPE from creating circular references patch by Robert Stupp; reviewed by Sylvain Lebresne for CASSANDRA-10339 Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/98c4a7cd Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/98c4a7cd Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/98c4a7cd Branch: refs/heads/trunk Commit: 98c4a7cd02022c55f174da16d074e3fc62203aac Parents: 0b8b67b Author: Robert Stupp <[email protected]> Authored: Fri Sep 18 09:48:17 2015 +0200 Committer: Robert Stupp <[email protected]> Committed: Fri Sep 18 09:48:17 2015 +0200 ---------------------------------------------------------------------- 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 +++ .../apache/cassandra/db/marshal/TupleType.java | 13 +++++- .../cql3/validation/entities/UserTypesTest.java | 49 ++++++++++++++++++++ 10 files changed, 108 insertions(+), 3 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/98c4a7cd/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index e2dd83a..166106d 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) http://git-wip-us.apache.org/repos/asf/cassandra/blob/98c4a7cd/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java b/src/java/org/apache/cassandra/cql3/statements/AlterTypeStatement.java index 74fafd6..6459e6b 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<AbstractType<?>> 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); } http://git-wip-us.apache.org/repos/asf/cassandra/blob/98c4a7cd/src/java/org/apache/cassandra/db/marshal/AbstractType.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/marshal/AbstractType.java b/src/java/org/apache/cassandra/db/marshal/AbstractType.java index 863cd47..bcb33dc 100644 --- a/src/java/org/apache/cassandra/db/marshal/AbstractType.java +++ b/src/java/org/apache/cassandra/db/marshal/AbstractType.java @@ -262,6 +262,14 @@ public abstract class AbstractType<T> implements Comparator<ByteBuffer> } /** + * 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()). * http://git-wip-us.apache.org/repos/asf/cassandra/blob/98c4a7cd/src/java/org/apache/cassandra/db/marshal/CompositeType.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/marshal/CompositeType.java b/src/java/org/apache/cassandra/db/marshal/CompositeType.java index 9ee9fb3..f8ac22d 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 http://git-wip-us.apache.org/repos/asf/cassandra/blob/98c4a7cd/src/java/org/apache/cassandra/db/marshal/ListType.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/marshal/ListType.java b/src/java/org/apache/cassandra/db/marshal/ListType.java index 510a526..c2f36c0 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<T> extends CollectionType<List<T>> this.isMultiCell = isMultiCell; } + @Override + public boolean references(AbstractType<?> check) + { + return super.references(check) || elements.references(check); + } + public AbstractType<T> getElementsType() { return elements; http://git-wip-us.apache.org/repos/asf/cassandra/blob/98c4a7cd/src/java/org/apache/cassandra/db/marshal/MapType.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/marshal/MapType.java b/src/java/org/apache/cassandra/db/marshal/MapType.java index 64f3e2a..0fb545d 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<K, V> extends CollectionType<Map<K, V>> this.isMultiCell = isMultiCell; } + @Override + public boolean references(AbstractType<?> check) + { + return super.references(check) || keys.references(check) || values.references(check); + } + public AbstractType<K> getKeysType() { return keys; http://git-wip-us.apache.org/repos/asf/cassandra/blob/98c4a7cd/src/java/org/apache/cassandra/db/marshal/ReversedType.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/marshal/ReversedType.java b/src/java/org/apache/cassandra/db/marshal/ReversedType.java index 1323dc6..0389a32 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<T> extends AbstractType<T> return baseType.getSerializer(); } + public boolean references(AbstractType<?> check) + { + return super.references(check) || baseType.references(check); + } + @Override public String toString() { http://git-wip-us.apache.org/repos/asf/cassandra/blob/98c4a7cd/src/java/org/apache/cassandra/db/marshal/SetType.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/marshal/SetType.java b/src/java/org/apache/cassandra/db/marshal/SetType.java index e10f2a1..b635208 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<T> extends CollectionType<Set<T>> this.isMultiCell = isMultiCell; } + @Override + public boolean references(AbstractType<?> check) + { + return super.references(check) || elements.references(check); + } + public AbstractType<T> getElementsType() { return elements; http://git-wip-us.apache.org/repos/asf/cassandra/blob/98c4a7cd/src/java/org/apache/cassandra/db/marshal/TupleType.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/marshal/TupleType.java b/src/java/org/apache/cassandra/db/marshal/TupleType.java index e07319b..dbce0db 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<ByteBuffer> 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); http://git-wip-us.apache.org/repos/asf/cassandra/blob/98c4a7cd/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java ---------------------------------------------------------------------- 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 ee647d6..d96abdb 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 <int,text>"); 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<list<" + 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<set<" + 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<map<text, " + 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<map<" + typeWithKs(type1) + ", text>>)"); + 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<list<" + 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<set<" + 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<map<text, " + 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<map<" + keyspace() + '.' + type2 + ", text>>)"); + 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<list<" + typeWithKs(type1) + ">>"); + } + + private String typeWithKs(String type1) + { + return keyspace() + '.' + type1; + } }
