diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index 79ec1589e..76504a81e 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -64,7 +64,9 @@ def create_asset(reject_conflicting_content: false, **attributes) if scratch_asset.new_record? begin scratch_asset.save! - rescue ActiveRecord::RecordNotUnique + rescue ActiveRecord::RecordNotUnique, ActiveRecord::RecordInvalid => e + raise unless duplicate_filename_error?(e) + logger.info("Scratch asset already created during concurrent upload: #{attributes.fetch(:filename)}") scratch_asset = ScratchAsset.find_by!(attributes) end @@ -79,6 +81,12 @@ def create_asset(reject_conflicting_content: false, **attributes) render json: { status: 'ok', 'content-name': params[:id] }, status: :created end + def duplicate_filename_error?(error) + return true if error.is_a?(ActiveRecord::RecordNotUnique) + + error.record.errors.of_kind?(:filename, :taken) + end + def attach_file_with_conflict_check(scratch_asset, filename) scratch_asset.with_lock do if scratch_asset.file.attached? @@ -93,7 +101,9 @@ def attach_file_with_conflict_check(scratch_asset, filename) end def attach_file_unless_present(scratch_asset, filename) - scratch_asset.file.attach(io: request.body, filename:) unless scratch_asset.file.attached? + scratch_asset.with_lock do + scratch_asset.file.attach(io: request.body, filename:) unless scratch_asset.file.attached? + end end def file_matches?(scratch_asset) diff --git a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb index 9ccb55f3a..e0035a75e 100644 --- a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb +++ b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb @@ -352,30 +352,61 @@ def make_request end it 'remains idempotent when another request creates the same project asset first' do - existing_asset = create_uploaded_scratch_asset(filename:, project:, body: 'winner-body') - racing_asset = ScratchAsset.new(filename:, project:, uploaded_user_id: teacher.id) - - allow(ScratchAsset).to receive(:find_or_initialize_by).and_wrap_original do |original, *args| - attributes = args.first - if attributes[:filename] == filename && - attributes[:project]&.id == project.id && - attributes[:uploaded_user_id] == teacher.id - racing_asset - else - original.call(*args) - end - end - allow(racing_asset).to receive(:save!).and_raise(ActiveRecord::RecordNotUnique) + first_asset = create_uploaded_scratch_asset(filename:, project:, body: 'winner-body') + second_asset = ScratchAsset.new(filename:, project:, uploaded_user_id: teacher.id) + stub_find_or_initialize_scratch_asset(second_asset, filename:, project:, uploaded_user_id: teacher.id) + allow(second_asset).to receive(:save!).and_raise(ActiveRecord::RecordNotUnique) blob_count = ActiveStorage::Blob.count expect { make_request }.not_to change(ScratchAsset, :count) expect(response).to have_http_status(:created) - expect(existing_asset.reload.file.download).to eq('winner-body') + expect(first_asset.reload.file.download).to eq('winner-body') + expect(ActiveStorage::Blob.count).to eq(blob_count) + end + + it 'remains idempotent when another request creates the same project asset first - validated after other commit' do + first_asset = create_uploaded_scratch_asset(filename:, project:, body: 'winner-body') + second_asset = ScratchAsset.new(filename:, project:, uploaded_user_id: teacher.id) + stub_find_or_initialize_scratch_asset(second_asset, filename:, project:, uploaded_user_id: teacher.id) + + blob_count = ActiveStorage::Blob.count + + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(response).to have_http_status(:created) + expect(first_asset.reload.file.download).to eq('winner-body') expect(ActiveStorage::Blob.count).to eq(blob_count) end + it 'does not attach a second file when another request attaches one first' do + existing_asset = ScratchAsset.create!(filename:, project:, uploaded_user_id: teacher.id) + second_asset = ScratchAsset.find(existing_asset.id) + second_asset.file.attached? + stub_find_or_initialize_scratch_asset(second_asset, filename:, project:, uploaded_user_id: teacher.id) + ScratchAsset.find(existing_asset.id).file.attach( + io: StringIO.new('winner-body'), filename:, content_type: 'image/png' + ) + + expect { make_request }.not_to change(ActiveStorage::Blob, :count) + + expect(response).to have_http_status(:created) + expect(ActiveStorage::Attachment.where(record: existing_asset, name: 'file').count).to eq(1) + expect(existing_asset.reload.file.download).to eq('winner-body') + end + + it 'raises any other errors not related to the race condition' do + invalid_asset = ScratchAsset.new(filename:, project:, uploaded_user_id: nil) + stub_find_or_initialize_scratch_asset(invalid_asset, filename:, project:, uploaded_user_id: teacher.id) + allow(Sentry).to receive(:capture_exception) + + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(response).to have_http_status(:internal_server_error) + expect(Sentry).to have_received(:capture_exception).with(an_instance_of(ActiveRecord::RecordInvalid)) + end + context 'when the current project can be viewed but not updated' do let(:student) { create(:student, school:) } let(:teacher_project) do @@ -688,4 +719,17 @@ def create_uploaded_scratch_asset(filename:, project:, body:, uploaded_user_id: asset.file.attach(io: StringIO.new(body), filename:, content_type:) end end + + def stub_find_or_initialize_scratch_asset(replacement, filename:, project:, uploaded_user_id:) + allow(ScratchAsset).to receive(:find_or_initialize_by).and_wrap_original do |original, *args| + attributes = args.first + if attributes[:filename] == filename && + attributes[:project]&.id == project&.id && + attributes[:uploaded_user_id] == uploaded_user_id + replacement + else + original.call(*args) + end + end + end end