From 25df666156279e5b392b429519b4f4ba01eefaac Mon Sep 17 00:00:00 2001
From: Zeger-Jan van de Weg <git@zjvandeweg.nl>
Date: Tue, 19 Sep 2017 09:14:06 +0200
Subject: [PATCH] Create Ci::Artifacts

To allow jobs/builds to have multiple artifacts, and to start seperating
concerns from Ci::Build a new model is created: Ci::Artifact.

Changes include the updating of the ArtifactUploader to adapt to a
slightly different interface. The uploader expects to be initialized
with a `Ci::Build`.

Futher a migration with the minimal fields, the needed foreign keys and
an index.

Last, the way this works is by prepending a module to Ci::Build so we
can basically override behaviour but if needed use `super` to get the
original behaviour.
---
 app/models/ci/artifact.rb                     | 26 +++++---
 app/models/ci/build.rb                        | 23 +++----
 app/models/concerns/artifact_migratable.rb    | 37 ++++++++++++
 app/uploaders/artifact_uploader.rb            |  4 ++
 db/migrate/20170918072948_create_artifacts.rb |  5 +-
 db/schema.rb                                  |  8 +--
 lib/api/entities.rb                           |  2 +-
 spec/factories/ci/artifacts.rb                | 22 +++++++
 spec/factories/ci/builds.rb                   | 27 ++-------
 spec/models/ci/artifact_spec.rb               | 60 ++++++++++++++++++-
 spec/models/ci/build_spec.rb                  | 17 ++----
 11 files changed, 163 insertions(+), 68 deletions(-)
 create mode 100644 app/models/concerns/artifact_migratable.rb
 create mode 100644 spec/factories/ci/artifacts.rb

diff --git a/app/models/ci/artifact.rb b/app/models/ci/artifact.rb
index c66c560037e..858609883ce 100644
--- a/app/models/ci/artifact.rb
+++ b/app/models/ci/artifact.rb
@@ -1,12 +1,24 @@
 module Ci
   class Artifact < ActiveRecord::Base
-    belongs_to :build, class_name: "Ci::Build"
-    belongs_to :project, class_name: "Ci::Build"
+    extend Gitlab::Ci::Model
 
-    enum type {
-      archive: 0,
-      metadata: 1,
-      trace: 2
-    }
+    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 4ea040dfad5..fae2f5590b4 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -1,5 +1,6 @@
 module Ci
   class Build < CommitStatus
+    prepend ArtifactMigratable
     include TokenAuthenticatable
     include AfterCommitQueue
     include Presentable
@@ -10,6 +11,7 @@ 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'
 
@@ -326,14 +328,6 @@ module Ci
       project.running_or_pending_build_count(force: true)
     end
 
