fix(services/s3): Support absolute path and idempotent delete (#937)

* fix(services/s3): Support absolute path and idempotent delete for compatibility * Update error type check * Update abs path on s3
beyondstorage · Oct 24, 2021 · 68c10ce · 68c10ce
1 parent 4dd9b59
commit 68c10ce
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 15 deletions.
diff --git a/services/s3/storage.go b/services/s3/storage.go
@@ -3,6 +3,7 @@ package s3
 import (
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -209,15 +210,20 @@ func (s *Storage) delete(ctx context.Context, path string, opt pairStorageDelete
 	if opt.HasMultipartID {
 		abortInput := s.formatAbortMultipartUploadInput(path, opt)
 
-		// S3 AbortMultipartUpload is idempotent, so we don't need to check NoSuchUpload error.
-		//
-		// References
-		// - [GSP-46](https://github.com/beyondstorage/specs/blob/master/rfcs/46-idempotent-delete.md)
-		// - https://docs.aws.amazon.com/AmazonS3/latest/API/API_AbortMultipartUpload.html
 		_, err = s.service.AbortMultipartUpload(ctx, abortInput)
+
 		if err != nil {
-			return
+			// AbortMultipartUpload is idempotent in s3, but non-idempotent in minio, we need to omit `NoSuchUpload` error for compatibility.
+			// ref: [GSP-46](https://github.com/beyondstorage/specs/blob/master/rfcs/46-idempotent-delete.md)
+			e := &s3types.NoSuchUpload{}
+			if errors.As(err, &e) {
+				err = nil
+			}
+		}
+		if err != nil {
+			return err
 		}
+		return nil
 	}
 
 	input, err := s.formatDeleteObjectInput(path, opt)

diff --git a/services/s3/utils.go b/services/s3/utils.go
@@ -16,6 +16,7 @@ import (
 	s3types "github.com/aws/aws-sdk-go-v2/service/s3/types"
 	"github.com/aws/smithy-go"
 	"github.com/aws/smithy-go/middleware"
+
 	"go.beyondstorage.io/credential"
 	"go.beyondstorage.io/endpoint"
 	ps "go.beyondstorage.io/v5/pairs"
@@ -220,19 +221,23 @@ func formatError(err error) error {
 	}
 
 	e := &smithy.GenericAPIError{}
-	if ok := errors.As(err, &e); !ok {
-		return fmt.Errorf("%w: %v", services.ErrUnexpected, err)
+	if errors.As(err, &e) {
+		switch e.Code {
+		// AWS SDK will use status code to generate awserr.Error, so "NotFound" should also be supported.
+		case "NoSuchKey", "NotFound":
+			return fmt.Errorf("%w: %v", services.ErrObjectNotExist, err)
+		case "AccessDenied":
+			return fmt.Errorf("%w: %v", services.ErrPermissionDenied, err)
+		}
 	}
 
-	switch e.Code {
-	// AWS SDK will use status code to generate awserr.Error, so "NotFound" should also be supported.
-	case "NoSuchKey", "NotFound":
+	noSuchKey := &s3types.NoSuchKey{}
+	notFound := &s3types.NotFound{}
+	if errors.As(err, &noSuchKey) || errors.As(err, &notFound) {
 		return fmt.Errorf("%w: %v", services.ErrObjectNotExist, err)
-	case "AccessDenied":
-		return fmt.Errorf("%w: %v", services.ErrPermissionDenied, err)
-	default:
-		return fmt.Errorf("%w: %v", services.ErrUnexpected, err)
 	}
+
+	return fmt.Errorf("%w: %v", services.ErrUnexpected, err)
 }
 
 // newStorage will create a new client.
@@ -281,6 +286,9 @@ func (s *Service) formatError(op string, err error, name string) error {
 
 // getAbsPath will calculate object storage's abs path
 func (s *Storage) getAbsPath(path string) string {
+	if strings.HasPrefix(path, "/") {
+		return strings.TrimPrefix(path, "/")
+	}
 	prefix := strings.TrimPrefix(s.workDir, "/")
 	return prefix + path
 }