From f09944264d4994466aeed3ec923f79c931cbaf60 Mon Sep 17 00:00:00 2001 From: Tim Hitchener Date: Tue, 30 Jun 2020 10:06:33 -0400 Subject: [PATCH] Reverts changes that added headers to the Drop function. - After reconsideration we came to the conclusion that it does not make sense to support custom headers at this time and they unnecessarily dirty the API. - We consider downloading to only be part of Drop() main functionality the other half being detecting on disk depenencies as well, the headers change only affected half of Drop() functionality which is a weird API to enforce. --- cargo/dependency_cacher.go | 5 ++-- cargo/dependency_cacher_test.go | 3 +-- cargo/fakes/downloader.go | 13 ++++------ cargo/transport.go | 11 ++------- cargo/transport_test.go | 42 ++++++++------------------------- postal/fakes/transport.go | 13 ++++------ postal/service.go | 5 ++-- 7 files changed, 27 insertions(+), 65 deletions(-) diff --git a/cargo/dependency_cacher.go b/cargo/dependency_cacher.go index ad201ae0..7e732851 100644 --- a/cargo/dependency_cacher.go +++ b/cargo/dependency_cacher.go @@ -3,7 +3,6 @@ package cargo import ( "fmt" "io" - "net/http" "os" "path/filepath" "strings" @@ -13,7 +12,7 @@ import ( //go:generate faux --interface Downloader --output fakes/downloader.go type Downloader interface { - Drop(root, uri string, header http.Header) (io.ReadCloser, error) + Drop(root, uri string) (io.ReadCloser, error) } type DependencyCacher struct { @@ -41,7 +40,7 @@ func (dc DependencyCacher) Cache(root string, deps []ConfigMetadataDependency) ( dc.logger.Subprocess("%s (%s) [%s]", dep.ID, dep.Version, strings.Join(dep.Stacks, ", ")) dc.logger.Action("↳ dependencies/%s", dep.SHA256) - source, err := dc.downloader.Drop("", dep.URI, nil) + source, err := dc.downloader.Drop("", dep.URI) if err != nil { return nil, fmt.Errorf("failed to download dependency: %s", err) } diff --git a/cargo/dependency_cacher_test.go b/cargo/dependency_cacher_test.go index a501d9b8..eb0c311c 100644 --- a/cargo/dependency_cacher_test.go +++ b/cargo/dependency_cacher_test.go @@ -5,7 +5,6 @@ import ( "fmt" "io" "io/ioutil" - "net/http" "os" "path/filepath" "strings" @@ -35,7 +34,7 @@ func testDependencyCacher(t *testing.T, context spec.G, it spec.S) { Expect(err).NotTo(HaveOccurred()) downloader = &fakes.Downloader{} - downloader.DropCall.Stub = func(root, uri string, header http.Header) (io.ReadCloser, error) { + downloader.DropCall.Stub = func(root, uri string) (io.ReadCloser, error) { switch uri { case "http://dep1-uri": return ioutil.NopCloser(strings.NewReader("dep1-contents")), nil diff --git a/cargo/fakes/downloader.go b/cargo/fakes/downloader.go index d9c217e3..893ef2a7 100644 --- a/cargo/fakes/downloader.go +++ b/cargo/fakes/downloader.go @@ -2,7 +2,6 @@ package fakes import ( "io" - "net/http" "sync" ) @@ -11,27 +10,25 @@ type Downloader struct { sync.Mutex CallCount int Receives struct { - Root string - Uri string - Header http.Header + Root string + Uri string } Returns struct { ReadCloser io.ReadCloser Error error } - Stub func(string, string, http.Header) (io.ReadCloser, error) + Stub func(string, string) (io.ReadCloser, error) } } -func (f *Downloader) Drop(param1 string, param2 string, param3 http.Header) (io.ReadCloser, error) { +func (f *Downloader) Drop(param1 string, param2 string) (io.ReadCloser, error) { f.DropCall.Lock() defer f.DropCall.Unlock() f.DropCall.CallCount++ f.DropCall.Receives.Root = param1 f.DropCall.Receives.Uri = param2 - f.DropCall.Receives.Header = param3 if f.DropCall.Stub != nil { - return f.DropCall.Stub(param1, param2, param3) + return f.DropCall.Stub(param1, param2) } return f.DropCall.Returns.ReadCloser, f.DropCall.Returns.Error } diff --git a/cargo/transport.go b/cargo/transport.go index cdc1096f..16c82fce 100644 --- a/cargo/transport.go +++ b/cargo/transport.go @@ -9,15 +9,13 @@ import ( "strings" ) -type Transport struct{ - header http.Header -} +type Transport struct{} func NewTransport() Transport { return Transport{} } -func (t Transport) Drop(root, uri string, header http.Header) (io.ReadCloser, error) { +func (t Transport) Drop(root, uri string) (io.ReadCloser, error) { if strings.HasPrefix(uri, "file://") { file, err := os.Open(filepath.Join(root, strings.TrimPrefix(uri, "file://"))) if err != nil { @@ -32,10 +30,6 @@ func (t Transport) Drop(root, uri string, header http.Header) (io.ReadCloser, er return nil, fmt.Errorf("failed to parse request uri: %s", err) } - if header != nil { - request.Header = header - } - response, err := http.DefaultClient.Do(request) if err != nil { return nil, fmt.Errorf("failed to make request: %s", err) @@ -43,4 +37,3 @@ func (t Transport) Drop(root, uri string, header http.Header) (io.ReadCloser, er return response.Body, nil } - diff --git a/cargo/transport_test.go b/cargo/transport_test.go index aeda71be..be7c5130 100644 --- a/cargo/transport_test.go +++ b/cargo/transport_test.go @@ -30,20 +30,11 @@ func testTransport(t *testing.T, context spec.G, it spec.S) { it.Before(func() { server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - if req.Header.Get("header") == "some-header" { - switch req.URL.Path { - case "/some-bundle-with-header": - w.Write([]byte("some-bundle-with-header-contents")) - default: - http.NotFound(w, req) - } - } else { - switch req.URL.Path { - case "/some-bundle": - w.Write([]byte("some-bundle-contents")) - default: - http.NotFound(w, req) - } + switch req.URL.Path { + case "/some-bundle": + w.Write([]byte("some-bundle-contents")) + default: + http.NotFound(w, req) } })) }) @@ -53,7 +44,7 @@ func testTransport(t *testing.T, context spec.G, it spec.S) { }) it("downloads the file from a URI", func() { - bundle, err := transport.Drop("", fmt.Sprintf("%s/some-bundle", server.URL), nil) + bundle, err := transport.Drop("", fmt.Sprintf("%s/some-bundle", server.URL)) Expect(err).NotTo(HaveOccurred()) contents, err := ioutil.ReadAll(bundle) @@ -63,23 +54,10 @@ func testTransport(t *testing.T, context spec.G, it spec.S) { Expect(bundle.Close()).To(Succeed()) }) - context("when there are request headers", func() { - it("downloads the file from a URI", func() { - bundle, err := transport.Drop("", fmt.Sprintf("%s/some-bundle-with-header", server.URL), http.Header{"header": []string{"some-header"}}) - Expect(err).NotTo(HaveOccurred()) - - contents, err := ioutil.ReadAll(bundle) - Expect(err).NotTo(HaveOccurred()) - Expect(string(contents)).To(Equal("some-bundle-with-header-contents")) - - Expect(bundle.Close()).To(Succeed()) - }) - }) - context("failure cases", func() { context("when the uri is malformed", func() { it("returns an error", func() { - _, err := transport.Drop("", "%%%%", nil) + _, err := transport.Drop("", "%%%%") Expect(err).To(MatchError(ContainSubstring("failed to parse request uri"))) Expect(err).To(MatchError(ContainSubstring("invalid URL escape"))) }) @@ -91,7 +69,7 @@ func testTransport(t *testing.T, context spec.G, it spec.S) { }) it("returns an error", func() { - _, err := transport.Drop("", fmt.Sprintf("%s/some-bundle", server.URL), nil) + _, err := transport.Drop("", fmt.Sprintf("%s/some-bundle", server.URL)) Expect(err).To(MatchError(ContainSubstring("failed to make request"))) Expect(err).To(MatchError(ContainSubstring("connection refused"))) }) @@ -121,7 +99,7 @@ func testTransport(t *testing.T, context spec.G, it spec.S) { }) it("returns the file descriptor", func() { - bundle, err := transport.Drop(dir, fmt.Sprintf("file://%s", path), nil) + bundle, err := transport.Drop(dir, fmt.Sprintf("file://%s", path)) Expect(err).NotTo(HaveOccurred()) contents, err := ioutil.ReadAll(bundle) @@ -138,7 +116,7 @@ func testTransport(t *testing.T, context spec.G, it spec.S) { context("when the file does not exist", func() { it("returns an error", func() { - _, err := transport.Drop(dir, fmt.Sprintf("file://%s", path), nil) + _, err := transport.Drop(dir, fmt.Sprintf("file://%s", path)) Expect(err).To(MatchError(ContainSubstring("failed to open file"))) Expect(err).To(MatchError(ContainSubstring("no such file or directory"))) }) diff --git a/postal/fakes/transport.go b/postal/fakes/transport.go index 47305cd2..2230199f 100644 --- a/postal/fakes/transport.go +++ b/postal/fakes/transport.go @@ -2,7 +2,6 @@ package fakes import ( "io" - "net/http" "sync" ) @@ -11,27 +10,25 @@ type Transport struct { sync.Mutex CallCount int Receives struct { - Root string - Uri string - Header http.Header + Root string + Uri string } Returns struct { ReadCloser io.ReadCloser Error error } - Stub func(string, string, http.Header) (io.ReadCloser, error) + Stub func(string, string) (io.ReadCloser, error) } } -func (f *Transport) Drop(param1 string, param2 string, param3 http.Header) (io.ReadCloser, error) { +func (f *Transport) Drop(param1 string, param2 string) (io.ReadCloser, error) { f.DropCall.Lock() defer f.DropCall.Unlock() f.DropCall.CallCount++ f.DropCall.Receives.Root = param1 f.DropCall.Receives.Uri = param2 - f.DropCall.Receives.Header = param3 if f.DropCall.Stub != nil { - return f.DropCall.Stub(param1, param2, param3) + return f.DropCall.Stub(param1, param2) } return f.DropCall.Returns.ReadCloser, f.DropCall.Returns.Error } diff --git a/postal/service.go b/postal/service.go index e74e959c..2f136030 100644 --- a/postal/service.go +++ b/postal/service.go @@ -3,7 +3,6 @@ package postal import ( "fmt" "io" - "net/http" "sort" "github.com/Masterminds/semver" @@ -16,7 +15,7 @@ import ( // Transport serves as the interface for types that can fetch dependencies // given a location uri using either the http:// or file:// scheme. type Transport interface { - Drop(root, uri string, header http.Header) (io.ReadCloser, error) + Drop(root, uri string) (io.ReadCloser, error) } // Service provides a mechanism for resolving and installing dependencies given @@ -97,7 +96,7 @@ func (s Service) Resolve(path, id, version, stack string) (Dependency, error) { // Dependency and will error if there are inconsistencies in the fetched // result. func (s Service) Install(dependency Dependency, cnbPath, layerPath string) error { - bundle, err := s.transport.Drop(cnbPath, dependency.URI, nil) + bundle, err := s.transport.Drop(cnbPath, dependency.URI) if err != nil { return fmt.Errorf("failed to fetch dependency: %s", err) }