Ruby on Rails | Screencasts | Download | Documentation | Weblog | Community | Source

Ticket #6514 (closed defect: fixed)

Opened 2 years ago

Last modified 2 years ago

[PATCH] page caching shouldn't cache redirects

Reported by: RSL Assigned to: David
Priority: normal Milestone: 1.2
Component: ActionPack Version: edge
Severity: major Keywords: page caching redirects
Cc: bitsweat

Description

Rails' page caching should not [as far as I can see] be caching 3** status codes. Maybe you kittens have a reason but I can't see it. :)

Attachments

caching_patch_tests.rb (5.1 kB) - added by RSL on 10/30/06 12:42:27.
Unit tests for my patch's changes
dont_cache_redirects.diff (430 bytes) - added by RSL on 11/09/06 13:03:34.
Updated again to cache only HTTP GET requests

Change History

10/30/06 00:42:23 changed by bitsweat

  • cc set to bitsweat.
  • priority changed from high to normal.
  • status changed from new to closed.
  • resolution set to untested.
  • severity changed from major to normal.

Perhaps only 200 OK should be cached. Please update with unit tests demonstrating the expected behavior.

(follow-up: ↓ 3 ) 10/30/06 08:06:49 changed by RSL

I couldn't get to sleep because I can't stop thinking about this. How do you write a unit test for part of ActionController? I'd think you need a functional test. But then I think, how do you write a functional test of this without fixtures, routing, and at least one controller? I'm going to ask around the #rubyonrails irc channel but I might end up just having to write a small app and put it up on an svn for you guys to check out. I welcome your suggestions if you have any. I'm sure I'm missing something obvious since it's 3 AM here and I've been lying in bed for an hour thinking about this.

So sleepy. :)

(in reply to: ↑ 2 ) 10/30/06 12:41:44 changed by RSL

  • status changed from closed to reopened.
  • resolution deleted.
  • severity changed from normal to major.

Well, I plowed through the Action Pack unit tests. [I figured out that functional tests are really unit tests. Heh.] It seems the page caching was never really tested as far as actually creating files and writing data. The tests in /vendor/rails/actionpack/test/controller/caching_test.rb seem to merely test url rewriting. I'm not sure, though.

Anyhow... The testing environment doesn't define RAILS_ROOT which cause problems in the actual page caching. But that's another problem. [For some other day.] I just want to prove that my change works. I wrote a rather long and completely awesome unit test [included] which needs to be put into the /vendor/rails/actionpack/test/controller directory like all the other action pack tests. I've got a good bit of comments in the code so that should explain what's going on.

1. I had to manual set ActionController::Base.page_cache_directory because of the RAILS_ROOT problem.

2. I redefined ActionController::Caching::Pages.caching_allowed thusly:

def caching_allowed
  !request.post? && response.headers['Status'] && response.headers['Status'].to_i == 200
end

Everything runs awesome in the tests and now I have an even greater mastery of testing. I hadn't thought about all the redefining and things that this task made me think about. I'm pretty damned proud of this test suite.

I'm reopening the ticket and resetting the priority to high because this is [to me] a very serious need to not cache pages that are redirected. I mean no disrespect reopening what you guys closed so please don't take it that way.

Hope y'all are having a great day. It's time for me to get that sleep I've been missing.

Oh, yeah. I've also redone the patch to reflect the fact that you said that only 200 should be cached.

10/30/06 12:42:27 changed by RSL

  • attachment caching_patch_tests.rb added.

Unit tests for my patch's changes

(follow-up: ↓ 5 ) 11/09/06 05:40:02 changed by dkubb

I'd also suggest the part !request.post? be changed to request.get?

With the way the patch is now, 200 OK responses to DELETE and POST requests will be cached too, which is probably not what you would want. With the recent push for REST and Rails integration, this is a distinct possibility.

(in reply to: ↑ 4 ) 11/09/06 13:02:38 changed by RSL

Replying to dkubb:

I'd also suggest the part !request.post? be changed to request.get? With the way the patch is now, 200 OK responses to DELETE and POST requests will be cached too, which is probably not what you would want. With the recent push for REST and Rails integration, this is a distinct possibility.

I completely agree. Updated that patch to "request.get?".

11/09/06 13:03:34 changed by RSL

  • attachment dont_cache_redirects.diff added.

Updated again to cache only HTTP GET requests

12/19/06 19:57:08 changed by bitsweat

  • summary changed from [PATCH] to [PATCH] page caching shouldn't cache redirects.
  • milestone changed from 1.x to 1.2.

12/19/06 20:25:52 changed by bitsweat

  • status changed from reopened to closed.
  • resolution set to fixed.

(In [5755]) Only cache GET requests with a 200 OK response. Closes #6514, #6743.

12/19/06 20:27:36 changed by bitsweat

(In [5756]) Merge [5755] from trunk. Closes #6514, closes #6743.