Correct response caching of origin-dependent and faulted responses #12733#12734
Open
yasmoradi wants to merge 1 commit into
Open
Correct response caching of origin-dependent and faulted responses #12733#12734yasmoradi wants to merge 1 commit into
yasmoradi wants to merge 1 commit into
Conversation
WalkthroughThe Boilerplate template disables origin-dependent sign-in caching, refines WebAuthn assertion cache handling, strengthens response-cache variation and cacheability rules, updates route markup and culture-prefix removal, normalizes cache eviction tags, and limits FusionCache auto-cloning to development. ChangesBoilerplate corrections
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
yasmoradi
changed the title
yasmoradi
force-pushed
the
fix/response-cache-origin-and-faulted-responses
branch
from
July 21, 2026 10:00
9b21bde to
8399f54
Compare
yasmoradi
force-pushed
the
fix/response-cache-origin-and-faulted-responses
branch
from
July 21, 2026 10:56
8399f54 to
3bac8a8
Compare
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/ResponseCacheService.cs (1)
22-27: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard against empty arrays and ensure cache tags match the absolute path format.
If
PurgeCacheis invoked without arguments, the resulting empty array passed to the Cloudflare API ({"files":[]}) can cause a400 Bad Requestand throw an exception.Additionally, because the output-cache tagging uses
Uri.AbsolutePath(which always begins with a/), anyrelativePathpassed without a leading slash will fail to match and won't be evicted.Consider adding early returns and formatting safeguards to make this method foolproof.
♻️ Proposed refactor
public async Task PurgeCache(params string[] relativePaths) { + if (relativePaths is null || relativePaths.Length == 0) + return; + foreach (var relativePath in relativePaths) { - await outputCacheStore.EvictByTagAsync(relativePath.ToLowerInvariant(), default); + if (string.IsNullOrWhiteSpace(relativePath)) + continue; + + var tag = relativePath.StartsWith('/') ? relativePath.ToLowerInvariant() : $"/{relativePath}".ToLowerInvariant(); + await outputCacheStore.EvictByTagAsync(tag, default); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/ResponseCacheService.cs` around lines 22 - 27, Update ResponseCacheService.PurgeCache to return immediately when relativePaths is null or empty, avoiding an empty cache eviction request. Before calling outputCacheStore.EvictByTagAsync, normalize each path to match Uri.AbsolutePath by ensuring it has a leading slash, then apply the existing lowercase normalization.src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Shared/Infrastructure/Services/AppResponseCachePolicy.cs (1)
35-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the indexer instead of
.Addto assign dictionary values.Using
.Addon theVaryByValuesdictionary could throw anArgumentExceptionif the key already exists (e.g., if it was pre-populated by a controller attribute or if the policy inadvertently runs twice). Using the indexer is a slightly safer and more robust approach.♻️ Proposed refactor
if (CultureInfoManager.InvariantGlobalization is false) { - context.CacheVaryByRules.VaryByValues.Add("Culture", CultureInfo.CurrentUICulture.Name); + context.CacheVaryByRules.VaryByValues["Culture"] = CultureInfo.CurrentUICulture.Name; } //#if (multitenant == true) // An authenticated request resolves its tenant from the user's claim rather than from the host (See TenantProvider), // and tenant scoped entities are filtered by that tenant (See AppDbContext.ConfigureTenantAwareEntity). Without this // rule two users of different tenants on the same host would share a single entry, so a UserAgnostic endpoint like // ProductViewController would serve one tenant's rows to another. if (context.HttpContext.User.GetTenantId() is Guid currentTenantId) { - context.CacheVaryByRules.VaryByValues.Add("Tenant", currentTenantId.ToString()); + context.CacheVaryByRules.VaryByValues["Tenant"] = currentTenantId.ToString(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Shared/Infrastructure/Services/AppResponseCachePolicy.cs` around lines 35 - 48, Replace the VaryByValues.Add calls in the response cache policy with dictionary indexer assignments for the "Culture" and "Tenant" keys. Update the existing assignments within the CultureInfoManager.InvariantGlobalization and currentTenantId branches while preserving their current values and conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/ResponseCacheService.cs`:
- Around line 22-27: Update ResponseCacheService.PurgeCache to return
immediately when relativePaths is null or empty, avoiding an empty cache
eviction request. Before calling outputCacheStore.EvictByTagAsync, normalize
each path to match Uri.AbsolutePath by ensuring it has a leading slash, then
apply the existing lowercase normalization.
In
`@src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Shared/Infrastructure/Services/AppResponseCachePolicy.cs`:
- Around line 35-48: Replace the VaryByValues.Add calls in the response cache
policy with dictionary indexer assignments for the "Culture" and "Tenant" keys.
Update the existing assignments within the
CultureInfoManager.InvariantGlobalization and currentTenantId branches while
preserving their current values and conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 222e6c59-1c0c-4696-9c7f-0c6d55d88560
📒 Files selected for processing (7)
src/Templates/Boilerplate/Bit.Boilerplate/src/Client/Boilerplate.Client.Core/Components/Routes.razorsrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.WebAuthn.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Infrastructure/Services/ResponseCacheService.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Shared/Infrastructure/Extensions/WebApplicationBuilderExtensions.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Shared/Infrastructure/Services/AppResponseCachePolicy.cssrc/Templates/Boilerplate/Bit.Boilerplate/src/Shared/Infrastructure/Extensions/UriExtensions.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.ExternalSignIn.cs
- src/Templates/Boilerplate/Bit.Boilerplate/src/Server/Boilerplate.Server.Api/Features/Identity/IdentityController.WebAuthn.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #12733
1.
GetExternalSignInUriis no longer shared-cacheableThe URL this action returns embeds
origin = Request.GetWebAppUrl(), andGetWebAppUrl()falls back to theX-Originrequest header when the origin is not in the query string.AppResponseCachePolicyvaries by host, query keys and culture, soX-Originis in neither the output cache's vary key nor a CDN's cache key, and a stored response did not depend only on the inputs it was keyed by.The
AppResponseCacheattribute is simply removed. Caching buys little here, and a vary rule would not be a real fix since CDNs do not honourVaryon arbitrary headers reliably.2. A response the output cache refuses to store no longer advertises shared caching
CacheRequestAsyncwritesCache-Controlbefore the endpoint runs.ServeResponseAsyncalready setAllowCacheStorage = falsefor non-200 responses and for responses carrying a non-cultureSet-Cookie, but left that header untouched, so CDNs and browsers kept caching a response ASP.NET Core had just declined to store. A 404 from/api/v1/ProductView/Get/{id}went out withpublic, s-maxage=604800, leaving a stale 404 on the edge for up to seven days after the product was created.The header is now downgraded to
no-store, privatealongsideAllowCacheStorage.The
response.HasStarted is falseguard matters:ServeResponseAsyncalso runs for streamed responses, whose headers are already flushed and read-only — without it the middleware throwsInvalidOperationException: Headers are read-only, response has already started. That path needs no downgrade anyway, since enabling shared caching is exactly what suppresses streaming pre-rendering (seeHttpRequestExtensions.IsStreamPrerenderingSuppressed).Also included
IdentityController.WebAuthnassertion options:new string(challenge.Select(b => (char)b)), putting NUL and control characters into the Redis key. It is now base64url encoded.SignInthrows on an invalid two-factor code, so a failed attempt still leaves the options in place for a retry.Verification
dotnet buildonBoilerplate.Server.Web— succeeds, no new warnings.dotnet test --filter "TestCategory=Caching"— passes.dotnet test --filter "TestCategory=SEO"— 3/3 pass.Prerendering_WithOutputCaching_Should_ReturnCompleteNonStreamedHomePageis what caught the missingHasStartedguard.Summary by CodeRabbit