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

🔥 feat: Add support for CBOR encoding #3173

Merged
merged 24 commits into from
Dec 1, 2024
Merged

Conversation

imsk17
Copy link
Contributor

@imsk17 imsk17 commented Oct 20, 2024

Description

Adds support for CBOR encoding and decoding through Gofiber.

Fixes #3156

Type of change

  • New feature (non-breaking change which adds functionality)
  • Documentation update (changes to documentation)

@imsk17 imsk17 requested a review from a team as a code owner October 20, 2024 22:16
@imsk17 imsk17 requested review from gaby, sixcolors, ReneWerner87 and efectn and removed request for a team October 20, 2024 22:16
Copy link
Contributor

coderabbitai bot commented Oct 20, 2024

Walkthrough

The changes in this pull request enhance the fiber package by adding support for CBOR (Concise Binary Object Representation) data serialization. Key modifications include the introduction of new configuration options for custom CBOR encoding and decoding, the addition of a CBOR method in the DefaultCtx struct for encoding data, and a new MIME type constant for CBOR. These updates provide users with more flexible data handling capabilities within the framework.

Changes

File Change Summary
app.go Added fields CBOREncoder and CBORDecoder to Config struct for custom CBOR handling.
constants.go Introduced new constant MIMEApplicationCBOR with value "application/cbor".
ctx.go, ctx_interface_gen.go, ctx_test.go Added CBOR method to DefaultCtx and Ctx interface for CBOR encoding, including error handling and tests.
bind.go, client/client.go, client/request.go, client/response.go Added methods for CBOR handling in Bind, Client, and Response structs, including serialization and deserialization capabilities.
middleware/cache/manager_msgp.go Modified DecodeMsg and UnmarshalMsg methods to initialize za0002 to an empty byte slice if nil.
docs/whats_new.md Updated to reflect changes in Fiber v3, including CBOR support and other framework enhancements.

Assessment against linked issues

Objective Addressed Explanation
Add support for CBOR codec
Implement CBOR serialization in context
Ensure CBOR handling aligns with RFC standards

Possibly related PRs

Suggested reviewers

  • gaby
  • sixcolors
  • efectn

Poem

🐰 In the land of code so bright,
CBOR hops in with delight.
New paths for data, oh what a sight,
With encoders and decoders, all feels right!
So let’s celebrate this joyful change,
In our framework, we’ll rearrange! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@gaby gaby added the v3 label Oct 20, 2024
@gaby gaby added this to the v3 milestone Oct 20, 2024
@gaby gaby changed the title feat(cbor): allow encoding response bodies in cbor feat: Add support for CBOR encoding Oct 20, 2024
@gaby gaby changed the title feat: Add support for CBOR encoding 🔥 feat: Add support for CBOR encoding Oct 20, 2024
@gaby
Copy link
Member

gaby commented Oct 20, 2024

@imsk17 Thank you for the PR. Seems the tests are failing. The hex values dont match.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (3)
ctx.go (2)

886-902: LGTM! Consider adding a brief comment.

The implementation of the CBOR method looks good. It follows the pattern of similar methods like JSON, handles errors appropriately, and allows for custom content types. The use of SetBodyRaw is efficient for setting the response body.

Consider adding a brief comment explaining the purpose of this method, similar to other methods in this file. For example:

// CBOR converts any interface or string to CBOR encoded bytes and sets it as the response body.
// If the ctype parameter is given, it sets the Content-Type header to the provided value.
// Otherwise, it sets the Content-Type header to "application/cbor".
func (c *DefaultCtx) CBOR(data any, ctype ...string) error {
    // ... (existing implementation)
}

Line range hint 1048-1066: Consider improving error handling in the JSON method

For consistency with the newly added CBOR method, consider updating the JSON method to return an error. This would allow for better error handling when JSON encoding fails.

Here's a suggested modification to the JSON method:

