🐛 fix: Align cache middleware with RFC7231 #3283
Conversation
miyamo2
requested review from
gaby,
sixcolors,
ReneWerner87 and
efectn
and removed request for
a team
WalkthroughThis pull request enhances the caching middleware in the Fiber framework by introducing a new variable, Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3283 +/- ##
==========================================
- Coverage 84.14% 84.04% -0.10%
==========================================
Files 116 116
Lines 11553 11557 +4
==========================================
- Hits 9721 9713 -8
- Misses 1402 1411 +9
- Partials 430 433 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![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)
51-56: LGTM! Consider adding a comment to explain TeaPot status code inclusion.The list of cacheable status codes aligns well with RFC 7231. However, it would be helpful to add a comment explaining why the TeaPot status code (418) is included despite not being cacheable according to the RFC.
var cacheableStatusCodes = []int{ fiber.StatusOK, fiber.StatusNonAuthoritativeInformation, fiber.StatusNoContent, fiber.StatusPartialContent, fiber.StatusMultipleChoices, fiber.StatusMovedPermanently, + // TeaPot status code is included for benchmarking purposes despite not being cacheable according to RFC 7231 fiber.StatusNotFound, fiber.StatusMethodNotAllowed, fiber.StatusGone, fiber.StatusTeapot, fiber.StatusRequestURITooLong, fiber.StatusNotImplemented, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
middleware/cache/cache.go(2 hunks)middleware/cache/cache_test.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint (1.62.2)
middleware/cache/cache_test.go
929-929: error returned from external package is unwrapped: sig: func strconv.Atoi(s string) (int, error)
(wrapcheck)
🪛 GitHub Check: lint
middleware/cache/cache_test.go
[failure] 929-929:
error returned from external package is unwrapped: sig: func strconv.Atoi(s string) (int, error) (wrapcheck)
🪛 GitHub Actions: golangci-lint
middleware/cache/cache_test.go
[error] 929-929: Error returned from external package strconv.Atoi is unwrapped. Consider wrapping the error using fmt.Errorf or custom error types.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (2)
middleware/cache/cache.go (1)
180-184: LGTM! The implementation effectively prevents caching of non-cacheable status codes.The conditional check is well-placed and correctly implements the PR objective of preventing responses with non-cacheable status codes from being stored in the cache.
middleware/cache/cache_test.go (1)
934-995: LGTM! Comprehensive test coverage for non-cacheable status codes.The test covers all categories of HTTP status codes (informational, successful, redirection, client error, and server error responses) ensuring thorough validation of the caching behavior.
|
Caution An unexpected error occurred while opening a pull request: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
gaby
changed the title
|
@gaby |
| fiber.StatusMethodNotAllowed: true, | ||
| fiber.StatusGone: true, | ||
| fiber.StatusRequestURITooLong: true, | ||
| fiber.StatusTeapot: true, |
There was a problem hiding this comment.
Maybe we should remove this one? Not sure?
There was a problem hiding this comment.
In a word, yes, that's sure.
418: Teapot was used in the benchmark test, so I kept it.
Also, there is the point that 418: Teapot itself was a joke and did not much care about it.
https://github.com/miyamo2/fiber/blob/04eb05e98b21b7a7b93448beceec3c28b0e45b3a/middleware/cache/cache_test.go#L1010C19-L1010C37
Description
This pull request updates the cache middleware to avoid caching uncacheable status codes.
This change improves cache accuracy and saves cache storage size.
https://datatracker.ietf.org/doc/html/rfc7231#section-6.1
See also:
However, for 418: TeaPot, although not actually cacheable, it continues to be cacheable since it is used in benchmark tests.
Thank you.
Fixes -
Type of change
Checklist
/docs/directory for Fiber's documentation.Benchmarks(on GitHub Codespaces)
Before
After