Repository: kopia/kopia
Pull request #3241 (2023-08-21 19:25:09)

refactor(providers): Create default provider that has common functions
Create a stub struct that has default implementations of trivial functions that providers are expected to implement. This allows for easier additions in the future as the new struct can be modified instead of all provider implementations. Providers that desire different functionality can just override specific functions

Reviewing by commit should help isolate individual sets of changes

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/kopia/kopia/pull/3241?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) Report
Patch coverage: **`77.77%`** and project coverage change: **`+0.07%`** :tada:
> Comparison is base [(`08c58d5`)](https://app.codecov.io/gh/kopia/kopia/commit/08c58d53b610878cac2955bf423754121d93d94f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) 75.38% compared to head [(`5ab8c7f`)](https://app.codecov.io/gh/kopia/kopia/pull/3241?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) 75.46%.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #3241      +/-   ##
==========================================
+ Coverage   75.38%   75.46%   +0.07%     
==========================================
  Files         459      459              
  Lines       36542    36542              
==========================================
+ Hits        27549    27576      +27     
+ Misses       7072     7052      -20     
+ Partials     1921     1914       -7     
```


| [Files Changed](https://app.codecov.io/gh/kopia/kopia/pull/3241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia) | Coverage Δ | |
|---|---|---|
| [repo/blob/filesystem/filesystem\_storage.go](https://app.codecov.io/gh/kopia/kopia/pull/3241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-cmVwby9ibG9iL2ZpbGVzeXN0ZW0vZmlsZXN5c3RlbV9zdG9yYWdlLmdv) | `87.33% <ø> (-0.22%)` | :arrow_down: |
| [repo/blob/readonly/readonly\_storage.go](https://app.codecov.io/gh/kopia/kopia/pull/3241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-cmVwby9ibG9iL3JlYWRvbmx5L3JlYWRvbmx5X3N0b3JhZ2UuZ28=) | `66.66% <ø> (ø)` | |
| [repo/blob/sftp/sftp\_storage.go](https://app.codecov.io/gh/kopia/kopia/pull/3241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-cmVwby9ibG9iL3NmdHAvc2Z0cF9zdG9yYWdlLmdv) | `59.15% <ø> (+0.31%)` | :arrow_up: |
| [repo/blob/sharded/sharded.go](https://app.codecov.io/gh/kopia/kopia/pull/3241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-cmVwby9ibG9iL3NoYXJkZWQvc2hhcmRlZC5nbw==) | `89.02% <ø> (+1.07%)` | :arrow_up: |
| [repo/blob/storage.go](https://app.codecov.io/gh/kopia/kopia/pull/3241?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia#diff-cmVwby9ibG9iL3N0b3JhZ2UuZ28=) | `92.51% <77.77%> (-1.01%)` | :arrow_down: |

... and [7 files with indirect coverage changes](https://app.codecov.io/gh/kopia/kopia/pull/3241/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia)


</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/kopia/kopia/pull/3241?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=kopia).


Patch:
diff --git a/repo/blob/azure/azure_storage.go b/repo/blob/azure/azure_storage.go
index d49983ab..6cdc1c36 100644
--- a/repo/blob/azure/azure_storage.go
+++ b/repo/blob/azure/azure_storage.go
@@ -28,20 +28,12 @@
 
 type azStorage struct {
 	Options
-	blob.UnsupportedBlobRetention
+	blob.DefaultProviderImplementation
 
 	service   *azblob.Client
 	container string
 }
 
-func (az *azStorage) GetCapacity(ctx context.Context) (blob.Capacity, error) {
-	return blob.Capacity{}, blob.ErrNotAVolume
-}
-
-func (az *azStorage) IsReadOnly() bool {
-	return false
-}
-
 func (az *azStorage) GetBlob(ctx context.Context, b blob.ID, offset, length int64, output blob.OutputBuffer) error {
 	if offset < 0 {
 		return errors.Wrap(blob.ErrInvalidRange, "invalid offset")
@@ -234,14 +226,6 @@ func (az *azStorage) DisplayName() string {
 	return fmt.Sprintf("Azure: %v", az.Options.Container)
 }
 
-func (az *azStorage) Close(ctx context.Context) error {
-	return nil
-}
-
-func (az *azStorage) FlushCaches(ctx context.Context) error {
-	return nil
-}
-
 // New creates new Azure Blob Storage-backed storage with specified options:
 //
 // - the 'Container', 'StorageAccount' and 'StorageKey' fields are required and all other parameters are optional.
diff --git a/repo/blob/b2/b2_storage.go b/repo/blob/b2/b2_storage.go
index 96ba3f44..f23403c8 100644
--- a/repo/blob/b2/b2_storage.go
+++ b/repo/blob/b2/b2_storage.go
@@ -26,20 +26,12 @@
 
 type b2Storage struct {
 	Options
-	blob.UnsupportedBlobRetention
+	blob.DefaultProviderImplementation
 
 	cli    *backblaze.B2
 	bucket *backblaze.Bucket
 }
 
-func (s *b2Storage) GetCapacity(ctx context.Context) (blob.Capacity, error) {
-	return blob.Capacity{}, blob.ErrNotAVolume
-}
-
-func (s *b2Storage) IsReadOnly() bool {
-	return false
-}
-
 func (s *b2Storage) GetBlob(ctx context.Context, id blob.ID, offset, length int64, output blob.OutputBuffer) error {
 	fileName := s.getObjectNameString(id)
 
@@ -251,14 +243,6 @@ func (s *b2Storage) DisplayName() string {
 	return fmt.Sprintf("B2: %v", s.BucketName)
 }
 
-func (s *b2Storage) Close(ctx context.Context) error {
-	return nil
-}
-
-func (s *b2Storage) FlushCaches(ctx context.Context) error {
-	return nil
-}
-
 func (s *b2Storage) String() string {
 	return fmt.Sprintf("b2://%s/%s", s.BucketName, s.Prefix)
 }
diff --git a/repo/blob/filesystem/filesystem_storage.go b/repo/blob/filesystem/filesystem_storage.go
index d8895ce5..a2acfaf9 100644
--- a/repo/blob/filesystem/filesystem_storage.go
+++ b/repo/blob/filesystem/filesystem_storage.go
@@ -34,7 +34,7 @@
 
 type fsStorage struct {
 	sharded.Storage
-	blob.UnsupportedBlobRetention
+	blob.DefaultProviderImplementation
 }
 
 type fsImpl struct {
@@ -336,14 +336,6 @@ func (fs *fsStorage) DisplayName() string {
 	return fmt.Sprintf("Filesystem: %v", fs.RootPath)
 }
 
-func (fs *fsStorage) Close(ctx context.Context) error {
-	return nil
-}
-
-func (fs *fsStorage) FlushCaches(ctx context.Context) error {
-	return nil
-}
-
 // New creates new filesystem-backed storage in a specified directory.
 func New(ctx context.Context, opts *Options, isCreate bool) (blob.Storage, error) {
 	var err error
diff --git a/repo/blob/gcs/gcs_storage.go b/repo/blob/gcs/gcs_storage.go
index 11f70c4f..46e5f4b3 100644
--- a/repo/blob/gcs/gcs_storage.go
+++ b/repo/blob/gcs/gcs_storage.go
@@ -32,20 +32,12 @@
 
 type gcsStorage struct {
 	Options
-	blob.UnsupportedBlobRetention
+	blob.DefaultProviderImplementation
 
 	storageClient *gcsclient.Client
 	bucket        *gcsclient.BucketHandle
 }
 
-func (gcs *gcsStorage) GetCapacity(ctx context.Context) (blob.Capacity, error) {
-	return blob.Capacity{}, blob.ErrNotAVolume
-}
-
-func (gcs *gcsStorage) IsReadOnly() bool {
-	return false
-}
-
 func (gcs *gcsStorage) GetBlob(ctx context.Context, b blob.ID, offset, length int64, output blob.OutputBuffer) error {
 	if offset < 0 {
 		return blob.ErrInvalidRange
@@ -213,10 +205,6 @@ func (gcs *gcsStorage) Close(ctx context.Context) error {
 	return errors.Wrap(gcs.storageClient.Close(), "error closing GCS storage")
 }
 
-func (gcs *gcsStorage) FlushCaches(ctx context.Context) error {
-	return nil
-}
-
 func tokenSourceFromCredentialsFile(ctx context.Context, fn string, scopes ...string) (oauth2.TokenSource, error) {
 	data, err := os.ReadFile(fn) //nolint:gosec
 	if err != nil {
diff --git a/repo/blob/gdrive/gdrive_storage.go b/repo/blob/gdrive/gdrive_storage.go
index f5060328..cb1daf8a 100644
--- a/repo/blob/gdrive/gdrive_storage.go
+++ b/repo/blob/gdrive/gdrive_storage.go
@@ -45,7 +45,7 @@
 
 type gdriveStorage struct {
 	Options
-	blob.UnsupportedBlobRetention
+	blob.DefaultProviderImplementation
 
 	client      *drive.FilesService
 	about       *drive.AboutService
@@ -73,10 +73,6 @@ func (gdrive *gdriveStorage) GetCapacity(ctx context.Context) (blob.Capacity, er
 	}, nil
 }
 
-func (gdrive *gdriveStorage) IsReadOnly() bool {
-	return false
-}
-
 func (gdrive *gdriveStorage) GetBlob(ctx context.Context, b blob.ID, offset, length int64, output blob.OutputBuffer) error {
 	if offset < 0 {
 		return blob.ErrInvalidRange
@@ -330,10 +326,6 @@ func (gdrive *gdriveStorage) DisplayName() string {
 	return fmt.Sprintf("Google Drive: %v", gdrive.folderID)
 }
 
-func (gdrive *gdriveStorage) Close(ctx context.Context) error {
-	return nil
-}
-
 func (gdrive *gdriveStorage) FlushCaches(ctx context.Context) error {
 	gdrive.fileIDCache.Clear()
 	return nil
diff --git a/repo/blob/readonly/readonly_storage.go b/repo/blob/readonly/readonly_storage.go
index 257d8ffd..5d01d963 100644
--- a/repo/blob/readonly/readonly_storage.go
+++ b/repo/blob/readonly/readonly_storage.go
@@ -15,7 +15,7 @@
 // readonlyStorage prevents all mutations on the underlying storage.
 type readonlyStorage struct {
 	base blob.Storage
-	blob.UnsupportedBlobRetention
+	blob.DefaultProviderImplementation
 }
 
 func (s readonlyStorage) GetCapacity(ctx context.Context) (blob.Capacity, error) {
diff --git a/repo/blob/s3/s3_storage.go b/repo/blob/s3/s3_storage.go
index 3226afb0..d267a8a3 100644
--- a/repo/blob/s3/s3_storage.go
+++ b/repo/blob/s3/s3_storage.go
@@ -30,20 +30,13 @@
 
 type s3Storage struct {
 	Options
+	blob.DefaultProviderImplementation
 
 	cli *minio.Client
 
 	storageConfig *StorageConfig
 }
 
-func (s *s3Storage) GetCapacity(ctx context.Context) (blob.Capacity, error) {
-	return blob.Capacity{}, blob.ErrNotAVolume
-}
-
-func (s *s3Storage) IsReadOnly() bool {
-	return false
-}
-
 func (s *s3Storage) GetBlob(ctx context.Context, b blob.ID, offset, length int64, output blob.OutputBuffer) error {
 	return s.getBlobWithVersion(ctx, b, latestVersionID, offset, length, output)
 }
@@ -300,10 +293,6 @@ func (s *s3Storage) ConnectionInfo() blob.ConnectionInfo {
 	}
 }
 
-func (s *s3Storage) Close(ctx context.Context) error {
-	return nil
-}
-
 func (s *s3Storage) String() string {
 	return fmt.Sprintf("s3://%v/%v", s.BucketName, s.Prefix)
 }
@@ -312,10 +301,6 @@ func (s *s3Storage) DisplayName() string {
 	return fmt.Sprintf("S3: %v %v", s.Endpoint, s.BucketName)
 }
 
-func (s *s3Storage) FlushCaches(ctx context.Context) error {
-	return nil
-}
-
 func getCustomTransport(opt *Options) (*http.Transport, error) {
 	if opt.DoNotVerifyTLS {
 		//nolint:gosec
diff --git a/repo/blob/sftp/sftp_storage.go b/repo/blob/sftp/sftp_storage.go
index 6b7fb28a..efbeb57e 100644
--- a/repo/blob/sftp/sftp_storage.go
+++ b/repo/blob/sftp/sftp_storage.go
@@ -40,7 +40,7 @@
 // sftpStorage implements blob.Storage on top of sftp.
 type sftpStorage struct {
 	sharded.Storage
-	blob.UnsupportedBlobRetention
+	blob.DefaultProviderImplementation
 }
 
 type sftpImpl struct {
@@ -360,10 +360,6 @@ func writeKnownHostsDataStringToTempFile(data string) (string, error) {
 	return tf.Name(), nil
 }
 
-func (s *sftpStorage) FlushCaches(ctx context.Context) error {
-	return nil
-}
-
 // getHostKeyCallback returns a HostKeyCallback that validates the connected host based on KnownHostsFile or KnownHostsData.
 func getHostKeyCallback(opt *Options) (ssh.HostKeyCallback, error) {
 	if opt.KnownHostsData != "" {
diff --git a/repo/blob/sharded/sharded.go b/repo/blob/sharded/sharded.go
index 23cbdb16..ab46f604 100644
--- a/repo/blob/sharded/sharded.go
+++ b/repo/blob/sharded/sharded.go
@@ -44,11 +44,6 @@ type Storage struct {
 	parameters *Parameters
 }
 
-// IsReadOnly implements blob.Storage.
-func (s *Storage) IsReadOnly() bool {
-	return false
-}
-
 // GetBlob implements blob.Storage.
 func (s *Storage) GetBlob(ctx context.Context, blobID blob.ID, offset, length int64, output blob.OutputBuffer) error {
 	dirPath, filePath, err := s.GetShardedPathAndFilePath(ctx, blobID)
diff --git a/repo/blob/storage.go b/repo/blob/storage.go
index 327c8ad6..d420a636 100644
--- a/repo/blob/storage.go
+++ b/repo/blob/storage.go
@@ -138,14 +138,39 @@ type ExtendOptions struct {
 	RetentionPeriod time.Duration
 }
 
-// UnsupportedBlobRetention provides a default implementation for ExtendBlobRetention.
-type UnsupportedBlobRetention struct{}
+// DefaultProviderImplementation provides a default implementation for
+// common functions that are mostly provider independent and have a sensible
+// default.
+//
+// Storage providers should imbed this struct and override functions that they
+// have different return values for.
+type DefaultProviderImplementation struct{}
 
 // ExtendBlobRetention provides a common implementation for unsupported blob retention storage.
-func (s *UnsupportedBlobRetention) ExtendBlobRetention(context.Context, ID, ExtendOptions) error {
+func (s DefaultProviderImplementation) ExtendBlobRetention(context.Context, ID, ExtendOptions) error {
 	return ErrUnsupportedObjectLock
 }
 
+// IsReadOnly complies with the Storage interface.
+func (s DefaultProviderImplementation) IsReadOnly() bool {
+	return false
+}
+
+// Close complies with the Storage interface.
+func (s DefaultProviderImplementation) Close(context.Context) error {
+	return nil
+}
+
+// FlushCaches complies with the Storage interface.
+func (s DefaultProviderImplementation) FlushCaches(context.Context) error {
+	return nil
+}
+
+// GetCapacity complies with the Storage interface.
+func (s DefaultProviderImplementation) GetCapacity(context.Context) (Capacity, error) {
+	return Capacity{}, ErrNotAVolume
+}
+
 // HasRetentionOptions returns true when blob-retention settings have been
 // specified, otherwise returns false.
 func (o PutOptions) HasRetentionOptions() bool {
diff --git a/repo/blob/webdav/webdav_storage.go b/repo/blob/webdav/webdav_storage.go
index a8b503be..0a4b3179 100644
--- a/repo/blob/webdav/webdav_storage.go
+++ b/repo/blob/webdav/webdav_storage.go
@@ -36,7 +36,7 @@
 // may be accessed using WebDAV or File interchangeably.
 type davStorage struct {
 	sharded.Storage
-	blob.UnsupportedBlobRetention
+	blob.DefaultProviderImplementation
 }
 
 type davStorageImpl struct {
@@ -45,10 +45,6 @@ type davStorageImpl struct {
 	cli *gowebdav.Client
 }
 
-func (d *davStorage) GetCapacity(ctx context.Context) (blob.Capacity, error) {
-	return blob.Capacity{}, blob.ErrNotAVolume
-}
-
 func (d *davStorageImpl) GetBlobFromPath(ctx context.Context, dirPath, path string, offset, length int64, output blob.OutputBuffer) error {
 	_ = dirPath
 
@@ -242,14 +238,6 @@ func (d *davStorage) DisplayName() string {
 	return fmt.Sprintf("WebDAV: %v", o.URL)
 }
 
-func (d *davStorage) Close(ctx context.Context) error {
-	return nil
-}
-
-func (d *davStorage) FlushCaches(ctx context.Context) error {
-	return nil
-}
-
 func isRetriable(err error) bool {
 	var pe *os.PathError
 


Test patch:
diff --git a/internal/blobtesting/map.go b/internal/blobtesting/map.go
index e9cd2d64..39aed327 100644
--- a/internal/blobtesting/map.go
+++ b/internal/blobtesting/map.go
@@ -18,7 +18,7 @@
 type DataMap map[blob.ID][]byte
 
 type mapStorage struct {
-	blob.UnsupportedBlobRetention
+	blob.DefaultProviderImplementation
 	// +checklocks:mutex
 	data DataMap
 	// +checklocks:mutex
@@ -28,14 +28,6 @@ type mapStorage struct {
 	mutex   sync.RWMutex
 }
 
-func (s *mapStorage) GetCapacity(ctx context.Context) (blob.Capacity, error) {
-	return blob.Capacity{}, blob.ErrNotAVolume
-}
-
-func (s *mapStorage) IsReadOnly() bool {
-	return false
-}
-
 func (s *mapStorage) GetBlob(ctx context.Context, id blob.ID, offset, length int64, output blob.OutputBuffer) error {
 	s.mutex.RLock()
 	defer s.mutex.RUnlock()
@@ -166,10 +158,6 @@ func (s *mapStorage) ListBlobs(ctx context.Context, prefix blob.ID, callback fun
 	return nil
 }
 
-func (s *mapStorage) Close(ctx context.Context) error {
-	return nil
-}
-
 func (s *mapStorage) TouchBlob(ctx context.Context, blobID blob.ID, threshold time.Duration) (time.Time, error) {
 	s.mutex.Lock()
 	defer s.mutex.Unlock()
@@ -193,10 +181,6 @@ func (s *mapStorage) DisplayName() string {
 	return "Map"
 }
 
-func (s *mapStorage) FlushCaches(ctx context.Context) error {
-	return nil
-}
-
 // NewMapStorage returns an implementation of Storage backed by the contents of given map.
 // Used primarily for testing.
 func NewMapStorage(data DataMap, keyTime map[blob.ID]time.Time, timeNow func() time.Time) blob.Storage {
diff --git a/internal/blobtesting/object_locking_map.go b/internal/blobtesting/object_locking_map.go
index d4ed624f..eb71b74e 100644
--- a/internal/blobtesting/object_locking_map.go
+++ b/internal/blobtesting/object_locking_map.go
@@ -32,6 +32,7 @@ type entry struct {
 // marker. This struct manages the retention time of each blob through the
 // PutBlob options.
 type objectLockingMap struct {
+	blob.DefaultProviderImplementation
 	// +checklocks:mutex
 	data    versionedEntries
 	timeNow func() time.Time // +checklocksignore
@@ -69,14 +70,6 @@ func (s *objectLockingMap) getLatestForMutationLocked(id blob.ID) (*entry, error
 	return e, nil
 }
 
-func (s *objectLockingMap) GetCapacity(ctx context.Context) (blob.Capacity, error) {
-	return blob.Capacity{}, blob.ErrNotAVolume
-}
-
-func (s *objectLockingMap) IsReadOnly() bool {
-	return false
-}
-
 // GetBlob works the same as map-storage GetBlob except that if the latest
 // version is a delete-marker then it will return ErrBlobNotFound.
 func (s *objectLockingMap) GetBlob(ctx context.Context, id blob.ID, offset, length int64, output blob.OutputBuffer) error {
@@ -266,11 +259,6 @@ func (s *objectLockingMap) ListBlobs(ctx context.Context, prefix blob.ID, callba
 	return nil
 }
 
-// Close is a no-op for this implementation.
-func (s *objectLockingMap) Close(ctx context.Context) error {
-	return nil
-}
-
 // TouchBlob updates the mtime of the latest version of the object. If it is a
 // delete-marker or if it does not exist then this becomes a no-op. If the
 // latest version has retention parameters set then they are respected.
@@ -309,11 +297,6 @@ func (s *objectLockingMap) DisplayName() string {
 	return "VersionedMap"
 }
 
-// FlushCaches is a no-op for this implementation.
-func (s *objectLockingMap) FlushCaches(ctx context.Context) error {
-	return nil
-}
-
 // NewVersionedMapStorage returns an implementation of Storage backed by the
 // contents of an internal in-memory map used primarily for testing.
 func NewVersionedMapStorage(timeNow func() time.Time) RetentionStorage {