-func (c *DefaultCtx) JSON(data any, ctype ...string) error {
+func (c *DefaultCtx) JSON(data any, ctype ...string) error {
 	raw, err := c.app.config.JSONEncoder(data)
 	if err != nil {
-		return err
+		return fmt.Errorf("failed to encode JSON: %w", err)
 	}
 	c.fasthttp.Response.SetBodyRaw(raw)
 	if len(ctype) > 0 {
 		c.fasthttp.Response.Header.SetContentType(ctype[0])
 	} else {
 		c.fasthttp.Response.Header.SetContentType(MIMEApplicationJSON)
 	}
 	return nil
 }

This change would make the error handling more informative and consistent with the new CBOR method.

app.go (1)

321-334: Ensure comments for CBOR encoder and decoder fields are clear and consistent

The comments for CBOREncoder and CBORDecoder in the Config struct could be improved for clarity and consistency with existing documentation comments. Consider revising the comments to match the style of other encoder/decoder fields.

Apply this diff to adjust the comments:

321,325c321,325
-	// When set by an external client of Fiber it will use the provided implementation of a
-	// CBORMarshal
-	//
-	// Allowing for flexibility in using another cbor library for encoding
-	// Default: cbor.Marshal
+	// When set by an external client of Fiber, it will use the provided implementation of a
+	// CBORMarshal.
+	//
+	// Allowing for flexibility in using another CBOR library for encoding.
+	// Default: cbor.Marshal

Similarly, adjust the comments for CBORDecoder:

328,333c328,333
-	// When set by an external client of Fiber it will use the provided implementation of a
-	// CBORUnmarshal
-	//
-	// Allowing for flexibility in using another cbor library for decoding
-	// Default: cbor.Unmarshal
+	// When set by an external client of Fiber, it will use the provided implementation of a
+	// CBORUnmarshal.
+	//
+	// Allowing for flexibility in using another CBOR library for decoding.
+	// Default: cbor.Unmarshal
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 298975a and 7f7fb38.

⛔ Files ignored due to path filters (2)
  • go.mod is excluded by !**/*.mod
  • go.sum is excluded by !**/*.sum, !**/*.sum
📒 Files selected for processing (6)
  • app.go (3 hunks)
  • constants.go (1 hunks)
  • ctx.go (1 hunks)
  • ctx_interface_gen.go (1 hunks)
  • ctx_test.go (2 hunks)
  • middleware/cache/manager_msgp.go (2 hunks)
🧰 Additional context used
🔇 Additional comments (14)
middleware/cache/manager_msgp.go (3)

55-57: Approve: Improved nil handling in headers map

This change enhances the robustness of the DecodeMsg method by ensuring that no nil values are stored in the headers map. By initializing nil byte slices to empty slices, it maintains consistency in the data structure and potentially prevents nil pointer dereferences in code that uses these headers. This is a good practice for handling potentially nil values.


273-275: Approve: Consistent nil handling in UnmarshalMsg

This change in the UnmarshalMsg method mirrors the improvement made in DecodeMsg. It ensures consistent handling of nil values across different serialization operations, maintaining data integrity and preventing potential issues with nil values in the headers map. This consistency is crucial and demonstrates a thorough approach to the nil-value handling enhancement.


55-57: Summary: Improved robustness in cache serialization

These changes enhance the reliability of the caching mechanism by ensuring consistent handling of nil values in the headers map during both decoding and unmarshaling operations. While not directly related to CBOR encoding, this improvement aligns with the PR's objective of enhancing data handling capabilities within the framework. It provides a more robust foundation for interacting with various data formats, including CBOR, by ensuring that the underlying data structures are always in a consistent state.

The nil-to-empty-slice conversion simplifies error handling for consumers of this cache data, as they can now assume that header values are always valid byte slices. This could lead to more predictable behavior and easier integration with new features like CBOR encoding.

While there might be a negligible performance impact due to the additional nil checks and potential memory allocations for empty slices, the benefits in terms of increased robustness and consistency outweigh this minor concern.

Also applies to: 273-275

constants.go (2)

26-26: LGTM: Correct MIME type constant for CBOR added

The addition of MIMEApplicationCBOR constant with the value "application/cbor" is correct and consistent with the existing MIME type constants in the file. This constant will be useful for setting the appropriate Content-Type header when sending CBOR-encoded responses.


26-26: Summary: Minimal, focused change for CBOR support

The addition of the MIMEApplicationCBOR constant is the only change in this file. It's a minimal and focused modification that supports the PR's objective of adding CBOR encoding capabilities. The change maintains backward compatibility and follows existing patterns in the codebase.

ctx_interface_gen.go (3)

166-170: New CBOR method added to Ctx interface

The addition of the CBOR method to the Ctx interface is consistent with the existing pattern for other data serialization methods like JSON and XML. This enhancement allows for encoding response bodies in CBOR format, which aligns with the PR objectives.

A few observations:

  1. The method signature CBOR(data any, ctype ...string) error is consistent with other serialization methods in the interface.
  2. The comment describes the functionality and default behavior for the Content-Type header, which is helpful for developers.

To ensure consistency across the codebase, let's verify the implementation of this method:

#!/bin/bash
# Search for the CBOR method implementation
rg -p 'func \(c \*DefaultCtx\) CBOR\(' --type go

Line range hint 1-170: Summary of changes to ctx_interface_gen.go

The changes to this file are minimal but significant:

  1. A new CBOR method has been added to the Ctx interface, enhancing the framework's serialization capabilities.
  2. The addition is consistent with existing patterns and doesn't break the interface contract.

Overall, this change successfully implements part of the PR's objective to allow encoding response bodies in CBOR format. The integration is clean and follows the established conventions of the fiber package.

To ensure full implementation:

Let's check for any related configuration options or utilities added for CBOR support:

#!/bin/bash
# Search for CBOR-related additions in other files
rg -p 'CBOR' --type go

Line range hint 1-170: Overall interface consistency and documentation considerations

The addition of the CBOR method to the Ctx interface is well-integrated and consistent with the existing design. It follows the same pattern as other serialization methods like JSON, JSONP, and XML.

Considerations:

  1. The interface remains backwards-compatible as the new method doesn't alter existing method signatures.
  2. The placement of the CBOR method alongside other serialization methods enhances discoverability for developers.

Consider updating the package documentation and examples to include usage of the new CBOR functionality. This will help developers understand how to utilize this new feature effectively.

Let's check if the documentation has been updated:

ctx.go (1)

Line range hint 1-3140: Overall, the changes look good with minor suggestions for improvement

The addition of the CBOR method to the DefaultCtx struct is well-implemented and consistent with the existing codebase. The suggested improvements (adding a comment to the CBOR method and updating the JSON method's error handling) would enhance the code's documentation and consistency.

No other significant issues were found in the surrounding code. The file maintains a good structure and follows consistent patterns throughout.

app.go (2)

26-26: Import statement added appropriately

The import of github.com/fxamacker/cbor/v2 is necessary to support CBOR encoding and decoding functionalities.


552-557: Default CBOR encoder and decoder are correctly initialized

The default CBOR encoder and decoder are properly set to cbor.Marshal and cbor.Unmarshal if they are not provided in the configuration, ensuring consistent behavior.

ctx_test.go (3)

15-15: Import of "encoding/hex" is appropriate

The addition of "encoding/hex" to the imports is necessary for encoding CBOR outputs to hex strings in the tests.


3576-3631: CBOR test functions are well implemented

The Test_Ctx_CBOR function provides comprehensive tests for the CBOR encoding functionality. It covers various cases including complex numbers, different data types, custom content types, empty values, and custom CBOR encoders. The tests are well-structured and follow existing patterns in the codebase.


3633-3655: Benchmark function for CBOR encoding is properly structured

The Benchmark_Ctx_CBOR function effectively measures the performance of CBOR encoding. It aligns with the standard benchmarking practices used in the codebase, ensuring consistency and reliability of benchmark results.

@imsk17
Copy link
Contributor Author

imsk17 commented Oct 20, 2024

Yea, I fixed that flaky test.

@gaby
Copy link
Member

gaby commented Oct 20, 2024

Yea, I fixed that flaky test.

Thanks! 💪

Copy link

codecov bot commented Oct 20, 2024

Codecov Report

Attention: Patch coverage is 69.35484% with 19 lines in your changes missing coverage. Please review.

Project coverage is 82.80%. Comparing base (31a503f) to head (fc1779b).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
middleware/cache/manager_msgp.go 0.00% 6 Missing ⚠️
client/client.go 58.33% 5 Missing ⚠️
binder/cbor.go 0.00% 4 Missing ⚠️
client/hooks.go 62.50% 2 Missing and 1 partial ⚠️
binder/mapping.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3173      +/-   ##
==========================================
+ Coverage   82.77%   82.80%   +0.03%     
==========================================
  Files         114      115       +1     
  Lines       11197    11258      +61     
==========================================
+ Hits         9268     9322      +54     
- Misses       1528     1537       +9     
+ Partials      401      399       -2     
Flag Coverage Δ
unittests 82.80% <69.35%> (+0.03%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 7f7fb38 and ea59267.

📒 Files selected for processing (1)
  • ctx_test.go (2 hunks)
🧰 Additional context used

ctx_test.go Outdated Show resolved Hide resolved
ctx_test.go Show resolved Hide resolved
@gaby
Copy link
Member

gaby commented Oct 20, 2024

@imsk17 Can you add a smalk section in https://github.com/gofiber/fiber/blob/main/docs/whats_new.md explaining the new feature. This will be used in the future to showcase Fiber v2 vs V3

Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

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

👍 LGTM, just missing a section in "Whats_Change.md"

@imsk17
Copy link
Contributor Author

imsk17 commented Oct 20, 2024

Done.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)
docs/whats_new.md (5)

199-200: Consider expanding the CBOR documentation

The introduction of CBOR support is a significant feature that aligns well with the PR objectives. To ensure users can fully leverage this new capability, consider expanding the documentation with:

  1. A brief explanation of what CBOR is and its benefits.
  2. An example of how to use the new CBOR method.
  3. Any performance considerations or best practices for using CBOR in Fiber.

This additional information would help users understand and adopt the new feature more easily.

Also applies to: 213-213


Line range hint 76-91: Enhance explanation of Router changes

The updates to the Router interface, particularly the changes to HTTP method registration and the new Route chaining, are significant improvements. To help users better understand and appreciate these changes, consider:

  1. Explaining the benefits of making the handler mandatory in the method signatures (e.g., improved type safety, clearer API).
  2. Providing a brief comparison of the old and new Route chaining syntax, highlighting how it improves code readability and maintainability.
  3. Adding a note on any potential breaking changes these updates might cause for existing codebases.

These additions would provide more context and help users understand why these changes were made and how they can benefit from them.

Also applies to: 96-102

🧰 Tools
🪛 LanguageTool

[uncategorized] ~203-~203: The official spelling of this programming framework is “Express.js”.
Context: ...ions. ### new methods - AutoFormat -> ExpressJs like - Host -> ExpressJs like - Port ->...

(NODE_JS)


Line range hint 252-254: Expand the Client package section

The Client package rebuild seems to be a major update in Fiber v3. To give users a better understanding of these changes, consider:

  1. Listing a few key new features or improvements (e.g., Cookiejar, request/response hooks) with brief explanations.
  2. Providing a simple before-and-after code example to illustrate how the usage of the Client package has changed.
  3. Mentioning any performance improvements or changes in behavior that users should be aware of.

This additional information would help users quickly grasp the significance of the Client package updates and how they might affect their existing code.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~203-~203: The official spelling of this programming framework is “Express.js”.
Context: ...ions. ### new methods - AutoFormat -> ExpressJs like - Host -> ExpressJs like - Port ->...

(NODE_JS)


Line range hint 321-326: Clarify middleware changes and migration steps

The updates to various middlewares represent significant changes in Fiber v3. To ensure a smooth transition for users, consider:

  1. For the Filesystem middleware removal:

    • Explain the rationale behind merging it into the Static middleware.
    • Provide a more detailed example of how to migrate from Filesystem to Static, covering common use cases.
  2. For the Monitor middleware move to Contrib:

    • Explain why this change was made (e.g., to keep the core lighter, to allow for more frequent updates).
    • Provide clear instructions on how to install and import the Contrib package.
  3. For other middleware updates (e.g., Adaptor, CORS):

    • Highlight any potential breaking changes.
    • Provide before-and-after examples for common configurations.

These additions would help users understand the impact of these changes and how to update their existing code effectively.

Also applies to: 410-416, 495-507

🧰 Tools
🪛 LanguageTool

[uncategorized] ~203-~203: The official spelling of this programming framework is “Express.js”.
Context: ...ions. ### new methods - AutoFormat -> ExpressJs like - Host -> ExpressJs like - Port ->...

(NODE_JS)


Line range hint 509-515: Enhance the migration guide structure and content

The migration guide is a crucial resource for users upgrading to Fiber v3. To make it even more effective, consider the following improvements:

  1. Add a brief introduction explaining the scope of changes in v3 and who needs to pay attention to which sections.
  2. Include a "Quick Start" section for users with simpler applications, highlighting the most common and critical changes.
  3. For each major change, provide:
    • A brief explanation of why the change was made.
    • A complete before-and-after code example.
    • Any gotchas or common pitfalls to watch out for during migration.
  4. Add a "Troubleshooting" section addressing common issues users might encounter during the upgrade process.
  5. Consider adding a "Deprecation Timeline" for any features that are being phased out but not immediately removed in v3.

These enhancements would make the migration guide more user-friendly and comprehensive, helping to ensure a smoother transition for all Fiber users.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~203-~203: The official spelling of this programming framework is “Express.js”.
Context: ...ions. ### new methods - AutoFormat -> ExpressJs like - Host -> ExpressJs like - Port ->...

(NODE_JS)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between ea59267 and db00468.

📒 Files selected for processing (1)
  • docs/whats_new.md (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
docs/whats_new.md (1)

Line range hint 1-515: Overall, a comprehensive and well-structured document with room for enhancement

This "What's New in v3" document provides a thorough overview of the changes and improvements in Fiber v3. It covers a wide range of topics including app configuration, routing, context handling, client package updates, and middleware changes. The inclusion of a migration guide is particularly helpful for users upgrading from v2.

To further improve this document, consider:

  1. Expanding on new features like CBOR support with more detailed explanations and examples.
  2. Providing more context for significant changes, such as those in the Router interface, to help users understand the benefits and potential impacts.
  3. Enhancing the Client package section with more specific details about new features and improvements.
  4. Clarifying the middleware changes, particularly for removed or relocated middlewares, with more detailed migration instructions.
  5. Structuring the migration guide to be more user-friendly, including a quick start section, more comprehensive examples, and a troubleshooting guide.

These enhancements would make the document even more valuable for Fiber users, ensuring a smooth transition to v3 and helping them leverage all the new features and improvements effectively.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~203-~203: The official spelling of this programming framework is “Express.js”.
Context: ...ions. ### new methods - AutoFormat -> ExpressJs like - Host -> ExpressJs like - Port ->...

(NODE_JS)

@gaby
Copy link
Member

gaby commented Oct 20, 2024

@imsk17 I found one more place CBOR is missing. In the bind/binder which have support for JSON and XML

See here:

@imsk17
Copy link
Contributor Author

imsk17 commented Oct 20, 2024

Right. I will have a look.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (4)
docs/whats_new.md (4)

199-200: Enhance CBOR documentation with examples and configuration options.

The CBOR feature introduction would benefit from:

  1. Code examples showing usage
  2. Configuration options (if any)
  3. Performance characteristics compared to JSON/XML

Add a code example like this:

// Example CBOR usage
app.Get("/", func(c *fiber.Ctx) error {
    data := map[string]interface{}{
        "message": "Hello, World!",
        "number": 42,
    }
    return c.CBOR(data)
})

213-213: Cross-reference CBOR method in Context section.

The CBOR method listing should include a link to its detailed API documentation and mention any available configuration options.

Update the line to:

-CBOR -> for CBOR encoding
+CBOR -> for CBOR encoding (see [CBOR API documentation](./api/ctx.md#cbor))

Line range hint 1-24: Consider adding version compatibility matrix.

The introduction section would benefit from a clear compatibility matrix showing:

  1. Go version requirements
  2. Breaking changes overview
  3. Deprecation timeline for v2

Add a compatibility matrix like:

| Fiber Version | Go Version | Status      |
|--------------|------------|-------------|
| v3.x         | ≥ 1.22     | Current     |
| v2.x         | ≥ 1.17     | Maintenance |
🧰 Tools
🪛 LanguageTool

[uncategorized] ~203-~203: The official spelling of this programming framework is “Express.js”.
Context: ...ions. ### new methods - AutoFormat -> ExpressJs like - Host -> ExpressJs like - Port ->...

(NODE_JS)


Line range hint 12-24: Update table of contents to match actual content.

The table of contents is missing some sections that are present in the document:

  1. Drop for old Go versions
  2. Generic functions

Update the table of contents to include all sections and maintain consistent ordering with the document body.

🧰 Tools
🪛 LanguageTool

[uncategorized] ~203-~203: The official spelling of this programming framework is “Express.js”.
Context: ...ions. ### new methods - AutoFormat -> ExpressJs like - Host -> ExpressJs like - Port ->...

(NODE_JS)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between db00468 and 84c71a5.

📒 Files selected for processing (1)
  • docs/whats_new.md (2 hunks)
🔇 Additional comments (1)
docs/whats_new.md (1)

Line range hint 199-213: Verify CBOR implementation details in codebase.

Let's verify the CBOR implementation details to ensure documentation accuracy.

✅ Verification successful

CBOR implementation is properly documented and implemented

The documentation accurately reflects the CBOR implementation in the codebase. The verification shows:

  • CBOR is fully implemented using the github.com/fxamacker/cbor/v2 package
  • The implementation includes:
    • CBOR() method in the context with proper content type handling
    • Configurable CBOREncoder/CBORDecoder interfaces
    • Comprehensive test coverage in ctx_test.go
    • Default MIME type constant MIMEApplicationCBOR

The feature is well-tested, properly documented, and follows the same patterns as other response formats in the framework.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for CBOR-related code
echo "Searching for CBOR method implementation..."
ast-grep --pattern 'func $_(c *fiber.Ctx) error {
  $$$
  c.CBOR($$$)
  $$$
}'

echo "Searching for CBOR tests..."
rg -l "TestCBOR" -g "**/*_test.go"

echo "Searching for CBOR configuration..."
rg "CBOR.*Config"

Length of output: 378


Script:

#!/bin/bash
# Let's try different search patterns to find CBOR implementation

# Search for CBOR-related files and functions
echo "Searching for CBOR in all files..."
rg -i "cbor" -A 5 -B 5

# Search for CBOR type or interface definitions
echo "Searching for CBOR type definitions..."
ast-grep --pattern 'type $_ struct {
  $$$
  CBOR $$$
  $$$
}'

