This project is archived and is in readonly mode.

#1244 ✓resolved
Mike Burns

caches_page does not respect Accept header

Reported by Mike Burns | October 21st, 2008 @ 07:15 PM | in 3.x

This little snippet in our JobsController was causing an issue:

caches_page :index, :if => Proc.new {|c| c.request.format.xml?}

When requested as /jobs.xml it would create the file public/jobs.xml . However, when requested as /jobs with a header of 'Accept: text/xml' it would render the XML but cache to public/jobs.html . This meant GET /jobs.html would now produce XML.

Attached is a patch with tests and a possible solution to the problem.

Comments and changes to this ticket

  • Pratik

    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
  • Andrew Bloomgarden

    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

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

    • Milestone changed from 2.x to 3.x
  • Dan Pickett

    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
  • Wijnand Wiersma

    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.

  • ronin-16776 (at lighthouseapp)

    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

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

    • Assigned user changed from “Jeremy Kemper” to “José Valim”
  • 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?

  • Wijnand Wiersma

    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.

  • Wijnand Wiersma

    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.

  • Rizwan Reza

    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.

  • ronin-16776 (at lighthouseapp)

    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

    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' )
    
  • Wijnand Wiersma

    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.

  • Wijnand Wiersma

    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.

  • Chris Hapgood

    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.

  • Chris Hapgood

    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

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

    • State changed from “open” to “resolved”

    Please look at #6110 for commit info.

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages