Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG?] #fetch does not provide a key attribute to the fallback block (but should provide) #701

Conversation

0exp
Copy link
Contributor

@0exp 0exp commented Sep 28, 2018

ActiveSupport::Cache::Storage abstraction requires that #fetch/#fetch_multi methods provides a key to the block used for generating entries that does not exist in cache storage (with a given key).

I found that ActiveSupport::Cache::DalliStore#fetch missed this logic (in my own project).

This pull request resolves this.

now it works correctly:

# INCORRECT:
cache.fetch("a") { |key| "#{key}-calculated" } # in current implementation the key is nil
# => { "a" => "-calculated" }

# CORRECT (pull request)
cache.fetch("a") { |key| "#{key}-calculated" } # in current pull request the key has a corresponding value
# => { "a" => "a-calculated" }

@0exp 0exp changed the title #fetchi does not provide a key attribute to the fallback block [BUG?] #fetchi does not provide a key attribute to the fallback block Sep 28, 2018
@0exp 0exp changed the title [BUG?] #fetchi does not provide a key attribute to the fallback block [BUG?] #fetch does not provide a key attribute to the fallback block Sep 28, 2018
@0exp 0exp changed the title [BUG?] #fetch does not provide a key attribute to the fallback block [BUG?] #fetch does not provide a key attribute to the fallback block => should provide Sep 30, 2018
@0exp 0exp changed the title [BUG?] #fetch does not provide a key attribute to the fallback block => should provide [BUG?] #fetch does not provide a key attribute to the fallback block (but should provide) Sep 30, 2018
@petergoldstein
Copy link
Owner

@0exp Can you please point me to the reference in the original abstraction? I'm not seeing this in the docs.

@0exp
Copy link
Contributor Author

0exp commented Oct 13, 2018

@petergoldstein sure:

#fetch_multi provides a cache key name (ActiveSupport::Cache::Store#fetch_multi):
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/cache.rb#L439

#fetch provides a cache key name (ActiveSupport::Cache::Store#fetch):
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/cache.rb#L331


This is really useful when you work with a set of keys (via #fetch_multi)

@petergoldstein
Copy link
Owner

@0exp Interesting. Definitely there in the code, but not at all reflected in the docs. All of their example blocks have no parameters.

Thanks for the contribution.

@petergoldstein petergoldstein merged commit e893004 into petergoldstein:master Oct 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants