Repository: C2FO/vfs
Pull request #41 (2020-04-24 15:18:37)

EP-3173 | Ensure that writing truncates existing file. Fizes #40.
Ensure that os backend truncates existing file when writing. 

https://github.com/C2FO/vfs/issues/40

codecov[bot]: # [Codecov](https://codecov.io/gh/C2FO/vfs/pull/41?src=pr&el=h1) Report
> Merging [#41](https://codecov.io/gh/C2FO/vfs/pull/41?src=pr&el=desc) into [master](https://codecov.io/gh/C2FO/vfs/commit/0a730dc3cd3f2463ae7b4a83495df0a83f04ba38&el=desc) will **decrease** coverage by `0.17%`.
> The diff coverage is `63.49%`.

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

```diff
@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
- Coverage   76.30%   76.12%   -0.18%     
==========================================
  Files          17       17              
  Lines        1730     1776      +46     
==========================================
+ Hits         1320     1352      +32     
- Misses        240      246       +6     
- Partials      170      178       +8     
```


| [Impacted Files](https://codecov.io/gh/C2FO/vfs/pull/41?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [backend/os/fileSystem.go](https://codecov.io/gh/C2FO/vfs/pull/41/diff?src=pr&el=tree#diff-YmFja2VuZC9vcy9maWxlU3lzdGVtLmdv) | `65.21% <ø> (ø)` | |
| [backend/os/file.go](https://codecov.io/gh/C2FO/vfs/pull/41/diff?src=pr&el=tree#diff-YmFja2VuZC9vcy9maWxlLmdv) | `66.66% <63.49%> (+0.97%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/C2FO/vfs/pull/41?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/41?src=pr&el=footer). Last update [0a730dc...032e6fd](https://codecov.io/gh/C2FO/vfs/pull/41?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 2d04ca8..bd7b4e5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,7 +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]
+## [5.5.2] - 2020-04-23
+### Fixed
+- Ensure that writing truncates existing file. Fixes #40
 
 ## [5.5.1] - 2020-02-20
 ### Fixed
diff --git a/backend/os/file.go b/backend/os/file.go
index c0b5b76..09dc140 100644
--- a/backend/os/file.go
+++ b/backend/os/file.go
@@ -1,8 +1,8 @@
 package os
 
 import (
-	"errors"
 	"fmt"
+	"io/ioutil"
 	"os"
 	"path"
 	"path/filepath"
@@ -12,11 +12,17 @@ import (
 	"github.com/c2fo/vfs/v5/utils"
 )
 
+type opener func(filePath string) (*os.File, error)
+
 //File implements vfs.File interface for os fs.
 type File struct {
-	file     *os.File
-	name     string
-	location vfs.Location
+	file        *os.File
+	name        string
+	location    vfs.Location
+	cursorPos   int64
+	tempFile    *os.File
+	useTempFile bool
+	fileOpener  opener
 }
 
 // Delete unlinks the file returning any error or nil.
@@ -61,6 +67,28 @@ 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
+	if f.tempFile != nil {
+		err := f.tempFile.Close()
+		if err != nil {
+			return err
+		}
+
+		// get original 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())
+		if err != nil && !os.IsNotExist(err) {
+			return err
+		}
+		f.tempFile = nil
+	}
 	if f.file == nil {
 		// Do nothing on files that were never referenced
 		return nil
@@ -75,37 +103,46 @@ func (f *File) Close() error {
 
 // Read implements the io.Reader interface.  It returns the bytes read and an error, if any.
 func (f *File) Read(p []byte) (int, error) {
-	if exists, err := f.Exists(); err != nil {
+
+	// if we have not written to this file, ensure the original file exists
+	if !f.useTempFile {
+		if exists, err := f.Exists(); err != nil {
+			return 0, err
+		} else if !exists {
+			return 0, fmt.Errorf("failed to read. File does not exist at %s", f)
+		}
+	}
+	// get the file we need, either tempFile or original file
+	useFile, err := f.getInternalFile()
+	if err != nil {
 		return 0, err
-	} else if !exists {
-		return 0, fmt.Errorf("failed to read. File does not exist at %s", f)
 	}
 
-	file, err := f.openFile()
+	read, err := useFile.Read(p)
 	if err != nil {
-		return 0, err
+		return read, err
 	}
 
-	return file.Read(p)
+	f.cursorPos += int64(read)
+
+	return read, nil
 }
 
-//Seek implements the io.Seeker interface.  It accepts an offset and "whench" where 0 means relative to the origin of
+//Seek implements the io.Seeker interface.  It accepts an offset and "whence" where 0 means relative to the origin of
 // the file, 1 means relative to the current offset, and 2 means relative to the end.  It returns the new offset and
 // an error, if any.
 func (f *File) Seek(offset int64, whence int) (int64, error) {
-
-	if exists, err := f.Exists(); !exists {
-		if err != nil {
-			return 0, err
-		}
-		return 0, errors.New("file does not exist")
+	useFile, err := f.getInternalFile()
+	if err != nil {
+		return 0, err
 	}
-	file, err := f.openFile()
+
+	f.cursorPos, err = useFile.Seek(offset, whence)
 	if err != nil {
 		return 0, err
 	}
 
-	return file.Seek(offset, whence)
+	return f.cursorPos, err
 }
 
 // Exists true if the file exists on the file system, otherwise false, and an error, if any.
@@ -125,11 +162,20 @@ func (f *File) Exists() (bool, error) {
 
 //Write implements the io.Writer interface.  It accepts a slice of bytes and returns the number of bytes written and an error, if any.
 func (f *File) Write(p []byte) (n int, err error) {
-	file, err := f.openFile()
+	f.useTempFile = true
+
+	useFile, err := f.getInternalFile()
 	if err != nil {
 		return 0, err
 	}
-	return file.Write(p)
+	write, err := useFile.Write(p)
+	if err != nil {
+		return 0, err
+	}
+	offset := int64(write)
+	f.cursorPos += offset
+
+	return write, err
 }
 
 // Location returns the underlying os.Location.
@@ -254,14 +300,30 @@ func (f *File) openFile() (*os.File, error) {
 		return f.file, nil
 	}
 
+	// replace default file opener, is set in struct
+	openFunc := openOSFile
+	if f.fileOpener != nil {
+		openFunc = f.fileOpener
+	}
+
+	file, err := openFunc(f.Path())
+	if err != nil {
+		return nil, err
+	}
+	f.file = file
+
+	return file, nil
+}
+
+func openOSFile(filePath string) (*os.File, error) {
+
 	// Ensure the path exists before opening the file, NoOp if dir already exists.
 	var fileMode os.FileMode = 0666
-	if err := os.MkdirAll(f.Location().Path(), os.ModeDir|0777); err != nil {
+	if err := os.MkdirAll(path.Dir(filePath), os.ModeDir|0777); err != nil {
 		return nil, err
 	}
 
-	file, err := os.OpenFile(f.Path(), os.O_RDWR|os.O_CREATE, fileMode)
-	f.file = file
+	file, err := os.OpenFile(filePath, os.O_RDWR|os.O_CREATE, fileMode)
 	return file, err
 }
 
@@ -275,3 +337,65 @@ func ensureDir(location vfs.Location) error {
 	}
 	return nil
 }
+
+// If cursor is not (0,0) will copy original file to a temp file,
+//opening its file descriptor to the current cursor position.
+//If cursor is (0,0), just begin writing to new temp file.
+//No need to copy original first.
+func (f *File) getInternalFile() (*os.File, error) {
+	// this is the use case of vfs.file
+	if f.useTempFile == false {
+		if f.file == nil {
+
+			// replace default file opener, is set in struct
+			openFunc := openOSFile
+			if f.fileOpener != nil {
+				openFunc = f.fileOpener
+			}
+
+			finalFile, err := openFunc(f.Path())
+			if err != nil {
+				return nil, err
+			}
+			f.file = finalFile
+		}
+		return f.file, nil
+	}
+	// this is the use case of vfs.tempFile
+	if f.tempFile == nil {
+		localTempFile, err := f.copyToLocalTempReader()
+		if err != nil {
+			return nil, err
+		}
+		f.tempFile = localTempFile
+	}
+
+	return f.tempFile, nil
+}
+
+func (f *File) copyToLocalTempReader() (*os.File, error) {
+	tmpFile, err := ioutil.TempFile("", fmt.Sprintf("%s.%d", f.Name(), time.Now().UnixNano()))
+	if err != nil {
+		return nil, err
+	}
+
+	openFunc := openOSFile
+	if f.fileOpener != nil {
+		openFunc = f.fileOpener
+	}
+
+	if _, err = openFunc(f.Path()); err != nil {
+		return nil, err
+	}
+	// todo: editing in place logic/appending logic (see issue #42)
+	//if _, err := io.Copy(tmpFile, f.file); err != nil {
+	//	return nil, err
+	//}
+	//
+	//// Return cursor to the beginning of the new temp file
+	//if _, err := tmpFile.Seek(f.cursorPos, 0); err != nil {
+	//	return nil, err
+	//}
+
+	return tmpFile, nil
+}
diff --git a/backend/os/fileSystem.go b/backend/os/fileSystem.go
index 905fac8..f1a2142 100644
--- a/backend/os/fileSystem.go
+++ b/backend/os/fileSystem.go
@@ -15,7 +15,7 @@ const name = "os"
 // FileSystem implements vfs.Filesystem for the OS file system.
 type FileSystem struct{}
 
-// Retry will return a retrier provided via options, or a no-op if none is provided.
+// Retry will return a retriever provided via options, or a no-op if none is provided.
 func (fs *FileSystem) Retry() vfs.Retry {
 	return vfs.DefaultRetryer()
 }


Test patch:
diff --git a/backend/os/file_test.go b/backend/os/file_test.go
index 558af78..a6d1754 100644
--- a/backend/os/file_test.go
+++ b/backend/os/file_test.go
@@ -1,6 +1,7 @@
 package os
 
 import (
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"os"
@@ -106,6 +107,7 @@ func (s *osFileTest) TestTouch() {
 	nextModTime, err := testfile.LastModified()
 	s.NoError(err)
 	s.True(firstModTime.Before(*nextModTime), "Last Modified was updated")
+	s.NoError(testfile.Close())
 }
 
 func (s *osFileTest) TestOpenFile() {
@@ -130,15 +132,35 @@ func (s *osFileTest) TestRead() {
 	s.NoError(err)
 	b, err := f.Write([]byte("blah"))
 	s.NoError(err)
-	s.Equal(int(4), b)
+	s.Equal(4, b)
 	s.NoError(f.Close())
 
 	//read from file
 	data = make([]byte, 4)
 	b, err = f.Read(data)
 	s.NoError(err)
-	s.Equal(int(4), b)
+	s.Equal(4, b)
 	s.Equal("blah", string(data))
+	s.NoError(f.Close())
+
+	//setup file for err out of opening
+	f, err = s.tmploc.NewFile("test_files/readFileFail.txt")
+	s.NoError(err)
+	f.(*File).useTempFile = true
+	f.(*File).fileOpener = func(filePath string) (*os.File, error) { return nil, errors.New("Bad Opener") }
+	data = make([]byte, 4)
+	b, err = f.Read(data)
+	s.Error(err)
+
+	f.(*File).fileOpener = nil
+	b, err = f.Write([]byte("blah"))
+	s.NoError(err)
+	s.Equal(4, b)
+	s.NoError(f.Close())
+	f.(*File).fileOpener = func(filePath string) (*os.File, error) { return nil, errors.New("Bad Opener") }
+	data = make([]byte, 4)
+	b, err = f.Read(data)
+	s.Error(err)
 }
 
 func (s *osFileTest) TestSeek() {
@@ -149,6 +171,15 @@ func (s *osFileTest) TestSeek() {
 	_, rerr := s.testFile.Read(data)
 	s.NoError(rerr, "read error not expected")
 	s.Equal(expectedText, string(data))
+	s.NoError(s.testFile.Close())
+
+	//setup file for err out of opening
+	f, err := s.tmploc.NewFile("test_files/seekFileFail.txt")
+	s.NoError(err)
+	f.(*File).useTempFile = true
+	f.(*File).fileOpener = func(filePath string) (*os.File, error) { return nil, errors.New("Bad Opener") }
+	_, err = f.Seek(0, 4)
+	s.Error(err)
 }
 
 func (s *osFileTest) TestCopyToLocation() {
@@ -350,7 +381,6 @@ func (s *osFileTest) TestMoveToFile() {
 	s.NoError(rerr, "read error not expected")
 	cErr := file2.Close()
 	s.NoError(cErr, "close error not expected")
-
 	s.Equal(text, string(data))
 
 	// test non-scheme MoveToFile
@@ -404,6 +434,133 @@ func (s *osFileTest) TestWrite() {
 	found2, eErr2 := file.Exists()
 	s.NoError(eErr2, "exists error not expected")
 	s.False(found2)
+
+	//setup file for err out of opening
+	f, err := s.tmploc.NewFile("test_files/writeFileFail.txt")
+	s.NoError(err)
+	f.(*File).useTempFile = true
+	f.(*File).fileOpener = func(filePath string) (*os.File, error) { return nil, errors.New("Bad Opener") }
+	data = make([]byte, 4)
+	_, err = f.Write(data)
+	s.Error(err)
+}
+
+func (s *osFileTest) TestCursor() {
+	file, err := s.tmploc.NewFile("test_files/originalFile.txt")
+	s.NoError(err)
+
+	expectedText := "mary had \na little lamb\n"
+	write, werr := file.Write([]byte(expectedText))
+	s.NoError(werr, "write error not expected")
+	s.Equal(24, write)
+	s.NoError(file.Close())
+
+	_, serr := file.Seek(5, 0) // cursor 5 - opens fd to orig file
+	s.Equal(int64(5), file.(*File).cursorPos)
+	s.NoError(serr)
+
+	data := make([]byte, 3)
+	sz, rerr := file.Read(data) // cursor 8 - orig file - data: "had"
+	s.NoError(rerr)
+	s.Equal(int64(8), file.(*File).cursorPos)
+	s.Equal("had", string(data)) // orig file contents = "had"
+
+	negsz := int64(-sz)
+	_, serr2 := file.Seek(negsz, 1) // cursor 5 - orig file
+	s.Equal(int64(5), file.(*File).cursorPos)
+	s.NoError(serr2)
+
+	sz, werr = file.Write([]byte("has")) // cursor 8 - tempfile copy of orig - write on tempfile has occurred
+	s.NoError(werr)
+	s.Equal(int64(8), file.(*File).cursorPos)
+
+	_, serr = file.Seek(0, 0) // cursor 0 - in temp file
+	s.Equal(int64(0), file.(*File).cursorPos)
+	s.NoError(serr)
+
+	data = make([]byte, 3)
+	sz, rerr = file.Read(data)
+	s.NoError(rerr)
+	s.Equal(int64(3), file.(*File).cursorPos)
+	s.Equal("has", string(data)) // tempFile contents = "has"
+
+	s.NoError(file.Close()) // moves tempfile containing "has" over original file
+
+	final := make([]byte, 3)
+	rd, err := file.Read(final)
+	s.NoError(err)
+	s.Equal(3, rd)
+	s.Equal("has", string(final))
+	s.NoError(file.Close())
+
+	// if a file exists and we overwrite with a smaller # of text, then it isn't completely overwritten
+	//	//somefile.txt contains "the quick brown"
+	file, err = s.tmploc.NewFile("test_files/someFile.txt")
+	s.NoError(err)
+
+	expectedText = "the quick brown"
+	write, werr = file.Write([]byte(expectedText))
+	s.NoError(werr, "write error not expected")
+	s.Equal(15, write)
+
+	s.NoError(file.Close())
+
+	overwrite, err := file.Write([]byte("hello")) // cursor 5 of tempfile
+	s.NoError(err)
+	s.Equal(int64(5), file.(*File).cursorPos)
+	s.Equal(5, overwrite)
+
+	data = make([]byte, 5)
+	_, serr = file.Seek(0, 0) // cursor 0 of tempfile
+	s.NoError(serr)
+
+	_, rerr = file.Read(data) // cursor 5 of tempfile - data: "hello"
+	s.NoError(rerr)
+	s.Equal("hello", string(data))
+
+	data = make([]byte, 3)
+	sought, serr := file.Seek(-3, 2) // cursor 3 from end of tempfile
+	s.NoError(serr)
+	s.Equal(int64(2), sought) // seek returns position relative to beginning of file
+
+	rd, rerr = file.Read(data) // cursor 0 from end of tempfile - data: "llo"
+	s.NoError(rerr)
+	s.Equal(3, rd)
+	s.Equal("llo", string(data))
+	s.Equal(int64(5), file.(*File).cursorPos)
+
+	_, serr = file.Seek(0, 0)
+	s.NoError(serr)
+	final = make([]byte, 5)
+	rd, err = file.Read(final)
+	s.NoError(err)
+	s.Equal(5, rd)
+	s.Equal("hello", string(final))
+	s.NoError(file.Close()) // moves tempfile containing "hello" over somefile.txt
+}
+
+func (s *osFileTest) TestCursorErrs() {
+	noFile, err := s.tmploc.NewFile("test_files/nonet.txt")
+	s.NoError(err)
+	data := make([]byte, 10)
+	_, err = noFile.Read(data)
+	s.Error(err)
+	s.NoError(noFile.Close())
+
+	_, err = noFile.Seek(10, 10)
+	s.Error(err)
+
+	_, err = noFile.Seek(-10, 2)
+	s.Error(err)
+	s.NoError(noFile.Close())
+
+	noFile.(*File).fileOpener = nil
+	noFile.(*File).useTempFile = false
+	b, err := noFile.Write([]byte("blah"))
+	s.NoError(err)
+	s.Equal(4, b)
+	noFile.(*File).fileOpener = func(filePath string) (*os.File, error) { return nil, errors.New("Bad Opener") }
+	s.Error(noFile.Close())
 }
 
 func (s *osFileTest) TestLastModified() {
diff --git a/backend/testsuite/backend_integration_test.go b/backend/testsuite/backend_integration_test.go
index 1107048..b58cc24 100644
--- a/backend/testsuite/backend_integration_test.go
+++ b/backend/testsuite/backend_integration_test.go
@@ -848,7 +848,7 @@ func (s *vfsTestSuite) File(baseLoc vfs.Location) {
 	_, err = srcFile.LastModified()
 	s.Error(err, "expected error because file does not exist")
 
-	seeked, err := srcFile.Seek(1, 2)
+	seeked, err := srcFile.Seek(-1, 2)
 	s.Error(err, "expected error because file does not exist")
 	s.Equal(int64(0x0), seeked)