🧹 chore: Improve cache middleware RFC compliance#3488
Conversation
WalkthroughThis update enhances the cache middleware to improve HTTP caching compliance. It introduces logic for handling the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant CacheMiddleware
participant CacheStore
participant UpstreamHandler
Client->>CacheMiddleware: HTTP Request
CacheMiddleware->>CacheStore: Check for cached response
alt Cache hit
CacheStore-->>CacheMiddleware: Cached response + metadata
CacheMiddleware->>CacheMiddleware: Calculate Age, set Age header
CacheMiddleware->>Client: Serve cached response
else Cache miss
CacheMiddleware->>UpstreamHandler: Forward request
UpstreamHandler-->>CacheMiddleware: Fresh response
CacheMiddleware->>CacheMiddleware: Check Cache-Control directives
alt Cacheable
CacheMiddleware->>CacheStore: Store response with TTL
CacheMiddleware->>Client: Serve fresh response (Age: 0)
else Not cacheable (e.g., no-store)
CacheMiddleware->>Client: Serve fresh response (not cached)
end
end
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 (4)
✨ Finishing Touches
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. 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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3488 +/- ##
==========================================
+ Coverage 83.75% 83.88% +0.12%
==========================================
Files 120 120
Lines 12246 12261 +15
==========================================
+ Hits 10257 10285 +28
+ Misses 1564 1553 -11
+ Partials 425 423 -2
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:
|
There was a problem hiding this comment.
Pull Request Overview
This PR refines the cache middleware to better align with RFC compliance by adding support for an RFC-compliant Age header and improved handling of Cache-Control directives.
- Updated cache manager internals (e.g., a new ttl field and revised get/release logic).
- Added comprehensive tests for Age header behavior, no-store directive, and Cache-Control preservation.
- Updated documentation to highlight the new caching behaviors.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| middleware/cache/manager.go | Introduces ttl field and revises logic to better handle storage cases. |
| middleware/cache/cache_test.go | Adds tests covering Age header, no-store directive, and max-age parsing. |
| middleware/cache/cache.go | Adjusts cache header setting logic to avoid overwriting and improve compliance. |
| docs/whats_new.md | Documents the changes regarding the new Age header and cache improvements. |
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
middleware/cache/manager.go (1)
75-93: Improved error handling and clarity in thegetmethod.The refactored implementation:
- Returns
nilimmediately on storage errors or missing data- Properly releases items if unmarshaling fails
- Simplifies the in-memory storage path
Consider adding test coverage for the unmarshal error case (lines 81-82) to ensure this error handling path works correctly.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 81-82: middleware/cache/manager.go#L81-L82
Added lines #L81 - L82 were not covered by testsmiddleware/cache/cache_test.go (1)
1002-1020: Improve test naming consistency and timing validation.The test correctly validates Age header behavior but has some areas for improvement:
- Naming Convention: The function should be
Test_CacheAgeHeaderto match the convention used by other tests in this file.- Long Sleep Duration: The 4-second sleep makes the test suite slower. Consider reducing to 1-2 seconds.
- Imprecise Age Validation: The test only checks that age is positive but doesn't validate it's approximately the expected duration.
-func TestCacheAgeHeader(t *testing.T) { +func Test_CacheAgeHeader(t *testing.T) { t.Parallel() app := fiber.New() app.Use(New(Config{Expiration: 10 * time.Second})) app.Get("/", func(c fiber.Ctx) error { return c.SendString("ok") }) resp, err := app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil)) require.NoError(t, err) require.Equal(t, "0", resp.Header.Get(fiber.HeaderAge)) - time.Sleep(4 * time.Second) + sleepDuration := 2 * time.Second + time.Sleep(sleepDuration) resp, err = app.Test(httptest.NewRequest(fiber.MethodGet, "/", nil)) require.NoError(t, err) require.Equal(t, cacheHit, resp.Header.Get("X-Cache")) age, err := strconv.Atoi(resp.Header.Get(fiber.HeaderAge)) require.NoError(t, err) - require.Positive(t, age) + require.GreaterOrEqual(t, age, int(sleepDuration.Seconds())) + require.LessOrEqual(t, age, int(sleepDuration.Seconds())+1) // Allow 1 second tolerance }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
docs/whats_new.md(1 hunks)middleware/cache/cache.go(5 hunks)middleware/cache/cache_test.go(1 hunks)middleware/cache/manager.go(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
middleware/cache/cache.go (3)
constants.go (2)
HeaderCacheControl(170-170)HeaderAge(169-169)app.go (1)
Storage(45-65)internal/storage/memory/memory.go (1)
Storage(13-18)
🪛 GitHub Check: codecov/patch
middleware/cache/manager.go
[warning] 81-82: middleware/cache/manager.go#L81-L82
Added lines #L81 - L82 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Compare
- GitHub Check: repeated
🔇 Additional comments (13)
docs/whats_new.md (1)
974-975: Documentation accurately reflects the new Age header feature.The documentation clearly explains the addition of the RFC-compliant Age header to cached responses, which aligns well with the PR's goal of improving cache middleware RFC compliance.
middleware/cache/manager.go (2)
21-21: Good addition of thettlfield for Age header support.The
ttlfield enables accurate calculation of the Age header by storing the total time-to-live duration when the cache entry is created.
59-62: Critical fix: Corrected the in-memory storage check.The condition has been properly fixed to
m.storage == nilto prevent items from being incorrectly released back to the pool when using in-memory storage. This is important because in-memory storage keeps direct references to the items.middleware/cache/cache.go (6)
165-174: Excellent RFC-compliant cache header implementation.The implementation correctly:
- Sets
Cache-Controlheader only when enabled and not already present- Calculates the Age header as
ttl - (exp - ts), which accurately represents how long the response has been cachedThe Age calculation is particularly well done, showing the elapsed time since the response was originally generated.
177-181: Proper resource management for external storage.Good addition to release the item back to the pool after serving from external storage, which aligns with the corrected release logic in
manager.go.
197-201: Correctly respects the server'sno-storedirective.This implementation properly follows RFC 7234 Section 5.2.2.3 by not caching responses when the server includes
Cache-Control: no-store.
242-244: Good practice: Initialize Age header for fresh responses.Setting the Age header to "0" for new responses that don't already have it ensures RFC compliance from the start of the caching lifecycle.
263-265: Smart cache expiration based on response headers.The code now respects the
max-agedirective from the response's Cache-Control header, allowing the server to control cache duration. Thettlis properly stored for Age header calculations.Also applies to: 271-271
303-314: Well-implementedparseMaxAgehelper function.The function correctly:
- Handles comma-separated directives
- Is case-insensitive
- Properly extracts and validates the max-age value
- Returns a boolean to indicate success
This implementation follows the Cache-Control header format specified in RFC 7234.
middleware/cache/cache_test.go (4)
1022-1038: LGTM! Excellent test for no-store directive compliance.This test correctly validates that responses with
Cache-Control: no-storeare not cached, which is essential for RFC compliance. The test properly verifies that both the initial and subsequent requests returncacheUnreachablestatus, ensuring the middleware respects the no-store directive.
1040-1055: LGTM! Important test for Cache-Control header preservation.This test correctly validates that when
StoreResponseHeadersis enabled, manually setCache-Controlheaders are preserved and not overwritten by the middleware. This is crucial for maintaining application-level control over caching directives while still benefiting from the cache middleware functionality.
1057-1074: LGTM! Proper validation of max-age directive behavior.This test correctly validates that the cache middleware respects the
max-agedirective from response headers, overriding the default expiration time. The test properly demonstrates that a cache entry withmax-age=1expires after 1 second, even when the default expiration is 10 seconds. This is essential for RFC compliance.
1076-1105: Excellent comprehensive unit test for parseMaxAge function.This table-driven test thoroughly validates the
parseMaxAgehelper function with excellent coverage:
- Valid Cases: Basic parsing, mixed directives, case insensitivity, zero values
- Invalid Cases: Missing values, malformed headers, non-numeric values
- Edge Cases: Proper handling of multiple directives and whitespace
- Best Practices: Uses
t.Runfor subtests and parallel executionThe test design ensures the parsing function correctly handles various real-world Cache-Control header formats while properly rejecting invalid input.
There was a problem hiding this comment.
⚠️ 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: 6e34870 | Previous: 557095b | Ratio |
|---|---|---|---|
Benchmark_GenericParseTypeBytes/benchmark_genericParseTypeBytes#03 |
24.97 ns/op 0 B/op 0 allocs/op |
12.78 ns/op 0 B/op 0 allocs/op |
1.95 |
Benchmark_GenericParseTypeBytes/benchmark_genericParseTypeBytes#03 - ns/op |
24.97 ns/op |
12.78 ns/op |
1.95 |
This comment was automatically generated by workflow using github-action-benchmark.
Summary
Ageheader