cassandra-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From s...@apache.org
Subject [3/6] cassandra git commit: Fix validation of non-frozen UDT cells
Date Tue, 22 Nov 2016 17:03:12 GMT
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/trunk
Commit: 9cc0c80de281e6c6f09187dd318deaa5b68cde82
Parents: 075539a
Author: Sam Tunnicliffe <sam@beobal.com>
Authored: Tue Nov 22 16:55:08 2016 +0000
Committer: Sam Tunnicliffe <sam@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()
     {


Mime
View raw message