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

Add support for jackson field ids #868

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Conversation

brenbar
Copy link

@brenbar brenbar commented Jan 18, 2025

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
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
Author

brenbar commented Jan 19, 2025

@komamitsu @xerial Another CI run please 🙏

@brenbar
Copy link
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
Author

brenbar commented Jan 20, 2025

@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);
Copy link
Author

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);
}

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

Successfully merging this pull request may close these issues.

1 participant