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

🐛 bug: fix EnableSplittingOnParsers is not functional #3231

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

efectn
Copy link
Member

@efectn efectn commented Dec 5, 2024

Description

It seems as if EnableSplittingOnParsers config property has lost its functionality during the introduction of new binding API PR in v3. This PR fixes default behavior (parsers should not split strings includes comma by default)

  • Also add support for external XML decoders just like JSON and CBOR.

Fixes #3159

Changes introduced

List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.

  • Benchmarks: Describe any performance benchmarks and improvements related to the changes.
  • Documentation Update: Detail the updates made to the documentation and links to the changed files.
  • Changelog/What's New: Include a summary of the additions for the upcoming release notes.
  • Migration Guide: If necessary, provide a guide or steps for users to migrate their existing code to accommodate these changes.
  • API Alignment with Express: Explain how the changes align with the Express API.
  • API Longevity: Discuss the steps taken to ensure that the new or updated APIs are consistent and not prone to breaking changes.
  • Examples: Provide examples demonstrating the new features or changes in action.

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Enhancement (improvement to existing features and functionality)
  • Documentation update (changes to documentation)
  • Performance improvement (non-breaking change which improves efficiency)
  • Code consistency (non-breaking change which improves code reliability and robustness)

Checklist

Before you submit your pull request, please make sure you meet these requirements:

  • Followed the inspiration of the Express.js framework for new functionalities, making them similar in usage.
  • Conducted a self-review of the code and provided comments for complex or critical parts.
  • Updated the documentation in the /docs/ directory for Fiber's documentation.
  • Added or updated unit tests to validate the effectiveness of the changes or new features.
  • Ensured that new and existing unit tests pass locally with the changes.
  • Verified that any new dependencies are essential and have been agreed upon by the maintainers/community.
  • Aimed for optimal performance with minimal allocations in the new code.
  • Provided benchmarks for the new code to analyze and improve upon.

Commit formatting

Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md

@efectn efectn requested a review from a team as a code owner December 5, 2024 08:02
@efectn efectn requested review from gaby, sixcolors and ReneWerner87 and removed request for a team December 5, 2024 08:03
Copy link
Contributor

coderabbitai bot commented Dec 5, 2024

Warning

Rate limit exceeded

@efectn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 5 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 725855d and a56943d.

Walkthrough

The pull request introduces significant enhancements to the binding functionalities across various files, primarily focusing on implementing a pooling mechanism for binder instances to optimize resource management. The bind.go file is updated to utilize this pooling strategy, while other binding types like CBORBinding, CookieBinding, and JSONBinding are restructured to include new configuration fields such as EnableSplitting. Additionally, comprehensive test cases are added to ctx_test.go to validate the new functionalities, ensuring robust handling of parameters, queries, and response headers.

Changes

File Change Summary
bind.go Added encoding/xml import; refactored binding methods to use a pooling mechanism for binders; updated XML method to use xml.Unmarshal.
bind_test.go Introduced EnableSplittingOnParsers configuration in test cases, affecting how headers and cookies are parsed.
binder/binder.go Removed default binder variables; added sync.Pool instances for binder management; introduced GetFromThePool and PutToThePool functions.
binder/cbor.go Renamed cborBinding to CBORBinding; added CBORDecoder field; modified Bind method to use CBORDecoder.
binder/cookie.go Renamed cookieBinding to CookieBinding; added EnableSplitting field; modified Bind method logic.
binder/form.go Renamed formBinding to FormBinding; added EnableSplitting field; modified Bind method logic.
binder/header.go Renamed headerBinding to HeaderBinding; added EnableSplitting field; modified Bind method logic.
binder/json.go Renamed jsonBinding to JSONBinding; added JSONDecoder field; modified Bind method to use JSONDecoder.
binder/mapping.go Simplified tags initialization; modified equalFieldType function to use static strings for binder types.
binder/query.go Renamed queryBinding to QueryBinding; added EnableSplitting field; modified Bind method logic.
binder/resp_header.go Renamed respHeaderBinding to RespHeaderBinding; added EnableSplitting field; modified Bind method logic.
binder/uri.go Renamed uriBinding to URIBinding; updated method receivers without altering logic.
binder/xml.go Renamed xmlBinding to XMLBinding; added XMLDecoder field; modified Bind method to use XMLDecoder.
ctx_test.go Added numerous tests for parameter binding, query handling, response header management, file handling, and error handling.

