Add support for jackson field ids by brenbar · Pull Request #868

Add support for jackson field ids by brenbar · Pull Request #868 · msgpack/msgpack-java

Conversation

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

Implement the formal jackson core interface for writing integer keys as field ids instead of strings.
Add a helper function on the msgpack parser that can be used in a jackson KeyDeserializer to deserialize field ids.

@komamitsu @xerial Can CI be run on this PR to show the failing test? Then I'll follow-up with the implementation changes.

🔨 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.

@komamitsu @xerial This is ready for another CI run to showcase the new test passing.

		@Override
		public void writeFieldId(long id) throws IOException
		{
		addKeyNode(id);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

brenbar marked this pull request as ready for review

February 3, 2025 06:27

@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.

@komamitsu Thanks for the review. All comments addressed.

@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?

Conversation

Background

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

2 participants