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(-) 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