Possibly related PRs

Suggested labels

☢️ Bug, v3

Suggested reviewers

  • sixcolors
  • gaby
  • ReneWerner87

🐰 In the land of code where bunnies hop,
We’ve pooled our binders, now they won’t stop!
With splitting enabled, our data flows free,
Testing each feature, as happy as can be!
So let’s celebrate this code we adore,
With every new change, we’ll leap even more! 🐇✨


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.

@efectn efectn added the v3 label Dec 5, 2024
@efectn efectn added this to the v3 milestone Dec 5, 2024
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 (8)
binder/binder.go (1)

14-66: Refactor to reduce code duplication in sync.Pool declarations

The sync.Pool declarations for various binder types are repetitive. Consider refactoring to reduce duplication by using a helper function.

Suggested change:

+func newBinderPool[T any](binder T) sync.Pool {
+    return sync.Pool{
+        New: func() any {
+            return binder
+        },
+    }
+}

-var HeaderBinderPool = sync.Pool{
-    New: func() any {
-        return &HeaderBinding{
-            EnableSplitting: false,
-        }
-    },
-}
+var HeaderBinderPool = newBinderPool(&HeaderBinding{
+    EnableSplitting: false,
+})

Apply similar changes to other binder pools.

binder/header.go (1)

Line range hint 28-34: Consider performance and whitespace handling improvements

The current implementation has two potential areas for improvement:

  1. Performance: strings.Contains followed by strings.Split creates unnecessary overhead when splitting is disabled
  2. Whitespace: Split values might contain leading/trailing whitespace

Consider this optimization:

-		if b.EnableSplitting && strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) {
-			values := strings.Split(v, ",")
+		if b.EnableSplitting && equalFieldType(out, reflect.Slice, k) {
+			if strings.Contains(v, ",") {
+				values := strings.Split(v, ",")
+				for i := 0; i < len(values); i++ {
+					data[k] = append(data[k], strings.TrimSpace(values[i]))
+				}
+				continue
+			}
+		}
-			for i := 0; i < len(values); i++ {
-				data[k] = append(data[k], values[i])
-			}
-		} else {
-			data[k] = append(data[k], v)
-		}
+		data[k] = append(data[k], v)
binder/resp_header.go (1)

Line range hint 28-34: Consider extracting common header binding logic

The header parsing logic is duplicated between HeaderBinding and RespHeaderBinding.

Consider creating a shared utility function:

+// headerBindingUtil handles common header binding logic
+func headerBindingUtil(enableSplitting bool, out any, visitor func(func(key, val []byte))) (map[string][]string, error) {
+	data := make(map[string][]string)
+	visitor(func(key, val []byte) {
+		k := utils.UnsafeString(key)
+		v := utils.UnsafeString(val)
+		
+		if enableSplitting && equalFieldType(out, reflect.Slice, k) {
+			if strings.Contains(v, ",") {
+				values := strings.Split(v, ",")
+				for i := 0; i < len(values); i++ {
+					data[k] = append(data[k], strings.TrimSpace(values[i]))
+				}
+				return
+			}
+		}
+		data[k] = append(data[k], v)
+	})
+	return data, nil
+}

Then update both binding implementations:

func (b *HeaderBinding) Bind(req *fasthttp.Request, out any) error {
-	data := make(map[string][]string)
-	req.Header.VisitAll(func(key, val []byte) {
-		k := utils.UnsafeString(key)
-		v := utils.UnsafeString(val)
-		if b.EnableSplitting && 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])
-			}
-		} else {
-			data[k] = append(data[k], v)
-		}
-	})
+	data, err := headerBindingUtil(b.EnableSplitting, out, req.Header.VisitAll)
+	if err != nil {
+		return err
+	}
	return parse(b.Name(), out, data)
}

