From 61864a5a5bb523953589c9398a431c4369fbfc76 Mon Sep 17 00:00:00 2001
From: Zeger-Jan van de Weg <git@zjvandeweg.nl>
Date: Thu, 21 Sep 2017 10:34:12 +0200
Subject: Rename Artifact to JobArtifact, split metadata out

Two things at ones, as there was no clean way to seperate the commit and
give me feedback from the tests.

But the model Artifact is now JobArtifact, and the table does not have a
type anymore, but the metadata is now its own model:
Ci::JobArtifactMetadata.
---
 app/models/ci/artifact.rb                  | 24 ---------------------
 app/models/ci/build.rb                     | 24 +++++++++++++--------
 app/models/ci/job_artifact.rb              | 26 +++++++++++++++++++++++
 app/models/concerns/artifact_migratable.rb | 30 ++++++++++++++++----------
 app/models/project_statistics.rb           |  5 ++++-
 app/uploaders/artifact_uploader.rb         |  8 +++----
 app/uploaders/job_artifact_uploader.rb     | 34 ++++++++++++++++++++++++++++++
 7 files changed, 101 insertions(+), 50 deletions(-)
 delete mode 100644 app/models/ci/artifact.rb
 create mode 100644 app/models/ci/job_artifact.rb
 create mode 100644 app/uploaders/job_artifact_uploader.rb

(limited to 'app')

diff --git a/app/models/ci/artifact.rb b/app/models/ci/artifact.rb
deleted file mode 100644
index 858609883ce..00000000000
--- a/app/models/ci/artifact.rb
+++ /dev/null
@@ -1,24 +0,0 @@
-module Ci
-  class Artifact < ActiveRecord::Base
-    extend Gitlab::Ci::Model
-
-    belongs_to :project
-    belongs_to :build, class_name: "Ci::Build", foreign_key: :ci_build_id
-
-    before_save :set_size, if: :file_changed?
-
-    mount_uploader :file, ArtifactUploader
-
-    enum type: { archive: 0, metadata: 1 }
-
-    # Allow us to use `type` as column name, without Rails thinking we're using
-    # STI: https://stackoverflow.com/a/29663933
-    def self.inheritance_column
-      nil
-    end
-
-    def set_size
-      self.size = file.exists? ? file.size : 0
-    end
-  end
-end
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index fae2f5590b4..6d0ebd7f932 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -11,10 +11,15 @@ module Ci
     belongs_to :erased_by, class_name: 'User'
 
     has_many :deployments, as: :deployable
-    has_many :artifacts, class_name: 'Ci::Artifact', foreign_key: :ci_build_id
+
     has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment'
     has_many :trace_sections, class_name: 'Ci::BuildTraceSection'
 
+    has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id
+    has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id
+    has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id
+
+
     # The "environment" field for builds is a String, and is the unexpanded name
     def persisted_environment
       @persisted_environment ||= Environment.find_by(
@@ -33,7 +38,9 @@ module Ci
 
     scope :unstarted, ->() { where(runner_id: nil) }
     scope :ignore_failures, ->() { where(allow_failure: false) }
-    scope :with_artifacts, ->() { where.not(artifacts_file: [nil, '']) }
+    scope :with_artifacts, ->() do
+      where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.ci_job_id'))
+    end
     scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) }
     scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) }
     scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) }
@@ -423,7 +430,7 @@ module Ci
       Gitlab::Ci::Build::Image.from_services(self)
     end
 
-    def artifacts_options
+    def artifacts
       [options[:artifacts]]
     end
 
@@ -464,12 +471,6 @@ module Ci
       super(options).merge(when: read_attribute(:when))
     end
 
-    def update_project_statistics
-      return unless project
-
-      ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size])
-    end
-
     private
 
     def update_artifacts_size
@@ -560,6 +561,11 @@ module Ci
       pipeline.config_processor.build_attributes(name)
     end
 
+    def update_project_statistics
+      return unless project
+
+      ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size])
+    end
 
     def update_project_statistics_after_save
       if previous_changes.include?('artifacts_size')
diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb
new file mode 100644
index 00000000000..9c709077ac6
--- /dev/null
+++ b/app/models/ci/job_artifact.rb
@@ -0,0 +1,26 @@
+module Ci
+  class JobArtifact < ActiveRecord::Base
+    extend Gitlab::Ci::Model
+
+    belongs_to :project
+    belongs_to :job, class_name: "Ci::Build", foreign_key: :ci_job_id
+
+    before_save :set_size, if: :file_changed?
+    after_commit :remove_file!, on: :destroy
+
+    mount_uploader :file, JobArtifactUploader
+
+    enum file_type: {
+      archive: 1,
+      metadata: 2
+    }
+
+    def self.artifacts_size_for(project)
+      self.where(project: project).sum(:size)
+    end
+
+    def set_size
+      self.size = file.size
+    end
+  end
+end
diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb
index a14a278df9f..8e331617cfa 100644
--- a/app/models/concerns/artifact_migratable.rb
+++ b/app/models/concerns/artifact_migratable.rb
@@ -3,15 +3,14 @@
 # Meant to be prepended so the interface can stay the same
 module ArtifactMigratable
   def artifacts_file
-    artifacts.archive.first&.file || super
+    job_archive&.file || super
   end
 
   def artifacts_metadata
-    artifacts.metadata.first&.file || super
+    job_metadata&.file || super
   end
 
   def artifacts?
-    byebug
     !artifacts_expired? && artifacts_file.exists?
   end
 
@@ -19,19 +18,28 @@ module ArtifactMigratable
     artifacts? && artifacts_metadata.exists?
   end
 
-  def remove_artifacts_file!
-    artifacts_file.remove!
+  def artifacts_file_changed?
+    job_archive&.file_changed? || super
   end
 
-  def remove_artifacts_metadata!
-    artifacts_metadata.remove!
+  def remove_artifacts_file!
+    if job_archive
+      job_archive.destroy
+    else
+      super
+    end
   end
 
-  def artifacts_file=(file)
-    artifacts.create(project: project, type: :archive, file: file)
+  def remove_artifacts_metadata!
+    if job_metadata
+      job_metadata.destroy
+    else
+      super
+    end
   end
 
-  def artifacts_metadata=(file)
-    artifacts.create(project: project, type: :metadata, file: file)
+  def artifacts_size
+    read_attribute(:artifacts_size).to_i +
+      job_archive&.size.to_i + job_metadata&.size.to_i
   end
 end
diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb
index 715b215d1db..a9c22d9cf18 100644
--- a/app/models/project_statistics.rb
+++ b/app/models/project_statistics.rb
@@ -35,7 +35,10 @@ class ProjectStatistics < ActiveRecord::Base
   end
 
   def update_build_artifacts_size
-    self.build_artifacts_size = project.builds.sum(:artifacts_size)
+    self.build_artifacts_size =
+      project.builds.sum(:artifacts_size) +
+      Ci::JobArtifact.artifacts_size_for(self) +
+      Ci::JobArtifactMetadata.artifacts_size_for(self)
   end
 
   def update_storage_size
diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb
index 8ac0e2fe5a2..d4dda8e9e67 100644
--- a/app/uploaders/artifact_uploader.rb
+++ b/app/uploaders/artifact_uploader.rb
@@ -12,10 +12,6 @@ class ArtifactUploader < GitlabUploader
   end
 
   def initialize(job, field)
-    # Temporairy conditional, needed to move artifacts to their own table,
-    # but keeping compat with Ci::Build for the time being
-    job = job.build if job.respond_to?(:build)
-
     @job, @field = job, field
   end
 
@@ -38,6 +34,8 @@ class ArtifactUploader < GitlabUploader
   end
 
   def default_path
-    File.join(job.created_at.utc.strftime('%Y_%m'), job.project_id.to_s, job.id.to_s)
+    File.join(job.project_id.to_s,
+              job.created_at.utc.strftime('%Y_%m'),
+              job.id.to_s)
   end
 end
diff --git a/app/uploaders/job_artifact_uploader.rb b/app/uploaders/job_artifact_uploader.rb
new file mode 100644
index 00000000000..6ea6a85b4a2
--- /dev/null
+++ b/app/uploaders/job_artifact_uploader.rb
@@ -0,0 +1,34 @@
+class JobArtifactUploader < ArtifactUploader
+  def initialize(artifact, _field)
+    @artifact = artifact
+  end
+
+  # If this record exists, the associatied artifact is there. Every artifact
+  # persisted will have an associated file
+  def exists?
+    true
+  end
+
+  def size
+    return super unless @artifact.size
+
+    @artifact.size
+  end
+
+  private
+
+  def disk_hash
+    @disk_hash ||= Digest::SHA2.hexdigest(job.project_id.to_s)
+  end
+
+  def default_path
+    creation_date = job.created_at.utc.strftime('%Y_%m_%d')
+
+    File.join(disk_hash[0..1], disk_hash[2..3], disk_hash,
+              creation_date, job.id.to_s, @artifact.id.to_s)
+  end
+
+  def job
+    @artifact.job
+  end
+end
-- 
cgit v1.2.1