# Search for CBOR method declarations
echo "Searching for CBOR method declarations..."
ast-grep --pattern 'func (c *Ctx) CBOR($$$) error'

# Look for imports of CBOR-related packages
echo "Searching for CBOR-related imports..."
rg "import.*cbor"

Length of output: 10999

🧰 Tools
🪛 LanguageTool

[uncategorized] ~203-~203: The official spelling of this programming framework is “Express.js”.
Context: ...ions. ### new methods - AutoFormat -> ExpressJs like - Host -> ExpressJs like - Port ->...

(NODE_JS)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)
client/response_test.go (2)

244-254: LGTM! Consider extracting test data as constants

The CBOR endpoint implementation is clean and follows the established patterns. The struct is properly tagged for CBOR serialization.

Consider extracting the test data into constants at the package level since it's reused in the test case below:

+const (
+    testName = "foo"
+    testAge  = 12
+)

 app.Get("/cbor", func(c fiber.Ctx) error {
     type cborData struct {
         Name string `cbor:"name"`
         Age  int    `cbor:"age"`
     }

     return c.CBOR(cborData{
-        Name: "foo",
-        Age:  12,
+        Name: testName,
+        Age:  testAge,
     })
 })

343-371: Consider improving test coverage and reducing duplication

The test case is well-structured and follows the established patterns. However, there are a few improvements that could be made:

  1. The cborData struct is duplicated from the endpoint implementation. Consider moving it to a shared location.
  2. The test only covers the success case. Consider adding test cases for error scenarios.

Here's a suggested refactor to improve the test coverage:

+// Move to package level
+type cborData struct {
+    Name string `cbor:"name"`
+    Age  int    `cbor:"age"`
+}

 t.Run("cbor body", func(t *testing.T) {
     t.Parallel()
-    type cborData struct {
-        Name string `cbor:"name"`
-        Age  int    `cbor:"age"`
-    }

     data := cborData{
         Name: "foo",
         Age:  12,
     }
     // ... rest of the success test
 })

+t.Run("cbor body invalid", func(t *testing.T) {
+    t.Parallel()
+    server := setupApp()
+    defer server.stop()
+
+    client := New().SetDial(server.dial())
+
+    resp, err := AcquireRequest().
+        SetClient(client).
+        Get("http://example.com/json") // Get JSON instead of CBOR
+
+    require.NoError(t, err)
+
+    tmp := &cborData{}
+    err = resp.CBOR(tmp)
+    require.Error(t, err)
+    resp.Close()
+})
client/hooks_test.go (1)

460-482: Consider adding more comprehensive test cases.

To ensure robust CBOR support, consider adding:

  1. Error case testing (e.g., invalid CBOR data)
  2. Tests for more complex data types (maps, arrays, nested structures)
  3. Verification of the application/cbor content-type header

Example addition:

+	t.Run("CBOR error case", func(t *testing.T) {
+		t.Parallel()
+		client := New()
+		req := AcquireRequest().
+			SetCBOR(make(chan int)) // Invalid CBOR type
+
+		err := parserRequestBody(client, req)
+		require.Error(t, err)
+	})
+
+	t.Run("CBOR complex types", func(t *testing.T) {
+		t.Parallel()
+		type nested struct {
+			Value string `cbor:"value"`
+		}
+		type complexData struct {
+			Map    map[string]nested `cbor:"map"`
+			Array  []int            `cbor:"array"`
+			Nested nested           `cbor:"nested"`
+		}
+		data := complexData{
+			Map: map[string]nested{"key": {Value: "value"}},
+			Array: []int{1, 2, 3},
+			Nested: nested{Value: "nested"},
+		}
+		
+		client := New()
+		req := AcquireRequest().
+			SetCBOR(data)
+
+		err := parserRequestBody(client, req)
+		require.NoError(t, err)
+		
+		// Verify content type
+		require.Equal(t, "application/cbor", string(req.RawRequest.Header.ContentType()))
+		
+		// Verify encoding
+		encoded, err := cbor.Marshal(data)
+		require.NoError(t, err)
+		require.Equal(t, encoded, req.RawRequest.Body())
+	})
bind_test.go (2)

1102-1129: Consider adding CBOR-specific struct tags

While the benchmark is well-structured, consider using CBOR-specific struct tags for clarity, even though the json tags work with CBOR.

 type Demo struct {
-    Name string `json:"name"`
+    Name string `json:"name" cbor:"name"`
 }

1750-1756: Remove extra empty line after error check

For consistency with the rest of the codebase, remove the extra empty line after the error check.

 if err != nil {
     t.Error(err)
 }
-
🧰 Tools
🪛 golangci-lint (1.62.2)

1753-1753: File is not gofumpt-ed with -extra

(gofumpt)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 2b11150 and 697243b.

📒 Files selected for processing (3)
  • bind_test.go (4 hunks)
  • client/hooks_test.go (2 hunks)
  • client/response_test.go (2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
bind_test.go

1753-1753: File is not gofumpt-ed with -extra

(gofumpt)

🔇 Additional comments (6)
client/response_test.go (1)

Line range hint 243-371: Verify CBOR implementation in related files

The test implementation looks good and aligns with the PR objectives. Let's verify that CBOR support is consistently implemented across all related files.

✅ Verification successful

Based on the search results, I can generate the final response. The codebase shows consistent CBOR implementation across key components:

CBOR implementation is properly integrated

The implementation is consistent and complete across the codebase:

  • CBOR interface method is properly defined in the core Ctx interface
  • CBOR-related code is present in client, binder, and core components
  • The implementation follows the same pattern as JSON/XML handlers
  • Test coverage exists for both client and server-side CBOR functionality
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for consistent CBOR implementation across related files

# Check for CBOR-related code in bind/binder files
echo "Checking bind/binder files for CBOR support..."
rg -l "cbor" ../bind/

# Check for MIME type constant
echo "Checking for CBOR MIME type constant..."
rg -l "application/cbor" ../utils/

# Check for CBOR encoder/decoder configuration
echo "Checking for CBOR configuration..."
rg -l "CBOREncoder|CBORDecoder" ../config/

# Check for CBOR method in interfaces
echo "Checking for CBOR method in interfaces..."
rg -l "CBOR\(" ../interfaces/

Length of output: 702


Script:

#!/bin/bash
# Let's try searching from the repository root instead of using relative paths
# First, let's understand the repository structure
fd --type d --max-depth 2

# Then search for CBOR-related code
echo "Checking for CBOR-related code..."
rg -l "cbor"

# Check for CBOR method implementations
echo "Checking for CBOR method implementations..."
ast-grep --pattern 'func $$$CBOR$$$'

# Check for CBOR-related types and interfaces
echo "Checking for CBOR-related types..."
ast-grep --pattern 'type $_ interface {
  $$$
  CBOR($$$)
  $$$
}'

Length of output: 28905

client/hooks_test.go (2)

13-13: LGTM! Good choice of CBOR library.

The fxamacker/cbor library is a well-maintained, security-focused implementation that aligns well with Fiber's quality standards.


460-482: LGTM! Well-structured test case.

The test follows the established patterns and properly verifies CBOR encoding functionality.

bind_test.go (3)

15-15: LGTM: Using a well-maintained CBOR library

The fxamacker/cbor library is a good choice as it's a widely-used, well-maintained implementation for CBOR in Go.


931-936: LGTM: Well-structured CBOR test case

The test case follows the established pattern and includes proper error handling.


Line range hint 760-760: Fix handling of CBOR binary data

CBOR data is binary and should not be converted to a string as it can lead to data corruption.

Apply this fix:

-testDecodeParser(MIMEApplicationCBOR, string(cb))
+testDecodeParser(MIMEApplicationCBOR, cb)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (7)
docs/whats_new.md (2)

262-263: Enhance CBOR documentation with examples and use cases.

While the introduction of CBOR support is well-documented, it would be beneficial to add:

  1. Code examples showing how to use CBOR encoding
  2. Common use cases where CBOR is preferred over JSON
  3. Performance comparisons with other encoding formats

Consider adding the following example:

+### CBOR Example
+
+```go
+app.Get("/", func(c fiber.Ctx) error {
+    // Example data
+    data := fiber.Map{
+        "message": "Hello, World!",
+        "numbers": []int{1, 2, 3},
+    }
+    
+    // Send response in CBOR format
+    return c.CBOR(data)
+})
+```
+
+CBOR is particularly useful when:
+- Bandwidth efficiency is crucial
+- Working with IoT devices
+- Dealing with binary data

277-277: Add CBOR method documentation with parameter details.

The CBOR method is listed but lacks implementation details. Consider adding:

  1. Method signature
  2. Parameter descriptions
  3. Return values
  4. Error handling examples

Add the following documentation:

+### CBOR Method
+
+```go
+func (c *Ctx) CBOR(data interface{}) error
+```
+
+Serializes the given data to CBOR format and sets the Content-Type header to "application/cbor".
+
+#### Parameters
+- `data interface{}`: The data to be encoded to CBOR format
+
+#### Returns
+- `error`: Returns an error if serialization fails
+
+#### Example with Error Handling
+```go
+app.Get("/", func(c fiber.Ctx) error {
+    data := fiber.Map{"hello": "world"}
+    if err := c.CBOR(data); err != nil {
+        return c.Status(fiber.StatusInternalServerError).SendString("Failed to encode CBOR")
+    }
+    return nil
+})
+```
client/request_test.go (1)

1010-1027: LGTM! Consider adding more test cases.

The CBOR test case follows the established pattern and verifies both content type and encoding. However, consider enhancing test coverage with additional cases:

  1. Test different data types (numbers, arrays, nested objects)
  2. Test edge cases (null values, empty structs)
  3. Test error cases (invalid CBOR data)

Example additional test case:

