Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure that cache-control headers are merged #6776

Merged
merged 1 commit into from Jun 19, 2012

Conversation

raggi
Copy link
Contributor

@raggi raggi commented Jun 19, 2012

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.

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.
@josevalim
Copy link
Contributor

Merged. A refactoring to this code to use a CacheControl object would indeed be very welcome.

josevalim pushed a commit that referenced this pull request Jun 19, 2012
Ensure that cache-control headers are merged
@josevalim josevalim merged commit 23c5894 into rails:master Jun 19, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants