app: rename use-v1-api to use-list-objects-v1 (peak#424)

* app: rename `use-v1-api` to `use-list-objects-v1` Let's be more specific about which V1 of API to use. * changelog: change flag name * storage/s3: add test for `use-list-objects-v1` flag * storage/s3: fix the change to verify if test works * storage/s3: remove unhelpful comments
vijaypabothu · Apr 6, 2022 · 9d49411 · 9d49411
1 parent ef19ae8
commit 9d49411
Show file tree

Hide file tree

Showing 6 changed files with 96 additions and 46 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,7 +9,7 @@
 - Added `--ignore-glacier-warnings` flag to `cp`, `mv` and `select` commands. ([#346](https://github.com/peak/s5cmd/issues/346))
 - Added `--force-glacier-transfer` flag to `select` command. ([#346](https://github.com/peak/s5cmd/issues/346))
 - Added AWS Single Sign-On (SSO) profiles support ([#385](https://github.com/peak/s5cmd/issues/385))
-- Added `--use-v1-api` flag to force using S3 ListObjects API instead of ListObjectsV2 API. ([#405](https://github.com/peak/s5cmd/issues/405)
+- Added `--use-list-objects-v1` flag to force using S3 ListObjects API instead of ListObjectsV2 API. ([#405](https://github.com/peak/s5cmd/issues/405)
 
 #### Improvements
 

diff --git a/README.md b/README.md
@@ -378,10 +378,11 @@ cp, mv, rm, mb ...
 
 ### S3 ListObjects API Backward Compatibility
 
-The `--use-v1-api` flag will force using S3 ListObjects API instead of ListObjectsV2 API.
+The `--use-list-objects-v1` flag will force using S3 ListObjectsV1 API. This
+flag is useful for services that do not support ListObjectsV2 API.
 
 ```
-s5cmd --use-v1-api ls
+s5cmd --use-list-objects-v1 ls s3://bucket/
 ```
 
 ### Specifying credentials

diff --git a/command/app.go b/command/app.go
@@ -70,8 +70,8 @@ var app = &cli.App{
 			Usage: "do not sign requests: credentials will not be loaded if --no-sign-request is provided",
 		},
 		&cli.BoolFlag{
-			Name:  "use-v1-api",
-			Usage: "enable backward compatibility with ListObjects API and disable ListObjectsV2 API",
+			Name:  "use-list-objects-v1",
+			Usage: "use ListObjectsV1 API for services that don't support ListObjectsV2",
 		},
 	},
 	Before: func(c *cli.Context) error {
@@ -138,12 +138,12 @@ var app = &cli.App{
 // NewStorageOpts creates storage.Options object from the given context.
 func NewStorageOpts(c *cli.Context) storage.Options {
 	return storage.Options{
-		MaxRetries:    c.Int("retry-count"),
-		Endpoint:      c.String("endpoint-url"),
-		NoVerifySSL:   c.Bool("no-verify-ssl"),
-		DryRun:        c.Bool("dry-run"),
-		NoSignRequest: c.Bool("no-sign-request"),
-		APIv1Enabled:  c.Bool("use-v1-api"),
+		MaxRetries:       c.Int("retry-count"),
+		Endpoint:         c.String("endpoint-url"),
+		NoVerifySSL:      c.Bool("no-verify-ssl"),
+		DryRun:           c.Bool("dry-run"),
+		NoSignRequest:    c.Bool("no-sign-request"),
+		UseListObjectsV1: c.Bool("use-list-objects-v1"),
 	}
 }
 

diff --git a/storage/s3.go b/storage/s3.go
@@ -53,12 +53,12 @@ var globalSessionCache = &SessionCache{
 // S3 is a storage type which interacts with S3API, DownloaderAPI and
 // UploaderAPI.
 type S3 struct {
-	api         s3iface.S3API
-	downloader  s3manageriface.DownloaderAPI
-	uploader    s3manageriface.UploaderAPI
-	endpointURL urlpkg.URL
-	dryRun      bool
-	v1Enabled   bool
+	api              s3iface.S3API
+	downloader       s3manageriface.DownloaderAPI
+	uploader         s3manageriface.UploaderAPI
+	endpointURL      urlpkg.URL
+	dryRun           bool
+	useListObjectsV1 bool
 }
 
 func parseEndpoint(endpoint string) (urlpkg.URL, error) {
@@ -91,12 +91,12 @@ func newS3Storage(ctx context.Context, opts Options) (*S3, error) {
 	}
 
 	return &S3{
-		api:         s3.New(awsSession),
-		downloader:  s3manager.NewDownloader(awsSession),
-		uploader:    s3manager.NewUploader(awsSession),
-		endpointURL: endpointURL,
-		dryRun:      opts.DryRun,
-		v1Enabled:   opts.APIv1Enabled,
+		api:              s3.New(awsSession),
+		downloader:       s3manager.NewDownloader(awsSession),
+		uploader:         s3manager.NewUploader(awsSession),
+		endpointURL:      endpointURL,
+		dryRun:           opts.DryRun,
+		useListObjectsV1: opts.UseListObjectsV1,
 	}, nil
 }
 
@@ -127,12 +127,10 @@ func (s *S3) Stat(ctx context.Context, url *url.URL) (*Object, error) {
 // keys. If no object found or an error is encountered during this period,
 // it sends these errors to object channel.
 func (s *S3) List(ctx context.Context, url *url.URL, _ bool) <-chan *Object {
-	if isGoogleEndpoint(s.endpointURL) {
-		return s.listObjects(ctx, url)
-	}
-	if s.v1Enabled {
+	if isGoogleEndpoint(s.endpointURL) || s.useListObjectsV1 {
 		return s.listObjects(ctx, url)
 	}
+
 	return s.listObjectsV2(ctx, url)
 }
 

diff --git a/storage/s3_test.go b/storage/s3_test.go
@@ -139,7 +139,7 @@ func TestS3ListURL(t *testing.T) {
 		api: mockApi,
 	}
 
-	mockApi.Handlers.Send.Clear() // mock sending
+	mockApi.Handlers.Send.Clear()
 	mockApi.Handlers.Unmarshal.Clear()
 	mockApi.Handlers.UnmarshalMeta.Clear()
 	mockApi.Handlers.ValidateResponse.Clear()
@@ -241,7 +241,7 @@ func TestS3ListNoItemFound(t *testing.T) {
 		api: mockApi,
 	}
 
-	mockApi.Handlers.Send.Clear() // mock sending
+	mockApi.Handlers.Send.Clear()
 	mockApi.Handlers.Unmarshal.Clear()
 	mockApi.Handlers.UnmarshalMeta.Clear()
 	mockApi.Handlers.ValidateResponse.Clear()
@@ -457,7 +457,7 @@ func TestS3Retry(t *testing.T) {
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
 
-			mockApi.Handlers.Send.Clear() // mock sending
+			mockApi.Handlers.Send.Clear()
 			mockApi.Handlers.Unmarshal.Clear()
 			mockApi.Handlers.UnmarshalMeta.Clear()
 			mockApi.Handlers.ValidateResponse.Clear()
@@ -985,6 +985,57 @@ func TestSessionAutoRegion(t *testing.T) {
 	}
 }
 
+func TestS3ListObjectsAPIVersions(t *testing.T) {
+	url, err := url.New("s3://bucket/key")
+	if err != nil {
+		t.Errorf("unexpected error: %v", err)
+	}
+
+	mockApi := s3.New(unit.Session)
+	mockS3 := &S3{api: mockApi}
+
+	mockApi.Handlers.Send.Clear()
+	mockApi.Handlers.Unmarshal.Clear()
+	mockApi.Handlers.UnmarshalMeta.Clear()
+	mockApi.Handlers.ValidateResponse.Clear()
+
+	t.Run("list-objects-v2", func(t *testing.T) {
+		var got interface{}
+		mockApi.Handlers.ValidateResponse.PushBack(func(r *request.Request) {
+			got = r.Data
+		})
+
+		ctx := context.Background()
+		mockS3.useListObjectsV1 = false
+		for range mockS3.List(ctx, url, false) {
+		}
+
+		expected := &s3.ListObjectsV2Output{}
+
+		if reflect.TypeOf(expected) != reflect.TypeOf(got) {
+			t.Errorf("expected %T, got: %T", expected, got)
+		}
+	})
+
+	t.Run("list-objects-v1", func(t *testing.T) {
+		var got interface{}
+		mockApi.Handlers.ValidateResponse.PushBack(func(r *request.Request) {
+			got = r.Data
+		})
+
+		ctx := context.Background()
+		mockS3.useListObjectsV1 = true
+		for range mockS3.List(ctx, url, false) {
+		}
+
+		expected := &s3.ListObjectsOutput{}
+
+		if reflect.TypeOf(expected) != reflect.TypeOf(got) {
+			t.Errorf("expected %T, got: %T", expected, got)
+		}
+	})
+}
+
 func valueAtPath(i interface{}, s string) interface{} {
 	v, err := awsutil.ValuesAtPath(i, s)
 	if err != nil || len(v) == 0 {

diff --git a/storage/storage.go b/storage/storage.go
@@ -48,14 +48,14 @@ func NewLocalClient(opts Options) *Filesystem {
 
 func NewRemoteClient(ctx context.Context, url *url.URL, opts Options) (*S3, error) {
 	newOpts := Options{
-		MaxRetries:    opts.MaxRetries,
-		Endpoint:      opts.Endpoint,
-		NoVerifySSL:   opts.NoVerifySSL,
-		DryRun:        opts.DryRun,
-		NoSignRequest: opts.NoSignRequest,
-		APIv1Enabled:  opts.APIv1Enabled,
-		bucket:        url.Bucket,
-		region:        opts.region,
+		MaxRetries:       opts.MaxRetries,
+		Endpoint:         opts.Endpoint,
+		NoVerifySSL:      opts.NoVerifySSL,
+		DryRun:           opts.DryRun,
+		NoSignRequest:    opts.NoSignRequest,
+		UseListObjectsV1: opts.UseListObjectsV1,
+		bucket:           url.Bucket,
+		region:           opts.region,
 	}
 	return newS3Storage(ctx, newOpts)
 }
@@ -69,14 +69,14 @@ func NewClient(ctx context.Context, url *url.URL, opts Options) (Storage, error)
 
 // Options stores configuration for storage.
 type Options struct {
-	MaxRetries    int
-	Endpoint      string
-	NoVerifySSL   bool
-	DryRun        bool
-	NoSignRequest bool
-	APIv1Enabled  bool
-	bucket        string
-	region        string
+	MaxRetries       int
+	Endpoint         string
+	NoVerifySSL      bool
+	DryRun           bool
+	NoSignRequest    bool
+	UseListObjectsV1 bool
+	bucket           string
+	region           string
 }
 
 func (o *Options) SetRegion(region string) {