Skip to content

Add support for jackson field ids#868

Merged
komamitsu merged 10 commits intomsgpack:mainfrom
brenbar:field-ids
Feb 11, 2025
Merged

Add support for jackson field ids#868
komamitsu merged 10 commits intomsgpack:mainfrom
brenbar:field-ids

Conversation

@brenbar
Copy link
Contributor

@brenbar brenbar commented Jan 18, 2025 •
edited
Loading

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

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

@brenbar
Copy link
Contributor Author

brenbar commented Jan 18, 2025

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

@brenbar
Copy link
Contributor Author

brenbar commented Jan 19, 2025

@komamitsu @xerial Another CI run please 🙏

@brenbar
Copy link
Contributor Author

brenbar commented Jan 20, 2025

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

@brenbar
Copy link
Contributor Author

brenbar commented Jan 20, 2025

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

brenbar
brenbar commented Jan 20, 2025
View reviewed changes
msgpack-jackson/src/main/java/org/msgpack/jackson/dataformat/MessagePackGenerator.java Outdated
@Override
public void writeFieldId(long id) throws IOException
{
addKeyNode(id);
Copy link
Contributor Author

@brenbar brenbar Jan 20, 2025

Choose a reason for hiding this comment

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

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

Suggested change
addKeyNode(id);
if (writeIntegerMapKeysAsStringKeys) {
super.writeFieldId(id);
} else {
addKeyNode(id);
}

Copy link
Member

@komamitsu komamitsu Feb 2, 2025

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 brenbar marked this pull request as ready for review February 3, 2025 06:27
@brenbar
Copy link
Contributor Author

brenbar commented Feb 3, 2025

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

@brenbar
Copy link
Contributor Author

brenbar commented Feb 9, 2025

@komamitsu Thanks for the review. All comments addressed.

@komamitsu
Copy link
Member

komamitsu commented Feb 9, 2025

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

@brenbar
Copy link
Contributor Author

brenbar commented Feb 10, 2025

@komamitsu I think your feedback is fair 😅

Ready again for review 👍

komamitsu
komamitsu approved these changes Feb 11, 2025
View reviewed changes
Copy link
Member

@komamitsu komamitsu left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@komamitsu komamitsu merged commit 1cbd05f into msgpack:main Feb 11, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@komamitsu komamitsu komamitsu approved these changes

Assignees

@brenbar brenbar

Labels

feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@brenbar @komamitsu