Skip to content

Commit

Permalink
feat(storage): wrap NotFound errors for buckets and objects (#11519)
Browse files Browse the repository at this point in the history 
Code that checks directly against ErrBucketNotExist and ErrObjectNotExist will break;
use `errors.Is()` instead.
  • Loading branch information
@BrennaEpp
BrennaEpp authored Jan 31, 2025
1 parent 8fc5a87 commit 0dd7d3d
Show file tree
Hide file tree
Showing 7 changed files with 63 additions and 86 deletions.
5 changes: 3 additions & 2 deletions storage/example_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package storage_test
import (
"bytes"
"context"
"errors"
"fmt"
"hash/crc32"
"io"
Expand Down Expand Up @@ -878,7 +879,7 @@ func ExampleBucketHandle_exists() {
}

attrs, err := client.Bucket("my-bucket").Attrs(ctx)
if err == storage.ErrBucketNotExist {
if errors.Is(err, storage.ErrBucketNotExist) {
fmt.Println("The bucket does not exist")
return
}
Expand All @@ -896,7 +897,7 @@ func ExampleObjectHandle_exists() {
}

attrs, err := client.Bucket("my-bucket").Object("my-object").Attrs(ctx)
if err == storage.ErrObjectNotExist {
if errors.Is(err, storage.ErrObjectNotExist) {
fmt.Println("The object does not exist")
return
}
Expand Down
29 changes: 10 additions & 19 deletions storage/grpc_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -305,17 +305,11 @@ func (c *grpcStorageClient) GetBucket(ctx context.Context, bucket string, conds
var battrs *BucketAttrs
err := run(ctx, func(ctx context.Context) error {
res, err := c.raw.GetBucket(ctx, req, s.gax...)

battrs = newBucketFromProto(res)

return err
}, s.retry, s.idempotent)

if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
return nil, ErrBucketNotExist
}

return battrs, err
return battrs, formatBucketError(err)
}
func (c *grpcStorageClient) UpdateBucket(ctx context.Context, bucket string, uattrs *BucketAttrsToUpdate, conds *BucketConditions, opts ...storageOption) (*BucketAttrs, error) {
s := callSettings(c.settings, opts...)
Expand Down Expand Up @@ -474,10 +468,7 @@ func (c *grpcStorageClient) ListObjects(ctx context.Context, bucket string, q *Q
return err
}, s.retry, s.idempotent)
if err != nil {
if st, ok := status.FromError(err); ok && st.Code() == codes.NotFound {
err = ErrBucketNotExist
}
return "", err
return "", formatBucketError(err)
}

for _, obj := range objects {
Expand Down Expand Up @@ -519,7 +510,7 @@ func (c *grpcStorageClient) DeleteObject(ctx context.Context, bucket, object str
return c.raw.DeleteObject(ctx, req, s.gax...)
}, s.retry, s.idempotent)
if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
return ErrObjectNotExist
return formatObjectErr(err)
}
return err
}
Expand Down Expand Up @@ -554,7 +545,7 @@ func (c *grpcStorageClient) GetObject(ctx context.Context, params *getObjectPara
}, s.retry, s.idempotent)

if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
return nil, ErrObjectNotExist
return nil, formatObjectErr(err)
}

return attrs, err
Expand Down Expand Up @@ -650,7 +641,7 @@ func (c *grpcStorageClient) UpdateObject(ctx context.Context, params *updateObje
return err
}, s.retry, s.idempotent)
if e, ok := status.FromError(err); ok && e.Code() == codes.NotFound {
return nil, ErrObjectNotExist
return nil, formatObjectErr(err)
}

