-
Notifications
You must be signed in to change notification settings - Fork 113
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
OCFS properly process empty file upload #734
Conversation
reproduced using these two curl commands: Scenario 1: Webdav PUT upload of empty file
Scenario 2: TUS upload of empty file
|
I added a few log lines and ignoered deletion of non existing files: diff --git a/pkg/storage/fs/owncloud/upload.go b/pkg/storage/fs/owncloud/upload.go
index 8b7be82..3b44aa2 100644
--- a/pkg/storage/fs/owncloud/upload.go
+++ b/pkg/storage/fs/owncloud/upload.go
@@ -158,6 +158,9 @@ func (fs *ocfs) NewUpload(ctx context.Context, info tusd.FileInfo) (upload tusd.
"LogLevel": log.GetLevel().String(),
}
+
+ log.Debug().Interface("info", info).Msg("ocfs: built storage info")
+
// Create binary file in the upload folder with no content
file, err := os.OpenFile(binPath, os.O_CREATE|os.O_WRONLY, defaultFilePerm)
if err != nil {
@@ -175,6 +178,7 @@ func (fs *ocfs) NewUpload(ctx context.Context, info tusd.FileInfo) (upload tusd.
if !info.SizeIsDeferred && info.Size == 0 {
// no need to create info file and finish directly
+ log.Debug().Interface("info", info).Msg("ocfs: finishing upload for empty file")
u.FinishUpload(ctx)
return u, nil
}
@@ -366,10 +370,14 @@ func (fs *ocfs) AsTerminatableUpload(upload tusd.Upload) tusd.TerminatableUpload
// Terminate terminates the upload
func (upload *fileUpload) Terminate(ctx context.Context) error {
if err := os.Remove(upload.infoPath); err != nil {
- return err
+ if !os.IsNotExist(err) {
+ return err
+ }
}
if err := os.Remove(upload.binPath); err != nil {
- return err
+ if !os.IsNotExist(err) {
+ return err
+ }
}
return nil
} |
Both scenarios work now for me locally, I was testing it wrong, forgot to restart a relevant service. I've added @butonic's change to avoid errors when the info file is not there. @butonic please test the EOS changes |
works on eos when using the |
Whenever an upload of an empty file has started, finish it directly instead of waiting for further bytes. This fixes an issue when uploading empty files with PUT or POST. Co-authored-by: Jörn Friedrich Dreyer <[email protected]>
I've fixed the linting issue and retested with OCFS to make sure there are no issues with the error condition check. I think this is good to go |
Whenever an upload of an empty file has started, finish it directly instead of waiting for further bytes. This fixes an issue when uploading empty files with PUT or POST. Co-authored-by: Jörn Friedrich Dreyer <[email protected]> Co-authored-by: Jörn Friedrich Dreyer <[email protected]>
We should fix the two TUS layers to properly process uploads with 0 bytes to automatically finish:
Fixes owncloud/ocis-reva#188
Scenario 1: Webdav PUT upload of empty file
In Phoenix when creating a new file, it uses a PUT with empty payload to create the file.
The PUT goes to ocdav/put.go and gets converted to a TUS upload to the backend.
In this PR the backend has a new shortcut to automatically finish the upload.
Scenario 2: TUS upload of empty file
In Phoenix, try uploading an empty file that exists on local disk.
In this scenario, the code flow goes into tus.go and initiates the upload in the backend storage.
However it doesn't create the file automatically despite the fix for scenario 1.
I tried to debug this and somehow it seems that it doesn't even reach owncloud/upload.go for the upload and aborts earlier.
I'm a bit confused about the code path between the file upload initiation and finishing the upload in the case of TUS. Maybe it's in the storage provider.
I also tried to make it send an empty PATCH to the backend but that makes it return 400 bad request.
Also: it is likely that we'll need a similar fix for EOS.
@butonic thoughts ?