You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by sa...@apache.org on 2016/11/22 17:03:11 UTC
[2/6] cassandra git commit: Fix validation of non-frozen UDT cells
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 <sa...@beobal.com>
Authored: Tue Nov 22 16:55:08 2016 +0000
Committer: Sam Tunnicliffe <sa...@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()
{