Hobo / hobo

The web app builder for Rails (moved from tablatom/hobo)
http://hobocentral.net
103 stars 39 forks source link

hobo-cache keys are too long for filesystem storage #20

Closed iox closed 11 years ago

iox commented 11 years ago

I noticed that while this works:

<hobo-cache updated="&this.updated_at">

This breaks with a "Filename too long" error:

<hobo-cache suffix="form" updated="&this.updated_at">

Seeing that Rails is using the filesystem as storage for the keys as default in production environment, I was wondering if Hobo should use a hash of the key as default. I made a quick patch to hobo_cache_helper.rb and I haven't seen any side effects:

diff --git a/hobo_rapid/app/helpers/hobo_cache_helper.rb b/hobo_rapid/app/helpers/hobo_cache_helper.rb
index 5e92f2a..d260970 100644
--- a/hobo_rapid/app/helpers/hobo_cache_helper.rb
+++ b/hobo_rapid/app/helpers/hobo_cache_helper.rb
@@ -23,7 +23,8 @@ module HoboCacheHelper

-    ActiveSupport::Cache.expand_cache_key(url_for(key_attrs).split('://').last, namespace)
+    key = ActiveSupport::Cache.expand_cache_key(url_for(key_attrs).split('://').last, namespace)
+    Digest::MD5.hexdigest(key)
   end

Maybe it would be better to make it an option, something like:

<hobo-cache suffix="form" updated="&this.updated_at" md5="true">

What do you think?

bryanlarsen commented 11 years ago

Does anybody actually use filesystem caching in production? I just assumed that it was the default only because it worked out of the box without configuration and external dependencies.

The problem with hashed keys are that they are no longer human readable, which is useful for debugging, although that can be mitigated somewhat by makings sure we log before we hash rather than after. The other problem is that delete_matched no longer works. Early implementations of hobo caching used that, but we no longer do so. So it would limit the user rather than breaking things.

That being said, filesystem storage is very useful in development, so it would be nice if it didn't break, so I'm inclined to accept the patch.

iox commented 11 years ago

Thanks for your insights Bryan, I just converted this issue to a pull request with the patch.