[ActiveSupport] [Cache] Inconsistent behavior in namespaced_key

[bbenezech]

fix for #8614

>  Rails.cache.send(:namespaced_key, User.first, {})
 => "users/508fefdb54ca455644000003-20121226093034" 
> Rails.cache.send(:namespaced_key, [User.first, User.last], {})
 => "users/508fefdb54ca455644000003-20121226093034/users/50b7832354ca45f043000011-20121129154539" 
> Rails.cache.send(:namespaced_key, [User.first], {})
 => "508fefdb54ca455644000003" 

# yurk, should probably be "users/508fefdb54ca455644000003-20121226093034" instead

Also this is inconsistent with the (internally used) ActiveSupport::Cache.expand_cache_key

> ActiveSupport::Cache.expand_cache_key User.first
 => "users/508fefdb54ca455644000003-20121226093034" 
> ActiveSupport::Cache.expand_cache_key [User.first, User.last]
 => "users/508fefdb54ca455644000003-20121226093034/users/50b7832354ca45f043000011-20121129154539" 
>   ActiveSupport::Cache.expand_cache_key [User.first]
 => "users/508fefdb54ca455644000003-20121226093034" 

[rafaelfranca]

I'm not sure what is the intended behavior, but this seems to be not backward compatible.

cc @jeremy

[bbenezech]

@rafaelfranca I did not see any doc/test indicating this is the intended behaviour. Code simply stayed as Brian wrote it 3 years ago. Maybe he can tell us more about it.

cc @bdurand

[bdurand]

The implementation should generate unique, consistent keys for distinct objects. I'm not sure if the previous implementation was correct (it doesn't look like it is from the output listed above), but I don't think the proposed fix is correct either.

Consider:

id = 1
Rails.cache.fetch(id){ User.find(id) }

will not give the same result as

id = [1]
Rails.cache.fetch{ User.find(id) }

The first case will return a single User record, the second will return an array containing one User record, but both would be stored with the same cache key with the proposed fix.

[bbenezech]

This is a whole different issue. It wasn't designed that way, was it ? The possible way that i can think of to differentiate both cases is to add a trailing slash at the end for the array with one element.

[bbenezech]

Bump. This behavior needs to be fixed somehow...

[JonRowe]

Any more feedback for @bbenezech core team? (cc @rafaelfranca @jeremy @bdurand)

[arthurnn]

👍 this is good to be merged into master... this will break backwards compatible, so it should not be backported.

[arthurnn]

Actually, I think we are missing some tests. The tests you added are all for cache_key. Maybe add tests that pass a string: e.g. @cache.write(['foo'], "bar")

[arthurnn]

From the same note on #14269 (comment), I dont think we should go forward with this, as we cannot change the cache_key structure.
Thanks anyways for the patch
cc @chancancode