Store expire_at in ci_job_artifacts

app/models/ci/build.rb

@@ -16,8 +16,8 @@ module Ci
     has_many :trace_sections, class_name: 'Ci::BuildTraceSection'
 
     has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-    has_one :job_artifacts_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id
-    has_one :job_artifacts_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id
+    has_one :job_artifacts_archive, -> { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id
+    has_one :job_artifacts_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id
 
     # The "environment" field for builds is a String, and is the unexpanded name
     def persisted_environment
@@ -38,7 +38,7 @@ module Ci
     scope :unstarted, ->() { where(runner_id: nil) }
     scope :ignore_failures, ->() { where(allow_failure: false) }
     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.job_id'))
+      where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.build_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) }
@@ -386,6 +386,7 @@ module Ci
 
     def keep_artifacts!
       self.update(artifacts_expire_at: nil)
+      self.job_artifacts.update_all(expire_at: nil)
     end
 
     def coverage_regex

app/models/ci/job_artifact.rb

@@ -21,5 +21,16 @@ module Ci
     def set_size
       self.size = file.size
     end
+
+    def expire_in
+      expire_at - Time.now if expire_at
+    end
+
+    def expire_in=(value)
+      self.expire_at =
+        if value
+          ChronicDuration.parse(value)&.seconds&.from_now
+        end
+    end
   end
 end

db/migrate/20170918072948_create_job_artifacts.rb

@@ -7,11 +7,12 @@ class CreateJobArtifacts < ActiveRecord::Migration
     create_table :ci_job_artifacts do |t|
       t.belongs_to :project, null: false, index: true, foreign_key: { on_delete: :cascade }
       t.integer :job_id, null: false, index: true
-      t.integer :size, limit: 8
       t.integer :file_type, null: false, index: true
+      t.integer :size, limit: 8
 
       t.datetime_with_timezone :created_at, null: false
       t.datetime_with_timezone :updated_at, null: false
+      t.datetime_with_timezone :expire_at
 
       t.string :file
 

db/schema.rb

@@ -322,10 +322,11 @@ ActiveRecord::Schema.define(version: 20171130145523) do
   create_table "ci_job_artifacts", force: :cascade do |t|
     t.integer "project_id", null: false
     t.integer "job_id", null: false
-    t.integer "size", limit: 8
     t.integer "file_type", null: false
+    t.integer "size", limit: 8
     t.datetime_with_timezone "created_at", null: false
     t.datetime_with_timezone "updated_at", null: false
+    t.datetime_with_timezone "expire_at"
     t.string "file"
   end
 

lib/api/runner.rb

@@ -222,12 +222,13 @@ module API
         bad_request!('Missing artifacts file!') unless artifacts
         file_to_large! unless artifacts.size < max_artifacts_size
 
-        job.job_artifacts.build(project: job.project, file_type: :archive, file: artifacts)
-        job.job_artifacts.build(project: job.project, file_type: :metadata, file: metadata)
-
-        job.artifacts_expire_in = params['expire_in'] ||
+        expire_in = params['expire_in'] ||
           Gitlab::CurrentSettings.current_application_settings.default_artifacts_expire_in
 
+        job.build_job_artifacts_archive(project: job.project, file_type: :archive, file: artifacts, expire_in: expire_in)
+        job.build_job_artifacts_metadata(project: job.project, file_type: :metadata, file: metadata, expire_in: expire_in) if metadata
+        job.artifacts_expire_in = expire_in
+
         if job.save
           present job, with: Entities::JobRequest::Response
         else

spec/models/ci/build_spec.rb

@@ -1008,11 +1008,23 @@ describe Ci::Build do
   describe '#keep_artifacts!' do
     let(:build) { create(:ci_build, artifacts_expire_at: Time.now + 7.days) }
 
+    subject { build.keep_artifacts! }
+
     it 'to reset expire_at' do
-      build.keep_artifacts!
+      subject
 
       expect(build.artifacts_expire_at).to be_nil
     end
+
+    context 'when having artifacts files' do
+      let!(:artifact) { create(:ci_job_artifact, job: build, expire_in: '7 days') }
+
+      it 'to reset dependent objects' do
+        subject
+
+        expect(artifact.reload.expire_at).to be_nil
+      end
+    end
   end
 
   describe '#merge_request' do

spec/models/ci/job_artifact_spec.rb

@@ -27,4 +27,48 @@ describe Ci::JobArtifact do
       it { is_expected.to respond_to(:work_dir) }
     end
   end
+
+  describe '#expire_in' do
+    subject { artifact.expire_in }
+
+    it { is_expected.to be_nil }
+
+    context 'when expire_at is specified' do
+      let(:expire_at) { Time.now + 7.days }
+
+      before do
+        artifact.expire_at = expire_at
+      end
+
+      it { is_expected.to be_within(5).of(expire_at - Time.now) }
+    end
+  end
+
+  describe '#expire_in=' do
+    subject { artifact.expire_in }
+
+    it 'when assigning valid duration' do
+      artifact.expire_in = '7 days'
+
+      is_expected.to be_within(10).of(7.days.to_i)
+    end
+
+    it 'when assigning invalid duration' do
+      expect { artifact.expire_in = '7 elephants' }.to raise_error(ChronicDuration::DurationParseError)
+      
+      is_expected.to be_nil
+    end
+
+    it 'when resetting value' do
+      artifact.expire_in = nil
+
+      is_expected.to be_nil
+    end
+
+    it 'when setting to 0' do
+      artifact.expire_in = '0'
+
+      is_expected.to be_nil
+    end
+  end
 end