diff --git a/CHANGELOG.md b/CHANGELOG.md index 28c53224..4ac416f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ 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 @@ -11,7 +13,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [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 diff --git a/backend/mem/file_test.go b/backend/mem/file_test.go index b36b82a7..9426cc40 100644 --- a/backend/mem/file_test.go +++ b/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") diff --git a/backend/os/file.go b/backend/os/file.go index 6e869180..41fc81d9 100644 --- a/backend/os/file.go +++ b/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 } diff --git a/backend/os/file_test.go b/backend/os/file_test.go index 1937f87d..bd36e727 100644 --- a/backend/os/file_test.go +++ b/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 diff --git a/backend/os/location_test.go b/backend/os/location_test.go index 29ce1f4b..21bba629 100644 --- a/backend/os/location_test.go +++ b/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.") diff --git a/utils/authority_test.go b/utils/authority_test.go index ebc26dc5..513f7288 100644 --- a/utils/authority_test.go +++ b/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)) } diff --git a/utils/utils_test.go b/utils/utils_test.go index 8e3270d1..8eb0f7ee 100644 --- a/utils/utils_test.go +++ b/utils/utils_test.go @@ -19,7 +19,7 @@ import ( ************TESTS***************** **********************************/ -type utilsTest struct { +type utilsSuite struct { suite.Suite } @@ -29,7 +29,7 @@ type slashTest struct { message string } -func (s *utilsTest) TestEnsureTrailingSlash() { +func (s *utilsSuite) TestEnsureTrailingSlash() { tests := []slashTest{ { path: "/some/path", @@ -59,11 +59,13 @@ func (s *utilsTest) TestEnsureTrailingSlash() { } for _, slashtest := range tests { - s.Equal(slashtest.expected, utils.EnsureTrailingSlash(slashtest.path), slashtest.message) + s.Run(slashtest.message, func() { + s.Equal(slashtest.expected, utils.EnsureTrailingSlash(slashtest.path), slashtest.message) + }) } } -func (s *utilsTest) TestEnsureLeadingSlash() { +func (s *utilsSuite) TestEnsureLeadingSlash() { tests := []slashTest{ { path: "some/path/", @@ -88,11 +90,13 @@ func (s *utilsTest) TestEnsureLeadingSlash() { } for _, slashtest := range tests { - s.Equal(slashtest.expected, utils.EnsureLeadingSlash(slashtest.path), slashtest.message) + s.Run(slashtest.message, func() { + s.Equal(slashtest.expected, utils.EnsureLeadingSlash(slashtest.path), slashtest.message) + }) } } -func (s *utilsTest) TestRemoveTrailingSlash() { +func (s *utilsSuite) TestRemoveTrailingSlash() { tests := []slashTest{ { path: "/some/path", @@ -122,11 +126,13 @@ func (s *utilsTest) TestRemoveTrailingSlash() { } for _, slashtest := range tests { - s.Equal(slashtest.expected, utils.RemoveTrailingSlash(slashtest.path), slashtest.message) + s.Run(slashtest.message, func() { + s.Equal(slashtest.expected, utils.RemoveTrailingSlash(slashtest.path), slashtest.message) + }) } } -func (s *utilsTest) TestRemoveLeadingSlash() { +func (s *utilsSuite) TestRemoveLeadingSlash() { tests := []slashTest{ { path: "some/path/", @@ -156,7 +162,9 @@ func (s *utilsTest) TestRemoveLeadingSlash() { } for _, slashtest := range tests { - s.Equal(slashtest.expected, utils.RemoveLeadingSlash(slashtest.path), slashtest.message) + s.Run(slashtest.message, func() { + s.Equal(slashtest.expected, utils.RemoveLeadingSlash(slashtest.path), slashtest.message) + }) } } @@ -166,7 +174,7 @@ type pathValidationTest struct { message string } -func (s *utilsTest) TestValidateAbsFilePath() { +func (s *utilsSuite) TestValidateAbsFilePath() { tests := []pathValidationTest{ { path: "/some/path/", @@ -221,16 +229,18 @@ func (s *utilsTest) TestValidateAbsFilePath() { } for _, validationTest := range tests { - err := utils.ValidateAbsoluteFilePath(validationTest.path) - if !validationTest.passExpected { - s.EqualError(err, utils.ErrBadAbsFilePath, validationTest.message) - } else { - s.NoError(err, validationTest.message) - } + s.Run(validationTest.message, func() { + err := utils.ValidateAbsoluteFilePath(validationTest.path) + if !validationTest.passExpected { + s.EqualError(err, utils.ErrBadAbsFilePath, validationTest.message) + } else { + s.NoError(err, validationTest.message) + } + }) } } -func (s *utilsTest) TestValidateAbsLocationPath() { +func (s *utilsSuite) TestValidateAbsLocationPath() { tests := []pathValidationTest{ { path: "/some/path/", @@ -285,16 +295,18 @@ func (s *utilsTest) TestValidateAbsLocationPath() { } for _, validationTest := range tests { - err := utils.ValidateAbsoluteLocationPath(validationTest.path) - if !validationTest.passExpected { - s.EqualError(err, utils.ErrBadAbsLocationPath, validationTest.message) - } else { - s.NoError(err, validationTest.message) - } + s.Run(validationTest.message, func() { + err := utils.ValidateAbsoluteLocationPath(validationTest.path) + if !validationTest.passExpected { + s.EqualError(err, utils.ErrBadAbsLocationPath, validationTest.message) + } else { + s.NoError(err, validationTest.message) + } + }) } } -func (s *utilsTest) TestValidateRelFilePath() { +func (s *utilsSuite) TestValidateRelFilePath() { tests := []pathValidationTest{ { path: "/some/path/", @@ -349,16 +361,18 @@ func (s *utilsTest) TestValidateRelFilePath() { } for _, validationTest := range tests { - err := utils.ValidateRelativeFilePath(validationTest.path) - if !validationTest.passExpected { - s.EqualError(err, utils.ErrBadRelFilePath, validationTest.message) - } else { - s.NoError(err, validationTest.message) - } + s.Run(validationTest.message, func() { + err := utils.ValidateRelativeFilePath(validationTest.path) + if !validationTest.passExpected { + s.EqualError(err, utils.ErrBadRelFilePath, validationTest.message) + } else { + s.NoError(err, validationTest.message) + } + }) } } -func (s *utilsTest) TestValidateRelLocationPath() { +func (s *utilsSuite) TestValidateRelLocationPath() { tests := []pathValidationTest{ { path: "/some/path/", @@ -413,16 +427,18 @@ func (s *utilsTest) TestValidateRelLocationPath() { } for _, validationTest := range tests { - err := utils.ValidateRelativeLocationPath(validationTest.path) - if !validationTest.passExpected { - s.EqualError(err, utils.ErrBadRelLocationPath, validationTest.message) - } else { - s.NoError(err, validationTest.message) - } + s.Run(validationTest.message, func() { + err := utils.ValidateRelativeLocationPath(validationTest.path) + if !validationTest.passExpected { + s.EqualError(err, utils.ErrBadRelLocationPath, validationTest.message) + } else { + s.NoError(err, validationTest.message) + } + }) } } -func (s *utilsTest) TestValidatePrefix() { +func (s *utilsSuite) TestValidatePrefix() { tests := []struct { prefix string passExpected bool @@ -501,12 +517,14 @@ func (s *utilsTest) TestValidatePrefix() { } for _, validationTest := range tests { - err := utils.ValidatePrefix(validationTest.prefix) - if !validationTest.passExpected { - s.EqualError(err, utils.ErrBadPrefix, validationTest.message) - } else { - s.NoError(err, validationTest.message) - } + s.Run(validationTest.message, func() { + err := utils.ValidatePrefix(validationTest.prefix) + if !validationTest.passExpected { + s.EqualError(err, utils.ErrBadPrefix, validationTest.message) + } else { + s.NoError(err, validationTest.message) + } + }) } } @@ -517,7 +535,7 @@ type URITest struct { isRegex bool } -func (s *utilsTest) TestPathToURI() { +func (s *utilsSuite) TestPathToURI() { tests := []URITest{ { path: "/absolute/path/", @@ -575,24 +593,27 @@ func (s *utilsTest) TestPathToURI() { } for _, slashtest := range tests { - uri, err := utils.PathToURI(slashtest.path) - s.Require().NoError(err, "no error expected") - if slashtest.isRegex { - s.Regexp(slashtest.expected, uri, slashtest.message) - } else { - s.Equal(slashtest.expected, uri, slashtest.message) - } + s.Run(slashtest.message, func() { + uri, err := utils.PathToURI(slashtest.path) + s.Require().NoError(err, "no error expected") + if slashtest.isRegex { + s.Regexp(slashtest.expected, uri, slashtest.message) + } else { + s.Equal(slashtest.expected, uri, slashtest.message) + } + }) } // test error return from bad uri.parse const nullChar = '\u0000' + // parse path with null character _, err := utils.PathToURI(fmt.Sprintf("/some%s/path/", string(nullChar))) s.Error(err, "expected error on ctrl char in path") s.EqualError(err, "parse \"/some\\x00/path/\": net/url: invalid control character in URL") } -func (s *utilsTest) TestGetURI() { +func (s *utilsSuite) TestGetURI() { // set up mocks mockFs1 := new(mocks.FileSystem) @@ -628,7 +649,7 @@ func (s *utilsTest) TestGetURI() { s.Equal("s3://mybucket/this/path/to/", utils.GetLocationURI(mockLoc2), "s3 location uri matches ") } -func (s *utilsTest) TestTouchCopy() { +func (s *utilsSuite) TestTouchCopy() { // write out blank file tmpfile, err := os.CreateTemp("", "utils_test") @@ -679,6 +700,7 @@ func (s *utilsTest) TestTouchCopy() { panic(err) } }() + s.NoError(writer.Close()) // writer file should exist fi, err := os.Stat(writer.Path()) @@ -708,7 +730,7 @@ func (s *utilsTest) TestTouchCopy() { } -func (s *utilsTest) TestTouchCopyBufferedDefaultBufferSize() { +func (s *utilsSuite) TestTouchCopyBufferedDefaultBufferSize() { // write out blank file tmpfile, err := os.CreateTemp("", "utils_test") @@ -759,6 +781,7 @@ func (s *utilsTest) TestTouchCopyBufferedDefaultBufferSize() { panic(err) } }() + s.NoError(writer.Close()) // writer file should exist fi, err := os.Stat(writer.Path()) @@ -788,7 +811,7 @@ func (s *utilsTest) TestTouchCopyBufferedDefaultBufferSize() { } -func (s *utilsTest) TestTouchCopyBufferedNonDefaultBufferSize() { +func (s *utilsSuite) TestTouchCopyBufferedNonDefaultBufferSize() { // write out blank file tmpfile, err := os.CreateTemp("", "utils_test") @@ -839,6 +862,7 @@ func (s *utilsTest) TestTouchCopyBufferedNonDefaultBufferSize() { panic(err) } }() + s.NoError(writer.Close()) // writer file should exist fi, err := os.Stat(writer.Path()) @@ -869,5 +893,5 @@ func (s *utilsTest) TestTouchCopyBufferedNonDefaultBufferSize() { } func TestUtils(t *testing.T) { - suite.Run(t, new(utilsTest)) + suite.Run(t, new(utilsSuite)) }