+t.Run("cbor complex types", func(t *testing.T) {
+    t.Parallel()
+    testRequest(t,
+        func(c fiber.Ctx) error {
+            require.Equal(t, "application/cbor", string(c.Request().Header.ContentType()))
+            return c.SendString(string(c.Request().Body()))
+        },
+        func(agent *Request) {
+            type nested struct {
+                Value int `cbor:"val"`
+            }
+            type args struct {
+                Numbers []int     `cbor:"nums"`
+                Nested  nested    `cbor:"obj"`
+                Null    *string   `cbor:"null"`
+            }
+            agent.SetCBOR(args{
+                Numbers: []int{1, 2, 3},
+                Nested:  nested{Value: 42},
+                Null:    nil,
+            })
+        },
+        // Add expected CBOR hex string here
+    )
+})
bind_test.go (1)

1111-1138: Add memory allocation assertions to CBOR benchmark

The benchmark should verify memory allocation patterns to ensure efficient CBOR handling.

Add allocation assertions:

 func Benchmark_Bind_Body_CBOR(b *testing.B) {
     // ... existing code ...
     
     b.ReportAllocs()
     b.ResetTimer()
     
     for n := 0; n < b.N; n++ {
         err = c.Bind().Body(d)
     }
     require.NoError(b, err)
     require.Equal(b, "john", d.Name)
+    
+    // Verify allocations are within expected range
+    if allocs := testing.AllocsPerRun(100, func() {
+        _ = c.Bind().Body(d)
+    }); allocs > 2 {
+        b.Errorf("too many allocations: %v", allocs)
+    }
 }
ctx_test.go (3)

3628-3629: Enhance error handling test coverage

The error test for complex numbers is good, but should be expanded to cover more error cases:

  1. Test encoding of channels (should error)
  2. Test encoding of functions (should error)
  3. Test encoding of recursive structures (should error)
  4. Test encoding of very large numbers that may overflow
// Test invalid types
err = c.CBOR(make(chan int))
require.Error(t, err)

err = c.CBOR(func() {})
require.Error(t, err)

// Test recursive struct
type Recursive struct {
    Next *Recursive
}
r := &Recursive{}
r.Next = r
err = c.CBOR(r)
require.Error(t, err)

3691-3712: Add comprehensive benchmark cases

The benchmark function is well-structured but could be expanded for more comprehensive testing:

b.Run("SimpleStruct", func(b *testing.B) {
    // Current benchmark code
})
b.Run("ComplexStruct", func(b *testing.B) {
    // Benchmark with a more complex struct
})
b.Run("LargeSlice", func(b *testing.B) {
    // Benchmark with a large slice of data
})

This would provide a more comprehensive view of the CBOR encoding performance across different data types and sizes.


3674-3678: Add comment explaining hex encoded test data

Add a comment explaining the expected hex string:

// Expected CBOR encoding for "random" is "72616e646f6d"
app := New(Config{
    CBOREncoder: func(_ any) ([]byte, error) {
        return []byte(hex.EncodeToString([]byte("random"))), nil
    },
})
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 697243b and 99f9893.

