fixed #82 Return error when CopyTo/MoveTo functions are called when S…

…eek offset is not (0,0) for all backends, (#83)

not just GCS

CHANGELOG.md

@@ -4,6 +4,9 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 ## [Unreleased]
+- fixed #82 Return error when CopyTo/MoveTo functions are called when Seek offset is not (0,0) for all backends,
+not just GCS.
+
 ## [5.7.0] - 2021-07-23
 ### Added
 - Add support of keyexchanges algorithm as a sftp option

backend/azure/file.go

@@ -12,6 +12,7 @@ import (
 	"github.com/Azure/azure-storage-blob-go/azblob"
 
 	"github.com/c2fo/vfs/v5"
+	"github.com/c2fo/vfs/v5/backend"
 	"github.com/c2fo/vfs/v5/utils"
 )
 
@@ -29,7 +30,7 @@ type File struct {
 func (f *File) Close() error {
 	if f.tempFile != nil {
 		defer func() {
-			f.tempFile.Close()
+			_ = f.tempFile.Close()
 			f.tempFile = nil
 			f.isDirty = false
 		}()
@@ -137,6 +138,11 @@ func (f *File) CopyToLocation(location vfs.Location) (vfs.File, error) {
 
 // CopyToFile puts the contents of the receiver (f *File) into the passed vfs.File parameter.
 func (f *File) CopyToFile(file vfs.File) error {
+	// validate seek is at 0,0 before doing copy
+	if err := backend.ValidateCopySeekPosition(f); err != nil {
+		return err
+	}
+
 	azFile, ok := file.(*File)
 	if ok {
 		if f.isSameAuth(azFile) {

backend/azure/file_test.go

@@ -122,7 +122,8 @@ func (s *FileTestSuite) TestLocation() {
 }
 
 func (s *FileTestSuite) TestCopyToLocation() {
-	client := MockAzureClient{}
+	fooReader := ioutil.NopCloser(strings.NewReader("blah"))
+	client := MockAzureClient{ExpectedResult: fooReader}
 	fs := NewFileSystem().WithClient(&client)
 	source, _ := fs.NewFile("test-container", "/foo.txt")
 	targetLoc, _ := fs.NewLocation("test-container", "/new/folder/")
@@ -133,7 +134,8 @@ func (s *FileTestSuite) TestCopyToLocation() {
 }
 
 func (s *FileTestSuite) TestCopyToFile() {
-	client := MockAzureClient{}
+	fooReader := ioutil.NopCloser(strings.NewReader("blah"))
+	client := MockAzureClient{ExpectedResult: fooReader}
 	fs := NewFileSystem().WithClient(&client)
 	source, _ := fs.NewFile("test-container", "/foo.txt")
 	target, _ := fs.NewFile("test-container", "/bar.txt")
@@ -143,7 +145,8 @@ func (s *FileTestSuite) TestCopyToFile() {
 }
 
 func (s *FileTestSuite) TestMoveToLocation() {
-	client := MockAzureClient{}
+	fooReader := ioutil.NopCloser(strings.NewReader("blah"))
+	client := MockAzureClient{ExpectedResult: fooReader}
 	fs := NewFileSystem().WithClient(&client)
 	source, _ := fs.NewFile("test-container", "/foo.txt")
 	target, _ := fs.NewLocation("test-container", "/new/folder/")
@@ -155,7 +158,8 @@ func (s *FileTestSuite) TestMoveToLocation() {
 }
 
 func (s *FileTestSuite) TestMoveToFile() {
-	client := MockAzureClient{}
+	fooReader := ioutil.NopCloser(strings.NewReader("blah"))
+	client := MockAzureClient{ExpectedResult: fooReader}
 	fs := NewFileSystem().WithClient(&client)
 	source, _ := fs.NewFile("test-container", "/foo.txt")
 	target, _ := fs.NewFile("test-container", "/bar.txt")

backend/gs/file.go

@@ -13,6 +13,7 @@ import (
 	"cloud.google.com/go/storage"
 
 	"github.com/c2fo/vfs/v5"
+	"github.com/c2fo/vfs/v5/backend"
 	"github.com/c2fo/vfs/v5/utils"
 )
 
@@ -33,7 +34,7 @@ type File struct {
 // local temp file, and triggers a write to GCS of anything in the f.writeBuffer if it has been created.
 func (f *File) Close() error {
 	if f.tempFile != nil {
-		defer f.tempFile.Close()
+		defer func() { _ = f.tempFile.Close() }()
 
 		err := os.Remove(f.tempFile.Name())
 		if err != nil && !os.IsNotExist(err) {
@@ -53,7 +54,7 @@ func (f *File) Close() error {
 		ctx, cancel := context.WithCancel(f.fileSystem.ctx)
 		defer cancel()
 		w := handle.NewWriter(ctx)
-		defer w.Close()
+		defer func() { _ = w.Close() }()
 		if _, err := io.Copy(w, f.writeBuffer); err != nil {
 			// cancel context (replaces CloseWithError)
 			return err
@@ -146,6 +147,12 @@ func (f *File) CopyToLocation(location vfs.Location) (vfs.File, error) {
 // method if the target file is also on GCS, otherwise uses io.Copy.
 // This method should be called on a closed file or a file with 0 cursor position to avoid errors.
 func (f *File) CopyToFile(file vfs.File) error {
+	// validate seek is at 0,0 before doing copy
+	if err := backend.ValidateCopySeekPosition(f); err != nil {
+		return err
+	}
+
+	// do native copy if same location/auth
 	if tf, ok := file.(*File); ok {
 		options, ok := tf.Location().FileSystem().(*FileSystem).options.(Options)
 		if ok {
@@ -154,13 +161,8 @@ func (f *File) CopyToFile(file vfs.File) error {
 			}
 		}
 	}
-	offset, err := f.Seek(0, io.SeekCurrent)
-	if err != nil {
-		return fmt.Errorf("failed to determine current cursor offset: %w", err)
-	}
-	if offset != 0 {
-		return fmt.Errorf("current cursor offset is not 0 as required for this operation")
-	}
+
+	// do non-native io copy
 	if err := utils.TouchCopy(file, f); err != nil {
 		return err
 	}

backend/helpers.go

@@ -0,0 +1,21 @@
+package backend
+
+import (
+	"fmt"
+	"io"
+
+	"github.com/c2fo/vfs/v5"
+)
+
+func ValidateCopySeekPosition(f vfs.File) error {
+	// validate seek is at 0,0 before doing copy
+	offset, err := f.Seek(0, io.SeekCurrent)
+	if err != nil {
+		return fmt.Errorf("failed to determine current cursor offset: %w", err)
+	}
+	if offset != 0 {
+		return vfs.CopyToNotPossible
+	}
+
+	return nil
+}

backend/mem/file.go

@@ -9,6 +9,7 @@ import (
 	"time"
 
 	"github.com/c2fo/vfs/v5"
+	"github.com/c2fo/vfs/v5/backend"
 	"github.com/c2fo/vfs/v5/utils"
 )
 
@@ -156,7 +157,7 @@ func (f *File) Seek(offset int64, whence int) (int64, error) {
 		}
 	case 1:
 		pos := f.cursor + int(offset)
-		if pos < length && pos >= 0 {
+		if pos <= length && pos >= 0 {
 			f.cursor = pos
 			return int64(pos), nil
 		}
@@ -289,6 +290,11 @@ func (f *File) CopyToFile(target vfs.File) error {
 		return nilReference()
 	}
 
+	// validate seek is at 0,0 before doing copy
+	if err := backend.ValidateCopySeekPosition(f); err != nil {
+		return err
+	}
+
 	if ex, _ := f.Exists(); !ex {
 		return doesNotExist()
 	}

backend/os/file.go

@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/c2fo/vfs/v5"
+	"github.com/c2fo/vfs/v5/backend"
 	"github.com/c2fo/vfs/v5/utils"
 )
 
@@ -192,6 +193,10 @@ func (f *File) Location() vfs.Location {
 
 // MoveToFile move a file. It accepts a target vfs.File and returns an error, if any.
 func (f *File) MoveToFile(file vfs.File) error {
+	// validate seek is at 0,0 before doing copy
+	if err := backend.ValidateCopySeekPosition(f); err != nil {
+		return err
+	}
 	// handle native os move/rename
 	if file.Location().FileSystem().Scheme() == Scheme {
 		return safeOsRename(f.Path(), file.Path())
@@ -270,13 +275,21 @@ func (f *File) MoveToLocation(location vfs.Location) (vfs.File, error) {
 
 // CopyToFile copies the file to a new File.  It accepts a vfs.File and returns an error, if any.
 func (f *File) CopyToFile(file vfs.File) error {
+	// validate seek is at 0,0 before doing copy
+	if err := backend.ValidateCopySeekPosition(f); err != nil {
+		return err
+	}
 	_, err := f.copyWithName(file.Name(), file.Location())
 	return err
 }
 
 // CopyToLocation copies existing File to new Location with the same name.
 // It accepts a vfs.Location and returns a vfs.File and error, if any.
 func (f *File) CopyToLocation(location vfs.Location) (vfs.File, error) {
+	// validate seek is at 0,0 before doing copy
+	if err := backend.ValidateCopySeekPosition(f); err != nil {
+		return nil, err
+	}
 	return f.copyWithName(f.Name(), location)
 }
 

backend/s3/file.go

@@ -15,6 +15,7 @@ import (
 	"github.com/aws/aws-sdk-go/service/s3/s3manager"
 
 	"github.com/c2fo/vfs/v5"
+	"github.com/c2fo/vfs/v5/backend"
 	"github.com/c2fo/vfs/v5/mocks"
 	"github.com/c2fo/vfs/v5/utils"
 )
@@ -90,6 +91,11 @@ func (f *File) Location() vfs.Location {
 // CopyToFile puts the contents of File into the targetFile passed. Uses the S3 CopyObject
 // method if the target file is also on S3, otherwise uses io.Copy.
 func (f *File) CopyToFile(file vfs.File) error {
+	// validate seek is at 0,0 before doing copy
+	if err := backend.ValidateCopySeekPosition(f); err != nil {
+		return err
+	}
+
 	// if target is S3
 	if tf, ok := file.(*File); ok {
 		input, err := f.getCopyObjectInput(tf)

backend/s3/file_test.go

@@ -163,6 +163,8 @@ func (ts *fileTestSuite) TestCopyToFile() {
 		key:    "testKey.txt",
 	}
 
+	fooReader := ioutil.NopCloser(strings.NewReader("blah"))
+	s3apiMock.On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).Return(&s3.GetObjectOutput{Body: fooReader}, nil)
 	s3apiMock.On("CopyObject", mock.AnythingOfType("*s3.CopyObjectInput")).Return(&s3.CopyObjectOutput{}, nil)
 
 	err := testFile.CopyToFile(targetFile)
@@ -197,6 +199,8 @@ func (ts *fileTestSuite) TestMoveToFile() {
 		key:    "testKey.txt",
 	}
 
+	fooReader := ioutil.NopCloser(strings.NewReader("blah"))
+	s3apiMock.On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).Return(&s3.GetObjectOutput{Body: fooReader}, nil)
 	s3apiMock.On("CopyObject", mock.AnythingOfType("*s3.CopyObjectInput")).Return(&s3.CopyObjectOutput{}, nil)
 	s3apiMock.On("DeleteObject", mock.AnythingOfType("*s3.DeleteObjectInput")).Return(&s3.DeleteObjectOutput{}, nil)
 
@@ -296,6 +300,8 @@ func (ts *fileTestSuite) TestMoveToFile_CopyError() {
 		key:    "testKey.txt",
 	}
 
+	fooReader := ioutil.NopCloser(strings.NewReader("blah"))
+	s3apiMock.On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).Return(&s3.GetObjectOutput{Body: fooReader}, nil)
 	s3apiMock.On("CopyObject", mock.AnythingOfType("*s3.CopyObjectInput")).Return(nil, errors.New("some copy error"))
 
 	err := testFile.MoveToFile(targetFile)
@@ -306,6 +312,8 @@ func (ts *fileTestSuite) TestMoveToFile_CopyError() {
 
 func (ts *fileTestSuite) TestCopyToLocation() {
 	s3Mock1 := &mocks.S3API{}
+	fooReader := ioutil.NopCloser(strings.NewReader("blah"))
+	s3Mock1.On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).Return(&s3.GetObjectOutput{Body: fooReader}, nil)
 	s3Mock1.On("CopyObject", mock.AnythingOfType("*s3.CopyObjectInput")).Return(nil, nil)
 	s3Mock1.On("HeadObject", mock.AnythingOfType("*s3.HeadObjectInput")).Return(&s3.HeadObjectOutput{}, nil)
 	f := &File{
@@ -341,6 +349,8 @@ func (ts *fileTestSuite) TestTouch() {
 	// Copy portion tested through CopyToLocation, just need to test whether or not Delete happens
 	// in addition to CopyToLocation
 	s3Mock1 := &mocks.S3API{}
+	fooReader := ioutil.NopCloser(strings.NewReader("blah"))
+	s3Mock1.On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).Return(&s3.GetObjectOutput{Body: fooReader}, nil)
 	s3Mock1.On("HeadObject", mock.AnythingOfType("*s3.HeadObjectInput")).Return(&s3.HeadObjectOutput{}, nil)
 	s3Mock1.On("CopyObject", mock.AnythingOfType("*s3.CopyObjectInput")).Return(nil, nil)
 	s3Mock1.On("DeleteObject", mock.AnythingOfType("*s3.DeleteObjectInput")).Return(&s3.DeleteObjectOutput{}, nil)
@@ -383,6 +393,8 @@ func (ts *fileTestSuite) TestMoveToLocation() {
 	// Copy portion tested through CopyToLocation, just need to test whether or not Delete happens
 	// in addition to CopyToLocation
 	s3Mock1 := &mocks.S3API{}
+	fooReader := ioutil.NopCloser(strings.NewReader("blah"))
+	s3Mock1.On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).Return(&s3.GetObjectOutput{Body: fooReader}, nil)
 	s3Mock1.On("CopyObject", mock.AnythingOfType("*s3.CopyObjectInput")).Return(nil, nil)
 	s3Mock1.On("HeadObject", mock.AnythingOfType("*s3.HeadObjectInput")).Return(&s3.HeadObjectOutput{}, nil)
 	f := &File{
@@ -396,6 +408,8 @@ func (ts *fileTestSuite) TestMoveToLocation() {
 	location := new(mocks.Location)
 	location.On("NewFile", mock.Anything).Return(f, nil)
 
+	fooReader2 := ioutil.NopCloser(strings.NewReader("blah"))
+	s3apiMock.On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).Return(&s3.GetObjectOutput{Body: fooReader2}, nil)
 	s3apiMock.On("CopyObject", mock.AnythingOfType("*s3.CopyObjectInput")).Return(&s3.CopyObjectOutput{}, nil)
 	s3apiMock.On("DeleteObject", mock.AnythingOfType("*s3.DeleteObjectInput")).Return(&s3.DeleteObjectOutput{}, nil)
 
@@ -418,6 +432,8 @@ func (ts *fileTestSuite) TestMoveToLocation() {
 		Return(&File{fileSystem: &FileSystem{client: s3Mock1}, bucket: "bucket", key: "/new/hello.txt"}, nil)
 
 	s3apiMock2 := &mocks.S3API{}
+	fooReader3 := ioutil.NopCloser(strings.NewReader("blah"))
+	s3apiMock2.On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).Return(&s3.GetObjectOutput{Body: fooReader3}, nil)
 	s3apiMock2.On("CopyObject", mock.AnythingOfType("*s3.CopyObjectInput")).Return(&s3.CopyObjectOutput{}, nil)
 
 	fs = FileSystem{client: s3apiMock2}
@@ -440,6 +456,8 @@ func (ts *fileTestSuite) TestMoveToLocationFail() {
 	location := new(mocks.Location)
 	location.On("NewFile", mock.Anything).Return(&File{fileSystem: &fs, bucket: "bucket", key: "/new/hello.txt"}, nil)
 
+	fooReader := ioutil.NopCloser(strings.NewReader("blah"))
+	s3apiMock.On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).Return(&s3.GetObjectOutput{Body: fooReader}, nil)
 	s3apiMock.On("CopyObject", mock.AnythingOfType("*s3.CopyObjectInput")).Return(nil, errors.New("didn't copy, oh noes"))
 
 	file, err := fs.NewFile("bucket", "/hello.txt")

backend/sftp/file.go

@@ -7,6 +7,7 @@ import (
 	"time"
 
 	"github.com/c2fo/vfs/v5"
+	"github.com/c2fo/vfs/v5/backend"
 	"github.com/c2fo/vfs/v5/utils"
 )
 
@@ -121,6 +122,10 @@ func (f *File) Location() vfs.Location {
 // If the given location is also sftp AND for the same user and host, the sftp Rename method is used, otherwise
 // we'll do a an io.Copy to the destination file then delete source file.
 func (f *File) MoveToFile(t vfs.File) error {
+	// validate seek is at 0,0 before doing copy
+	if err := backend.ValidateCopySeekPosition(f); err != nil {
+		return err
+	}
 	// sftp rename if vfs is sftp and for the same user/host
 	if f.fileSystem.Scheme() == t.Location().FileSystem().Scheme() &&
 		f.Authority.User == t.(*File).Authority.User &&
@@ -154,20 +159,20 @@ func (f *File) MoveToFile(t vfs.File) error {
 // MoveToLocation works by creating a new file on the target location then calling MoveToFile() on it.
 func (f *File) MoveToLocation(location vfs.Location) (vfs.File, error) {
 
-	newFile, err := location.FileSystem().NewFile(location.Volume(), path.Join(location.Path(), f.Name()))
+	newFile, err := location.NewFile(f.Name())
 	if err != nil {
 		return nil, err
 	}
 
-	err = f.MoveToFile(newFile)
-	if err != nil {
-		return nil, err
-	}
-	return newFile, nil
+	return newFile, f.MoveToFile(newFile)
 }
 
 // CopyToFile puts the contents of File into the targetFile passed.
 func (f *File) CopyToFile(file vfs.File) error {
+	// validate seek is at 0,0 before doing copy
+	if err := backend.ValidateCopySeekPosition(f); err != nil {
+		return err
+	}
 
 	if err := utils.TouchCopy(file, f); err != nil {
 		return err
@@ -184,23 +189,12 @@ func (f *File) CopyToFile(file vfs.File) error {
 // path at the given location.
 func (f *File) CopyToLocation(location vfs.Location) (vfs.File, error) {
 
-	newFile, err := location.FileSystem().NewFile(location.Volume(), path.Join(location.Path(), f.Name()))
+	newFile, err := location.NewFile(f.Name())
 	if err != nil {
 		return nil, err
 	}
 
-	if err := utils.TouchCopy(newFile, f); err != nil {
-		return nil, err
-	}
-	// Close target file to flush and ensure that cursor isn't at the end of the file when the caller reopens for read
-	if cerr := newFile.Close(); cerr != nil {
-		return nil, cerr
-	}
-	// Close file (f) reader
-	if cerr := f.Close(); cerr != nil {
-		return nil, cerr
-	}
-	return newFile, nil
+	return newFile, f.CopyToFile(newFile)
 }
 
 // CRUD Operations