-    def artifacts?
-      !artifacts_expired? && artifacts_file.exists?
-    end
-
-    def artifacts_metadata?
-      artifacts? && artifacts_metadata.exists?
-    end
-
     def artifacts_metadata_entry(path, **options)
       metadata = Gitlab::Ci::Build::Artifacts::Metadata.new(
         artifacts_metadata.path,
@@ -429,7 +423,7 @@ module Ci
       Gitlab::Ci::Build::Image.from_services(self)
     end
 
-    def artifacts
+    def artifacts_options
       [options[:artifacts]]
     end
 
@@ -470,6 +464,12 @@ 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,11 +560,6 @@ 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/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb
new file mode 100644
index 00000000000..a14a278df9f
--- /dev/null
+++ b/app/models/concerns/artifact_migratable.rb
@@ -0,0 +1,37 @@
+# Adapter class to unify the interface between mounted uploaders and the
+# Ci::Artifact model
+# Meant to be prepended so the interface can stay the same
+module ArtifactMigratable
+  def artifacts_file
+    artifacts.archive.first&.file || super
+  end
+
+  def artifacts_metadata
+    artifacts.metadata.first&.file || super
+  end
+
+  def artifacts?
+    byebug
+    !artifacts_expired? && artifacts_file.exists?
+  end
+
+  def artifacts_metadata?
+    artifacts? && artifacts_metadata.exists?
+  end
+
+  def remove_artifacts_file!
+    artifacts_file.remove!
+  end
+
+  def remove_artifacts_metadata!
+    artifacts_metadata.remove!
+  end
+
+  def artifacts_file=(file)
+    artifacts.create(project: project, type: :archive, file: file)
+  end
+
+  def artifacts_metadata=(file)
+    artifacts.create(project: project, type: :metadata, file: file)
+  end
+end
diff --git a/app/uploaders/artifact_uploader.rb b/app/uploaders/artifact_uploader.rb
index 14addb6cf14..8ac0e2fe5a2 100644
--- a/app/uploaders/artifact_uploader.rb
+++ b/app/uploaders/artifact_uploader.rb
@@ -12,6 +12,10 @@ 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
 
diff --git a/db/migrate/20170918072948_create_artifacts.rb b/db/migrate/20170918072948_create_artifacts.rb
index dd0af2a7066..1dd5edc3f15 100644
--- a/db/migrate/20170918072948_create_artifacts.rb
+++ b/db/migrate/20170918072948_create_artifacts.rb
@@ -3,15 +3,12 @@ class CreateArtifacts < ActiveRecord::Migration
     create_table :ci_artifacts do |t|
       t.belongs_to :project, null: false, foreign_key: { on_delete: :cascade }
       t.belongs_to :ci_build, null: false, foreign_key: { on_delete: :cascade }
+      t.integer :type, default: 0, null: false
       t.integer :size, limit: 8, default: 0
 
       t.datetime_with_timezone :created_at, null: false
       t.datetime_with_timezone :updated_at, null: false
 
-      t.datetime_with_timezone :expire_at
-      t.integer :erased_by_id, null: false
-      t.datetime_with_timezone :erased_at
-
       t.text :file
     end
 
diff --git a/db/schema.rb b/db/schema.rb
index b37e0eabbd6..b4048371676 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -229,12 +229,10 @@ ActiveRecord::Schema.define(version: 20171124150326) do
   create_table "ci_artifacts", force: :cascade do |t|
     t.integer "project_id", null: false
     t.integer "ci_build_id", null: false
+    t.integer "type", default: 0, null: false
     t.integer "size", limit: 8, default: 0
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
-    t.datetime "expire_at"
-    t.integer "erased_by_id", null: false
-    t.datetime "erased_at"
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
     t.text "file"
   end
 
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index ce332fe85d2..9eb2c98c436 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -1036,7 +1036,7 @@ module API
         expose :steps, using: Step
         expose :image, using: Image
         expose :services, using: Service
-        expose :artifacts, using: Artifacts
+        expose :artifacts_options, as: :artifacts, using: Artifacts
         expose :cache, using: Cache
         expose :credentials, using: Credentials
         expose :dependencies, using: Dependency
diff --git a/spec/factories/ci/artifacts.rb b/spec/factories/ci/artifacts.rb
new file mode 100644
index 00000000000..085e707d686
--- /dev/null
+++ b/spec/factories/ci/artifacts.rb
@@ -0,0 +1,22 @@
+include ActionDispatch::TestProcess
+
+FactoryGirl.define do
+  factory :artifact, class: Ci::Artifact do
+    project
+    build factory: :ci_build
+
+    after :create do |artifact|
+      artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'), 'application/zip')
+      artifact.save
+    end
+
+    factory :artifact_metadata do
+      type :metadata
+
+      after :create do |artifact|
+        artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'), 'application/x-gzip')
+        artifact.save
+      end
+    end
+  end
+end
diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb
index cf38066dedc..69d58f367ac 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -156,32 +156,13 @@ FactoryGirl.define do
 
     trait :artifacts do
       after(:create) do |build, _|
-        build.artifacts_file =
-          fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'),
-                             'application/zip')
-
-        build.artifacts_metadata =
-          fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'),
-                             'application/x-gzip')
-
-        build.save!
+        create(:artifact, build: build)
+        create(:artifact_metadata, build: build)
       end
     end
 
