add expirable Rails.cache capability

[pinnymz]

Adds the following 2 settings:

• Shortener.cache_urls (boolean, default: false)
• Shortener.cache_expiration (numeric, default: nil)

[pinnymz]

@craigmcnamara @jpmcgrath any thoughts on this?

[craigmcnamara]

Personally, I think this is better left as an application level concern.

[pinnymz]

@craigmcnamara I see your point, but the README does list this as a future improvement :)

[craigmcnamara]

Oh, sorry about that, @jpmcgrath probably wrote that, I'll leave this one up to him.

[stephenedwards]

Personally, I think this is better left as an application level concern.

Since this feature is disabled by default, I definitely think it should be added. Large url tables or high volume user's will definitely benefit from fewer db hits.

[lifeiscontent]

::Shortener::ShortenedUrl.unexpired.where(unique_key: token).first should be ::Shortener::ShortenedUrl.unexpired.find_by(unique_key: token)

[pinnymz]

@lifeiscontent, pretty sure find_by is not supported by Rails 3. I'm fine with that personally, but the README states that Rails 3 is a target for this project (see https://github.com/jpmcgrath/shortener#dependencies and https://github.com/jpmcgrath/shortener#installation)

[lifeiscontent]

ah, ok, makes sense 👍

[pinnymz]

Revisiting, any thoughts on merging this?

[craigmcnamara]

@pinnymz I'd reiterate that caching is best left as an application level concern. You can wrap your calls to shortener however you see fit instead of complicating the internals of Shortener.

Ex:

Rails.cache.fetch("cache-key", expires_in: 30.minutes) do
  Shortener::ShortenedUrl.generate("http://example.com")
end

This has the advantage of allowing you to control the cache behavior based on the application instead of applying a blanket cache policy to the shortener. In my apps using Shortener there are a variety of situations and a single cache config would be inappropriate.

[pinnymz]

@craigmcnamara in our earlier exchange, I pointed out that this was listed as a future improvement in the README, and you mentioned leaving this up to @jpmcgrath. Has something changed in that regard since that time?

Your idea about using Rails.cache explicitly will certainly work for those that want tight control over how the caching will work. For those looking for a batteries-included solution, this feature could still be useful and unobtrusive by comparison. Additionally, your approach won't be very elegant if the shortened urls are generated solely by helper methods within a view.

Lastly, the logic gets more cumbersome if additional params are being passed to the url. I've handled this by appending an MD5 hash in that scenario, but I'm not convinced this is the best solution. And if someone does come up with something better, certainly it would be better for a general solution to a common problem to be handled by a gem upgrade.

I'm currently using this feature in a production system, and not losing any sleep over the lack of fine-grained control. Given that this would be disabled by default, why not let people choose what works best for their system?