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.

12 files changed, 96 insertions, 108 deletions

diff --git a/spec/factories/ci/artifacts.rb b/spec/factories/ci/artifacts.rb
deleted file mode 100644
index 085e707d686..00000000000
--- a/spec/factories/ci/artifacts.rb
+++ /dev/null
@@ -1,22 +0,0 @@
-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 69d58f367ac..6cb612a58d2 100644
--- a/spec/factories/ci/builds.rb
+++ b/spec/factories/ci/builds.rb
@@ -155,14 +155,12 @@ FactoryGirl.define do
     end
 
     trait :artifacts do
-      after(:create) do |build, _|
-        create(:artifact, build: build)
-        create(:artifact_metadata, build: build)
-      end
+      job_archive factory: :ci_job_artifact
+      job_metadata factory: :ci_job_metadata
     end
 
     trait :expired do
-      artifacts_expire_at = 1.minute.ago
+      artifacts_expire_at 1.minute.ago
     end
 
     trait :with_commit do
diff --git a/spec/factories/ci/job_artifacts.rb b/spec/factories/ci/job_artifacts.rb
new file mode 100644
index 00000000000..8a7e04c747f
--- /dev/null
+++ b/spec/factories/ci/job_artifacts.rb
@@ -0,0 +1,28 @@
+include ActionDispatch::TestProcess
+
+FactoryGirl.define do
+  factory :ci_job_artifact, class: Ci::JobArtifact do
+    project
+    job factory: :ci_build
+    file_type :archive
+
+    after :create do |artifact|
+      if artifact.archive?
+        artifact.file = fixture_file_upload(Rails.root.join('spec/fixtures/ci_build_artifacts.zip'),
+                                            'application/zip')
+
+        artifact.save
+      end
+    end
+  end
+
+  factory :ci_job_metadata, parent: :ci_job_artifact do
+    file_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
diff --git a/spec/models/ci/artifact_spec.rb b/spec/models/ci/artifact_spec.rb
deleted file mode 100644
index 5e39f73763b..00000000000
--- a/spec/models/ci/artifact_spec.rb
+++ /dev/null
@@ -1,59 +0,0 @@
-require 'spec_helper'
-
-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 52a3732d0cd..f8dbed91c1a 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -146,7 +146,7 @@ describe Ci::Build do
       it { is_expected.to be_truthy }
 
       context 'is expired' do
-        let(:build) { create(:ci_build, :artifacts, :expired) }
+        let!(:build) { create(:ci_build, :artifacts, :expired) }
 
         it { is_expected.to be_falsy }
       end
diff --git a/spec/models/ci/job_artifact_spec.rb b/spec/models/ci/job_artifact_spec.rb
new file mode 100644
index 00000000000..5202a8183af
--- /dev/null
+++ b/spec/models/ci/job_artifact_spec.rb
@@ -0,0 +1,30 @@
+require 'spec_helper'
+
+describe Ci::JobArtifact do
+  set(:artifact) { create(:ci_job_artifact) }
+
+  describe "Associations" do
+    it { is_expected.to belong_to(:project) }
+    it { is_expected.to belong_to(:job) }
+  end
+
+  it { is_expected.to respond_to(:file) }
+  it { is_expected.to respond_to(:created_at) }
+  it { is_expected.to respond_to(:updated_at) }
+
+  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
+end
diff --git a/spec/models/project_statistics_spec.rb b/spec/models/project_statistics_spec.rb
index 59e20e84c2f..95e8d519bdd 100644
--- a/spec/models/project_statistics_spec.rb
+++ b/spec/models/project_statistics_spec.rb
@@ -133,15 +133,17 @@ describe ProjectStatistics do
 
   describe '#update_build_artifacts_size' do
     let!(:pipeline) { create(:ci_pipeline, project: project) }
-    let!(:build1) { create(:ci_build, pipeline: pipeline, artifacts_size: 45.megabytes) }
-    let!(:build2) { create(:ci_build, pipeline: pipeline, artifacts_size: 56.megabytes) }
+    let!(:ci_build) { create(:ci_build, pipeline: pipeline, artifacts_size: 45.megabytes) }
 
     before do
+      create(:ci_build, pipeline: pipeline, artifacts_size: 56.megabytes)
+      create(:ci_job_artifact, project: pipeline.project, job: ci_build)
+
       statistics.update_build_artifacts_size
     end
 
     it "stores the size of related build artifacts" do
-      expect(statistics.build_artifacts_size).to eq 101.megabytes
+      expect(statistics.build_artifacts_size).to eq(106012541)
     end
   end
 
diff --git a/spec/requests/api/runner_spec.rb b/spec/requests/api/runner_spec.rb
index 47f4ccd4887..f320a366e6e 100644
--- a/spec/requests/api/runner_spec.rb
+++ b/spec/requests/api/runner_spec.rb
@@ -945,7 +945,7 @@ describe API::Runner do
         context 'when artifacts are being stored inside of tmp path' do
           before do
             # by configuring this path we allow to pass temp file from any path
-            allow(ArtifactUploader).to receive(:artifacts_upload_path).and_return('/')
+            allow(JobArtifactUploader).to receive(:artifacts_upload_path).and_return('/')
           end
 
           context 'when job has been erased' do
@@ -1106,7 +1106,7 @@ describe API::Runner do
                 expect(response).to have_gitlab_http_status(201)
                 expect(stored_artifacts_file.original_filename).to eq(artifacts.original_filename)
                 expect(stored_metadata_file.original_filename).to eq(metadata.original_filename)
