fix(decoder): handle null arrays correctly #3144
Conversation
There was a problem hiding this comment.
ARRAY: Represents a sequence of objects of a given type T. Type T can be either a primitive type (e.g. STRING) or a structure. First, the length N is given as an INT32. Then N instances of type T follow. A null array is represented with a length of -1. In protocol documentation an array of T instances is referred to as [T].
—https://kafka.apache.org/protocol.html
Checks out. The cast to int32 first is necessary to get signedness before then casting to a maybe-larger int.
I’m not sure that we’re still handling -1 length NULL arrays properly though. Should we be returning errInvalidArrayLength or should we be returning a []T(nil)?
|
Yeah the latter I think. @vladoatanasov spotted the original problem whilst using the decoder code elsewhere. I was mildly surprised we'd never hit any problems due to this, but I can only guess it's an uncommon response for the client to decode and we obviously don't have any unittest coverage of this case |
|
Hey @puellanivis, I stumbled upon this issue while implementing the DescribeConfigs API here. The spec states |
|
Thank you for your contribution! However, this pull request has not had any activity in the past 90 days and will be closed in 30 days if no updates occur. |
|
🤔 This probably shouldn’t still be in draft mode? The change is sound. (Even though it’s a little weird that it has a double cast, it’s correct.) |
|
@puellanivis yeah I think I intended to come back and add a unittest before getting a review and merging, but obviously forgot about it |
|
@dnwe it's probably worth it adding a code comment as to why a double cast is needed. I had already forgotten about this use-case. Someone looking at this code in the future might be wondering what's going on. |
|
🤔 I think a unit test might be a bit hard, because it would never fail in a 32-bit environment. But then, I think we get plenty of 64-bit environment testing done. I have +1’ed already the idea of adding a comment about why a double cast is necessary. I remembered pretty fast why it’s there, but it is kind of an astonishing thing to need to do. |
Array lengths in the Kafka protocol are encoded as a 32-bit signed integer in big-endian format. For a null value this corresponds to the byte sequence FF FF FF FF which is intended to represent null as a length of -1. To correctly read that we need to cast the value to int32 before widening to int. Signed-off-by: Dominic Evans <[email protected]>
Array lengths in the Kafka protocol are encoded as a 32-bit signed integer in big-endian format. For a null value this corresponds to the byte sequence FF FF FF FF which is intended to represent null as a length of -1. To correctly read that we need to cast the value to int32 before widening to int.