This project is archived and is in readonly mode.
caches_page does not respect Accept header
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.
-
caching-extension.patch
4.47 KB
-
Pratik- 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+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.
-
-
Dan Pickett- Tag changed from cache, caches_page, caching, format, patch, respond_to to bugmash, cache, caches_page, caching, format, patch, respond_to
-
Wijnand WiersmaThe 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.86 KB
-
cache_extension-2-3.patch
2.88 KB
-
-
ronin-16776 (at lighthouseapp)+1 verified
The patches are working on both master and 2-3-stable. Tests run fine.
-
Santiago Pastorino
- Assigned user changed from Jeremy Kemper to José Valim
-
José ValimThe 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
I agree changing the value is not very clean. I am currently writing a very different patch.
-
Wijnand Wiersma
- 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.68 KB
-
cache_extension-2-3.patch
2.71 KB
-
Rizwan Reza- 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)
+1 verified
The latest patches work on master and 2-3-stable. Tests are running fine for me.
-
José Valim
@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
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
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- 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:
-
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 Migrations will not run on a fresh database below is implemented.
-
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
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- State changed from open to resolved
Please look at #6110 Rails respond_to processes XML but caches HTML for commit info.