-    trait :artifacts_expired do
-      after(:create) do |build, _|
-        build.artifacts_file =
-          fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'),
-            'application/zip')
-
-        build.artifacts_metadata =
-          fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts_metadata.gz'),
-            'application/x-gzip')
-
-        build.artifacts_expire_at = 1.minute.ago
-
-        build.save!
-      end
+    trait :expired do
+      artifacts_expire_at = 1.minute.ago
     end
 
     trait :with_commit do
diff --git a/spec/models/ci/artifact_spec.rb b/spec/models/ci/artifact_spec.rb
index 438964fd787..5e39f73763b 100644
--- a/spec/models/ci/artifact_spec.rb
+++ b/spec/models/ci/artifact_spec.rb
@@ -1,5 +1,59 @@
-require 'rails_helper'
+require 'spec_helper'
 
-RSpec.describe Artifact, type: :model do
-  pending "add some examples to (or delete) #{__FILE__}"
+describe Ci::Artifact do
+  it { is_expected.to belong_to(:project) }
+  it { is_expected.to belong_to(:build) }
+
+  it { is_expected.to respond_to(:file) }
+  it { is_expected.to respond_to(:created_at) }
+  it { is_expected.to respond_to(:updated_at) }
+  it { is_expected.to respond_to(:type) }
+
+  let(:artifact) { create(:artifact) }
+
+  describe '#type' do
+    it 'defaults to archive' do
+      expect(artifact.type).to eq("archive")
+    end
+  end
+
+  describe '#set_size' do
+    it 'sets the size' do
+      expect(artifact.size).to eq(106365)
+    end
+  end
+
+  describe '#file' do
+    subject { artifact.file }
+
+    context 'the uploader api' do
+      it { is_expected.to respond_to(:store_dir) }
+      it { is_expected.to respond_to(:cache_dir) }
+      it { is_expected.to respond_to(:work_dir) }
+    end
+  end
+
+  describe '#remove_file' do
+    it 'removes the file from the database' do
+      artifact.remove_file!
+
+      expect(artifact.file.exists?).to be_falsy
+    end
+  end
+
+  describe '#exists?' do
+    context 'when the artifact file is present' do
+      it 'is returns true' do
+        expect(artifact.exists?).to be(true)
+      end
+    end
+
+    context 'when the file has been removed' do
+      it 'does not exist' do
+        artifact.remove_file!
+
+        expect(artifact.exists?).to be_falsy
+      end
+    end
+  end
 end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 1795ee8e9a4..52a3732d0cd 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -23,6 +23,8 @@ describe Ci::Build do
   it { is_expected.to respond_to(:has_trace?) }
   it { is_expected.to respond_to(:trace) }
 
+  it { is_expected.to be_a(ArtifactMigratable) }
+
   describe 'callbacks' do
     context 'when running after_create callback' do
       it 'triggers asynchronous build hooks worker' do
@@ -130,33 +132,26 @@ describe Ci::Build do
   end
 
   describe '#artifacts?' do
+    let(:build) { create(:ci_build, :artifacts) }
+
     subject { build.artifacts? }
 
     context 'artifacts archive does not exist' do
-      before do
-        build.update_attributes(artifacts_file: nil)
-      end
+      let(:build) { create(:ci_build) }
 
       it { is_expected.to be_falsy }
     end
 
     context 'artifacts archive exists' do
-      let(:build) { create(:ci_build, :artifacts) }
       it { is_expected.to be_truthy }
 
       context 'is expired' do
-        before do
-          build.update(artifacts_expire_at: Time.now - 7.days)
-        end
+        let(:build) { create(:ci_build, :artifacts, :expired) }
 
         it { is_expected.to be_falsy }
       end
 
       context 'is not expired' do
-        before do
-          build.update(artifacts_expire_at: Time.now + 7.days)
-        end
-
         it { is_expected.to be_truthy }
       end
     end