fix the artifact/lfs specs

app/controllers/concerns/uploads_actions.rb

@@ -20,7 +20,6 @@ module UploadsActions
 
   # This should either find the @file and redirect to its URL
   def show
-    binding.pry
     return render_404 unless uploader.exists?
 
     # send to the remote URL
@@ -29,6 +28,7 @@ module UploadsActions
     # or send the file
     disposition = uploader.image_or_video? ? 'inline' : 'attachment'
     expires_in 0.seconds, must_revalidate: true, private: true
+    binding.pry
     send_file uploader.file.path, disposition: disposition
   end
 

app/controllers/projects/uploads_controller.rb

 class Projects::UploadsController < Projects::ApplicationController
   include UploadsActions
 
+  # These will kick you out if you don't have access.
   skip_before_action :project, :repository,
     if: -> { action_name == 'show' && image_or_video? }
 
@@ -8,14 +9,24 @@ class Projects::UploadsController < Projects::ApplicationController
 
   private
 
-  def show_model
-    strong_memoize(:show_model) do
-      namespace = params[:namespace_id]
-      id = params[:project_id]
+  def upload_model_class
+    Project
+  end
+
+  def uploader_class
+    FileUploader
+  end
 
-      Project.find_by_full_path("#{namespace}/#{id}")
-    end
+  def find_model
+    return @project if @project
+
+    namespace = params[:namespace_id]
+    id = params[:project_id]
+
+    Project.find_by_full_path("#{namespace}/#{id}")
   end
 
-  alias_method :model, :project
+  def model
+    @model ||= @project || find_model
+  end
 end

app/models/appearance.rb

@@ -45,6 +45,6 @@ class Appearance < ActiveRecord::Base
     return unless identifier
 
     paths = uploader.store_dirs.map { |store, path| File.join(path, identifier) }
-    uploads.where(uploader: uploader.class.to_s, paths: paths)&.last
+    uploads.where(uploader: uploader.class.to_s, path: paths)&.last
   end
 end

app/models/upload.rb

@@ -9,8 +9,8 @@ class Upload < ActiveRecord::Base
   validates :model, presence: true
   validates :uploader, presence: true
 
-  before_save  :calculate_checksum!, if:     :foreground_checksum?
-  after_commit :schedule_checksum,   unless: :foreground_checksum?
+  before_save  :calculate_checksum!, if: :foreground_checksumable?
+  after_commit :schedule_checksum,   if: :checksumable?
 
   def self.remove_path(path)
     where(path: path).destroy_all
@@ -38,9 +38,7 @@ class Upload < ActiveRecord::Base
 
   def calculate_checksum!
     self.checksum = nil
-
-    return unless local?
-    return unless exist?
+    return unless checksumable?
 
     self.checksum = Digest::SHA256.file(absolute_path).hexdigest
   end
@@ -57,14 +55,18 @@ class Upload < ActiveRecord::Base
 
   private
 
+  def checksumable?
+    checksum.nil? && local? && exist?
+  end
+
   def local?
     return true if store.nil?
 
     store == ObjectStorage::Store::LOCAL
   end
 
-  def foreground_checksum?
-    size <= CHECKSUM_THRESHOLD
+  def foreground_checksumable?
+    checksumable? && size <= CHECKSUM_THRESHOLD
   end
 
   def schedule_checksum

app/uploaders/file_mover.rb

@@ -21,7 +21,6 @@ class FileMover
   end
 
   def update_markdown
-    binding.pry
     updated_text = model.read_attribute(update_field)
                         .gsub(temp_file_uploader.markdown_link, uploader.markdown_link)
     model.update_attribute(update_field, updated_text)

app/uploaders/file_uploader.rb

@@ -72,6 +72,10 @@ class FileUploader < GitlabUploader
     File.join(secret, identifier)
   end
 
+  def self.generate_secret
+    SecureRandom.hex
+  end
+
   def initialize(model, secret = nil)
     @model = model
     @secret = secret
@@ -131,7 +135,7 @@ class FileUploader < GitlabUploader
   end
 
   def secret
-    @secret ||= generate_secret
+    @secret ||= self.class.generate_secret
   end
 
   private
@@ -148,10 +152,6 @@ class FileUploader < GitlabUploader
     secret
   end
 
-  def generate_secret
-    SecureRandom.hex
-  end
-
   def secure_url
     File.join('/uploads', @secret, file.filename)
   end

app/uploaders/gitlab_uploader.rb

@@ -12,7 +12,7 @@ class GitlabUploader < CarrierWave::Uploader::Base
 
     # represent the directory namespacing at the class level
     def base_dir
-      storage_options&.base_dir || ""
+      storage_options.fetch('base_dir', '')
     end
 
     def file_storage?

app/uploaders/job_artifact_uploader.rb

@@ -10,6 +10,10 @@ class JobArtifactUploader < GitlabUploader
     model.size
   end
 
