rails/rails — pull request #6776 by raggi (2012-06-19 00:04:35+00:00)
Title: Ensure that cache-control headers are merged

### Scenario

A developer added a before_filter that did: `response.headers['Cache-Control'] = 'no-transform'` in order to avoid mobile carriers from making invalid modifications to content.

### Issues

expires_* calls were no longer applied (see commit details), and as such, Rack::Cache begun caching all mobile renders.

### Arguments

It may be considered that setting only "no-transform" should result in the above behavior. I would stand by this view point, to a large extent, but many developers find these areas confusing. During investigating the root causes, additional bugs or at least vague semantics were found that were also addressed. It can be argued that the commit aligns a little better with the documentation, and applies some notion of POLS, whereby Rails calls / data structures are authoritative. An alternative POLS for many developers may be that the "last modification to Cache-Control should win", in which case this is still a violation if the header is modified after expires_in in a conflicting way.

### Extended commit message:

There are several aspects to this commit, that don't well fit into broken down
commits, so they are detailed here:

* When a user uses response.headers['Cache-Control'] = some_value, then the
documented convention in ConditionalGet is not adhered to, in this case,
response.cache_control is ignored due to `return if
self[CACHE_CONTROL].present?`
* When a middleware sets cache-control headers that would clobber, they're
converted to symbols directly, without underscores. This would lead to bugs.
* Items that would live in :extras if set through expires_in, are placed
directly in the @cache_control hash, and not respected in many cases
(somewhat adhering to the aforementioned documentation).
* Although quite useless, any directive named 'extras' would be ignored.

The general convention applied is that `expires_*` take precedence, but no longer
overwrite everything and `expires_*` are ALWAYS applied, even if the header is
set.

I am still unhappy about the contents of this commit, and the code in general.
Ideally it should be refactored to no longer use :extras. I'd likely recommend
expanding @cache_control into a class, and giving it the power to handle the
merge in a more efficient fashion. Such a commit would be a larger change that
could have additional semantic changes for other libraries unless they utilize
expires_in in very standard ways.