Merge pull request #6952 from samvera/chunk-upload-follow-up

chunk upload bug fix

app/controllers/hyrax/uploads_controller.rb

@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+
 module Hyrax
   class UploadsController < ApplicationController
     load_and_authorize_resource class: Hyrax::UploadedFile
@@ -9,6 +10,7 @@ def create
       else
         handle_chunked_upload
       end
+
       @upload.save!
     end
 
@@ -23,6 +25,16 @@ def handle_new_upload
       @upload.attributes = { file: params[:files].first, user: current_user }
     end
 
+    def chunk_valid?(upload)
+      content_range = request.headers['CONTENT-RANGE']
+      return false unless content_range
+
+      begin_of_chunk = content_range[/\ (.*?)-/, 1].to_i
+      current_size = upload.file.size
+
+      upload.file.present? && begin_of_chunk == current_size
+    end
+
     def handle_chunked_upload
       @upload = Hyrax::UploadedFile.find(params[:id])
       unpersisted_upload = Hyrax::UploadedFile.new(file: params[:files].first, user: current_user)
@@ -34,18 +46,12 @@ def handle_chunked_upload
       end
     end
 
-    def chunk_valid?(upload)
-      current_size = upload.file.size
-      content_range = request.headers['CONTENT-RANGE']
-
-      return false unless content_range
-
-      begin_of_chunk = content_range[/\ (.*?)-/, 1].to_i
-      upload.file.present? && begin_of_chunk == current_size
-    end
-
     def append_chunk(upload)
-      File.open(upload.file.path, "ab") { |f| f.write(params[:files].first.read) }
+      File.open(upload.file.path, "ab") do |f|
+        f.write(params[:files].first.read)
+      end
+
+      upload.reload
     end
 
     def replace_file(upload, unpersisted_upload)

spec/controllers/hyrax/uploads_controller_spec.rb

@@ -21,17 +21,25 @@
       end
 
       context "when uploading in chunks" do
-        it "appends chunks when they are valid" do
+        it "appends chunks in correct sequence" do
           original_file = fixture_file_upload('/world.png', 'image/png')
           post :create, params: { files: [original_file], format: 'json' }
           original_upload = assigns(:upload)
 
-          request.headers['CONTENT-RANGE'] = 'bytes 0-99/1000'
-          new_chunk = fixture_file_upload('/world.png', 'image/png')
-          post :create, params: { files: [new_chunk], id: original_upload.id, format: 'json' }
+          initial_size = original_upload.file.size
+          request.headers['CONTENT-RANGE'] = "bytes #{initial_size}-#{initial_size + original_file.size - 1}/5000"
+          first_chunk = fixture_file_upload('/world.png', 'image/png')
+          post :create, params: { files: [first_chunk], id: original_upload.id, format: 'json' }
+          original_upload.reload
+          expected_size_after_first_chunk = initial_size * 2
+          expect(original_upload.file.size).to eq(expected_size_after_first_chunk)
 
+          request.headers['CONTENT-RANGE'] = "bytes #{initial_size * 2}-#{(initial_size * 2) + original_file.size - 1}/5000"
+          second_chunk = fixture_file_upload('/world.png', 'image/png')
+          post :create, params: { files: [second_chunk], id: original_upload.id, format: 'json' }
           original_upload.reload
-          expect(original_upload.file.size).to eq(File.size(original_upload.file.path))
+          expected_size_after_second_chunk = initial_size * 3
+          expect(original_upload.file.size).to eq(expected_size_after_second_chunk)
         end
 
         it "replaces file if chunks are mismatched" do
@@ -40,13 +48,12 @@
           original_upload = assigns(:upload)
           original_content = File.read(original_upload.file.path)
 
-          request.headers['CONTENT-RANGE'] = 'bytes 101-200/1000'
+          request.headers['CONTENT-RANGE'] = 'bytes 2000-2999/5000'
           different_chunk = fixture_file_upload('/different_file.png', 'image/png')
           post :create, params: { files: [different_chunk], id: original_upload.id, format: 'json' }
 
           original_upload.reload
           new_content = File.read(original_upload.file.path)
-
           expect(new_content).not_to eq(original_content)
         end
 
@@ -56,15 +63,31 @@
           original_upload = assigns(:upload)
           original_size = original_upload.file.size
 
-          request.headers['CONTENT-RANGE'] = 'bytes 101-200/1000'
+          request.headers['CONTENT-RANGE'] = 'bytes 2000-2999/5000'
           different_chunk = fixture_file_upload('/different_file.png', 'image/png')
           post :create, params: { files: [different_chunk], id: original_upload.id, format: 'json' }
 
           original_upload.reload
           new_size = original_upload.file.size
-
           expect(new_size).not_to eq(original_size)
         end
+
+        it "does not append mismatched chunks" do
+          original_file = fixture_file_upload('/world.png', 'image/png')
+          post :create, params: { files: [original_file], format: 'json' }
+          original_upload = assigns(:upload)
+
+          request.headers['CONTENT-RANGE'] = 'bytes 0-999/5000'
+          first_chunk = fixture_file_upload('/world.png', 'image/png')
+          post :create, params: { files: [first_chunk], id: original_upload.id, format: 'json' }
+
+          request.headers['CONTENT-RANGE'] = 'bytes 3000-3999/5000'
+          out_of_order_chunk = fixture_file_upload('/different_file.png', 'image/png')
+          post :create, params: { files: [out_of_order_chunk], id: original_upload.id, format: 'json' }
+
+          original_upload.reload
+          expect(original_upload.file.size).not_to eq(4000)
+        end
       end
     end