+  def store_dir
+    dynamic_segment
+  end
+
   private
 
   def dynamic_segment

app/uploaders/legacy_artifact_uploader.rb

@@ -4,6 +4,10 @@ class LegacyArtifactUploader < GitlabUploader
 
   storage_options Gitlab.config.artifacts
 
+  def store_dir
+    dynamic_segment
+  end
+
   private
 
   def dynamic_segment

app/uploaders/lfs_object_uploader.rb

@@ -4,6 +4,10 @@ class LfsObjectUploader < GitlabUploader
 
   storage_options Gitlab.config.lfs
 
+  def store_dir
+    dynamic_segment
+  end
+
   def filename
     model.oid[4..-1]
   end

config/initializers/1_settings.rb

@@ -392,7 +392,6 @@ Settings['lfs'] ||= Settingslogic.new({})
 Settings.lfs['enabled']      = true if Settings.lfs['enabled'].nil?
 Settings.lfs['storage_path'] = Settings.absolute(Settings.lfs['storage_path'] || File.join(Settings.shared['path'], "lfs-objects"))
 Settings.lfs['object_store'] ||= Settingslogic.new({}).tap do |object_store|
-  binding.pry
   object_store['enabled']           ||= false
   object_store['remote_directory']  ||= nil
   object_store['background_upload'] ||= true

ee/app/uploaders/object_storage.rb

@@ -31,6 +31,10 @@ module ObjectStorage
         self.upload = model&.try(:"#{mounted_as}_upload", self) if mounted_as
       end
 
+      def record_upload(_tempfile = nil)
+        self.upload = super
+      end
+
       def upload=(upload)
         return unless upload
 

spec/controllers/uploads_controller_spec.rb

@@ -65,7 +65,6 @@ describe UploadsController do
 
         it 'creates a corresponding Upload record' do
           upload = Upload.last
-          binding.pry
 
           aggregate_failures do
             expect(upload).to exist
@@ -213,9 +212,7 @@ describe UploadsController do
 
         context "when not signed in" do
           it "responds with status 200" do
-            binding.pry
             get :show, model: "project", mounted_as: "avatar", id: project.id, filename: "image.png"
-
             expect(response).to have_gitlab_http_status(200)
           end
 

spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb

@@ -2,6 +2,7 @@ shared_examples 'handle uploads' do
   let(:user)  { create(:user) }
   let(:jpg)   { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }
   let(:txt)   { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }
+  let(:secret) { FileUploader.generate_secret }
 
   describe "POST #create" do
     context 'when a user is not authorized to upload a file' do
@@ -65,7 +66,12 @@ shared_examples 'handle uploads' do
 
   describe "GET #show" do
     let(:show_upload) do
-      get :show, params.merge(secret: "123456", filename: "image.jpg")
+      get :show, params.merge(secret: secret, filename: "rails_sample.jpg")
+    end
+
+    before do
+      expect(FileUploader).to receive(:generate_secret).and_return(secret)
+      UploadService.new(model, jpg).execute
     end
 
     context "when the model is public" do
@@ -75,11 +81,6 @@ shared_examples 'handle uploads' do
 
       context "when not signed in" do
         context "when the file exists" do
-          before do
-            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-            allow(jpg).to receive(:exists?).and_return(true)
-          end
-
           it "responds with status 200" do
             show_upload
 
@@ -88,6 +89,10 @@ shared_examples 'handle uploads' do
         end
 
         context "when the file doesn't exist" do
+          before do
+            allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+          end
+
           it "responds with status 404" do
             show_upload
 
@@ -102,11 +107,6 @@ shared_examples 'handle uploads' do
         end
 
         context "when the file exists" do
-          before do
-            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-            allow(jpg).to receive(:exists?).and_return(true)
-          end
-
           it "responds with status 200" do
             show_upload
 
@@ -115,6 +115,10 @@ shared_examples 'handle uploads' do
         end
 
         context "when the file doesn't exist" do
+          before do
+            allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+          end
+
           it "responds with status 404" do
             show_upload
 
@@ -131,11 +135,6 @@ shared_examples 'handle uploads' do
 
       context "when not signed in" do
         context "when the file exists" do
-          before do
-            allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-            allow(jpg).to receive(:exists?).and_return(true)
-          end
-
           context "when the file is an image" do
             before do
               allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
@@ -149,6 +148,10 @@ shared_examples 'handle uploads' do
           end
 
           context "when the file is not an image" do
+            before do
+              allow_any_instance_of(FileUploader).to receive(:image?).and_return(false)
+            end
+
             it "redirects to the sign in page" do
               show_upload
 
@@ -158,6 +161,10 @@ shared_examples 'handle uploads' do
         end
 
         context "when the file doesn't exist" do
+          before do
+            allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+          end
+
           it "redirects to the sign in page" do
             show_upload
 
@@ -177,11 +184,6 @@ shared_examples 'handle uploads' do
           end
 
           context "when the file exists" do
