Rails Ruby on Rails → 3.x

Comments and changes to this ticket

• 

  Pratik March 10th, 2009 @ 11:18 AM

  • Assigned user set to “Jeremy Kemper”
  • Tag changed from “cache, caches_page, caching, format, respond_to” to “cache, caches_page, caching, format, patch, respond_to”

• You flagged this item as spam.
  

  Andrew Bloomgarden May 30th, 2009 @ 11:47 PM

  +1

  We're encountering this problem on Rails 2.3. I'd like to fix it temporarily with an around_filter, but since caching happens after that I can't. Fixing it with a before_filter will change the type forever in production.

• 

  Jeremy Kemper May 4th, 2010 @ 06:48 PM

  • Milestone changed from “2.x” to “3.x”

  [bulk edit]

• 

  Dan Pickett May 9th, 2010 @ 05:54 PM

  • Tag changed from “cache, caches_page, caching, format, patch, respond_to” to “bugmash, cache, caches_page, caching, format, patch, respond_to”

• You flagged this item as spam.
  

  Wijnand Wiersma May 16th, 2010 @ 08:53 AM

  The old patch did not apply to master anymore. I reworked it and made it less intrusive.
  The old patch also introduced a test which tested the page_cache_extension but it seemed to do this in a wrong way so it failed while the actual feature still worked.

  I will attach a patch for both master as 2-3-stable. Tests run fine and my app tests show me correct behaviour.

  • cache_extension.patch 2.9 KB
  • cache_extension-2-3.patch 2.9 KB

• You flagged this item as spam.
  

  ronin-16776 (at lighthouseapp) May 16th, 2010 @ 12:08 PM

  +1 verified

  The patches are working on both master and 2-3-stable. Tests run fine.

• 

  Santiago Pastorino May 16th, 2010 @ 02:54 PM

  • Assigned user changed from “Jeremy Kemper” to “José Valim”
• 

  José Valim May 16th, 2010 @ 03:15 PM

  The patch needs to be improved. You should not change the attribute of the class variable page_cache_extension from the instance. And why not add the extension to all caches?

• You flagged this item as spam.
  

  Wijnand Wiersma May 16th, 2010 @ 03:54 PM

  I agree changing the value is not very clean. I am currently writing a very different patch.

• You flagged this item as spam.
  

  Wijnand Wiersma May 16th, 2010 @ 04:54 PM

  • Tag changed from “bugmash, cache, caches_page, caching, format, patch, respond_to” to “bugmash, bugmash-review, cache, caches_page, caching, format, patch, respond_to”

  I attached updated patches. No more messing around with page_cache_extension. Just add the correct extension to the path when no extension is known.

  • cache_extension.patch 2.7 KB
  • cache_extension-2-3.patch 2.7 KB
• 

  Rizwan Reza May 16th, 2010 @ 05:00 PM

  • Tag changed from “bugmash, bugmash-review, cache, caches_page, caching, format, patch, respond_to” to “bugmash, cache, caches_page, caching, format, patch, respond_to”
  • State changed from “new” to “open”

  Please test this and report back.

• You flagged this item as spam.
  

  ronin-16776 (at lighthouseapp) May 16th, 2010 @ 05:34 PM

  +1 verified

  The latest patches work on master and 2-3-stable. Tests are running fine for me.

• 

  José Valim May 16th, 2010 @ 07:47 PM

  @Wijnand, it looks good! I just have one question: why are you explicitly checking for text/html?

  if (self.request.format != 'text/html' )
  

• You flagged this item as spam.
  

  Wijnand Wiersma May 16th, 2010 @ 07:54 PM

  Well, in normal requests you want to respect any ActionController::Base.page_cache_extension configuration a user might have done. The issue here was special requests like xml, json etc. For these special requests we need to cache with a different extension, we should not break regular requests.

  If you think there is a better way to detect this I will change it, but this check looked sane enough for me.

• You flagged this item as spam.
  

  Wijnand Wiersma May 16th, 2010 @ 10:28 PM

  After discussing with José Valim we decided it would be better to set ActionController::Base.page_cache_extension to nil by default and always use request.format.to_sym as extension if no page_cache_extension is set.

  page_cache_extension however is used a lot throughout the tree, including ActionDispatch::Static.
  I can change everything to figure out the extension on the fly, but I think this will result in ugly and messy code since request.format is not readily available everywhere.

  I also think keeping page_cache_extension set to '.html' is a sane default and relying on it is not really a bad thing.

  I really believe my last set of patches are the best solution to this issue.
  Anything different will require lots of rails refactoring and that is completely out of the scope here.

• You flagged this item as spam.
  

  Chris Hapgood October 7th, 2010 @ 10:49 PM

  • Importance changed from “” to “High”

  In the before filter, we consult with request.cache_format, AC::Base.page_cache_extension, params[:format], request.path, etc. in an attempt to match

  a. The requested content type -which is intentionally vague (image/, ACCEPT header with q values, IE's infamous /*, etc.).
  b. The available content type(s) -which can be UNKNOWN in the case of a cache key without an extension.

  This problem seems hopeless in the current implementation. We can't change (a), but we can address (b). And once (b) is addressed, Rails' existing content type matching capability (in respond_to) can be leveraged.

  Here's a proposed approach:

  1. Post-action, generate the cache key from the response, not the request. The user has explicitly told Rails the content type (send_file, send_data, respond_to, etc.) -it should be easy to get an extension for this content type and use it as the extension of the cache key. In some scenarios, this will have immediate benefits regardless of whether #1 below is implemented.

  2. Pre-action, don't generate a single cache key from the request. Instead, use the current Rails' respond_to logic (which compares params[:format], HTTP_ACCEPT and some smart xhr defaults against the available formats) to find the most appropriate cache key.

  The biggest impediment would seem to be identifying available cached formats. Some cache stores might allow efficient wildcard lookup ("/controller/action.*") while for others it might be necessary to store an index in a separate key.

• You flagged this item as spam.
  

  Chris Hapgood October 11th, 2010 @ 04:28 PM

  I've opened a ticket (#5783) for a similar problem on caches_action, but with my proposed "responds_to" solution.

  https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...

• 

  Neeraj Singh December 9th, 2010 @ 03:26 PM

  • State changed from “open” to “resolved”

  Please look at #6110 for commit info.