From a3481c5f1c3e33a4144f2ecba7d29a25694f0bb2 Mon Sep 17 00:00:00 2001 From: Brian Bland Date: Tue, 18 Nov 2014 17:41:48 -0800 Subject: [PATCH] Adds ability to unwrap ipc errors into their original type This only works for a specific whitelist of error types, which is currently all errors in the storagedriver package. Also improves storagedriver tests to enforce proper error types are returned --- storagedriver/filesystem/driver.go | 11 +++++- storagedriver/ipc/client.go | 18 ++++----- storagedriver/ipc/ipc.go | 54 ++++++++++++++++++++++++-- storagedriver/s3/s3.go | 15 ++++--- storagedriver/testsuites/testsuites.go | 8 ++++ 5 files changed, 85 insertions(+), 21 deletions(-) diff --git a/storagedriver/filesystem/driver.go b/storagedriver/filesystem/driver.go index 46134259..eabb493d 100644 --- a/storagedriver/filesystem/driver.go +++ b/storagedriver/filesystem/driver.go @@ -84,7 +84,7 @@ func (d *Driver) PutContent(subPath string, contents []byte) error { func (d *Driver) ReadStream(path string, offset uint64) (io.ReadCloser, error) { file, err := os.OpenFile(d.subPath(path), os.O_RDONLY, 0644) if err != nil { - return nil, err + return nil, storagedriver.PathNotFoundError{Path: path} } seekPos, err := file.Seek(int64(offset), os.SEEK_SET) @@ -201,7 +201,14 @@ func (d *Driver) List(subPath string) ([]string, error) { // Move moves an object stored at sourcePath to destPath, removing the original // object. func (d *Driver) Move(sourcePath string, destPath string) error { - err := os.Rename(d.subPath(sourcePath), d.subPath(destPath)) + source := d.subPath(sourcePath) + dest := d.subPath(destPath) + + if _, err := os.Stat(source); os.IsNotExist(err) { + return storagedriver.PathNotFoundError{Path: sourcePath} + } + + err := os.Rename(source, dest) return err } diff --git a/storagedriver/ipc/client.go b/storagedriver/ipc/client.go index 332afe1e..51b02b46 100644 --- a/storagedriver/ipc/client.go +++ b/storagedriver/ipc/client.go @@ -126,7 +126,7 @@ func (driver *StorageDriverClient) Start() error { } if response.Error != nil { - return response.Error + return response.Error.Unwrap() } driver.version = response.Version @@ -194,7 +194,7 @@ func (driver *StorageDriverClient) GetContent(path string) ([]byte, error) { } if response.Error != nil { - return nil, response.Error + return nil, response.Error.Unwrap() } defer response.Reader.Close() @@ -226,7 +226,7 @@ func (driver *StorageDriverClient) PutContent(path string, contents []byte) erro } if response.Error != nil { - return response.Error + return response.Error.Unwrap() } return nil @@ -253,7 +253,7 @@ func (driver *StorageDriverClient) ReadStream(path string, offset uint64) (io.Re } if response.Error != nil { - return nil, response.Error + return nil, response.Error.Unwrap() } return response.Reader, nil @@ -280,7 +280,7 @@ func (driver *StorageDriverClient) WriteStream(path string, offset, size uint64, } if response.Error != nil { - return response.Error + return response.Error.Unwrap() } return nil @@ -307,7 +307,7 @@ func (driver *StorageDriverClient) CurrentSize(path string) (uint64, error) { } if response.Error != nil { - return 0, response.Error + return 0, response.Error.Unwrap() } return response.Position, nil @@ -334,7 +334,7 @@ func (driver *StorageDriverClient) List(path string) ([]string, error) { } if response.Error != nil { - return nil, response.Error + return nil, response.Error.Unwrap() } return response.Keys, nil @@ -361,7 +361,7 @@ func (driver *StorageDriverClient) Move(sourcePath string, destPath string) erro } if response.Error != nil { - return response.Error + return response.Error.Unwrap() } return nil @@ -387,7 +387,7 @@ func (driver *StorageDriverClient) Delete(path string) error { } if response.Error != nil { - return response.Error + return response.Error.Unwrap() } return nil diff --git a/storagedriver/ipc/ipc.go b/storagedriver/ipc/ipc.go index 898d10bf..182a1af6 100644 --- a/storagedriver/ipc/ipc.go +++ b/storagedriver/ipc/ipc.go @@ -37,9 +37,13 @@ type Request struct { } // ResponseError is a serializable error type. +// The Type and Parameters may be used to reconstruct the same error on the +// client side, falling back to using the Type and Message if this cannot be +// done. type ResponseError struct { - Type string - Message string + Type string + Message string + Parameters map[string]interface{} } // WrapError wraps an error in a serializable struct containing the error's type @@ -48,10 +52,52 @@ func WrapError(err error) *ResponseError { if err == nil { return nil } - return &ResponseError{ - Type: reflect.TypeOf(err).String(), + v := reflect.ValueOf(err) + re := ResponseError{ + Type: v.Type().String(), Message: err.Error(), } + + if v.Kind() == reflect.Struct { + re.Parameters = make(map[string]interface{}) + for i := 0; i < v.NumField(); i++ { + field := v.Type().Field(i) + re.Parameters[field.Name] = v.Field(i).Interface() + } + } + return &re +} + +// Unwrap returns the underlying error if it can be reconstructed, or the +// original ResponseError otherwise. +func (err *ResponseError) Unwrap() error { + var errVal reflect.Value + var zeroVal reflect.Value + + switch err.Type { + case "storagedriver.PathNotFoundError": + errVal = reflect.ValueOf(&storagedriver.PathNotFoundError{}) + case "storagedriver.InvalidOffsetError": + errVal = reflect.ValueOf(&storagedriver.InvalidOffsetError{}) + } + if errVal == zeroVal { + return err + } + + for k, v := range err.Parameters { + fieldVal := errVal.Elem().FieldByName(k) + if fieldVal == zeroVal { + return err + } + fieldVal.Set(reflect.ValueOf(v)) + } + + if unwrapped, ok := errVal.Elem().Interface().(error); ok { + return unwrapped + } + + return err + } func (err *ResponseError) Error() string { diff --git a/storagedriver/s3/s3.go b/storagedriver/s3/s3.go index 82071b2e..def03e3e 100644 --- a/storagedriver/s3/s3.go +++ b/storagedriver/s3/s3.go @@ -106,7 +106,11 @@ func New(accessKey string, secretKey string, region aws.Region, encrypt bool, bu // GetContent retrieves the content stored at "path" as a []byte. func (d *Driver) GetContent(path string) ([]byte, error) { - return d.Bucket.Get(path) + content, err := d.Bucket.Get(path) + if err != nil { + return nil, storagedriver.PathNotFoundError{Path: path} + } + return content, nil } // PutContent stores the []byte content at a location designated by "path". @@ -121,11 +125,10 @@ func (d *Driver) ReadStream(path string, offset uint64) (io.ReadCloser, error) { headers.Add("Range", "bytes="+strconv.FormatUint(offset, 10)+"-") resp, err := d.Bucket.GetResponseWithHeaders(path, headers) - if resp != nil { - return resp.Body, err + if err != nil { + return nil, storagedriver.PathNotFoundError{Path: path} } - - return nil, err + return resp.Body, nil } // WriteStream stores the contents of the provided io.ReadCloser at a location @@ -242,7 +245,7 @@ func (d *Driver) Move(sourcePath string, destPath string) error { s3.CopyOptions{Options: d.getOptions(), MetadataDirective: "", ContentType: d.getContentType()}, d.Bucket.Name+"/"+sourcePath) if err != nil { - return err + return storagedriver.PathNotFoundError{Path: sourcePath} } return d.Delete(sourcePath) diff --git a/storagedriver/testsuites/testsuites.go b/storagedriver/testsuites/testsuites.go index 217237f7..45633d10 100644 --- a/storagedriver/testsuites/testsuites.go +++ b/storagedriver/testsuites/testsuites.go @@ -126,6 +126,7 @@ func (suite *DriverSuite) TestReadNonexistent(c *check.C) { filename := randomString(32) _, err := suite.StorageDriver.GetContent(filename) c.Assert(err, check.NotNil) + c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{}) } // TestWriteReadStreams1 tests a simple write-read streaming workflow @@ -247,6 +248,7 @@ func (suite *DriverSuite) TestReadNonexistentStream(c *check.C) { filename := randomString(32) _, err := suite.StorageDriver.ReadStream(filename, 0) c.Assert(err, check.NotNil) + c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{}) } // TestList checks the returned list of keys after populating a directory tree @@ -297,6 +299,7 @@ func (suite *DriverSuite) TestMove(c *check.C) { _, err = suite.StorageDriver.GetContent(sourcePath) c.Assert(err, check.NotNil) + c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{}) } // TestMoveNonexistent checks that moving a nonexistent key fails @@ -306,6 +309,7 @@ func (suite *DriverSuite) TestMoveNonexistent(c *check.C) { err := suite.StorageDriver.Move(sourcePath, destPath) c.Assert(err, check.NotNil) + c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{}) } // TestDelete checks that the delete operation removes data from the storage @@ -324,6 +328,7 @@ func (suite *DriverSuite) TestDelete(c *check.C) { _, err = suite.StorageDriver.GetContent(filename) c.Assert(err, check.NotNil) + c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{}) } // TestDeleteNonexistent checks that removing a nonexistent key fails @@ -331,6 +336,7 @@ func (suite *DriverSuite) TestDeleteNonexistent(c *check.C) { filename := randomString(32) err := suite.StorageDriver.Delete(filename) c.Assert(err, check.NotNil) + c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{}) } // TestDeleteFolder checks that deleting a folder removes all child elements @@ -354,9 +360,11 @@ func (suite *DriverSuite) TestDeleteFolder(c *check.C) { _, err = suite.StorageDriver.GetContent(path.Join(dirname, filename1)) c.Assert(err, check.NotNil) + c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{}) _, err = suite.StorageDriver.GetContent(path.Join(dirname, filename2)) c.Assert(err, check.NotNil) + c.Assert(err, check.FitsTypeOf, storagedriver.PathNotFoundError{}) } func (suite *DriverSuite) writeReadCompare(c *check.C, filename string, contents, expected []byte) {