From 8a7e91b084c9d502ebe569c6c64616f1ebdba7fd Mon Sep 17 00:00:00 2001 From: Patrick Toomey Date: Tue, 22 Aug 2017 12:45:22 -0600 Subject: Normalize/process Cach-Control headers consistently In the existing logic, the `Cache-Control` header may or may not get normalized by additional logic depending on whether `response.cache_conrol` has been modified. This leads to inconsistent behavior, since sometimes `Cache-Control` can contain whatever a user sets and sometimes it gets normalized, based on the logic inside of `set_conditional_cache_control!`. It seems like this normalization process should happen regardless to ensure consistent behavior. --- actionpack/lib/action_dispatch/http/cache.rb | 4 +--- actionpack/test/controller/render_test.rb | 23 +++++++++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/http/cache.rb b/actionpack/lib/action_dispatch/http/cache.rb index 2cc609406a..eaff10cb54 100644 --- a/actionpack/lib/action_dispatch/http/cache.rb +++ b/actionpack/lib/action_dispatch/http/cache.rb @@ -167,9 +167,7 @@ module ActionDispatch end def handle_conditional_get! - if etag? || last_modified? || !@cache_control.empty? - set_conditional_cache_control!(@cache_control) - end + set_conditional_cache_control!(@cache_control) end DEFAULT_CACHE_CONTROL = "max-age=0, private, must-revalidate".freeze diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index 3619afc513..f1ec383070 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -162,6 +162,17 @@ class TestController < ActionController::Base render action: "hello_world" end + def conditional_hello_with_expires_and_confliciting_cache_control_headers + response.headers["Cache-Control"] = "no-cache, must-revalidate" + expires_now + render action: "hello_world" + end + + def conditional_hello_without_expires_and_confliciting_cache_control_headers + response.headers["Cache-Control"] = "no-cache, must-revalidate" + render action: "hello_world" + end + def conditional_hello_with_bangs render action: "hello_world" end @@ -368,6 +379,18 @@ class ExpiresInRenderTest < ActionController::TestCase assert_match(/no-transform/, @response.headers["Cache-Control"]) end + def test_expires_now_with_conflicting_cache_control_headers + get :conditional_hello_with_expires_and_confliciting_cache_control_headers + assert_match(/no-cache/, @response.headers["Cache-Control"]) + refute_match(/must-revalidate/, @response.headers["Cache-Control"]) + end + + def test_no_expires_now_with_conflicting_cache_control_headers + get :conditional_hello_without_expires_and_confliciting_cache_control_headers + assert_match(/no-cache/, @response.headers["Cache-Control"]) + refute_match(/must-revalidate/, @response.headers["Cache-Control"]) + end + def test_date_header_when_expires_in time = Time.mktime(2011, 10, 30) Time.stub :now, time do -- cgit v1.2.3 From 2b4c547ea803e3de8684b7cc3665b9a1cd0e6d3a Mon Sep 17 00:00:00 2001 From: Patrick Toomey Date: Tue, 22 Aug 2017 13:11:07 -0600 Subject: Use equality in place of refute assertions for accuracy --- actionpack/test/controller/render_test.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) (limited to 'actionpack') diff --git a/actionpack/test/controller/render_test.rb b/actionpack/test/controller/render_test.rb index f1ec383070..e067a7b78d 100644 --- a/actionpack/test/controller/render_test.rb +++ b/actionpack/test/controller/render_test.rb @@ -381,14 +381,12 @@ class ExpiresInRenderTest < ActionController::TestCase def test_expires_now_with_conflicting_cache_control_headers get :conditional_hello_with_expires_and_confliciting_cache_control_headers - assert_match(/no-cache/, @response.headers["Cache-Control"]) - refute_match(/must-revalidate/, @response.headers["Cache-Control"]) + assert_equal "no-cache", @response.headers["Cache-Control"] end def test_no_expires_now_with_conflicting_cache_control_headers get :conditional_hello_without_expires_and_confliciting_cache_control_headers - assert_match(/no-cache/, @response.headers["Cache-Control"]) - refute_match(/must-revalidate/, @response.headers["Cache-Control"]) + assert_equal "no-cache", @response.headers["Cache-Control"] end def test_date_header_when_expires_in -- cgit v1.2.3 From 50777ca41b72eb97e7001034dd100b86bd0d6448 Mon Sep 17 00:00:00 2001 From: Patrick Toomey Date: Tue, 22 Aug 2017 15:47:48 -0600 Subject: Let middleware handle default cache behavior --- actionpack/lib/action_dispatch/http/cache.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/http/cache.rb b/actionpack/lib/action_dispatch/http/cache.rb index eaff10cb54..468a53e202 100644 --- a/actionpack/lib/action_dispatch/http/cache.rb +++ b/actionpack/lib/action_dispatch/http/cache.rb @@ -189,7 +189,8 @@ module ActionDispatch control.merge! cache_control if control.empty? - self._cache_control = DEFAULT_CACHE_CONTROL + # Let middleware handle default behavior + self._cache_control = nil elsif control[:no_cache] self._cache_control = NO_CACHE if control[:extras] -- cgit v1.2.3 From 6ab35ee11bca2f73384fe9b1b570139b884630f2 Mon Sep 17 00:00:00 2001 From: Patrick Toomey Date: Tue, 22 Aug 2017 15:52:47 -0600 Subject: This constant is no longer used --- actionpack/lib/action_dispatch/http/cache.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/http/cache.rb b/actionpack/lib/action_dispatch/http/cache.rb index 468a53e202..ec5704ff9e 100644 --- a/actionpack/lib/action_dispatch/http/cache.rb +++ b/actionpack/lib/action_dispatch/http/cache.rb @@ -170,7 +170,6 @@ module ActionDispatch set_conditional_cache_control!(@cache_control) end - DEFAULT_CACHE_CONTROL = "max-age=0, private, must-revalidate".freeze NO_CACHE = "no-cache".freeze PUBLIC = "public".freeze PRIVATE = "private".freeze -- cgit v1.2.3 From 94b602055b0d6e2e1a9f6d33b49b433260f5e656 Mon Sep 17 00:00:00 2001 From: Patrick Toomey Date: Wed, 23 Aug 2017 11:18:11 -0600 Subject: Decouple the merge/normalization and conditional cache control logic The prior logic was trying to do too many things at once. For all responses, we want to perform two distinct steps: * Merge/normalize the `Cache-Control` values found in HTTP headers and those found in the `@cache_control` hash. * Conditionally set a default `Cache-Control` header value when we have an ETag This change separates these concerns since the merge/normalize step should occur for all responses, but the second should only occur when we have already set an ETag/last modified value. Normally ETag middleware will set a default `Cache-Control`, but only if an existing ETag is not already set. So, in the cases where an ETag is set, we need to set the default `Cache-Control` value ourselves. --- actionpack/lib/action_dispatch/http/cache.rb | 17 ++++++++++++----- actionpack/lib/action_dispatch/http/response.rb | 1 + 2 files changed, 13 insertions(+), 5 deletions(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/http/cache.rb b/actionpack/lib/action_dispatch/http/cache.rb index ec5704ff9e..2107b4d224 100644 --- a/actionpack/lib/action_dispatch/http/cache.rb +++ b/actionpack/lib/action_dispatch/http/cache.rb @@ -166,16 +166,23 @@ module ActionDispatch @cache_control = cache_control_headers end - def handle_conditional_get! - set_conditional_cache_control!(@cache_control) - end - + DEFAULT_CACHE_CONTROL = "max-age=0, private, must-revalidate".freeze NO_CACHE = "no-cache".freeze PUBLIC = "public".freeze PRIVATE = "private".freeze MUST_REVALIDATE = "must-revalidate".freeze - def set_conditional_cache_control!(cache_control) + def handle_conditional_get! + # Normally default cache control setting is handled by ETag + # middleware. But, if an etag is already set, the middleware + # defaults to `no-cache` unless a default `Cache-Control` value is + # previously set. So, set a default one here. + if (etag? || last_modified?) && !self._cache_control + self._cache_control = DEFAULT_CACHE_CONTROL + end + end + + def merge_and_normalize_cache_control!(cache_control) control = {} cc_headers = cache_control_headers if extras = cc_headers.delete(:extras) diff --git a/actionpack/lib/action_dispatch/http/response.rb b/actionpack/lib/action_dispatch/http/response.rb index b314dbecfe..0c7b153420 100644 --- a/actionpack/lib/action_dispatch/http/response.rb +++ b/actionpack/lib/action_dispatch/http/response.rb @@ -433,6 +433,7 @@ module ActionDispatch # :nodoc: def before_committed return if committed? assign_default_content_type_and_charset! + merge_and_normalize_cache_control!(@cache_control) handle_conditional_get! handle_no_content! end -- cgit v1.2.3 From 651ec51fc5b3d044083d301e571ff108ab02d62f Mon Sep 17 00:00:00 2001 From: Patrick Toomey Date: Wed, 23 Aug 2017 11:47:34 -0600 Subject: Don't touch an unused header The prior logic explictly set `Cache-Control` to `nil`. But, we would only reach that logic if the header was not set to begin with. So, rather than give it any value at all, just leave it alone. --- actionpack/lib/action_dispatch/http/cache.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'actionpack') diff --git a/actionpack/lib/action_dispatch/http/cache.rb b/actionpack/lib/action_dispatch/http/cache.rb index 2107b4d224..ba39091119 100644 --- a/actionpack/lib/action_dispatch/http/cache.rb +++ b/actionpack/lib/action_dispatch/http/cache.rb @@ -196,7 +196,6 @@ module ActionDispatch if control.empty? # Let middleware handle default behavior - self._cache_control = nil elsif control[:no_cache] self._cache_control = NO_CACHE if control[:extras] -- cgit v1.2.3