IBM
fix(decoder): handle null arrays correctly
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.
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 The configuration keys to list, or null to list all configuration keys. for the configuration keys array. And that's how clients behave, they send -1, aka nil, when no config keys are specified. Tried this behavior with Sarama and Kafka cli. So I believe the decoder should return nil when the array size is -1
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. If you believe the changes are still valid then please verify your branch has no conflicts with main and rebase if needed. If you are awaiting a (re-)review then please let us know.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
🤔 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.