◐ Shell
reader mode source ↗
Skip to content

Add support for jackson field ids#868

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

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

Conversation

@brenbar

@brenbar brenbar commented Jan 18, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

brenbar commented Jan 18, 2025

Copy link
Copy Markdown
Contributor Author

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

@brenbar

brenbar commented Jan 19, 2025

Copy link
Copy Markdown
Contributor Author

@komamitsu @xerial Another CI run please 🙏

@brenbar

brenbar commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

🔨 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

brenbar commented Jan 20, 2025

Copy link
Copy Markdown
Contributor Author

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

@brenbar brenbar marked this pull request as ready for review February 3, 2025 06:27
@brenbar

brenbar commented Feb 3, 2025

Copy link
Copy Markdown
Contributor Author

@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

brenbar commented Feb 9, 2025

Copy link
Copy Markdown
Contributor Author

@komamitsu Thanks for the review. All comments addressed.

@komamitsu

komamitsu commented Feb 9, 2025

Copy link
Copy Markdown
Member

@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

brenbar commented Feb 10, 2025

Copy link
Copy Markdown
Contributor Author

@komamitsu I think your feedback is fair 😅

Ready again for review 👍

komamitsu
komamitsu approved these changes Feb 11, 2025
View reviewed changes

@komamitsu komamitsu left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM, thank you!

@komamitsu komamitsu merged commit 1cbd05f into msgpack:main Feb 11, 2025
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