Skip to content

Commit

Permalink
Updated sftp backend to to fix issue where some servers return a gene… (
Browse files Browse the repository at this point in the history 
#159)

* Updated sftp backend to to fix issue where some servers return a generic error message when a file is opened for RW on Read(). Fixes #158.

* Added changelog release version
  • Loading branch information
@funkyshu
funkyshu authored Feb 2, 2024
1 parent d995180 commit 02139cf
Show file tree
Hide file tree
Showing 3 changed files with 86 additions and 18 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
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.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.
Expand Down
72 changes: 61 additions & 11 deletions backend/sftp/file.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type File struct {
opener fileOpener
seekCalled bool
readCalled bool
flagsUsed int
}

// this type allow for injecting a mock fileOpener function
Expand Down Expand Up @@ -265,7 +266,7 @@ func (f *File) Read(p []byte) (n int, err error) {
f.fileSystem.connTimerStop()
defer f.fileSystem.connTimerStart()

sftpfile, err := f.openFile(os.O_RDWR)
sftpfile, err := f.openFile(os.O_RDONLY)
if err != nil {
return 0, err
}
Expand All @@ -281,7 +282,7 @@ func (f *File) Seek(offset int64, whence int) (int64, error) {
f.fileSystem.connTimerStop()
defer f.fileSystem.connTimerStart()

sftpfile, err := f.openFile(os.O_RDWR)
sftpfile, err := f.openFile(os.O_RDONLY)
if err != nil {
return 0, err
}
Expand All @@ -296,8 +297,10 @@ func (f *File) Write(data []byte) (res int, err error) {
f.fileSystem.connTimerStop()
defer f.fileSystem.connTimerStart()

// unless seek or read is called first, writes should replace a file (not edit)
// writes should edit a file if seek or read is called first
flags := os.O_RDWR | os.O_CREATE
if !f.readCalled || f.seekCalled {
if !f.readCalled && !f.seekCalled {
flags |= os.O_TRUNC
}

Expand All @@ -324,18 +327,70 @@ func (f *File) String() string {
*/

// openFile wrapper allows us to inject a file opener (for mocking) vs the defaultOpenFile.
func (f *File) openFile(flag int) (ReadWriteSeekCloser, error) {
func (f *File) openFile(flags int) (ReadWriteSeekCloser, error) {
if f.sftpfile != nil {
FlagsUsedContainsReadOnly := f.flagsUsed&(os.O_WRONLY|os.O_RDWR) == 0
flagsContainsReadWrite := flags&os.O_RDWR != 0

// if we're trying to open a file for writing and it's already open for read, repoen it for read/write and
// seek to current position
if FlagsUsedContainsReadOnly && flagsContainsReadWrite {
var pos int64

// capture current position if file is open for read (only in edit mode)
if f.readCalled || f.seekCalled {
var err error
// get current position
pos, err = f.sftpfile.Seek(0, io.SeekCurrent)
if err != nil {
return nil, err
}
}

// close file
if err := f.sftpfile.Close(); err != nil {
return nil, err
}

// reopen file for read/write
file, err := f._open(flags)
if err != nil {
return nil, err
}

// seek to current position (only in edit mode)
if f.readCalled || f.seekCalled {
if _, err := file.Seek(pos, io.SeekStart); err != nil {
return nil, err
}
}

f.flagsUsed = flags
f.sftpfile = file

}
return f.sftpfile, nil
}

file, err := f._open(flags)
if err != nil {
return nil, err
}

f.flagsUsed = flags
f.sftpfile = file

return file, nil
}

func (f *File) _open(flags int) (ReadWriteSeekCloser, error) {
client, err := f.fileSystem.Client(f.Authority)
if err != nil {
return nil, err
}
// normally we'd do a defer of fs connTimerStart() here but not necessary since we handle it in the openFile caller

if flag&os.O_CREATE != 0 {
if flags&os.O_CREATE != 0 {
// vfs specifies that all implementations make dir path if it doesn't exist
err = client.MkdirAll(path.Dir(f.path))
if err != nil {
Expand All @@ -350,13 +405,8 @@ func (f *File) openFile(flag int) (ReadWriteSeekCloser, error) {
opener = defaultOpenFile
}

file, err := opener(client, f.Path(), flag)
if err != nil {
return nil, err
}
return opener(client, f.Path(), flags)

f.sftpfile = file
return file, nil
}

// defaultOpenFile uses sftp.Client to open a file and returns an sftp.File
Expand Down
28 changes: 21 additions & 7 deletions backend/sftp/file_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -237,10 +237,11 @@ func (ts *fileTestSuite) TestCopyToFile() {

// set up target
targetClient := &mocks.Client{}
targetClient.On("MkdirAll", "/some").Return(nil).Once()

targetSftpFile := &mocks.SFTPFile{}
targetSftpFile.On("Write", mock.Anything).Return(len(content), nil).Once()
targetSftpFile.On("Close").Return(nil).Once()
targetSftpFile.On("Close").Return(nil).Twice()

auth2, err := utils.NewAuthority("[email protected]:22")
ts.NoError(err)
Expand All @@ -253,6 +254,7 @@ func (ts *fileTestSuite) TestCopyToFile() {
Authority: auth2,
path: "/some/path.txt",
sftpfile: targetSftpFile,
opener: func(c Client, p string, f int) (ReadWriteSeekCloser, error) { return targetSftpFile, nil },
}

// run tests
Expand Down Expand Up @@ -292,10 +294,11 @@ func (ts *fileTestSuite) TestCopyToFileBuffered() {

// set up target
targetClient := &mocks.Client{}
targetClient.On("MkdirAll", "/some").Return(nil).Once()

targetSftpFile := &mocks.SFTPFile{}
targetSftpFile.On("Write", mock.Anything).Return(len(content), nil).Once()
targetSftpFile.On("Close").Return(nil).Once()
targetSftpFile.On("Close").Return(nil).Twice()

auth2, err := utils.NewAuthority("[email protected]:22")
ts.NoError(err)
Expand All @@ -308,6 +311,7 @@ func (ts *fileTestSuite) TestCopyToFileBuffered() {
Authority: auth2,
path: "/some/path.txt",
sftpfile: targetSftpFile,
opener: func(c Client, p string, f int) (ReadWriteSeekCloser, error) { return targetSftpFile, nil },
}

targetMockLocation := &_mocks.Location{}
Expand Down Expand Up @@ -348,10 +352,11 @@ func (ts *fileTestSuite) TestCopyToFileEmpty() {

// set up target
targetClient := &mocks.Client{}
targetClient.On("MkdirAll", "/some").Return(nil).Once()

targetSftpFile := &mocks.SFTPFile{}
targetSftpFile.On("Write", mock.Anything).Return(len(content), nil).Once()
targetSftpFile.On("Close").Return(nil).Once()
targetSftpFile.On("Close").Return(nil).Twice()

auth2, err := utils.NewAuthority("[email protected]:22")
ts.NoError(err)
Expand All @@ -364,6 +369,7 @@ func (ts *fileTestSuite) TestCopyToFileEmpty() {
Authority: auth2,
path: "/some/path.txt",
sftpfile: targetSftpFile,
opener: func(c Client, p string, f int) (ReadWriteSeekCloser, error) { return targetSftpFile, nil },
}

targetMockLocation := &_mocks.Location{}
Expand Down Expand Up @@ -404,10 +410,11 @@ func (ts *fileTestSuite) TestCopyToFileEmptyBuffered() {

// set up target
targetClient := &mocks.Client{}
targetClient.On("MkdirAll", "/some").Return(nil).Once()

targetSftpFile := &mocks.SFTPFile{}
targetSftpFile.On("Write", mock.Anything).Return(len(content), nil).Once()
targetSftpFile.On("Close").Return(nil).Once()
targetSftpFile.On("Close").Return(nil).Twice()

auth2, err := utils.NewAuthority("[email protected]:22")
ts.NoError(err)
Expand All @@ -420,6 +427,7 @@ func (ts *fileTestSuite) TestCopyToFileEmptyBuffered() {
Authority: auth2,
path: "/some/path.txt",
sftpfile: targetSftpFile,
opener: func(c Client, p string, f int) (ReadWriteSeekCloser, error) { return targetSftpFile, nil },
}

targetMockLocation := &_mocks.Location{}
Expand Down Expand Up @@ -461,10 +469,11 @@ func (ts *fileTestSuite) TestCopyToLocation() {

// set up target
targetClient := &mocks.Client{}
targetClient.On("MkdirAll", "/some").Return(nil).Once()

targetSftpFile := &mocks.SFTPFile{}
targetSftpFile.On("Write", mock.Anything).Return(len(content), nil).Once()
targetSftpFile.On("Close").Return(nil).Once()
targetSftpFile.On("Close").Return(nil).Twice()

auth2, err := utils.NewAuthority("[email protected]:22")
ts.NoError(err)
Expand All @@ -477,6 +486,7 @@ func (ts *fileTestSuite) TestCopyToLocation() {
Authority: auth2,
path: "/some/path.txt",
sftpfile: targetSftpFile,
opener: func(c Client, p string, f int) (ReadWriteSeekCloser, error) { return targetSftpFile, nil },
}

targetMockLocation := &_mocks.Location{}
Expand Down Expand Up @@ -521,10 +531,11 @@ func (ts *fileTestSuite) TestMoveToFile_differentAuthority() {

// set up target
targetClient := &mocks.Client{}
targetClient.On("MkdirAll", "/some").Return(nil).Once()

targetSftpFile := &mocks.SFTPFile{}
targetSftpFile.On("Write", mock.Anything).Return(len(content), nil).Once()
targetSftpFile.On("Close").Return(nil).Once()
targetSftpFile.On("Close").Return(nil).Twice()

auth2, err := utils.NewAuthority("[email protected]:22")
ts.NoError(err)
Expand All @@ -537,6 +548,7 @@ func (ts *fileTestSuite) TestMoveToFile_differentAuthority() {
Authority: auth2,
path: "/some/path.txt",
sftpfile: targetSftpFile,
opener: func(c Client, p string, f int) (ReadWriteSeekCloser, error) { return targetSftpFile, nil },
}

// run tests
Expand Down Expand Up @@ -625,10 +637,11 @@ func (ts *fileTestSuite) TestMoveToLocation() {

// set up target
targetClient := &mocks.Client{}
targetClient.On("MkdirAll", "/some/other").Return(nil).Once()

targetSftpFile := &mocks.SFTPFile{}
targetSftpFile.On("Write", mock.Anything).Return(len(content), nil).Once()
targetSftpFile.On("Close").Return(nil).Once()
targetSftpFile.On("Close").Return(nil).Twice()

auth2, err := utils.NewAuthority("[email protected]:22")
ts.NoError(err)
Expand All @@ -641,6 +654,7 @@ func (ts *fileTestSuite) TestMoveToLocation() {
Authority: auth2,
path: "/some/other/path.txt",
sftpfile: targetSftpFile,
opener: func(c Client, p string, f int) (ReadWriteSeekCloser, error) { return targetSftpFile, nil },
}

targetMockLocation := &_mocks.Location{}
Expand Down

0 comments on commit 02139cf

Please sign in to comment.