Ktor 8345: Make VaryHeader check lenient #4816
Conversation
And log the problem so that users are aware of it.
WalkthroughThis change updates the HTTP cache handling logic in the Ktor client library. The cache lookup mechanism is modified to relax the requirement that the number of Changes
✨ 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:
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt(1 hunks)ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/UnlimitedCacheStorage.kt(2 hunks)ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.kt(1 hunks)
🔇 Additional comments (3)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/storage/UnlimitedCacheStorage.kt (2)
26-26: Makes HTTP cache lookup mechanism more lenientThis change relaxes the Vary header validation by removing the requirement that the number of
varyKeysmust exactly match. Now a cached entry will be considered a match as long as all the requested keys match, even if the cached entry contains additional keys.This aligns with the PR objectives to improve developer experience when dealing with servers that don't strictly follow the HTTP specification for Vary headers.
48-48: Consistent implementation of the lenient cache matching logicThis change correctly applies the same lenient matching logic to the second implementation of the cache lookup mechanism, ensuring consistent behavior.
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt (1)
346-352:✅ Verification successful
Add helpful logging for Vary header size mismatch
This change adds informative logging when a Vary header size mismatch is detected, enhancing the developer experience by clearly explaining:
- That the mismatch is a violation of the specification
- That the library will return the best matching cache despite the mismatch
- That the issue should be reported to the server team
This implements the "improved logging" part of the PR objectives while maintaining backward compatibility.
🏁 Script executed:
#!/bin/bash # Check if the default logging level would show this message rg --type kotlin "val LOGGER" ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt -A 1 # Check if there are similar messages elsewhere that use a different log level rg "LOGGER\.info.*Vary header" --type kotlinLength of output: 510
Confirm INFO-level logging for Vary header mismatch
The new
LOGGER.infocall in HttpCache.kt will be visible by default, sinceKtorSimpleLoggerdefaults to INFO. This change clearly:
- Notes the Vary‐header spec violation
- Explains that the best matching cache is still returned
- Advises reporting the server’s incorrect behavior
LGTM.
| val first = client.get("$TEST_SERVER/cache/different-vary") { | ||
| header("200", "true") | ||
| header("Set-Vary", "X-Requested-With,Accept-Encoding") | ||
| } | ||
| assertFailsWith<InvalidCacheStateException> { | ||
| client.get("$TEST_SERVER/cache/different-vary") { | ||
| header("Set-Vary", "X-Requested-With") | ||
| } | ||
| } | ||
| }.body<String>() | ||
| val second = client.get(url) { | ||
| header("Set-Vary", "X-Requested-With") | ||
| }.body<String>() | ||
|
|
||
| assertEquals(first, second) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Test updated to verify lenient Vary header behavior
The test has been correctly updated to verify the new lenient behavior. Instead of expecting an exception when Vary headers differ, it now confirms that the response is successfully returned from cache even with different Vary headers.
This change properly validates the changes made to the cache lookup mechanism.
🏁 Script executed:
#!/bin/bash
# Check if the 'url' variable referenced in line 853 is defined somewhere
# or if it should be a direct reference to the URL used in the first request
rg --type kotlin "val url " -A 2 ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.ktLength of output: 2169
🏁 Script executed:
#!/bin/bash
# Locate the definition of `different-vary` to confirm that `val url` points to the same endpoint
rg --type kotlin "different-vary" -n -C 3 ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.ktLength of output: 401
Define url before using it in the different-vary test
The second client.get(url) call references an undefined url variable, which will lead to a compilation error. Introduce a val url = "$TEST_SERVER/cache/different-vary" (or inline the same literal) before your requests so both calls use the same endpoint.
• File: CacheTest.kt, inside the different-vary test block (around lines 849–858)
• Suggested change:
test { client ->
+ val url = "$TEST_SERVER/cache/different-vary"
val first = client.get(url) {
header("200", "true")
header("Set-Vary", "X-Requested-With,Accept-Encoding")
}.body<String>()
val second = client.get(url) {
header("Set-Vary", "X-Requested-With")
}.body<String>()
assertEquals(first, second)
}Committable suggestion skipped: line range outside the PR's diff.
|
I haven't looked into the code at all, so I don't know if this is possible. But, one solution might be to log and catch the exception, then retry the request without any conditional headers like |
|
@JohannesTrussellRasch yep, that might be a solution. But also a bit overkill I think 🤔. But still I think ktor should be more lenient here. |
Subsystem
Client, Caching
Motivation & Solution
This PR fixes https://youtrack.jetbrains.com/issue/KTOR-8345/HttpCache-InvalidCacheStateException-thrown-when-Vary-header-has-different-entries-is-overly-severe
Let me explain:
All starts with this.
Someone requested to update cache in case the Vary header differs.
Actually this violates the Vary header spec.
See https://developer.mozilla.org/de/docs/Web/HTTP/Reference/Headers/Vary
To make Ktor more compliant with the specs, this PR was opened.
The change leads to throwing of an
InvalidCacheStateExceptionin case the Vary header differs from the original response (200) to the second response (304).E.g.
GET 200 -> Vary: Accept-Encoding, Language
GET 304 -> Vary: Accept-Encoding -->
InvalidCacheStateExceptionThe problem with that approach?
We (as client developers) can't fix this on ourselves.
We just end up with yelling at our backend team (or even worse, third-party servers) to fix their Vary header.
That is all we can do, sadly.
This PR makes this check more lenient (again).
How it was before❗
But instead of running blindly into issues like KTOR-7104, we now log the problem with a clear description that we return "the best matching cache" (which doesn't mean it is the correct one!).
(I'm not sure if this Logging is also propergated to users of the lib, or if its just internal. Let me know if this is wrong and if I have to adjust it.)
Honestly, I'm not sure if this is the best-fitting solution.
But I also think that we can't serve both problems - relying on the spec & make client developers happy.
Since the later is IMHO more important, I thought we can relax the ktor client here a bit.
What do you think?