Rails core - Oct 2008 - request.fresh? implementation does not follow HTTP specification

According to the specification, if both Last-Modified date and Entity
Tags are sent, both must be validated to consider the request fresh:

http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.3.4

Then the current implementation is wrong, because it considers the
request fresh with one OR the other is fresh:

    def fresh?(response)
      not_modified?(response.last_modified) || etag_matches?
(response.etag)
    end

The correct implementation would be:

    def not_modified?(modified_at)
      modified_at && if_modified_since >= modified_at
    end

    def etag_matches?(etag)
      if_none_match == etag
    end

    def fresh?(response)
      if if_modified_since && if_none_match
        not_modified?(response.last_modified) && etag_matches?
(response.etag)
      if if_modified_since
        not_modified?(response.last_modified)
      if if_none_match
        etag_matches?(response.etag)
      else
        false
      end
    end

Why bother? Sometimes I want to cache a resource considering its
timestamp, but this cache is only valid for the current user (works
like scoping):

  def show
    response.last_modified = resource.updated_at
    response.etag = current_user

    if request.fresh?(response)
      head :not_modified
    else
      # ...
    end
  end

In the current implementation, even the first example given by Ryan
Daigle on his blog will fail:

http://www.ryandaigle.com/articles/2008/8/14/what-s-new-in-edge-rails-simpler-conditional-get-support-etags

---
José Valim
http://josevalim.blogspot.com/
http://www.pagestacker.com/
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

On Sun, Oct 19, 2008 at 3:51 PM, José Valim <jose.valim@gmail.com>
wrote:
>
> According to the specification, if both Last-Modified date and Entity
> Tags are sent, both must be validated to consider the request fresh:
>
> http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.3.4
I''m not sure you''re reading that paragraph correctly. 
It''s intended
to supporting both http 1.0 and 1.1 caches, if we made the change
you''re suggesting any 1.0 caches will ignore the etags and serve your
old data.  Which would defeat the purpose of its inclusion.

I believe the current behaviour is valid, it uses both validators,
merely shortcircuiting once it''s found a match.
>  def show
>    response.last_modified = resource.updated_at
>    response.etag = current_user
>
>    if request.fresh?(response)
>      head :not_modified
>    else
>      # ...
>    end
>  end
>
> In the current implementation, even the first example given by Ryan
> Daigle on his blog will fail:
You should include the user id in the etag along with the other
freshness information.



-- 
Cheers

Koz

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

> According to the specification, if both Last-Modified date and Entity
> Tags are sent, both must be validated to consider the request fresh:
You''re right. The relevant part from the spec is: "An HTTP/1.1
caching
proxy, upon receiving a conditional request that includes both a Last-
Modified date and one or more entity tags as cache validators, MUST
NOT return a locally cached response to the client unless that cached
response is consistent with all of the conditional header fields in
the request."

I''ll get it changed today.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

DHH,

Considering this, we also have to change automatic etagging.
We should not automatically set etag (with body digest) when Last-
Modified field is set, because to check this etag, we would have to
render the action and this is what we want to avoid in first place.

And the current implementation of conditional get
(handler_conditional_get!) automatically replies 304 whenever the etag
is matching (even if it''s not body digest).

I believe this might be the right implementation:

      def handle_conditional_get!
        if etag? || last_modified?
          set_conditional_cache_control!
        elsif nonempty_ok_response?
          self.etag = body
          if request && request.etag_matches?(etag)
            self.status = ''304 Not Modified''
            self.body = ''''
          end
          set_conditional_cache_control!
        end
      end

I can create a patch if needed.

Thanks,

---
José Valim
http://josevalim.blogspot.com/
http://www.pagestacker.com/
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---

I''m glad to help! =)

I was checking those things because I''m writing some code that relies
on such features.
It uses classes methods to do HTTP Cache:

  class ListsController < ApplicationController
    http_cache :index, :show, :last_modified => :list, :etag
=> :current_user

    protected
      def list
        @list ||= List.find(params[:id])
      end

      def current_user
        @current_user ||= User.find(params[:user_id])
      end
  end

It''s very lightweight (100 lines) and accepts also Procs, Methods and
Symbols as parameters.
If there is some interest in merging into Rails Core, I will be glad
to help again.

README and source code are here: http://github.com/josevalim/easy-http-cache/

Regards,
--
José Valim
http://josevalim.blogspot.com/
http://www.pagestacker.com/

On Oct 21, 12:59 pm, DHH <david.heineme...@gmail.com>
wrote:
> > Considering this, we also have to change automatic etagging.
> > We should not automatically set etag (with body digest) when Last-
> > Modified field is set, because to check this etag, we would have to
> > render the action and this is what we want to avoid in first place.
>
> You''re right. I''ve added your implementation and tests
for it. Thanks
> for helping to sanity check all this.
--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to the Google Groups
"Ruby on Rails: Core" group.
To post to this group, send email to rubyonrails-core@googlegroups.com
To unsubscribe from this group, send email to
rubyonrails-core+unsubscribe@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/rubyonrails-core?hl=en
-~----------~----~----~----~------~----~------~--~---