Repository: C2FO/vfs
Pull request #54 (2021-02-01 06:10:48)

Cross dev link os move bug
fixes#48 bug where 'invalid cross-device link' error occurs when renaming os files that are found on different devices/volumes including temp dirs

codecov[bot]: # [Codecov](https://codecov.io/gh/C2FO/vfs/pull/54?src=pr&el=h1) Report
> Merging [#54](https://codecov.io/gh/C2FO/vfs/pull/54?src=pr&el=desc) (d5dc2ba) into [master](https://codecov.io/gh/C2FO/vfs/commit/5a37f1aa8d9111bfae552de195d30f747bf8ae3f?el=desc) (5a37f1a) will **decrease** coverage by `0.46%`.
> The diff coverage is `25.00%`.

[![Impacted file tree graph](https://codecov.io/gh/C2FO/vfs/pull/54/graphs/tree.svg?width=650&height=150&src=pr&token=vCl9BUzOBW)](https://codecov.io/gh/C2FO/vfs/pull/54?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
- Coverage   74.03%   73.57%   -0.47%     
==========================================
  Files          17       17              
  Lines        1533     1540       +7     
==========================================
- Hits         1135     1133       -2     
- Misses        228      240      +12     
+ Partials      170      167       -3     
```


| [Impacted Files](https://codecov.io/gh/C2FO/vfs/pull/54?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [backend/os/file.go](https://codecov.io/gh/C2FO/vfs/pull/54/diff?src=pr&el=tree#diff-YmFja2VuZC9vcy9maWxlLmdv) | `66.66% <25.00%> (-3.86%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/C2FO/vfs/pull/54?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/C2FO/vfs/pull/54?src=pr&el=footer). Last update [5a37f1a...d5dc2ba](https://codecov.io/gh/C2FO/vfs/pull/54?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

thatsmysky: is it possible to add tests so we don't reintroduce this bug in future changes?

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index a7c64d1..73a65a7 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 - fixed failing test due to CI asking for mod tidy.
 - Fixed ineffassign failures keeping vfs from achieving 100% in goreportcard.
+- fixed #48 bug where 'invalid cross-device link' error occurs when renaming os files that are found on different devices/volumes including temp dirs.
 
 ### Security
 - Upgraded dependencies.  Required regen of S3 API mock due to a couple new methods.
@@ -43,7 +44,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [5.4.0] - 2019-08-24
 ### Added
-- Added fallback to TouchCopy for S3/GCS implementations for CopyTo and MoveTo operations between files and locations which use different authentication and region configurations. When possible, the directoy copy/move operations will be used by the underlying clients.
+- Added fallback to TouchCopy for S3/GCS implementations for CopyTo and MoveTo operations between files and locations which use different authentication and region configurations. When possible, the directory copy/move operations will be used by the underlying clients.
 
 ## [5.3.0] - 2019-08-24
 ### Added
diff --git a/backend/os/file.go b/backend/os/file.go
index a32b286..5f9b5ee 100644
--- a/backend/os/file.go
+++ b/backend/os/file.go
@@ -2,6 +2,7 @@ package os
 
 import (
 	"fmt"
+	"io"
 	"io/ioutil"
 	"os"
 	"path"
@@ -12,6 +13,8 @@ import (
 	"github.com/c2fo/vfs/v5/utils"
 )
 
+const osCrossDeviceLinkError = "invalid cross-device link"
+
 type opener func(filePath string) (*os.File, error)
 
 //File implements vfs.File interface for os fs.
@@ -67,23 +70,24 @@ func (f *File) Size() (uint64, error) {
 
 // Close implements the io.Closer interface, closing the underlying *os.File. its an error, if any.
 func (f *File) Close() error {
-	// check if temp file
-	// close temp file
-	// os.Rename() (replace) temp file to file
 	f.useTempFile = false
 	f.cursorPos = 0
+
+	// check if temp file
 	if f.tempFile != nil {
+		// close temp (os) file
 		err := f.tempFile.Close()
 		if err != nil {
 			return err
 		}
 
-		// get original file, open it if it has not been opened
+		// get original (os) file, open it if it has not been opened
 		finalFile, err := f.getInternalFile()
 		if err != nil {
 			return err
 		}
-		err = os.Rename(f.tempFile.Name(), finalFile.Name())
+		// rename temp file to actual file
+		err = safeOsRename(f.tempFile.Name(), finalFile.Name())
 		if err != nil && !os.IsNotExist(err) {
 			return err
 		}
@@ -190,48 +194,74 @@ func (f *File) Location() vfs.Location {
 func (f *File) MoveToFile(file vfs.File) error {
 	// handle native os move/rename
 	if file.Location().FileSystem().Scheme() == Scheme {
-		err := os.Rename(f.Path(), file.Path())
-		if err != nil {
-			return err
-		}
-	} else {
-		// do copy/delete move for non-native os moves
-		_, err := f.copyWithName(file.Name(), file.Location())
-		if err != nil {
-			return err
-		}
+		return safeOsRename(f.Path(), file.Path())
+	}
 
-		err = f.Delete()
-		if err != nil {
-			return err
+	// do copy/delete move for non-native os moves
+	_, err := f.copyWithName(file.Name(), file.Location())
+	return err
+}
+
+// safeOsRename will attempt to do an os.Rename. If error is "invalid cross-device link" (where one OS file is on a
+// different device/volume than the other), then fall back to doing a copy-delete.
+func safeOsRename(srcName, dstName string) error {
+	err := os.Rename(srcName, dstName)
+	if err != nil {
+		e, ok := err.(*os.LinkError)
+		if ok && e.Err.Error() == osCrossDeviceLinkError {
+			// do cross-device renaming
+			if err := osCopy(srcName, dstName); err != nil {
+				return err
+			}
+			// delete original file
+			return os.Remove(srcName)
 		}
+		// return non-CrossDeviceLink error
+		return err
 	}
 	return nil
 }
 
+// osCopy just io.Copy's the os files
+func osCopy(srcName, dstName string) error {
+	// setup os reader
+	srcReader, err := os.Open(srcName)
+	if err != nil {
+		return err
+	}
+	defer func() { _ = srcReader.Close() }()
+
+	// setup os writer
+	dstWriter, err := os.Create(dstName)
+	if err != nil {
+		return err
+	}
+	defer func() { _ = dstWriter.Close() }()
+
+	// copy os files. Note that os.Create always does a "touch" (creates an empty file before writing data) so no need to
+	// do a TouchCopy like we do with other filesystems.
+	_, err = io.Copy(dstWriter, srcReader)
+	return err
+}
+
 // MoveToLocation moves a file to a new Location. It accepts a target vfs.Location and returns a vfs.File and an error, if any.
 func (f *File) MoveToLocation(location vfs.Location) (vfs.File, error) {
-	// handle native os move/rename
 	if location.FileSystem().Scheme() == Scheme {
 		if err := ensureDir(location); err != nil {
 			return nil, err
 		}
-		err := os.Rename(f.Path(), path.Join(location.Path(), f.Name()))
-		if err != nil {
-			return nil, err
-		}
-	} else {
-		// do copy/delete move for non-native os moves
-		_, err := f.copyWithName(f.Name(), location)
-		if err != nil {
-			return f, err
-		}
+	}
 
-		delErr := f.Delete()
-		if delErr != nil {
-			return f, delErr
-		}
+	// do a MoveToFile call (delegating native rename vs copy/delete logic)
+	file, err := location.NewFile(f.Name())
+	if err != nil {
+		return nil, err
 	}
+	err = f.MoveToFile(file)
+	if err != nil {
+		return nil, err
+	}
+
 	//return vfs.File for newly moved file
 	return location.NewFile(f.Name())
 }


Test patch:
diff --git a/backend/os/file_test.go b/backend/os/file_test.go
index c926618..ad7cf84 100644
--- a/backend/os/file_test.go
+++ b/backend/os/file_test.go
@@ -328,17 +328,84 @@ func (s *osFileTest) TestMoveToLocation() {
 	mockLocation.On("Volume").Return("")
 	mockLocation.On("Path").Return("/some/path/to/")
 	mockLocation.On("Close").Return(nil)
-	mockLocation.On("NewFile", mock.Anything).Return(&File{}, nil)
 	mockFile := new(mocks.File)
 	mockFile.On("Location").Return(mockLocation, nil)
 	mockFile.On("Name").Return("/some/path/to/move.txt")
 	mockFile.On("Location ").Return(mockLocation, nil)
+	mockLocation.On("NewFile", mock.Anything).Return(mockFile, nil)
 	mockfs.On("NewLocation", mock.Anything, mock.Anything).Return(mockLocation)
 
 	_, err = movedFile.MoveToLocation(mockLocation)
 	s.NoError(err)
 }
 
+func (s *osFileTest) TestSafeOsRename() {
+	dir, err := ioutil.TempDir(path.Join(s.tmploc.Path(), "test_files"), "example")
+	s.NoError(err)
+	defer func() {
+		err := os.RemoveAll(dir)
+		s.NoError(err, "remove all error not expected")
+	}()
+
+	// TODO: I haven't figured out a way to test safeOsRename since setting up the scenario is
+	//     very os-dependent so here I will actually only ensure non-"invalid cross-device link"
+	//     errors are handled correctly and that normal renames work.
+
+	// test that normal rename works
+	testfile := path.Join(dir, "original.txt")
+	file1, err := s.fileSystem.NewFile("", testfile)
+	s.NoError(err)
+	var testBytes = []byte("test me")
+	_, err = file1.Write(testBytes)
+	s.NoError(err)
+	s.NoError(file1.Close())
+
+	newFile := path.Join(dir, "new.txt")
+	s.NoError(safeOsRename(testfile, newFile))
+
+	exists, err := file1.Exists()
+	s.NoError(err)
+	s.False(exists)
+	file2, err := s.fileSystem.NewFile("", newFile)
+	s.NoError(err)
+	exists, err = file2.Exists()
+	s.NoError(err)
+	s.True(exists)
+
+	// test that a rename failure (non-"invalid cross-device link" error) is returned properly
+	badfile := path.Join(dir, "this_should_not_exist.txt")
+
+	err = safeOsRename(badfile, newFile)
+	s.Error(err)
+	s.NotEqual(err.Error(), osCrossDeviceLinkError)
+
+}
+
+func (s *osFileTest) TestOsCopy() {
+	dir, err := ioutil.TempDir(path.Join(s.tmploc.Path(), "test_files"), "example")
+	s.NoError(err)
+	defer func() {
+		err := os.RemoveAll(dir)
+		s.NoError(err, "remove all error not expected")
+	}()
+
+	file1, err := s.fileSystem.NewFile("", path.Join(dir, "original.txt"))
+	s.NoError(err)
+	var testBytes = []byte("test me")
+	_, err = file1.Write(testBytes)
+	s.NoError(err)
+	s.NoError(file1.Close())
+
+	file2, err := s.fileSystem.NewFile("", path.Join(dir, "move.txt"))
+	s.NoError(err)
+
+	s.NoError(osCopy(path.Join(file1.Location().Volume(), file1.Path()), path.Join(file2.Location().Volume(), file2.Path())), "test osCopy")
+
+	b, err := ioutil.ReadAll(file2)
+	s.NoError(err)
+	s.Equal(testBytes, b, "contents match")
+}
+
 func (s *osFileTest) TestMoveToFile() {
 	dir, terr := ioutil.TempDir(path.Join(s.tmploc.Path(), "test_files"), "example")
 	s.NoError(terr)
@@ -637,7 +704,8 @@ func (s *osFileTest) TestLocationRightAfterChangeDir() {
 	loc := file.Location()
 	s.NotContains(loc.Path(), "someDir/", "location should not contain 'someDir/'")
 
-	loc.ChangeDir(chDir)
+	err = loc.ChangeDir(chDir)
+	s.NoError(err)
 	s.Contains(loc.Path(), "someDir/", "location now should contain 'someDir/'")
 
 	// file location shouldn't be affected by ChangeDir() on Location