Bugfix: Remove early finish for zero byte file uploads (#2309)

* fix empty file upload behaviour * fix upload tests * fix upload tests * refactor upload helper * add more upload related tests
cs3org · Dec 2, 2021 · dd009dc · dd009dc
1 parent 4543e1a
commit dd009dc
Show file tree

Hide file tree

Showing 10 changed files with 196 additions and 169 deletions.
diff --git a/changelog/unreleased/fix-skip-early-empty-file-creation.md b/changelog/unreleased/fix-skip-early-empty-file-creation.md
@@ -0,0 +1,7 @@
+Bugfix: Remove early finish for zero byte file uploads
+
+We've fixed the upload of zero byte files by removing the
+early upload finishing mechanism.
+
+https://github.com/cs3org/reva/issues/2309
+https://github.com/owncloud/ocis/issues/2609
diff --git a/internal/http/services/owncloud/ocdav/tus.go b/internal/http/services/owncloud/ocdav/tus.go
@@ -226,40 +226,36 @@ func (s *svc) handleTusPost(ctx context.Context, w http.ResponseWriter, r *http.
 
 		var httpRes *http.Response
 
-		if length != 0 {
-			httpReq, err := rhttp.NewRequest(ctx, http.MethodPatch, ep, r.Body)
-			if err != nil {
-				log.Debug().Err(err).Msg("wrong request")
-				w.WriteHeader(http.StatusInternalServerError)
-				return
-			}
+		httpReq, err := rhttp.NewRequest(ctx, http.MethodPatch, ep, r.Body)
+		if err != nil {
+			log.Debug().Err(err).Msg("wrong request")
+			w.WriteHeader(http.StatusInternalServerError)
+			return
+		}
 
-			httpReq.Header.Set(HeaderContentType, r.Header.Get(HeaderContentType))
-			httpReq.Header.Set(HeaderContentLength, r.Header.Get(HeaderContentLength))
-			if r.Header.Get(HeaderUploadOffset) != "" {
-				httpReq.Header.Set(HeaderUploadOffset, r.Header.Get(HeaderUploadOffset))
-			} else {
-				httpReq.Header.Set(HeaderUploadOffset, "0")
-			}
-			httpReq.Header.Set(HeaderTusResumable, r.Header.Get(HeaderTusResumable))
+		httpReq.Header.Set(HeaderContentType, r.Header.Get(HeaderContentType))
+		httpReq.Header.Set(HeaderContentLength, r.Header.Get(HeaderContentLength))
+		if r.Header.Get(HeaderUploadOffset) != "" {
+			httpReq.Header.Set(HeaderUploadOffset, r.Header.Get(HeaderUploadOffset))
+		} else {
+			httpReq.Header.Set(HeaderUploadOffset, "0")
+		}
+		httpReq.Header.Set(HeaderTusResumable, r.Header.Get(HeaderTusResumable))
 
-			httpRes, err = s.client.Do(httpReq)
-			if err != nil {
-				log.Error().Err(err).Msg("error doing GET request to data service")
-				w.WriteHeader(http.StatusInternalServerError)
-				return
-			}
-			defer httpRes.Body.Close()
+		httpRes, err = s.client.Do(httpReq)
+		if err != nil {
+			log.Error().Err(err).Msg("error doing GET request to data service")
+			w.WriteHeader(http.StatusInternalServerError)
+			return
+		}
+		defer httpRes.Body.Close()
 
-			w.Header().Set(HeaderUploadOffset, httpRes.Header.Get(HeaderUploadOffset))
-			w.Header().Set(HeaderTusResumable, httpRes.Header.Get(HeaderTusResumable))
-			w.Header().Set(HeaderTusUploadExpires, httpRes.Header.Get(HeaderTusUploadExpires))
-			if httpRes.StatusCode != http.StatusNoContent {
-				w.WriteHeader(httpRes.StatusCode)
-				return
-			}
-		} else {
-			log.Debug().Msg("Skipping sending a Patch request as body is empty")
+		w.Header().Set(HeaderUploadOffset, httpRes.Header.Get(HeaderUploadOffset))
+		w.Header().Set(HeaderTusResumable, httpRes.Header.Get(HeaderTusResumable))
+		w.Header().Set(HeaderTusUploadExpires, httpRes.Header.Get(HeaderTusUploadExpires))
+		if httpRes.StatusCode != http.StatusNoContent {
+			w.WriteHeader(httpRes.StatusCode)
+			return
 		}
 
 		// check if upload was fully completed

diff --git a/pkg/storage/fs/owncloud/upload.go b/pkg/storage/fs/owncloud/upload.go
@@ -53,18 +53,7 @@ var defaultFilePerm = os.FileMode(0664)
 func (fs *ocfs) Upload(ctx context.Context, ref *provider.Reference, r io.ReadCloser) error {
 	upload, err := fs.GetUpload(ctx, ref.GetPath())
 	if err != nil {
-		// Upload corresponding to this ID was not found.
-		// Assume that this corresponds to the resource path to which the file has to be uploaded.
-
-		// Set the length to 0 and set SizeIsDeferred to true
-		metadata := map[string]string{"sizedeferred": "true"}
-		uploadIDs, err := fs.InitiateUpload(ctx, ref, 0, metadata)
-		if err != nil {
-			return err
-		}
-		if upload, err = fs.GetUpload(ctx, uploadIDs["simple"]); err != nil {
-			return errors.Wrap(err, "ocfs: error retrieving upload")
-		}
+		return errors.Wrap(err, "ocfs: error retrieving upload")
 	}
 
 	uploadInfo := upload.(*fileUpload)
@@ -234,16 +223,6 @@ func (fs *ocfs) NewUpload(ctx context.Context, info tusd.FileInfo) (upload tusd.
 		ctx:      ctx,
 	}
 
-	if !info.SizeIsDeferred && info.Size == 0 {
-		log.Debug().Interface("info", info).Msg("ocfs: finishing upload for empty file")
-		// no need to create info file and finish directly
-		err := u.FinishUpload(ctx)
-		if err != nil {
-			return nil, err
-		}
-		return u, nil
-	}
-
 	// writeInfo creates the file by itself if necessary
 	err = u.writeInfo()
 	if err != nil {

diff --git a/pkg/storage/fs/owncloudsql/upload.go b/pkg/storage/fs/owncloudsql/upload.go
@@ -49,18 +49,7 @@ var defaultFilePerm = os.FileMode(0664)
 func (fs *owncloudsqlfs) Upload(ctx context.Context, ref *provider.Reference, r io.ReadCloser) error {
 	upload, err := fs.GetUpload(ctx, ref.GetPath())
 	if err != nil {
-		// Upload corresponding to this ID was not found.
-		// Assume that this corresponds to the resource path to which the file has to be uploaded.
-
-		// Set the length to 0 and set SizeIsDeferred to true
-		metadata := map[string]string{"sizedeferred": "true"}
-		uploadIDs, err := fs.InitiateUpload(ctx, ref, 0, metadata)
-		if err != nil {
-			return err
-		}
-		if upload, err = fs.GetUpload(ctx, uploadIDs["simple"]); err != nil {
-			return errors.Wrap(err, "owncloudsql: error retrieving upload")
-		}
+		return errors.Wrap(err, "owncloudsql: error retrieving upload")
 	}
 
 	uploadInfo := upload.(*fileUpload)
@@ -236,16 +225,6 @@ func (fs *owncloudsqlfs) NewUpload(ctx context.Context, info tusd.FileInfo) (upl
 		ctx:      ctx,
 	}
 
-	if !info.SizeIsDeferred && info.Size == 0 {
-		log.Debug().Interface("info", info).Msg("owncloudsql: finishing upload for empty file")
-		// no need to create info file and finish directly
-		err := u.FinishUpload(ctx)
-		if err != nil {
-			return nil, err
-		}
-		return u, nil
-	}
-
 	// writeInfo creates the file by itself if necessary
 	err = u.writeInfo()
 	if err != nil {

diff --git a/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go b/pkg/storage/utils/decomposedfs/decomposedfs_concurrency_test.go
@@ -20,7 +20,6 @@ package decomposedfs_test
 
 import (
 	"context"
-	"fmt"
 	"io/ioutil"
 	"os"
 	"path"
@@ -85,36 +84,24 @@ var _ = Describe("Decomposed", func() {
 	Describe("concurrent", func() {
 		Describe("Upload", func() {
 			var (
-				f, f1 *os.File
+				r1 = []byte("test")
+				r2 = []byte("another run")
 			)
 
-			BeforeEach(func() {
-				// Prepare two test files for upload
-				err := ioutil.WriteFile(fmt.Sprintf("%s/%s", tmpRoot, "f.lol"), []byte("test"), 0644)
-				Expect(err).ToNot(HaveOccurred())
-				f, err = os.Open(fmt.Sprintf("%s/%s", tmpRoot, "f.lol"))
-				Expect(err).ToNot(HaveOccurred())
-
-				err = ioutil.WriteFile(fmt.Sprintf("%s/%s", tmpRoot, "f1.lol"), []byte("another run"), 0644)
-				Expect(err).ToNot(HaveOccurred())
-				f1, err = os.Open(fmt.Sprintf("%s/%s", tmpRoot, "f1.lol"))
-				Expect(err).ToNot(HaveOccurred())
-			})
-
 			PIt("generates two revisions", func() {
 				// runtime.GOMAXPROCS(1) // uncomment to remove concurrency and see revisions working.
 				wg := &sync.WaitGroup{}
 				wg.Add(2)
 
 				// upload file with contents: "test"
 				go func(wg *sync.WaitGroup) {
-					_ = fs.Upload(ctx, &provider.Reference{Path: "uploaded.txt"}, f)
+					_ = helpers.Upload(ctx, fs, &provider.Reference{Path: "uploaded.txt"}, r1)
 					wg.Done()
 				}(wg)
 
 				// upload file with contents: "another run"
 				go func(wg *sync.WaitGroup) {
-					_ = fs.Upload(ctx, &provider.Reference{Path: "uploaded.txt"}, f1)
+					_ = helpers.Upload(ctx, fs, &provider.Reference{Path: "uploaded.txt"}, r2)
 					wg.Done()
 				}(wg)
 

diff --git a/pkg/storage/utils/decomposedfs/upload.go b/pkg/storage/utils/decomposedfs/upload.go
@@ -57,18 +57,7 @@ var defaultFilePerm = os.FileMode(0664)
 func (fs *Decomposedfs) Upload(ctx context.Context, ref *provider.Reference, r io.ReadCloser) (err error) {
 	upload, err := fs.GetUpload(ctx, ref.GetPath())
 	if err != nil {
-		// Upload corresponding to this ID was not found.
-		// Assume that this corresponds to the resource path to which the file has to be uploaded.
-
-		// Set the length to 0 and set SizeIsDeferred to true
-		metadata := map[string]string{"sizedeferred": "true"}
-		uploadIDs, err := fs.InitiateUpload(ctx, ref, 0, metadata)
-		if err != nil {
-			return err
-		}
-		if upload, err = fs.GetUpload(ctx, uploadIDs["simple"]); err != nil {
-			return errors.Wrap(err, "Decomposedfs: error retrieving upload")
-		}
+		return errors.Wrap(err, "Decomposedfs: error retrieving upload")
 	}
 
 	uploadInfo := upload.(*fileUpload)
@@ -297,16 +286,6 @@ func (fs *Decomposedfs) NewUpload(ctx context.Context, info tusd.FileInfo) (uplo
 		ctx:      ctx,
 	}
 
-	if !info.SizeIsDeferred && info.Size == 0 {
-		log.Debug().Interface("info", info).Msg("Decomposedfs: finishing upload for empty file")
-		// no need to create info file and finish directly
-		err := u.FinishUpload(ctx)
-		if err != nil {
-			return nil, err
-		}
-		return u, nil
-	}
-
 	// writeInfo creates the file by itself if necessary
 	err = u.writeInfo()
 	if err != nil {