From bf4e8f723863050bbbcd4ff80e1e317bf85f2e42 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Fri, 14 Mar 2014 11:51:02 -0700 Subject: [PATCH] gethandler: rewrite to not use SeekerFromStreamingFetcher; add tests Change-Id: I785dd56f7bc7c1f113575700c74f910b4f4ea4a0 --- pkg/blobserver/gethandler/get.go | 42 +++++----- pkg/blobserver/gethandler/get_test.go | 112 ++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 21 deletions(-) diff --git a/pkg/blobserver/gethandler/get.go b/pkg/blobserver/gethandler/get.go index 22c58844b..fea58eec6 100644 --- a/pkg/blobserver/gethandler/get.go +++ b/pkg/blobserver/gethandler/get.go @@ -18,7 +18,6 @@ limitations under the License. package gethandler import ( - "bytes" "fmt" "io" "net/http" @@ -26,10 +25,10 @@ import ( "regexp" "strings" "time" - "unicode/utf8" "camlistore.org/pkg/blob" "camlistore.org/pkg/httputil" + "camlistore.org/pkg/types" ) var kGetPattern = regexp.MustCompile(`/camli/` + blob.Pattern + `$`) @@ -62,9 +61,7 @@ func (h *Handler) ServeHTTP(conn http.ResponseWriter, req *http.Request) { // ServeBlobRef serves a blob. func ServeBlobRef(rw http.ResponseWriter, req *http.Request, blobRef blob.Ref, fetcher blob.StreamingFetcher) { - seekFetcher := blob.SeekerFromStreamingFetcher(fetcher) - - file, size, err := seekFetcher.Fetch(blobRef) + rc, size, err := fetcher.FetchStreaming(blobRef) switch err { case nil: break @@ -76,29 +73,32 @@ func ServeBlobRef(rw http.ResponseWriter, req *http.Request, blobRef blob.Ref, f httputil.ServeError(rw, req, err) return } - defer file.Close() - var content io.ReadSeeker = file - + defer rc.Close() rw.Header().Set("Content-Type", "application/octet-stream") - if req.Header.Get("Range") == "" { + + var content io.ReadSeeker = types.NewFakeSeeker(rc, int64(size)) + rangeHeader := req.Header.Get("Range") != "" + const small = 32 << 10 + var b *blob.Blob + if rangeHeader || size < small { + // Slurp to memory, so we can actually seek on it (for Range support), + // or if we're going to be showing it in the browser (below). + b, err = blob.FromReader(blobRef, rc, size) + if err != nil { + httputil.ServeError(rw, req, err) + return + } + content = b.Open() + } + if !rangeHeader && size < small { // If it's small and all UTF-8, assume it's text and // just render it in the browser. This is more for // demos/debuggability than anything else. It isn't // part of the spec. - if size <= 32<<10 { - var buf bytes.Buffer - _, err := io.Copy(&buf, file) - if err != nil { - httputil.ServeError(rw, req, err) - return - } - if utf8.Valid(buf.Bytes()) { - rw.Header().Set("Content-Type", "text/plain; charset=utf-8") - } - content = bytes.NewReader(buf.Bytes()) + if b.IsUTF8() { + rw.Header().Set("Content-Type", "text/plain; charset=utf-8") } } - http.ServeContent(rw, req, "", dummyModTime, content) } diff --git a/pkg/blobserver/gethandler/get_test.go b/pkg/blobserver/gethandler/get_test.go index c3f1e5083..12fdeb82a 100644 --- a/pkg/blobserver/gethandler/get_test.go +++ b/pkg/blobserver/gethandler/get_test.go @@ -17,6 +17,14 @@ limitations under the License. package gethandler import ( + "bytes" + "errors" + "io" + "io/ioutil" + "net/http" + "net/http/httptest" + "os" + "strings" "testing" "camlistore.org/pkg/blob" @@ -32,3 +40,107 @@ func TestBlobFromURLPath(t *testing.T) { t.Fatalf("got = %v; want %v", br, want) } } + +func TestServeBlobRef_UTF8(t *testing.T) { + testServeBlobContents(t, "foo", "text/plain; charset=utf-8") +} + +func TestServeBlobRef_Binary(t *testing.T) { + testServeBlobContents(t, "foo\xff\x00\x80", "application/octet-stream") +} + +func TestServeBlobRef_Missing(t *testing.T) { + rr := testServeBlobRef(nil, fetcher{}) + if rr.Code != 404 { + t.Errorf("Response code = %d; want 404", rr.Code) + } +} + +func TestServeBlobRef_Error(t *testing.T) { + rr := testServeBlobRef(nil, fetcher{size: -1}) + if rr.Code != 500 { + t.Errorf("Response code = %d; want 500", rr.Code) + } +} + +func TestServeBlobRef_Range(t *testing.T) { + req, _ := http.NewRequest("GET", "/path/isn't/used", nil) + req.Header.Set("Range", "bytes=0-2") + br := blob.MustParse("foo-000") + rr := httptest.NewRecorder() + rr.Body = new(bytes.Buffer) + ServeBlobRef(rr, req, br, fetcher{strings.NewReader("foobar"), 6}) + if rr.Body.String() != "foo" { + t.Errorf("Got %q; want foo", rr.Body) + } +} + +func TestServeBlobRef_Streams(t *testing.T) { + var whatWasRead bytes.Buffer + const size = 1 << 20 + testServeBlobRef(failWriter{}, fetcher{ + io.TeeReader( + strings.NewReader(strings.Repeat("x", size)), + &whatWasRead), + size, + }) + if whatWasRead.Len() == size { + t.Errorf("handler slurped instead of streamed") + } +} + +func testServeBlobContents(t *testing.T, contents, wantType string) { + rr := testServeBlobRef(nil, fetcher{strings.NewReader(contents), int64(len(contents))}) + if rr.Code != 200 { + t.Errorf("Response code = %d; want 200", rr.Code) + } + if g, w := rr.HeaderMap.Get("Content-Type"), wantType; g != w { + t.Errorf("Content-Type = %q; want %q", g, w) + } + if rr.Body.String() != contents { + t.Errorf("Wrote %q; want %q", rr.Body.String(), contents) + } +} + +func testServeBlobRef(w io.Writer, fetcher blob.Fetcher) *httptest.ResponseRecorder { + req, _ := http.NewRequest("GET", "/path/isn't/used", nil) + br := blob.MustParse("foo-123") + + rr := httptest.NewRecorder() + rr.Body = new(bytes.Buffer) + var rw http.ResponseWriter = rr + if w != nil { + rw = &altWriterRecorder{io.MultiWriter(w, rr.Body), rr} + } + ServeBlobRef(rw, req, br, fetcher) + return rr +} + +type fetcher struct { + r io.Reader + size int64 +} + +func (f fetcher) FetchStreaming(br blob.Ref) (rc io.ReadCloser, size uint32, err error) { + if f.r == nil { + if f.size < 0 { + return nil, 0, errors.New("some other error type") + } + return nil, 0, os.ErrNotExist + } + if rc, ok := f.r.(io.ReadCloser); ok { + return rc, uint32(f.size), nil + } + return ioutil.NopCloser(f.r), uint32(f.size), nil +} + +type altWriterRecorder struct { + w io.Writer + *httptest.ResponseRecorder +} + +func (a *altWriterRecorder) Write(p []byte) (int, error) { return a.w.Write(p) } + +type failWriter struct{} + +func (failWriter) Write([]byte) (int, error) { return 0, errors.New("failed to write") }