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

send normalized keys to the cache backends so they do not need to man… #22215

Merged
merged 2 commits into from Nov 16, 2015

Conversation

@grosser
Copy link
Contributor

grosser commented Nov 7, 2015

…age this themselves

key manipulation is a recurring pattern in the cache stores (base + file + mem_cached) which makes the code less readable and mixes concerns

this moves all the key manipulation into a single method normalize_key and reuses that, which will also enable users to inherit or share logic better via modules since they no longer have to modify the key (escape_key or key_file_path or namespaced_key)

@grosser
grosser reviewed Nov 7, 2015
View changes
activesupport/lib/active_support/cache/strategy/local_cache.rb Outdated
value
end

def decrement(name, amount = 1, options = nil) # :nodoc:
value = bypass_local_cache{super}
set_cache_value(value, name, amount, options)

This comment has been minimized.

Copy link
@grosser

grosser Nov 7, 2015

Author Contributor

cleaning this up ... passes amount but it is not being used ...

@grosser
Copy link
Contributor Author

grosser commented Nov 10, 2015

@kaspth another one ... this did not get assigned to anyone ...

@jeremy
Copy link
Member

jeremy commented Nov 10, 2015

API changes break any non-core cache store subclasses that override or call these methods.

@grosser
Copy link
Contributor Author

grosser commented Nov 10, 2015

so leave the method as namespaced_key so nobody that calls super gets hurt ?

@jeremy
Copy link
Member

jeremy commented Nov 10, 2015

And the callers of any of the methods that now require a namespaced/normalized key argument.

@grosser
Copy link
Contributor Author

grosser commented Nov 10, 2015

Only private methods were changes, so this would only affect
users that do Rails.cache.send(:read_entry) ...

On Mon, Nov 9, 2015 at 8:51 PM, Jeremy Daer notifications@github.com
wrote:

And the callers of any of the methods that now require a
namespaced/normalized key argument.


Reply to this email directly or view it on GitHub
#22215 (comment).

@jeremy
Copy link
Member

jeremy commented Nov 10, 2015

The consumers of these API are other cache store subclasses. They call or super() to these boilerplate methods.

@grosser
Copy link
Contributor Author

grosser commented Nov 10, 2015

so they would still work fine when renamed to normalize_parameters, right ?

@grosser
Copy link
Contributor Author

grosser commented Nov 10, 2015

I added an alias for the old method, so there should be no fallout ... good to go ?

@grosser grosser force-pushed the grosser:grosser/normalize_key branch Nov 11, 2015
@grosser
Copy link
Contributor Author

grosser commented Nov 11, 2015

rebased

@jeremy looking good ?

@grosser grosser force-pushed the grosser:grosser/normalize_key branch to f6bc5ac Nov 11, 2015
@grosser
Copy link
Contributor Author

grosser commented Nov 12, 2015

@rafaelfranca got 2 cents on that ?

@grosser
Copy link
Contributor Author

grosser commented Nov 16, 2015

@jeremy / @kaspth / @rafaelfranca ok to merge this ?

@rafaelfranca
Copy link
Member

rafaelfranca commented Nov 16, 2015

The API is being maintained by the alias so I think it is good. Thanks

rafaelfranca added a commit that referenced this pull request Nov 16, 2015
send normalized keys to the cache backends so they do not need to man…
@rafaelfranca rafaelfranca merged commit b58c37e into rails:master Nov 16, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
key = expanded_key(key)
namespace = options[:namespace] if options
prefix = namespace.is_a?(Proc) ? namespace.call : namespace
key = "#{prefix}:#{key}" if prefix
key
end
alias namespaced_key normalize_key

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 18, 2015

Member

This doesn't cover backwards compatibility. The file and mem cache stores changes break user written subclasses.

This comment has been minimized.

Copy link
@grosser

grosser Nov 18, 2015

Author Contributor

user written subclasses of stores, not Cache itself, right ?

File.atomic_write(file_name, cache_path) {|f| Marshal.dump(entry, f)}
return false if options[:unless_exist] && File.exist?(key)
ensure_cache_path(File.dirname(key))
File.atomic_write(key, cache_path) {|f| Marshal.dump(entry, f)}

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 18, 2015

Member

An example of the compat break: an overwritten write_entry wouldn't resolve the key to a file_name.

@@ -118,7 +115,8 @@ def lock_file(file_name, &block) # :nodoc:
end

# Translate a key into a file path.
def key_file_path(key)
def normalize_key(key, options)

This comment has been minimized.

Copy link
@kaspth

kaspth Nov 18, 2015

Member

While this is private scope, we can't just remove the old method because subclasses can still freely call them.

This comment has been minimized.

Copy link
@grosser

grosser Nov 18, 2015

Author Contributor

we could add a noop method for key_file_path ...

@kaspth
Copy link
Member

kaspth commented Nov 18, 2015

Missed the conversation going on in #22205 (comment). Backing off 🤘

grosser added a commit to ccocchi/libmemcached_store that referenced this pull request Nov 20, 2015
grosser added a commit to grosser/rails that referenced this pull request Nov 20, 2015
rafaelfranca added a commit that referenced this pull request Nov 20, 2015
add deprecations for a smooth transition after #22215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.