Skip to content

Commit

Permalink
Sftp writes should truncate 154 (#155)
Browse files Browse the repository at this point in the history
* added io integration tests based on unix fs behaviors

* Updated sftp backend to overwrite except after read/seek. fixes #154

* address lint issues

* update changelog

* fixed type in changelog

* forgot to add back intergration flags

* remove no-longer-relevant comment from test case
  • Loading branch information
funkyshu authored Jan 31, 2024
1 parent 0b59a4f commit d995180
Show file tree
Hide file tree
Showing 5 changed files with 650 additions and 63 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ 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]

## [6.11.2] - 2024-01-30
### Fixed
- Fixed 154 bug. Updated sftp backend to overwrite except after read/seek.

## [6.11.1] - 2024-01-22
### Fixed
- Fixed #152 bug where s3 backend failed to read empty files
Expand Down
17 changes: 15 additions & 2 deletions backend/sftp/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ type File struct {
path string
sftpfile ReadWriteSeekCloser
opener fileOpener
seekCalled bool
readCalled bool
}

// this type allow for injecting a mock fileOpener function
Expand Down Expand Up @@ -243,6 +245,9 @@ func (f *File) Close() error {
f.fileSystem.connTimerStop()
defer f.fileSystem.connTimerStart()

f.seekCalled = false
f.readCalled = false

if f.sftpfile != nil {
err := f.sftpfile.Close()
if err != nil {
Expand All @@ -260,11 +265,13 @@ func (f *File) Read(p []byte) (n int, err error) {
f.fileSystem.connTimerStop()
defer f.fileSystem.connTimerStart()

sftpfile, err := f.openFile(os.O_RDONLY)
sftpfile, err := f.openFile(os.O_RDWR)
if err != nil {
return 0, err
}

f.readCalled = true

return sftpfile.Read(p)
}

Expand All @@ -279,6 +286,7 @@ func (f *File) Seek(offset int64, whence int) (int64, error) {
return 0, err
}

f.seekCalled = true
return sftpfile.Seek(offset, whence)
}

Expand All @@ -288,7 +296,12 @@ func (f *File) Write(data []byte) (res int, err error) {
f.fileSystem.connTimerStop()
defer f.fileSystem.connTimerStart()

sftpfile, err := f.openFile(os.O_WRONLY | os.O_CREATE)
flags := os.O_RDWR | os.O_CREATE
if !f.readCalled || f.seekCalled {
flags |= os.O_TRUNC
}

sftpfile, err := f.openFile(flags)
if err != nil {
return 0, err
}
Expand Down
69 changes: 8 additions & 61 deletions backend/testsuite/backend_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,7 @@ import (

"github.com/c2fo/vfs/v6"
"github.com/c2fo/vfs/v6/backend/azure"
"github.com/c2fo/vfs/v6/backend/ftp"
"github.com/c2fo/vfs/v6/backend/gs"
"github.com/c2fo/vfs/v6/backend/mem"
_os "github.com/c2fo/vfs/v6/backend/os"
"github.com/c2fo/vfs/v6/backend/s3"
"github.com/c2fo/vfs/v6/backend/sftp"
"github.com/c2fo/vfs/v6/utils"
"github.com/c2fo/vfs/v6/vfssimple"
Expand All @@ -33,55 +29,6 @@ type vfsTestSuite struct {
testLocations map[string]vfs.Location
}

func copyOsLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*_os.Location)
ret := &cp

// setup os location
exists, err := ret.Exists()
if err != nil {
panic(err)
}
if !exists {
err := os.Mkdir(ret.Path(), 0755)
if err != nil {
panic(err)
}
}

return ret
}

func copyMemLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*mem.Location)
return &cp
}

func copyS3Location(loc vfs.Location) vfs.Location {
cp := *loc.(*s3.Location)
return &cp
}

func copySFTPLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*sftp.Location)
return &cp
}

func copyFTPLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*ftp.Location)
return &cp
}

func copyGSLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*gs.Location)
return &cp
}

func copyAzureLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*azure.Location)
return &cp
}

func buildExpectedURI(fs vfs.FileSystem, volume, path string) string {
if fs.Name() == "azure" {
azFs := fs.(*azure.FileSystem)
Expand All @@ -98,19 +45,19 @@ func (s *vfsTestSuite) SetupSuite() {
s.NoError(err)
switch l.FileSystem().Scheme() {
case "file":
s.testLocations[l.FileSystem().Scheme()] = copyOsLocation(l)
s.testLocations[l.FileSystem().Scheme()] = CopyOsLocation(l)
case "s3":
s.testLocations[l.FileSystem().Scheme()] = copyS3Location(l)
s.testLocations[l.FileSystem().Scheme()] = CopyS3Location(l)
case "sftp":
s.testLocations[l.FileSystem().Scheme()] = copySFTPLocation(l)
s.testLocations[l.FileSystem().Scheme()] = CopySFTPLocation(l)
case "gs":
s.testLocations[l.FileSystem().Scheme()] = copyGSLocation(l)
s.testLocations[l.FileSystem().Scheme()] = CopyGSLocation(l)
case "mem":
s.testLocations[l.FileSystem().Scheme()] = copyMemLocation(l)
s.testLocations[l.FileSystem().Scheme()] = CopyMemLocation(l)
case "https":
s.testLocations[l.FileSystem().Scheme()] = copyAzureLocation(l)
s.testLocations[l.FileSystem().Scheme()] = CopyAzureLocation(l)
case "ftp":
s.testLocations[l.FileSystem().Scheme()] = copyFTPLocation(l)
s.testLocations[l.FileSystem().Scheme()] = CopyFTPLocation(l)
default:
panic(fmt.Sprintf("unknown scheme: %s", l.FileSystem().Scheme()))
}
Expand Down Expand Up @@ -914,7 +861,7 @@ func (s *vfsTestSuite) File(baseLoc vfs.Location) {
// gs-specific test cases
func (s *vfsTestSuite) gsList(baseLoc vfs.Location) {
/*
test scenario:
test description:
When a persistent "folder" is created through the UI, it simply creates a zero length object
with a trailing "/". The UI or gsutil knows to interpret these objects as folders but they are
still just objects. List(), in its current state, should ignore these objects.
Expand Down
63 changes: 63 additions & 0 deletions backend/testsuite/common.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package testsuite

import (
"os"

"github.com/c2fo/vfs/v6"
"github.com/c2fo/vfs/v6/backend/azure"
"github.com/c2fo/vfs/v6/backend/ftp"
"github.com/c2fo/vfs/v6/backend/gs"
"github.com/c2fo/vfs/v6/backend/mem"
_os "github.com/c2fo/vfs/v6/backend/os"
"github.com/c2fo/vfs/v6/backend/s3"
"github.com/c2fo/vfs/v6/backend/sftp"
)

func CopyOsLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*_os.Location)
ret := &cp

// setup os location
exists, err := ret.Exists()
if err != nil {
panic(err)
}
if !exists {
err := os.Mkdir(ret.Path(), 0750)
if err != nil {
panic(err)
}
}

return ret
}

func CopyMemLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*mem.Location)
return &cp
}

func CopyS3Location(loc vfs.Location) vfs.Location {
cp := *loc.(*s3.Location)
return &cp
}

func CopySFTPLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*sftp.Location)
return &cp
}

func CopyFTPLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*ftp.Location)
return &cp
}

func CopyGSLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*gs.Location)
return &cp
}

func CopyAzureLocation(loc vfs.Location) vfs.Location {
cp := *loc.(*azure.Location)
return &cp
}
Loading

0 comments on commit d995180

Please sign in to comment.