small cleanup of check304 and add more tests

Remove unused ResponseWriter parameter from check304, add function docs,
and add TODO for alternate Etag header values that we should handle.

Add tests for Proxy.allowed and check304.

imageproxy.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-// Package proxy provides the image proxy.
+// Package imageproxy provides an image proxy server.
 package imageproxy // import "willnorris.com/go/imageproxy"
 
 import (
@@ -97,7 +97,7 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	w.Header().Add("Expires", resp.Header.Get("Expires"))
 	w.Header().Add("Etag", resp.Header.Get("Etag"))
 
-	if is304 := check304(w, r, resp); is304 {
+	if is304 := check304(r, resp); is304 {
 		w.WriteHeader(http.StatusNotModified)
 		return
 	}
@@ -125,7 +125,13 @@ func (p *Proxy) allowed(u *url.URL) bool {
 	return false
 }
 
-func check304(w http.ResponseWriter, req *http.Request, resp *http.Response) bool {
+// check304 checks whether we should send a 304 Not Modified in response to
+// req, based on the response resp.  This is determined using the last modified
+// time and the entity tag of resp.
+func check304(req *http.Request, resp *http.Response) bool {
+	// TODO(willnorris): if-none-match header can be a comma separated list
+	// of multiple tags to be matched, or the special value "*" which
+	// matches all etags
 	etag := resp.Header.Get("Etag")
 	if etag != "" && etag == req.Header.Get("If-None-Match") {
 		return true

imageproxy_test.go

@@ -0,0 +1,114 @@
+package imageproxy
+
+import (
+	"bufio"
+	"net/http"
+	"net/url"
+	"strings"
+	"testing"
+)
+
+func TestAllowed(t *testing.T) {
+	p := &Proxy{
+		Whitelist: []string{"a.test", "*.b.test", "*c.test"},
+	}
+
+	tests := []struct {
+		url     string
+		allowed bool
+	}{
+		{"http://a.test/image", true},
+		{"http://x.a.test/image", false},
+
+		{"http://b.test/image", true},
+		{"http://x.b.test/image", true},
+		{"http://x.y.b.test/image", true},
+
+		{"http://c.test/image", false},
+		{"http://xc.test/image", false},
+		{"/image", false},
+	}
+
+	for _, tt := range tests {
+		u, err := url.Parse(tt.url)
+		if err != nil {
+			t.Errorf("error parsing url %q: %v", tt.url, err)
+		}
+		if got, want := p.allowed(u), tt.allowed; got != want {
+			t.Errorf("allowed(%q) returned %v, want %v", u, got, want)
+		}
+	}
+}
+
+func TestCheck304(t *testing.T) {
+	tests := []struct {
+		req, resp string
+		is304     bool
+	}{
+		{ // etag match
+			"GET / HTTP/1.1\nIf-None-Match: \"v\"\n\n",
+			"HTTP/1.1 200 OK\nEtag: \"v\"\n\n",
+			true,
+		},
+		{ // last-modified match
+			"GET / HTTP/1.1\nIf-Modified-Since: Sun, 02 Jan 2000 00:00:00 GMT\n\n",
+			"HTTP/1.1 200 OK\nLast-Modified: Sat, 01 Jan 2000 00:00:00 GMT\n\n",
+			true,
+		},
+
+		// mismatches
+		{
+			"GET / HTTP/1.1\n\n",
+			"HTTP/1.1 200 OK\n\n",
+			false,
+		},
+		{
+			"GET / HTTP/1.1\n\n",
+			"HTTP/1.1 200 OK\nEtag: \"v\"\n\n",
+			false,
+		},
+		{
+			"GET / HTTP/1.1\nIf-None-Match: \"v\"\n\n",
+			"HTTP/1.1 200 OK\n\n",
+			false,
+		},
+		{
+			"GET / HTTP/1.1\nIf-None-Match: \"a\"\n\n",
+			"HTTP/1.1 200 OK\nEtag: \"b\"\n\n",
+			false,
+		},
+		{ // last-modified match
+			"GET / HTTP/1.1\n\n",
+			"HTTP/1.1 200 OK\nLast-Modified: Sat, 01 Jan 2000 00:00:00 GMT\n\n",
+			false,
+		},
+		{ // last-modified match
+			"GET / HTTP/1.1\nIf-Modified-Since: Sun, 02 Jan 2000 00:00:00 GMT\n\n",
+			"HTTP/1.1 200 OK\n\n",
+			false,
+		},
+		{ // last-modified match
+			"GET / HTTP/1.1\nIf-Modified-Since: Fri, 31 Dec 1999 00:00:00 GMT\n\n",
+			"HTTP/1.1 200 OK\nLast-Modified: Sat, 01 Jan 2000 00:00:00 GMT\n\n",
+			false,
+		},
+	}
+
+	for _, tt := range tests {
+		buf := bufio.NewReader(strings.NewReader(tt.req))
+		req, err := http.ReadRequest(buf)
+		if err != nil {
+			t.Errorf("http.ReadRequest(%q) returned error: %v", tt.req, err)
+		}
+
+		buf = bufio.NewReader(strings.NewReader(tt.resp))
+		resp, err := http.ReadResponse(buf, req)
+		if err != nil {
+			t.Errorf("http.ReadResponse(%q) returned error: %v", tt.resp, err)
+		}
+
+		if got, want := check304(req, resp), tt.is304; got != want {
+			t.Errorf("check304(%q, %q) returned: %v, want %v", tt.req, tt.resp, got, want)
+		}
+	}
+}