Revert "Merge branch '3867-port-to-ce' into 'master'"

This reverts commit 54a575f1bbba44573ab92dc58a4242f1ee734c5d, reversing
changes made to c63af942e5baf7849a94fa99da8494bcba28e3f8.

10 files changed, 294 insertions, 134 deletions

diff --git a/spec/uploaders/attachment_uploader_spec.rb b/spec/uploaders/attachment_uploader_spec.rb
index 091ba824fc6..04ee6e9bfad 100644
--- a/spec/uploaders/attachment_uploader_spec.rb
+++ b/spec/uploaders/attachment_uploader_spec.rb
@@ -1,14 +1,28 @@
 require 'spec_helper'
 
 describe AttachmentUploader do
-  let(:note) { create(:note, :with_attachment) }
-  let(:uploader) { note.attachment }
-  let(:upload) { create(:upload, :attachment_upload, model: uploader.model) }
+  let(:uploader) { described_class.new(build_stubbed(:user)) }
 
-  subject { uploader }
+  describe "#store_dir" do
+    it "stores in the system dir" do
+      expect(uploader.store_dir).to start_with("uploads/-/system/user")
+    end
 
-  it_behaves_like 'builds correct paths',
-                  store_dir: %r[uploads/-/system/note/attachment/],
-                  upload_path: %r[uploads/-/system/note/attachment/],
-                  absolute_path: %r[#{CarrierWave.root}/uploads/-/system/note/attachment/]
+    it "uses the old path when using object storage" do
+      expect(described_class).to receive(:file_storage?).and_return(false)
+      expect(uploader.store_dir).to start_with("uploads/user")
+    end
+  end
+
+  describe '#move_to_cache' do
+    it 'is true' do
+      expect(uploader.move_to_cache).to eq(true)
+    end
+  end
+
+  describe '#move_to_store' do
+    it 'is true' do
+      expect(uploader.move_to_store).to eq(true)
+    end
+  end
 end
diff --git a/spec/uploaders/avatar_uploader_spec.rb b/spec/uploaders/avatar_uploader_spec.rb
index bf9028c9260..1dc574699d8 100644
--- a/spec/uploaders/avatar_uploader_spec.rb
+++ b/spec/uploaders/avatar_uploader_spec.rb
@@ -1,16 +1,18 @@
 require 'spec_helper'
 
 describe AvatarUploader do
-  let(:model) { create(:user, :with_avatar) }
-  let(:uploader) { described_class.new(model, :avatar) }
-  let(:upload) { create(:upload, model: model) }
+  let(:uploader) { described_class.new(build_stubbed(:user)) }
 
-  subject { uploader }
+  describe "#store_dir" do
+    it "stores in the system dir" do
+      expect(uploader.store_dir).to start_with("uploads/-/system/user")
+    end
 
-  it_behaves_like 'builds correct paths',
-                  store_dir: %r[uploads/-/system/user/avatar/],
-                  upload_path: %r[uploads/-/system/user/avatar/],
-                  absolute_path: %r[#{CarrierWave.root}/uploads/-/system/user/avatar/]
+    it "uses the old path when using object storage" do
+      expect(described_class).to receive(:file_storage?).and_return(false)
+      expect(uploader.store_dir).to start_with("uploads/user")
+    end
+  end
 
   describe '#move_to_cache' do
     it 'is false' do
diff --git a/spec/uploaders/file_mover_spec.rb b/spec/uploaders/file_mover_spec.rb
index bc024cd307c..0cf462e9553 100644
--- a/spec/uploaders/file_mover_spec.rb
+++ b/spec/uploaders/file_mover_spec.rb
@@ -3,13 +3,13 @@ require 'spec_helper'
 describe FileMover do
   let(:filename) { 'banana_sample.gif' }
   let(:file) { fixture_file_upload(Rails.root.join('spec', 'fixtures', filename)) }
-  let(:temp_file_path) { File.join('uploads/-/system/temp', 'secret55', filename) }
-
   let(:temp_description) do
-    "test ![banana_sample](/#{temp_file_path}) "\
-    "same ![banana_sample](/#{temp_file_path}) "
+    'test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) same ![banana_sample]'\
+    '(/uploads/-/system/temp/secret55/banana_sample.gif)'
   end
-  let(:file_path) { File.join('uploads/-/system/personal_snippet', snippet.id.to_s, 'secret55', filename) }
+  let(:temp_file_path) { File.join('secret55', filename).to_s }
+  let(:file_path) { File.join('uploads', '-', 'system', 'personal_snippet', snippet.id.to_s, 'secret55', filename).to_s }
+
   let(:snippet) { create(:personal_snippet, description: temp_description) }
 
   subject { described_class.new(file_path, snippet).execute }
@@ -28,8 +28,8 @@ describe FileMover do
 
         expect(snippet.reload.description)
           .to eq(
-            "test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "\
-            "same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif) "
+            "test ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"\
+            " same ![banana_sample](/uploads/-/system/personal_snippet/#{snippet.id}/secret55/banana_sample.gif)"
           )
       end
 
@@ -50,8 +50,8 @@ describe FileMover do
 
         expect(snippet.reload.description)
           .to eq(
-            "test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) "\
-            "same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif) "
+            "test ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif)"\
+            " same ![banana_sample](/uploads/-/system/temp/secret55/banana_sample.gif)"
           )
       end
 
diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb
index a72f853df75..845516e5004 100644
--- a/spec/uploaders/file_uploader_spec.rb
+++ b/spec/uploaders/file_uploader_spec.rb
@@ -1,57 +1,118 @@
 require 'spec_helper'
 
 describe FileUploader do
-  let(:group) { create(:group, name: 'awesome') }
-  let(:project) { create(:project, namespace: group, name: 'project') }
-  let(:uploader) { described_class.new(project) }
-  let(:upload)  { double(model: project, path: 'secret/foo.jpg') }
+  let(:uploader) { described_class.new(build_stubbed(:project)) }
 
-  subject { uploader }
+  context 'legacy storage' do
+    let(:project) { build_stubbed(:project) }
+
+    describe '.absolute_path' do
+      it 'returns the correct absolute path by building it dynamically' do
+        upload = double(model: project, path: 'secret/foo.jpg')
+
+        dynamic_segment = project.full_path
 
-  shared_examples 'builds correct legacy storage paths' do
-    include_examples 'builds correct paths',
-                     store_dir: %r{awesome/project/\h+},
-                     absolute_path: %r{#{described_class.root}/awesome/project/secret/foo.jpg}
+        expect(described_class.absolute_path(upload))
+          .to end_with("#{dynamic_segment}/secret/foo.jpg")
+      end
+    end
+
+    describe "#store_dir" do
+      it "stores in the namespace path" do
+        uploader = described_class.new(project)
+
+        expect(uploader.store_dir).to include(project.full_path)
+        expect(uploader.store_dir).not_to include("system")
+      end
+    end
   end
 
-  shared_examples 'uses hashed storage' do
+  context 'hashed storage' do
     context 'when rolled out attachments' do
-      before do
-        allow(project).to receive(:disk_path).and_return('ca/fe/fe/ed')
+      let(:project) { build_stubbed(:project, :hashed) }
+
+      describe '.absolute_path' do
+        it 'returns the correct absolute path by building it dynamically' do
+          upload = double(model: project, path: 'secret/foo.jpg')
+
+          dynamic_segment = project.disk_path
+
+          expect(described_class.absolute_path(upload))
+            .to end_with("#{dynamic_segment}/secret/foo.jpg")
+        end
       end
 
-      let(:project) { build_stubbed(:project, :hashed, namespace: group, name: 'project') }
+      describe "#store_dir" do
+        it "stores in the namespace path" do
+          uploader = described_class.new(project)
 
-      it_behaves_like 'builds correct paths',
-                      store_dir: %r{ca/fe/fe/ed/\h+},
-                      absolute_path: %r{#{described_class.root}/ca/fe/fe/ed/secret/foo.jpg}
+          expect(uploader.store_dir).to include(project.disk_path)
+          expect(uploader.store_dir).not_to include("system")
+        end
+      end
     end
 
     context 'when only repositories are rolled out' do
-      let(:project) { build_stubbed(:project, namespace: group, name: 'project', storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) }
+      let(:project) { build_stubbed(:project, storage_version: Project::HASHED_STORAGE_FEATURES[:repository]) }
 
-      it_behaves_like 'builds correct legacy storage paths'
-    end
-  end
+      describe '.absolute_path' do
+        it 'returns the correct absolute path by building it dynamically' do
+          upload = double(model: project, path: 'secret/foo.jpg')
 
-  context 'legacy storage' do
-    it_behaves_like 'builds correct legacy storage paths'
-    include_examples 'uses hashed storage'
+          dynamic_segment = project.full_path
+
+          expect(described_class.absolute_path(upload))
+            .to end_with("#{dynamic_segment}/secret/foo.jpg")
+        end
+      end
+
+      describe "#store_dir" do
+        it "stores in the namespace path" do
+          uploader = described_class.new(project)
+
+          expect(uploader.store_dir).to include(project.full_path)
+          expect(uploader.store_dir).not_to include("system")
+        end
+      end
+    end
   end
 
   describe 'initialize' do
-    let(:uploader) { described_class.new(double, 'secret') }
+    it 'generates a secret if none is provided' do
+      expect(SecureRandom).to receive(:hex).and_return('secret')
+
+      uploader = described_class.new(double)
+
+      expect(uploader.secret).to eq 'secret'
+    end
 
     it 'accepts a secret parameter' do
-      expect(described_class).not_to receive(:generate_secret)
-      expect(uploader.secret).to eq('secret')
+      expect(SecureRandom).not_to receive(:hex)
+
+      uploader = described_class.new(double, 'secret')
+
+      expect(uploader.secret).to eq 'secret'
     end
   end
 
-  describe '#secret' do
-    it 'generates a secret if none is provided' do
-      expect(described_class).to receive(:generate_secret).and_return('secret')
-      expect(uploader.secret).to eq('secret')
+  describe '#move_to_cache' do
+    it 'is true' do
+      expect(uploader.move_to_cache).to eq(true)
+    end
+  end
+
+  describe '#move_to_store' do
+    it 'is true' do
+      expect(uploader.move_to_store).to eq(true)
+    end
+  end
+
+  describe '#relative_path' do
+    it 'removes the leading dynamic path segment' do
+      fixture = Rails.root.join('spec', 'fixtures', 'rails_sample.jpg')
+      uploader.store!(fixture_file_upload(fixture))
+
+      expect(uploader.relative_path).to match(%r{\A\h{32}/rails_sample.jpg\z})
     end
   end
 end
diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb
index d606404e95d..a067c3e75f4 100644
--- a/spec/uploaders/job_artifact_uploader_spec.rb
+++ b/spec/uploaders/job_artifact_uploader_spec.rb
@@ -3,13 +3,33 @@ require 'spec_helper'
 describe JobArtifactUploader do
   let(:job_artifact) { create(:ci_job_artifact) }
   let(:uploader) { described_class.new(job_artifact, :file) }
+  let(:local_path) { Gitlab.config.artifacts.path }
 
-  subject { uploader }
+  describe '#store_dir' do
+    subject { uploader.store_dir }
 
-  it_behaves_like "builds correct paths",
-                  store_dir: %r[\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z],
-                  cache_dir: %r[artifacts/tmp/cache],
-                  work_dir: %r[artifacts/tmp/work]
+    let(:path) { "#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/#{job_artifact.job_id}/#{job_artifact.id}" }
+
+    context 'when using local storage' do
+      it { is_expected.to start_with(local_path) }
+      it { is_expected.to match(%r{\h{2}/\h{2}/\h{64}/\d{4}_\d{1,2}_\d{1,2}/\d+/\d+\z}) }
+      it { is_expected.to end_with(path) }
+    end
+  end
+
+  describe '#cache_dir' do
+    subject { uploader.cache_dir }
+
+    it { is_expected.to start_with(local_path) }
+    it { is_expected.to end_with('/tmp/cache') }
+  end
+
+  describe '#work_dir' do
+    subject { uploader.work_dir }
+
+    it { is_expected.to start_with(local_path) }
+    it { is_expected.to end_with('/tmp/work') }
+  end
 
   context 'file is stored in valid local_path' do
     let(:file) do
@@ -23,7 +43,7 @@ describe JobArtifactUploader do
 
     subject { uploader.file.path }
 
-    it { is_expected.to start_with("#{uploader.root}/#{uploader.class.base_dir}") }
+    it { is_expected.to start_with(local_path) }
     it { is_expected.to include("/#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/") }
     it { is_expected.to include("/#{job_artifact.job_id}/#{job_artifact.id}/") }
     it { is_expected.to end_with("ci_build_artifacts.zip") }
diff --git a/spec/uploaders/legacy_artifact_uploader_spec.rb b/spec/uploaders/legacy_artifact_uploader_spec.rb
index 54c6a8b869b..efeffb78772 100644
--- a/spec/uploaders/legacy_artifact_uploader_spec.rb
+++ b/spec/uploaders/legacy_artifact_uploader_spec.rb
@@ -3,22 +3,49 @@ require 'rails_helper'
 describe LegacyArtifactUploader do
   let(:job) { create(:ci_build) }
   let(:uploader) { described_class.new(job, :legacy_artifacts_file) }
-  let(:local_path) { described_class.root }
+  let(:local_path) { Gitlab.config.artifacts.path }
 
-  subject { uploader }
+  describe '.local_store_path' do
+    subject { described_class.local_store_path }
 
-  # TODO: move to Workhorse::UploadPath
-  describe '.workhorse_upload_path' do
-    subject { described_class.workhorse_upload_path }
+    it "delegate to artifacts path" do
+      expect(Gitlab.config.artifacts).to receive(:path)
+
+      subject
+    end
+  end
+
+  describe '.artifacts_upload_path' do
+    subject { described_class.artifacts_upload_path }
 
     it { is_expected.to start_with(local_path) }
-    it { is_expected.to end_with('tmp/uploads') }
+    it { is_expected.to end_with('tmp/uploads/') }
+  end
+
+  describe '#store_dir' do
+    subject { uploader.store_dir }
+
+    let(:path) { "#{job.created_at.utc.strftime('%Y_%m')}/#{job.project_id}/#{job.id}" }
+
+    context 'when using local storage' do
+      it { is_expected.to start_with(local_path) }
+      it { is_expected.to end_with(path) }
+    end
   end
 
-  it_behaves_like "builds correct paths",
-                  store_dir: %r[\d{4}_\d{1,2}/\d+/\d+\z],
-                  cache_dir: %r[artifacts/tmp/cache],
-                  work_dir: %r[artifacts/tmp/work]
+  describe '#cache_dir' do
+    subject { uploader.cache_dir }
+
+    it { is_expected.to start_with(local_path) }
+    it { is_expected.to end_with('/tmp/cache') }
+  end
+
+  describe '#work_dir' do
+    subject { uploader.work_dir }
+
+    it { is_expected.to start_with(local_path) }
+    it { is_expected.to end_with('/tmp/work') }
+  end
 
   describe '#filename' do
     # we need to use uploader, as this makes to use mounter
@@ -42,7 +69,7 @@ describe LegacyArtifactUploader do
 
     subject { uploader.file.path }
 
-    it { is_expected.to start_with("#{uploader.root}") }
+    it { is_expected.to start_with(local_path) }
     it { is_expected.to include("/#{job.created_at.utc.strftime('%Y_%m')}/") }
     it { is_expected.to include("/#{job.project_id}/") }
     it { is_expected.to end_with("ci_build_artifacts.zip") }
diff --git a/spec/uploaders/lfs_object_uploader_spec.rb b/spec/uploaders/lfs_object_uploader_spec.rb
index 6ebc885daa8..7088bc23334 100644
--- a/spec/uploaders/lfs_object_uploader_spec.rb
+++ b/spec/uploaders/lfs_object_uploader_spec.rb
@@ -2,13 +2,39 @@ require 'spec_helper'
 
 describe LfsObjectUploader do
   let(:lfs_object) { create(:lfs_object, :with_file) }
-  let(:uploader) { described_class.new(lfs_object, :file) }
+  let(:uploader) { described_class.new(lfs_object) }
   let(:path) { Gitlab.config.lfs.storage_path }
 
-  subject { uploader }
+  describe '#move_to_cache' do
+    it 'is true' do
+      expect(uploader.move_to_cache).to eq(true)
+    end
+  end
 
-  it_behaves_like "builds correct paths",
-                  store_dir: %r[\h{2}/\h{2}],
-                  cache_dir: %r[/lfs-objects/tmp/cache],
-                  work_dir: %r[/lfs-objects/tmp/work]
+  describe '#move_to_store' do
+    it 'is true' do
+      expect(uploader.move_to_store).to eq(true)
+    end
+  end
+
+  describe '#store_dir' do
+    subject { uploader.store_dir }
+
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with("#{lfs_object.oid[0, 2]}/#{lfs_object.oid[2, 2]}") }
+  end
+
+  describe '#cache_dir' do
+    subject { uploader.cache_dir }
+
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with('/tmp/cache') }
+  end
+
+  describe '#work_dir' do
+    subject { uploader.work_dir }
+
+    it { is_expected.to start_with(path) }
+    it { is_expected.to end_with('/tmp/work') }
+  end
 end
diff --git a/spec/uploaders/namespace_file_uploader_spec.rb b/spec/uploaders/namespace_file_uploader_spec.rb
index 24a2fc0f72e..c6c4500c179 100644
--- a/spec/uploaders/namespace_file_uploader_spec.rb
+++ b/spec/uploaders/namespace_file_uploader_spec.rb
@@ -1,16 +1,21 @@
 require 'spec_helper'
 
-IDENTIFIER = %r{\h+/\S+}
-
 describe NamespaceFileUploader do
   let(:group) { build_stubbed(:group) }
   let(:uploader) { described_class.new(group) }
-  let(:upload) { create(:upload, :namespace_upload, model: group) }
 
-  subject { uploader }
+  describe "#store_dir" do
+    it "stores in the namespace id directory" do
+      expect(uploader.store_dir).to include(group.id.to_s)
+    end
+  end
+
+  describe ".absolute_path" do
+    it "stores in thecorrect directory" do
+      upload_record = create(:upload, :namespace_upload, model: group)
 
-  it_behaves_like 'builds correct paths',
-                  store_dir: %r[uploads/-/system/namespace/\d+],
-                  upload_path: IDENTIFIER,
-                  absolute_path: %r[#{CarrierWave.root}/uploads/-/system/namespace/\d+/#{IDENTIFIER}]
+      expect(described_class.absolute_path(upload_record))
+        .to include("-/system/namespace/#{group.id}")
+    end
+  end
 end
diff --git a/spec/uploaders/personal_file_uploader_spec.rb b/spec/uploaders/personal_file_uploader_spec.rb
index ed1fba6edda..cbafa9f478d 100644
--- a/spec/uploaders/personal_file_uploader_spec.rb
+++ b/spec/uploaders/personal_file_uploader_spec.rb
@@ -1,27 +1,25 @@
 require 'spec_helper'
 
-IDENTIFIER = %r{\h+/\S+}
-
 describe PersonalFileUploader do
-  let(:model) { create(:personal_snippet) }
-  let(:uploader) { described_class.new(model) }
-  let(:upload) { create(:upload, :personal_snippet_upload) }
+  let(:uploader) { described_class.new(build_stubbed(:project)) }
+  let(:snippet) { create(:personal_snippet) }
 
-  subject { uploader }
+  describe '.absolute_path' do
+    it 'returns the correct absolute path by building it dynamically' do
+      upload = double(model: snippet, path: 'secret/foo.jpg')
 
-  it_behaves_like 'builds correct paths',
-                  store_dir: %r[uploads/-/system/personal_snippet/\d+],
-                  upload_path: IDENTIFIER,
-                  absolute_path: %r[#{CarrierWave.root}/uploads/-/system/personal_snippet/\d+/#{IDENTIFIER}]
+      dynamic_segment = "personal_snippet/#{snippet.id}"
 
-  describe '#to_h' do
-    before do
-      subject.instance_variable_set(:@secret, 'secret')
+      expect(described_class.absolute_path(upload)).to end_with("/-/system/#{dynamic_segment}/secret/foo.jpg")
     end
+  end
+
+  describe '#to_h' do
+    it 'returns the hass' do
+      uploader = described_class.new(snippet, 'secret')
 
-    it 'is correct' do
       allow(uploader).to receive(:file).and_return(double(extension: 'txt', filename: 'file_name'))
-      expected_url = "/uploads/-/system/personal_snippet/#{model.id}/secret/file_name"
+      expected_url = "/uploads/-/system/personal_snippet/#{snippet.id}/secret/file_name"
 
       expect(uploader.to_h).to eq(
         alt: 'file_name',
diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb
index 9a3e5d83e01..7ef7fb7d758 100644
--- a/spec/uploaders/records_uploads_spec.rb
+++ b/spec/uploaders/records_uploads_spec.rb
@@ -3,16 +3,16 @@ require 'rails_helper'
 describe RecordsUploads do
   let!(:uploader) do
     class RecordsUploadsExampleUploader < GitlabUploader
-      include RecordsUploads::Concern
+      include RecordsUploads
 
       storage :file
 
-      def dynamic_segment
-        'co/fe/ee'
+      def model
+        FactoryBot.build_stubbed(:user)
       end
     end
 
-    RecordsUploadsExampleUploader.new(build_stubbed(:user))
+    RecordsUploadsExampleUploader.new
   end
 
   def upload_fixture(filename)
@@ -20,55 +20,48 @@ describe RecordsUploads do
   end
 
   describe 'callbacks' do
-    let(:upload) { create(:upload) }
-
-    before do
-      uploader.upload = upload
-    end
-
-    it '#record_upload after `store`' do
+    it 'calls `record_upload` after `store`' do
       expect(uploader).to receive(:record_upload).once
 
       uploader.store!(upload_fixture('doc_sample.txt'))
     end
 
-    it '#destroy_upload after `remove`' do
+    it 'calls `destroy_upload` after `remove`' do
+      expect(uploader).to receive(:destroy_upload).once
+
       uploader.store!(upload_fixture('doc_sample.txt'))
 
-      expect(uploader).to receive(:destroy_upload).once
       uploader.remove!
     end
   end
 
   describe '#record_upload callback' do
-    it 'creates an Upload record after store' do
-      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.to change { Upload.count }.by(1)
-    end
+    it 'returns early when not using file storage' do
+      allow(uploader).to receive(:file_storage?).and_return(false)
+      expect(Upload).not_to receive(:record)
 
-    it 'creates a new record and assigns size, path, model, and uploader' do
       uploader.store!(upload_fixture('rails_sample.jpg'))
-
-      upload = uploader.upload
-      aggregate_failures do
-        expect(upload).to be_persisted
-        expect(upload.size).to eq uploader.file.size
-        expect(upload.path).to eq uploader.upload_path
-        expect(upload.model_id).to eq uploader.model.id
-        expect(upload.model_type).to eq uploader.model.class.to_s
-        expect(upload.uploader).to eq uploader.class.to_s
-      end
     end
 
-    it "does not create an Upload record when the file doesn't exist" do
+    it "returns early when the file doesn't exist" do
       allow(uploader).to receive(:file).and_return(double(exists?: false))
+      expect(Upload).not_to receive(:record)
 
-      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count }
+      uploader.store!(upload_fixture('rails_sample.jpg'))
+    end
+
+    it 'creates an Upload record after store' do
+      expect(Upload).to receive(:record)
+        .with(uploader)
+
+      uploader.store!(upload_fixture('rails_sample.jpg'))
     end
 
     it 'does not create an Upload record if model is missing' do
-      allow_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil)
+      expect_any_instance_of(RecordsUploadsExampleUploader).to receive(:model).and_return(nil)
+      expect(Upload).not_to receive(:record).with(uploader)
 
-      expect { uploader.store!(upload_fixture('rails_sample.jpg')) }.not_to change { Upload.count }
+      uploader.store!(upload_fixture('rails_sample.jpg'))
     end
 
     it 'it destroys Upload records at the same path before recording' do
@@ -79,15 +72,29 @@ describe RecordsUploads do
         uploader: uploader.class.to_s
       )
 
-      uploader.upload = existing
       uploader.store!(upload_fixture('rails_sample.jpg'))
 
       expect { existing.reload }.to raise_error(ActiveRecord::RecordNotFound)
-      expect(Upload.count).to eq(1)
+      expect(Upload.count).to eq 1
     end
   end
 
   describe '#destroy_upload callback' do
+    it 'returns early when not using file storage' do
+      uploader.store!(upload_fixture('rails_sample.jpg'))
+
+      allow(uploader).to receive(:file_storage?).and_return(false)
+      expect(Upload).not_to receive(:remove_path)
+
+      uploader.remove!
+    end
+
+    it 'returns early when file is nil' do
+      expect(Upload).not_to receive(:remove_path)
+
+      uploader.remove!
+    end
+
     it 'it destroys Upload records at the same path after removal' do
       uploader.store!(upload_fixture('rails_sample.jpg'))