From 79e3c67bbdb0722fd211a67f8a7c5c034a655a15 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Sat, 25 Feb 2017 11:09:57 +0000 Subject: [PATCH] local, yandex, dropbox: fix NewObject suceeding on a directory #1079 Add tests to make it consistent across all remotes --- amazonclouddrive/amazonclouddrive_test.go | 1 + b2/b2_test.go | 1 + crypt/crypt2_test.go | 1 + crypt/crypt_test.go | 1 + drive/drive_test.go | 1 + dropbox/dropbox.go | 36 +++++++++++-------- dropbox/dropbox_test.go | 1 + fs/fs.go | 1 + fstest/fstests/fstests.go | 11 +++++- googlecloudstorage/googlecloudstorage_test.go | 1 + hubic/hubic_test.go | 1 + local/local.go | 3 ++ local/local_test.go | 1 + onedrive/onedrive_test.go | 1 + s3/s3_test.go | 1 + sftp/sftp.go | 4 +-- sftp/sftp_test.go | 4 +-- swift/swift_test.go | 1 + yandex/yandex.go | 28 ++++++++------- yandex/yandex_test.go | 1 + 20 files changed, 66 insertions(+), 34 deletions(-) diff --git a/amazonclouddrive/amazonclouddrive_test.go b/amazonclouddrive/amazonclouddrive_test.go index 657afc1dd..57e235221 100644 --- a/amazonclouddrive/amazonclouddrive_test.go +++ b/amazonclouddrive/amazonclouddrive_test.go @@ -37,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/b2/b2_test.go b/b2/b2_test.go index d3bc2546a..fa9cd9ede 100644 --- a/b2/b2_test.go +++ b/b2/b2_test.go @@ -37,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/crypt/crypt2_test.go b/crypt/crypt2_test.go index 0af830b05..ed78eeeea 100644 --- a/crypt/crypt2_test.go +++ b/crypt/crypt2_test.go @@ -38,6 +38,7 @@ func TestFsListSubdir2(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel22(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile12(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject2(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir2(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and22(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy2(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove2(t *testing.T) { fstests.TestFsMove(t) } diff --git a/crypt/crypt_test.go b/crypt/crypt_test.go index 7a96b7bb2..f7942ddc4 100644 --- a/crypt/crypt_test.go +++ b/crypt/crypt_test.go @@ -38,6 +38,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/drive/drive_test.go b/drive/drive_test.go index 1b0f50d22..61e62847f 100644 --- a/drive/drive_test.go +++ b/drive/drive_test.go @@ -37,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/dropbox/dropbox.go b/dropbox/dropbox.go index c19a1db83..a9b07610e 100644 --- a/dropbox/dropbox.go +++ b/dropbox/dropbox.go @@ -207,18 +207,18 @@ func (f *Fs) setRoot(root string) { // Return an Object from a path // // If it can't be found it returns the error fs.ErrorObjectNotFound. -func (f *Fs) newObjectWithInfo(remote string, info *dropbox.Entry) (fs.Object, error) { - o := &Object{ +func (f *Fs) newObjectWithInfo(remote string, info *dropbox.Entry) (o *Object, err error) { + o = &Object{ fs: f, remote: remote, } if info != nil { - o.setMetadataFromEntry(info) + err = o.setMetadataFromEntry(info) } else { - err := o.readEntryAndSetMetadata() - if err != nil { - return nil, err - } + err = o.readEntryAndSetMetadata() + } + if err != nil { + return nil, err } return o, nil } @@ -512,7 +512,10 @@ func (f *Fs) Copy(src fs.Object, remote string) (fs.Object, error) { if err != nil { return nil, errors.Wrap(err, "copy failed") } - dstObj.setMetadataFromEntry(entry) + err = dstObj.setMetadataFromEntry(entry) + if err != nil { + return nil, errors.Wrap(err, "copy failed") + } return dstObj, nil } @@ -555,7 +558,10 @@ func (f *Fs) Move(src fs.Object, remote string) (fs.Object, error) { if err != nil { return nil, errors.Wrap(err, "move failed") } - dstObj.setMetadataFromEntry(entry) + err = dstObj.setMetadataFromEntry(entry) + if err != nil { + return nil, errors.Wrap(err, "move failed") + } return dstObj, nil } @@ -631,11 +637,15 @@ func (o *Object) Size() int64 { // setMetadataFromEntry sets the fs data from a dropbox.Entry // // This isn't a complete set of metadata and has an inacurate date -func (o *Object) setMetadataFromEntry(info *dropbox.Entry) { +func (o *Object) setMetadataFromEntry(info *dropbox.Entry) error { + if info.IsDir { + return errors.Wrapf(fs.ErrorNotAFile, "%q", o.remote) + } o.bytes = info.Bytes o.modTime = time.Time(info.ClientMtime) o.mimeType = info.MimeType o.hasMetadata = true + return nil } // Reads the entry from dropbox @@ -662,8 +672,7 @@ func (o *Object) readEntryAndSetMetadata() error { if err != nil { return err } - o.setMetadataFromEntry(entry) - return nil + return o.setMetadataFromEntry(entry) } // Returns the remote path for the object @@ -761,8 +770,7 @@ func (o *Object) Update(in io.Reader, src fs.ObjectInfo) error { if err != nil { return errors.Wrap(err, "upload failed") } - o.setMetadataFromEntry(entry) - return nil + return o.setMetadataFromEntry(entry) } // Remove an object diff --git a/dropbox/dropbox_test.go b/dropbox/dropbox_test.go index 3d5b87a06..c883cba6c 100644 --- a/dropbox/dropbox_test.go +++ b/dropbox/dropbox_test.go @@ -37,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/fs/fs.go b/fs/fs.go index 923491887..0604620f6 100644 --- a/fs/fs.go +++ b/fs/fs.go @@ -42,6 +42,7 @@ var ( ErrorListAborted = errors.New("list aborted") ErrorListOnlyRoot = errors.New("can only list from root") ErrorIsFile = errors.New("is a file not a directory") + ErrorNotAFile = errors.New("is a not a regular file") ErrorNotDeleting = errors.New("not deleting files as there were IO errors") ErrorCantMoveOverlapping = errors.New("can't move files on overlapping remotes") ) diff --git a/fstest/fstests/fstests.go b/fstest/fstests/fstests.go index 984a803c9..a384d04ef 100644 --- a/fstest/fstests/fstests.go +++ b/fstest/fstests/fstests.go @@ -7,7 +7,6 @@ package fstests import ( "bytes" - "errors" "flag" "fmt" "io" @@ -21,6 +20,7 @@ import ( "github.com/ncw/rclone/fs" "github.com/ncw/rclone/fstest" + "github.com/pkg/errors" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -403,6 +403,15 @@ func TestFsListFile1and2(t *testing.T) { fstest.CheckListing(t, remote, []fstest.Item{file1, file2}) } +// TestFsNewObjectDir tests NewObject on a directory which should produce an error +func TestFsNewObjectDir(t *testing.T) { + skipIfNotOk(t) + dir := path.Dir(file2.Path) + obj, err := remote.NewObject(dir) + assert.Nil(t, obj) + assert.NotNil(t, err) +} + // TestFsCopy tests Copy func TestFsCopy(t *testing.T) { skipIfNotOk(t) diff --git a/googlecloudstorage/googlecloudstorage_test.go b/googlecloudstorage/googlecloudstorage_test.go index 80aa3af52..7bc11da5c 100644 --- a/googlecloudstorage/googlecloudstorage_test.go +++ b/googlecloudstorage/googlecloudstorage_test.go @@ -37,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/hubic/hubic_test.go b/hubic/hubic_test.go index 1b97d5b7a..50ed80e7e 100644 --- a/hubic/hubic_test.go +++ b/hubic/hubic_test.go @@ -37,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/local/local.go b/local/local.go index b0b58f310..30b82b3f4 100644 --- a/local/local.go +++ b/local/local.go @@ -162,6 +162,9 @@ func (f *Fs) newObjectWithInfo(remote string, info os.FileInfo) (fs.Object, erro return nil, err } } + if !o.info.Mode().IsRegular() { + return nil, errors.Wrapf(fs.ErrorNotAFile, "%q", remote) + } return o, nil } diff --git a/local/local_test.go b/local/local_test.go index 4dd685cf9..7eedd7460 100644 --- a/local/local_test.go +++ b/local/local_test.go @@ -37,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/onedrive/onedrive_test.go b/onedrive/onedrive_test.go index 3b4e3fc8c..9adf43ebd 100644 --- a/onedrive/onedrive_test.go +++ b/onedrive/onedrive_test.go @@ -37,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/s3/s3_test.go b/s3/s3_test.go index ca5998070..b064e51b6 100644 --- a/s3/s3_test.go +++ b/s3/s3_test.go @@ -37,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/sftp/sftp.go b/sftp/sftp.go index 88a439773..6a08b2a1d 100644 --- a/sftp/sftp.go +++ b/sftp/sftp.go @@ -480,9 +480,7 @@ func (o *Object) stat() error { return errors.Wrap(err, "stat failed") } if info.IsDir() { - // FIXME this error message doesn't seem right, but it - // is necessary for the NewFS code - return fs.ErrorObjectNotFound + return errors.Wrapf(fs.ErrorNotAFile, "%q", o.remote) } o.info = info return nil diff --git a/sftp/sftp_test.go b/sftp/sftp_test.go index 2e1c01db1..26b20bc30 100644 --- a/sftp/sftp_test.go +++ b/sftp/sftp_test.go @@ -2,9 +2,6 @@ // // Automatically generated - DO NOT EDIT // Regenerate with: make gen_tests - -// +build !plan9 - package sftp_test import ( @@ -40,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/swift/swift_test.go b/swift/swift_test.go index f67a81b3c..75910f167 100644 --- a/swift/swift_test.go +++ b/swift/swift_test.go @@ -37,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) } diff --git a/yandex/yandex.go b/yandex/yandex.go index bd5aa8b6d..0a9b99e44 100644 --- a/yandex/yandex.go +++ b/yandex/yandex.go @@ -313,24 +313,27 @@ func (f *Fs) NewObject(remote string) (fs.Object, error) { // Return an Object from a path // // If it can't be found it returns the error fs.ErrorObjectNotFound. -func (f *Fs) newObjectWithInfo(remote string, info *yandex.ResourceInfoResponse) (fs.Object, error) { - o := &Object{ +func (f *Fs) newObjectWithInfo(remote string, info *yandex.ResourceInfoResponse) (o *Object, err error) { + o = &Object{ fs: f, remote: remote, } if info != nil { - o.setMetaData(info) + err = o.setMetaData(info) } else { - err := o.readMetaData() - if err != nil { - return nil, err - } + err = o.readMetaData() + } + if err != nil { + return nil, err } return o, nil } // setMetaData sets the fs data from a storage.Object -func (o *Object) setMetaData(info *yandex.ResourceInfoResponse) { +func (o *Object) setMetaData(info *yandex.ResourceInfoResponse) (err error) { + if info.ResourceType != "file" { + return errors.Wrapf(fs.ErrorNotAFile, "%q", o.remote) + } o.bytes = info.Size o.md5sum = info.Md5 o.mimeType = info.MimeType @@ -347,10 +350,10 @@ func (o *Object) setMetaData(info *yandex.ResourceInfoResponse) { } t, err := time.Parse(time.RFC3339Nano, modTimeString) if err != nil { - fs.Logf("Failed to parse modtime from %q: %v", modTimeString, err) - } else { - o.modTime = t + return errors.Wrapf(err, "failed to parse modtime from %q", modTimeString) } + o.modTime = t + return nil } // readMetaData gets the info if it hasn't already been fetched @@ -371,8 +374,7 @@ func (o *Object) readMetaData() (err error) { } return err } - o.setMetaData(ResourceInfoResponse) - return nil + return o.setMetaData(ResourceInfoResponse) } // Put the object diff --git a/yandex/yandex_test.go b/yandex/yandex_test.go index bb2f8dd34..99dd2537e 100644 --- a/yandex/yandex_test.go +++ b/yandex/yandex_test.go @@ -37,6 +37,7 @@ func TestFsListSubdir(t *testing.T) { fstests.TestFsListSubdir(t) } func TestFsListLevel2(t *testing.T) { fstests.TestFsListLevel2(t) } func TestFsListFile1(t *testing.T) { fstests.TestFsListFile1(t) } func TestFsNewObject(t *testing.T) { fstests.TestFsNewObject(t) } +func TestFsNewObjectDir(t *testing.T) { fstests.TestFsNewObjectDir(t) } func TestFsListFile1and2(t *testing.T) { fstests.TestFsListFile1and2(t) } func TestFsCopy(t *testing.T) { fstests.TestFsCopy(t) } func TestFsMove(t *testing.T) { fstests.TestFsMove(t) }