From 539700e59861fb232f22b5814cb2caf02ff545b0 Mon Sep 17 00:00:00 2001 From: Michal Pristas Date: Fri, 24 Aug 2018 20:00:00 +0200 Subject: [PATCH] KindNotFound for all storages (#560) * all exc mongo * module, version associated with each storage error * remove confusion * removed formatted error in favor of raw flavor * once more --- pkg/storage/azurecdn/storage.go | 2 +- pkg/storage/fs/checker.go | 2 +- pkg/storage/fs/deleter.go | 2 +- pkg/storage/fs/lister.go | 8 ++++- pkg/storage/fs/saver.go | 14 ++++----- pkg/storage/gcp/deleter.go | 2 +- pkg/storage/gcp/getter.go | 16 +++++----- pkg/storage/gcp/lister.go | 2 +- pkg/storage/gcp/saver.go | 4 +-- pkg/storage/minio/checker.go | 2 +- pkg/storage/minio/deleter.go | 8 ++--- pkg/storage/minio/getter.go | 6 ++++ pkg/storage/minio/lister.go | 7 ++++- pkg/storage/mongo/checker.go | 7 ++++- pkg/storage/mongo/deleter.go | 6 ++-- pkg/storage/mongo/getter.go | 2 +- pkg/storage/mongo/lister.go | 4 +++ pkg/storage/mongo/saver.go | 6 ++-- pkg/storage/s3/storage.go | 2 +- .../module_storage/storage_test.go | 31 ++++++++++++++++++- 20 files changed, 94 insertions(+), 39 deletions(-) diff --git a/pkg/storage/azurecdn/storage.go b/pkg/storage/azurecdn/storage.go index a665c0ef..d63750e0 100644 --- a/pkg/storage/azurecdn/storage.go +++ b/pkg/storage/azurecdn/storage.go @@ -65,7 +65,7 @@ func (s *Storage) Save(ctx context.Context, module, version string, mod []byte, // // Do that only after module source+metadata is uploaded if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } return nil } diff --git a/pkg/storage/fs/checker.go b/pkg/storage/fs/checker.go index 033b253c..8b9df216 100644 --- a/pkg/storage/fs/checker.go +++ b/pkg/storage/fs/checker.go @@ -16,7 +16,7 @@ func (v *storageImpl) Exists(ctx context.Context, module, version string) (bool, versionedPath := v.versionLocation(module, version) exists, err := afero.Exists(v.filesystem, filepath.Join(versionedPath, "go.mod")) if err != nil { - return false, errors.E(op, err) + return false, errors.E(op, errors.M(module), errors.V(version), err) } return exists, nil diff --git a/pkg/storage/fs/deleter.go b/pkg/storage/fs/deleter.go index 5462043a..f72dd2a5 100644 --- a/pkg/storage/fs/deleter.go +++ b/pkg/storage/fs/deleter.go @@ -15,7 +15,7 @@ func (v *storageImpl) Delete(ctx context.Context, module, version string) error versionedPath := v.versionLocation(module, version) exists, err := v.Exists(ctx, module, version) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } if !exists { return errors.E(op, errors.M(module), errors.V(version), errors.KindNotFound) diff --git a/pkg/storage/fs/lister.go b/pkg/storage/fs/lister.go index 5d59f0c4..97376b90 100644 --- a/pkg/storage/fs/lister.go +++ b/pkg/storage/fs/lister.go @@ -2,6 +2,7 @@ package fs import ( "context" + "os" "github.com/gomods/athens/pkg/errors" opentracing "github.com/opentracing/opentracing-go" @@ -15,7 +16,12 @@ func (l *storageImpl) List(ctx context.Context, module string) ([]string, error) loc := l.moduleLocation(module) fileInfos, err := afero.ReadDir(l.filesystem, loc) if err != nil { - return nil, errors.E(op, err) + kind := errors.KindUnexpected + if os.IsNotExist(err) { + kind = errors.KindNotFound + } + + return nil, errors.E(op, errors.M(module), err, kind) } ret := []string{} for _, fileInfo := range fileInfos { diff --git a/pkg/storage/fs/saver.go b/pkg/storage/fs/saver.go index 75003eed..e5c1cb31 100644 --- a/pkg/storage/fs/saver.go +++ b/pkg/storage/fs/saver.go @@ -11,36 +11,36 @@ import ( "github.com/spf13/afero" ) -func (s *storageImpl) Save(ctx context.Context, module, vsn string, mod []byte, zip io.Reader, info []byte) error { +func (s *storageImpl) Save(ctx context.Context, module, version string, mod []byte, zip io.Reader, info []byte) error { const op errors.Op = "fs.Save" sp, ctx := opentracing.StartSpanFromContext(ctx, "storage.fs.Save") defer sp.Finish() - dir := s.versionLocation(module, vsn) + dir := s.versionLocation(module, version) // TODO: 777 is not the best filemode, use something better // make the versioned directory to hold the go.mod and the zipfile if err := s.filesystem.MkdirAll(dir, os.ModeDir|os.ModePerm); err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } // write the go.mod file if err := afero.WriteFile(s.filesystem, filepath.Join(dir, "go.mod"), mod, os.ModePerm); err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } // write the zipfile f, err := s.filesystem.OpenFile(filepath.Join(dir, "source.zip"), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, os.ModePerm) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } defer f.Close() _, err = io.Copy(f, zip) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } // write the info file - err = afero.WriteFile(s.filesystem, filepath.Join(dir, vsn+".info"), info, os.ModePerm) + err = afero.WriteFile(s.filesystem, filepath.Join(dir, version+".info"), info, os.ModePerm) if err != nil { return errors.E(op, err) } diff --git a/pkg/storage/gcp/deleter.go b/pkg/storage/gcp/deleter.go index 74143e60..f1d42198 100644 --- a/pkg/storage/gcp/deleter.go +++ b/pkg/storage/gcp/deleter.go @@ -18,7 +18,7 @@ func (s *Storage) Delete(ctx context.Context, module, version string) error { defer sp.Finish() exists, err := s.bucket.Exists(ctx, config.PackageVersionedName(module, version, "mod")) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } if !exists { return errors.E(op, errors.M(module), errors.V(version), errors.KindNotFound) diff --git a/pkg/storage/gcp/getter.go b/pkg/storage/gcp/getter.go index f88d9f54..8d88b538 100644 --- a/pkg/storage/gcp/getter.go +++ b/pkg/storage/gcp/getter.go @@ -18,7 +18,7 @@ func (s *Storage) Info(ctx context.Context, module, version string) ([]byte, err defer sp.Finish() exists, err := s.Exists(ctx, module, version) if err != nil { - return nil, errors.E(op, err) + return nil, errors.E(op, err, errors.M(module), errors.V(version)) } if !exists { return nil, errors.E(op, errors.M(module), errors.V(version), errors.KindNotFound) @@ -26,12 +26,12 @@ func (s *Storage) Info(ctx context.Context, module, version string) ([]byte, err infoReader, err := s.bucket.Open(ctx, config.PackageVersionedName(module, version, "info")) if err != nil { - return nil, errors.E(op, err) + return nil, errors.E(op, err, errors.M(module), errors.V(version)) } infoBytes, err := ioutil.ReadAll(infoReader) infoReader.Close() if err != nil { - return nil, errors.E(op, fmt.Errorf("could not read bytes of info file: %s", err)) + return nil, errors.E(op, err, errors.M(module), errors.V(version)) } return infoBytes, nil } @@ -43,7 +43,7 @@ func (s *Storage) GoMod(ctx context.Context, module, version string) ([]byte, er defer sp.Finish() exists, err := s.Exists(ctx, module, version) if err != nil { - return nil, errors.E(op, err) + return nil, errors.E(op, err, errors.M(module), errors.V(version)) } if !exists { return nil, errors.E(op, errors.M(module), errors.V(version), errors.KindNotFound) @@ -51,12 +51,12 @@ func (s *Storage) GoMod(ctx context.Context, module, version string) ([]byte, er modReader, err := s.bucket.Open(ctx, config.PackageVersionedName(module, version, "mod")) if err != nil { - return nil, errors.E(op, err) + return nil, errors.E(op, err, errors.M(module), errors.V(version)) } modBytes, err := ioutil.ReadAll(modReader) modReader.Close() if err != nil { - return nil, errors.E(op, fmt.Errorf("could not get new reader for mod file: %s", err)) + return nil, errors.E(op, fmt.Errorf("could not get new reader for mod file: %s", err), errors.M(module), errors.V(version)) } return modBytes, nil @@ -69,7 +69,7 @@ func (s *Storage) Zip(ctx context.Context, module, version string) (io.ReadClose defer sp.Finish() exists, err := s.Exists(ctx, module, version) if err != nil { - return nil, errors.E(op, err) + return nil, errors.E(op, err, errors.M(module), errors.V(version)) } if !exists { return nil, errors.E(op, errors.M(module), errors.V(version), errors.KindNotFound) @@ -77,7 +77,7 @@ func (s *Storage) Zip(ctx context.Context, module, version string) (io.ReadClose zipReader, err := s.bucket.Open(ctx, config.PackageVersionedName(module, version, "zip")) if err != nil { - return nil, errors.E(op, err) + return nil, errors.E(op, err, errors.M(module), errors.V(version)) } return zipReader, nil diff --git a/pkg/storage/gcp/lister.go b/pkg/storage/gcp/lister.go index 7ca50f9b..83e34fb1 100644 --- a/pkg/storage/gcp/lister.go +++ b/pkg/storage/gcp/lister.go @@ -16,7 +16,7 @@ func (s *Storage) List(ctx context.Context, module string) ([]string, error) { defer sp.Finish() paths, err := s.bucket.List(ctx, module) if err != nil { - return nil, err + return nil, errors.E(op, err, errors.M(module)) } versions := extractVersions(paths) if len(versions) < 1 { diff --git a/pkg/storage/gcp/saver.go b/pkg/storage/gcp/saver.go index b92d1401..96fb7fdc 100644 --- a/pkg/storage/gcp/saver.go +++ b/pkg/storage/gcp/saver.go @@ -25,7 +25,7 @@ func (s *Storage) Save(ctx context.Context, module, version string, mod []byte, defer sp.Finish() exists, err := s.Exists(ctx, module, version) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } if exists { return errors.E(op, "already exists", errors.M(module), errors.V(version), errors.KindAlreadyExists) @@ -33,7 +33,7 @@ func (s *Storage) Save(ctx context.Context, module, version string, mod []byte, err = moduploader.Upload(ctx, module, version, bytes.NewReader(info), bytes.NewReader(mod), zip, s.upload) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } // TODO: take out lease on the /list file and add the version to it diff --git a/pkg/storage/minio/checker.go b/pkg/storage/minio/checker.go index 64ea00cf..f2a2ad52 100644 --- a/pkg/storage/minio/checker.go +++ b/pkg/storage/minio/checker.go @@ -26,7 +26,7 @@ func (v *storageImpl) Exists(ctx context.Context, module, version string) (bool, } if err != nil { - return false, errors.E(op, err) + return false, errors.E(op, err, errors.M(module), errors.V(version)) } return true, nil diff --git a/pkg/storage/minio/deleter.go b/pkg/storage/minio/deleter.go index 9a840453..1cc75b16 100644 --- a/pkg/storage/minio/deleter.go +++ b/pkg/storage/minio/deleter.go @@ -14,7 +14,7 @@ func (v *storageImpl) Delete(ctx context.Context, module, version string) error defer sp.Finish() exists, err := v.Exists(ctx, module, version) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } if !exists { @@ -25,18 +25,18 @@ func (v *storageImpl) Delete(ctx context.Context, module, version string) error modPath := fmt.Sprintf("%s/go.mod", versionedPath) if err := v.minioClient.RemoveObject(v.bucketName, modPath); err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } zipPath := fmt.Sprintf("%s/source.zip", versionedPath) if err := v.minioClient.RemoveObject(v.bucketName, zipPath); err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } infoPath := fmt.Sprintf("%s/%s.info", versionedPath, version) err = v.minioClient.RemoveObject(v.bucketName, infoPath) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } return nil } diff --git a/pkg/storage/minio/getter.go b/pkg/storage/minio/getter.go index 55e472f5..6c6c87de 100644 --- a/pkg/storage/minio/getter.go +++ b/pkg/storage/minio/getter.go @@ -49,7 +49,13 @@ func (v *storageImpl) Zip(ctx context.Context, module, vsn string) (io.ReadClose const op errors.Op = "minio.Zip" sp, ctx := opentracing.StartSpanFromContext(ctx, "storage.minio.Zip") defer sp.Finish() + zipPath := fmt.Sprintf("%s/source.zip", v.versionLocation(module, vsn)) + _, err := v.minioClient.StatObject(v.bucketName, zipPath, minio.StatObjectOptions{}) + if err != nil { + return nil, errors.E(op, err, errors.KindNotFound, errors.M(module), errors.V(vsn)) + } + zipReader, err := v.minioClient.GetObject(v.bucketName, zipPath, minio.GetObjectOptions{}) if err != nil { return nil, errors.E(op, err) diff --git a/pkg/storage/minio/lister.go b/pkg/storage/minio/lister.go index 69491346..16450d9a 100644 --- a/pkg/storage/minio/lister.go +++ b/pkg/storage/minio/lister.go @@ -21,7 +21,7 @@ func (l *storageImpl) List(ctx context.Context, module string) ([]string, error) objectCh := l.minioClient.ListObjectsV2(l.bucketName, searchPrefix, false, doneCh) for object := range objectCh { if object.Err != nil { - return nil, errors.E(op, object.Err) + return nil, errors.E(op, object.Err, errors.M(module)) } parts := strings.Split(object.Key, "/") ver := parts[len(parts)-2] @@ -29,7 +29,12 @@ func (l *storageImpl) List(ctx context.Context, module string) ([]string, error) dict[ver] = struct{}{} } } + ret := []string{} + if len(dict) == 0 { + return ret, errors.E(op, errors.M(module), errors.KindNotFound) + } + for ver := range dict { ret = append(ret, ver) } diff --git a/pkg/storage/mongo/checker.go b/pkg/storage/mongo/checker.go index 6ea153e8..dabcfbbd 100644 --- a/pkg/storage/mongo/checker.go +++ b/pkg/storage/mongo/checker.go @@ -4,14 +4,19 @@ import ( "context" "github.com/globalsign/mgo/bson" + "github.com/gomods/athens/pkg/errors" opentracing "github.com/opentracing/opentracing-go" ) // Exists checks for a specific version of a module func (s *ModuleStore) Exists(ctx context.Context, module, vsn string) (bool, error) { + var op errors.Op = "storage.mongo.Exists" sp, ctx := opentracing.StartSpanFromContext(ctx, "storage.mongo.Exists") defer sp.Finish() c := s.s.DB(s.d).C(s.c) count, err := c.Find(bson.M{"module": module, "version": vsn}).Count() - return err == nil && count > 0, err + if err != nil { + return false, errors.E(op, errors.M(module), errors.V(vsn), err) + } + return count > 0, nil } diff --git a/pkg/storage/mongo/deleter.go b/pkg/storage/mongo/deleter.go index 4afeb5de..2a21f9da 100644 --- a/pkg/storage/mongo/deleter.go +++ b/pkg/storage/mongo/deleter.go @@ -15,7 +15,7 @@ func (s *ModuleStore) Delete(ctx context.Context, module, version string) error defer sp.Finish() exists, err := s.Exists(ctx, module, version) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } if !exists { return errors.E(op, errors.M(module), errors.V(version), errors.KindNotFound) @@ -25,11 +25,11 @@ func (s *ModuleStore) Delete(ctx context.Context, module, version string) error c := db.C(s.c) err = db.GridFS("fs").Remove(s.gridFileName(module, version)) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } err = c.Remove(bson.M{"module": module, "version": version}) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } return nil } diff --git a/pkg/storage/mongo/getter.go b/pkg/storage/mongo/getter.go index 9b856ca0..09b652bc 100644 --- a/pkg/storage/mongo/getter.go +++ b/pkg/storage/mongo/getter.go @@ -63,7 +63,7 @@ func (s *ModuleStore) Zip(ctx context.Context, module, vsn string) (io.ReadClose if err == mgo.ErrNotFound { kind = errors.KindNotFound } - return nil, errors.E(op, err, kind) + return nil, errors.E(op, err, kind, errors.M(module), errors.V(vsn)) } return f, nil diff --git a/pkg/storage/mongo/lister.go b/pkg/storage/mongo/lister.go index 9a4cb711..849917c2 100644 --- a/pkg/storage/mongo/lister.go +++ b/pkg/storage/mongo/lister.go @@ -27,6 +27,10 @@ func (s *ModuleStore) List(ctx context.Context, module string) ([]string, error) } versions := make([]string, len(result)) + if len(result) == 0 { + return versions, errors.E(op, errors.M(module), errors.KindNotFound) + } + for i, r := range result { versions[i] = r.Version } diff --git a/pkg/storage/mongo/saver.go b/pkg/storage/mongo/saver.go index d584e2f2..a3b56adc 100644 --- a/pkg/storage/mongo/saver.go +++ b/pkg/storage/mongo/saver.go @@ -19,13 +19,13 @@ func (s *ModuleStore) Save(ctx context.Context, module, version string, mod []by fs := s.s.DB(s.d).GridFS("fs") f, err := fs.Create(zipName) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } defer f.Close() _, err = io.Copy(f, zip) // check number of bytes written? if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } m := &storage.Module{ @@ -38,7 +38,7 @@ func (s *ModuleStore) Save(ctx context.Context, module, version string, mod []by c := s.s.DB(s.d).C(s.c) err = c.Insert(m) if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } return nil diff --git a/pkg/storage/s3/storage.go b/pkg/storage/s3/storage.go index 57cbfcf3..6b5d18d0 100644 --- a/pkg/storage/s3/storage.go +++ b/pkg/storage/s3/storage.go @@ -87,7 +87,7 @@ func (s *Storage) Save(ctx context.Context, module, version string, mod []byte, // // Do that only after module source+metadata is uploaded if err != nil { - return errors.E(op, err) + return errors.E(op, err, errors.M(module), errors.V(version)) } return nil } diff --git a/pkg/storage/storage_tests/module_storage/storage_test.go b/pkg/storage/storage_tests/module_storage/storage_test.go index 6800778b..4df4428d 100644 --- a/pkg/storage/storage_tests/module_storage/storage_test.go +++ b/pkg/storage/storage_tests/module_storage/storage_test.go @@ -36,7 +36,7 @@ type TestSuites struct { func (d *TestSuites) SetupTest() { ra := d.Require() - // + // file system fsTests, err := fs.NewTestSuite(d.Model) ra.NoError(err) d.storages = append(d.storages, fsTests) @@ -71,6 +71,7 @@ func TestModuleStorages(t *testing.T) { func (d *TestSuites) TestStorages() { for _, store := range d.storages { d.testNotFound(store) + d.testKindNotFound(store) d.testGetSaveListRoundTrip(store) d.testList(store) d.testDelete(store) @@ -86,6 +87,34 @@ func (d *TestSuites) testNotFound(ts storage.TestSuite) { d.Require().Equal(true, errors.IsNotFoundErr(err), "Invalid error type for %s: %#v", ts.StorageHumanReadableName(), err) } +func (d *TestSuites) testKindNotFound(ts storage.TestSuite) { + s := ts.Storage() + mod, ver := "xxx", "yyy" + ctx := context.Background() + r := d.Require() + hrn := ts.StorageHumanReadableName() + + err := s.Delete(ctx, mod, ver) + r.Error(err, hrn) + r.Equal(errors.KindNotFound, errors.Kind(err), hrn) + + _, err = s.GoMod(ctx, mod, ver) + r.Error(err, hrn) + r.Equal(errors.KindNotFound, errors.Kind(err), hrn) + + _, err = s.Info(ctx, mod, ver) + r.Error(err, hrn) + r.Equal(errors.KindNotFound, errors.Kind(err), hrn) + + _, err = s.List(ctx, mod) + r.Error(err, hrn) + r.Equal(errors.KindNotFound, errors.Kind(err), hrn) + + _, err = s.Zip(ctx, mod, ver) + r.Error(err, hrn) + r.Equal(errors.KindNotFound, errors.Kind(err), hrn) +} + func (d *TestSuites) testList(ts storage.TestSuite) { r := d.Require() hrn := ts.StorageHumanReadableName()