This refactoring would:

  1. Eliminate code duplication
  2. Make future maintenance easier
  3. Ensure consistent behavior across both bindings
  4. Include the performance and whitespace improvements mentioned earlier
binder/form.go (1)

Line range hint 1-67: Consider adding integration tests for the EnableSplitting feature

The changes across all three binder types introduce similar splitting functionality. We should ensure comprehensive testing of this feature.

Would you like me to help create integration tests that verify the consistent behavior of EnableSplitting across all binder types? This would help ensure that:

  1. The feature works consistently across all binder types
  2. Edge cases are handled uniformly
  3. The interaction with existing features (like array notation) is tested
binder/mapping.go (2)

35-35: Consider using constants for binder type names.

Using string literals directly makes the code fragile to changes. Consider defining these as constants or getting them from the binder types to maintain a single source of truth.

-tags = []string{"header", "respHeader", "cookie", "query", "form", "uri"}
+const (
+    HeaderBinder     = "header"
+    RespHeaderBinder = "respHeader"
+    CookieBinder     = "cookie"
+    QueryBinder      = "query"
+    FormBinder       = "form"
+    URIBinder        = "uri"
+)
+tags = []string{HeaderBinder, RespHeaderBinder, CookieBinder, QueryBinder, FormBinder, URIBinder}

226-226: Use the defined constant for query binder name.

Using a hardcoded string "query" creates a maintenance burden. This should use the same constant as defined for the tags array to maintain consistency.

-inputFieldName := typeField.Tag.Get("query") // Name of query binder
+inputFieldName := typeField.Tag.Get(QueryBinder) // Name of query binder
bind.go (1)

190-197: Consider making XMLDecoder configurable.

While other decoders (JSON, CBOR) are configurable through app config, the XML decoder is hardcoded to xml.Unmarshal. Consider making it configurable for consistency.

-bind.XMLDecoder = xml.Unmarshal
+bind.XMLDecoder = b.ctx.App().Config().XMLDecoder
bind_test.go (1)

35-37: Good test coverage for EnableSplittingOnParsers.

The tests thoroughly verify the splitting behavior across different binding types. However, consider adding negative test cases where EnableSplittingOnParsers is false to ensure the default behavior is maintained.

Add test cases like:

t.Run("WithoutSplitting", func(t *testing.T) {
    app := New(Config{
        EnableSplittingOnParsers: false,
    })
    // Test that values aren't split
})

Also applies to: 116-118, 550-552, 760-762, 1352-1354, 1427-1429

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 89a0cd3 and 725855d.

📒 Files selected for processing (14)
  • bind.go (10 hunks)
  • bind_test.go (10 hunks)
  • binder/binder.go (2 hunks)
  • binder/cbor.go (1 hunks)
  • binder/cookie.go (2 hunks)
  • binder/form.go (3 hunks)
  • binder/header.go (1 hunks)
  • binder/json.go (1 hunks)
  • binder/mapping.go (2 hunks)
  • binder/query.go (2 hunks)
  • binder/resp_header.go (1 hunks)
  • binder/uri.go (1 hunks)
  • binder/xml.go (1 hunks)
  • ctx_test.go (1 hunks)
🔇 Additional comments (13)
ctx_test.go (2)

1450-1534: Subtests in Test_Ctx_Binders improve test organization

The use of subtests enhances readability and maintainability of the test cases.


1536-1555: Ensure consistent behavior with EnableSplittingOnParsers configuration

In Test_Ctx_Binders_Comma, the EnableSplittingOnParsers is set to true. Verify that this is the intended behavior and consistent with other tests.

binder/uri.go (1)

4-4: LGTM! Consistent with binding type exports

The renaming of uriBinding to URIBinding aligns with the package's move towards exported binding types. Correctly maintains original functionality without adding EnableSplitting since URI parameters are inherently single values.