-            before do
-              allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-              allow(jpg).to receive(:exists?).and_return(true)
-            end
-
             it "responds with status 200" do
               show_upload
 
@@ -190,6 +192,10 @@ shared_examples 'handle uploads' do
           end
 
           context "when the file doesn't exist" do
+            before do
+              allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+            end
+
             it "responds with status 404" do
               show_upload
 
@@ -200,11 +206,6 @@ shared_examples 'handle uploads' do
 
         context "when the user doesn't have access to the model" do
           context "when the file exists" do
-            before do
-              allow_any_instance_of(FileUploader).to receive(:file).and_return(jpg)
-              allow(jpg).to receive(:exists?).and_return(true)
-            end
-
             context "when the file is an image" do
               before do
                 allow_any_instance_of(FileUploader).to receive(:image?).and_return(true)
@@ -218,6 +219,10 @@ shared_examples 'handle uploads' do
             end
 
             context "when the file is not an image" do
+              before do
+                allow_any_instance_of(FileUploader).to receive(:image?).and_return(false)
+              end
+
               it "responds with status 404" do
                 show_upload
 
@@ -227,6 +232,10 @@ shared_examples 'handle uploads' do
           end
 
           context "when the file doesn't exist" do
+            before do
+              allow_any_instance_of(FileUploader).to receive(:exists?).and_return(false)
+            end
+
             it "responds with status 404" do
               show_upload
 

spec/uploaders/file_mover_spec.rb

@@ -24,7 +24,6 @@ describe FileMover do
 
     context 'when move and field update successful' do
       it 'updates the description correctly' do
-        binding.pry
         subject
 
         expect(snippet.reload.description)

spec/uploaders/file_uploader_spec.rb

@@ -54,14 +54,14 @@ describe FileUploader do
     let(:uploader) { described_class.new(double, 'secret') }
 
     it 'accepts a secret parameter' do
-      expect(uploader).not_to receive(:generate_secret)
+      expect(described_class).not_to receive(:generate_secret)
       expect(uploader.secret).to eq('secret')
     end
   end
 
   describe '#secret' do
     it 'generates a secret if none is provided' do
-      expect(uploader).to receive(:generate_secret).and_return('secret')
+      expect(described_class).to receive(:generate_secret).and_return('secret')
       expect(uploader.secret).to eq('secret')
     end
   end

spec/uploaders/job_artifact_uploader_spec.rb

@@ -8,7 +8,6 @@ describe JobArtifactUploader do
   subject { uploader }
 
   it_behaves_like "builds correct paths",
-                  base_dir: %r[artifacts],
                   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]
@@ -36,7 +35,7 @@ describe JobArtifactUploader do
 
     subject { uploader.file.path }
 
-    it { is_expected.to start_with("#{uploader.root}/artifacts") }
+    it { is_expected.to start_with("#{uploader.root}") }
     it { is_expected.to include("/#{job_artifact.created_at.utc.strftime('%Y_%m_%d')}/") }
     it { is_expected.to include("/#{job_artifact.project_id}/") }
     it { is_expected.to end_with("ci_build_artifacts.zip") }

spec/uploaders/legacy_artifact_uploader_spec.rb

@@ -17,7 +17,6 @@ describe LegacyArtifactUploader do
   end
 
   it_behaves_like "builds correct paths",
-                  base_dir: %r[artifacts],
                   store_dir: %r[\d{4}_\d{1,2}/\d+/\d+\z],
                   cache_dir: %r[artifacts/tmp/cache],
                   work_dir: %r[artifacts/tmp/work]
@@ -55,7 +54,7 @@ describe LegacyArtifactUploader do
 
     subject { uploader.file.path }
 
-    it { is_expected.to start_with("#{uploader.root}/artifacts") }
+    it { is_expected.to start_with("#{uploader.root}") }
     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") }

spec/uploaders/lfs_object_uploader_spec.rb

@@ -20,7 +20,6 @@ describe LfsObjectUploader do
   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
 

spec/uploaders/object_storage_spec.rb

@@ -125,7 +125,7 @@ describe ObjectStorage do
 
     describe 'delegates the object_store logic to the `Upload` model' do
       it 'call the <mounted_as>_uploader hook' do
-        expect(object).to receive(:avatar_uploader)
+        expect(object).to receive(:avatar_upload)
 
         expect(uploader).to be
       end

spec/workers/upload_checksum_worker_spec.rb

@@ -11,11 +11,11 @@ describe UploadChecksumWorker do
     end
 
     context 'with a valid record' do
-      let(:upload) { create(:upload) }
+      let(:upload) { create(:user, :with_avatar).avatar.upload }
 
       before do
         expect(Upload).to receive(:find).and_return(upload)
-        expect(upload).to receive(:foreground_checksum?).and_return(false)
+        allow(upload).to receive(:foreground_checksumable?).and_return(false)
       end
 
       it 'calls calculate_checksum!' do