Add in-memory caching delegating handler to Boilerplate (#9897)#9900
Add in-memory caching delegating handler to Boilerplate (#9897)#9900msynk merged 20 commits intobitfoundation:developfrom yasmoradi:9897
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes modify both client and server projects with a focus on caching. On the client side, a new Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant CDH as CacheDelegatingHandler
participant BH as Base Handler
C->>CDH: Send HTTP Request
alt Cache Hit
CDH->>C: Return Cached Response
else Cache Miss
CDH->>BH: Forward Request
BH->>CDH: Return New Response
CDH->>C: Return Response (and cache it)
end
Poem
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 (
|
![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)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/CacheDelegatingHandler.cs (1)
65-73: Audit the size and content of cached response items.Caching the entire response content, headers, and
LogScopeDatacan be large or contain sensitive data. Confirm memory usage bounds and potential data leakage in multi-tenant scenarios. Also, consider making the classsealedif inheritance is not needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/CacheDelegatingHandler.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/HttpMessageHandlersChainFactory.cs(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/AppResponseCachePolicy.cs(4 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.Services.cs(0 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Services/AppResponseCachePolicy.cs(4 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Boilerplate.Shared.csproj(1 hunks)src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/ISharedServiceCollectionExtensions.cs(1 hunks)
💤 Files with no reviewable changes (2)
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Program.Services.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Program.Services.cs
✅ Files skipped from review due to trivial changes (1)
- src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/HttpMessageHandlersChainFactory.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build and test
🔇 Additional comments (13)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/CacheDelegatingHandler.cs (3)
6-8: Constructor injection and handler chaining look good.Using constructor injection to obtain
IMemoryCacheand chaining to anotherHttpMessageHandleris a clean approach. Theinternalaccess modifier is sensible if usage is strictly within this assembly.
9-19: Ensure that sensitive data isn’t stored in logs or cache.The
logScopeDatadictionary and subsequentlogScopeDatausage may contain user-specific or private information. Verify that no personally identifiable or sensitive data is inadvertently cached and shared across requests.
41-63: Avoid caching non-GET or user-specific responses.Currently, any successful request with a suitable
CacheControlheader is cached, regardless of HTTP method or user content. Some endpoints (e.g., POST, PUT) might not be safe or desirable to cache. Double-check whether restricting caching to GET or idempotent requests is necessary to prevent stale or incorrect responses for other request methods.src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Extensions/ISharedServiceCollectionExtensions.cs (1)
43-43: In-memory cache registration is appropriate.Registering memory caching here is consistent with the new caching features. This ensures that
IMemoryCacheis readily available for theCacheDelegatingHandlerand other caching needs.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Web/Services/AppResponseCachePolicy.cs (4)
38-40: Renaming to clientCacheTtl clarifies caching scope.Replacing
browserCacheTtlwithclientCacheTtlmore accurately reflects multi-platform clients. No issues identified with the rename itself.
62-64: Disabling edge/client caching for pre-rendered pages is consistent.This aligns with the comment about not supporting culture-specific variations on edge/client caches. The logic remains coherent with the existing code flow.
73-81: Conditional client/edge cache directives look good.The code properly updates the response headers with either
MaxAgeorSharedMaxAgebased onclientCacheTtlandedgeCacheTtl. This approach is consistent with the revised nomenclature.
95-95: Verify exposure of caching details in response headers.The header
"App-Cache-Response"reveals caching statuses (Output,Edge, andClient) to the client. If this is an intentional transparency feature, it’s fine; otherwise, consider removing or restricting it.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Services/AppResponseCachePolicy.cs (2)
38-38: LGTM! Consistent terminology update from 'browser' to 'client' cache.The renaming provides better clarity as the caching can occur at any client level, not just browsers.
Also applies to: 65-65, 99-99
62-63:Details
❓ Verification inconclusive
Verify the comment about cache variations.
The comment states that edge and client caches don't support culture variations. Please verify this limitation is still accurate with your current caching implementation.
🌐 Web query:
Do HTTP caches (CDN/edge/client) support variations by Accept-Language or custom culture headers?💡 Result:
HTTP caches (CDN/edge/client) can support content variations based on
Accept-Languageor custom culture headers through mechanisms like theVaryheader and vendor-specific implementations. Here’s how different caching layers handle this:
1. Standard
VaryHeader SupportMost HTTP caches use the
Varyheader to determine whether a cached response matches a new request based on specified request headers:
Accept-Language:
Supported by most CDNs (e.g., Fastly [1], Google Cloud CDN [5], and Varnish [4]) and browsers when paired withVary: Accept-Language. For example:Vary: Accept-LanguageThis ensures separate cached versions for different language preferences.
Custom Headers:
Possible viaVary: X-Custom-Header, but many CDNs restrict high-cardinality headers (e.g.,User-Agent,Cookie) due to cache fragmentation risks. For example:Vary: X-CultureGoogle Cloud CDN explicitly blocks certain headers ([5]), while Netlify allows custom rules via
Netlify-Vary([2]).
2. Browser Caching Behavior
Browsers treat
Varyas a validator rather than storing multiple variants:
- Only one variant per URL is cached (e.g., storing responses for
en-USbut notfr-FRsimultaneously) ([1]).- Ignored for HTTP/2 Server Push resources ([1]).
3. CDN/Edge-Specific Implementations
Netlify
Supports granular control via
Netlify-Vary, enabling variations by:
- Language groups:
Netlify-Vary: language=en|de- Country groups:
Netlify-Vary: country=us|ca+mx- Custom query/headers/cookies ([2]).
Google Cloud CDN
Restricts high-cardinality headers by default but allows:
- Custom cache keys via
includeHeadersconfiguration.- Overrides using
FORCE_CACHE_ALLmode (ignores origin’sCache-Controlbut respectsVary) ([5]).Cloudflare
Historically lacked full
Varysupport ([3]), but recent updates may offer limited implementations (not covered in sources).
4. Best Practices
- Use standard headers like
Accept-Languageinstead of custom ones where possible.- Avoid overusing
Vary: Multiple variants reduce cache efficiency.- For multilingual sites:
- Prefer URL-based localization (
/en/page,/fr/page) for better caching.- Use
KeyorVariantsproposals (experimental) for smarter variation grouping ([1]).
Summary Table
Cache Type Supports Accept-Language?Supports Custom Headers? Notes Browsers Yes (as validator) Limited Stores only one variant per URL ([1]). Fastly Yes Yes Uses standard Varylogic ([1]).Netlify Yes Yes Customizable via Netlify-Vary([2]).Google Cloud CDN Partial Partial Blocks high-cardinality headers by default; configurable ([5]). Cloudflare Limited Limited Partial/no native support historically; check latest docs ([3]). For dynamic content requiring culture-specific variations, combine HTTP caching with edge/serverless logic or use frameworks like Next.js that automate header-based caching optimizations ([2][7]).
Citations:
- 1: https://www.fastly.com/blog/understanding-vary-header-browser
- 2: https://www.netlify.com/blog/netlify-cache-key-variations/
- 3: https://community.cloudflare.com/t/cloudflare-cdn-cache-to-support-http-vary-header/160802
- 4: https://www.varnish-software.com/developers/tutorials/http-caching-basics/
- 5: https://cloud.google.com/cdn/docs/caching
- 6: https://simonhearne.com/2022/caching-header-best-practices/
- 7: https://hackernoon.com/the-6-core-concepts-for-http-caching
- 8: https://www.nexcess.net/help/manage-your-edge-cdn-cache-website-expiry-date-values/
- 9: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control
- 10: https://dev.to/jareechang/http-caching-the-6-core-concepts-5agd
Action: Revisit our caching strategy regarding culture-specific variations.
It turns out that most HTTP caches—especially CDN and edge caches—can support content variations based on headers like
Accept-Language(and even custom headers) when configured correctly (e.g., via theVaryheader). However, note that browser caches generally store a single variant per URL regardless of the Vary header.
- CDN/Edge Caching: Modern CDNs such as Fastly, Google Cloud CDN, and Netlify can be configured to accommodate such variations. This suggests that the blanket statement about edge caches not supporting culture variations may no longer apply universally.
- Client (Browser) Caching: Browsers typically cache only one variant per URL, so the limitation for client-side caching remains valid.
Given these nuances, please verify that our current configuration for pre-rendered pages correctly reflects these realities. If our CDN is set up to handle Vary-based variations, it might be beneficial to re-enable edge caching (while continuing to treat browser caching with caution) so as to fully leverage caching across layers.
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Extensions/IClientCoreServiceCollectionExtensions.cs (2)
85-85: LGTM! Appropriate service registration.Registering CacheDelegatingHandler as scoped ensures each HTTP client scope gets its own handler instance.
76-81:Details
❓ Verification inconclusive
Verify the caching handler's position in the chain.
The CacheDelegatingHandler is positioned after logging but before auth/retry/exception handling, which is a good order as it:
- Ensures cache operations are logged
- Can serve cached responses before hitting auth/retry logic
- Avoids caching error responses
However, please verify that this order doesn't cache unauthorized responses.
🏁 Script executed:
#!/bin/bash # Search for cache-related logic in CacheDelegatingHandler ast-grep --pattern 'class CacheDelegatingHandler { $$$ SendAsync($_, $_) { $$$ } }'Length of output: 211
Verify Caching Behavior in Handler Chain
The ordering in the
IClientCoreServiceCollectionExtensionslooks sound—placing theCacheDelegatingHandlerafter logging and before auth/retry/exception handling enables logging of cache operations and early serving of cached responses. However, due to non-conclusive automated output (an error reading a non-UTF8 file interfered with our script), please manually verify the implementation ofCacheDelegatingHandler.SendAsyncto ensure that unauthorized responses (or other error statuses) are not cached.
- Action Required:
- Manually inspect the
SendAsyncmethod inCacheDelegatingHandlerto confirm that caching is bypassed for unauthorized (401) responses or other error cases, if applicable.- Ensure that caching behavior aligns with the intended order in the handler chain.
src/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Boilerplate.Shared.csproj (1)
11-11:Details
❓ Verification inconclusive
Verify the Microsoft.Extensions.Caching.Memory package version.
The package is correctly added for in-memory caching support. Since no version is specified, please verify that the central package management uses a compatible version.
🏁 Script executed:
#!/bin/bash # Search for central package version management file fd --type f "Directory.Packages.props" --exec cat {} \; | grep -A 5 "Microsoft.Extensions.Caching.Memory"Length of output: 106
Action Required: Verify Central Package Version Configuration for Microsoft.Extensions.Caching.Memory
The automated search did not find any version information for the Microsoft.Extensions.Caching.Memory package in the central package management file (Directory.Packages.props). Please manually verify whether a compatible version is defined in your central package configuration. If no version is managed centrally, consider specifying an explicit version in the project file to ensure consistent caching behavior.
...te/src/Client/Boilerplate.Client.Core/Services/HttpMessageHandlers/CacheDelegatingHandler.cs
Show resolved
Hide resolved
2 participants
closes #9897
Summary by CodeRabbit
New Features
Refactor
Chores