From 7be2ef8b85c781f30264080843d5080c8a5f735e Mon Sep 17 00:00:00 2001 From: Andrey Smirnov Date: Fri, 24 Oct 2014 08:51:04 +0400 Subject: [PATCH] Don't fallback between compression methods available unless we get strictly HTTP 404. #129 #125 Prior to that, some real errors could have been masked away by that fallback. --- deb/remote_test.go | 26 +++++++++++++------------- http/download.go | 20 +++++++++++++++++--- http/download_test.go | 22 +++++++++------------- 3 files changed, 39 insertions(+), 29 deletions(-) diff --git a/deb/remote_test.go b/deb/remote_test.go index 01ab3d42..ababd23c 100644 --- a/deb/remote_test.go +++ b/deb/remote_test.go @@ -192,7 +192,7 @@ func (s *RemoteRepoSuite) TestFetch(c *C) { func (s *RemoteRepoSuite) TestFetchNullVerifier1(c *C) { downloader := http.NewFakeDownloader() - downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/InRelease", errors.New("404")) + downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/InRelease", &http.HTTPError{Code: 404}) downloader.ExpectResponse("http://mirror.yandex.ru/debian/dists/squeeze/Release", exampleReleaseFile) downloader.ExpectResponse("http://mirror.yandex.ru/debian/dists/squeeze/Release.gpg", "GPG") @@ -252,8 +252,8 @@ func (s *RemoteRepoSuite) TestDownload(c *C) { err := s.repo.Fetch(s.downloader, nil) c.Assert(err, IsNil) - s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages.bz2", errors.New("HTTP 404")) - s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages.gz", errors.New("HTTP 404")) + s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages.bz2", &http.HTTPError{Code: 404}) + s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages.gz", &http.HTTPError{Code: 404}) s.downloader.ExpectResponse("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages", examplePackagesFile) err = s.repo.DownloadPackageIndexes(s.progress, s.downloader, s.collectionFactory, false) @@ -281,11 +281,11 @@ func (s *RemoteRepoSuite) TestDownloadWithSources(c *C) { err := s.repo.Fetch(s.downloader, nil) c.Assert(err, IsNil) - s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages.bz2", errors.New("HTTP 404")) - s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages.gz", errors.New("HTTP 404")) + s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages.bz2", &http.HTTPError{Code: 404}) + s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages.gz", &http.HTTPError{Code: 404}) s.downloader.ExpectResponse("http://mirror.yandex.ru/debian/dists/squeeze/main/binary-i386/Packages", examplePackagesFile) - s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/source/Sources.bz2", errors.New("HTTP 404")) - s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/source/Sources.gz", errors.New("HTTP 404")) + s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/source/Sources.bz2", &http.HTTPError{Code: 404}) + s.downloader.ExpectError("http://mirror.yandex.ru/debian/dists/squeeze/main/source/Sources.gz", &http.HTTPError{Code: 404}) s.downloader.ExpectResponse("http://mirror.yandex.ru/debian/dists/squeeze/main/source/Sources", exampleSourcesFile) err = s.repo.DownloadPackageIndexes(s.progress, s.downloader, s.collectionFactory, false) @@ -322,8 +322,8 @@ func (s *RemoteRepoSuite) TestDownloadWithSources(c *C) { func (s *RemoteRepoSuite) TestDownloadFlat(c *C) { downloader := http.NewFakeDownloader() downloader.ExpectResponse("http://repos.express42.com/virool/precise/Release", exampleReleaseFile) - downloader.ExpectError("http://repos.express42.com/virool/precise/Packages.bz2", errors.New("HTTP 404")) - downloader.ExpectError("http://repos.express42.com/virool/precise/Packages.gz", errors.New("HTTP 404")) + downloader.ExpectError("http://repos.express42.com/virool/precise/Packages.bz2", &http.HTTPError{Code: 404}) + downloader.ExpectError("http://repos.express42.com/virool/precise/Packages.gz", &http.HTTPError{Code: 404}) downloader.ExpectResponse("http://repos.express42.com/virool/precise/Packages", examplePackagesFile) err := s.flat.Fetch(downloader, nil) @@ -352,11 +352,11 @@ func (s *RemoteRepoSuite) TestDownloadWithSourcesFlat(c *C) { downloader := http.NewFakeDownloader() downloader.ExpectResponse("http://repos.express42.com/virool/precise/Release", exampleReleaseFile) - downloader.ExpectError("http://repos.express42.com/virool/precise/Packages.bz2", errors.New("HTTP 404")) - downloader.ExpectError("http://repos.express42.com/virool/precise/Packages.gz", errors.New("HTTP 404")) + downloader.ExpectError("http://repos.express42.com/virool/precise/Packages.bz2", &http.HTTPError{Code: 404}) + downloader.ExpectError("http://repos.express42.com/virool/precise/Packages.gz", &http.HTTPError{Code: 404}) downloader.ExpectResponse("http://repos.express42.com/virool/precise/Packages", examplePackagesFile) - downloader.ExpectError("http://repos.express42.com/virool/precise/Sources.bz2", errors.New("HTTP 404")) - downloader.ExpectError("http://repos.express42.com/virool/precise/Sources.gz", errors.New("HTTP 404")) + downloader.ExpectError("http://repos.express42.com/virool/precise/Sources.bz2", &http.HTTPError{Code: 404}) + downloader.ExpectError("http://repos.express42.com/virool/precise/Sources.gz", &http.HTTPError{Code: 404}) downloader.ExpectResponse("http://repos.express42.com/virool/precise/Sources", exampleSourcesFile) err := s.flat.Fetch(downloader, nil) diff --git a/http/download.go b/http/download.go index adf3c914..cbf096c0 100644 --- a/http/download.go +++ b/http/download.go @@ -16,6 +16,17 @@ import ( "strings" ) +// HTTPError is download error connected to HTTP code +type HTTPError struct { + Code int + URL string +} + +// Error +func (e *HTTPError) Error() string { + return fmt.Sprintf("HTTP code %d while fetching %s", e.Code, e.URL) +} + // Check interface var ( _ aptly.Downloader = (*downloaderImpl)(nil) @@ -139,7 +150,7 @@ func (downloader *downloaderImpl) handleTask(task *downloadTask) { } if resp.StatusCode < 200 || resp.StatusCode > 299 { - task.result <- fmt.Errorf("HTTP code %d while fetching %s", resp.StatusCode, task.url) + task.result <- &HTTPError{Code: resp.StatusCode, URL: task.url} return } @@ -307,13 +318,16 @@ func DownloadTryCompression(downloader aptly.Downloader, url string, expectedChe } if err != nil { - continue + if err1, ok := err.(*HTTPError); ok && err1.Code == 404 { + continue + } + return nil, nil, err } var uncompressed io.Reader uncompressed, err = method.transformation(file) if err != nil { - continue + return nil, nil, err } return uncompressed, file, err diff --git a/http/download_test.go b/http/download_test.go index cfe51c02..f54505a7 100644 --- a/http/download_test.go +++ b/http/download_test.go @@ -229,7 +229,7 @@ func (s *DownloaderSuite) TestDownloadTryCompression(c *C) { // bzip2 not available, but gz is buf = make([]byte, 4) d = NewFakeDownloader() - d.ExpectError("http://example.com/file.bz2", errors.New("404")) + d.ExpectError("http://example.com/file.bz2", &HTTPError{Code: 404}) d.ExpectResponse("http://example.com/file.gz", gzipData) r, file, err = DownloadTryCompression(d, "http://example.com/file", expectedChecksums, false) c.Assert(err, IsNil) @@ -241,8 +241,8 @@ func (s *DownloaderSuite) TestDownloadTryCompression(c *C) { // bzip2 & gzip not available, but raw is buf = make([]byte, 4) d = NewFakeDownloader() - d.ExpectError("http://example.com/file.bz2", errors.New("404")) - d.ExpectError("http://example.com/file.gz", errors.New("404")) + d.ExpectError("http://example.com/file.bz2", &HTTPError{Code: 404}) + d.ExpectError("http://example.com/file.gz", &HTTPError{Code: 404}) d.ExpectResponse("http://example.com/file", rawData) r, file, err = DownloadTryCompression(d, "http://example.com/file", expectedChecksums, false) c.Assert(err, IsNil) @@ -254,14 +254,10 @@ func (s *DownloaderSuite) TestDownloadTryCompression(c *C) { // gzip available, but broken buf = make([]byte, 4) d = NewFakeDownloader() - d.ExpectError("http://example.com/file.bz2", errors.New("404")) + d.ExpectError("http://example.com/file.bz2", &HTTPError{Code: 404}) d.ExpectResponse("http://example.com/file.gz", "x") - d.ExpectResponse("http://example.com/file", "recovered") r, file, err = DownloadTryCompression(d, "http://example.com/file", nil, false) - c.Assert(err, IsNil) - defer file.Close() - io.ReadFull(r, buf) - c.Assert(string(buf), Equals, "reco") + c.Assert(err, ErrorMatches, "unexpected EOF") c.Assert(d.Empty(), Equals, true) } @@ -271,15 +267,15 @@ func (s *DownloaderSuite) TestDownloadTryCompressionErrors(c *C) { c.Assert(err, ErrorMatches, "unexpected request.*") d = NewFakeDownloader() - d.ExpectError("http://example.com/file.bz2", errors.New("404")) - d.ExpectError("http://example.com/file.gz", errors.New("404")) + d.ExpectError("http://example.com/file.bz2", &HTTPError{Code: 404}) + d.ExpectError("http://example.com/file.gz", &HTTPError{Code: 404}) d.ExpectError("http://example.com/file", errors.New("403")) _, _, err = DownloadTryCompression(d, "http://example.com/file", nil, false) c.Assert(err, ErrorMatches, "403") d = NewFakeDownloader() - d.ExpectError("http://example.com/file.bz2", errors.New("404")) - d.ExpectError("http://example.com/file.gz", errors.New("404")) + d.ExpectError("http://example.com/file.bz2", &HTTPError{Code: 404}) + d.ExpectError("http://example.com/file.gz", &HTTPError{Code: 404}) d.ExpectResponse("http://example.com/file", rawData) _, _, err = DownloadTryCompression(d, "http://example.com/file", map[string]utils.ChecksumInfo{"file": utils.ChecksumInfo{Size: 7}}, false) c.Assert(err, ErrorMatches, "checksums don't match.*")