search

mhgbrown / cached_resource

Caching for ActiveResource
MIT License
80 stars 28 forks source link

Disallow spaces in generated keys #13

Closed ches closed 10 years ago

ches commented 10 years ago

A find call like that illustrated in the spec in this commit would result in a key containing spaces, which is invalid for memcached with ASCII protocol.

More sanitization might be warranted: at least newlines, carriage returns and null bytes are invalid, I believe, but these seem unlikely to pop up from Ruby's *args.to_s...

mhgbrown commented 10 years ago

Cool, this seems legit. My only concern right now is that this could invalidate existing caches (I'm pretty sure). Is there some way we could avoid this or gracefully deal with it? Or is it just something to update and forget about?

ches commented 10 years ago

Yeah, I wondered about that myself, whether you'd want a version bump that expresses backwards incompatibility. I'm not sure if it should really be called "backwards incompatible" or not, I think really we'd need to test how different memcached client libs behave -- the memcached gem is strict about this: I encountered the issue because it throws an exception and refuses to cache the key with spaces, so it's not exactly backwards-incompatible because I was never able to cache any data that way :smile: I'm not sure about what other libs like Dalli do in this situation though, I'll try to take some time to test.

mhgbrown commented 10 years ago

Awesome, that would be great. I think bumping the version number to indicate a "backwards-incompatible" change could be a reasonable thing to do.

mhgbrown commented 10 years ago

Hahah, well almost 6 months later...

I think I'll merge this and then bump the major version number up one.

ches commented 10 years ago

Thanks, sorry I never got around to testing Dalli or the like. Haven't had any problems running with this change in practice though FWIW—occasional cases where a generated key is too large (some batch operation method that receives an argument of lots of IDs or something), but that's somewhat of a separate issue (and not terribly onerous to work around so far).

mhgbrown commented 10 years ago

No prob no prob. Good to know about the long cache keys tho. We just had an extended beta-testing phase ;)

Best, Morgan http://morgan.io

On Tue, May 6, 2014 at 11:24 AM, Ches Martin notifications@github.comwrote:

Thanks, sorry I never got around to testing Dalli or the like. Haven't had any problems running with this change in practice though FWIW—occasional cases where a generated key is too large (some batch operation method that receives an argument of lots of IDs or something), but that's somewhat of a separate issue (and not terribly onerous to work around so far).

— Reply to this email directly or view it on GitHubhttps://github.com/Ahsizara/cached_resource/pull/13#issuecomment-42282097 .