From 451f9c6ce071b3dd727d1cc097da9c58a08c4dc7 Mon Sep 17 00:00:00 2001 From: Igor Drozdov Date: Tue, 16 Nov 2021 14:56:49 +0300 Subject: [PATCH] Allow uploading up to 10 files Currently, we can't upload multiple designs at once It fixes the bug Changelog: fixed --- .../user_uploads_designs_spec.rb | 18 +++++++++++++----- .../artifacts/artifacts_upload_test.go | 2 +- workhorse/internal/upload/rewrite.go | 10 ++++++---- .../internal/upload/saved_file_tracker.go | 4 ++++ .../internal/upload/saved_file_tracker_test.go | 11 +++++++++++ workhorse/internal/upload/uploads.go | 4 ++-- workhorse/internal/upload/uploads_test.go | 10 +++++----- 7 files changed, 42 insertions(+), 17 deletions(-) diff --git a/spec/features/projects/issues/design_management/user_uploads_designs_spec.rb b/spec/features/projects/issues/design_management/user_uploads_designs_spec.rb index bfa7be5bb5cdbb..e885c0c44131d7 100644 --- a/spec/features/projects/issues/design_management/user_uploads_designs_spec.rb +++ b/spec/features/projects/issues/design_management/user_uploads_designs_spec.rb @@ -27,10 +27,10 @@ expect(page).to have_content('dk.png') end - upload_design(gif_fixture, count: 2) + upload_design([gif_fixture, logo_svg_fixture, big_image_fixture], count: 4) - expect(page).to have_selector('.js-design-list-item', count: 2) - expect(page.all('.js-design-list-item').map(&:text)).to eq(['dk.png', 'banana_sample.gif']) + expect(page).to have_selector('.js-design-list-item', count: 4) + expect(page.all('.js-design-list-item').map(&:text)).to eq(['dk.png', 'banana_sample.gif', 'logo_sample.svg', 'big-image.png']) end end @@ -50,8 +50,16 @@ def gif_fixture Rails.root.join('spec', 'fixtures', 'banana_sample.gif') end - def upload_design(fixture, count:) - attach_file(:upload_file, fixture, match: :first, make_visible: true) + def logo_svg_fixture + Rails.root.join('spec', 'fixtures', 'logo_sample.svg') + end + + def big_image_fixture + Rails.root.join('spec', 'fixtures', 'big-image.png') + end + + def upload_design(fixtures, count:) + attach_file(:upload_file, fixtures, multiple: true, match: :first, make_visible: true) wait_for('designs uploaded') do issue.reload.designs.count == count diff --git a/workhorse/internal/artifacts/artifacts_upload_test.go b/workhorse/internal/artifacts/artifacts_upload_test.go index df1c30dcff0b76..3e8a52be1a1c6b 100644 --- a/workhorse/internal/artifacts/artifacts_upload_test.go +++ b/workhorse/internal/artifacts/artifacts_upload_test.go @@ -270,7 +270,7 @@ func TestUploadHandlerForMultipleFiles(t *testing.T) { require.NoError(t, s.writer.Close()) response := testUploadArtifacts(t, s.writer.FormDataContentType(), s.url, s.buffer) - require.Equal(t, http.StatusBadRequest, response.Code) + require.Equal(t, http.StatusInternalServerError, response.Code) } func TestUploadFormProcessing(t *testing.T) { diff --git a/workhorse/internal/upload/rewrite.go b/workhorse/internal/upload/rewrite.go index 3dfab120188be6..b9324ac8b7b756 100644 --- a/workhorse/internal/upload/rewrite.go +++ b/workhorse/internal/upload/rewrite.go @@ -24,10 +24,12 @@ import ( "gitlab.com/gitlab-org/gitlab/workhorse/internal/upload/exif" ) +const maxFilesAllowed = 10 + // ErrInjectedClientParam means that the client sent a parameter that overrides one of our own fields var ( - ErrInjectedClientParam = errors.New("injected client parameter") - ErrMultipleFilesUploaded = errors.New("upload request contains more than one file") + ErrInjectedClientParam = errors.New("injected client parameter") + ErrTooManyFilesUploaded = fmt.Errorf("upload request contains more than %v files", maxFilesAllowed) ) var ( @@ -117,8 +119,8 @@ func rewriteFormFilesFromMultipart(r *http.Request, writer *multipart.Writer, pr } func (rew *rewriter) handleFilePart(ctx context.Context, name string, p *multipart.Part, opts *filestore.SaveFileOpts) error { - if rew.filter.Count() > 0 { - return ErrMultipleFilesUploaded + if rew.filter.Count() >= maxFilesAllowed { + return ErrTooManyFilesUploaded } multipartFiles.WithLabelValues(rew.filter.Name()).Inc() diff --git a/workhorse/internal/upload/saved_file_tracker.go b/workhorse/internal/upload/saved_file_tracker.go index cd41e661b7d77b..e6f9a8c9a88909 100644 --- a/workhorse/internal/upload/saved_file_tracker.go +++ b/workhorse/internal/upload/saved_file_tracker.go @@ -27,6 +27,10 @@ func (s *SavedFileTracker) Count() int { } func (s *SavedFileTracker) ProcessFile(_ context.Context, fieldName string, file *filestore.FileHandler, _ *multipart.Writer) error { + if _, ok := s.rewrittenFields[fieldName]; ok { + return fmt.Errorf("the %v field has already been processed", fieldName) + } + s.Track(fieldName, file.LocalPath) return nil } diff --git a/workhorse/internal/upload/saved_file_tracker_test.go b/workhorse/internal/upload/saved_file_tracker_test.go index b34dd9aed4ffaa..ba927db253e2e9 100644 --- a/workhorse/internal/upload/saved_file_tracker_test.go +++ b/workhorse/internal/upload/saved_file_tracker_test.go @@ -37,3 +37,14 @@ func TestSavedFileTracking(t *testing.T) { require.Contains(t, rewrittenFields, "test") } + +func TestDuplicatedFileProcessing(t *testing.T) { + tracker := SavedFileTracker{} + file := &filestore.FileHandler{} + + require.NoError(t, tracker.ProcessFile(context.Background(), "file", file, nil)) + + err := tracker.ProcessFile(context.Background(), "file", file, nil) + require.Error(t, err) + require.Equal(t, "the file field has already been processed", err.Error()) +} diff --git a/workhorse/internal/upload/uploads.go b/workhorse/internal/upload/uploads.go index 8f4c8bb95f08a9..b408d260379360 100644 --- a/workhorse/internal/upload/uploads.go +++ b/workhorse/internal/upload/uploads.go @@ -35,8 +35,8 @@ func HandleFileUploads(w http.ResponseWriter, r *http.Request, h http.Handler, p switch err { case ErrInjectedClientParam: helper.CaptureAndFail(w, r, err, "Bad Request", http.StatusBadRequest) - case ErrMultipleFilesUploaded: - helper.CaptureAndFail(w, r, err, "Uploading multiple files is not allowed", http.StatusBadRequest) + case ErrTooManyFilesUploaded: + helper.CaptureAndFail(w, r, err, err.Error(), http.StatusBadRequest) case http.ErrNotMultipart: h.ServeHTTP(w, r) case filestore.ErrEntityTooLarge: diff --git a/workhorse/internal/upload/uploads_test.go b/workhorse/internal/upload/uploads_test.go index b5db8003d41b80..7c22b3b4e2859c 100644 --- a/workhorse/internal/upload/uploads_test.go +++ b/workhorse/internal/upload/uploads_test.go @@ -260,10 +260,10 @@ func TestUploadingMultipleFiles(t *testing.T) { var buffer bytes.Buffer writer := multipart.NewWriter(&buffer) - _, err = writer.CreateFormFile("file", "my.file") - require.NoError(t, err) - _, err = writer.CreateFormFile("file", "my.file") - require.NoError(t, err) + for i := 0; i < 11; i++ { + _, err = writer.CreateFormFile(fmt.Sprintf("file %v", i), "my.file") + require.NoError(t, err) + } require.NoError(t, writer.Close()) httpRequest, err := http.NewRequest("PUT", "/url/path", &buffer) @@ -279,7 +279,7 @@ func TestUploadingMultipleFiles(t *testing.T) { HandleFileUploads(response, httpRequest, nilHandler, apiResponse, &testFormProcessor{}, opts) require.Equal(t, 400, response.Code) - require.Equal(t, "Uploading multiple files is not allowed\n", response.Body.String()) + require.Equal(t, "upload request contains more than 10 files\n", response.Body.String()) } func TestUploadProcessingFile(t *testing.T) { -- GitLab