[api] Improve caching

- sort query string in cache key
- include "Accept", "Authorization" headers in cache key
- allow caching of `/graphql` responses

commands/apply.go

@@ -3,6 +3,7 @@ package commands
 import (
 	"io"
 	"io/ioutil"
+	"os"
 	"regexp"
 
 	"github.com/github/hub/github"
@@ -96,7 +97,10 @@ func transformApplyArgs(args *Args) {
 			continue
 		}
 
-		patchFile, err := ioutil.TempFile("", "hub")
+		tempDir := os.TempDir()
+		err = os.MkdirAll(tempDir, 0775)
+		utils.Check(err)
+		patchFile, err := ioutil.TempFile(tempDir, "hub")
 		utils.Check(err)
 
 		_, err = io.Copy(patchFile, patch)

features/api.feature

@@ -176,3 +176,79 @@ Feature: hub api
       """
       .query	repository(owner: "octocat", name: "Hello-World")\n\n
       """
+
+  Scenario: Cache response
+    Given the GitHub API server:
+      """
+      count = 0
+      get('/count') {
+        count += 1
+        json :count => count
+      }
+      """
+    When I successfully run `hub api -t 'count?a=1&b=2' --cache 5`
+    And I successfully run `hub api -t 'count?b=2&a=1' --cache 5`
+    Then the output should contain exactly:
+      """
+      .count	1
+      .count	1\n
+      """
+
+  Scenario: Cache graphql response
+    Given the GitHub API server:
+      """
+      count = 0
+      post('/graphql') {
+        halt 400 unless params[:query] =~ /^Q\d$/
+        count += 1
+        json :count => count
+      }
+      """
+    When I successfully run `hub api -t graphql -F query=Q1 --cache 5`
+    And I successfully run `hub api -t graphql -F query=Q1 --cache 5`
+    And I successfully run `hub api -t graphql -F query=Q2 --cache 5`
+    Then the output should contain exactly:
+      """
+      .count	1
+      .count	1
+      .count	2\n
+      """
+
+  Scenario: Avoid caching unsucessful response
+    Given the GitHub API server:
+      """
+      count = 0
+      get('/count') {
+        count += 1
+        status 400 if count == 1
+        json :count => count
+      }
+      """
+    When I run `hub api -t count --cache 5`
+    And I successfully run `hub api -t count --cache 5`
+    And I successfully run `hub api -t count --cache 5`
+    Then the output should contain exactly:
+      """
+      .count	2
+      .count	2
+      Error: HTTP 400 Bad Request
+      .count	1\n
+      """
+
+  Scenario: Avoid caching response if the OAuth token changes
+    Given the GitHub API server:
+      """
+      count = 0
+      get('/count') {
+        count += 1
+        json :count => count
+      }
+      """
+    When I successfully run `hub api -t count --cache 5`
+    Given I am "octocat" on github.com with OAuth token "TOKEN2"
+    When I successfully run `hub api -t count --cache 5`
+    Then the output should contain exactly:
+      """
+      .count	1
+      .count	2\n
+      """

features/support/env.rb

@@ -21,6 +21,7 @@ Before do
   set_env 'GIT_PROXY_COMMAND', 'echo'
   # avoids reading from current user's "~/.gitconfig"
   set_env 'HOME', File.expand_path(File.join(current_dir, 'home'))
+  set_env 'TMPDIR', File.expand_path(File.join(current_dir, 'tmp'))
   # https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html#variables
   set_env 'XDG_CONFIG_HOME', nil
   set_env 'XDG_CONFIG_DIRS', nil

github/http.go

@@ -3,6 +3,7 @@ package github
 import (
 	"bytes"
 	"context"
+	"crypto/md5"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -11,8 +12,10 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"path"
 	"path/filepath"
 	"regexp"
+	"sort"
 	"strings"
 	"time"
 
@@ -247,7 +250,8 @@ func (c *simpleClient) performRequestUrl(method string, url *url.URL, body io.Re
 		configure(req)
 	}
 
-	if cachedResponse := c.cacheRead(req); cachedResponse != nil {
+	key := cacheKey(req)
+	if cachedResponse := c.cacheRead(key, req); cachedResponse != nil {
 		res = &simpleResponse{cachedResponse}
 		return
 	}
@@ -257,15 +261,23 @@ func (c *simpleClient) performRequestUrl(method string, url *url.URL, body io.Re
 		return
 	}
 
-	c.cacheWrite(httpResponse)
+	c.cacheWrite(key, httpResponse)
 	res = &simpleResponse{httpResponse}
 
 	return
 }
 
-func (c *simpleClient) cacheRead(req *http.Request) (res *http.Response) {
-	if c.CacheTTL > 0 && req.Method == "GET" {
-		f := cacheFile(cacheKey(req))
+func isGraphQL(req *http.Request) bool {
+	return req.URL.Path == "/graphql"
+}
+
+func canCache(req *http.Request) bool {
+	return strings.EqualFold(req.Method, "GET") || isGraphQL(req)
+}
+
+func (c *simpleClient) cacheRead(key string, req *http.Request) (res *http.Response) {
+	if c.CacheTTL > 0 && canCache(req) {
+		f := cacheFile(key)
 		cacheInfo, err := os.Stat(f)
 		if err != nil {
 			return
@@ -285,10 +297,9 @@ func (c *simpleClient) cacheRead(req *http.Request) (res *http.Response) {
 	return
 }
 
-func (c *simpleClient) cacheWrite(res *http.Response) {
-	if c.CacheTTL > 0 && res.StatusCode < 300 && res.Request.Method == "GET" && res.Body != nil {
+func (c *simpleClient) cacheWrite(key string, res *http.Response) {
+	if c.CacheTTL > 0 && canCache(res.Request) && res.StatusCode < 300 && res.Body != nil {
 		bodyCopy := &bytes.Buffer{}
-		key := cacheKey(res.Request)
 		bodyReplacement := readCloserCallback{
 			Reader: io.TeeReader(res.Body, bodyCopy),
 			Closer: res.Body,
@@ -325,19 +336,33 @@ func (rc *readCloserCallback) Close() error {
 }
 
 func cacheKey(req *http.Request) string {
-	// TODO:
-	// - sort query string
-	// - Accept header
-	// - auth token
-	path := strings.Replace(req.URL.RequestURI(), "/", "-", -1)
+	path := strings.Replace(req.URL.EscapedPath(), "/", "-", -1)
 	if len(path) > 1 {
 		path = strings.TrimPrefix(path, "-")
 	}
-	return fmt.Sprintf("%s/%s", req.URL.Host, path)
+	host := req.Host
+	if host == "" {
+		host = req.URL.Host
+	}
+	hash := md5.New()
+	io.WriteString(hash, req.Header.Get("Accept"))
+	io.WriteString(hash, req.Header.Get("Authorization"))
+	queryParts := strings.Split(req.URL.RawQuery, "&")
+	sort.Strings(queryParts)
+	for _, q := range queryParts {
+		fmt.Fprintf(hash, "%s&", q)
+	}
+	if isGraphQL(req) && req.Body != nil {
+		if b, err := ioutil.ReadAll(req.Body); err == nil {
+			req.Body = ioutil.NopCloser(bytes.NewBuffer(b))
+			hash.Write(b)
+		}
+	}
+	return fmt.Sprintf("%s/%s_%x", host, path, hash.Sum(nil))
 }
 
 func cacheFile(key string) string {
-	return fmt.Sprintf("%s/hub/%s", os.TempDir(), key)
+	return path.Join(os.TempDir(), "hub", key)
 }
 
 func (c *simpleClient) jsonRequest(method, path string, body interface{}, configure func(*http.Request)) (*simpleResponse, error) {