Fix data inconsistency issue for old artifacts by moving them to a currently used path

6 files changed, 27 insertions, 53 deletions

diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb
index ea036b1f705..b400670746f 100644
--- a/app/controllers/projects/artifacts_controller.rb
+++ b/app/controllers/projects/artifacts_controller.rb
@@ -10,7 +10,7 @@ class Projects::ArtifactsController < Projects::ApplicationController
   before_action :set_path_and_entry, only: [:file, :raw]
 
   def download
-    if artifacts_file.file_storage?
+    if artifacts_file.local_file?
       send_file artifacts_file.path, disposition: 'attachment'
     else
       redirect_to artifacts_file.url
diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb
index eef53730291..7493bf38bba 100644
--- a/app/controllers/uploads_controller.rb
+++ b/app/controllers/uploads_controller.rb
@@ -80,7 +80,7 @@ class UploadsController < ApplicationController
     else
       @uploader = @model.send(upload_mount)
 
-      redirect_to @uploader.url unless @uploader.file_storage?
+      redirect_to @uploader.url unless @uploader.local_storage?
     end
 
     @uploader
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 60b71ff0d93..63b72b8e6a9 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -255,38 +255,6 @@ module Ci
       Time.now - updated_at > 15.minutes.to_i
     end
 
-    ##
-    # Deprecated
-    #
-    # This contains a hotfix for CI build data integrity, see #4246
-    #
-    # This method is used by `ArtifactUploader` to create a store_dir.
-    # Warning: Uploader uses it after AND before file has been stored.
-    #
-    # This method returns old path to artifacts only if it already exists.
-    #
-    def artifacts_path
-      # We need the project even if it's soft deleted, because whenever
-      # we're really deleting the project, we'll also delete the builds,
-      # and in order to delete the builds, we need to know where to find
-      # the artifacts, which is depending on the data of the project.
-      # We need to retain the project in this case.
-      the_project = project || unscoped_project
-
-      old = File.join(created_at.utc.strftime('%Y_%m'),
-                      the_project.ci_id.to_s,
-                      id.to_s)
-
-      old_store = File.join(ArtifactUploader.artifacts_path, old)
-      return old if the_project.ci_id && File.directory?(old_store)
-
-      File.join(
-        created_at.utc.strftime('%Y_%m'),
-        the_project.id.to_s,
-        id.to_s
-      )
-    end
-
     def valid_token?(token)
       self.token && ActiveSupport::SecurityUtils.variable_size_secure_compare(token, self.token)
     end
diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb
index 3e36ec91205..668b6b8e048 100644
--- a/app/uploaders/artifact_uploader.rb
+++ b/app/uploaders/artifact_uploader.rb
@@ -1,33 +1,31 @@
 class ArtifactUploader < GitlabUploader
   storage :file
 
-  attr_accessor :build, :field
+  attr_reader :job, :field
 
-  def self.artifacts_path
+  def self.local_artifacts_store
     Gitlab.config.artifacts.path
   end
 
-  def self.artifacts_upload_path
-    File.join(self.artifacts_path, 'tmp/uploads/')
+  def initialize(job, field)
+    @job, @field = job, field
   end
 
-  def self.artifacts_cache_path
-    File.join(self.artifacts_path, 'tmp/cache/')
+  def store_dir
+    default_local_path
   end
 
-  def initialize(build, field)
-    @build, @field = build, field
+  def cache_dir
+    File.join(self.class.local_artifacts_store, 'tmp/cache')
   end
 
-  def store_dir
-    File.join(self.class.artifacts_path, @build.artifacts_path)
-  end
+  private
 
-  def cache_dir
-    File.join(self.class.artifacts_cache_path, @build.artifacts_path)
+  def default_local_path
+    File.join(self.class.local_artifacts_store, default_path)
   end
 
-  def filename
-    file.try(:filename)
+  def default_path
+    File.join(job.created_at.utc.strftime('%Y_%m'), job.project_id.to_s, job.id.to_s)
   end
 end
diff --git a/app/uploaders/gitlab_uploader.rb b/app/uploaders/gitlab_uploader.rb
index e0a6c9b4067..c1d9007dc71 100644
--- a/app/uploaders/gitlab_uploader.rb
+++ b/app/uploaders/gitlab_uploader.rb
@@ -9,8 +9,16 @@ class GitlabUploader < CarrierWave::Uploader::Base
 
   delegate :base_dir, to: :class
 
-  def file_storage?
-    self.class.storage == CarrierWave::Storage::File
+  def local_file?
+    local_storage? && file&.is_a?(CarrierWave::SanitizedFile)
+  end
+
+  def local_storage?
+    storage.is_a?(CarrierWave::Storage::File)
+  end
+
+  def local_cache_storage?
+    cache_storage.is_a?(CarrierWave::Storage::File)
   end
 
   # Reduce disk IO
diff --git a/app/uploaders/records_uploads.rb b/app/uploaders/records_uploads.rb
index 4c127f29250..be3a2caa60c 100644
--- a/app/uploaders/records_uploads.rb
+++ b/app/uploaders/records_uploads.rb
@@ -16,7 +16,7 @@ module RecordsUploads
   #
   # Called `after :store`
   def record_upload(_tempfile)
-    return unless file_storage?
+    return unless local_file?
     return unless file.exists?
 
     Upload.record(self)
@@ -26,7 +26,7 @@ module RecordsUploads
   #
   # Called `before :remove`
   def destroy_upload(*args)
-    return unless file_storage?
+    return unless local_file?
     return unless file
 
     Upload.remove_path(relative_path)