From 7519a77bbe3d120bf06cf84006f8ced9e1b788b8 Mon Sep 17 00:00:00 2001 From: Marwan Sulaiman Date: Mon, 8 Jul 2019 17:36:18 -0400 Subject: [PATCH] pkg/module: return KindNotFound on incorrect mod download (#1300) --- pkg/download/latest.go | 4 +++- pkg/download/list.go | 4 +++- pkg/download/version_info.go | 3 ++- pkg/download/version_module.go | 2 ++ pkg/download/version_zip.go | 2 ++ pkg/errors/errors.go | 12 ++++++++++++ pkg/errors/errors_test.go | 16 ++++++++++++++++ pkg/module/go_get_fetcher.go | 12 ++++++++---- pkg/module/go_get_fetcher_test.go | 16 ++++++++++++++++ 9 files changed, 64 insertions(+), 7 deletions(-) diff --git a/pkg/download/latest.go b/pkg/download/latest.go index bd5b1bed..e288141f 100644 --- a/pkg/download/latest.go +++ b/pkg/download/latest.go @@ -26,7 +26,9 @@ func LatestHandler(dp Protocol, lggr log.Entry, df *mode.DownloadFile) http.Hand info, err := dp.Latest(r.Context(), mod) if err != nil { - lggr.SystemErr(errors.E(op, err)) + severityLevel := errors.Expect(err, errors.KindNotFound) + err = errors.E(op, err, severityLevel) + lggr.SystemErr(err) w.WriteHeader(errors.Kind(err)) return } diff --git a/pkg/download/list.go b/pkg/download/list.go index 58ed6d2c..3de919a4 100644 --- a/pkg/download/list.go +++ b/pkg/download/list.go @@ -27,7 +27,9 @@ func ListHandler(dp Protocol, lggr log.Entry, df *mode.DownloadFile) http.Handle versions, err := dp.List(r.Context(), mod) if err != nil { - lggr.SystemErr(errors.E(op, err)) + severityLevel := errors.Expect(err, errors.KindNotFound) + err = errors.E(op, err, severityLevel) + lggr.SystemErr(err) w.WriteHeader(errors.Kind(err)) return } diff --git a/pkg/download/version_info.go b/pkg/download/version_info.go index abef94ea..7b9ea857 100644 --- a/pkg/download/version_info.go +++ b/pkg/download/version_info.go @@ -23,7 +23,8 @@ func InfoHandler(dp Protocol, lggr log.Entry, df *mode.DownloadFile) http.Handle } info, err := dp.Info(r.Context(), mod, ver) if err != nil { - lggr.SystemErr(errors.E(op, err, errors.M(mod), errors.V(ver))) + severityLevel := errors.Expect(err, errors.KindNotFound, errors.KindRedirect) + lggr.SystemErr(errors.E(op, err, errors.M(mod), errors.V(ver), severityLevel)) if errors.Kind(err) == errors.KindRedirect { http.Redirect(w, r, getRedirectURL(df.URL(mod), r.URL.Path), errors.KindRedirect) return diff --git a/pkg/download/version_module.go b/pkg/download/version_module.go index fc402b47..67e5a238 100644 --- a/pkg/download/version_module.go +++ b/pkg/download/version_module.go @@ -24,6 +24,8 @@ func ModuleHandler(dp Protocol, lggr log.Entry, df *mode.DownloadFile) http.Hand } modBts, err := dp.GoMod(r.Context(), mod, ver) if err != nil { + severityLevel := errors.Expect(err, errors.KindNotFound, errors.KindRedirect) + err = errors.E(op, err, severityLevel) lggr.SystemErr(err) if errors.Kind(err) == errors.KindRedirect { http.Redirect(w, r, getRedirectURL(df.URL(mod), r.URL.Path), errors.KindRedirect) diff --git a/pkg/download/version_zip.go b/pkg/download/version_zip.go index 9c8702b5..c0dcf1d7 100644 --- a/pkg/download/version_zip.go +++ b/pkg/download/version_zip.go @@ -24,6 +24,8 @@ func ZipHandler(dp Protocol, lggr log.Entry, df *mode.DownloadFile) http.Handler } zip, err := dp.Zip(r.Context(), mod, ver) if err != nil { + severityLevel := errors.Expect(err, errors.KindNotFound, errors.KindRedirect) + err = errors.E(op, err, severityLevel) lggr.SystemErr(err) if errors.Kind(err) == errors.KindRedirect { http.Redirect(w, r, getRedirectURL(df.URL(mod), r.URL.Path), errors.KindRedirect) diff --git a/pkg/errors/errors.go b/pkg/errors/errors.go index 6e67073c..9700b936 100644 --- a/pkg/errors/errors.go +++ b/pkg/errors/errors.go @@ -130,6 +130,18 @@ func Severity(err error) logrus.Level { return e.Severity } +// Expect is a helper that returns an Info level +// if the error has the expected kind, otherwise +// it returns an Error level. +func Expect(err error, kinds ...int) logrus.Level { + for _, kind := range kinds { + if Kind(err) == kind { + return logrus.InfoLevel + } + } + return logrus.ErrorLevel +} + // Kind recursively searches for the // first error kind it finds. func Kind(err error) int { diff --git a/pkg/errors/errors_test.go b/pkg/errors/errors_test.go index 2875b0e6..7556f67f 100644 --- a/pkg/errors/errors_test.go +++ b/pkg/errors/errors_test.go @@ -80,3 +80,19 @@ func (op *OpTests) TestString() { const op1 Op = "testOps.op1" require.Equal(op.T(), op1.String(), "testOps.op1") } + +func TestExpect(t *testing.T) { + err := E("TestExpect", "error message", KindBadRequest) + + severity := Expect(err, KindBadRequest) + require.Equalf(t, severity, logrus.InfoLevel, "expected an info level log but got %v", severity) + + severity = Expect(err, KindAlreadyExists) + require.Equalf(t, severity, logrus.ErrorLevel, "expected an error level but got %v", severity) + + severity = Expect(err, KindAlreadyExists, KindBadRequest) + require.Equalf(t, severity, logrus.InfoLevel, "expected an info level log but got %v", severity) + + severity = Expect(err, KindAlreadyExists, KindNotImplemented) + require.Equalf(t, severity, logrus.ErrorLevel, "expected an error level but got %v", severity) +} diff --git a/pkg/module/go_get_fetcher.go b/pkg/module/go_get_fetcher.go index 08f28a8f..6d6e1c7a 100644 --- a/pkg/module/go_get_fetcher.go +++ b/pkg/module/go_get_fetcher.go @@ -115,11 +115,15 @@ func downloadModule(goBinaryName string, fs afero.Fs, gopath, repoRoot, module, err := cmd.Run() if err != nil { err = fmt.Errorf("%v: %s", err, stderr) - // github quota exceeded - if isLimitHit(err.Error()) { - return goModule{}, errors.E(op, err, errors.KindRateLimit) + var m goModule + if jsonErr := json.NewDecoder(stdout).Decode(&m); jsonErr != nil { + return goModule{}, errors.E(op, err) } - return goModule{}, errors.E(op, err) + // github quota exceeded + if isLimitHit(m.Error) { + return goModule{}, errors.E(op, m.Error, errors.KindRateLimit) + } + return goModule{}, errors.E(op, m.Error, errors.KindNotFound) } var m goModule diff --git a/pkg/module/go_get_fetcher_test.go b/pkg/module/go_get_fetcher_test.go index 1edf8b59..5ccee759 100644 --- a/pkg/module/go_get_fetcher_test.go +++ b/pkg/module/go_get_fetcher_test.go @@ -5,6 +5,7 @@ import ( "io/ioutil" "runtime" + "github.com/gomods/athens/pkg/errors" "github.com/spf13/afero" "github.com/stretchr/testify/assert" ) @@ -51,3 +52,18 @@ func (s *ModuleSuite) TestGoGetFetcherFetch() { // close the version's zip file (which also cleans up the underlying GOPATH) and expect it to fail again r.NoError(ver.Zip.Close()) } + +func (s *ModuleSuite) TestNotFoundFetches() { + r := s.Require() + fetcher, err := NewGoGetFetcher(s.goBinaryName, afero.NewOsFs()) + r.NoError(err) + // when someone buys laks47dfjoijskdvjxuyyd.com, and implements + // a git server on top of it, this test will fail :) + _, err = fetcher.Fetch(ctx, "laks47dfjoijskdvjxuyyd.com/pkg/errors", "v0.8.1") + if err == nil { + s.Fail("expected an error but got nil") + } + if errors.Kind(err) != errors.KindNotFound { + s.Failf("incorrect error kind", "expected a not found error but got %v", errors.Kind(err)) + } +}