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.

Sign up for GitHub

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

Fixed #152 bug where s3 backend failed to read empty files #153

Merged
merged 2 commits into from
Jan 22, 2024
Merged

Fixed #152 bug where s3 backend failed to read empty files #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b6b5261
Fixed #152 bug where s3 backend failed to read empty files
funkyshu Jan 22, 2024
9b2f04f
corrected changelog version
funkyshu Jan 22, 2024
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
View file
Open in desktop
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.1] - 2024-01-22
### Fixed
- Fixed #152 bug where s3 backend failed to read empty files

## [6.11.0] - 2024-01-22
### Added
- Added support for hmac-sha1 and hmac-sha1-96 and removed hmac-ripemd160
Expand Down
40 changes: 25 additions & 15 deletions backend/s3/file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"io"
"net/url"
"path"
"strings"
"time"

"github.com/aws/aws-sdk-go/aws/awserr"
Expand Down Expand Up @@ -535,26 +536,35 @@ func waitUntilFileExists(file vfs.File, retries int) error {

func (f *File) getReader() (io.ReadCloser, error) {
if f.reader == nil {
Copy link
Member Author

@funkyshu funkyshu Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

GH does a poor job of showing that I just moved this down to be nested under 'else'. Basically, were checking the source size and return a noop readcloser if it's zero. Otherwize return the usual range-based s3 reader.

// Create the request to get the object
input := new(s3.GetObjectInput).
SetBucket(f.bucket).
SetKey(f.key).
SetRange(fmt.Sprintf("bytes=%d-", f.cursorPos))

// Get the client
client, err := f.fileSystem.Client()
sz, err := f.Size()
if err != nil {
return nil, err
}
if sz == 0 {
f.reader = io.NopCloser(strings.NewReader(""))
} else {

// Request the object
result, err := client.GetObject(input)
if err != nil {
return nil, err
}
// Create the request to get the object
input := new(s3.GetObjectInput).
SetBucket(f.bucket).
SetKey(f.key).
SetRange(fmt.Sprintf("bytes=%d-", f.cursorPos))

// Set the reader to the body of the object
f.reader = result.Body
// Get the client
client, err := f.fileSystem.Client()
if err != nil {
return nil, err
}

// Request the object
result, err := client.GetObject(input)
if err != nil {
return nil, err
}

// Set the reader to the body of the object
f.reader = result.Body
}
}
return f.reader, nil
}
Expand Down
16 changes: 16 additions & 0 deletions backend/s3/file_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ func (ts *fileTestSuite) TestRead() {
}

var localFile = bytes.NewBuffer([]byte{})
s3apiMock.
On("HeadObject", mock.AnythingOfType("*s3.HeadObjectInput")).
Return(&s3.HeadObjectOutput{ContentLength: aws.Int64(12)}, nil).
Once()
s3apiMock.
On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).
Return(&s3.GetObjectOutput{Body: io.NopCloser(strings.NewReader(contents))}, nil).
Expand All @@ -74,6 +78,10 @@ func (ts *fileTestSuite) TestRead() {
ts.Equal(contents, localFile.String(), "Copying an s3 file to a buffer should fill buffer with file's contents")

// test read with error
s3apiMock.
On("HeadObject", mock.AnythingOfType("*s3.HeadObjectInput")).
Return(&s3.HeadObjectOutput{ContentLength: aws.Int64(12)}, nil).
Once()
s3apiMock.
On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).
Return(nil, errors.New("some error")).
Expand Down Expand Up @@ -135,6 +143,10 @@ func (ts *fileTestSuite) TestSeek() {
ts.Equal(tc.expectedPos, pos, "Expected position does not match for seek offset %d and whence %d", tc.seekOffset, tc.seekWhence)

// Mock the GetObject call
s3apiMock.
On("HeadObject", mock.AnythingOfType("*s3.HeadObjectInput")).
Return(headOutput, nil).
Once()
s3apiMock.On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).
Return(&s3.GetObjectOutput{Body: io.NopCloser(strings.NewReader(tc.readContent))}, nil).
Once()
Expand Down Expand Up @@ -235,6 +247,10 @@ func (ts *fileTestSuite) TestEmptyCopyToFile() {
targetFile := &mocks.File{}
targetFile.On("Write", mock.Anything).Return(0, nil)
targetFile.On("Close").Return(nil)
s3apiMock.
On("HeadObject", mock.AnythingOfType("*s3.HeadObjectInput")).
Return(&s3.HeadObjectOutput{ContentLength: aws.Int64(0)}, nil).
Once()
s3apiMock.
On("GetObject", mock.AnythingOfType("*s3.GetObjectInput")).
Return(&s3.GetObjectOutput{Body: io.NopCloser(strings.NewReader(""))}, nil).
Expand Down
Loading