Let's verify the consistency of binding type exports:

Also applies to: 7-8, 12-12

✅ Verification successful

All binding types are consistently exported

The verification confirms that all binding types in the codebase follow the same pattern of being exported (capitalized) struct types:

  • URIBinding
  • RespHeaderBinding
  • XMLBinding
  • QueryBinding
  • HeaderBinding
  • CookieBinding
  • CBORBinding
  • FormBinding
  • JSONBinding
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if all binding types are exported
rg -g 'binder/*.go' "type.*Binding.*struct"

Length of output: 433

binder/header.go (1)

12-14: LGTM! Clear struct definition with EnableSplitting flag

The addition of EnableSplitting field is well-documented and properly encapsulated.

binder/resp_header.go (1)

12-14: LGTM! Consistent with HeaderBinding implementation

The RespHeaderBinding structure mirrors HeaderBinding, maintaining consistency across the package.

binder/cookie.go (2)

11-14: LGTM: Clean struct definition with clear documentation

The exported CookieBinding struct with the EnableSplitting field is well-documented and follows Go conventions.


Line range hint 34-41: Verify the behavior of empty values in comma-separated strings

The splitting logic looks correct, but we should handle edge cases for comma-separated values.

Consider handling empty values explicitly:

 if b.EnableSplitting && strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) {
     values := strings.Split(v, ",")
+    // Filter out empty values
+    filtered := make([]string, 0, len(values))
     for i := 0; i < len(values); i++ {
-        data[k] = append(data[k], values[i])
+        if trimmed := strings.TrimSpace(values[i]); trimmed != "" {
+            filtered = append(filtered, trimmed)
+        }
     }
+    if len(filtered) > 0 {
+        data[k] = filtered
+    }
 } else {
binder/query.go (2)

11-14: LGTM: Consistent struct definition with cookie binding

The QueryBinding struct follows the same pattern as CookieBinding, maintaining consistency across the package.


Line range hint 38-45: Consider order of operations for bracket parsing and value splitting

The implementation correctly handles both square bracket parsing and value splitting, but we should verify the interaction between these two features.

✅ Verification successful

Let me gather more specific information about the test cases and implementation.


Let me try a different approach to find the relevant test cases and implementation.


Based on the test cases found, I can now verify the implementation. Let me check the specific query binding implementation.


Based on the test cases and implementation found, I can now generate the final response.

The interaction between array notation and comma-separated values is properly handled

The test cases demonstrate that both features work correctly together:

  • When EnableSplitting is true, comma-separated values are properly split into slices
  • Array notation with square brackets is handled independently
  • Test cases verify both scenarios:
    • data[]=1&data[]=2 works for array notation
    • hobby=basketball,football works for comma splitting
    • hobby=soccer&hobby=basketball,football correctly combines both approaches

The implementation in binder/query.go correctly handles both cases by:

  1. First checking for EnableSplitting and comma presence
  2. Then handling array notation separately
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for test cases combining array notation with comma-separated values
rg -A 5 'TestQueryBinding.*Array.*Split' --type go

# Look for actual usage patterns
rg -A 2 '\[.*\].*=.*,.*,' --type go

Length of output: 10870


Script:

#!/bin/bash
# Search for test cases specifically testing array binding behavior
ast-grep --pattern 'func Test$_Bind$_($$$) {
  $$$
}'

# Look for the actual implementation of array binding
ast-grep --pattern 'func ($b $_) bind($$$) {
  $$$
}'

# Search for test cases with array notation and comma-separated values
rg -B 2 -A 2 'TestBind.*func.*\{.*\[\]' --type go

Length of output: 163


Script:

#!/bin/bash
# Search for test cases related to query binding with arrays
rg -B 3 -A 5 'TestQuery.*func' --type go

# Look for the implementation of query binding
rg -B 3 -A 10 'func.*query.*bind' --type go

# Find test cases with array notation in query strings
rg -B 2 -A 2 'SetQueryString.*\[\]' --type go

Length of output: 751


