Caching with Rails does not work as specified on README.md

[trein]

I followed the instructions on README.md (snippet below)

class Cache
  def get(request)
    Rails.cache.read(request)
  end

  def set(request, response)
    Rails.cache.write(request, response)
  end
end

Typhoeus::Config.cache = Cache.new

on how to setup caching on Rails, but I did not work out. I had to change Cache implementation in order to define a different key for Rails.cache:

class Cache
  def get(request)
    Rails.cache.read(request.base_url)
  end

  def set(request, response)
    Rails.cache.write(request.base_url, response)
  end
end

Typhoeus::Config.cache = Cache.new

Maybe it would be a good idea to add a unique attribute in Typhoeus::Request so that it can be used as key of the cache.

[hanshasselberg]

@trein thanks for reporting! There are Request#cache_key

typhoeus/lib/typhoeus/request.rb

Line 164 in bbd817c

def cache_key

and Request#hash

typhoeus/lib/typhoeus/request.rb

Line 156 in bbd817c

def hash

which are exactly what you are looking for. I am a bit surprised to hear it is not working because I thought cache_key is automatically picked up by ActiveSupport...

[hanshasselberg]

See http://guides.rubyonrails.org/caching_with_rails.html 2.9: Cache keys.

[trein]

@i0rek thanks for the heads up. I can see that :cache_key adds request's options.

def cache_key
    "#{self.class.name}#{base_url}#{hashable_string_for(options)}"
end

I will double check whether I'm changing it for every request as soon as I get home. It would be an explanation why I'm experiencing cache miss.

[trein]

@i0rek I double checked my implementation and realized I was using v0.6.6. That version does not have the :cached_key attribute implemented.

Updating to v0.6.8 it seems to be working, but I ended up getting a different error when caching the request: Errno::ENAMETOOLONG - File name too long. I don't know very well typhoeus' code base to suggest something smarter but what about sha-1 hashing the whole content of :cached_key?

[hanshasselberg]

I cannot remember why we didn't do that in the first place. should be fine.

[trein]

Here it goes my suggestion. I've just tested and it's working perfectly for me.

[hanshasselberg]

Could you change the method hash instead and alias the method cache_key? It doesn't really matter which way, but I would like to have them doing the same thing since there is no difference any more.

[hanshasselberg]

... And create a PR?

[trein]

Well, hash returns an integer and cache_key is supposed to return a string. I agree with you on the fact it may be a good idea to use either one. What I can do is start using hash as cache_key, but I'm not sure about it since we could be increasing the probability of getting collisions. Considering the fact a cache like that won't be so big, probably we could neglect that. What do you think?

[hanshasselberg]

Sorry, I did not got an email for your comment :(.
Your commit is fine - it fixes the cache_key. I was trying to fix hash too because I just read that crc23 is not a good choice as a hash key...

[trein]

@i0rek Great. I've just created a PR.

[svperfecta]

Very nice!

[hanshasselberg]

this issue can be closed now, right?

[trein]

Yes! Closing.