search

Jroland / kafka-net

Native C# client for Kafka queue servers.
Apache License 2.0
483 stars 232 forks source link

empty or null strings are not packed correctly #39

Closed xtofs closed 9 years ago

xtofs commented 9 years ago

Line 107 in BigEndianWriter https://github.com/Jroland/kafka-net/blob/master/src/kafka-net/Common/BigEndianBinaryWriter.cs#L107 writes four bytes for length for empty or null strings even when encoding is StringPrefixEncoding.Int16

Discovered while experimenting with Offset management messages (e.g. OffsetCommitRequest) that are more likely to have empty string than Fetch/Produce

Jroland commented 9 years ago

Nice thanks xtofs. If you want to make a failing unit test for this case and send in a pull request I can pull it in and github will show you as contributing to the project. EIther way thanks for the bug.

xtofs commented 9 years ago

Hi James, it will take a little while for me to set up a fork. But your commit is close to what I would have done anyway.

Just a minor detail. Technically the empty string should be packed differently from null If I get to it I would want to add the test case line 163 

  [TestCase("", new Byte[] { 0x00, 0x00 }, StringPrefixEncoding.Int16)]
Jroland commented 9 years ago

Hey xtofs, sorry about that, I figured your bug was significant enough that I wanted to jump on it right away. Thanks for reviewing it. If you want to post that change above, ill wait for that if you want.

xtofs commented 9 years ago

Hi James, no worries, fully understand that you wanted to fix it. Finally got the pull-request out. There is now a thing with the BigEndianBinaryReader. Looking at it the method ReadInt16String does the right thing and all the code seems to use that method. But the test goes against the virtual method ReadString which doesn't deal with the length -1.Happy to look into that that. Christof

Date: Tue, 20 Jan 2015 09:32:44 -0800 From: notifications@github.com To: kafka-net@noreply.github.com CC: christof_sprenger@hotmail.com Subject: Re: [kafka-net] empty or null strings are not packed correctly (#39)

Hey xtofs, sorry about that, I figured your bug was significant enough that I wanted to jump on it right away. Thanks for reviewing it. If you want to post that change above, ill wait for that if you want.

— Reply to this email directly or view it on GitHub.

                  =