[GitHub] [cassandra] adelapena commented on a change in pull request #924: Throw IOE in AbstractType.writeValue if value has wrong fixed length (CASSANDRA-16500)

[GitBox <gi...@apache.org>]

adelapena commented on a change in pull request #924:
URL: https://github.com/apache/cassandra/pull/924#discussion_r592348731



##########
File path: src/java/org/apache/cassandra/db/marshal/AbstractType.java
##########
@@ -435,11 +435,21 @@ public void writeValue(ByteBuffer value, DataOutputPlus out) throws IOException
     // This assumes that no empty values are passed
     public  <V> void writeValue(V value, ValueAccessor<V> accessor, DataOutputPlus out) throws IOException
     {
-        assert !accessor.isEmpty(value);
-        if (valueLengthIfFixed() >= 0)
-            accessor.write(value, out);
+        assert !accessor.isEmpty(value) : "bytes should not be empty for type " + this;
+        int expectedValueLength = valueLengthIfFixed();
+        if (expectedValueLength >= 0)
+        {
+            int actualValueLength = accessor.size(value);
+            if (actualValueLength == expectedValueLength)
+                accessor.write(value, out);
+             else
+                 throw new IOException(String.format("Expected exactly %d bytes, but was %d",

Review comment:
       Not sure about whether `AssertionError` or `IOException` makes more sense, since we seem to be mixing both types of exceptions across the `marshal` package for similar cases, probably being `AssertionError` more common. If we decide to use `IOException` maybe we could also use it in the first check for empty values:
   ```java
   int actualValueLength = accessor.size(value);
   if (actualValueLength == 0)
       throw new IOException("bytes should not be empty for type " + this);
   ```
   And perhaps we could also use `IOException` in `EmptyValue#writeValue`, which overrides this method, wdyt?

##########
File path: test/unit/org/apache/cassandra/db/marshal/TypeValidationTest.java
##########
@@ -19,16 +19,19 @@
 package org.apache.cassandra.db.marshal;
 
 import org.apache.cassandra.Util;
+import org.apache.cassandra.io.util.DataOutputPlus;

Review comment:
       Not related to the changes, but we could fix the license header to be a block comment instead of JavaDoc, replacing `/**` by `/*` in the very first line of the file.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org