Fix validation of non-frozen UDT cells Patch by Sergey Dobrodey; reviewed by Sam Tunnicliffe and Tyler Hobbs for CASSANDRA-12916
Project: http://git-wip-us.apache.org/repos/asf/cassandra/repo Commit: http://git-wip-us.apache.org/repos/asf/cassandra/commit/9cc0c80d Tree: http://git-wip-us.apache.org/repos/asf/cassandra/tree/9cc0c80d Diff: http://git-wip-us.apache.org/repos/asf/cassandra/diff/9cc0c80d Branch: refs/heads/cassandra-3.X Commit: 9cc0c80de281e6c6f09187dd318deaa5b68cde82 Parents: 075539a Author: Sam Tunnicliffe <s...@beobal.com> Authored: Tue Nov 22 16:55:08 2016 +0000 Committer: Sam Tunnicliffe <s...@beobal.com> Committed: Tue Nov 22 16:58:02 2016 +0000 ---------------------------------------------------------------------- CHANGES.txt | 1 + .../cassandra/config/ColumnDefinition.java | 24 ++++- .../apache/cassandra/db/marshal/UserType.java | 15 +++ .../apache/cassandra/db/rows/AbstractCell.java | 18 +--- .../apache/cassandra/db/rows/AbstractRow.java | 34 +++++-- .../cassandra/thrift/ThriftValidation.java | 3 +- test/unit/org/apache/cassandra/db/CellTest.java | 100 ++++++++++++++++++- 7 files changed, 165 insertions(+), 30 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/cassandra/blob/9cc0c80d/CHANGES.txt ---------------------------------------------------------------------- diff --git a/CHANGES.txt b/CHANGES.txt index 24641a6..92bf1ce 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,4 +1,5 @@ 3.11 + * Fix validation of non-frozen UDT cells (CASSANDRA-12916) * AnticompactionRequestSerializer serializedSize is incorrect (CASSANDRA-12934) 3.10 http://git-wip-us.apache.org/repos/asf/cassandra/blob/9cc0c80d/src/java/org/apache/cassandra/config/ColumnDefinition.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/config/ColumnDefinition.java b/src/java/org/apache/cassandra/config/ColumnDefinition.java index 6044ee9..61739a3 100644 --- a/src/java/org/apache/cassandra/config/ColumnDefinition.java +++ b/src/java/org/apache/cassandra/config/ColumnDefinition.java @@ -384,12 +384,30 @@ public class ColumnDefinition extends ColumnSpecification implements Selectable, return CollectionType.cellPathSerializer; } - public void validateCellValue(ByteBuffer value) + public void validateCell(Cell cell) { - type.validateCellValue(value); + if (cell.isTombstone()) + { + if (cell.value().hasRemaining()) + throw new MarshalException("A tombstone should not have a value"); + if (cell.path() != null) + validateCellPath(cell.path()); + } + else if(type.isUDT()) + { + // To validate a non-frozen UDT field, both the path and the value + // are needed, the path being an index into an array of value types. + ((UserType)type).validateCell(cell); + } + else + { + type.validateCellValue(cell.value()); + if (cell.path() != null) + validateCellPath(cell.path()); + } } - public void validateCellPath(CellPath path) + private void validateCellPath(CellPath path) { if (!isComplex()) throw new MarshalException("Only complex cells should have a cell path"); http://git-wip-us.apache.org/repos/asf/cassandra/blob/9cc0c80d/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 176ab84..475c01d 100644 --- a/src/java/org/apache/cassandra/db/marshal/UserType.java +++ b/src/java/org/apache/cassandra/db/marshal/UserType.java @@ -169,6 +169,21 @@ public class UserType extends TupleType return TupleType.buildValue(components); } + public void validateCell(Cell cell) throws MarshalException + { + if (isMultiCell) + { + ByteBuffer path = cell.path().get(0); + nameComparator().validate(path); + Short fieldPosition = nameComparator().getSerializer().deserialize(path); + fieldType(fieldPosition).validate(cell.value()); + } + else + { + validate(cell.value()); + } + } + // 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 http://git-wip-us.apache.org/repos/asf/cassandra/blob/9cc0c80d/src/java/org/apache/cassandra/db/rows/AbstractCell.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/rows/AbstractCell.java b/src/java/org/apache/cassandra/db/rows/AbstractCell.java index 002abe6..ca83783 100644 --- a/src/java/org/apache/cassandra/db/rows/AbstractCell.java +++ b/src/java/org/apache/cassandra/db/rows/AbstractCell.java @@ -138,19 +138,11 @@ public abstract class AbstractCell extends Cell if (isExpiring() && localDeletionTime() == NO_DELETION_TIME) throw new MarshalException("Shoud not have a TTL without an associated local deletion time"); - if (isTombstone()) - { - // If cell is a tombstone, it shouldn't have a value. - if (value().hasRemaining()) - throw new MarshalException("A tombstone should not have a value"); - } - else - { - column().validateCellValue(value()); - } - - if (path() != null) - column().validateCellPath(path()); + // non-frozen UDTs require both the cell path & value to validate, + // so that logic is pushed down into ColumnDefinition. Tombstone + // validation is done there too as it also involves the cell path + // for complex columns + column().validateCell(this); } public long maxTimestamp() http://git-wip-us.apache.org/repos/asf/cassandra/blob/9cc0c80d/src/java/org/apache/cassandra/db/rows/AbstractRow.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/db/rows/AbstractRow.java b/src/java/org/apache/cassandra/db/rows/AbstractRow.java index 59addeb..f87b7c2 100644 --- a/src/java/org/apache/cassandra/db/rows/AbstractRow.java +++ b/src/java/org/apache/cassandra/db/rows/AbstractRow.java @@ -20,12 +20,16 @@ import java.nio.ByteBuffer; import java.security.MessageDigest; import java.util.AbstractCollection; import java.util.Objects; +import java.util.function.Function; +import java.util.stream.Collectors; +import java.util.stream.StreamSupport; import com.google.common.collect.Iterables; import org.apache.cassandra.config.CFMetaData; import org.apache.cassandra.db.*; import org.apache.cassandra.db.marshal.CollectionType; +import org.apache.cassandra.db.marshal.UserType; import org.apache.cassandra.serializers.MarshalException; import org.apache.cassandra.utils.FBUtilities; @@ -144,16 +148,30 @@ public abstract class AbstractRow extends AbstractCollection<ColumnData> impleme } else { - ComplexColumnData complexData = (ComplexColumnData)cd; - CollectionType ct = (CollectionType)cd.column().type; - sb.append(cd.column().name).append("={"); - int i = 0; - for (Cell cell : complexData) + sb.append(cd.column().name).append('='); + ComplexColumnData complexData = (ComplexColumnData) cd; + Function<Cell, String> transform = null; + if (cd.column().type.isCollection()) + { + CollectionType ct = (CollectionType) cd.column().type; + transform = cell -> String.format("%s -> %s", + ct.nameComparator().getString(cell.path().get(0)), + ct.valueComparator().getString(cell.value())); + + } + else if (cd.column().type.isUDT()) { - sb.append(i++ == 0 ? "" : ", "); - sb.append(ct.nameComparator().getString(cell.path().get(0))).append("->").append(ct.valueComparator().getString(cell.value())); + UserType ut = (UserType)cd.column().type; + transform = cell -> { + Short fId = ut.nameComparator().getSerializer().deserialize(cell.path().get(0)); + return String.format("%s -> %s", + ut.fieldNameAsString(fId), + ut.fieldType(fId).getString(cell.value())); + }; } - sb.append('}'); + sb.append(StreamSupport.stream(complexData.spliterator(), false) + .map(transform != null ? transform : cell -> "") + .collect(Collectors.joining(", ", "{", "}"))); } } } http://git-wip-us.apache.org/repos/asf/cassandra/blob/9cc0c80d/src/java/org/apache/cassandra/thrift/ThriftValidation.java ---------------------------------------------------------------------- diff --git a/src/java/org/apache/cassandra/thrift/ThriftValidation.java b/src/java/org/apache/cassandra/thrift/ThriftValidation.java index 56c4865..36a9755 100644 --- a/src/java/org/apache/cassandra/thrift/ThriftValidation.java +++ b/src/java/org/apache/cassandra/thrift/ThriftValidation.java @@ -454,8 +454,7 @@ public class ThriftValidation try { LegacyLayout.LegacyCellName cn = LegacyLayout.decodeCellName(metadata, scName, column.name); - cn.column.validateCellValue(column.value); - + cn.column.type.validateCellValue(column.value); } catch (UnknownColumnException e) { http://git-wip-us.apache.org/repos/asf/cassandra/blob/9cc0c80d/test/unit/org/apache/cassandra/db/CellTest.java ---------------------------------------------------------------------- diff --git a/test/unit/org/apache/cassandra/db/CellTest.java b/test/unit/org/apache/cassandra/db/CellTest.java index 8fb8adb..86fb20e 100644 --- a/test/unit/org/apache/cassandra/db/CellTest.java +++ b/test/unit/org/apache/cassandra/db/CellTest.java @@ -31,6 +31,7 @@ import org.apache.cassandra.config.CFMetaData; import org.apache.cassandra.config.ColumnDefinition; import org.apache.cassandra.config.DatabaseDescriptor; import org.apache.cassandra.cql3.ColumnIdentifier; +import org.apache.cassandra.cql3.FieldIdentifier; import org.apache.cassandra.db.marshal.*; import org.apache.cassandra.db.rows.*; import org.apache.cassandra.exceptions.ConfigurationException; @@ -40,6 +41,8 @@ import org.apache.cassandra.serializers.MarshalException; import org.apache.cassandra.utils.ByteBufferUtil; import org.apache.cassandra.utils.FBUtilities; +import static java.util.Arrays.*; + public class CellTest { static @@ -145,13 +148,13 @@ public class CellTest // But this should be valid even though the underlying value is an empty BB (catches bug #11618) assertValid(BufferCell.tombstone(c, 0, 4)); // And of course, this should be valid with a proper value - assertValid(BufferCell.live(c, 0, ByteBufferUtil.bytes((short)4))); + assertValid(BufferCell.live(c, 0, bbs(4))); // Invalid ttl - assertInvalid(BufferCell.expiring(c, 0, -4, 4, ByteBufferUtil.bytes(4))); + assertInvalid(BufferCell.expiring(c, 0, -4, 4, bbs(4))); // Invalid local deletion times - assertInvalid(BufferCell.expiring(c, 0, 4, -4, ByteBufferUtil.bytes(4))); - assertInvalid(BufferCell.expiring(c, 0, 4, Cell.NO_DELETION_TIME, ByteBufferUtil.bytes(4))); + assertInvalid(BufferCell.expiring(c, 0, 4, -5, bbs(4))); + assertInvalid(BufferCell.expiring(c, 0, 4, Cell.NO_DELETION_TIME, bbs(4))); c = fakeColumn("c", MapType.getInstance(Int32Type.instance, Int32Type.instance, true)); // Valid cell path @@ -161,6 +164,75 @@ public class CellTest } @Test + public void testValidateNonFrozenUDT() + { + FieldIdentifier f1 = field("f1"); // has field position 0 + FieldIdentifier f2 = field("f2"); // has field position 1 + UserType udt = new UserType("ks", + bb("myType"), + asList(f1, f2), + asList(Int32Type.instance, UTF8Type.instance), + true); + ColumnDefinition c; + + // Valid cells + c = fakeColumn("c", udt); + assertValid(BufferCell.live(c, 0, bb(1), CellPath.create(bbs(0)))); + assertValid(BufferCell.live(c, 0, bb("foo"), CellPath.create(bbs(1)))); + assertValid(BufferCell.expiring(c, 0, 4, 4, bb(1), CellPath.create(bbs(0)))); + assertValid(BufferCell.expiring(c, 0, 4, 4, bb("foo"), CellPath.create(bbs(1)))); + assertValid(BufferCell.tombstone(c, 0, 4, CellPath.create(bbs(0)))); + + // Invalid value (text in an int field) + assertInvalid(BufferCell.live(c, 0, bb("foo"), CellPath.create(bbs(0)))); + + // Invalid ttl + assertInvalid(BufferCell.expiring(c, 0, -4, 4, bb(1), CellPath.create(bbs(0)))); + // Invalid local deletion times + assertInvalid(BufferCell.expiring(c, 0, 4, -5, bb(1), CellPath.create(bbs(0)))); + assertInvalid(BufferCell.expiring(c, 0, 4, Cell.NO_DELETION_TIME, bb(1), CellPath.create(bbs(0)))); + + // Invalid cell path (int values should be 0 or 2 bytes) + assertInvalid(BufferCell.live(c, 0, bb(1), CellPath.create(ByteBufferUtil.bytes((long)4)))); + } + + @Test + public void testValidateFrozenUDT() + { + FieldIdentifier f1 = field("f1"); // has field position 0 + FieldIdentifier f2 = field("f2"); // has field position 1 + UserType udt = new UserType("ks", + bb("myType"), + asList(f1, f2), + asList(Int32Type.instance, UTF8Type.instance), + false); + + ColumnDefinition c = fakeColumn("c", udt); + ByteBuffer val = udt(bb(1), bb("foo")); + + // Valid cells + assertValid(BufferCell.live(c, 0, val)); + assertValid(BufferCell.live(c, 0, val)); + assertValid(BufferCell.expiring(c, 0, 4, 4, val)); + assertValid(BufferCell.expiring(c, 0, 4, 4, val)); + assertValid(BufferCell.tombstone(c, 0, 4)); + // fewer values than types is accepted + assertValid(BufferCell.live(c, 0, udt(bb(1)))); + + // Invalid values + // invalid types + assertInvalid(BufferCell.live(c, 0, udt(bb("foo"), bb(1)))); + // too many types + assertInvalid(BufferCell.live(c, 0, udt(bb(1), bb("foo"), bb("bar")))); + + // Invalid ttl + assertInvalid(BufferCell.expiring(c, 0, -4, 4, val)); + // Invalid local deletion times + assertInvalid(BufferCell.expiring(c, 0, 4, -5, val)); + assertInvalid(BufferCell.expiring(c, 0, 4, Cell.NO_DELETION_TIME, val)); + } + + @Test public void testExpiringCellReconile() { // equal @@ -183,6 +255,26 @@ public class CellTest return ByteBufferUtil.bytes(i); } + private static ByteBuffer bbs(int s) + { + return ByteBufferUtil.bytes((short) s); + } + + private static ByteBuffer bb(String str) + { + return UTF8Type.instance.decompose(str); + } + + private static ByteBuffer udt(ByteBuffer...buffers) + { + return UserType.buildValue(buffers); + } + + private static FieldIdentifier field(String field) + { + return FieldIdentifier.forQuoted(field); + } + @Test public void testComplexCellReconcile() {