feat: Add support for MsgPack #118
Conversation
hotrungnhan
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @hotrungnhan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces full support for MsgPack, a binary serialization format, into the project. This includes adding the necessary external library, registering its MIME type for HTTP handling, defining clear utility types for marshaling and unmarshaling, and providing robust unit tests to validate the implementation.
Highlights
- MsgPack Dependency: I've added the
github.com/shamaton/msgpack/v2library as a new dependency ingo.modto enable MsgPack serialization and deserialization. - MIME Type Registration: The
http.gofile has been updated to includeapplication/msgpackin themimeExtensionsmap, ensuring proper content type handling for MsgPack data. - MsgPack Utility Types: A new file,
msgpack.go, introducesMsgPackMarshalandMsgPackUnmarshalfunction types. These types provide clear interfaces for encoding and decoding data using MsgPack. - Unit Tests for MsgPack: Comprehensive unit tests have been added in
msgpack_test.goto verify the correct functionality of both MsgPack encoding and decoding processes, ensuring the new integration works as expected.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces support for MessagePack (MsgPack) by adding the github.com/shamaton/msgpack/v2 dependency, defining MsgPackMarshal and MsgPackUnmarshal function types in a new msgpack.go file, and registering the application/msgpack MIME type in http.go. The changes are generally well-implemented and clear.
One critical issue was identified in msgpack_test.go: a mismatch between the defined struct name (sampleStructure2) and the struct name used in the test functions (sampleStructure) will lead to a compilation error. This needs to be corrected for the tests to pass and validate the MsgPack integration.
|
We already support msgp via https://github.com/gofiber/fiber/blob/3b2af61152d0dc9bb75bd9451e506274266b638e/go.mod#L12 using the most used library for golang. The one added on this PR is barely used by projects |
ah okay, i'll closed this PR. |
|
We do need this, but lets us discuss internally which library to use. |
My goal is to improve the performance of JSON serialization in our microservices. When comparing Protobuf and Msgpack, I found that Msgpack is noticeably faster than JSON, though still slower than Protobuf. However, that trade-off is acceptable — Msgpack offers a good balance of speed and convenience, as it can serve as a drop-in replacement with minimal changes required. I chose shamaton/msgpack because its API is quite similar to the standard JSON API, making it easier to adopt. Its performance is also solid, although i currently lack of omit empty, LOL. I’ve benchmarked several libraries, as shown below:
I didn’t have time to include tinylib/msgp in my own benchmarks, but Serializer Benchmark shows that it also performs well. While tinylib/msgp is around 3x faster than shamaton/msgpack (reflect version), it requires code generation for the models. In contrast, shamaton/msgpack can be used as a drop-in replacement for JSON without extra steps. I've also implemented support for Binding, Autoformat, and Msgpack Encoder API in this fork: Note: I still need to update the utils dependency to include this change before opening a PR. |
WalkthroughThe changes update the MIME type for the "msgpack" file extension in the HTTP utility, introduce new function types for MsgPack marshaling and unmarshaling in a utility package, and add unit tests for MsgPack encoding and decoding using an external library. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Function
participant Struct as sampleStructure
participant MsgPack as MsgPack Encoder/Decoder
Test->>Struct: Create instance with data
Test->>MsgPack: Marshal Struct to bytes
MsgPack-->>Test: Return marshaled bytes
Test->>MsgPack: Unmarshal bytes into new Struct
MsgPack-->>Test: Return decoded Struct
Test->>Test: Assert original and decoded data are equal
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
msgpack_test.go (1)
10-10: Fix struct name to match test usage.The struct is defined as
sampleStructure2but the tests referencesampleStructure. This will cause compilation errors.-type sampleStructure2 struct { +type sampleStructure struct {
🧹 Nitpick comments (1)
http.go (1)
156-156: Fix inconsistency between comment and implementation.The comment mentions "application/msgpack" but the actual MIME type used in the code is "application/vnd.msgpack" (line 191). The code is correct according to IANA standards, but the comment should match the implementation.
-// - Use "application/msgpack" instead of "application/x-msgpack" +// - Use "application/vnd.msgpack" instead of "application/x-msgpack"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (3)
http.go(2 hunks)msgpack.go(1 hunks)msgpack_test.go(1 hunks)
🔇 Additional comments (4)
msgpack.go (1)
1-9: LGTM! Well-designed function type definitions.The function types follow Go conventions and provide a clean abstraction for MsgPack operations. The documentation clearly describes the expected behavior, including error handling for invalid unmarshal targets.
http.go (1)
191-191: LGTM! Correct MIME type according to IANA standards.The update to use "application/vnd.msgpack" aligns with the official IANA media type registration.
msgpack_test.go (2)
14-31: LGTM! Well-structured test for MsgPack encoding.The test properly validates both marshaling and unmarshaling operations with appropriate error checking and assertions.
33-51: LGTM! Comprehensive test for MsgPack decoding.The test follows the same solid pattern as the encoder test, validating the complete encode-decode cycle with proper error handling.
|
@hotrungnhan Problem is that shamaton/msgpack is barely used by projects. We can't rely on a new library. Our options are: Ideally the interface is generic like JSON/XML, and we use tinylib/msgp as default. |
It generates code and works differently from JSON. Do you have any suggestions on how we could integrate it? |
|
@gaby @ReneWerner87 do i still need change the lib ? |
|
good question, for me it would be ok |
|
@gaby WDYT ? |
LGTM |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #118 +/- ##
=======================================
Coverage 96.03% 96.03%
=======================================
Files 9 9
Lines 505 505
=======================================
Hits 485 485
Misses 14 14
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Shall we merge guy ? |
Summary by CodeRabbit