Fixes #156 - Update os backend to use new io integration test suite (#…

…157)

* Fixes #156 - Update os backend to use new io integration test suite

* add comment for Seek

* updated changelog

* add comment to copyToLocalTempReader

* update tests that werent closing files after write

CHANGELOG.md

@@ -4,14 +4,16 @@ 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]
+### Added
+- Fixes #156 - Update os backend to comply with new io integration test suite
 
 ## [6.11.3] - 2024-02-02
 ### Fixed
 - Fixed #158 bug. Updated sftp backend to to fix issue where some servers return a generic error message when a file is opened for RW on Read().
 
 ## [6.11.2] - 2024-01-30
 ### Fixed
-- Fixed 154 bug. Updated sftp backend to overwrite except after read/seek.
+- Fixed #154 bug. Updated sftp backend to overwrite except after read/seek.
 
 ## [6.11.1] - 2024-01-22
 ### Fixed

backend/mem/file_test.go

@@ -369,6 +369,7 @@ func (s *memFileTest) TestCopyToLocationOS() {
 	s.NoError(err, "unexpected error creating osFile")
 	_, err = osFile.Write(make([]byte, 0))
 	s.NoError(err, "unexpected error writing zero bytes to osFile")
+	s.NoError(osFile.Close())
 
 	exists, err := osFile.Exists()
 	s.NoError(err, "unexpected existence error")

backend/os/file.go

@@ -3,6 +3,7 @@ package os
 import (
 	"fmt"
 	"io"
+	"io/fs"
 	"os"
 	"path"
 	"path/filepath"
@@ -27,6 +28,8 @@ type File struct {
 	tempFile    *os.File
 	useTempFile bool
 	fileOpener  opener
+	seekCalled  bool
+	readCalled  bool
 }
 
 // Delete unlinks the file returning any error or nil.
@@ -49,12 +52,19 @@ func (f *File) LastModified() (*time.Time, error) {
 	return &statsTime, err
 }
 
-// Name returns the full name of the File relative to Location.Name().
+// Name returns the base name of the file path.
+//
+// For `file:///some/path/to/file.txt`, it would return `file.txt`
 func (f *File) Name() string {
 	return path.Base(f.name)
 }
 
-// Path returns the the path of the File relative to Location.Name().
+// Path returns absolute path, including filename,
+// For `file:///some/path/to/file.txt`, it would return `/some/path/to/file.txt`
+//
+// If the directory portion of a file is desired, call
+//
+//	someFile.Location().Path()
 func (f *File) Path() string {
 	return filepath.Join(f.Location().Path(), f.Name())
 }
@@ -73,6 +83,8 @@ func (f *File) Size() (uint64, error) {
 func (f *File) Close() error {
 	f.useTempFile = false
 	f.cursorPos = 0
+	f.seekCalled = false
+	f.readCalled = false
 
 	// check if temp file
 	if f.tempFile != nil {
@@ -128,6 +140,7 @@ func (f *File) Read(p []byte) (int, error) {
 		return read, err
 	}
 
+	f.readCalled = true
 	f.cursorPos += int64(read)
 
 	return read, nil
@@ -137,6 +150,15 @@ func (f *File) Read(p []byte) (int, error) {
 // 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) {
+	// when writing, we first write to a temp file which ensures a file isn't created before we call close.
+	// However, if we've never written AND the original file doesn't exist, we can't seek.
+	exists, err := f.Exists()
+	if err != nil {
+		return 0, fmt.Errorf("unable to Seek: %w", err)
+	}
+	if !exists && !f.useTempFile {
+		return 0, fmt.Errorf("unable to Seek: %w", fs.ErrNotExist)
+	}
 	useFile, err := f.getInternalFile()
 	if err != nil {
 		return 0, err
@@ -147,6 +169,7 @@ func (f *File) Seek(offset int64, whence int) (int64, error) {
 		return 0, err
 	}
 
+	f.seekCalled = true
 	return f.cursorPos, err
 }
 
@@ -167,6 +190,7 @@ 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) {
+	// useTempFile prevents the immediate update of the file until we Close()
 	f.useTempFile = true
 
 	useFile, err := f.getInternalFile()
@@ -369,7 +393,7 @@ 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(path.Dir(filePath), os.ModeDir|0777); err != nil {
+	if err := os.MkdirAll(path.Dir(filePath), os.ModeDir|0750); err != nil {
 		return nil, err
 	}
 
@@ -428,23 +452,39 @@ func (f *File) copyToLocalTempReader() (*os.File, error) {
 		return nil, err
 	}
 
-	openFunc := openOSFile
-	if f.fileOpener != nil {
-		openFunc = f.fileOpener
-	}
-
-	if _, err = openFunc(f.Path()); err != nil {
+	exists, err := f.Exists()
+	if 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
-	// }
+
+	// If file exists AND we've called Seek or Read first, any subsequent writes should edit the file (temp),
+	// so we copy the original file to the temp file then set the cursor position on the temp file to the current position.
+	// If we're opening because Write is called first, we always overwrite the file, so no need to copy the original contents.
 	//
-	// // Return cursor to the beginning of the new temp file
-	// if _, err := tmpFile.Seek(f.cursorPos, 0); err != nil {
-	//	return nil, err
-	// }
+	// So imagine we have a file with content "hello world" and we call Seek(6, 0) and then Write([]byte("there")), the
+	// temp file should have "hello there" and not "there".  Then finally when Close is called, the temp file is renamed
+	// to the original file.  This code ensures that scenario works as expected.
+	if exists && (f.seekCalled || f.readCalled) {
+		openFunc := openOSFile
+		if f.fileOpener != nil {
+			openFunc = f.fileOpener
+		}
+
+		actualFile, err := openFunc(f.Path())
+		if err != nil {
+			return nil, err
+		}
+		if _, err := io.Copy(tmpFile, actualFile); err != nil {
+			return nil, err
+		}
+
+		if f.cursorPos > 0 {
+			// match cursor position in tmep file
+			if _, err := tmpFile.Seek(f.cursorPos, 0); err != nil {
+				return nil, err
+			}
+		}
+	}
 
 	return tmpFile, nil
 }

backend/os/file_test.go

@@ -506,7 +506,9 @@ func (s *osFileTest) TestWrite() {
 	// setup file for err out of opening
 	f, err := s.tmploc.NewFile("test_files/writeFileFail.txt")
 	s.NoError(err)
-	f.(*File).useTempFile = true
+	s.NoError(f.Touch())
+	_, err = f.Seek(0, 0)
+	s.NoError(err)
 	f.(*File).fileOpener = func(filePath string) (*os.File, error) { return nil, errors.New("bad opener") }
 	data = make([]byte, 4)
 	_, err = f.Write(data)
@@ -538,29 +540,30 @@ func (s *osFileTest) TestCursor() {
 	s.Equal(int64(5), file.(*File).cursorPos)
 	s.NoError(serr2)
 
+	// because seek and/or read were called before write, write is now in in-place edit mode (not truncate-write)
 	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)
 	s.Equal(3, sz)
 
-	_, serr = file.Seek(0, 0) // cursor 0 - in temp file
-	s.Equal(int64(0), file.(*File).cursorPos)
+	_, serr = file.Seek(5, 0) // cursor 5 - in temp file
+	s.Equal(int64(5), 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(int64(8), file.(*File).cursorPos)
 	s.Equal("has", string(data)) // tempFile contents = "has"
 	s.Equal(3, sz)
 
 	s.NoError(file.Close()) // moves tempfile containing "has" over original file
 
-	final := make([]byte, 3)
+	final := make([]byte, 8)
 	rd, err := file.Read(final)
 	s.NoError(err)
-	s.Equal(3, rd)
-	s.Equal("has", string(final))
+	s.Equal(8, rd)
+	s.Equal("mary 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

backend/os/location_test.go

@@ -158,6 +158,8 @@ func (s *osLocationTest) TestDeleteFile() {
 	_, err = file.Write([]byte(expectedText))
 	s.NoError(err, "Shouldn't fail to write text to file.")
 
+	s.NoError(file.Close())
+
 	exists, err := file.Exists()
 	s.NoError(err, "Exists shouldn't throw error.")
 	s.True(exists, "Exists should return true for test file.")

utils/authority_test.go

@@ -258,6 +258,6 @@ func (a *authoritySuite) TestAuthority() {
 	}
 }
 
-func TestUtils(t *testing.T) {
+func TestAuthority(t *testing.T) {
 	suite.Run(t, new(authoritySuite))
 }