return attrs, err
Expand All @@ -677,7 +668,7 @@ func (c *grpcStorageClient) RestoreObject(ctx context.Context, params *restoreOb
return err
}, s.retry, s.idempotent)
if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
return nil, ErrObjectNotExist
return nil, formatObjectErr(err)
}
return attrs, err
}
Expand Down Expand Up @@ -707,7 +698,7 @@ func (c *grpcStorageClient) MoveObject(ctx context.Context, params *moveObjectPa
return err
}, s.retry, s.idempotent)
if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
return nil, ErrObjectNotExist
return nil, formatObjectErr(err)
}
return attrs, err
}
Expand Down Expand Up @@ -950,7 +941,7 @@ func (c *grpcStorageClient) ComposeObject(ctx context.Context, req *composeObjec
return err
}, s.retry, s.idempotent); err != nil {
if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
return nil, fmt.Errorf("%w: %w", ErrObjectNotExist, err)
return nil, formatObjectErr(err)
}
return nil, err
}
Expand Down Expand Up @@ -1002,7 +993,7 @@ func (c *grpcStorageClient) RewriteObject(ctx context.Context, req *rewriteObjec

if err := run(ctx, retryCall, s.retry, s.idempotent); err != nil {
if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
return nil, fmt.Errorf("%w: %w", ErrObjectNotExist, err)
return nil, formatObjectErr(err)
}
return nil, err
}
Expand Down Expand Up @@ -1584,7 +1575,7 @@ func (c *grpcStorageClient) NewRangeReader(ctx context.Context, params *newRange
// These types of errors show up on the RecvMsg call, rather than the
// initialization of the stream via BidiReadObject above.
if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
return ErrObjectNotExist
return formatObjectErr(err)
}
if err != nil {
return err
Expand Down
11 changes: 3 additions & 8 deletions storage/grpc_reader.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,8 @@ import (
"cloud.google.com/go/storage/internal/apiv2/storagepb"
"github.com/googleapis/gax-go/v2"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/encoding"
"google.golang.org/grpc/mem"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/encoding/protowire"
"google.golang.org/protobuf/proto"
)
Expand Down Expand Up @@ -146,13 +144,10 @@ func (c *grpcStorageClient) NewRangeReaderReadObject(ctx context.Context, params
// use a custom decoder to avoid an extra copy at the protobuf layer.
databufs := mem.BufferSlice{}
err := stream.RecvMsg(&databufs)
// These types of errors show up on the Recv call, rather than the
// initialization of the stream via ReadObject above.
if s, ok := status.FromError(err); ok && s.Code() == codes.NotFound {
return ErrObjectNotExist
}
if err != nil {
return err
// NotFound types of errors show up on the Recv call, rather than the
// initialization of the stream via ReadObject above.
return formatObjectErr(err)
}
// Use a custom decoder that uses protobuf unmarshalling for all
// fields except the object data. Object data is handled separately
Expand Down
62 changes: 13 additions & 49 deletions storage/http_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -287,12 +287,8 @@ func (c *httpStorageClient) GetBucket(ctx context.Context, bucket string, conds
return err
}, s.retry, s.idempotent)

var e *googleapi.Error
if ok := errors.As(err, &e); ok && e.Code == http.StatusNotFound {
return nil, ErrBucketNotExist
}
if err != nil {
return nil, err
return nil, formatBucketError(err)
}
return newBucket(resp)
}
Expand Down Expand Up @@ -382,11 +378,7 @@ func (c *httpStorageClient) ListObjects(ctx context.Context, bucket string, q *Q
return err
}, s.retry, s.idempotent)
if err != nil {
var e *googleapi.Error
if ok := errors.As(err, &e); ok && e.Code == http.StatusNotFound {
err = ErrBucketNotExist
}
return "", err
return "", formatBucketError(err)
}
for _, item := range resp.Items {
it.items = append(it.items, newObject(item))
Expand Down Expand Up @@ -416,11 +408,7 @@ func (c *httpStorageClient) DeleteObject(ctx context.Context, bucket, object str
req.UserProject(s.userProject)
}
err := run(ctx, func(ctx context.Context) error { return req.Context(ctx).Do() }, s.retry, s.idempotent)
var e *googleapi.Error
if ok := errors.As(err, &e); ok && e.Code == http.StatusNotFound {
return ErrObjectNotExist
}
return err
return formatObjectErr(err)
}