Script:

#!/bin/bash
# Look for the query binding implementation in binder package
rg -B 5 -A 10 'EnableSplitting.*Split' --type go

# Find test cases validating array notation and splitting behavior
cat bind_test.go

# Check for any related configuration or documentation
rg -l 'EnableSplitting' --type go

Length of output: 52822

binder/form.go (2)

11-14: LGTM: Consistent struct definition with other binders

The FormBinding struct maintains consistency with CookieBinding and QueryBinding.


Line range hint 38-45: Consider consistent behavior between Bind and BindMultipart

The splitting logic is implemented in Bind but not in BindMultipart. This could lead to inconsistent behavior.

Consider implementing the same splitting logic in BindMultipart:

 func (b *FormBinding) BindMultipart(reqCtx *fasthttp.RequestCtx, out any) error {
     data, err := reqCtx.MultipartForm()
     if err != nil {
         return err
     }
 
+    // Apply splitting logic to multipart form values
+    if b.EnableSplitting {
+        processedData := make(map[string][]string)
+        for k, vals := range data.Value {
+            for _, v := range vals {
+                if strings.Contains(v, ",") && equalFieldType(out, reflect.Slice, k) {
+                    values := strings.Split(v, ",")
+                    processedData[k] = append(processedData[k], values...)
+                } else {
+                    processedData[k] = append(processedData[k], v)
+                }
+            }
+        }
+        return parse(b.Name(), out, processedData)
+    }
 
     return parse(b.Name(), out, data.Value)
 }
bind.go (1)

82-89: LGTM! Good resource management with binder pooling.

The implementation properly manages binder lifecycle:

  1. Gets binder from pool
  2. Configures EnableSplitting
  3. Ensures cleanup in defer block
bind_test.go (1)

325-325: Verify the reduced Hobby array length is intentional.

The test expects shorter array lengths after enabling splitting. Please confirm this is the intended behavior of the EnableSplittingOnParsers feature.

Also applies to: 331-331, 393-393, 399-399

✅ Verification successful

Let me gather more context about the test file and the splitting behavior.


Let me check the documentation and implementation of EnableSplittingOnParsers to understand the intended behavior.


Based on the documentation and implementation details gathered, I can now provide the final response:

The reduced array length is expected with EnableSplittingOnParsers enabled

The test is correctly verifying the behavior of the Fiber framework's binding functionality. When EnableSplittingOnParsers is set to true in the app config (as shown in the test file), comma-separated values in headers are not split into arrays. This is the intended behavior as evidenced by the test setup and documentation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if this behavior is documented or mentioned in issues
gh issue view 3223

Length of output: 3171


Script:

#!/bin/bash
# Check the test file content around the mentioned lines
rg -B 10 -A 10 "require.Len\(t, q.Hobby, 1\)" bind_test.go

# Look for any related test cases or documentation about splitting
rg -l "EnableSplittingOnParsers" .

Length of output: 886


Script:

#!/bin/bash
# Check documentation about EnableSplittingOnParsers
cat docs/api/bind.md

# Look for the implementation and related tests
rg -A 5 -B 5 "EnableSplittingOnParsers" bind.go bind_test.go

Length of output: 22910

