Skip to content

Loading…

Conditional requests #195

Closed
wants to merge 1 commit into from

7 participants

@brainopia

Since making conditional request does not count against rate limit, it would be great to support it: http://developer.github.com/v3/#conditional-requests

I think, It would be easy to do with additional faraday middleware. This middleware would add information from response version headers (Etag, Last-Modified) to resulting Hashie::Mash. Also it would convert appropriate request params (etag, last-modified) to If-None-Match, If-Modified-Since headers.

What do you think @pengwynn?

@pengwynn
octokit member

It's definitely on the roadmap. I'd welcome a patch that kept configuration easy and out of the way. Check the discussion on #189 for an approach.

@brainopia

Ok, I have a rough sketch:

        # add support to request.rb for etag/last_modified options
        request.headers['If-None-Match'] = options.delete(:etag) if options[:etag]
        request.headers['If-Modified-Since'] = options.delete(:last_modified) if options[:last_modified]

        # faraday middleware to save etag/last_modified/rates on response

        module Faraday
          class Response::Headers < Faraday::Middleware
            LAST_MODIFIED   = 'last-modified'.freeze
            ETAG            = 'etag'.freeze
            RATE_LIMIT      = 'x-ratelimit-limit'.freeze
            RATE_REMAINING  = 'x-ratelimit-remaining'.freeze

            def call(env)
              @app.call(env).tap do |response|
                next unless response.body
                response.body.instance_eval do
                  self.last_modified = response.headers[LAST_MODIFIED]
                  self.etag          = response.headers[ETAG]

                  define_singleton_method :rate_limit do
                    response.headers[RATE_LIMIT].to_i
                  end

                  define_singleton_method :rate_remaining do
                    response.headers[RATE_REMAINING].to_i
                  end
                end
              end
            end
          end
        end

       # and before other middlewares
       builder.use Faraday::Response::Headers
@pengwynn
octokit member

I've been debating going with a homegrown approach over the rack-cache way. I'm inclined to keep this in-house like this since we can get a three-for in the same middleware.

I like where this is headed. Want to work up a patch?

@brainopia

Sure :)

@brainopia

Done :)

@pengwynn
octokit member

Nifty first step, but it breaks down on repeat calls because the body is nil. I like the simplicity of handling the conditional requests, but I think we need to roll some sort of caching layer in with this to make it transparent to the caller. I'm not crazy about adding an ActiveSupport dependency like Rack::Cache, but it'd be great to support memory, disk, and memcached.

Either way we need Octokit.user('brainopia') to return the same data even on the 2..n calls.

@brainopia

Sorry, I have not provided an explanation of taken approach, so you've probably misunderstood me.

All usual octokit requests will return the same data as before. To use conditional requests you would need to explicitly manage etag or last_modified values.

Lets see an example. Conditional requests make most sense in case you want to store github data somewhere and update it from time to time.

   user = Octokit.user('brainopia')
   User.create user.merge(last_modified: user.last_modified)

   # next day if we want to check if user has changed
   user = User.find_by_login 'brainopia'
   new_data = Octokit.user user.login, last_modified: user.last_modified
   if new_data
     user.last_modified = new_data.last_modified
     user.update_attributes new_data
   end
@brainopia

Initially I've wanted to return etag/last_modified together with response hash: { login: ..., last_modified: ... }, but wasn't sure what to do with response array, so I've used singleton methods. But may be it would be better to iterate over response array and set them for every hash. Somehow I like the idea of last_modified being the additional attribute of returning objects.

@brainopia

Approach with manual conditional requests would let developers decide themselves where they want to store github data and how to interact with it.

@cored

@brainopia This is quite cool, I was the one checking for this feature with @pengwynn before. You are approach looks quite simple from the development behind octokit point of view, but don't you think it could be easier for a user to have all the storing part handle by octokit or another thing inside it? like Rack::Cache?

@cored

I'm trying the following with Octokit to get Cache support, without any success. Can you guys help me out to know what is happening with this code?

require 'rack/cache'

Octokit.configure do |c|
   c.faraday_config { |f| f.use use FaradayMiddleware::RackCompatible, Rack::Cache::Context,
                                          :metastore   => "file:#{cache_dir}/rack/meta",
  :entitystore => "file:#{cache_dir}/rack/body",
  :ignore_headers => %w[Set-Cookie X-Content-Digest]
}

end

oc = Octokit.new :login => 'username', :password => 'pass'
oc.issues # always do a new request.

Am I doing something wrong?

@pengwynn
octokit member

@cored At first glance it looks like you have f.use use ?

@cored

@pengwynn That was a typo, it's still doing the same thing, always a new request. Don't know really why. I've been trying with actual octokit's stable code and also with this patch brainopia@2105eb1 without any luck.

Don't know what to do now...

@brainopia

Yeah, the reason is simple. By default (even with my patch) Octokit always uses a non-conditional request. To turn it into conditional request you have to supply last_modified or etag field.

That's what this patch does: it allows you to extract last_modified and etag values from any Octokit request and it allows you to send a conditional request later.

Also as a bonus patch allows you to get x-ratelimit-limit and x-ratelimit-remaining values from any Octokit request (so you don't have to send a separate request to track your current api limit).

Now back to your question. Rack::Cache is a reverse proxy cache. That's what they use at github (ok, at their scale it has to be varnish or squid) to response to our requests when we supply latest version what we store (conditional request).

So even with FaradayMiddleware::RackCompatible I don't think Rack::Cache will suddenly work for outgoing-requests (although I admit, I haven't investigated how RackCompatible works).

So in this case you need to create a faraday middleware which will work like Rack::Cache only in reverse – it should cache outgoing requests based on headers of responses and turn usual requests into conditional ones based on cache information.

Such middleware would be much easier for simple use-cases definitely. But it won't work in my use-case. If you want high efficiency you need to be able to manipulate this caching logic much more granularly. That's what this patch covers for.

So in the end, I think my patch would be helpful to some as it is while other people would benefit from transparent caching middleware (but it would take some work to make). So these two approaches are both useful, each for their own use-cases.

@cored
@simeonwillbanks

But may be it would be better to iterate over response array and set them for every hash.

@brainopia I think this is vital. Typically, I'll fetch resources in bulk to reduce API requests.

Octokit.configure do |c|
  c.auto_traversal = true
end
> gists = client.gists(username, options)
=> [{"created_at"=>"2011-06-01T17:10:45Z", ...
> gists.first.etag
=> nil
> gists.etag
=> "\"2b03894113ae53f26f77489b71acd43e\"" # whoa

As you can see, the current implementation is a bit confusing. Which resource belongs to gists.etag?

Anyway, thanks for working on this enhancement. Its going to be very useful.

@sigmavirus24

@simeonwillbanks, your example confused me as well, so I did the following

~ curl -i https://api.github.com/users/sigmavirus24/gists
HTTP/1.1 200 OK
Server: GitHub.com
Date: Thu, 07 Feb 2013 22:10:35 GMT
Content-Type: application/json; charset=utf-8
Connection: keep-alive
Status: 200 OK
Content-Length: 40299
X-Content-Type-Options: nosniff
Cache-Control: public, s-maxage=60, max-age=60
Vary: Accept
X-RateLimit-Remaining: 54
X-GitHub-Media-Type: github.beta
ETag: "9963805bd60407b857fbf005b7dec12e"
Last-Modified: Thu, 07 Feb 2013 22:00:54 GMT
X-RateLimit-Limit: 60

So you can see that the headers used by the client for conditional requests. This allows you to do a curl on that resource, e.g.,

~ curl -i -H 'If-None-Match: "9963805bd60407b857fbf005b7dec12e"' https://api.github.com/users/sigmavirus24/gists
HTTP/1.1 304 Not Modified
Server: GitHub.com
Date: Thu, 07 Feb 2013 22:13:29 GMT
Connection: keep-alive
Status: 304 Not Modified
X-RateLimit-Limit: 60
Vary: Accept
X-RateLimit-Remaining: 52
ETag: "9963805bd60407b857fbf005b7dec12e"
Cache-Control: public, max-age=60, s-maxage=60
X-Content-Type-Options: nosniff
Last-Modified: Thu, 07 Feb 2013 22:00:54 GMT

So in short, that belongs to the list of gists, not to any one of the gists.

@simeonwillbanks

@sigmavirus24 Makes sense. Obviously, each url has a unique ETag even if the url responds with a collection. I should have tested my request with octokit and curl. Thanks!

@sigmavirus24

@simeonwillbanks no worries. I just wish I could think of returning that to my users in github3.py. At least you guys have a way of doing it.

@simeonwillbanks

Why don't we use PStore for caching the response body? The ETag is a natural key. A middleware can transparently read and write the cache. Plus, we don't introduce any external dependencies.

Maybe a client has to opt-in to caching? This way, we always have a path for writing, and the client isn't surprised with random files.

Octokit.configure do |c|
  c.cache_responses = true
  c.cache_path = File.join(Rails.root, "tmp", "cache", "octokit")
end
@brainopia

@pengwynn I can add a transparent cache with gem moneta to support various storages if you wish?

@mrreynolds

Any update on this one? I really like @brainopia's approach - smart and lightweight. I think exposing the etag and allowing support for basic conditional requests by passing an etag to a call would be perfectly sufficient in the first place.

@sigmavirus24

@mrreynolds that's what I do with github3.py. I haven't heard any complaints yet.

@pengwynn
octokit member

A preview release of 2.0.0 is out and clears the way for approaches like this by:

  • properly memoizing Client objects with the same options
  • exposing the last HTTP response via Client#last_response

You should be able to roll your own or add faraday-http-cache to your middleware stack.

@pengwynn pengwynn closed this
@mrreynolds

exposing the last HTTP response via Client#last_response

In order to do proper conditional requests this won't be sufficient. Extracting an etag from the last response is one thing, but as far as I can see there's still no option to pass a given etag to a subsequent request. Am I missing something?

@pengwynn
octokit member

Try this:

issues = Octokit.issues 'rails/rails'
etag = Octokit.last_response.headers['etag']
# => "\"c1c7643c2d75472653613c185f45374c\""

issues = Octokit.issues 'rails/rails', :headers => {'If-None-Match' => etag}
# => ""

Octokit.last_response
# => <Sawyer::Response: 304 @rels={} @data="">

Of course you'd have to manage your own etag => payload cache, which is essentially what the Faraday middleware I linked earlier will do for you.

@mrreynolds

Nice, I must have missed the :headers option.

@xystushi

It seems pulls ignores the conditionals.

If you try this instead:

pulls = Octokit.pulls 'rails/rails';
etag = Octokit.last_response.headers['etag']
# => "\"deda972870491aaa85feb17b6987a56d\""

pulls = Octokit.pulls 'rails/rails', :headers => {'If-None-Match' => etag}
# => [#<Sawyer::Resource:0x007fa51fa50660 ... ]

Octokit.last_response.headers['status']
# => "200 OK"

You'll see pulls always gets the full list of requests and returns 200 status code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 11, 2012
  1. @brainopia
View
33 lib/faraday/response/headers.rb
@@ -0,0 +1,33 @@
+require 'faraday'
+
+module Faraday
+ class Response::Headers < Faraday::Middleware
+ LAST_MODIFIED = 'last-modified'.freeze
+ ETAG = 'etag'.freeze
+ RATE_LIMIT = 'x-ratelimit-limit'.freeze
+ RATE_REMAINING = 'x-ratelimit-remaining'.freeze
+
+ def call(env)
+ @app.call(env).tap do |response|
+ next unless response.body
+ response.body.instance_eval do
+ define_singleton_method :last_modified do
+ response.headers[LAST_MODIFIED]
+ end
+
+ define_singleton_method :etag do
+ response.headers[ETAG]
+ end
+
+ define_singleton_method :rate_limit do
+ response.headers[RATE_LIMIT].to_i
+ end
+
+ define_singleton_method :rate_remaining do
+ response.headers[RATE_REMAINING].to_i
+ end
+ end
+ end
+ end
+ end
+end
View
6 lib/octokit/client/users.rb
@@ -36,11 +36,11 @@ def all_users(options={})
# @see http://developer.github.com/v3/users/#get-a-single-user
# @example
# Octokit.user("sferik")
- def user(user=nil)
+ def user(user=nil, options={})
if user
- get "users/#{user}"
+ get "users/#{user}", options
else
- get "user"
+ get "user", options
end
end
View
2 lib/octokit/connection.rb
@@ -1,5 +1,6 @@
require 'faraday_middleware'
require 'faraday/response/raise_octokit_error'
+require 'faraday/response/headers'
module Octokit
# @private
@@ -28,6 +29,7 @@ def connection(options={})
builder.request :json
+ builder.use Faraday::Response::Headers
builder.use Faraday::Response::RaiseOctokitError
builder.use FaradayMiddleware::FollowRedirects
builder.use FaradayMiddleware::Mashify
View
2 lib/octokit/request.rb
@@ -58,6 +58,8 @@ def request(method, path, options={})
response = connection(conn_options).send(method) do |request|
request.headers['Accept'] = options.delete(:accept) || 'application/vnd.github.beta+json'
+ request.headers['If-None-Match'] = options.delete(:etag).to_s
+ request.headers['If-Modified-Since'] = options.delete(:last_modified).to_s
if token
request.headers[:authorization] = "token #{token}"
View
20 spec/faraday/response_spec.rb
@@ -52,4 +52,24 @@
end
end
+ {
+ ['etag', :etag] => '"5999a524d53874f13d57f70a5b4bd1a6"',
+ ['last-modified', :last_modified] => '"Mon, 10 Dec 2012 18:30:20 GMT',
+ ['x-ratelimit-limit', :rate_limit] => 60,
+ ['x-ratelimit-remaining', :rate_remaining] => 54
+ }.each do |(header_name, header_method), header_value|
+ context "when conditional header is present in response" do
+
+ before do
+ stub_get('https://api.github.com/users/sferik').
+ to_return(:headers => { header_name => header_value },
+ :body => json_response("user.json"))
+ end
+
+ it "adds version to response hash" do
+ response = @client.user 'sferik'
+ expect(response.send header_method).to eq(header_value)
+ end
+ end
+ end
end
Something went wrong with that request. Please try again.