Add `RecordsUploads` module to record Upload records via callbacks

7 files changed, 151 insertions, 6 deletions

diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb
index 6aa1f5a8c50..109eb2fea0b 100644
--- a/app/uploaders/attachment_uploader.rb
+++ b/app/uploaders/attachment_uploader.rb
@@ -1,4 +1,5 @@
 class AttachmentUploader < GitlabUploader
+  include RecordsUploads
   include UploaderHelper
 
   storage :file
diff --git a/app/uploaders/avatar_uploader.rb b/app/uploaders/avatar_uploader.rb
index b4c393c6f2c..66d3bcb998a 100644
--- a/app/uploaders/avatar_uploader.rb
+++ b/app/uploaders/avatar_uploader.rb
@@ -1,4 +1,5 @@
 class AvatarUploader < GitlabUploader
+  include RecordsUploads
   include UploaderHelper
 
   storage :file
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index 0d2edaeff3b..2cf97a1b6fd 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -1,5 +1,7 @@
 class FileUploader < GitlabUploader
+  include RecordsUploads
   include UploaderHelper
+
   MARKDOWN_PATTERN = %r{\!?\[.*?\]\(/uploads/(?<secret>[0-9a-f]{32})/(?<file>.*?)\)}
 
   storage :file
@@ -20,6 +22,10 @@ class FileUploader < GitlabUploader
     File.join(base_dir, 'tmp', @project.path_with_namespace, @secret)
   end
 
+  def model
+    project
+  end
+
   def to_markdown
     to_h[:markdown]
   end
diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb
new file mode 100644
index 00000000000..7a0424b5adf
--- /dev/null
+++ b/app/uploaders/records_uploads.rb
@@ -0,0 +1,38 @@
+module RecordsUploads
+  extend ActiveSupport::Concern
+
+  included do
+    after :store,   :record_upload
+    before :remove, :destroy_upload
+  end
+
+  private
+
+  # After storing an attachment, create a corresponding Upload record
+  #
+  # NOTE: We're ignoring the argument passed to this callback because we want
+  # the `SanitizedFile` object from `CarrierWave::Uploader::Base#file`, not the
+  # `Tempfile` object the callback gets.
+  #
+  # Called `after :store`
+  def record_upload(_tempfile)
+    return unless file_storage?
+    return unless file.exists?
+
+    Upload.record(self)
+  end
+
+  # When removing an attachment, destroy any Upload records at the same path
+  #
+  # Note: this _will not work_ for Uploaders which relativize paths, such as
+  # `FileUploader`, but because that uploader uses different paths for every
+  # upload, that's an acceptable caveat.
+  #
+  # Called `before :remove`
+  def destroy_upload(*args)
+    return unless file_storage?
+    return unless file
+
+    Upload.remove_path(relative_path)
+  end
+end
diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb
index b0f5be55c33..396b0abdaef 100644
--- a/spec/uploaders/file_uploader_spec.rb
+++ b/spec/uploaders/file_uploader_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe FileUploader do
-  let(:uploader) { described_class.new(build_stubbed(:project)) }
+  let(:uploader) { described_class.new(build_stubbed(:empty_project)) }
 
   describe 'initialize' do
     it 'generates a secret if none is provided' do
diff --git a/spec/uploaders/records_uploads_spec.rb b/spec/uploaders/records_uploads_spec.rb
new file mode 100644
index 00000000000..a104dc4257f
--- /dev/null
+++ b/spec/uploaders/records_uploads_spec.rb
@@ -0,0 +1,97 @@
+require 'rails_helper'
+
+describe RecordsUploads do
+  let(:uploader) do
+    example_uploader = Class.new(GitlabUploader) do
+      include RecordsUploads
+
+      storage :file
+
+      def model
+        FactoryGirl.build_stubbed(:user)
+      end
+    end
+
+    example_uploader.new
+  end
+
+  def upload_fixture(filename)
+    fixture_file_upload(Rails.root.join('spec', 'fixtures', filename))
+  end
+
+  describe 'callbacks' do
+    it 'calls `record_upload` after `store`' do
+      expect(uploader).to receive(:record_upload).once
+
+      uploader.store!(upload_fixture('doc_sample.txt'))
+    end
+
+    it 'calls `destroy_upload` after `remove`' do
+      expect(uploader).to receive(:destroy_upload).once
+
+      uploader.store!(upload_fixture('doc_sample.txt'))
+
+      uploader.remove!
+    end
+  end
+
+  describe '#record_upload callback' do
+    it 'returns early when not using file storage' do
+      allow(uploader).to receive(:file_storage?).and_return(false)
+      expect(Upload).not_to receive(:record)
+
+      uploader.store!(upload_fixture('rails_sample.jpg'))
+    end
+
+    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)
+
+      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 'it destroys Upload records at the same path before recording' do
+      existing = Upload.create!(
+        path: File.join(CarrierWave.root, 'uploads', 'rails_sample.jpg'),
+        size: 512.kilobytes,
+        model: build_stubbed(:user),
+        uploader: described_class.to_s
+      )
+
+      uploader.store!(upload_fixture('rails_sample.jpg'))
+
+      expect { existing.reload }.to raise_error(ActiveRecord::RecordNotFound)
+      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'))
+
+      expect { uploader.remove! }.to change { Upload.count }.from(1).to(0)
+    end
+  end
+end
diff --git a/spec/uploaders/uploader_helper_spec.rb b/spec/uploaders/uploader_helper_spec.rb
index e9efd13b9aa..c47f09adb6d 100644
--- a/spec/uploaders/uploader_helper_spec.rb
+++ b/spec/uploaders/uploader_helper_spec.rb
@@ -1,10 +1,14 @@
 require 'rails_helper'
 
 describe UploaderHelper do
-  class ExampleUploader < CarrierWave::Uploader::Base
-    include UploaderHelper
+  let(:uploader) do
+    example_uploader = Class.new(CarrierWave::Uploader::Base) do
+      include UploaderHelper
 
-    storage :file
+      storage :file
+    end
+
+    example_uploader.new
   end
 
   def upload_fixture(filename)
@@ -12,8 +16,6 @@ describe UploaderHelper do
   end
 
   describe '#image_or_video?' do
-    let(:uploader) { ExampleUploader.new }
-
     it 'returns true for an image file' do
       uploader.store!(upload_fixture('dk.png'))