docs: clarify CBOR/MsgPack setup and fix tests#3584
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis change removes the default dependency on the external CBOR library and replaces the default CBOR marshal/unmarshal functions with stub functions that panic unless explicitly configured. Tests and documentation are updated to require explicit CBOR setup, and new documentation and examples are provided for CBOR configuration. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Binder
User->>App: Initialize with Config
alt CBOR encoder/decoder not set
App->>Binder: Call UnimplementedCborMarshal/Unmarshal
Binder-->>App: Panic with setup instruction
else CBOR encoder/decoder set
App->>Binder: Call user-provided CBOR marshal/unmarshal
Binder-->>App: Encode/Decode data
end
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (10)
✨ Finishing Touches
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 (
|
![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 @ReneWerner87, 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 refines the handling of CBOR (Concise Binary Object Representation) within the Fiber framework by transitioning its implementation from an implicit default to an explicit opt-in mechanism. This change aims to provide greater clarity and control for users, ensuring they are aware of and actively configure their preferred CBOR serialization libraries. The core changes involve updating Fiber's configuration defaults, introducing panic-on-unimplemented functions for better user feedback, and thoroughly updating both the internal test suite and external documentation to reflect these new requirements.
Highlights
- CBOR Default Behavior Change: Fiber no longer includes a default CBOR implementation. The
CBOREncoderandCBORDecoderfields infiber.Confignow default tobinder.UnimplementedCborMarshalandbinder.UnimplementedCborUnmarshalrespectively. TheseUnimplementedfunctions will panic if invoked without explicit configuration, providing clear guidance to the user. - Enhanced User Guidance: The newly introduced
UnimplementedCborMarshalandUnimplementedCborUnmarshalfunctions panic with a specific message that directs users to the official documentation for instructions on how to properly set up CBOR encoding and decoding. - Comprehensive Documentation Updates: The documentation has been significantly updated to reflect the new CBOR setup requirements. This includes notes in the API reference for
bindandctxmethods, updates to thefiber.Configtable, and a detailed new section in the 'Advance Format' guide with code examples for configuring CBOR. - Test Suite Alignment: All relevant unit and benchmark tests across
bind_test.go,client/helper_test.go, andctx_test.gohave been updated to explicitly configure CBOR encoders and decoders when creating a new Fiber application, ensuring the test suite aligns with the new explicit setup requirement.
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.
Pull Request Overview
Clarifies the binary serialization setup for CBOR alongside MsgPack and updates code/tests to require explicit CBOR configuration.
- Added CBOR section and setup instructions in the advanced formatting guide and linked it from API docs.
- Changed default CBOR encoder/decoder to unimplemented stubs and removed the fxamacker/cbor import from core.
- Updated tests and examples to explicitly configure CBOREncoder/CBORDecoder.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/guide/advance-format.md | Added CBOR setup instructions and updated frontmatter title |
| docs/api/fiber.md | Fixed table formatting and updated default CBOREncoder/Decoder |
| docs/api/ctx.md | Added a note linking to the CBOR guide |
| docs/api/bind.md | Added a note linking to the CBOR guide |
| ctx_test.go | Updated tests to supply CBOREncoder/CBORDecoder in Config |
| client/helper_test.go | Updated test server setup to include CBOR encoder/decoder |
| binder/cbor.go | Introduced unimplemented panic stubs for CBOR marshal/unmarshal |
| binder/cbor_test.go | Added tests for the new unimplemented CBOR stubs |
| bind_test.go | Added CBOR config to bind tests and benchmarks |
| app.go | Switched default CBOR encoder/decoder to unimplemented stubs |
Comments suppressed due to low confidence (3)
binder/cbor.go:27
- Use the full uppercase acronym for CBOR in function names (e.g., rename to UnimplementedCBORMarshal) to stay consistent with the CBOREncoder/CBORDecoder type names.
func UnimplementedCborMarshal(_ any) ([]byte, error) {
binder/cbor.go:31
- Use the full uppercase acronym for CBOR in function names (e.g., rename to UnimplementedCBORUnmarshal) to stay consistent with the CBOREncoder/CBORDecoder type names.
func UnimplementedCborUnmarshal(_ []byte, _ any) error {
docs/api/ctx.md:1900
- [nitpick] For published docs linking, consider removing the
.mdextension so the link resolves correctly (e.g.,../guide/advance-format#cbor).
> **Note:** Before using any CBOR-related features, make sure to follow the [CBOR setup instructions](../guide/advance-format.md#cbor).
| func Test_Ctx_Accepts(t *testing.T) { | ||
| t.Parallel() | ||
| app := New() | ||
| app := New(Config{ |
There was a problem hiding this comment.
[nitpick] Several tests repeat Config setup with CBOREncoder/CBORDecoder; consider extracting a shared helper function to DRY up the test setup.
There was a problem hiding this comment.
Code Review
This pull request does a great job of making CBOR and MsgPack optional dependencies. This is a solid architectural improvement that reduces the default dependency footprint of the framework. The changes are consistent across the codebase, with updates to default configurations, tests, and documentation.
I've provided a few suggestions to improve test conciseness and fix some markdown formatting in the documentation for better readability. Overall, this is a well-executed change.
| func Test_UnimplementedCborMarshal_Panics(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| require.Panics(t, func() { | ||
| _, _ = UnimplementedCborMarshal(struct{ Name string }{Name: "test"}) //nolint:errcheck // this is just a test to trigger the panic | ||
| }) | ||
| } | ||
|
|
||
| func Test_UnimplementedCborUnmarshal_Panics(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| require.Panics(t, func() { | ||
| var out any | ||
| _ = UnimplementedCborUnmarshal([]byte{0xa0}, &out) //nolint:errcheck // this is just a test to trigger the panic | ||
| }) | ||
| } | ||
|
|
||
| func Test_UnimplementedCborMarshal_PanicMessage(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| require.Contains(t, r, "Must explicitly setup CBOR") | ||
| } | ||
| }() | ||
| _, _ = UnimplementedCborMarshal(struct{ Name string }{Name: "test"}) //nolint:errcheck // this is just a test to trigger the panic | ||
| } | ||
|
|
||
| func Test_UnimplementedCborUnmarshal_PanicMessage(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| defer func() { | ||
| if r := recover(); r != nil { | ||
| require.Contains(t, r, "Must explicitly setup CBOR") | ||
| } | ||
| }() | ||
| var out any | ||
| _ = UnimplementedCborUnmarshal([]byte{0xa0}, &out) //nolint:errcheck // this is just a test to trigger the panic | ||
| } |
There was a problem hiding this comment.
These tests for panics and panic messages are a bit redundant. The _Panics and _PanicMessage tests for both Marshal and Unmarshal can be combined into a single test for each by using require.PanicsWithValue.
This will make the tests more concise and easier to maintain while also providing stronger assertions by checking for the exact panic message.
| func Test_UnimplementedCborMarshal_Panics(t *testing.T) { | |
| t.Parallel() | |
| require.Panics(t, func() { | |
| _, _ = UnimplementedCborMarshal(struct{ Name string }{Name: "test"}) //nolint:errcheck // this is just a test to trigger the panic | |
| }) | |
| } | |
| func Test_UnimplementedCborUnmarshal_Panics(t *testing.T) { | |
| t.Parallel() | |
| require.Panics(t, func() { | |
| var out any | |
| _ = UnimplementedCborUnmarshal([]byte{0xa0}, &out) //nolint:errcheck // this is just a test to trigger the panic | |
| }) | |
| } | |
| func Test_UnimplementedCborMarshal_PanicMessage(t *testing.T) { | |
| t.Parallel() | |
| defer func() { | |
| if r := recover(); r != nil { | |
| require.Contains(t, r, "Must explicitly setup CBOR") | |
| } | |
| }() | |
| _, _ = UnimplementedCborMarshal(struct{ Name string }{Name: "test"}) //nolint:errcheck // this is just a test to trigger the panic | |
| } | |
| func Test_UnimplementedCborUnmarshal_PanicMessage(t *testing.T) { | |
| t.Parallel() | |
| defer func() { | |
| if r := recover(); r != nil { | |
| require.Contains(t, r, "Must explicitly setup CBOR") | |
| } | |
| }() | |
| var out any | |
| _ = UnimplementedCborUnmarshal([]byte{0xa0}, &out) //nolint:errcheck // this is just a test to trigger the panic | |
| } | |
| func Test_UnimplementedCborMarshal_Panics(t *testing.T) { | |
| t.Parallel() | |
| expectedPanicMsg := "Must explicitly setup CBOR, please check docs: https://docs.gofiber.io/next/guide/advance-format#cbor" | |
| require.PanicsWithValue(t, expectedPanicMsg, func() { | |
| _, _ = UnimplementedCborMarshal(struct{ Name string }{Name: "test"}) //nolint:errcheck // this is just a test to trigger the panic | |
| }) | |
| } | |
| func Test_UnimplementedCborUnmarshal_Panics(t *testing.T) { | |
| t.Parallel() | |
| expectedPanicMsg := "Must explicitly setup CBOR, please check docs: https://docs.gofiber.io/next/guide/advance-format#cbor" | |
| require.PanicsWithValue(t, expectedPanicMsg, func() { | |
| var out any | |
| _ = UnimplementedCborUnmarshal([]byte{0xa0}, &out) //nolint:errcheck // this is just a test to trigger the panic | |
| }) | |
| } |
| | Property | Type | Description | Default | | ||
| |---------------------------------------------------------------------------------------|-----------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------| |
There was a problem hiding this comment.
The markdown table formatting for the Config fields seems to be misaligned, especially for the Default column. This can affect the readability of the raw markdown file. It's better to ensure the column separators are properly aligned with the content.
| | Property | Type | Description | Default | | |
| |---------------------------------------------------------------------------------------|-----------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|------------------------------------------------------------------------| | |
| | Property | Type | Description | Default | | |
| |---------------------------------------------------------------------------------------|-----------------------------------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------------------------------------------| |
| | Property | Type | Description | Default | | ||
| |-------------------------------------------------------------------------|-------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------------| |
There was a problem hiding this comment.
Similar to the previous table, the markdown formatting for this table is misaligned. Aligning the columns will improve the readability of the raw markdown.
| | Property | Type | Description | Default | | |
| |-------------------------------------------------------------------------|-------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|--------------------| | |
| | Property | Type | Description | Default | | |
| |-------------------------------------------------------------------------|-------------------------------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|----------------------| |
Description
Please provide a clear and concise description of the changes you've made and the problem they address. Include the purpose of the change, any relevant issues it solves, and the benefits it brings to the project. If this change introduces new features or adjustments, highlight them here.
Fixes # (issue)
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md