From 6fda4cceed7260ef3735d6bb593d8a4cf9dcbdd7 Mon Sep 17 00:00:00 2001 From: Daniel Moran Date: Fri, 15 Oct 2021 15:14:59 -0700 Subject: [PATCH] feat: handle pagination in `bucket list` (#307) --- CHANGELOG.md | 1 + clients/bucket/list.go | 97 ++++++++++++++++++++++++++++++++++--- clients/bucket/list_test.go | 58 ++++++++++++++++++++++ cmd/influx/bucket.go | 16 ++++++ 4 files changed, 165 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a83bfa1..64457e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ 1. [259](https://github.com/influxdata/influx-cli/pull/259): Add `-b` shorthand for `--bucket` to `delete` 1. [285](https://github.com/influxdata/influx-cli/pull/285): Add short-hand `--all-access` and `--operator` options to `auth create`. +1. [307](https://github.com/influxdata/influx-cli/pull/307): Handle pagination in `bucket list` to support displaying more than 20 buckets. ### Bug Fixes diff --git a/clients/bucket/list.go b/clients/bucket/list.go index 71ad002..0a94eea 100644 --- a/clients/bucket/list.go +++ b/clients/bucket/list.go @@ -4,14 +4,18 @@ import ( "context" "fmt" + "github.com/influxdata/influx-cli/v2/api" "github.com/influxdata/influx-cli/v2/clients" ) type BucketsListParams struct { - OrgID string - OrgName string - Name string - ID string + OrgID string + OrgName string + Name string + ID string + Limit int + Offset int + PageSize int } func (c Client) List(ctx context.Context, params *BucketsListParams) error { @@ -29,16 +33,95 @@ func (c Client) List(ctx context.Context, params *BucketsListParams) error { if params.OrgID == "" && params.OrgName == "" { req = req.Org(c.ActiveConfig.Org) } - if params.Name != "" { - req = req.Name(params.Name) + if params.Name != "" || params.ID != "" { + return c.findOneBucket(params, req) } + + // Set the limit for the number of items to return per HTTP request. + // NOTE this is not the same as the `--limit` option to the CLI, which sets + // the max number of rows to pull across _all_ HTTP requests in total. + limit := params.PageSize + // Adjust if the total limit < the per-request limit. + // This is convenient for users, since the per-request limit has a default + // value that people might not want to override on every CLI call. + if params.Limit != 0 && params.Limit < limit { + limit = params.Limit + } + offset := params.Offset + + printOpts := bucketPrintOptions{} + + // Iteratively fetch pages of bucket metadata. + // + // NOTE: This algorithm used an `offset`-based pagination. The API also documents that an `after`-based + // approach would work. Pagination via `after` would typically be much more efficient than the `offset`- + // based approach used here, but we still use `offset` because: + // 1. As of this writing (10/15/2021) Cloud doesn't support pagination via `after` when filtering by org. + // Versions of OSS prior to v2.1 had the same restriction. Both Cloud and OSS have supported `offset`- + // based pagination since before the OSS/Cloud split. + // 2. Listing buckets-by-org uses a secondary index on orgID+bucketName in both OSS and Cloud. Since bucket + // ID isn't part of the index, implementing pagination via `after` requires scanning the subset of the + // index for the target org until the given ID is found. This is just as (in)efficient as the implementation + // of `offset`-based filtering. + // + // If you are thinking of copy-paste-adjusting this code for another command, CONSIDER USING `after` INSTEAD OF + // `offset`! The conditions that make `offset` OK for buckets probably aren't present for other resource types. + // + // We should eventually convert this to use `after` for consistency across the CLI. + for { + req := req + if limit != 0 { + req = req.Limit(int32(limit)) + } + if offset != 0 { + req = req.Offset(int32(offset)) + } + + res, err := req.Execute() + if err != nil { + return fmt.Errorf("failed to list buckets: %w", err) + } + var buckets []api.Bucket + if res.Buckets != nil { + buckets = *res.Buckets + } + printOpts.buckets = append(printOpts.buckets, buckets...) + + // Break out of the loop if: + // * The server returned less than a full page of results + // * We've collected as many results as the user asked for + if len(buckets) == 0 || len(buckets) < params.PageSize || + (params.Limit != 0 && len(printOpts.buckets) == params.Limit) { + break + } + + // Adjust the page-size for the next request (if needed) so we don't pull down more information + // than the user requested. + if params.Limit != 0 && len(printOpts.buckets)+limit > params.Limit { + limit = params.Limit - len(printOpts.buckets) + } + // Bump the offset for the next request to pull the next page. + offset = offset + len(buckets) + } + + return c.printBuckets(printOpts) +} + +// findOneBucket queries for a single bucket resoruce using the list API. +// Used to look up buckets by ID or name. +func (c Client) findOneBucket(params *BucketsListParams, req api.ApiGetBucketsRequest) error { + var description string if params.ID != "" { req = req.Id(params.ID) + description = " by ID" + } else if params.Name != "" { + req = req.Name(params.Name) + description = " by name" } buckets, err := req.Execute() if err != nil { - return fmt.Errorf("failed to list buckets: %w", err) + return fmt.Errorf("failed to find bucket%s: %w", description, err) } printOpts := bucketPrintOptions{} diff --git a/clients/bucket/list_test.go b/clients/bucket/list_test.go index 90bd1ba..63cbc3e 100644 --- a/clients/bucket/list_test.go +++ b/clients/bucket/list_test.go @@ -89,6 +89,62 @@ func TestBucketsList(t *testing.T) { `123\s+my-bucket\s+1h0m0s\s+n/a\s+456`, }, }, + { + name: "pagination via 'offset'", + params: bucket.BucketsListParams{ + PageSize: 2, + Offset: 1, + }, + configOrgName: "my-default-org", + registerBucketExpectations: func(t *testing.T, bucketsApi *mock.MockBucketsApi) { + bucketsApi.EXPECT().GetBuckets(gomock.Any()).Return(api.ApiGetBucketsRequest{ApiService: bucketsApi}) + bucketsApi.EXPECT().GetBucketsExecute(tmock.MatchedBy(func(in api.ApiGetBucketsRequest) bool { + return assert.Equal(t, "my-default-org", *in.GetOrg()) && + assert.Equal(t, int32(2), *in.GetLimit()) && + assert.Equal(t, int32(1), *in.GetOffset()) + })).Return(api.Buckets{ + Buckets: &[]api.Bucket{ + { + Id: api.PtrString("222"), + Name: "my-bucket2", + OrgID: api.PtrString("456"), + RetentionRules: []api.RetentionRule{ + {EverySeconds: 2400}, + }, + }, + { + Id: api.PtrString("333"), + Name: "my-bucket3", + OrgID: api.PtrString("456"), + RetentionRules: []api.RetentionRule{ + {EverySeconds: 3600}, + }, + }, + }, + }, nil) + bucketsApi.EXPECT().GetBucketsExecute(tmock.MatchedBy(func(in api.ApiGetBucketsRequest) bool { + return assert.Equal(t, "my-default-org", *in.GetOrg()) && + assert.Equal(t, int32(2), *in.GetLimit()) && + assert.Equal(t, int32(3), *in.GetOffset()) + })).Return(api.Buckets{ + Buckets: &[]api.Bucket{ + { + Id: api.PtrString("444"), + Name: "my-bucket4", + OrgID: api.PtrString("456"), + RetentionRules: []api.RetentionRule{ + {EverySeconds: 4800}, + }, + }, + }, + }, nil) + }, + expectedStdoutPatterns: []string{ + `222\s+my-bucket2\s+40m0s\s+n/a\s+456`, + `333\s+my-bucket3\s+1h0m0s\s+n/a\s+456`, + `444\s+my-bucket4\s+1h20m0s\s+n/a\s+456`, + }, + }, { name: "override org by ID", params: bucket.BucketsListParams{ @@ -109,6 +165,7 @@ func TestBucketsList(t *testing.T) { name: "override org by name", params: bucket.BucketsListParams{ OrgName: "my-org", + Limit: 2, }, configOrgName: "my-default-org", registerBucketExpectations: func(t *testing.T, bucketsApi *mock.MockBucketsApi) { @@ -148,6 +205,7 @@ func TestBucketsList(t *testing.T) { name: "list multiple bucket schema types", params: bucket.BucketsListParams{ OrgName: "my-org", + Limit: 3, }, configOrgName: "my-default-org", registerBucketExpectations: func(t *testing.T, bucketsApi *mock.MockBucketsApi) { diff --git a/cmd/influx/bucket.go b/cmd/influx/bucket.go index 90eb6dd..7404698 100644 --- a/cmd/influx/bucket.go +++ b/cmd/influx/bucket.go @@ -156,6 +156,22 @@ func newBucketListCmd() cli.Command { EnvVar: "INFLUX_ORG", Destination: ¶ms.OrgName, }, + &cli.IntFlag{ + Name: "limit", + Usage: "Total number of buckets to fetch from the server, or 0 to return all buckets", + Destination: ¶ms.Limit, + }, + &cli.IntFlag{ + Name: "offset", + Usage: "Number of buckets to skip over in the list", + Destination: ¶ms.Offset, + }, + &cli.IntFlag{ + Name: "page-size", + Usage: "Number of buckets to fetch per request to the server", + Value: 20, + Destination: ¶ms.PageSize, + }, ), Action: func(ctx *cli.Context) error { api := getAPI(ctx)