🐛 bug: Fix cfg.CacheControl being ignored #3726
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 |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@PolyAnthi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 26 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the 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. 📒 Files selected for processing (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @PolyAnthi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request addresses a critical bug within the GoFiber caching middleware. The core issue was that the CacheControl configuration, intended to manage how caching interacts with browser No-Cache directives, was not being properly applied. This fix ensures that the middleware now correctly respects the CacheControl setting, preventing unintended cache bypasses and improving the reliability of the caching mechanism.
Highlights
- CacheControl Configuration Fix: Resolved an issue in the GoFiber caching middleware where the "CacheControl" configuration option was being ignored. Previously, browsers sending a "No-Cache" header could bypass caching even when "CacheControl" was set to false, which was unintended. The fix ensures that this setting is now correctly enforced.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where the CacheControl configuration was being ignored by the middleware. The change ensures that the Cache-Control: no-cache request header is only respected when cfg.CacheControl is explicitly set to true.
I have one suggestion with medium severity to improve the readability of the conditional logic.
More importantly, there is a high severity issue regarding testing. It appears this change will cause the existing test Test_Cache_WithNoCacheRequestDirective to fail, as it asserts behavior that is now correctly fixed. This test needs to be updated to reflect the new, correct behavior (i.e., expect a cache hit when CacheControl is false and a no-cache header is present). It would also be beneficial to add a new test case to verify that the no-cache directive is still respected when CacheControl is set to true to ensure full coverage of this feature.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
I believe the solution there with converting one to deal with CacheControl: false and one to use the config with CacheControl: true is appropriate here. |
|
/gemini summary |
Summary of ChangesThis pull request addresses a critical bug in GoFiber's caching middleware where the Highlights
Changelog
Activity
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
middleware/cache/cache_test.go (1)
686-699: Add a direct no-ETag test for no-cache when CacheControl=true.You cover the ETag path well. A small additional test ensures we also respect no-cache without ETag.
+func Test_Cache_WithNoCacheRequestDirective_RespectWhenEnabled(t *testing.T) { + t.Parallel() + app := fiber.New() + app.Use(New(Config{CacheControl: true})) + app.Get("/", func(c *fiber.Ctx) error { + return c.SendString(c.Query("id", "1")) + }) + // Seed cache with id=1 + _, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil)) + utils.AssertEqual(t, nil, err) + // no-cache should bypass cache and fetch fresh + req := httptest.NewRequest(fiber.MethodGet, "/?id=2", nil) + req.Header.Set(fiber.HeaderCacheControl, noCache) + resp, err := app.Test(req) + utils.AssertEqual(t, nil, err) + body, err := io.ReadAll(resp.Body) + utils.AssertEqual(t, nil, err) + utils.AssertEqual(t, cacheMiss, resp.Header.Get("X-Cache")) + utils.AssertEqual(t, []byte("2"), body) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
middleware/cache/cache_test.go(26 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Format Go code using gofumpt (enforced viamake format)
Ensure code passes golangci-lint checks (enforced viamake lint)
Optimize struct field alignment using betteralign (enforced viamake betteralign)
Modernize Go code using gopls modernize (enforced viamake modernize)
Files:
middleware/cache/cache_test.go
🧠 Learnings (5)
📚 Learning: 2024-11-15T07:56:21.623Z
Learnt from: ReneWerner87
PR: gofiber/fiber#3161
File: app.go:923-932
Timestamp: 2024-11-15T07:56:21.623Z
Learning: In the Fiber framework, breaking changes are acceptable when moving from version 2 to version 3, including modifications to method signatures such as in the `Test` method in `app.go`.
Applied to files:
middleware/cache/cache_test.go
📚 Learning: 2024-10-02T23:03:31.727Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/session/store.go:164-167
Timestamp: 2024-10-02T23:03:31.727Z
Learning: Unit tests in this project use testify require.
Applied to files:
middleware/cache/cache_test.go
📚 Learning: 2024-10-08T19:06:06.583Z
Learnt from: sixcolors
PR: gofiber/fiber#2922
File: middleware/cors/utils.go:63-71
Timestamp: 2024-10-08T19:06:06.583Z
Learning: The project uses the testify/assert package for assertions in unit tests.
Applied to files:
middleware/cache/cache_test.go
📚 Learning: 2024-11-10T23:44:13.704Z
Learnt from: gaby
PR: gofiber/fiber#3193
File: middleware/adaptor/adaptor.go:111-111
Timestamp: 2024-11-10T23:44:13.704Z
Learning: In the `middleware/adaptor/adaptor.go` file of the Fiber framework, when updating context handling, replacing `c.Context()` with `c.RequestCtx()` is appropriate to access the `fasthttp.RequestCtx`.
Applied to files:
middleware/cache/cache_test.go
📚 Learning: 2024-09-25T15:57:10.221Z
Learnt from: sixcolors
PR: gofiber/fiber#3016
File: middleware/csrf/csrf_test.go:188-193
Timestamp: 2024-09-25T15:57:10.221Z
Learning: In the Fiber framework tests, using `ctx.Response.Header.Cookie` may not be suitable for parsing cookies from the response header, as it requires a `*Cookie` and fills it rather than returning a string value; thus, manual parsing of the `Set-Cookie` header may be necessary.
Applied to files:
middleware/cache/cache_test.go
🧬 Code graph analysis (1)
middleware/cache/cache_test.go (4)
ctx_interface_gen.go (1)
Ctx(17-414)middleware/cache/cache.go (1)
New(68-302)middleware/cache/config.go (1)
Config(11-75)req.go (2)
Query(726-733)Params(611-617)
🔇 Additional comments (5)
middleware/cache/cache_test.go (5)
34-44: LGTM: verifies Cache-Control header emission when CacheControl=true.Asserting "public, max-age=10" on the second request correctly exercises the cache-hit path with CacheControl enabled.
118-174: Good coverage: confirms no-cache is ignored when CacheControl=false (the bug fix).Requests with "Cache-Control: no-cache" still return a cache hit and the original payload, matching the intended behavior when CacheControl is disabled.
186-239: ETag + no-cache behavior validated end-to-end with CacheControl=true.Nice sequence covering 200 vs 304 and cache miss vs hit—this guards against regressions in the interaction between ETag and cache-control directives.
249-261: ** no-store request directive correctly bypasses cache.**This ensures the middleware unconditionally skips caching when clients send no-store.
269-297: Defaults are correct – no changes needed
ConfigDefaultexplicitly setsCacheControl: falseandconfigDefaultpreserves the zero‐value when not overridden, so tests remain valid.
|
Appears I was updating my, local, V2 copy of middleware and pushed that. I have amended this to the V3 copy from the repo |
gaby
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3726 +/- ##
==========================================
- Coverage 92.08% 92.03% -0.06%
==========================================
Files 115 115
Lines 11535 11535
==========================================
- Hits 10622 10616 -6
- Misses 662 667 +5
- Partials 251 252 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
Reply with @codex fix comments to fix any unresolved comments.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| _, size := heap.remove(e.heapidx) | ||
| storedBytes -= size | ||
| } | ||
| } else if e.exp != 0 && !hasRequestDirective(c, noCache) { | ||
| } else if e.exp != 0 && !(cfg.CacheControl && hasRequestDirective(c, noCache)) { |
There was a problem hiding this comment.
[P1] Honor client no-cache regardless of CacheControl flag
The new condition !(cfg.CacheControl && hasRequestDirective(c, noCache)) now serves cached entries whenever CacheControl is left at its default false, even if the client explicitly sends Cache-Control: no-cache. With this change there is no longer a way for clients to force a refresh of a still‑valid entry (e.g. when Expiration is long or zero) unless the application also enables CacheControl, which was previously only responsible for emitting response headers. Existing deployments that rely on no-cache requests to bypass server-side caching will now receive stale data until the entry expires or is manually invalidated. Consider keeping the bypass behavior independent from the response header option or providing a dedicated switch for it.
Useful? React with 👍 / 👎.
I checked the code. Perhaps this should be better documented in the configuration. This configuration should not be used to control caching; this is controlled by setting the middleware itself. The no-cache parameter in the request header should always be considered, because it is a clear instruction from the client to the server. Or is the idea to prevent the cache from being controlled on the client side? |
|
@ReneWerner87 Agree on your statement. The Reject this PR |
|
Closing in favor of #3740 |
Description
The caching middleware inside of GoFiber has an option called "CacheControl". By default, this is set to false. However, it is never actually enforced and browsers with "No-Cache" as its cache control header will still bypass (when on false). This PR fixes that.
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
Checklist
/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