mumoshu / play2-memcached

A memcached plugin for Play 2.x
Other
161 stars 66 forks source link

Hash keys before each operation #26

Closed s-a-y closed 10 years ago

s-a-y commented 11 years ago

My application uses your memcached plugin + securesocial plugin for authentication. Using both simultaneously causes Exceptions like "key too long (maximum length 250...". That's why I propose for better compatibility with the rest of the world to hash keys before each operation (or add relevant configuration parameter), since there may be other cases, when 250 bytes is not enough

mkubala commented 11 years ago

Hi Sergey,

Here is my workaround: http://marcinkubala.wordpress.com/2013/08/11/securesocial-and-memcached/

Actually you don't have to override DefaultAuthenticatorStore.find when you're using latest version from master branch (see https://github.com/mumoshu/play2-memcached/commit/99bc5687581247d0d3d7e5083ef7b74b8f71ef2e)

I'm also waiting for decision about my pull request for securesocial plugin (https://github.com/jaliss/securesocial/pull/282) which will provide possibility for setting length of generated Id's. After that, all what you need to do will be just set the value for "securesocial.generatedIdLength" to 250. So play2-memcached and securesocial plugins will work together almost out of the box..

s-a-y commented 11 years ago

This is fine, but in my opinion it's better to fix here, than in every other application or plugin which could use long keys. There is nothing complicated in having option for hashing keys. And who's really concerned about performance could just disabled it.

mkubala commented 11 years ago

You're right, hashing keys will solve all sort of such problems in one place and we'll never need to wait for pull requests in other projects or hack them. With a "magic config dongle" for enable/disable keys hashing it seems to be fine :) :+1:

Are you going to implement that?

s-a-y commented 11 years ago

I'm pretty occupied right now, so at least I need a feedback from repo owner if he likes the idea. Because I don't want to spend time on it if he's not gonna merge it.

mumoshu commented 10 years ago

Hi, Thanks for using play2-memcached!

@s-a-y

Though I'm afraid of collisions on hashed keys (depends on the hashing algorithm used for this), I like your idea because it's a practical solution in cases that we have to deal the key length problem on play2-memcached side. I would be happy to merge your changes!

@mkubala

In the certain case for securesocial, I think https://github.com/jaliss/securesocial/pull/282 is the best solution(because it wouldn't affect performance at all) so I +1'ed it!

mumoshu commented 10 years ago

@s-a-y Thanks a lot for your pull request! I have just merged it.

I'm considering to modify your code to cache the MessageDigest object here: https://github.com/s-a-y/play2-memcached/commit/586be98571864efb89d9a20e7bd310c585211be5#diff-b0135484f56f42503d27dc3c3c0b9785R165

After that modification, I will publish the next versions of the plugin asap.

s-a-y commented 10 years ago

You're welcome, I hope it's useful.

Regarding MessageDigest object you're right, I don't know about cache, but at least make it lazy val. For some reason this thought didn't visit me before.