fix(cache): improve hasDirective function to handle various cases and…#4181
fix(cache): improve hasDirective function to handle various cases and…#4181KambojRajan wants to merge 1 commit intogofiber:mainfrom
Conversation
|
Thanks for opening this pull request! 🎉 Please check out our contributing guidelines. If you need help or want to chat with us, join us on Discord https://gofiber.io/discord |
WalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@KambojRajan What exactly is the improvement? The addition of several functions from the "strings" package may negatively impact overall performance compared to our existing utils. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the hasDirective function in the cache middleware to use standard string manipulation functions, enhancing readability and ensuring correct parsing of directives with values. A comprehensive test suite was also introduced to verify various Cache-Control header scenarios. Feedback suggests utilizing strings.SplitSeq to improve performance by avoiding unnecessary slice allocations.
| } | ||
| } | ||
| if i+dirLen == ccLen || cc[i+dirLen] == ',' { | ||
| for _, part := range strings.Split(cc, ",") { |
There was a problem hiding this comment.
For better performance and to reduce allocations, consider using strings.SplitSeq instead of strings.Split. strings.SplitSeq provides an iterator over the substrings, avoiding the allocation of a slice to hold all of them at once. This is more memory-efficient, especially for Cache-Control headers with many directives.
This change would also be consistent with the implementation of parseVary in this file, which already uses strings.SplitSeq.
| for _, part := range strings.Split(cc, ",") { | |
| for part := range strings.SplitSeq(cc, ",") { |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
middleware/cache/cache.go (1)
911-914: Switch toutils.TrimSpacefor consistencyLines 911 and 914 use
strings.TrimSpace, but this file already usesutils.TrimSpaceelsewhere (line 1253) and the project convention (from coding guidelines) requires preferringgithub.com/gofiber/utils/v2helpers for common string operations. Update both calls to useutils.TrimSpaceinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/cache/cache.go` around lines 911 - 914, Replace the two calls to strings.TrimSpace in the parsing code (the Trim of variable part and the Trim of variable key) with utils.TrimSpace (from github.com/gofiber/utils/v2) so they match the file's existing convention; ensure the utils package is imported and remove the strings import if it becomes unused. This affects the code that sets part = ... and key = ... (use utils.TrimSpace(part) and utils.TrimSpace(key)) so the behavior remains identical but follows project helper usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@middleware/cache/cache.go`:
- Around line 910-916: The loop that does strings.Split(cc, ",") can
misinterpret commas inside quoted directive values; replace this ad-hoc
comma-splitting with the RFC-aware tokenizer by calling
parseCacheControlDirectives(cc) and iterate its returned directives (or map of
key→value) instead of splitting, then use strings.EqualFold against the
directive variable to match keys (and use the provided value from
parseCacheControlDirectives when needed); update the logic in the function
containing the current loop so it consumes parseCacheControlDirectives' output
to avoid false matches from quoted commas.
---
Nitpick comments:
In `@middleware/cache/cache.go`:
- Around line 911-914: Replace the two calls to strings.TrimSpace in the parsing
code (the Trim of variable part and the Trim of variable key) with
utils.TrimSpace (from github.com/gofiber/utils/v2) so they match the file's
existing convention; ensure the utils package is imported and remove the strings
import if it becomes unused. This affects the code that sets part = ... and key
= ... (use utils.TrimSpace(part) and utils.TrimSpace(key)) so the behavior
remains identical but follows project helper usage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 584c726d-92c1-4b97-b440-ed548b6b62db
📒 Files selected for processing (2)
middleware/cache/cache.gomiddleware/cache/cache_test.go
| for _, part := range strings.Split(cc, ",") { | ||
| part = strings.TrimSpace(part) | ||
|
|
||
| key, _, _ := strings.Cut(part, "=") | ||
| key = strings.TrimSpace(key) | ||
|
|
||
| if strings.EqualFold(key, directive) { |
There was a problem hiding this comment.
Quoted commas can produce false directive matches
The current comma-split approach can misparse valid quoted directive values and report a directive that only appears inside a quoted string. Consider reusing parseCacheControlDirectives here for RFC-consistent tokenization.
Suggested fix
func hasDirective(cc, directive string) bool {
- for _, part := range strings.Split(cc, ",") {
- part = strings.TrimSpace(part)
-
- key, _, _ := strings.Cut(part, "=")
- key = strings.TrimSpace(key)
-
- if strings.EqualFold(key, directive) {
- return true
- }
- }
- return false
+ found := false
+ parseCacheControlDirectives(utils.UnsafeBytes(cc), func(key, _ []byte) {
+ if !found && utils.EqualFold(utils.UnsafeString(key), directive) {
+ found = true
+ }
+ })
+ return found
}📝 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.
| for _, part := range strings.Split(cc, ",") { | |
| part = strings.TrimSpace(part) | |
| key, _, _ := strings.Cut(part, "=") | |
| key = strings.TrimSpace(key) | |
| if strings.EqualFold(key, directive) { | |
| func hasDirective(cc, directive string) bool { | |
| found := false | |
| parseCacheControlDirectives(utils.UnsafeBytes(cc), func(key, _ []byte) { | |
| if !found && utils.EqualFold(utils.UnsafeString(key), directive) { | |
| found = true | |
| } | |
| }) | |
| return found | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@middleware/cache/cache.go` around lines 910 - 916, The loop that does
strings.Split(cc, ",") can misinterpret commas inside quoted directive values;
replace this ad-hoc comma-splitting with the RFC-aware tokenizer by calling
parseCacheControlDirectives(cc) and iterate its returned directives (or map of
key→value) instead of splitting, then use strings.EqualFold against the
directive variable to match keys (and use the provided value from
parseCacheControlDirectives when needed); update the logic in the function
containing the current loop so it consumes parseCacheControlDirectives' output
to avoid false matches from quoted commas.
|
@gaby |
|
instead of using strings, I'll use simpler methods to achieve the same |
|
@KambojRajan I appreciate it, but we already have someone working on this. See #4144 |
4 participants
… add tests
Description
Please provide a clear and concise description of the changes you've made and the problem they address. Include the purpose of the change, any relevant issues it solves, and the benefits it brings to the project. If this change introduces new features or adjustments, highlight them here.
Fixes # (issue)
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md