Fix nested Tuples/UDTs validation patch by Benjamin Lerer; reviewed by Andrés de la Peña for CASSANDRA-13646
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/b77e11cf Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/b77e11cf Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/b77e11cf Branch: refs/heads/trunk Commit: b77e11cfd51ddb0f3ac07530399abe999df0573e Parents: 0163b97 Author: Benjamin Lerer <b.le...@gmail.com> Authored: Fri Jul 7 13:29:00 2017 +0200 Committer: Benjamin Lerer <b.le...@gmail.com> Committed: Fri Jul 7 13:29:00 2017 +0200 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../org/apache/cassandra/cql3/UserTypes.java | 4 +- .../apache/cassandra/db/marshal/TupleType.java | 45 ++++--------- .../apache/cassandra/db/marshal/UserType.java | 50 +++++---------- .../cassandra/serializers/TupleSerializer.java | 66 +++++++++++++++++++ .../serializers/UserTypeSerializer.java | 67 ++++++++++++++++++++ .../validation/entities/CollectionsTest.java | 36 +++++++++++ .../cql3/validation/entities/TupleTypeTest.java | 10 ++- .../cql3/validation/entities/UserTypesTest.java | 9 +++ 9 files changed, 219 insertions(+), 69 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/b77e11cf/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index c966c92..08c6e48 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 2.2.11 + * Fix nested Tuples/UDTs validation (CASSANDRA-13646) * Remove unused max_value_size_in_mb config setting from yaml (CASSANDRA-13625 http://git-wip-us.apache.org/repos/asf/cassandra/blob/b77e11cf/src/java/org/apache/cassandra/cql3/UserTypes.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/cql3/UserTypes.java b/src/java/org/apache/cassandra/cql3/UserTypes.java index de3f545..6766d07 100644 --- a/src/java/org/apache/cassandra/cql3/UserTypes.java +++ b/src/java/org/apache/cassandra/cql3/UserTypes.java @@ -91,7 +91,7 @@ public abstract class UserTypes private void validateAssignableTo(String keyspace, ColumnSpecification receiver) throws InvalidRequestException { if (!(receiver.type instanceof UserType)) - throw new InvalidRequestException(String.format("Invalid user type literal for %s of type %s", receiver, receiver.type.asCQL3Type())); + throw new InvalidRequestException(String.format("Invalid user type literal for %s of type %s", receiver.name, receiver.type.asCQL3Type())); UserType ut = (UserType)receiver.type; for (int i = 0; i < ut.size(); i++) @@ -103,7 +103,7 @@ public abstract class UserTypes ColumnSpecification fieldSpec = fieldSpecOf(receiver, i); if (!value.testAssignment(keyspace, fieldSpec).isAssignable()) - throw new InvalidRequestException(String.format("Invalid user type literal for %s: field %s is not of type %s", receiver, field, fieldSpec.type.asCQL3Type())); + throw new InvalidRequestException(String.format("Invalid user type literal for %s: field %s is not of type %s", receiver.name, field, fieldSpec.type.asCQL3Type())); } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/b77e11cf/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 0d08a52..bf7eae4 100644 --- a/src/java/org/apache/cassandra/db/marshal/TupleType.java +++ b/src/java/org/apache/cassandra/db/marshal/TupleType.java @@ -39,11 +39,23 @@ public class TupleType extends AbstractType<ByteBuffer> { protected final List<AbstractType<?>> types; + private final TupleSerializer serializer; + public TupleType(List<AbstractType<?>> types) { for (int i = 0; i < types.size(); i++) types.set(i, types.get(i).freeze()); this.types = types; + this.serializer = new TupleSerializer(fieldSerializers(types)); + } + + private static List<TypeSerializer<?>> fieldSerializers(List<AbstractType<?>> types) + { + int size = types.size(); + List<TypeSerializer<?>> serializers = new ArrayList<>(size); + for (int i = 0; i < size; i++) + serializers.add(types.get(i).getSerializer()); + return serializers; } public static TupleType getInstance(TypeParser parser) throws ConfigurationException, SyntaxException @@ -119,37 +131,6 @@ public class TupleType extends AbstractType<ByteBuffer> return 1; } - @Override - public void validate(ByteBuffer bytes) throws MarshalException - { - ByteBuffer input = bytes.duplicate(); - for (int i = 0; i < size(); i++) - { - // we allow the input to have less fields than declared so as to support field addition. - if (!input.hasRemaining()) - return; - - if (input.remaining() < 4) - throw new MarshalException(String.format("Not enough bytes to read size of %dth component", i)); - - int size = input.getInt(); - - // size < 0 means null value - if (size < 0) - continue; - - if (input.remaining() < size) - throw new MarshalException(String.format("Not enough bytes to read %dth component", i)); - - ByteBuffer field = ByteBufferUtil.readBytes(input, size); - types.get(i).validate(field); - } - - // We're allowed to get less fields than declared, but not more - if (input.hasRemaining()) - throw new MarshalException("Invalid remaining data after end of tuple value"); - } - /** * Split a tuple value into its component values. */ @@ -297,7 +278,7 @@ public class TupleType extends AbstractType<ByteBuffer> public TypeSerializer<ByteBuffer> getSerializer() { - return BytesSerializer.instance; + return serializer; } @Override http://git-wip-us.apache.org/repos/asf/cassandra/blob/b77e11cf/src/java/org/apache/cassandra/db/marshal/UserType.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/marshal/UserType.java b/src/java/org/apache/cassandra/db/marshal/UserType.java index 187deeb..93059cd 100644 --- a/src/java/org/apache/cassandra/db/marshal/UserType.java +++ b/src/java/org/apache/cassandra/db/marshal/UserType.java @@ -28,6 +28,8 @@ import org.apache.cassandra.cql3.*; import org.apache.cassandra.exceptions.ConfigurationException; import org.apache.cassandra.exceptions.SyntaxException; import org.apache.cassandra.serializers.MarshalException; +import org.apache.cassandra.serializers.TypeSerializer; +import org.apache.cassandra.serializers.UserTypeSerializer; import org.apache.cassandra.utils.ByteBufferUtil; import org.apache.cassandra.utils.Pair; @@ -42,6 +44,7 @@ public class UserType extends TupleType public final ByteBuffer name; private final List<ByteBuffer> fieldNames; private final List<String> stringFieldNames; + private final UserTypeSerializer serializer; public UserType(String keyspace, ByteBuffer name, List<ByteBuffer> fieldNames, List<AbstractType<?>> fieldTypes) { @@ -51,17 +54,22 @@ public class UserType extends TupleType this.name = name; this.fieldNames = fieldNames; this.stringFieldNames = new ArrayList<>(fieldNames.size()); - for (ByteBuffer fieldName : fieldNames) + LinkedHashMap<String , TypeSerializer<?>> fieldSerializers = new LinkedHashMap<>(fieldTypes.size()); + for (int i = 0, m = fieldNames.size(); i < m; i++) { + ByteBuffer fieldName = fieldNames.get(i); try { - stringFieldNames.add(ByteBufferUtil.string(fieldName, StandardCharsets.UTF_8)); + String stringFieldName = ByteBufferUtil.string(fieldName, StandardCharsets.UTF_8); + stringFieldNames.add(stringFieldName); + fieldSerializers.put(stringFieldName, fieldTypes.get(i).getSerializer()); } catch (CharacterCodingException ex) { throw new AssertionError("Got non-UTF8 field name for user-defined type: " + ByteBufferUtil.bytesToHex(fieldName), ex); } } + this.serializer = new UserTypeSerializer(fieldSerializers); } public static UserType getInstance(TypeParser parser) throws ConfigurationException, SyntaxException @@ -109,38 +117,6 @@ public class UserType extends TupleType return UTF8Type.instance.compose(name); } - // Note: the only reason we override this is to provide nicer error message, but since that's not that much code... - @Override - public void validate(ByteBuffer bytes) throws MarshalException - { - ByteBuffer input = bytes.duplicate(); - for (int i = 0; i < size(); i++) - { - // we allow the input to have less fields than declared so as to support field addition. - if (!input.hasRemaining()) - return; - - if (input.remaining() < 4) - throw new MarshalException(String.format("Not enough bytes to read size of %dth field %s", i, fieldNameAsString(i))); - - int size = input.getInt(); - - // size < 0 means null value - if (size < 0) - continue; - - if (input.remaining() < size) - throw new MarshalException(String.format("Not enough bytes to read %dth field %s", i, fieldNameAsString(i))); - - ByteBuffer field = ByteBufferUtil.readBytes(input, size); - types.get(i).validate(field); - } - - // We're allowed to get less fields than declared, but not more - if (input.hasRemaining()) - throw new MarshalException("Invalid remaining data after end of UDT value"); - } - @Override public Term fromJSONObject(Object parsed) throws MarshalException { @@ -243,4 +219,10 @@ public class UserType extends TupleType { return getClass().getName() + TypeParser.stringifyUserTypeParameters(keyspace, name, fieldNames, types); } + + @Override + public TypeSerializer<ByteBuffer> getSerializer() + { + return serializer; + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/b77e11cf/src/java/org/apache/cassandra/serializers/TupleSerializer.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/serializers/TupleSerializer.java b/src/java/org/apache/cassandra/serializers/TupleSerializer.java new file mode 100644 index 0000000..7cf71c6 --- /dev/null +++ b/src/java/org/apache/cassandra/serializers/TupleSerializer.java @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.serializers; + +import java.nio.ByteBuffer; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.apache.cassandra.utils.ByteBufferUtil; + +public class TupleSerializer extends BytesSerializer +{ + public final List<TypeSerializer<?>> fields; + + public TupleSerializer(List<TypeSerializer<?>> fields) + { + this.fields = fields; + } + + @Override + public void validate(ByteBuffer bytes) throws MarshalException + { + ByteBuffer input = bytes.duplicate(); + for (int i = 0; i < fields.size(); i++) + { + // we allow the input to have less fields than declared so as to support field addition. + if (!input.hasRemaining()) + return; + + if (input.remaining() < 4) + throw new MarshalException(String.format("Not enough bytes to read size of %dth component", i)); + + int size = input.getInt(); + + // size < 0 means null value + if (size < 0) + continue; + + if (input.remaining() < size) + throw new MarshalException(String.format("Not enough bytes to read %dth component", i)); + + ByteBuffer field = ByteBufferUtil.readBytes(input, size); + fields.get(i).validate(field); + } + + // We're allowed to get less fields than declared, but not more + if (input.hasRemaining()) + throw new MarshalException("Invalid remaining data after end of tuple value"); + } +} http://git-wip-us.apache.org/repos/asf/cassandra/blob/b77e11cf/src/java/org/apache/cassandra/serializers/UserTypeSerializer.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/serializers/UserTypeSerializer.java b/src/java/org/apache/cassandra/serializers/UserTypeSerializer.java new file mode 100644 index 0000000..472e39b --- /dev/null +++ b/src/java/org/apache/cassandra/serializers/UserTypeSerializer.java @@ -0,0 +1,67 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.cassandra.serializers; + +import java.nio.ByteBuffer; +import java.util.LinkedHashMap; +import java.util.Map.Entry; + +import org.apache.cassandra.utils.ByteBufferUtil; + +public class UserTypeSerializer extends BytesSerializer +{ + public final LinkedHashMap<String, TypeSerializer<?>> fields; + + public UserTypeSerializer(LinkedHashMap<String, TypeSerializer<?>> fields) + { + this.fields = fields; + } + + @Override + public void validate(ByteBuffer bytes) throws MarshalException + { + ByteBuffer input = bytes.duplicate(); + int i = 0; + for (Entry<String, TypeSerializer<?>> entry : fields.entrySet()) + { + // we allow the input to have less fields than declared so as to support field addition. + if (!input.hasRemaining()) + return; + + if (input.remaining() < 4) + throw new MarshalException(String.format("Not enough bytes to read size of %dth field %s", i, entry.getKey())); + + int size = input.getInt(); + + // size < 0 means null value + if (size < 0) + continue; + + if (input.remaining() < size) + throw new MarshalException(String.format("Not enough bytes to read %dth field %s", i, entry.getKey())); + + ByteBuffer field = ByteBufferUtil.readBytes(input, size); + entry.getValue().validate(field); + i++; + } + + // We're allowed to get less fields than declared, but not more + if (input.hasRemaining()) + throw new MarshalException("Invalid remaining data after end of UDT value"); + } +} http://git-wip-us.apache.org/repos/asf/cassandra/blob/b77e11cf/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java b/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java index 99d9695..69d5a5c 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/CollectionsTest.java @@ -748,4 +748,40 @@ public class CollectionsTest extends CQLTester execute("UPDATE %s SET s = s - ? , s = s + ? WHERE pk = ?", set(3), set(3, 4), 1); assertRows(execute("SELECT * FROM %s WHERE pk = 1") , row(1, set(0, 1, 2, 4))); } + + @Test + public void testInsertingCollectionsWithInvalidElements() throws Throwable + { + createTable("CREATE TABLE %s (k int PRIMARY KEY, s frozen<set<tuple<int, text, double>>>)"); + assertInvalidMessage("Invalid remaining data after end of tuple value", + "INSERT INTO %s (k, s) VALUES (0, ?)", + set(tuple(1, "1", 1.0, 1), tuple(2, "2", 2.0, 2))); + + assertInvalidMessage("Invalid set literal for s: value (1, '1', 1.0, 1) is not of type tuple<int, text, double>", + "INSERT INTO %s (k, s) VALUES (0, {(1, '1', 1.0, 1)})"); + + createTable("CREATE TABLE %s (k int PRIMARY KEY, l frozen<list<tuple<int, text, double>>>)"); + assertInvalidMessage("Invalid remaining data after end of tuple value", + "INSERT INTO %s (k, l) VALUES (0, ?)", + list(tuple(1, "1", 1.0, 1), tuple(2, "2", 2.0, 2))); + + assertInvalidMessage("Invalid list literal for l: value (1, '1', 1.0, 1) is not of type tuple<int, text, double>", + "INSERT INTO %s (k, l) VALUES (0, [(1, '1', 1.0, 1)])"); + + createTable("CREATE TABLE %s (k int PRIMARY KEY, m frozen<map<tuple<int, text, double>, int>>)"); + assertInvalidMessage("Invalid remaining data after end of tuple value", + "INSERT INTO %s (k, m) VALUES (0, ?)", + map(tuple(1, "1", 1.0, 1), 1, tuple(2, "2", 2.0, 2), 2)); + + assertInvalidMessage("Invalid map literal for m: key (1, '1', 1.0, 1) is not of type tuple<int, text, double>", + "INSERT INTO %s (k, m) VALUES (0, {(1, '1', 1.0, 1) : 1})"); + + createTable("CREATE TABLE %s (k int PRIMARY KEY, m frozen<map<int, tuple<int, text, double>>>)"); + assertInvalidMessage("Invalid remaining data after end of tuple value", + "INSERT INTO %s (k, m) VALUES (0, ?)", + map(1, tuple(1, "1", 1.0, 1), 2, tuple(2, "2", 2.0, 2))); + + assertInvalidMessage("Invalid map literal for m: value (1, '1', 1.0, 1) is not of type tuple<int, text, double>", + "INSERT INTO %s (k, m) VALUES (0, {1 : (1, '1', 1.0, 1)})"); + } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/b77e11cf/test/unit/org/apache/cassandra/cql3/validation/entities/TupleTypeTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/cql3/validation/entities/TupleTypeTest.java b/test/unit/org/apache/cassandra/cql3/validation/entities/TupleTypeTest.java index 0783dd1..3b4fb40 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/TupleTypeTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/TupleTypeTest.java @@ -123,6 +123,14 @@ public class TupleTypeTest extends CQLTester assertInvalidMessage("Invalid tuple literal for t: too many elements. Type tuple<int, text, double> expects 3 but got 4", "INSERT INTO %s (k, t) VALUES (0, (2, 'foo', 3.1, 'bar'))"); + + createTable("CREATE TABLE %s (k int PRIMARY KEY, t frozen<tuple<int, tuple<int, text, double>>>)"); + assertInvalidMessage("Invalid remaining data after end of tuple value", + "INSERT INTO %s (k, t) VALUES (0, ?)", + tuple(1, tuple(1, "1", 1.0, 1))); + + assertInvalidMessage("Invalid tuple literal for t: component 1 is not of type tuple<int, text, double>", + "INSERT INTO %s (k, t) VALUES (0, (1, (1, '1', 1.0, 1)))"); } @Test @@ -137,7 +145,7 @@ public class TupleTypeTest extends CQLTester // select using unset assertInvalidMessage("Invalid unset value for tuple field number 0", "SELECT * FROM %s WHERE k = ? and t = (?,?,?)", unset(), unset(), unset(), unset()); } - + /** * Test the syntax introduced by #4851, * migrated from cql_tests.py:TestCQL.tuple_notation_test() http://git-wip-us.apache.org/repos/asf/cassandra/blob/b77e11cf/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 3803e5c..9bafe4a 100644 --- a/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java +++ b/test/unit/org/apache/cassandra/cql3/validation/entities/UserTypesTest.java @@ -53,6 +53,15 @@ public class UserTypesTest extends CQLTester "INSERT INTO %s (pk, t) VALUES (?, ?)", 1, "test"); assertInvalidMessage("Not enough bytes to read 0th field f", "INSERT INTO %s (pk, t) VALUES (?, ?)", 1, Long.MAX_VALUE); + + String type = createType("CREATE TYPE %s (a int, b tuple<int, text, double>)"); + createTable("CREATE TABLE %s (k int PRIMARY KEY, t frozen<" + type + ">)"); + assertInvalidMessage("Invalid remaining data after end of tuple value", + "INSERT INTO %s (k, t) VALUES (0, ?)", + userType(1, tuple(1, "1", 1.0, 1))); + + assertInvalidMessage("Invalid user type literal for t: field b is not of type tuple<int, text, double>", + "INSERT INTO %s (k, t) VALUES (0, {a: 1, b: (1, '1', 1.0, 1)})"); } @Test --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@cassandra.apache.org For additional commands, e-mail: commits-h...@cassandra.apache.org
