fix: use SeparateBodyFileCache for file caching#10816
fix: use SeparateBodyFileCache for file caching#10816JPK64 wants to merge 1 commit intopython-poetry:mainfrom
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideSwitches HTTP response caching in the authenticator from FileCache to SeparateBodyFileCache to avoid serialization issues with large (>2 GiB) response bodies while preserving existing cache location and configuration. Class diagram for authenticator cache change from FileCache to SeparateBodyFileCacheclassDiagram
class Authenticator {
- Config _config
- PasswordManager _password_manager
- CacheControlAdapter _cache_control
- Path _repository_cache_directory
- str cache_id
+ Authenticator(config, cache_id)
}
class FileCache {
- Path directory
+ FileCache(directory)
+ get(key)
+ set(key, value)
}
class SeparateBodyFileCache {
- Path directory
+ SeparateBodyFileCache(directory)
+ get(key)
+ set(key, value)
}
class PasswordManager {
- Config config
+ PasswordManager(config)
+ get_credentials(repository)
}
class Config {
+ Path repository_cache_directory
}
class CacheControlAdapter {
- Any cache
+ CacheControlAdapter(cache)
+ send(request)
}
Authenticator --> Config : uses
Authenticator --> PasswordManager : owns
Authenticator --> CacheControlAdapter : owns
Authenticator ..> SeparateBodyFileCache : constructs
CacheControlAdapter --> SeparateBodyFileCache : uses as cache
%% Previous relationship (now replaced)
CacheControlAdapter ..> FileCache : previously_used_as_cache
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Switching the Although all of these issues can be fixed by clearing the cache, I am not keen on breaking the caches of an entire user base. I could probably implement some kind of cache converter that converts from the old cache format to the new cache format, or implement a proxy that uses the correct cache implementation for reading from the cache and just the |
What are the steps to reproduce this issue? In a quick test, Poetry seems to lock fine after switching from |
Unfortunately, I am no longer able to reproduce the issue myself. Like you described, it seems to work fine. But I do get the behaviour I described when doing the reverse, so creating cache entries with the I'll mark this pull request as ready for review. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider whether existing cache directories created with
FileCacheremain compatible withSeparateBodyFileCacheor if a migration/cleanup strategy is needed to avoid stale or unreadable cache entries. - It may be useful to make the choice of
SeparateBodyFileCachevsFileCacheconfigurable (even if defaulting toSeparateBodyFileCache), in case some environments rely on the previous behavior or cache layout.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider whether existing cache directories created with `FileCache` remain compatible with `SeparateBodyFileCache` or if a migration/cleanup strategy is needed to avoid stale or unreadable cache entries.
- It may be useful to make the choice of `SeparateBodyFileCache` vs `FileCache` configurable (even if defaulting to `SeparateBodyFileCache`), in case some environments rely on the previous behavior or cache layout.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Thus, the upgrade path seems fine but there is an issue if a user downgrades again or switches back and forth between different versions of Poetry. I think we should change the cache suffix from |
2 participants
Use
SeparateBodyFileCacheinstead ofFileCachefor caching.FileCacheattempts to serialize the HTTP response body along with the rest of the response which can cause aValueError: memoryview is too largefor files larger than 2 GiB.cachecontrolprovidesSeparateBodyFileCachewhich is better suited for caching large responses and does not cause this issue.Pull Request Checklist
Resolves: #10814
Summary by Sourcery
Bug Fixes: