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:
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 (
|
![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.
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.kt
Outdated
Show resolved
Hide resolved
|
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. |
|
@StefMa Thanks for the PR. You are right that client users shouldn't get an exception in this case. It's better to log it than to throw it, so let's add logging because as you mentioned, it isn't a correct behaviour, according to RFC 7232:
But RFC 9111 also says the next:
But I think we can simplify it and retrieve the cache if I added a commit with removing check @osipxd can you please take a look at the changes? |
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 (2)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt(2 hunks)ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheLegacy.kt(2 hunks)
🚧 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
🧰 Additional context used
🧬 Code Graph Analysis (1)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheLegacy.kt (2)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt (2)
findResponse(348-364)findResponse(366-379)ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheEntry.kt (1)
HttpCacheEntry(17-20)
🔇 Additional comments (1)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheLegacy.kt (1)
130-135: Simplified cache refresh logic aligns with lenient approach.The change from using
response.varyKeys()tocache.varyKeysin theHttpCacheEntryconstructor is consistent with the PR's goal of making Vary header handling more lenient. By preserving the original cached response's vary keys instead of using the 304 response's vary keys, this prevents potential cache inconsistencies.This implementation correctly maintains cache consistency while being more tolerant of server-side Vary header inconsistencies.
| if (responseFromCache.varyKeys().size != response.varyKeys().size) { | ||
| LOGGER.warn( | ||
| "Vary header mismatch on cached response for ${response.call.request.url}. " + | ||
| "Received 304 Not Modified with Vary: ${response.varyKeys()} " + | ||
| "but cached response has Vary: ${responseFromCache.varyKeys()}. " + | ||
| "According to RFC 7232 §4.1 and RFC 9111 §4.1, " + | ||
| "the server must include the full Vary header in 304 responses. " + | ||
| "Falling back to missing cache logic. " + | ||
| "Consider reporting this issue to the server maintainers." | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent warning message about fallback behavior.
The warning message states "Falling back to missing cache logic" but the code actually continues to use the cached response (proceedWith(responseFromCache) on line 82). This is misleading as no fallback occurs - the implementation proceeds with the cached response despite the Vary header mismatch.
Consider updating the warning message to accurately reflect the actual behavior:
- "Falling back to missing cache logic. " +
+ "Proceeding with cached response despite mismatch. " +The RFC references and overall warning structure are excellent and provide clear guidance to developers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (responseFromCache.varyKeys().size != response.varyKeys().size) { | |
| LOGGER.warn( | |
| "Vary header mismatch on cached response for ${response.call.request.url}. " + | |
| "Received 304 Not Modified with Vary: ${response.varyKeys()} " + | |
| "but cached response has Vary: ${responseFromCache.varyKeys()}. " + | |
| "According to RFC 7232 §4.1 and RFC 9111 §4.1, " + | |
| "the server must include the full Vary header in 304 responses. " + | |
| "Falling back to missing cache logic. " + | |
| "Consider reporting this issue to the server maintainers." | |
| ) | |
| } | |
| if (responseFromCache.varyKeys().size != response.varyKeys().size) { | |
| LOGGER.warn( | |
| "Vary header mismatch on cached response for ${response.call.request.url}. " + | |
| "Received 304 Not Modified with Vary: ${response.varyKeys()} " + | |
| "but cached response has Vary: ${responseFromCache.varyKeys()}. " + | |
| "According to RFC 7232 §4.1 and RFC 9111 §4.1, " + | |
| "the server must include the full Vary header in 304 responses. " + | |
| "Proceeding with cached response despite mismatch. " + | |
| "Consider reporting this issue to the server maintainers." | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCacheLegacy.kt
around lines 69 to 79, the warning message incorrectly states "Falling back to
missing cache logic" while the code actually proceeds with the cached response
despite the Vary header mismatch. Update the warning message to accurately
reflect that the cached response is still being used despite the mismatch,
removing the misleading fallback statement to avoid confusion.
There was a problem hiding this comment.
I have the same question as the rabbit about "Falling back to missing cache logic". The rest looks good to me
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?