-                expect(stored_artifacts_size).to eq(71759)
+                expect(stored_artifacts_size).to eq(72821)
               end
             end
 
diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb
index 8fc1ceedc34..7144b66585c 100644
--- a/spec/serializers/pipeline_serializer_spec.rb
+++ b/spec/serializers/pipeline_serializer_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe PipelineSerializer do
-  let(:user) { create(:user) }
+  set(:user) { create(:user) }
 
   let(:serializer) do
     described_class.new(current_user: user)
diff --git a/spec/uploaders/artifact_uploader_spec.rb b/spec/uploaders/artifact_uploader_spec.rb
index 2a3bd0e3bb2..9cb2c090b43 100644
--- a/spec/uploaders/artifact_uploader_spec.rb
+++ b/spec/uploaders/artifact_uploader_spec.rb
@@ -1,7 +1,7 @@
 require 'rails_helper'
 
 describe ArtifactUploader do
-  let(:job) { create(:ci_build) }
+  set(:job) { create(:ci_build) }
   let(:uploader) { described_class.new(job, :artifacts_file) }
   let(:path) { Gitlab.config.artifacts.path }
 
@@ -26,7 +26,7 @@ describe ArtifactUploader do
     subject { uploader.store_dir }
 
     it { is_expected.to start_with(path) }
-    it { is_expected.to end_with("#{job.project_id}/#{job.id}") }
+    it { is_expected.to end_with("#{job.project_id}/#{job.created_at.utc.strftime('%Y_%m')}/#{job.id}") }
   end
 
   describe '#cache_dir' do
diff --git a/spec/uploaders/job_artifact_uploader_spec.rb b/spec/uploaders/job_artifact_uploader_spec.rb
new file mode 100644
index 00000000000..d045acf9089
--- /dev/null
+++ b/spec/uploaders/job_artifact_uploader_spec.rb
@@ -0,0 +1,15 @@
+require 'spec_helper'
+
+describe JobArtifactUploader do
+  set(:job_artifact) { create(:ci_job_artifact) }
+  let(:job) { job_artifact.job }
+  let(:uploader) { described_class.new(job_artifact, :file) }
+
+  describe '#store_dir' do
+    subject { uploader.store_dir }
+
+    it { is_expected.to start_with(Gitlab.config.artifacts.path) }
+    it { is_expected.not_to end_with("#{job.project_id}/#{job.created_at.utc.strftime('%Y_%m')}/#{job.id}") }
+    it { is_expected.to match(/\h{2}\/\h{2}\/\h{64}\/\d{4}_\d{1,2}_\d{1,2}\/\d+\/\d+\z/) }
+  end
+end
diff --git a/spec/workers/expire_build_instance_artifacts_worker_spec.rb b/spec/workers/expire_build_instance_artifacts_worker_spec.rb
index bed5c5e2ecb..c0d2b1b7411 100644
--- a/spec/workers/expire_build_instance_artifacts_worker_spec.rb
+++ b/spec/workers/expire_build_instance_artifacts_worker_spec.rb
@@ -11,12 +11,8 @@ describe ExpireBuildInstanceArtifactsWorker do
     end
 
     context 'with expired artifacts' do
-      let(:artifacts_expiry) { { artifacts_expire_at: Time.now - 7.days } }
-
       context 'when associated project is valid' do
-        let(:build) do
-          create(:ci_build, :artifacts, artifacts_expiry)
-        end
+        let(:build) { create(:ci_build, :artifacts, :expired) }
 
         it 'does expire' do
           expect(build.reload.artifacts_expired?).to be_truthy
@@ -26,14 +22,14 @@ describe ExpireBuildInstanceArtifactsWorker do
           expect(build.reload.artifacts_file.exists?).to be_falsey
         end
 
-        it 'does nullify artifacts_file column' do
-          expect(build.reload.artifacts_file_identifier).to be_nil
+        it 'does remove the job artifact record' do
+          expect(build.reload.job_archive).to be_nil
         end
       end
     end
 
     context 'with not yet expired artifacts' do
-      let(:build) do
+      set(:build) do
         create(:ci_build, :artifacts, artifacts_expire_at: Time.now + 7.days)
       end
 
@@ -45,8 +41,8 @@ describe ExpireBuildInstanceArtifactsWorker do
         expect(build.reload.artifacts_file.exists?).to be_truthy
       end
 
-      it 'does not nullify artifacts_file column' do
-        expect(build.reload.artifacts_file_identifier).not_to be_nil
+      it 'does not remove the job artifact record' do
+        expect(build.reload.job_archive).not_to be_nil
       end
     end
 
@@ -61,13 +57,13 @@ describe ExpireBuildInstanceArtifactsWorker do
         expect(build.reload.artifacts_file.exists?).to be_truthy
       end
 
-      it 'does not nullify artifacts_file column' do
-        expect(build.reload.artifacts_file_identifier).not_to be_nil
+      it 'does not remove the job artifact record' do
+        expect(build.reload.job_archive).not_to be_nil
       end
     end
 
     context 'for expired artifacts' do
-      let(:build) { create(:ci_build, artifacts_expire_at: Time.now - 7.days) }
+      let(:build) { create(:ci_build, :expired) }
 
       it 'is still expired' do
         expect(build.reload.artifacts_expired?).to be_truthy