Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[protobuf] Some fields are left null #54

Closed
niksu7 opened this issue Mar 6, 2017 · 10 comments
Closed

[protobuf] Some fields are left null #54

niksu7 opened this issue Mar 6, 2017 · 10 comments
Labels
Milestone

Comments

@niksu7
Copy link

niksu7 commented Mar 6, 2017

Depending on field names and ordering, some fields are at times left out, probably when reading in proto message (since this can be reproduced also by writing the same content with c++ compiled proto interface and reading in with Jackson).

Attached JUnit test case with two ways to work around the problem (JsonPropertyOrder annotation or leaving out a nested object value).

Tested with version 2.8.7 (and same problems in previous versions too, but I haven't run this exact test with any other version).
TestProto.java.txt

@niksu7
Copy link
Author

niksu7 commented Mar 7, 2017

Correction: the problem must be in writing part, since the message is only 11 bytes when the nested object (testClass.b.d) is set, but 23 bytes when it is not set.

@niksu7
Copy link
Author

niksu7 commented Mar 7, 2017

I have a correction to this now, with minor changes to ProtobufGenerator.java and ByteAccumulator.java.

niksu7 added a commit to niksu7/jackson-dataformats-binary that referenced this issue Mar 7, 2017
@cowtowncoder cowtowncoder changed the title protobuf: some fields are left null [protobuf] Some fields are left null Mar 9, 2017
@cowtowncoder
Copy link
Member

cowtowncoder commented Mar 9, 2017

Excellent detective work: thank you for the patch & test! I will backport this in 2.8.

@cowtowncoder
Copy link
Member

As per notes on PR itself, it would appear that fix does not fix test failure (although naming of test class prevents it from getting run via Maven, hiding the fail).

@niksu7
Copy link
Author

niksu7 commented Mar 9, 2017

Oops, I'll check that today. I ran the test manually from Eclipse while testing the fix. It might be that I didn't test the latter commit manually as I assumed Maven would run the test...

@niksu7
Copy link
Author

niksu7 commented Mar 9, 2017

Yes, there was a stupid bug in the last commit.
Also renamed test + refactored it a bit.

@cowtowncoder cowtowncoder added this to the 2.8.8 milestone Mar 10, 2017
@cowtowncoder
Copy link
Member

Perfect, thank you! I merged it manually for 2.8 so will close PR but changes are the same.
Thank you once again for fixing this. Will be in 2.8.8 and 2.9.0.pr2

@cowtowncoder
Copy link
Member

Hmmh. I suspect this is related to (or causing) #67, fwtw. Reordering of buffered content, perhaps harmless in some cases.

@niksu7
Copy link
Author

niksu7 commented Apr 5, 2017 via email

@cowtowncoder
Copy link
Member

cowtowncoder commented Apr 5, 2017

@niksu7 Ah sorry, was meant just as FYI -- I was actually able to figure out what is/was happening with #67 and fix it.

And in all likelihood it's more that the fix here was just in related are, not causing it. Just not handling bit different case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants