refactor(encoding): replace msgpack with JSON encoding#115
Conversation
- Switch from msgpack to JSON encoding in daemon sync handler - Replace msgpack with JSON in handshake service - Update tests to use JSON encoding - Simplifies serialization and improves compatibility 🤖 Generated with Claude Code (https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Summary of ChangesHello @AnnatarHe, 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 refactors the project's serialization mechanism by migrating from msgpack to standard JSON encoding. The primary goal is to leverage JSON's widespread support and ease of debugging, which is deemed sufficient for current performance requirements. This change impacts the daemon's synchronization logic and the handshake service's data handling, along with their respective tests. Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to replace msgpack with JSON encoding, which is a good move for standardization and debuggability. The changes in the handshake service and tests are a good start, but the refactoring appears to be incomplete and has introduced a critical issue.
Specifically:
- In
model/handshake.go, thesendfunction still sets theContent-Typeheader toapplication/msgpack, which will cause the handshake to fail since the payload is now JSON. - The refactoring in
daemon/handlers.sync.gois partial; the file still contains logic that uses themsgpacklibrary. - Several other parts of the codebase, such as
model/api.base.go, still have dependencies onmsgpack.
I've added specific comments for the critical issue in model/handshake.go and the incomplete refactoring. Please address these issues to ensure the functionality works correctly and consider completing the msgpack to JSON migration across the rest of the codebase for consistency.
| } | ||
|
|
||
| jsonData, err := msgpack.Marshal(data) | ||
| jsonData, err := json.Marshal(data) |
There was a problem hiding this comment.
While you've correctly switched to json.Marshal here, the send function (called on line 111) still hardcodes the Content-Type header as "application/msgpack". This will cause the server to reject the request because the payload is now JSON.
This needs to be updated to "application/json" in the send function.
Additionally, for code cleanup, the msgpack struct tags in handshakeInitRequest (lines 79-81) are now redundant and should be removed.
| } | ||
|
|
||
| jsonData, err := msgpack.Marshal(data) | ||
| jsonData, err := json.Marshal(data) |
There was a problem hiding this comment.
Similar to the Init function, you've switched to json.Marshal here, but the send function (called on line 138) will send the request with an incorrect Content-Type of "application/msgpack". This should be "application/json".
Also, the msgpack tag in the handshakeCheckRequest struct (line 124) should be removed as it's no longer needed.
| } | ||
|
|
||
| buf, err := msgpack.Marshal(payload) | ||
| buf, err := json.Marshal(payload) |
There was a problem hiding this comment.
While you've updated this Marshal call to use JSON, the function still uses msgpack at the beginning (lines 14-21) to process the socketMsgPayload. To complete the refactoring in this file and fully remove the msgpack dependency, that initial processing should also be updated to use json.Marshal and json.Unmarshal.
Update the HTTP request header from application/msgpack to application/json as part of the migration from msgpack to JSON encoding. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
Summary
Rationale
This change simplifies the serialization format by using standard JSON encoding instead of msgpack. JSON is more widely supported and easier to debug, while still providing adequate performance for our use case.
Changes
daemon/handlers.sync.go: Switch from msgpack.Marshal to json.Marshalmodel/handshake.go: Replace msgpack with JSON in Init and Check methodsmodel/handshake_test.go: Update tests to reflect JSON encodingTest plan
🤖 Generated with Claude Code (https://claude.com/claude-code)