Add support for jackson field ids - #868
Conversation
Sorry, something went wrong.
|
@komamitsu @xerial Can CI be run on this PR to show the failing test? Then I'll follow-up with the implementation changes. |
Sorry, something went wrong.
|
@komamitsu @xerial Another CI run please 🙏 |
Sorry, something went wrong.
|
🔨 Build failed with test showcasing new edge case. [info] Test org.msgpack.jackson.dataformat.MessagePackDataformatForFieldIdTest.testMixedKeys started
[error] Test org.msgpack.jackson.dataformat.MessagePackDataformatForFieldIdTest.testMixedKeys failed: java.lang.AssertionError: expected: java.util.HashMap<{1=one, 2=two}> but was: java.util.HashMap<{1=one, 2=two}>, took 0.258 sec
[error] at org.msgpack.jackson.dataformat.MessagePackDataformatForFieldIdTest.testMixedKeys(MessagePackDataformatForFieldIdTest.java:105)
[error] ...
[info] Test run org.msgpack.jackson.dataformat.MessagePackDataformatForFieldIdTest finished: 1 failed, 0 ignored, 1 total, 0.264s
Will now demonstrate passing test with implementation changes. |
Sorry, something went wrong.
|
@komamitsu @xerial This is ready for another CI run to showcase the new test passing. |
Sorry, something went wrong.
| @Override | ||
| public void writeFieldId(long id) throws IOException | ||
| { | ||
| addKeyNode(id); |
There was a problem hiding this comment.
@komamitsu @xerial For backwards compatibility, I imagine we need some sort of feature flag that defaults to the prior implementation. Do you guys have any suggestions for how that flag should be defined? Should I just use the MessagePack.PackerConfig, or should I pass it directly from the MessagePackFactory to the MessagePackGenerator?
| addKeyNode(id); | |
| if (writeIntegerMapKeysAsStringKeys) { | |
| super.writeFieldId(id); | |
| } else { | |
| addKeyNode(id); | |
| } |
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, for the delayed reply. PackerConfig is for the core configuration and shouldn't be used for jackson-dataformat-msgpack. I think it's okay to pass the flag like reuseResource.
Sorry, something went wrong.
|
@komamitsu @xerial This is ready for a final draft review. I added a test to show backwards compatibility with new feature flag. ./sbt jcheckStyle and ./sbt test both pass locally for me. |
Sorry, something went wrong.
|
@komamitsu Thanks for the review. All comments addressed. |
Sorry, something went wrong.
|
@brenbar Thanks! LGTM 👍 After reviewing all the changes, writeIntegerKeysAsStringKeys feels a bit verbose? supportIntegerKeys (false by default) or something might be simpler. What do you think? |
Sorry, something went wrong.
|
@komamitsu I think your feedback is fair 😅 Ready again for review 👍 |
Sorry, something went wrong.
komamitsu
left a comment
There was a problem hiding this comment.
LGTM, thank you!
Sorry, something went wrong.
Background
Jackson core has interfaces for field ids. This is a great opportunity for msgpack, since the protocol allows for integer keys, enabling more advanced binary serialization strategies for further reduced message size.
Current implementation of msgpack-jackson only allows for coercing strings to integers. Implementing the formal interfaces will enable end-to-end map serialization with mixed string/integer keys. Other msgpack implementations already support this.
Summary of Changes