local, yandex, dropbox: fix NewObject suceeding on a directory #1079

Add tests to make it consistent across all remotes

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) }

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) }

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) }

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) }

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) }

dropbox/dropbox.go

@@ -207,19 +207,19 @@ 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()
+		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

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) }

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")
 )

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)

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) }

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) }

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
 }
 

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) }

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) }

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) }

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

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) }

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) }

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()
+		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

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) }