Refactor ExpirePipelineCacheWorker#perform

Make it gracefully handle unexisting pipelines and refactor iterating
all the merge request paths.

3 files changed, 27 insertions, 13 deletions

diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb
index 0210d459048..603e2f1aaea 100644
--- a/app/workers/expire_pipeline_cache_worker.rb
+++ b/app/workers/expire_pipeline_cache_worker.rb
@@ -3,14 +3,18 @@ class ExpirePipelineCacheWorker
   include PipelineQueue
 
   def perform(pipeline_id)
-    pipeline = Ci::Pipeline.find(pipeline_id)
+    pipeline = Ci::Pipeline.find_by(id: pipeline_id)
+    return unless pipeline
+
     project = pipeline.project
     store = Gitlab::EtagCaching::Store.new
 
     store.touch(project_pipelines_path(project))
     store.touch(commit_pipelines_path(project, pipeline.commit)) if pipeline.commit
     store.touch(new_merge_request_pipelines_path(project))
-    merge_requests_pipelines_paths(project, pipeline).each { |path| store.touch(path) }
+    each_pipelines_merge_request_path(project, pipeline) do |path|
+      store.touch(path)
+    end
 
     Gitlab::Cache::Ci::ProjectPipelineStatus.update_for_pipeline(pipeline)
   end
@@ -39,13 +43,15 @@ class ExpirePipelineCacheWorker
       format: :json)
   end
 
-  def merge_requests_pipelines_paths(project, pipeline)
-    pipeline.all_merge_requests.collect do |merge_request|
-      Gitlab::Routing.url_helpers.pipelines_namespace_project_merge_request_path(
+  def each_pipelines_merge_request_path(project, pipeline)
+    pipeline.all_merge_requests.each do |merge_request|
+      path = Gitlab::Routing.url_helpers.pipelines_namespace_project_merge_request_path(
         project.namespace,
         project,
         merge_request,
         format: :json)
+
+      yield(path)
     end
   end
 end
diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index be14fdca81c..8b9b51bcf2f 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -1014,11 +1014,12 @@ describe Ci::Pipeline, models: true do
   end
 
   describe "#merge_requests" do
-    let(:project) { create(:project, :repository) }
-    let(:pipeline) { FactoryGirl.create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master').id) }
+    let(:project) { create(:empty_project) }
+    let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: 'a288a022a53a5a944fae87bcec6efc87b7061808') }
 
     it "returns merge requests whose `diff_head_sha` matches the pipeline's SHA" do
       merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref)
+      allow_any_instance_of(MergeRequest).to receive(:diff_head_sha) { 'a288a022a53a5a944fae87bcec6efc87b7061808' }
 
       expect(pipeline.merge_requests).to eq([merge_request])
     end
@@ -1038,23 +1039,24 @@ describe Ci::Pipeline, models: true do
   end
 
   describe "#all_merge_requests" do
-    let(:project) { create(:project, :repository) }
-    let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master').id) }
+    let(:project) { create(:empty_project) }
+    let(:pipeline) { create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: 'a288a022a53a5a944fae87bcec6efc87b7061808') }
 
-    it "returns merge requests whose `diff_head_sha` matches the pipeline's SHA" do
+    it "returns merge request if pipeline runs on `diff_head_sha`" do
       merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref)
+      allow_any_instance_of(MergeRequest).to receive(:diff_head_sha) { 'a288a022a53a5a944fae87bcec6efc87b7061808' }
 
       expect(pipeline.all_merge_requests).to eq([merge_request])
     end
 
-    it "returns merge requests whose source branch matches the pipeline's source branch" do
-      pipeline = create(:ci_empty_pipeline, status: 'created', project: project, ref: 'master', sha: project.repository.commit('master^').id)
+    it "returns merge request if pipeline runs any commit of the `source_branch`" do
       merge_request = create(:merge_request, source_project: project, source_branch: pipeline.ref)
+      allow_any_instance_of(MergeRequest).to receive(:diff_head_sha) { '97de212e80737a608d939f648d959671fb0a0142b' }
 
       expect(pipeline.all_merge_requests).to eq([merge_request])
     end
 
-    it "doesn't return merge requests whose source branch doesn't match the pipeline's ref" do
+    it "doesn't return merge request if pipeline runs on a different `source_branch`" do
       create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'master')
 
       expect(pipeline.all_merge_requests).to be_empty
diff --git a/spec/workers/expire_pipeline_cache_worker_spec.rb b/spec/workers/expire_pipeline_cache_worker_spec.rb
index 67f01539ada..58f587f6d11 100644
--- a/spec/workers/expire_pipeline_cache_worker_spec.rb
+++ b/spec/workers/expire_pipeline_cache_worker_spec.rb
@@ -29,6 +29,12 @@ describe ExpirePipelineCacheWorker do
       subject.perform(pipeline.id)
     end
 
+    it "doesn't do anything if the pipeline not exist" do
+      expect_any_instance_of(Gitlab::EtagCaching::Store).not_to receive(:touch)
+
+      subject.perform(617748)
+    end
+
     it 'updates the cached status for a project' do
       expect(Gitlab::Cache::Ci::ProjectPipelineStatus).to receive(:update_for_pipeline).
                                                             with(pipeline)