Fix cache misses with content negotiation#5623
Open
Matteo Mazza (matteomazza91) wants to merge 2 commits into
Open
Fix cache misses with content negotiation#5623Matteo Mazza (matteomazza91) wants to merge 2 commits into
Matteo Mazza (matteomazza91) wants to merge 2 commits into
Conversation
…otiation is installed Three tests demonstrating that HttpCache misses the cache for every request when ContentNegotiation is installed and the response includes a Vary header. Root cause: varyKeys() stores multi-value headers joined with "," while mergedHeadersLookup() joins them with ";". When ContentNegotiation appends a second Accept value the stored "a,b" never equals the looked-up "a;b", so findResponse() always returns null. - varyKeysStoredWithCommaSeparatorButMergedHeadersLookupUsesSemicolon: fails, directly showing the separator divergence (expected to fail before the fix) - freshCacheableEntryWithVaryAcceptIsNeverServedFromCacheWhenContentNegotiationIsInstalled: fails, serverCallCount==2 instead of 1 - freshCacheableEntryWithVaryAcceptIsServedFromCacheWhenContentNegotiationIsNotInstalled: passes, confirms the cache works with a single Accept value
Contributor
📝 WalkthroughWalkthroughNormalizes multi-value header joining (RFC 7230) and aligns cache lookup/validation: adds joinHeaderValues(), updates varyKeys() and mergedHeadersLookup(), and adds tests plus ContentNegotiation test dependency to ensure consistent Vary: Accept cache behavior. ChangesCache Vary header normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ktor-client/ktor-client-core/common/test/HttpCacheTest.kt (1)
233-257: 💤 Low valueConsider renaming test to describe expected behavior rather than the bug.
The test name
freshCacheableEntryWithVaryAcceptIsNeverServedFromCacheWhenContentNegotiationIsInstalleddescribes the bug behavior (never served from cache), but the assertionassertEquals(1, serverCallCount)verifies the fix (cache hit occurs). This creates confusion about what the test is validating.Consider renaming to reflect the expected behavior after the fix, for example:
freshCacheableEntryWithVaryAcceptIsServedFromCacheWhenContentNegotiationIsInstalledcacheHitOccursWithContentNegotiationAndVaryAcceptAlso, the comment at lines 252-254 uses "Actual:" to describe the old buggy behavior, which is confusing since "actual" typically refers to the current test result. Consider rephrasing to "Before fix:" or "Previously:".
🤖 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 `@ktor-client/ktor-client-core/common/test/HttpCacheTest.kt` around lines 233 - 257, Rename the test function freshCacheableEntryWithVaryAcceptIsNeverServedFromCacheWhenContentNegotiationIsInstalled to reflect the expected/verified behavior (e.g. freshCacheableEntryWithVaryAcceptIsServedFromCacheWhenContentNegotiationIsInstalled or cacheHitOccursWithContentNegotiationAndVaryAccept) and update the inline comment that currently reads "Actual:" to something like "Before fix:" or "Previously:" so the comment no longer conflicts with the assertion that serverCallCount == 1; adjust occurrences where the test name is referenced (e.g., test runners) so the symbol change is consistent.
🤖 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 `@ktor-client/ktor-client-core/common/test/HttpCacheTest.kt`:
- Around line 233-257: Rename the test function
freshCacheableEntryWithVaryAcceptIsNeverServedFromCacheWhenContentNegotiationIsInstalled
to reflect the expected/verified behavior (e.g.
freshCacheableEntryWithVaryAcceptIsServedFromCacheWhenContentNegotiationIsInstalled
or cacheHitOccursWithContentNegotiationAndVaryAccept) and update the inline
comment that currently reads "Actual:" to something like "Before fix:" or
"Previously:" so the comment no longer conflicts with the assertion that
serverCallCount == 1; adjust occurrences where the test name is referenced
(e.g., test runners) so the symbol change is consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 00797568-3f25-4fae-8a1e-089603bf9e98
📒 Files selected for processing (4)
ktor-client/ktor-client-core/build.gradle.ktsktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheEntry.ktktor-client/ktor-client-core/common/test/HttpCacheTest.kt
…ry.varyKeys() mergedHeadersLookup() joined multi-value headers with ";" while varyKeys() joined them with ",", so findResponse() always discarded cached entries for requests with repeated header values (e.g. when ContentNegotiation appends a second Accept value). Extract joinHeaderValues() (RFC 7230 §3.2.2 comma separator) and use it in both places so the stored and looked-up strings are always identical.
Matteo Mazza (matteomazza91)
force-pushed
the
fix-cache-misses-with-content-negotiation
branch
from
370cfd9 to
3b02ff7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ktor-client/ktor-client-core/common/test/HttpCacheTest.kt (1)
210-210: ⚡ Quick winUse descriptive backticked test names to match repo test style.
These new test names should be converted to backticked descriptive names per the test-file guideline.
As per coding guidelines: "
**/*Test.{kt,kts}: Prefer descriptive test names in backticks:describe what is being tested".Also applies to: 233-233, 263-263
🤖 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 `@ktor-client/ktor-client-core/common/test/HttpCacheTest.kt` at line 210, Rename the test function varyKeysSeparatorMatchesMergedHeadersLookupSeparator to a descriptive backticked test name (e.g. `describe what is being tested`) to follow the file's test naming style; likewise update the two other tests called out in the review to use backticked descriptive names instead of plain function identifiers so all tests in HttpCacheTest.kt follow the "`describe what is being tested`" convention.
🤖 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.
Inline comments:
In `@ktor-client/ktor-client-core/common/test/HttpCacheTest.kt`:
- Around line 210-225: The test
varyKeysSeparatorMatchesMergedHeadersLookupSeparator is tautological because
both stored and lookup are produced with joinHeaderValues(), so change the
assertions to compare each result against the exact RFC-compliant header-list
string instead of against each other: compute expected as the exact string (e.g.
"application/vnd.github+json, application/json" per RFC header-list spacing),
then assertEquals(expected, stored) and assertEquals(expected, lookup);
reference HttpCacheEntry.varyKeys(), mergedHeadersLookup(), and
joinHeaderValues() to locate the code to update.
---
Nitpick comments:
In `@ktor-client/ktor-client-core/common/test/HttpCacheTest.kt`:
- Line 210: Rename the test function
varyKeysSeparatorMatchesMergedHeadersLookupSeparator to a descriptive backticked
test name (e.g. `describe what is being tested`) to follow the file's test
naming style; likewise update the two other tests called out in the review to
use backticked descriptive names instead of plain function identifiers so all
tests in HttpCacheTest.kt follow the "`describe what is being tested`"
convention.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac9cea0d-4388-457a-b536-326cad77ba6c
📒 Files selected for processing (3)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.ktktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheEntry.ktktor-client/ktor-client-core/common/test/HttpCacheTest.kt
✅ Files skipped from review due to trivial changes (1)
- ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheEntry.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt
Comment on lines
+210
to
+225
| fun varyKeysSeparatorMatchesMergedHeadersLookupSeparator() { | ||
| val requestHeaders = Headers.build { | ||
| append(HttpHeaders.Accept, "application/vnd.github+json") // set by caller | ||
| append(HttpHeaders.Accept, "application/json") // appended by ContentNegotiation | ||
| } | ||
|
|
||
| // How HttpCacheEntry.varyKeys() stores the value (joinHeaderValues → comma): | ||
| val stored = requestHeaders.getAll(HttpHeaders.Accept).joinHeaderValues() | ||
| // → "application/vnd.github+json,application/json" | ||
|
|
||
| // How mergedHeadersLookup() now computes the lookup value (joinHeaderValues → comma): | ||
| val lookup = requestHeaders.getAll(HttpHeaders.Accept).joinHeaderValues() | ||
| // → "application/vnd.github+json,application/json" | ||
|
|
||
| assertEquals(stored, lookup) | ||
| } |
Contributor
There was a problem hiding this comment.
Regression assertion is currently tautological and can miss separator regressions.
stored and lookup are both built via the same joinHeaderValues() call, so this test can still pass if both paths regress to the same wrong separator. Assert the exact expected RFC-compliant value to make this test fail on separator drift.
Suggested patch
fun varyKeysSeparatorMatchesMergedHeadersLookupSeparator() {
val requestHeaders = Headers.build {
append(HttpHeaders.Accept, "application/vnd.github+json") // set by caller
append(HttpHeaders.Accept, "application/json") // appended by ContentNegotiation
}
// How HttpCacheEntry.varyKeys() stores the value (joinHeaderValues → comma):
val stored = requestHeaders.getAll(HttpHeaders.Accept).joinHeaderValues()
- // → "application/vnd.github+json,application/json"
+ val expected = "application/vnd.github+json,application/json"
// How mergedHeadersLookup() now computes the lookup value (joinHeaderValues → comma):
val lookup = requestHeaders.getAll(HttpHeaders.Accept).joinHeaderValues()
- // → "application/vnd.github+json,application/json"
-
+
+ assertEquals(expected, stored)
+ assertEquals(expected, lookup)
assertEquals(stored, lookup)
}🤖 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 `@ktor-client/ktor-client-core/common/test/HttpCacheTest.kt` around lines 210 -
225, The test varyKeysSeparatorMatchesMergedHeadersLookupSeparator is
tautological because both stored and lookup are produced with
joinHeaderValues(), so change the assertions to compare each result against the
exact RFC-compliant header-list string instead of against each other: compute
expected as the exact string (e.g. "application/vnd.github+json,
application/json" per RFC header-list spacing), then assertEquals(expected,
stored) and assertEquals(expected, lookup); reference HttpCacheEntry.varyKeys(),
mergedHeadersLookup(), and joinHeaderValues() to locate the code to update.
Contributor
Author
There was a problem hiding this comment.
it's not important that the serialization complies with the RFC as this is not sent on the network. It's just an internal representation.
stored and lookup are both built via the same joinHeaderValues() call
that's the intention of the PR. To avoid an implicit dependency of 2 pieces of code that needs to be serialized with the same format. The new function forces an explicit dependency.
Contributor
There was a problem hiding this comment.
@matteomazza91, that makes sense — the test is intentionally documenting the shared dependency on joinHeaderValues(), making it explicit that both varyKeys() and mergedHeadersLookup() must use the same function rather than two independent inline joins that could silently drift. The integration tests (cacheHitOccursWithContentNegotiationAndVaryAccept) provide the behavioral safety net if joinHeaderValues() itself ever regresses. Understood and acknowledged!
(ᵔᴥᵔ)🐇
🧠 Learnings used
Learnt from: bjhham
Repo: ktorio/ktor PR: 5345
File: ktor-server/ktor-server-core/common/src/io/ktor/server/routing/RouteSelector.kt:694-699
Timestamp: 2026-02-05T10:32:08.803Z
Learning: In Kotlin sources, avoid annotating declarations with InternalAPI when they already have internal visibility, because internal already restricts access within the module. Apply this guidance across all Kotlin files (pattern **/*.kt). If a declaration is intended to be part of a public or exported API, or if InternalAPI conveys additional semantics beyond visibility, consider keeping the annotation and documenting the rationale.
Learnt from: nomisRev
Repo: ktorio/ktor PR: 5449
File: ktor-server/ktor-server-core/common/src/io/ktor/server/config/ApplicationConfig.kt:160-179
Timestamp: 2026-03-18T08:26:32.639Z
Learning: In the ktorio/ktor repository, apply a minimal KDoc style guideline for public API extension functions: include a brief description and a [Report a problem] link, and do not require explicit param, return, or throws tags. Do not flag missing KDoc param/return/throws as issues in this project; this guideline should apply to Kotlin source files across the codebase where public API surface is documented.
Learnt from: osipxd
Repo: ktorio/ktor PR: 5527
File: ktor-server/ktor-server-netty/jvm/src/io/ktor/server/netty/NettyApplicationEngine.kt:187-187
Timestamp: 2026-05-06T08:59:44.186Z
Learning: In the ktorio/ktor repository, prefer kotlin.time.Duration over Long for time/duration-based parameters in all new public Kotlin APIs (e.g., timeout, delay, interval). Apply this guideline to all Kotlin source files (pattern **/*.kt). When a call site must pass milliseconds to Java/Netty APIs, convert using .inWholeMilliseconds. Update existing API signatures accordingly and add tests to cover Duration usage and the conversion boundary where needed.
Contributor
Author
test names are matching the style of the file where they've been added |
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.
Subsystem
Client, HttpCache
Motivation
When a
Varyheader contains multiple values, requests always generate cache-misses andIf-None-Match/If-Modified-Sinceare never sentSolution
Centralize multi-values header serialization in one place to force the same serialization approach (i.e. use
,instead of;)Use Case
When
HttpCacheandContentNegotiationare both installed, responses withAcceptin theVaryheader (e.g.Vary: Accept) are never served from cache if the client specifies a specific Accept header.e.g. to make requests to github, the client should use
Accept: application/vnd.github+jsonbut the ContentNegotiation plugin also unconditionally appends anAcceptfor every registered codec:registrations.forEach { request.accept(it.contentTypeToSend) }When the caller also sets
Accept(e.g.accept(ContentType.parse("application/vnd.github+json"))), the request ends up with twoAcceptvalues. The stored varyKey and the lookup key then differ only in separator:joinToString(","))"application/vnd.github+json,application/json"joinToString(";"))"application/vnd.github+json;application/json"Ktor version
Reproduced on 3.5.0-dev (main branch as of 2026-05-15) and 2.3.7.
In 2.3.7:
varyKeys()stored values usingHeaders.get()(first value only) whilemergedHeadersLookupusedgetAll().joinToString(";"). An earlier fix changedget()togetAll().joinToString(",")but left the separator inmergedHeadersLookupas";", so the mismatch persists.Platform / engine
JVM — the bug is in
ktor-client-core, not engine-specific.Steps to reproduce
Option A — Checkout the failing tests from this fork
Expected: 2 tests fail (the unit test and the integration test), 1 passes (the counterpart without
ContentNegotiation).Expected behaviour
The second GET should be served from in-memory cache (
max-age=60has not expired).serverCallCount == 1.Actual behaviour
serverCallCount == 2. The cache entry is stored after the first request but never matched on subsequent requests.