search

mhgbrown / cached_resource

Caching for ActiveResource
MIT License
80 stars 28 forks source link

Fix bugs when handling string keys #9

Closed ianejames closed 11 years ago

ianejames commented 11 years ago

Cache synchronization doesn't work properly if using primary keys that are strings. This is because the "cache_read" and "cache_write" methods attempt to automagically infer whether to process a purported key by checking whether it is a string. If the primary key value already is a string, then it will not be processed into an actual cache key.

This can be fixed by removing the automatic processing from the two methods mentioned above, and always presenting those methods with properly formatted keys.

This pull request changes the expected behaviour of two private methods, necessitating a small refactor of three other private methods. Tests have been updated as well.

mhgbrown commented 11 years ago

Awesome, thanks for this. A couple of questions:

Does the test suite pass? Have you had a chance to try it with Rails 4?

I had some time to try it out with Rails 4 and some tests were failing. I am trying to find some more time to get these things fixed and a new release out. I guess this is probably a Rails independent problem... Would appreciate any thoughts you might have.

ianejames commented 11 years ago

Yes, the test suite passes, although I was working with rails 3.2.13 (on ruby 1.9.2). I tried with rails 4 (ruby 1.9.3), and it did indeed fail, although the current master branch is failing the same way.

(time passes...)

Ok, I've identified the problems. There are two issues with Rails 4, and one issue that seems to stem from Ruby 1.9.3 (and 2.0.0) and a spec that tries to grab at the internals of a Logger instance. I'll get fixes up tomorrow. In the meantime, those errors are unrelated to this patch, so it should be safe to merge it.

mhgbrown commented 11 years ago

Great, thanks! I'll get it merged.