Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sftp writes should truncate 154 #155

Merged
merged 7 commits into from
Jan 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only moved contents of this file to common.go.

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 {
funkyshu marked this conversation as resolved.
Show resolved Hide resolved
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
Loading