[PATCH] Action cache must only cache status 200 responses

Patch file

Please commit this patch!

Please commit

Revert spammed info

Please reopen this ticket once you have provided unit tests of the existence of the defect.

Combined Patch for caching.rb and for caching_test.rb

I submitted two simple tests in the >200 status code range

(302 by using redirect_to and 403 by setting the header by hand)

I also resubmitted the caching.rb patch in the diff to demonstrate the effect of toms test

Awesome, all tests pass.

+1 for the idea behind this patch.

This patch should probably be updated to only match against the 3 digit status code "200" and ignore the reason phrase "OK". This is according to RFC 2616 (sec 6.1.1):

The Reason-Phrase is intended to give a short textual description
of the Status-Code. The Status-Code is intended for use by automata
and the Reason-Phrase is intended for the human user. The client is
not required to examine or display the Reason-Phrase.

I could technically set the Status to "200 Awesome" and it should still be cached. ;)

Also, should action caching work for responses to non-GET requests? I'm not entirely sure you'd want the response to a PUT or POST request to be cached. This would be consistent with page caching behaviour added in [5755]. Could the caching_allowed method (that's used in page caching) be used to determine whether or not to cache an action too?

+1 Uploaded a revised patch with dkubb suggestions.

Looks good. Should only 200 status be cached? What about e.g. 204 No Content? Or all 2xx?

I guess the question would be: how many GET/HEAD requests would result in something other than 200 OK? Not including Conditional requests that result in 304's, I can't think of too many others.

If you review the 2xx response codes most of them are usually the result of something other than GET. Even 204 is rarely used in response to GET (in most apps I've seen). its much more common to see 204 sent in response to stuff like DELETE requests.

I suppose the only 2xx that comes close is 206 Partial Content, but I'm not sure how you'd cache that, unless you worked in the Range request header into the cache key. Even in that case it would probably make more sense to cache the normal 200 OK response and just return the range of content when the client requests it. (if anyone's interested, I could submit this as a patch although it shouldn't delay this particular patch from being applied)

Anyone have any other scenarios where 2xx responses other than 200 OK need to be cached? I can't think of any.

Indeed. This is a good step that we can iterate on later should the need arise. Thanks!

(In [6970]) Action caching is limited to GET requests returning 200 OK status. Closes #3335.