adutra commented on a change in pull request #924:
URL: https://github.com/apache/cassandra/pull/924#discussion_r593345250
##########
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
I guess the real question is: do we consider an invalid length as a
violation of the class invariants (in which case `AssertionError` is the
appropriate error), or is it an exceptional state that we need to deal with (in
which case we would go with `IOException`)?
I did a small research in the `marshal` package:
`AssertionError` or `assert` is used mostly to detect violations of:
* Empty / non-empty type expectations
* ASCII / UTF-8 encoding expectations
* Single / Multi cell expectations
* UUID length expectations
`IOException` is in fact very rare. It is declared to be thrown in various
methods, but it is only actually thrown in
`AbtractType.read()`. Otherwise, most IOEs come from the methods in
`ValueAccessor`.
There is also `MarshalException`. It is being thrown for validation errors
and string parsing errors mostly.
With the above in mind, and considering my limited knowledge of Cassandra
internals, I would say that invalid lengths could very well happen IRL, and
therefore I wouldn't consider them as invariants to be expressed as assertions.
IOW `IOException` doesn't look like an awkward choice in the present case.
As an example of when and how this could happen, maybe you recall [this
test](https://github.com/adutra/cassandra/blob/576cb2b8a4267467b507cb88e841462314d2aaee/test/unit/org/apache/cassandra/index/sasi/SASIIndexTest.java#L1186)?
It manages to violate the buffer length expectation in question. Granted, to
get there, it needs to fake a pretty serious condition: one mutation is created
with the wrong type for one of the cells. I don't know if the system can be put
in such an inconsistent state by user intervention, but it does seem possible,
at least, to violate the expectation with just a few lines of code.
> maybe we could also use it in the first check for empty values
I am not sure. Buffer emptiness, on the other hand, does look like a
breakage in the class invariants. We have `EmptyType` that deals with empty
buffers, so using any other instance of `AbstractType` for that seems to me
like an impossible situation. But again, I might be misinterpreting all this.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
