[KTOR-8970] Lowercase varyKeys keys when deserializing from file storage
#5123
Conversation
WalkthroughNormalizes Vary header keys to lowercase during file cache deserialization, updates JVM tests to exercise case variations, adds a test server route returning 304 with Vary: Accept-Language, and adds a client test to verify upgrading old cache entries with case-sensitive Vary keys. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt (1)
37-44: Test correctly validates case-insensitive vary-key matching.The change from
"key"to"Key"on line 41 intentionally tests that FileCacheStorage finds entries regardless of vary-key capitalization, which validates the PR's deserialization fix.Consider adding a brief comment explaining the intentional case mismatch, or renaming the test to something like
testFindCaseInsensitiveto make the test's purpose more explicit.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt(1 hunks)ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt(1 hunks)ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.kt(2 hunks)ktor-test-server/src/main/kotlin/test/server/tests/Cache.kt(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{kt,kts}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{kt,kts}: Follow Kotlin official style guide for all Kotlin source and build scripts
Use star imports for io.ktor.* packages
Max line length is 120 characters
Indent with 4 spaces in Kotlin code
Include a copyright header in new Kotlin files
Files:
ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.ktktor-test-server/src/main/kotlin/test/server/tests/Cache.ktktor-client/ktor-client-core/jvm/test/FileStorageTest.ktktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.kt
**/*.kt
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.kt: Document all public Kotlin APIs, including parameters, return types, and exceptions
Annotate internal APIs with @internalapi
Follow Kotlin error-handling conventions and use specific Ktor exceptions
Files:
ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.ktktor-test-server/src/main/kotlin/test/server/tests/Cache.ktktor-client/ktor-client-core/jvm/test/FileStorageTest.ktktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.kt
🧬 Code graph analysis (2)
ktor-client/ktor-client-core/jvm/test/FileStorageTest.kt (1)
ktor-client/ktor-client-core/jvm/test/CachingCacheStorageTest.kt (1)
data(103-113)
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.kt (1)
ktor-client/ktor-client-core/common/src/io/ktor/client/plugins/cache/HttpCache.kt (3)
install(161-259)publicStorage(123-125)privateStorage(136-138)
🔇 Additional comments (3)
ktor-test-server/src/main/kotlin/test/server/tests/Cache.kt (1)
153-156: LGTM!The new route provides the necessary test endpoint for validating the cache upgrade scenario with case-sensitive vary keys. The implementation correctly sets the Vary header and returns a 304 Not Modified response, which will trigger the cache refresh flow being tested.
ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt (1)
225-225: LGTM! Vary key normalization during deserialization.The lowercase normalization ensures backward compatibility with cache entries created before the case-insensitive vary key change (PR #5030). This prevents
InvalidCacheStateExceptionduring cache upgrades when old entries with capitalized vary keys (e.g., "Accept-Language") are deserialized and then looked up with lowercase keys.ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.kt (1)
20-20: LGTM! Import used for test data preparation.The import is correctly used on line 955 to convert the cached response body string to a byte array.
ktor-client/ktor-client-tests/common/test/io/ktor/client/tests/plugins/CacheTest.kt
Outdated
Show resolved
Hide resolved
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @sheepmaster. * #5123 (comment) The following files were modified: * `ktor-client/ktor-client-core/jvm/src/io/ktor/client/plugins/cache/storage/FileCacheStorage.kt` * `ktor-test-server/src/main/kotlin/test/server/tests/Cache.kt`
1 participant
#5030 updated the
varyKeysmap to use lowercased keys in order to allow case-insensitive matching. The corresponding logic inFileCacheStoragehas not been updated however, which can cause an exception when upgrading from an older version to 3.3.0. This is the sequence of events:varyKeyskey that contains uppercase lettersfindResponse()findResponse()uses header lookup (which is case-insensitive) to verify that theVarykeys matchNot ModifiedfindAndRefresh()with the response, which calls the other variant offindResponse()CacheStorage.find(), which looks up keys directly in thevaryKeysmap (case-sensitive)InvalidCacheStateExceptionto be thrown because it looks like the cache entry has disappeared.This PR updates the code in
FileCacheStoragethat deserializes cache entries from disk to lowercase the keys in thevaryKeysmap, as that's the only other "real" source of cache entries (in-memory caches need to be populated from somewhere else).