diff --git a/storage/bucket.go b/storage/bucket.go index 83198f18c105..4a2edd5cd93c 100644 --- a/storage/bucket.go +++ b/storage/bucket.go @@ -1435,9 +1435,9 @@ func (b *BucketHandle) Objects(ctx context.Context, q *Query) *ObjectIterator { // on the new handle will use the customized retry configuration. // Retry options set on a object handle will take precedence over options set on // the bucket handle. -// These retry options will merge with the client's retryer (if set) for the -// returned handle. Options passed into this method will take precedence over -// options on the client's retryers. Note that you must explicitly pass in each +// These retry options will merge with the client's retry configuration (if set) +// for the returned handle. Options passed into this method will take precedence +// over retry options on the client. Note that you must explicitly pass in each // option you want to override. func (b *BucketHandle) Retryer(opts ...RetryOption) *BucketHandle { b2 := *b diff --git a/storage/emulator_test.sh b/storage/emulator_test.sh index 092fecf0f4fd..884b5371512a 100755 --- a/storage/emulator_test.sh +++ b/storage/emulator_test.sh @@ -58,7 +58,6 @@ trap cleanup EXIT # TODO: move to passing once fixed FAILING=( "buckets.setIamPolicy" "objects.insert" - "hmacKey.update" ) # TODO: remove regex once all tests are passing # Unfortunately, there is no simple way to skip specific tests (see https://github.com/golang/go/issues/41583) @@ -85,6 +84,7 @@ PASSING=( "buckets.list" "hmacKey.list" "hmacKey.create" "hmacKey.delete" + "hmacKey.update" "notifications.list" "notifications.create" "notifications.get" diff --git a/storage/hmac.go b/storage/hmac.go index 4de5c173d0b4..6f834d4e9edd 100644 --- a/storage/hmac.go +++ b/storage/hmac.go @@ -216,12 +216,12 @@ func (c *Client) CreateHMACKey(ctx context.Context, projectID, serviceAccountEma setClientHeader(call.Header()) var hkPb *raw.HmacKey - var err error - err = run(ctx, func() error { - hkPb, err = call.Context(ctx).Do() + + if err := run(ctx, func() error { + h, err := call.Context(ctx).Do() + hkPb = h return err - }, c.retry, false) - if err != nil { + }, c.retry, false); err != nil { return nil, err } @@ -263,10 +263,11 @@ func (h *HMACKeyHandle) Update(ctx context.Context, au HMACKeyAttrsToUpdate, opt var metadata *raw.HmacKeyMetadata var err error + isIdempotent := len(au.Etag) > 0 err = run(ctx, func() error { metadata, err = call.Context(ctx).Do() return err - }, h.retry, true) + }, h.retry, isIdempotent) if err != nil { return nil, err diff --git a/storage/retry_conformance_test.go b/storage/retry_conformance_test.go index 9a7db2d6d3ec..939e36984447 100644 --- a/storage/retry_conformance_test.go +++ b/storage/retry_conformance_test.go @@ -252,12 +252,14 @@ var methods = map[string][]retryFunc{ "storage.hmacKey.update": { func(ctx context.Context, c *Client, fs *resources, preconditions bool) error { key := c.HMACKeyHandle(projectID, fs.hmacKey.AccessID) + uattrs := HMACKeyAttrsToUpdate{State: "INACTIVE"} - _, err := key.Update(ctx, HMACKeyAttrsToUpdate{State: "INACTIVE"}) - if err != nil { - return err + if preconditions { + uattrs.Etag = fs.hmacKey.Etag } - return fmt.Errorf("Etag preconditions not supported") + + _, err := key.Update(ctx, uattrs) + return err }, }, "storage.objects.compose": { diff --git a/storage/storage.go b/storage/storage.go index aa1ab088b1cc..ffa18accbcd0 100644 --- a/storage/storage.go +++ b/storage/storage.go @@ -1793,8 +1793,8 @@ func setConditionField(call reflect.Value, name string, value interface{}) bool // on the new handle will use the customized retry configuration. // These retry options will merge with the bucket's retryer (if set) for the // returned handle. Options passed into this method will take precedence over -// options on the bucket's and client's retryers. Note that you must explicitly -// pass in each option you want to override. +// retry options on the bucket and client. Note that you must explicitly pass in +// each option you want to override. func (o *ObjectHandle) Retryer(opts ...RetryOption) *ObjectHandle { o2 := *o var retry *retryConfig @@ -1812,13 +1812,14 @@ func (o *ObjectHandle) Retryer(opts ...RetryOption) *ObjectHandle { return &o2 } -// Retryer returns an Client that is configured with custom retry behavior as -// specified by the options that are passed to it. All operations on the new -// handle will use the customized retry configuration. +// SetRetry configures the client with custom retry behavior as specified by the +// options that are passed to it. All operations using this client will use the +// customized retry configuration. +// This should be called once before using the client for network operations, as +// there could be indeterminate behaviour with operations in progress. // Retry options set on a bucket or object handle will take precedence over // these options. -func (c *Client) Retryer(opts ...RetryOption) *Client { - c2 := *c +func (c *Client) SetRetry(opts ...RetryOption) { var retry *retryConfig if c.retry != nil { // merge the options with the existing retry @@ -1829,8 +1830,7 @@ func (c *Client) Retryer(opts ...RetryOption) *Client { for _, opt := range opts { opt.apply(retry) } - c2.retry = retry - return &c2 + c.retry = retry } // RetryOption allows users to configure non-default retry behavior for API diff --git a/storage/storage_test.go b/storage/storage_test.go index 702df794cb99..75410d4f6f8e 100644 --- a/storage/storage_test.go +++ b/storage/storage_test.go @@ -875,32 +875,29 @@ func TestObjectRetryer(t *testing.T) { } } -// Test that Client.Retryer correctly configures the retry configuration +// Test that Client.SetRetry correctly configures the retry configuration // on the Client. -func TestClientRetryer(t *testing.T) { +func TestClientSetRetry(t *testing.T) { testCases := []struct { - name string - call func(c *Client) *Client - want *retryConfig + name string + clientOptions []RetryOption + want *retryConfig }{ { - name: "all defaults", - call: func(c *Client) *Client { - return c.Retryer() - }, - want: &retryConfig{}, + name: "all defaults", + clientOptions: []RetryOption{}, + want: &retryConfig{}, }, { name: "set all options", - call: func(c *Client) *Client { - return c.Retryer( - WithBackoff(gax.Backoff{ - Initial: 2 * time.Second, - Max: 30 * time.Second, - Multiplier: 3, - }), - WithPolicy(RetryAlways), - WithErrorFunc(func(err error) bool { return false })) + clientOptions: []RetryOption{ + WithBackoff(gax.Backoff{ + Initial: 2 * time.Second, + Max: 30 * time.Second, + Multiplier: 3, + }), + WithPolicy(RetryAlways), + WithErrorFunc(func(err error) bool { return false }), }, want: &retryConfig{ backoff: &gax.Backoff{ @@ -914,11 +911,10 @@ func TestClientRetryer(t *testing.T) { }, { name: "set some backoff options", - call: func(c *Client) *Client { - return c.Retryer( - WithBackoff(gax.Backoff{ - Multiplier: 3, - })) + clientOptions: []RetryOption{ + WithBackoff(gax.Backoff{ + Multiplier: 3, + }), }, want: &retryConfig{ backoff: &gax.Backoff{ @@ -927,8 +923,8 @@ func TestClientRetryer(t *testing.T) { }, { name: "set policy only", - call: func(c *Client) *Client { - return c.Retryer(WithPolicy(RetryNever)) + clientOptions: []RetryOption{ + WithPolicy(RetryNever), }, want: &retryConfig{ policy: RetryNever, @@ -936,9 +932,8 @@ func TestClientRetryer(t *testing.T) { }, { name: "set ErrorFunc only", - call: func(c *Client) *Client { - return c.Retryer( - WithErrorFunc(func(err error) bool { return false })) + clientOptions: []RetryOption{ + WithErrorFunc(func(err error) bool { return false }), }, want: &retryConfig{ shouldRetry: func(err error) bool { return false }, @@ -947,7 +942,13 @@ func TestClientRetryer(t *testing.T) { } for _, tc := range testCases { t.Run(tc.name, func(s *testing.T) { - c := tc.call(&Client{}) + c, err := NewClient(context.Background()) + if err != nil { + t.Fatalf("NewClient: %v", err) + } + defer c.Close() + c.SetRetry(tc.clientOptions...) + if diff := cmp.Diff( c.retry, tc.want, @@ -1156,7 +1157,7 @@ func TestRetryer(t *testing.T) { } defer c.Close() if len(tc.clientOptions) > 0 { - c = c.Retryer(tc.clientOptions...) + c.SetRetry(tc.clientOptions...) } b := c.Bucket("buck") if len(tc.bucketOptions) > 0 {