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
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (4)
✨ 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? |
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?