Comment on lines +68 to +75
func GetFromThePool[T any](pool *sync.Pool) T {
binder, ok := pool.Get().(T)
if !ok {
panic(errors.New("failed to type-assert to T"))
}

return binder
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid panicking on type assertion failure in GetFromThePool

Using panic on type assertion failures can lead to unexpected crashes. Consider returning an error instead to handle such cases gracefully.

Suggested change:

-func GetFromThePool[T any](pool *sync.Pool) T {
+func GetFromThePool[T any](pool *sync.Pool) (T, error) {
    binder, ok := pool.Get().(T)
    if !ok {
-       panic(errors.New("failed to type-assert to T"))
+       var zero T
+       return zero, errors.New("failed to type-assert to T")
    }

-   return binder
+   return binder, nil
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
func GetFromThePool[T any](pool *sync.Pool) T {
binder, ok := pool.Get().(T)
if !ok {
panic(errors.New("failed to type-assert to T"))
}
return binder
}
func GetFromThePool[T any](pool *sync.Pool) (T, error) {
binder, ok := pool.Get().(T)
if !ok {
var zero T
return zero, errors.New("failed to type-assert to T")
}
return binder, nil
}

Comment on lines +14 to +66
var HeaderBinderPool = sync.Pool{
New: func() any {
return &HeaderBinding{}
},
}

var RespHeaderBinderPool = sync.Pool{
New: func() any {
return &RespHeaderBinding{}
},
}

var CookieBinderPool = sync.Pool{
New: func() any {
return &CookieBinding{}
},
}

var QueryBinderPool = sync.Pool{
New: func() any {
return &QueryBinding{}
},
}

var FormBinderPool = sync.Pool{
New: func() any {
return &FormBinding{}
},
}

var URIBinderPool = sync.Pool{
New: func() any {
return &URIBinding{}
},
}

var XMLBinderPool = sync.Pool{
New: func() any {
return &XMLBinding{}
},
}

var JSONBinderPool = sync.Pool{
New: func() any {
return &JSONBinding{}
},
}

var CBORBinderPool = sync.Pool{
New: func() any {
return &CBORBinding{}
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Initialize decoder fields in binder instances to avoid nil pointer dereferences

The JSONDecoder, XMLDecoder, and CBORDecoder fields in JSONBinding, XMLBinding, and CBORBinding are not initialized, which may lead to nil pointer dereferences when their Bind methods are called. Ensure these fields are properly initialized when creating binder instances.

For example, initialize JSONDecoder in JSONBinding:

return &JSONBinding{
+   JSONDecoder: utils.JSONUnmarshalDefault,
}

Similarly, initialize XMLDecoder and CBORDecoder:

// For XMLBinding
return &XMLBinding{
+   XMLDecoder: utils.XMLUnmarshalDefault,
}

// For CBORBinding
return &CBORBinding{
+   CBORDecoder: utils.CBORUnmarshalDefault,
}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var HeaderBinderPool = sync.Pool{
New: func() any {
return &HeaderBinding{}
},
}
var RespHeaderBinderPool = sync.Pool{
New: func() any {
return &RespHeaderBinding{}
},
}
var CookieBinderPool = sync.Pool{
New: func() any {
return &CookieBinding{}
},
}
var QueryBinderPool = sync.Pool{
New: func() any {
return &QueryBinding{}
},
}
var FormBinderPool = sync.Pool{
New: func() any {
return &FormBinding{}
},
}
var URIBinderPool = sync.Pool{
New: func() any {
return &URIBinding{}
},
}
var XMLBinderPool = sync.Pool{
New: func() any {
return &XMLBinding{}
},
}
var JSONBinderPool = sync.Pool{
New: func() any {
return &JSONBinding{}
},
}
var CBORBinderPool = sync.Pool{
New: func() any {
return &CBORBinding{}
},
}
var HeaderBinderPool = sync.Pool{
New: func() any {
return &HeaderBinding{}
},
}
var RespHeaderBinderPool = sync.Pool{
New: func() any {
return &RespHeaderBinding{}
},
}
var CookieBinderPool = sync.Pool{
New: func() any {
return &CookieBinding{}
},
}
var QueryBinderPool = sync.Pool{
New: func() any {
return &QueryBinding{}
},
}
var FormBinderPool = sync.Pool{
New: func() any {
return &FormBinding{}
},
}
var URIBinderPool = sync.Pool{
New: func() any {
return &URIBinding{}
},
}
var XMLBinderPool = sync.Pool{
New: func() any {
return &XMLBinding{
XMLDecoder: utils.XMLUnmarshalDefault,
}
},
}
var JSONBinderPool = sync.Pool{
New: func() any {
return &JSONBinding{
JSONDecoder: utils.JSONUnmarshalDefault,
}
},
}
var CBORBinderPool = sync.Pool{
New: func() any {
return &CBORBinding{
CBORDecoder: utils.CBORUnmarshalDefault,
}
},
}

Comment on lines +7 to +10
// JSONBinding is the JSON binder for JSON request body.
type JSONBinding struct {
JSONDecoder utils.JSONUnmarshal
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Initialize JSONDecoder field in JSONBinding to prevent nil pointer dereference

The JSONDecoder field is not initialized, which may cause a nil pointer dereference when Bind is called. Ensure that JSONDecoder is set to a valid unmarshalling function.

Suggested change:

type JSONBinding struct {
-   JSONDecoder utils.JSONUnmarshal
+   JSONDecoder utils.JSONUnmarshal
}

+func NewJSONBinding() *JSONBinding {
+    return &JSONBinding{
+        JSONDecoder: utils.JSONUnmarshalDefault,
+    }
+}

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +7 to +10
// CBORBinding is the CBOR binder for CBOR request body.
type CBORBinding struct {
CBORDecoder utils.CBORUnmarshal
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Initialize CBORDecoder field in CBORBinding to prevent nil pointer dereference

The CBORDecoder field is not initialized, which may cause a nil pointer dereference when Bind is called. Ensure that CBORDecoder is set to a valid unmarshalling function.

Suggested change:

type CBORBinding struct {
-   CBORDecoder utils.CBORUnmarshal
+   CBORDecoder utils.CBORUnmarshal
}

+func NewCBORBinding() *CBORBinding {
+    return &CBORBinding{
+        CBORDecoder: utils.CBORUnmarshalDefault,
+    }
+}

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +9 to +12
// XMLBinding is the XML binder for XML request body.
type XMLBinding struct {
XMLDecoder utils.XMLUnmarshal
}
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Initialize XMLDecoder field in XMLBinding to prevent nil pointer dereference

The XMLDecoder field is not initialized, which may cause a nil pointer dereference when Bind is called. Ensure that XMLDecoder is set to a valid unmarshalling function.

Suggested change:

type XMLBinding struct {
-   XMLDecoder utils.XMLUnmarshal
+   XMLDecoder utils.XMLUnmarshal
}

+func NewXMLBinding() *XMLBinding {
+    return &XMLBinding{
+        XMLDecoder: utils.XMLUnmarshalDefault,
+    }
+}

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.

Benchmark suite Current: a56943d Previous: 89a0cd3 Ratio
Benchmark_Compress_Levels_Parallel/Brotli_LevelBestCompression - B/op 1 B/op 0 B/op +∞

This comment was automatically generated by workflow using github-action-benchmark.

Copy link

codecov bot commented Dec 5, 2024

Codecov Report

Attention: Patch coverage is 53.59477% with 71 lines in your changes missing coverage. Please review.

Project coverage is 82.51%. Comparing base (89a0cd3) to head (a56943d).

Files with missing lines Patch % Lines
binder/binder.go 0.00% 34 Missing ⚠️
bind.go 88.63% 7 Missing and 3 partials ⚠️
binder/form.go 0.00% 4 Missing ⚠️
binder/cbor.go 0.00% 3 Missing ⚠️
binder/cookie.go 0.00% 3 Missing ⚠️
binder/header.go 0.00% 3 Missing ⚠️
binder/json.go 0.00% 3 Missing ⚠️
binder/query.go 0.00% 3 Missing ⚠️
binder/resp_header.go 0.00% 3 Missing ⚠️
binder/xml.go 0.00% 3 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3231      +/-   ##
==========================================
- Coverage   82.86%   82.51%   -0.36%     
==========================================
  Files         115      116       +1     
  Lines       11295    11410     +115     
==========================================
+ Hits         9360     9415      +55     
- Misses       1537     1595      +58     
- Partials      398      400       +2     
Flag Coverage Δ
unittests 82.51% <53.59%> (-0.36%) ⬇️

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.

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

Successfully merging this pull request may close these issues.

🤗 [Question]: How do we get multiple values for a key in form submission?
1 participant