Allow configuring the cache with a cache store instance - #1711
Merged
Conversation
Owner
|
This is excellent. Thank you for the great code and clear explanation. Merging! |
Contributor
Author
|
Yeah! Any change to get a new geocoder release (1.8.7) with this update? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Geocoder::Cachepicks its cache store adapter from the class name of the configuredcache:object:Any object whose class is not literally named
Redissilently falls back toGeocoder::CacheStore::Generic, which does not applycache_options[:expiration]. In our app the Redis client is wrapped in a small namespacing class, so the configuredexpiration: 2.dayswas silently ignored and keys accumulated in Redis with no TTL — we only noticed when Redis memory kept growing. The same pitfall is described in #1670 (comment).Passing an already-built cache store instance — as the README suggests for Rails (
cache: Geocoder::CacheStore::Generic.new(Rails.cache, {})) — only works by accident today:Geocoder::Cachewraps the instance in a secondGenericadapter, whose duck-typing dispatch happens to delegatewrite/readto the inner adapter.expireis actually broken in that setup, becauseGeneric#removecallsstore.delete, which cache store adapters don't implement:Change
When the configured
cache:is already aGeocoder::CacheStore::Baseinstance, use it directly instead of wrapping it. This gives an explicit way to choose the adapter for stores whose class name is not recognized:Expiration support is still never guessed for unrecognized stores (in line with the intent expressed in #1670): the user opts in explicitly by picking the adapter.
Backward compatible: the class name lookup and the
Genericfallback are unchanged for every other value ofcache:. The README example forRails.cachenow also works fully (no double wrapping,expireincluded).Related to #1670.