Ability to prevent obscure keys #71

Open
phillipsnick opened this Issue Aug 23, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@phillipsnick

Currently hierarchical keys are hashed with sha1 (https://github.com/php-cache/cache/blob/master/src/Hierarchy/HierarchicalCachePoolTrait.php#L78)

From what I can tell this isn't a requirement of PSR-6?
Would it be possible to have a configurable option for this?

Alternatively, I'd extend this myself, but due to the way it's structured I can't see a simple way to extend the classes and override the getHierarchyKey method.

Perhaps the ability to define the SomeCachePool in config used by the adapter factory classes inside getAdapter would make this much simpler?

Some example points where this could be implemented:

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Aug 23, 2016

Member

That is correct, it is not a requirement of PSR-6. If I remember correctly we do a sha1 on the hierarchy cache because there is an upper limit of the key length in Memcached. Since this is the internals of the cache adapter I assumed it did not matter.

May I ask why you would like to prevent hashing of the keys?

Member

Nyholm commented Aug 23, 2016

That is correct, it is not a requirement of PSR-6. If I remember correctly we do a sha1 on the hierarchy cache because there is an upper limit of the key length in Memcached. Since this is the internals of the cache adapter I assumed it did not matter.

May I ask why you would like to prevent hashing of the keys?

@phillipsnick

This comment has been minimized.

Show comment
Hide comment
@phillipsnick

phillipsnick Aug 23, 2016

Theres a comment regarding that Make sure we do not get awfully long (>250 chars) keys which makes sense, appreciate my scenario is probably a bit of an edge case!

Currently using Redis in both the PHP application and a NodeJS application where the PHP side adds items to a queue using RPUSH. The NodeJS application works this list, but occasionally requires some data we store in cache. If it's not available then it simply moved it to the back of the queue until it's available.

Probably a bad design/hack, but it's been built like that now and there's no time to find better alternatives :(

Theres a comment regarding that Make sure we do not get awfully long (>250 chars) keys which makes sense, appreciate my scenario is probably a bit of an edge case!

Currently using Redis in both the PHP application and a NodeJS application where the PHP side adds items to a queue using RPUSH. The NodeJS application works this list, but occasionally requires some data we store in cache. If it's not available then it simply moved it to the back of the queue until it's available.

Probably a bad design/hack, but it's been built like that now and there's no time to find better alternatives :(

@Nyholm

This comment has been minimized.

Show comment
Hide comment
@Nyholm

Nyholm Aug 23, 2016

Member

Does the data that NodeJS want to access need to be a hierarchy cache? Maybe you can store a cache key in a object that NodeJS do can access.

If you want to override the getHierarchyKey you need to extend an adapter and then create a new Factory (if you are using the AdapterBundle).

Im not sure there is a fix that we can make for the scenario.

Member

Nyholm commented Aug 23, 2016

Does the data that NodeJS want to access need to be a hierarchy cache? Maybe you can store a cache key in a object that NodeJS do can access.

If you want to override the getHierarchyKey you need to extend an adapter and then create a new Factory (if you are using the AdapterBundle).

Im not sure there is a fix that we can make for the scenario.

@phillipsnick

This comment has been minimized.

Show comment
Hide comment
@phillipsnick

phillipsnick Aug 23, 2016

After investigating further I didn't realise that the data was run through serialize before being stored in cache. That's just going to make life interesting on the NodeJS side.

Think I'll have to connect to Redis separately and manage it via something else. Thanks for your help.

What are your thoughts on the ability to define the cache pool class in config on the adapter bundle? It would save having to extend the factory.

DoctrineBundle does something similar, eg https://github.com/doctrine/DoctrineBundle/blob/master/Resources/config/orm.xml#L13

EDIT: probably one for another issue though :)

phillipsnick commented Aug 23, 2016

After investigating further I didn't realise that the data was run through serialize before being stored in cache. That's just going to make life interesting on the NodeJS side.

Think I'll have to connect to Redis separately and manage it via something else. Thanks for your help.

What are your thoughts on the ability to define the cache pool class in config on the adapter bundle? It would save having to extend the factory.

DoctrineBundle does something similar, eg https://github.com/doctrine/DoctrineBundle/blob/master/Resources/config/orm.xml#L13

EDIT: probably one for another issue though :)

@audriusrudalevicius

This comment has been minimized.

Show comment
Hide comment
@audriusrudalevicius

audriusrudalevicius Sep 29, 2016

I have problem with sha1 keys when i using shared redis server with more that one application.
I can't easy separate and see how many keys have each app. And clear all keys of one application by prefix.
Its good idea to have prefixed keys for each app in readis or memcache.
But its not possible easy override getHierarchyKey due #72

I have problem with sha1 keys when i using shared redis server with more that one application.
I can't easy separate and see how many keys have each app. And clear all keys of one application by prefix.
Its good idea to have prefixed keys for each app in readis or memcache.
But its not possible easy override getHierarchyKey due #72

@Korbeil

This comment has been minimized.

Show comment
Hide comment
@Korbeil

Korbeil May 18, 2018

Since the key length is only a problem in Memcached, I think we should make it without hashes for common use and make an inherited version of it for Memcached with the sha-1 hash.

Thoughts ?

Korbeil commented May 18, 2018

Since the key length is only a problem in Memcached, I think we should make it without hashes for common use and make an inherited version of it for Memcached with the sha-1 hash.

Thoughts ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment