Support conditional requests

[sqs]

GitHub API endpoints accept conditional requests using ETag and/or If-Modified-Since (http://developer.github.com/v3/#conditional-requests).

This library could support conditional requests by modifying API methods to take a struct parameter containing those fields (or the fields could be added to, e.g., ListOptions, as in https://github.com/sqs/go-github/compare/b01324...User_events__xpr).

Or there could be a more serious refactor where the current API becomes the high-level API and there is a new lower level API that separates the construction of abstract API requests from the HTTP headers and client. You would first create the abstract API request (and each existing API method would correspond to a helper method for creating the abstract API request), and then pass it to an executor along with any arbitrary set of HTTP headers you want to use for that particular request.

I'm interested in support for conditional requests and wanted to get feedback on the right approach. (Let me know if it's outside the scope of what you want in the library.)

[willnorris]

My thinking thus far is that this should be done at the HTTP level... that is, you provide an http.Client that handles caching for you, based on the headers in the request and response... it could cache in-memory, with memcache, whatever. The client of the library (and even the go-github library itself) should be oblivious to it. I haven't yet looked at what it would take for goauth2 to wrap a caching http.Client (or the other way around). I also haven't looked too hard yet as to what http caching libraries are available.

The httplib2 python library has a good example of this with how it interacts with memcache.

[willnorris]

/cc @jcgregorio since we talked about this last week at OSCON

[jcgregorio]

Agreed, keeping it at the HTTP level is the right approach. I'm interested in what you find for Go libraries in that space. Maybe I need to write httplib2 for Go :-)

[sqs]

Yeah, I think you guys are right.

One easy way would just be to use a Squid caching proxy and define all of the GitHub API caching, staleness, and rate limiting logic in Squid's config. Then in Go, you'd just point it at that proxy.

Other than httplib2 and memcache, are you aware of any good examples of how other API clients have addressed this?

[sqs]

Here is a first attempt at a library that provides a caching http.Client:

https://github.com/sourcegraph/apiproxy/blob/master/service/github/client_test.go

It uses https://github.com/gregjones/httpcache for HTTP caching, but it also exposes functionality to bypass the cache (or set other Cache-Control, etc., headers) on a per-request basis, even when the library's only configuration point is an http.Client (as in the case of go-github):

    reqModifyingTransport.Override(regexp.MustCompile(`^/repos/sourcegraph/apiproxy$`), apiproxy.NoCache, true)

and to extend the lifetimes of cached responses for specific URLs: https://github.com/sourcegraph/apiproxy/blob/master/service/github/github.go#L13.

This library is obviously not specific to go-github, but if @willnorris or @jcgregorio or anyone else has any feedback, I would love to hear it. The library is still a WIP.

[willnorris]

closing this issue out. I actually started using apiproxy (which uses httpcache under the covers) for my project at Google and can certainly recommend it.

[dmitshur]

apiproxy is indeed extremely excellent, thank you @sqs for doing the work.

Wouldn't it make a lot of sense to add a section to the go-github package readme and docs about it?

There are already sections on Authentication, Rate Limiting, etc.

I think adding a Conditional Requests section with a link or sample usage of apiproxy would be very helpful to other people, as I imagine it's a very useful thing to have, and this issue/that package don't have a lot of visibility.

[willnorris]

added initial docs in e929dfd.

I was going to include a code sample showing the use of httpcache together with goauth2, but given that httpcache just uses the request URL as the cache key, we need to be careful about how we recommend people use that. I'll reopen this issue to track updating the docs to talk about this.

[dmitshur]

I was going to include a code sample showing the use of httpcache together with goauth2

I would really like to see that, because it's not trivial and hard to be sure if you're doing the right thing.

I've tried the following so far, and it seems to work, but I haven't really investigated any potential implications that come out of that use. Any thoughts?

[willnorris]

the way you're setting it up is fine. The bigger question is how you are using the constructed client. For apps that are always making all requests using a single set of credentials, this is fine. This is almost always the case with command line apps.

For apps that have users go through an OAuth flow with GitHub, and then that user's credentials are used to make API calls, then it becomes a much bigger concern. You absolutely can't share the client object between users because the credential is added at the http.Transport level, so all requests are made with that credential.

httpcache introduces additional concerns. If you're using a shared cache between clients, then you have to make sure that users don't accidentally get each others cached entries. I've not played with httpcache enough in this kind of environment to know how likely this risk is... my app is only caching requests made with a single set of credentials.

[sqs]

(Caveat: I haven't tried this and can't guarantee that it is correct or safe. In particular, I have not tried this with apiproxy.)

Can't you just use the "Vary: Authorization" HTTP header to tell the HTTP cache to keep separate caches for different users?

Technically, of course, that would keep per-token caches (i.e., the value of the Authorization header), which would mean the cache is effectively empty each time you obtain a new access token, which happens rarely because GitHub uses long-lived access tokens (in lieu of a refresh taken). So, you could just inject a custom header like "X-GitHub-User: sqs" using a custom RoundTripper and use "Vary: X-GitHub-User" so that the cache is per-user. You may also want to add the user's list of authorized scopes to the cache key.

BTW, I'm not sure how using custom Vary headers would interact with the "Vary: Authorization" header that GitHub's API returns.