⛔ Files ignored due to path filters (1)
  • go.mod is excluded by !**/*.mod
📒 Files selected for processing (10)
  • app.go (3 hunks)
  • bind.go (2 hunks)
  • bind_test.go (4 hunks)
  • client/client.go (4 hunks)
  • client/client_test.go (3 hunks)
  • client/request_test.go (2 hunks)
  • ctx.go (1 hunks)
  • ctx_interface_gen.go (1 hunks)
  • ctx_test.go (2 hunks)
  • docs/whats_new.md (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
  • bind.go
  • client/client.go
  • ctx.go
  • ctx_interface_gen.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
bind_test.go

1762-1762: File is not gofumpt-ed with -extra

(gofumpt)

🔇 Additional comments (10)
app.go (3)

323-336: LGTM! Well-structured CBOR configuration fields.

The new CBOR encoder and decoder configuration fields are well-documented and follow the established pattern used for JSON and XML serialization.


28-28: Verify CBOR MIME type constant.

The CBOR implementation requires a MIME type constant. Let's verify its existence and value.

✅ Verification successful

CBOR MIME type constant is correctly defined and used

The MIME type constant for CBOR is properly defined as MIMEApplicationCBOR = "application/cbor" in constants.go and is appropriately used across the codebase:

  • In content type setting for responses (ctx.go)
  • In request parsing and binding (bind.go)
  • In test cases validating CBOR functionality (bind_test.go)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify CBOR MIME type constant

# Check for CBOR MIME type constant
echo "Checking for CBOR MIME type constant..."
rg "MIMEApplicationCBOR.*=.*\"application/cbor\"" --type go

# Check usage of CBOR MIME type
echo "Checking usage of CBOR MIME type..."
rg "MIMEApplicationCBOR" -A 2 -B 2 --type go

Length of output: 1823


554-559: LGTM! Verify CBOR encoder/decoder initialization.

The default initialization of CBOR encoder/decoder is correct and follows the same pattern as JSON/XML. Let's verify the implementation across the codebase.

✅ Verification successful

CBOR encoder/decoder initialization and implementation verified successfully

The verification confirms that:

  • CBOR implementation is consistent across the codebase with proper encoder/decoder initialization
  • All necessary CBOR methods are implemented where JSON/XML methods exist
  • Error handling follows the same pattern as other encoders/decoders
  • Default initialization uses cbor.Marshal and cbor.Unmarshal as expected
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify CBOR implementation consistency across the codebase

# Check for CBOR-related implementations
echo "Checking for CBOR-related implementations..."
rg -l "cbor\." --type go

# Check for potential missing CBOR implementations in binding
echo "Checking for potential missing CBOR implementations in binding..."
rg -l "func.*JSON|func.*XML" --type go | xargs rg -L "func.*CBOR"

# Check for consistent error handling pattern
echo "Checking for consistent error handling in CBOR operations..."
rg "cbor\." -A 5 -B 5 --type go

Length of output: 6696

client/request_test.go (1)

83-84: LGTM! Good type safety improvement.

The change from anonymous struct to named struct type improves type safety and code clarity.

client/client_test.go (3)

6-6: LGTM!

The addition of the encoding/hex import is necessary for CBOR test cases and follows the import ordering convention.


1474-1476: LGTM!

The context key implementation follows Go best practices by using an empty struct for memory efficiency.


229-254: ⚠️ Potential issue

Enhance CBOR test coverage with more comprehensive test cases.

The current test cases are too basic and need improvement:

  1. The success case only tests with CBOR null value (f6), which doesn't validate actual data encoding.
  2. Missing test for the default CBOR marshaling function.
  3. The error message is too generic and should be CBOR-specific.

Apply this diff to enhance the test coverage:

 t.Run("set cbor marshal", func(t *testing.T) {
     t.Parallel()
-    bs, err := hex.DecodeString("f6")
+    // Test complex CBOR data structure
+    // Map: {"foo": "bar", "array": [1, 2, 3]}
+    bs, err := hex.DecodeString("a263666f6f63626172656172726179830102")
     if err != nil {
         t.Error(err)
     }
     client := New().
         SetCBORMarshal(func(_ any) ([]byte, error) {
             return bs, nil
         })
     val, err := client.CBORMarshal()(nil)

     require.NoError(t, err)
     require.Equal(t, bs, val)
 })

+t.Run("default cbor marshal", func(t *testing.T) {
+    t.Parallel()
+    data := map[string]interface{}{
+        "foo": "bar",
+        "array": []int{1, 2, 3},
+    }
+    client := New()
+    val, err := client.CBORMarshal()(data)
+    
+    require.NoError(t, err)
+    require.NotNil(t, val)
+})

 t.Run("set cbor marshal error", func(t *testing.T) {
     t.Parallel()
     client := New().SetCBORMarshal(func(_ any) ([]byte, error) {
-        return nil, errors.New("invalid struct")
+        return nil, errors.New("cbor: cannot marshal cyclic structure")
     })

     val, err := client.CBORMarshal()(nil)
     require.Nil(t, val)
-    require.Equal(t, errors.New("invalid struct"), err)
+    require.Equal(t, errors.New("cbor: cannot marshal cyclic structure"), err)
 })
bind_test.go (3)

15-15: LGTM: Import of CBOR package

The import of github.com/fxamacker/cbor/v2 is appropriate as it's a well-maintained and widely used CBOR implementation for Go.


926-932: LGTM: Test helper function refactored to handle binary data

The testDecodeParser function has been correctly modified to accept []byte instead of string for the body parameter, which is essential for handling binary formats like CBOR.


1769-1769: ⚠️ Potential issue

Avoid converting CBOR binary data to string

CBOR data is binary and should not be converted to a string as it may lead to data corruption.

-    testDecodeParser(MIMEApplicationCBOR, string(cb))
+    testDecodeParser(MIMEApplicationCBOR, cb)

Likely invalid or redundant comment.

bind_test.go Outdated Show resolved Hide resolved
bind_test.go Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 5

🧹 Outside diff range and nitpick comments (7)
docs/api/bind.md (2)

232-234: Consider enhancing the CBOR description

While the description is clear, it would be helpful to briefly mention CBOR's benefits (e.g., compact binary format, efficient encoding/decoding) and its common use cases. This would help users understand when to choose CBOR over other formats.

Consider expanding the description like this:

-Binds the request CBOR body to a struct.
+Binds the request CBOR (Concise Binary Object Representation) body to a struct. CBOR is a binary data format that offers compact message sizes and efficient encoding/decoding, making it ideal for bandwidth-constrained scenarios or high-performance applications.

261-265: Enhance the testing section with developer-friendly tools

The current curl command uses raw CBOR hex data, which might be challenging for developers to generate manually. Consider adding information about tools that can help generate CBOR payloads.

Add a note about CBOR tools:

Run tests with the following `curl` command:

```bash
curl -X POST -H "Content-Type: application/cbor" --data "\xa2dnamedjohndpasscdoe" localhost:3000

+You can use tools like cbor.me (online CBOR playground) or the cbor2 Python package to generate CBOR payloads:
+
+python +import cbor2 +import binascii + +# Generate CBOR payload +data = {"name": "john", "pass": "doe"} +cbor_bytes = cbor2.dumps(data) +hex_string = binascii.hexlify(cbor_bytes).decode() +print(f"\\x{hex_string}") # Use this in the curl command +


</blockquote></details>
<details>
<summary>docs/client/rest.md (2)</summary><blockquote>

`84-85`: **Replace hard tab with spaces**

The indentation on line 85 uses a hard tab instead of spaces, which is inconsistent with the rest of the file.

```diff
-	  cborUnmarshal utils.CBORUnmarshal
+    cborUnmarshal utils.CBORUnmarshal
🧰 Tools
🪛 Markdownlint (0.35.0)

85-85: Column: 1
Hard tabs

(MD010, no-hard-tabs)


319-351: Enhance CBOR documentation section

While the CBOR documentation follows the established structure, it could be improved in several ways:

  1. Add return type documentation for each method
  2. Include usage examples similar to other sections
  3. Provide more detailed descriptions explaining the purpose and common use cases

Here's a suggested enhancement for the CBORMarshal method (apply similar pattern to other methods):

 ### CBOR

 #### CBORMarshal

-CBORMarshal returns CBOR marshal function in Core.
+CBORMarshal returns the CBOR marshal function used by the client for encoding data into CBOR format.
+
+Returns:
+- `utils.CBORMarshal`: A function that takes an interface{} and returns ([]byte, error)

 ```go title="Signature"
 func (c *Client) CBORMarshal() utils.CBORMarshal

+```go title="Example"
+cc := client.New()
+
+// Get the current CBOR marshal function
+marshal := cc.CBORMarshal()
+
+// Use it to encode data
+data := map[string]interface{}{

  • "name": "john",
  • "age": 30,
    +}

+encoded, err := marshal(data)
+if err != nil {

  • panic(err)
    +}
    +```

</blockquote></details>
<details>
<summary>client/client.go (1)</summary><blockquote>

Line range hint `1-800`: **Consider adding performance benchmarks for CBOR operations**

Since CBOR is often chosen for its performance benefits over JSON, it would be valuable to add benchmarks comparing CBOR vs JSON serialization/deserialization performance.


Would you like me to help create benchmark tests for comparing CBOR and JSON performance?

</blockquote></details>
<details>
<summary>docs/api/ctx.md (1)</summary><blockquote>

`929-930`: **Add context about CBOR benefits and use cases**

Consider adding a brief explanation of CBOR's benefits and common use cases to help users understand when to use CBOR over other encoding formats like JSON or XML.

Add a note like:

```diff
+CBOR (Concise Binary Object Representation) is a binary data format that offers smaller message sizes and faster processing compared to JSON. It's particularly useful for IoT devices, real-time applications, and scenarios where bandwidth or processing power is limited.
+
 CBOR converts any interface or string to cbor encoded bytes.
ctx_test.go (1)

3624-3684: Enhance error handling and test coverage

The main CBOR test function looks good overall, but could be improved:

  1. The error from cbor.Marshal on line 3682 is not checked
  2. The test coverage could be expanded to include more edge cases

Consider adding these test cases:

// Test recursive struct
type Recursive struct {
    Next *Recursive
}
r := &Recursive{}
r.Next = r
err = c.CBOR(r)
require.Error(t, err)

// Test very large numbers
err = c.CBOR(uint64(math.MaxUint64))
require.NoError(t, err)
🧰 Tools
🪛 golangci-lint (1.62.2)

3682-3682: Error return value of cbor.Marshal is not checked

(errcheck)

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 99f9893 and 190704a.

📒 Files selected for processing (7)
  • client/client.go (4 hunks)
  • ctx_test.go (3 hunks)
  • docs/api/bind.md (2 hunks)
  • docs/api/ctx.md (1 hunks)
  • docs/client/request.md (1 hunks)
  • docs/client/response.md (1 hunks)
  • docs/client/rest.md (2 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ctx_test.go

3682-3682: Error return value of cbor.Marshal is not checked

(errcheck)

🪛 Markdownlint (0.35.0)
docs/client/rest.md

85-85: Column: 1
Hard tabs

(MD010, no-hard-tabs)

🔇 Additional comments (8)
docs/api/bind.md (1)

23-23: LGTM!

The CBOR entry is correctly added to the table of contents, maintaining consistency with other binding methods.

client/client.go (4)

16-16: LGTM: Appropriate import for CBOR support

The import of github.com/fxamacker/cbor/v2 is correct as it's a well-maintained and secure CBOR implementation for Go.


48-49: Initialize CBOR marshal/unmarshal functions in New() constructor

The CBOR marshal/unmarshal fields are added but not initialized in the New() constructor, which could lead to nil pointer dereferences when these functions are called.


156-176: LGTM: Well-structured CBOR getter/setter methods

The implementation follows the established pattern in the codebase:

  • Consistent method naming
  • Proper method documentation
  • Thread-safe implementation (using the existing mutex)
  • Fluent interface pattern maintained

734-735: Verify CBOR implementation in request/response handling

The CBOR functions are correctly initialized, but we should verify their integration with the request/response cycle.

✅ Verification successful

Let me gather more specific information about the CBOR implementation.


CBOR implementation is properly integrated in request/response handling

The verification shows a complete and consistent CBOR implementation:

  • Content type application/cbor is properly defined and used
  • Request handling includes SetCBOR method with proper body type setting
  • Response handling includes CBOR method for unmarshaling responses
  • CBOR marshal/unmarshal functions are correctly initialized and used throughout the codebase
  • Full test coverage exists for CBOR functionality in requests, responses, and bindings
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if CBOR request/response handling is implemented consistently

# Check for CBOR content type handling
echo "Checking for CBOR content type handling..."
rg -l "application/cbor"

# Check for CBOR body type handling
echo "Checking for CBOR body type handling..."
ast-grep --pattern 'type bodyType = $$$'

# Check for SetCBOR method implementation
echo "Checking for SetCBOR method..."
ast-grep --pattern 'func $$(r *Request) SetCBOR(v any) *Request'

# Check for CBOR response handling
echo "Checking for CBOR response handling..."
ast-grep --pattern 'func $$(r *Response) CBOR(v any) error'

Length of output: 663


Script:

#!/bin/bash
# Check CBOR content type constant definition and usage
echo "Checking CBOR content type constant..."
rg "MIMEApplicationCBOR" -B 2 -A 2

# Check CBOR request handling implementation
echo "Checking CBOR request handling..."
rg "SetCBOR" -B 2 -A 5

# Check CBOR response handling implementation
echo "Checking CBOR response handling..."
rg "func.*CBOR" -B 2 -A 5

# Check CBOR marshal/unmarshal usage
echo "Checking CBOR marshal/unmarshal usage..."
rg "cbor\.(Un)?Marshal" -B 2 -A 2

Length of output: 12868

docs/api/ctx.md (1)

939-973: LGTM! Well-structured examples

The examples are clear, comprehensive, and follow the established documentation pattern. They demonstrate both struct and map usage, include struct tags, and show the binary output representation.

ctx_test.go (2)

3685-3703: LGTM! Well-structured custom encoder test

The custom encoder test is well-implemented with good validation of both the response body and content type header.


3705-3726: LGTM! Comprehensive benchmark implementation

The benchmark function follows Go best practices:

  • Uses b.ReportAllocs() to track allocations
  • Properly resets timer
  • Validates results after benchmarking

docs/client/response.md Outdated Show resolved Hide resolved
docs/client/request.md Outdated Show resolved Hide resolved
docs/api/ctx.md Outdated Show resolved Hide resolved
ctx_test.go Outdated Show resolved Hide resolved
@efectn efectn requested a review from gaby November 29, 2024 12:34
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (5)
bind_test.go (1)

1111-1138: Improve error handling in the benchmark.

While the benchmark is well-structured, the error handling could be more robust.

Consider this improvement:

-	body, err := cbor.Marshal(&Demo{Name: "john"})
-	if err != nil {
-		b.Error(err)
-	}
+	body, err := cbor.Marshal(&Demo{Name: "john"})
+	require.NoError(b, err, "Failed to marshal CBOR data")
docs/api/ctx.md (2)

927-933: Add more context about CBOR benefits and use cases.

Consider enhancing the introduction by explaining:

  • CBOR's advantages over JSON (smaller payload size, faster processing)
  • Common use cases (IoT, real-time applications)
  • Link to RFC 8949 for reference
 ## CBOR
 
-CBOR converts any interface or string to cbor encoded bytes.
+CBOR converts any interface or string to CBOR (Concise Binary Object Representation) encoded bytes. CBOR is a binary data format that offers smaller payload sizes and faster processing compared to JSON, making it ideal for IoT devices and real-time applications. This implementation follows RFC 8949.

939-973: Improve example readability and coverage.

The examples could be enhanced by:

  1. Adding comments showing the decoded CBOR output for better readability
  2. Including more diverse use cases (arrays, nested structures)
  3. Demonstrating error handling
 app.Get("/cbor", func(c fiber.Ctx) error {
   // Create data struct:
   data := SomeStruct{
     Name: "Grame",
     Age:  20,
   }
 
   return c.CBOR(data)
   // => Content-Type: application/cbor
-  // => \xa2dnameeGramecage\x14
+  // => Binary: \xa2dnameeGramecage\x14
+  // => Decoded: {"name": "Grame", "age": 20}
 
+  // Array example
+  return c.CBOR([]string{"hello", "world"})
+  // => Content-Type: application/cbor
+  // => Decoded: ["hello", "world"]
+
+  // Nested structure example
+  return c.CBOR(fiber.Map{
+    "user": fiber.Map{
+      "name": "Grame",
+      "scores": []int{10, 20, 30},
+    },
+  })
+  // => Content-Type: application/cbor
+  // => Decoded: {"user": {"name": "Grame", "scores": [10, 20, 30]}}
+
+  // Error handling example
+  if err := c.CBOR(data); err != nil {
+    return c.Status(fiber.StatusInternalServerError).SendString("Failed to encode CBOR")
+  }
ctx_test.go (2)

3671-3675: Remove unused struct definition

The SomeStruct type is defined but never used in the test cases.

-  type SomeStruct struct {
-    Name string `cbor:"name"`
-    Pass string `cbor:"pass"`
-  }
🧰 Tools
🪛 golangci-lint (1.62.2)

3671-3671: type SomeStruct is unused

(unused)


3628-3629: Consider adding more error test cases

While the error testing for complex numbers is good, consider adding tests for:

  1. Channels (should error)
  2. Functions (should error)
  3. Recursive structures (should error)
  4. Very large numbers that may overflow
// Test invalid types
err = c.CBOR(make(chan int))
require.Error(t, err)

err = c.CBOR(func() {})
require.Error(t, err)

// Test recursive struct
type Recursive struct {
    Next *Recursive
}
r := &Recursive{}
r.Next = r
err = c.CBOR(r)
require.Error(t, err)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 190704a and e397dc2.

📒 Files selected for processing (4)
  • bind_test.go (4 hunks)
  • ctx_test.go (2 hunks)
  • docs/api/ctx.md (1 hunks)
  • docs/client/request.md (1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
ctx_test.go

3671-3671: type SomeStruct is unused

(unused)

🔇 Additional comments (5)
bind_test.go (3)

15-15: LGTM! Good choice of CBOR library.

The fxamacker/cbor is a well-maintained and secure CBOR implementation for Go.


940-945: LGTM! Well-structured CBOR test case.

The test case properly validates CBOR encoding/decoding functionality and correctly handles binary data.


1760-1765: ⚠️ Potential issue

Fix binary data handling in CBOR test.

Converting CBOR binary data to string can lead to data corruption. The test should handle CBOR data as bytes.

Apply this fix:

-	testDecodeParser(MIMEApplicationCBOR, string(cb))
+	testDecodeParser(MIMEApplicationCBOR, cb)
ctx_test.go (2)

3622-3694: Well-structured CBOR test implementation

The test cases are comprehensive and well-organized, covering:

  • Basic CBOR encoding
  • Error handling for complex types
  • Empty value handling
  • Custom content types
  • Custom encoder functionality
🧰 Tools
🪛 golangci-lint (1.62.2)

3671-3671: type SomeStruct is unused

(unused)


3696-3717: LGTM: Well-structured benchmark implementation

The benchmark implementation follows best practices:

  • Uses realistic test data
  • Properly tracks allocations
  • Validates results
  • Has clear error handling

docs/client/request.md Outdated Show resolved Hide resolved
bind_test.go Show resolved Hide resolved
binder/cbor.go Show resolved Hide resolved
docs/api/bind.md Outdated Show resolved Hide resolved
docs/api/ctx.md Outdated Show resolved Hide resolved
docs/client/response.md Outdated Show resolved Hide resolved
docs/whats_new.md Outdated Show resolved Hide resolved
docs/whats_new.md Outdated Show resolved Hide resolved
docs/client/request.md Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (6)
client/request_test.go (2)

83-84: Consider simplifying the context key definition.

The change to use a dedicated struct type for context keys improves type safety. However, you could simplify it further:

-type ctxKey struct{}
-var key ctxKey = struct{}{}
+type ctxKey struct{}
+var key ctxKey

This achieves the same result with less code while maintaining type safety.

Also applies to: 88-88


1010-1027: Consider enhancing CBOR test coverage.

The test case effectively verifies the CBOR encoding and content type. However, consider adding server-side CBOR decoding to ensure the data can be correctly deserialized:

 func(c fiber.Ctx) error {
     require.Equal(t, "application/cbor", string(c.Request().Header.ContentType()))
+    var data struct {
+        Content string `cbor:"content"`
+    }
+    err := c.BodyParser(&data)
+    require.NoError(t, err)
+    require.Equal(t, "hello", data.Content)
     return c.SendString(string(c.Request().Body()))
 },

This addition would verify the full request-response cycle including CBOR deserialization.

client/client_test.go (1)

1475-1479: Consider optimizing context key definition.

The context key could be improved for better reusability and performance:

-type ctxKey struct{}
-var key ctxKey = struct{}{}
+// Define as package-level variable for reuse across tests
+var testCtxKey = struct{}{}

 ctx := context.Background()
-ctx = context.WithValue(ctx, key, "v1")
+ctx = context.WithValue(ctx, testCtxKey, "v1")
ctx_test.go (3)

3622-3670: Test coverage for CBOR encoding looks good but could be enhanced

The test cases cover essential scenarios including error handling, basic encoding, content type, and empty values. However, consider adding:

  1. Test cases for more complex data structures (nested structs, maps, slices)
  2. Test cases for concurrent encoding
  3. Test cases for large payloads to verify performance characteristics
// Add test cases for nested structures
type nestedStruct struct {
    User dummyStruct
    Meta map[string]interface{}
    Tags []string
}

3671-3689: Custom CBOR encoder test could be more comprehensive

The current test for custom encoder is good but could be enhanced to:

  1. Test error scenarios in custom encoder
  2. Verify encoder is actually used by checking the encoded format
  3. Test with different payload sizes
// Add error case test
app := New(Config{
    CBOREncoder: func(_ any) ([]byte, error) {
        return nil, errors.New("custom error")
    },
})

3691-3712: Benchmark implementation looks good but could be expanded

The benchmark covers basic CBOR encoding performance. Consider adding:

  1. Benchmarks with different payload sizes
  2. Benchmarks for concurrent encoding
  3. Comparison benchmarks with other encoding formats (JSON, XML)
// Add benchmark for large payload
data := SomeStruct{
    Name: strings.Repeat("x", 1024*1024), // 1MB string
    Age:  20,
}
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between e397dc2 and a1f2dff.

📒 Files selected for processing (12)
  • bind_test.go (4 hunks)
  • client/client_test.go (3 hunks)
  • client/request_test.go (2 hunks)
  • client/response.go (1 hunks)
  • ctx.go (1 hunks)
  • ctx_interface_gen.go (1 hunks)
  • ctx_test.go (2 hunks)
  • docs/api/bind.md (2 hunks)
  • docs/api/ctx.md (1 hunks)
  • docs/client/request.md (1 hunks)
  • docs/client/response.md (1 hunks)
  • docs/whats_new.md (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • client/response.go
  • ctx.go
  • ctx_interface_gen.go
  • docs/api/bind.md
  • docs/api/ctx.md
  • docs/client/response.md
🔇 Additional comments (6)
client/client_test.go (1)

229-254: ⚠️ Potential issue

Enhance CBOR test coverage with more comprehensive test cases.

The current test cases need improvement in several areas:

  1. The successful test case only tests CBOR's "null" value (f6).
  2. The error message is too generic and doesn't indicate CBOR-specific issues.
  3. Missing test for default CBOR marshaling.

Apply this diff to enhance the test coverage:

 t.Run("set cbor marshal", func(t *testing.T) {
     t.Parallel()
-    bs, err := hex.DecodeString("f6")
+    // Test complex CBOR data structure
+    // Map: {"foo": "bar", "array": [1, 2, 3]}
+    bs, err := hex.DecodeString("a263666f6f636261726561727261798203010203")
     if err != nil {
         t.Error(err)
     }
     client := New().
         SetCBORMarshal(func(_ any) ([]byte, error) {
             return bs, nil
         })
     val, err := client.CBORMarshal()(nil)

     require.NoError(t, err)
     require.Equal(t, bs, val)
 })

 t.Run("set cbor marshal error", func(t *testing.T) {
     t.Parallel()
     client := New().SetCBORMarshal(func(_ any) ([]byte, error) {
-        return nil, errors.New("invalid struct")
+        return nil, errors.New("cbor: cannot marshal cyclic structure")
     })

     val, err := client.CBORMarshal()(nil)
     require.Nil(t, val)
-    require.Equal(t, errors.New("invalid struct"), err)
+    require.Equal(t, errors.New("cbor: cannot marshal cyclic structure"), err)
 })

+t.Run("default cbor marshal", func(t *testing.T) {
+    t.Parallel()
+    client := New()
+    data := map[string]interface{}{
+        "foo": "bar",
+        "array": []int{1, 2, 3},
+    }
+    val, err := client.CBORMarshal()(data)
+    require.NoError(t, err)
+    require.NotNil(t, val)
+
+    // Verify the encoded CBOR data matches the expected structure
+    expected, _ := hex.DecodeString("a263666f6f636261726561727261798203010203")
+    require.Equal(t, expected, val)
+})
bind_test.go (5)

15-15: LGTM: CBOR library import

The import of the CBOR library is correctly specified with version pinning.


947-955: LGTM: Comprehensive error testing

Good inclusion of negative test cases for invalid CBOR data. The test properly verifies error handling for malformed input.


941-944: 🛠️ Refactor suggestion

Improve error handling in CBOR test case

The error handling in the CBOR marshaling could be more robust.

-enc, err := cbor.Marshal(&Demo{Name: "john"})
-if err != nil {
-    t.Error(err)
-}
+enc, err := cbor.Marshal(&Demo{Name: "john"})
+require.NoError(t, err, "Failed to marshal CBOR data")

Likely invalid or redundant comment.


1131-1134: 🛠️ Refactor suggestion

Improve error handling in benchmark setup

The error handling in the benchmark setup could be more robust.

-body, err := cbor.Marshal(&Demo{Name: "john"})
-if err != nil {
-    b.Error(err)
-}
+body, err := cbor.Marshal(&Demo{Name: "john"})
+require.NoError(b, err, "Failed to marshal CBOR data in benchmark setup")

Likely invalid or redundant comment.


1770-1775: ⚠️ Potential issue

Fix potential binary data corruption

Converting CBOR binary data to string and back could lead to data corruption. The testDecodeParser function should be used with the raw byte slice.

-testDecodeParser(MIMEApplicationCBOR, string(cb))
+testDecodeParser(MIMEApplicationCBOR, cb)

Likely invalid or redundant comment.

docs/client/request.md Outdated Show resolved Hide resolved
docs/whats_new.md Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (1)
docs/client/rest.md (1)

84-85: Fix indentation to match surrounding fields

The indentation of the new CBOR-related fields doesn't match the surrounding fields. Adjust the spacing to maintain consistency with other marshal/unmarshal fields.

-    cborMarshal   utils.CBORMarshal
-    cborUnmarshal utils.CBORUnmarshal
+    cborMarshal    utils.CBORMarshal
+    cborUnmarshal  utils.CBORUnmarshal
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a1f2dff and ef215f3.

📒 Files selected for processing (2)
  • docs/client/request.md (1 hunks)
  • docs/client/rest.md (2 hunks)

docs/client/rest.md Show resolved Hide resolved
docs/client/request.md Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between ef215f3 and 602ae7a.

📒 Files selected for processing (1)
  • binder/README.md (1 hunks)
🔇 Additional comments (1)
binder/README.md (1)

25-25: LGTM! Addition of CBOR to default binders list.

The addition of CBOR to the default binders list is consistent with the PR objectives.

@@ -22,6 +22,7 @@ Fiber provides several default binders out of the box:
- [Cookie](cookie.go)
- [JSON](json.go)
- [XML](xml.go)
- [CBOR](cbor.go)
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Documentation needs CBOR usage examples.

To maintain consistency with other binder documentation, please add CBOR usage examples similar to the existing JSON and XML examples. This should include:

  1. Example struct with CBOR tags
  2. Example handler using CBOR binding
  3. Curl command demonstrating CBOR request

Here's a suggested addition after the existing examples:

 // XML
 curl -X POST -H "Content-Type: application/xml" --data "<login><name>john</name><pass>doe</pass></login>" localhost:3000
+
+// CBOR
+curl -X POST -H "Content-Type: application/cbor" --data-binary @person.cbor localhost:3000

Also consider adding a complete CBOR example:

### CBOR Binding Example

```go
type Person struct {
    Name string `cbor:"name" json:"name"`
    Pass string `cbor:"pass" json:"pass"`
}

app.Post("/", func(c fiber.Ctx) error {
    p := new(Person)

    if err := c.Bind().Body(p); err != nil {
        return err
    }

    // Additional logic...
    return c.CBOR(p)
})

You can test this endpoint using a tool that supports CBOR, or using Python with the cbor2 library:

import cbor2
import requests

data = {'name': 'john', 'pass': 'doe'}
cbor_data = cbor2.dumps(data)

response = requests.post(
    'http://localhost:3000',
    headers={'Content-Type': 'application/cbor'},
    data=cbor_data
)

---

_:hammer_and_wrench: Refactor suggestion_

**Document new methods mentioned in the AI summary.**

The AI summary mentions several new methods that should be documented:
- `CBOR()` method in the `Bind` struct
- Updated `Body()` method with CBOR support


Consider adding these to the documentation:

```markdown
### CBOR Binding

The `CBOR()` method allows direct binding of CBOR-encoded request bodies:

```go
if err := c.Bind().CBOR(&data); err != nil {
    return err
}

The Body() method automatically handles CBOR content when the Content-Type header is set to application/cbor.


<!-- This is an auto-generated comment by CodeRabbit -->

Copy link
Member

@gaby gaby left a comment

Choose a reason for hiding this comment

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

👍 LGTM

app.go Show resolved Hide resolved
Copy link
Member

@ReneWerner87 ReneWerner87 left a comment

Choose a reason for hiding this comment

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

only the 2 tasks for the docs are missing

@ReneWerner87
Copy link
Member

iam on the last changes

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (21)
docs/api/fiber.md (2)

67-68: Enhance CBOR configuration documentation with examples

The CBOR encoder/decoder configuration documentation would benefit from including usage examples, similar to how JSON and XML configurations are documented elsewhere.

Consider adding an example section:

| <Reference id="cborencoder">CBOREncoder</Reference> | `utils.CBORMarshal` | Allowing for flexibility in using another cbor library for encoding. | `cbor.Marshal` |
| <Reference id="cbordecoder">CBORDecoder</Reference> | `utils.CBORUnmarshal` | Allowing for flexibility in using another cbor library for decoding. | `cbor.Unmarshal` |

+```go title="Example"
+// Custom CBOR encoder/decoder
+app := fiber.New(fiber.Config{
+    CBOREncoder: customCBORMarshal,
+    CBORDecoder: customCBORUnmarshal,
+})
+```

47-47: Fix grammatical issues in documentation

Several grammatical issues need to be addressed to improve documentation quality:

Apply these corrections:

-| <Reference id="appname">AppName</Reference> | `string` | This allows to setup app name for the app | `""` |
+| <Reference id="appname">AppName</Reference> | `string` | This allows you to set up the app name | `""` |

-By default `c.IP()`will return
+By default, `c.IP()` will return

-Fiber doesn't have default validator,
+Fiber doesn't have a default validator,

-before setting the path for the context, so that
+before setting the path for the context so that

Also applies to: 70-70, 78-78, 80-80

🧰 Tools
🪛 LanguageTool

[grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space.
Context: ... | This allows to setup app name for the app ...

(NOUN_VERB_CONFUSION)

docs/api/constants.md (2)

76-76: Consider removing the unused status code

The StatusSwitchProxy (306) is marked as unused in RFC 9110. Including unused status codes in the API could lead to confusion. Consider removing this constant unless there's a specific requirement for historical completeness.


295-296: LGTM: Private Network Access headers

The new headers HeaderAccessControlAllowPrivateNetwork and HeaderAccessControlRequestPrivateNetwork are correctly defined and follow the naming convention. These headers are part of the Private Network Access specification, enabling CORS-like controls for private network requests.

These headers are particularly useful when your application needs to:

  • Make requests to private network endpoints (e.g., local devices, intranet services)
  • Control access to private network resources from public websites
binder/xml.go (4)

8-9: Enhance type documentation for better clarity.

Consider expanding the documentation to provide more context and follow godoc conventions:

-// xmlBinding is the XML binder for XML request body.
+// xmlBinding implements the Binder interface for XML request bodies.
+// It provides functionality to unmarshal XML data into Go structs.
+//
+// Example usage:
+//
+//	var data MyStruct
+//	binder := &xmlBinding{}
+//	err := binder.Bind(body, &data)

11-14: Enhance method documentation.

Consider expanding the documentation to be more descriptive:

-// Name returns the binding name.
+// Name returns the name of the binding implementation ("xml").
+// This method satisfies the Binder interface requirement.

Line range hint 16-23: Add input validation and enhance error handling.

The current implementation could benefit from several improvements:

-// Bind parses the request body as XML and returns the result.
+// Bind unmarshals the XML request body into the provided output object.
+// It returns an error if the body is empty or if unmarshaling fails.
+//
+// The out parameter must be a pointer to the target struct.
 func (*xmlBinding) Bind(body []byte, out any) error {
+	if len(body) == 0 {
+		return fmt.Errorf("empty request body")
+	}
+
+	if out == nil {
+		return fmt.Errorf("output parameter cannot be nil")
+	}
+
 	if err := xml.Unmarshal(body, out); err != nil {
-		return fmt.Errorf("failed to unmarshal xml: %w", err)
+		return fmt.Errorf("failed to unmarshal XML body into %T: %w", out, err)
 	}
 
 	return nil
}

Line range hint 1-23: Consider implementing a parallel CBOR binding structure.

Since this PR aims to add CBOR support, consider creating a parallel implementation following the same pattern:

  • Create a new file binder/cbor.go
  • Implement cborBinding struct with Name() and Bind() methods
  • Follow the same error handling and validation patterns

This will maintain consistency across different binding implementations.

Would you like me to help generate the CBOR binding implementation?

bind_test.go (2)

1121-1148: Improve error handling in CBOR benchmark

While the benchmark follows the established pattern, the error handling could be more robust.

-	body, err := cbor.Marshal(&Demo{Name: "john"})
-	if err != nil {
-		b.Error(err)
-	}
+	body, err := cbor.Marshal(&Demo{Name: "john"})
+	require.NoError(b, err, "Failed to marshal CBOR data")

940-946: Consider adding more CBOR-specific test cases

While the basic CBOR functionality is tested, consider adding test cases for:

  • Complex nested structures
  • Arrays and maps
  • Different CBOR data types (integers, floats, strings, etc.)
binder/cookie.go (1)

11-12: Enhance type documentation

While the documentation follows Go conventions, it would be more helpful to include:

  • Example usage
  • Description of the binding behavior
  • Any specific limitations or considerations

Consider expanding the documentation like this:

-// cookieBinding is the cookie binder for cookie request body.
+// cookieBinding implements request binding for HTTP cookies.
+// It parses cookies from the request and binds them to struct fields,
+// supporting both single values and comma-separated lists for slice fields.
binder/header.go (3)

11-12: Enhance struct documentation

While the documentation is clear, it could be more descriptive by explaining:

  • The purpose of header binding
  • How it's used in the request processing pipeline
  • Any specific behaviors or limitations

Consider expanding the documentation:

-// headerBinding is the header binder for header request body.
+// headerBinding implements the binding interface for HTTP headers.
+// It allows binding HTTP request headers into Go structs, supporting
+// both single values and comma-separated values for slice fields.

14-17: LGTM with minor documentation enhancement

The implementation is correct, but consider enhancing the documentation to explain that this method satisfies part of the binding interface contract.

-// Name returns the binding name.
+// Name returns the binding name "header". This method implements part
+// of the binding interface contract.

Line range hint 26-31: Optimize comma-separated value handling

The current implementation creates intermediate strings through strings.Split. Consider using a more efficient approach for large headers.

-		if strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) {
-			values := strings.Split(v, ",")
-			for i := 0; i < len(values); i++ {
-				data[k] = append(data[k], values[i])
+		if equalFieldType(out, reflect.Slice, k) {
+			start := 0
+			for i := 0; i < len(v); i++ {
+				if v[i] == ',' || i == len(v)-1 {
+					end := i
+					if i == len(v)-1 && v[i] != ',' {
+						end = i + 1
+					}
+					data[k] = append(data[k], strings.TrimSpace(v[start:end]))
+					start = i + 1
+				}
binder/uri.go (1)

6-9: Consider using a value receiver instead of a pointer receiver.

Since Name() doesn't modify the receiver and uriBinding is an empty struct, using a value receiver would be more idiomatic and slightly more efficient.

-func (*uriBinding) Name() string {
+func (uriBinding) Name() string {
binder/resp_header.go (2)

11-12: Enhance struct documentation for clarity.

The documentation comment could be more descriptive about the purpose and usage of this binder. Consider expanding it to explain its role in the response header binding process.

-// respHeaderBinding is the respHeader binder for response header.
+// respHeaderBinding implements a binder for HTTP response headers, allowing
+// structured binding of response header values into Go types.

Line range hint 22-36: Consider performance optimizations and error handling improvements.

The current implementation has several areas that could be improved:

  1. Memory allocation: The method allocates a new map and performs multiple string conversions for each call.
  2. Error handling: The method doesn't handle potential errors from the parse function or invalid header values.
  3. Type safety: The reflection-based type checking could be more robust.

Consider these improvements:

 func (b *respHeaderBinding) Bind(resp *fasthttp.Response, out any) error {
+	if resp == nil {
+		return ErrBindingNilResponse
+	}
 	data := make(map[string][]string)
 	resp.Header.VisitAll(func(key, val []byte) {
 		k := utils.UnsafeString(key)
 		v := utils.UnsafeString(val)
 
-		if strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) {
+		// Pre-check if the field exists and is a slice before splitting
+		if ok, isSlice := checkField(out, k); ok && isSlice && strings.Contains(v, ",") {
 			values := strings.Split(v, ",")
+			// Pre-allocate slice capacity
+			data[k] = make([]string, 0, len(values))
 			for i := 0; i < len(values); i++ {
-				data[k] = append(data[k], values[i])
+				data[k] = append(data[k], strings.TrimSpace(values[i]))
 			}
 		} else {
 			data[k] = append(data[k], v)
 		}
 	})
 
-	return parse(b.Name(), out, data)
+	if err := parse(b.Name(), out, data); err != nil {
+		return fmt.Errorf("failed to parse response headers: %w", err)
+	}
+	return nil
 }

Additionally, consider adding a new helper function for field checking:

// checkField returns whether the field exists and is a slice
func checkField(out any, field string) (exists bool, isSlice bool) {
	v := reflect.ValueOf(out)
	if v.Kind() != reflect.Ptr || v.IsNil() {
		return false, false
	}
	v = v.Elem()
	if v.Kind() != reflect.Struct {
		return false, false
	}
	f := v.FieldByName(field)
	return f.IsValid(), f.Kind() == reflect.Slice
}
binder/query.go (4)

11-12: Enhance type documentation

While the documentation is clear, consider adding more details about:

  • The role of this binder in the binding system
  • How it relates to other binders
  • Any specific query parameter formats it supports
-// queryBinding is the query binder for query request body.
+// queryBinding implements the Binder interface for HTTP query parameters.
+// It supports parsing both simple key-value pairs and complex structures
+// using square bracket notation (e.g., user[name]=john) and comma-separated
+// values for slices.

Line range hint 19-46: Consider improving error handling and security

The Bind method has several areas that could be enhanced:

  1. Error handling is generic - consider adding specific error types for different failure cases
  2. No validation of query parameter size or complexity
  3. Using unsafe string conversions without bounds checking

Consider these improvements:

 func (b *queryBinding) Bind(reqCtx *fasthttp.RequestCtx, out any) error {
+    // Validate input size
+    if reqCtx.QueryArgs().Len() > maxQueryParams {
+        return ErrTooManyParameters
+    }
+
     data := make(map[string][]string)
-    var err error
+    var bindErr error
 
     reqCtx.QueryArgs().VisitAll(func(key, val []byte) {
-        if err != nil {
+        if bindErr != nil {
             return
         }
 
+        // Validate key/value sizes
+        if len(key) > maxKeyLength || len(val) > maxValueLength {
+            bindErr = ErrParameterTooLong
+            return
+        }
+
         k := utils.UnsafeString(key)
         v := utils.UnsafeString(val)
 
         if strings.Contains(k, "[") {
-            k, err = parseParamSquareBrackets(k)
+            k, bindErr = parseParamSquareBrackets(k)
         }

Line range hint 22-23: Optimize memory allocation

The initial map allocation could be more efficient by pre-allocating with an estimated size based on the number of query parameters.

-    data := make(map[string][]string)
+    data := make(map[string][]string, reqCtx.QueryArgs().Len())

Based on the code analysis, I can now provide a final response:

Add bounds checking for comma-separated values

The current implementation lacks any validation for the size of comma-separated values in query parameters. Specifically:

  • No limit on the number of values after splitting by comma
  • No validation of individual value lengths
  • No protection against memory exhaustion from large inputs

Suggested improvements:

  • Add a maximum limit for the number of values in the comma-separated list
  • Validate individual value lengths before appending to the slice
  • Consider adding a total size limit for all values combined
🔗 Analysis chain

Line range hint 33-39: Add validation for slice values

The comma-separated value handling could benefit from additional validation and bounds checking.

Let's verify if there are any existing size limits or validation in the codebase:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any existing parameter validation or size limits
rg -A 3 "maxQueryParams|maxKeyLength|maxValueLength" 

Length of output: 54


Script:

#!/bin/bash
# Let's search for any validation patterns in the query parsing logic
rg -A 5 "parseParam|validateParam|checkParam|validate.*query" 

# Also search for any length or size checks in the query handling
rg -A 5 "len\(.*\).*>|size.*limit|max.*size" 

Length of output: 64529


Script:

#!/bin/bash
# Let's search for any existing validation or bounds checking in the query binding logic
ast-grep --pattern 'func equalFieldType($_, $_, $_) {
  $$$
}'

# Also search for any validation in the parseParamSquareBrackets function
ast-grep --pattern 'func parseParamSquareBrackets($_, $_) {
  $$$
}'

Length of output: 135


Script:

#!/bin/bash
# Let's look for the actual implementation of these functions
rg -A 10 "^func equalFieldType|^func parseParamSquareBrackets" binder/

# Also search for any size or length validation patterns in the binder package
rg -A 5 "maxLength|maxSize|validate|check.*length" binder/

Length of output: 1117

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 602ae7a and fc1779b.

📒 Files selected for processing (15)
  • bind.go (3 hunks)
  • bind_test.go (8 hunks)
  • binder/cbor.go (1 hunks)
  • binder/cookie.go (1 hunks)
  • binder/form.go (2 hunks)
  • binder/header.go (1 hunks)
  • binder/json.go (1 hunks)
  • binder/mapping.go (2 hunks)
  • binder/query.go (1 hunks)
  • binder/resp_header.go (1 hunks)
  • binder/uri.go (1 hunks)
  • binder/xml.go (1 hunks)
  • client/request.go (2 hunks)
  • docs/api/constants.md (3 hunks)
  • docs/api/fiber.md (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • binder/mapping.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • client/request.go
  • bind.go
  • binder/cbor.go
🧰 Additional context used
🪛 LanguageTool
docs/api/fiber.md

[grammar] ~47-~47: The word “setup” is a noun. The verb is spelled with a space.
Context: ... | This allows to setup app name for the app ...

(NOUN_VERB_CONFUSION)


[uncategorized] ~70-~70: Did you mean: “By default,”?
Context: ...turn the value of the given header key. By default c.IP()will return the Remote IP from ...

(BY_DEFAULT_COMMA)


[uncategorized] ~78-~78: Possible missing article found.
Context: ...ne struct validator. Fiber doesn't have default validator, so it'll skip validator step...

(AI_HYDRA_LEO_MISSING_A)


[uncategorized] ~78-~78: Possible missing article found.
Context: ...t have default validator, so it'll skip validator step if you don't use any validator. ...

(AI_HYDRA_LEO_MISSING_THE)


[typographical] ~80-~80: The conjunction “so that” does not require a comma.
Context: ... before setting the path for the context, so that the routing can also work with URL enco...

(SO_THAT_UNNECESSARY_COMMA)

🔇 Additional comments (14)
docs/api/constants.md (1)

36-36: LGTM: CBOR MIME type addition

The MIMEApplicationCBOR constant with value "application/cbor" is correctly defined according to IANA media type standards and follows the existing naming convention.

binder/xml.go (1)

Line range hint 1-6: LGTM! Clean package structure and imports.

The package declaration and imports are minimal and appropriate for XML handling functionality.

binder/json.go (1)

7-18: LGTM! Clean implementation of JSON binding

The implementation is concise, follows the binder interface pattern, and correctly handles JSON binding.

binder/form.go (1)

Line range hint 11-61: LGTM! Robust form binding implementation

The implementation properly handles form data binding with support for:

  • Nested structures
  • Array handling
  • Error propagation
bind_test.go (2)

940-955: LGTM! Comprehensive CBOR test coverage

The test implementation properly handles CBOR binary data and includes both success and error cases.


1770-1775: ⚠️ Potential issue

Fix potential binary data corruption in CBOR test

Converting CBOR binary data to string could lead to data corruption. The test should use the raw byte slice.

-	testDecodeParser(MIMEApplicationCBOR, string(cb))
+	testDecodeParser(MIMEApplicationCBOR, cb)

Likely invalid or redundant comment.

binder/cookie.go (2)

14-17: LGTM!

The Name() method is well-documented and correctly implemented.


Line range hint 19-45: Several improvements needed for robustness and safety

The Bind implementation has several areas that could be improved:

  1. Add input validation for the out parameter
  2. Consider memory pre-allocation for the data map
  3. Add specific error types for different failure cases
  4. Document the safety implications of using unsafe string conversions

Consider applying these improvements:

 // Bind parses the request cookie and returns the result.
 func (b *cookieBinding) Bind(reqCtx *fasthttp.RequestCtx, out any) error {
+    if out == nil {
+        return fmt.Errorf("binding target cannot be nil")
+    }
+
+    // Pre-allocate map with estimated size
     data := make(map[string][]string)
-    var err error
+    var bindErr error
 
     reqCtx.Request.Header.VisitAllCookie(func(key, val []byte) {
-        if err != nil {
+        if bindErr != nil {
             return
         }
 
+        // Safety: These unsafe conversions are okay here as the byte slices
+        // are only used within this scope and fasthttp guarantees their validity
+        // during the request lifetime
         k := utils.UnsafeString(key)
         v := utils.UnsafeString(val)
 
         if strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) {
             values := strings.Split(v, ",")
+            data[k] = make([]string, 0, len(values))
             for i := 0; i < len(values); i++ {
                 data[k] = append(data[k], values[i])
             }
         } else {
-            data[k] = append(data[k], v)
+            data[k] = []string{v}
         }
     })
 
-    if err != nil {
-        return err
+    if bindErr != nil {
+        return fmt.Errorf("failed to bind cookies: %w", bindErr)
     }
 
     return parse(b.Name(), out, data)
 }

Let's verify the usage of equalFieldType to ensure it properly handles all edge cases:

✅ Verification successful

Let me check the implementation of equalFieldType to ensure it properly handles nil and edge cases.


Improvements needed but core implementation is sound

The review suggestions for the Bind method are valid, but the concerns about equalFieldType are less critical as the implementation is well-tested and handles edge cases appropriately:

  1. It properly handles nil checks implicitly through reflect.TypeOf(out).Elem()
  2. Has comprehensive type checking for maps and structs
  3. Supports embedded structs
  4. Includes proper tag handling
  5. Has good test coverage as seen in mapping_test.go

The suggested improvements for the Bind method remain valuable, particularly:

  • Adding nil check for out
  • Pre-allocating slices
  • Improving error handling
  • Documenting unsafe string usage

But the equalFieldType usage is correct and doesn't need modification.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for all usages and tests of equalFieldType
rg -A 5 "equalFieldType" 

Length of output: 3223


Script:

#!/bin/bash
# Get the implementation of equalFieldType
ast-grep --pattern 'func equalFieldType($_, $_, $_) $_ {
  $$$
}'

Length of output: 3143

binder/header.go (1)

Line range hint 19-35: Consider security and performance implications

The implementation has several areas that warrant attention:

  1. The use of utils.UnsafeString for performance optimization needs careful consideration
  2. Reflection usage in equalFieldType could impact performance
  3. No maximum size limit for header values could lead to memory issues
  4. No validation of header keys/values could pose security risks

Let's verify the safety of these operations:

Consider these improvements:

  1. Add header value size limits
  2. Implement header key/value validation
  3. Cache reflection results for better performance
  4. Add detailed error messages for binding failures
binder/uri.go (2)

1-4: LGTM! Clean and well-documented struct declaration.

The package name and struct declaration follow Go conventions, with appropriate documentation.


Line range hint 11-19: Add input validation and optimize map initialization.

Several improvements could enhance the robustness of this implementation:

  1. The map capacity assumes one value per parameter, but the slice could grow with multiple values
  2. Missing validation for nil parameters and callback
  3. No sanitization of parameter names

Let's verify the parse function implementation and usage:

Consider applying these improvements:

 func (b *uriBinding) Bind(params []string, paramsFunc func(key string, defaultValue ...string) string, out any) error {
+    if params == nil || paramsFunc == nil {
+        return errors.New("invalid parameters or callback")
+    }
+
     data := make(map[string][]string, len(params))
     for _, param := range params {
+        if param == "" {
+            continue
+        }
         data[param] = append(data[param], paramsFunc(param))
     }
+
+    if len(data) == 0 {
+        return errors.New("no valid parameters found")
+    }
 
     return parse(b.Name(), out, data)
 }
binder/resp_header.go (2)

14-17: LGTM!

The Name method implementation is correct and follows the binder interface requirements.


Line range hint 19-36: Verify integration with CBOR encoded responses.

Since this PR adds CBOR support, we should verify that the header binding works correctly with CBOR encoded responses.

binder/query.go (1)

14-17: LGTM!

The Name method is correctly implemented and well-documented.

binder/header.go Show resolved Hide resolved
binder/resp_header.go Show resolved Hide resolved
@ReneWerner87 ReneWerner87 merged commit 26cc477 into gofiber:main Dec 1, 2024
17 of 18 checks passed
Copy link

welcome bot commented Dec 1, 2024

Congrats on merging your first pull request! 🎉 We here at Fiber are proud of you! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

📝 [Proposal]: Add support for CBOR codec
4 participants