func (c *httpStorageClient) GetObject(ctx context.Context, params *getObjectParams, opts ...storageOption) (*ObjectAttrs, error) {
Expand All @@ -445,12 +433,8 @@ func (c *httpStorageClient) GetObject(ctx context.Context, params *getObjectPara
obj, err = req.Context(ctx).Do()
return err
}, s.retry, s.idempotent)
var e *googleapi.Error
if ok := errors.As(err, &e); ok && e.Code == http.StatusNotFound {
return nil, ErrObjectNotExist
}
if err != nil {
return nil, err
return nil, formatObjectErr(err)
}
return newObject(obj), nil
}
Expand Down Expand Up @@ -555,12 +539,8 @@ func (c *httpStorageClient) UpdateObject(ctx context.Context, params *updateObje
var obj *raw.Object
var err error
err = run(ctx, func(ctx context.Context) error { obj, err = call.Context(ctx).Do(); return err }, s.retry, s.idempotent)
var e *googleapi.Error
if errors.As(err, &e) && e.Code == http.StatusNotFound {
return nil, ErrObjectNotExist
}
if err != nil {
return nil, err
return nil, formatObjectErr(err)
}
return newObject(obj), nil
}
Expand All @@ -585,9 +565,8 @@ func (c *httpStorageClient) RestoreObject(ctx context.Context, params *restoreOb
var obj *raw.Object
var err error
err = run(ctx, func(ctx context.Context) error { obj, err = req.Context(ctx).Do(); return err }, s.retry, s.idempotent)
var e *googleapi.Error
if ok := errors.As(err, &e); ok && e.Code == http.StatusNotFound {
return nil, ErrObjectNotExist
if err != nil {
return nil, formatObjectErr(err)
}
return newObject(obj), err
}
Expand All @@ -610,9 +589,8 @@ func (c *httpStorageClient) MoveObject(ctx context.Context, params *moveObjectPa
var obj *raw.Object
var err error
err = run(ctx, func(ctx context.Context) error { obj, err = req.Context(ctx).Do(); return err }, s.retry, s.idempotent)
var e *googleapi.Error
if ok := errors.As(err, &e); ok && e.Code == http.StatusNotFound {
return nil, ErrObjectNotExist
if err != nil {
return nil, formatObjectErr(err)
}
return newObject(obj), err
}
Expand Down Expand Up @@ -800,11 +778,7 @@ func (c *httpStorageClient) ComposeObject(ctx context.Context, req *composeObjec
retryCall := func(ctx context.Context) error { obj, err = call.Context(ctx).Do(); return err }

if err := run(ctx, retryCall, s.retry, s.idempotent); err != nil {
var e *googleapi.Error
if errors.As(err, &e) && e.Code == http.StatusNotFound {
return nil, fmt.Errorf("%w: %w", ErrObjectNotExist, err)
}
return nil, err
return nil, formatObjectErr(err)
}
return newObject(obj), nil
}
Expand Down Expand Up @@ -851,11 +825,7 @@ func (c *httpStorageClient) RewriteObject(ctx context.Context, req *rewriteObjec
retryCall := func(ctx context.Context) error { res, err = call.Context(ctx).Do(); return err }

if err := run(ctx, retryCall, s.retry, s.idempotent); err != nil {
var e *googleapi.Error
if errors.As(err, &e) && e.Code == http.StatusNotFound {
return nil, fmt.Errorf("%w: %w", ErrObjectNotExist, err)
}
return nil, err
return nil, formatObjectErr(err)
}

r := &rewriteObjectResponse{
Expand Down Expand Up @@ -1420,13 +1390,7 @@ func readerReopen(ctx context.Context, header http.Header, params *newRangeReade
err = run(ctx, func(ctx context.Context) error {
res, err = doDownload(ctx)
if err != nil {
var e *googleapi.Error
if errors.As(err, &e) {
if e.Code == http.StatusNotFound {
return ErrObjectNotExist
}
}
return err
return formatObjectErr(err)
}

if res.StatusCode == http.StatusNotFound {
Expand All @@ -1435,7 +1399,7 @@ func readerReopen(ctx context.Context, header http.Header, params *newRangeReade
return ErrObjectNotExist
}
if res.StatusCode < 200 || res.StatusCode > 299 {
body, _ := ioutil.ReadAll(res.Body)
body, _ := io.ReadAll(res.Body)
res.Body.Close()
return &googleapi.Error{
Code: res.StatusCode,
Expand Down
8 changes: 4 additions & 4 deletions storage/integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -934,7 +934,7 @@ func TestIntegration_BucketCreateDelete(t *testing.T) {
t.Fatalf("BucketHandle.Delete(%q): %v", newBucketName, err)
}
_, err = b.Attrs(ctx)
if err != ErrBucketNotExist {
if !errors.Is(err, ErrBucketNotExist) {
t.Fatalf("expected ErrBucketNotExist, got %v", err)
}
})
Expand Down Expand Up @@ -2442,7 +2442,7 @@ func TestIntegration_Encoding(t *testing.T) {

// Test NotFound.
_, err = bkt.Object("obj-not-exists").NewReader(ctx)
if err != ErrObjectNotExist {
if !errors.Is(err, ErrObjectNotExist) {
t.Errorf("Object should not exist, err found to be %v", err)
}
})
Expand Down Expand Up @@ -3325,11 +3325,11 @@ func TestIntegration_NonexistentBucket(t *testing.T) {
ctx := skipExtraReadAPIs(context.Background(), "no reads in test")
multiTransportTest(ctx, t, func(t *testing.T, ctx context.Context, _, prefix string, client *Client) {
bkt := client.Bucket(prefix + uidSpace.New())
if _, err := bkt.Attrs(ctx); err != ErrBucketNotExist {
if _, err := bkt.Attrs(ctx); !errors.Is(err, ErrBucketNotExist) {
t.Errorf("Attrs: got %v, want ErrBucketNotExist", err)
}
it := bkt.Objects(ctx, nil)
if _, err := it.Next(); err != ErrBucketNotExist {
if _, err := it.Next(); !errors.Is(err, ErrBucketNotExist) {
t.Errorf("Objects: got %v, want ErrBucketNotExist", err)
}
})
Expand Down
30 changes: 28 additions & 2 deletions storage/storage.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,10 @@ import (
raw "google.golang.org/api/storage/v1"
"google.golang.org/api/transport"
htransport "google.golang.org/api/transport/http"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/experimental/stats"
"google.golang.org/grpc/stats/opentelemetry"
"google.golang.org/grpc/status"
"google.golang.org/protobuf/proto"
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/types/known/fieldmaskpb"
Expand All @@ -65,9 +67,11 @@ import (
var signedURLMethods = map[string]bool{"DELETE": true, "GET": true, "HEAD": true, "POST": true, "PUT": true}

var (
// ErrBucketNotExist indicates that the bucket does not exist.
// ErrBucketNotExist indicates that the bucket does not exist. It should be
// checked for using [errors.Is] instead of direct equality.
ErrBucketNotExist = errors.New("storage: bucket doesn't exist")
// ErrObjectNotExist indicates that the object does not exist.
// ErrObjectNotExist indicates that the object does not exist. It should be
// checked for using [errors.Is] instead of direct equality.
ErrObjectNotExist = errors.New("storage: object doesn't exist")
// errMethodNotSupported indicates that the method called is not currently supported by the client.
// TODO: Export this error when launching the transport-agnostic client.
Expand Down Expand Up @@ -2619,3 +2623,25 @@ func applyCondsProto(method string, gen int64, conds *Conditions, msg proto.Mess
}
return nil
}

// formatObjectErr checks if the provided error is NotFound and if so, wraps
// it in an ErrObjectNotExist error. If not, formatObjectErr has no effect.
func formatObjectErr(err error) error {
var e *googleapi.Error
if s, ok := status.FromError(err); (ok && s.Code() == codes.NotFound) ||
(errors.As(err, &e) && e.Code == http.StatusNotFound) {
return fmt.Errorf("%w: %w", ErrObjectNotExist, err)
}
return err
}

// formatBucketError checks if the provided error is NotFound and if so, wraps
// it in an ErrBucketNotExist error. If not, formatBucketError has no effect.
func formatBucketError(err error) error {
var e *googleapi.Error
if s, ok := status.FromError(err); (ok && s.Code() == codes.NotFound) ||
(errors.As(err, &e) && e.Code == http.StatusNotFound) {
return fmt.Errorf("%w: %w", ErrBucketNotExist, err)
}
return err
}
4 changes: 2 additions & 2 deletions storage/transfermanager/integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -560,7 +560,7 @@ func TestIntegration_DownloaderErrorSync(t *testing.T) {

// Check that the nonexistent object returned an error.
if got.Object == nonexistentObject {
if got.Err != storage.ErrObjectNotExist {
if !errors.Is(got.Err, storage.ErrObjectNotExist) {
t.Errorf("Object(%q) should not exist, err found to be %v", got.Object, got.Err)
}
continue
Expand Down Expand Up @@ -718,7 +718,7 @@ func TestIntegration_DownloaderErrorAsync(t *testing.T) {
callbackMu.Unlock()

// Check that the nonexistent object returned an error.
if got.Err != storage.ErrObjectNotExist {
if !errors.Is(got.Err, storage.ErrObjectNotExist) {
t.Errorf("Object(%q) should not exist, err found to be %v", got.Object, got.Err)
}
},
Expand Down

0 comments on commit 0dd7d3d

Please sign in to comment.