From 13c8f8cc4bcbb5c9c22b65db6451b3d5552e0433 Mon Sep 17 00:00:00 2001 From: Seth Erickson Date: Sat, 2 Mar 2024 13:28:49 -0800 Subject: [PATCH] add content-length header to download response. closes #14 --- server/access_service.go | 44 +++++++++++++++++++++++------------ server/access_service_test.go | 44 ++++++++++++++++++++++++----------- 2 files changed, 59 insertions(+), 29 deletions(-) diff --git a/server/access_service.go b/server/access_service.go index 0ec4383..eabb907 100644 --- a/server/access_service.go +++ b/server/access_service.go @@ -27,7 +27,7 @@ func (s *AccessService) Handler() (string, http.Handler) { // are handled in the hander functions. route, handle := chaparralv1connect.NewAccessServiceHandler(s) fn := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path == chap.RouteDownload && r.Method == http.MethodGet { + if r.URL.Path == chap.RouteDownload && (r.Method == http.MethodGet || r.Method == http.MethodHead) { s.DownloadHandler(w, r) return } @@ -139,10 +139,15 @@ func (srv *AccessService) DownloadHandler(w http.ResponseWriter, r *http.Request chap.QueryStorageRoot, storeID, chap.QueryObjectID, objectID, chap.QueryDigest, digest, - ) + chap.QueryContentPath, contentPath) ) defer func() { if err != nil { + pathErr := &fs.PathError{} + if errors.As(err, &pathErr) && strings.HasSuffix(pathErr.Path, contentPath) { + // only report path relative to the storage root FS + pathErr.Path = contentPath + } fmt.Fprint(w, err.Error()) } }() @@ -215,23 +220,32 @@ func (srv *AccessService) DownloadHandler(w http.ResponseWriter, r *http.Request w.WriteHeader(http.StatusNotFound) return } - logger.Error("during uploader: " + err.Error()) + logger.Error("during download: " + err.Error()) w.WriteHeader(http.StatusInternalServerError) return } - defer f.Close() - if _, err = io.Copy(w, f); err != nil { - pathErr := &fs.PathError{} - if errors.As(err, &pathErr) && strings.HasSuffix(pathErr.Path, fullPath) { - // only report path relative to the storage root FS - pathErr.Path = fullPath - } - if strings.HasSuffix(err.Error(), "is a directory") { - // error triggered when reading a file descriptor for a directory - // from local filesystem. This message may be os-specific. - w.WriteHeader(http.StatusBadRequest) - return + defer func() { + if closeErr := f.Close(); closeErr != nil { + logger.Error("closing file: %w", closeErr) } + }() + info, err := f.Stat() + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + return + } + if info.IsDir() { + err = errors.New("path is a directory") + w.WriteHeader(http.StatusBadRequest) + return + } + if info != nil { + w.Header().Add("Content-Length", fmt.Sprintf("%d", info.Size())) + } + if r.Method == http.MethodHead { + return + } + if _, err = io.Copy(w, f); err != nil { logger.Error(err.Error()) w.WriteHeader(http.StatusInternalServerError) return diff --git a/server/access_service_test.go b/server/access_service_test.go index 48df205..3776179 100644 --- a/server/access_service_test.go +++ b/server/access_service_test.go @@ -21,9 +21,16 @@ import ( "github.com/srerickson/ocfl-go/ocflv1" ) +// digest from testdata manifest +const ( + testDigest = "43a43fe8a8a082d3b5343dfaf2fd0c8b8e370675b1f376e92e9994612c33ea255b11298269d72f797399ebb94edeefe53df243643676548f584fb8603ca53a0f" + contentLength = 20 +) + var _ chaparralv1connect.AccessServiceHandler = (*server.AccessService)(nil) func TestAccessServiceHandler(t *testing.T) { + ctx := context.Background() testdataDir := filepath.Join("..", "testdata") storePath := path.Join("storage-roots", "root-01") objectID := "ark:123/abc" @@ -35,7 +42,6 @@ func TestAccessServiceHandler(t *testing.T) { httpClient := srv.Client() // load fixture for comparison - ctx := context.Background() storeB, err := ocflv1.GetStore(ctx, ocfl.DirFS(testdataDir), storePath) if err != nil { t.Fatal("in test setup:", err) @@ -45,9 +51,8 @@ func TestAccessServiceHandler(t *testing.T) { t.Fatal("in test setup:", err) } expectState := obj.Inventory.Version(0).State - t.Run("GetObjectState()", func(t *testing.T) { + t.Run("get object version", func(t *testing.T) { chap := chaparralv1connect.NewAccessServiceClient(httpClient, srv.URL) - ctx := context.Background() req := connect.NewRequest(&chaparralv1.GetObjectVersionRequest{ StorageRootId: storeID, ObjectId: objectID, @@ -71,34 +76,45 @@ func TestAccessServiceHandler(t *testing.T) { } u := srv.URL + chap.RouteDownload + "?" + vals.Encode() resp, err := httpClient.Get(u) - if err != nil { - t.Fatal("http client error:", err) - } + be.NilErr(t, err) defer resp.Body.Close() if resp.StatusCode != 200 { b, _ := io.ReadAll(resp.Body) - t.Log(string(b)) - t.Fatalf("Get(%q): status=%d", u, resp.StatusCode) + t.Fatalf("status=%d, resp=%s", resp.StatusCode, string(b)) } + // fixture inventory.json size + be.Equal(t, 744, resp.ContentLength) }) t.Run("download by digest", func(t *testing.T) { vals := url.Values{ - chap.QueryDigest: {"43a43fe8a8a082d3b5343dfaf2fd0c8b8e370675b1f376e92e9994612c33ea255b11298269d72f797399ebb94edeefe53df243643676548f584fb8603ca53a0f"}, + chap.QueryDigest: {testDigest}, chap.QueryObjectID: {objectID}, chap.QueryStorageRoot: {storeID}, } u := srv.URL + chap.RouteDownload + "?" + vals.Encode() resp, err := httpClient.Get(u) - if err != nil { - t.Fatal("http client error:", err) - } + be.NilErr(t, err) defer resp.Body.Close() if resp.StatusCode != 200 { b, _ := io.ReadAll(resp.Body) - t.Log(string(b)) - t.Fatalf("Get(%q): status=%d", u, resp.StatusCode) + t.Fatalf("status=%d, resp=%s", resp.StatusCode, string(b)) } + be.Equal(t, contentLength, resp.ContentLength) + }) + + t.Run("head by digest", func(t *testing.T) { + vals := url.Values{ + chap.QueryDigest: {testDigest}, + chap.QueryObjectID: {objectID}, + chap.QueryStorageRoot: {storeID}, + } + u := srv.URL + chap.RouteDownload + "?" + vals.Encode() + resp, err := httpClient.Head(u) + be.NilErr(t, err) + defer resp.Body.Close() + be.Equal(t, 200, resp.StatusCode) + be.Equal(t, contentLength, resp.ContentLength) }) t.Run("download dir", func(t *testing.T) {