Merge branch 'fix/handle-raw-trace-error-on-old-builds' into 'master'

Handle error on trace raw download with old builds (DB stored)

## What does this MR do?

Handles error on `raw build trace` download action for old builds (which are stored in DB instead of file).

## Are there points in the code the reviewer needs to double check?

No.

## Why was this MR needed?

At the beginning build traces were stored in database but at some point we moved to store them in files. All trace related actions are aware of this, but not `raw trace download`.

## What are the relevant issue numbers?

Fixes #18900

## Does this MR meet the acceptance criteria?


- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [ ] ~~[Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)~~
- [ ] ~~API support added~~
- Tests
  - [x] Added for this feature/bug
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [ ] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !4822

6 files changed, 176 insertions, 32 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 85f86d3031e..ab4aff0f89a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -73,6 +73,7 @@ v 8.12.0 (unreleased)
   - Fix hover leading space bug in pipeline graph !5980
   - User can edit closed MR with deleted fork (Katarzyna Kobierska Ula Budziszewska) !5496
   - Fixed invisible scroll controls on build page on iPhone
+  - Fix error on raw build trace download for old builds stored in database !4822
 
 v 8.11.5 (unreleased)
   - Optimize branch lookups and force a repository reload for Repository#find_branch
diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb
index 12195c3cbb8..77934ff9962 100644
--- a/app/controllers/projects/builds_controller.rb
+++ b/app/controllers/projects/builds_controller.rb
@@ -78,8 +78,8 @@ class Projects::BuildsController < Projects::ApplicationController
   end
 
   def raw
-    if @build.has_trace?
-      send_file @build.path_to_trace, type: 'text/plain; charset=utf-8', disposition: 'inline'
+    if @build.has_trace_file?
+      send_file @build.trace_file_path, type: 'text/plain; charset=utf-8', disposition: 'inline'
     else
       render_404
     end
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb
index 23c8de6f650..61052437318 100644
--- a/app/models/ci/build.rb
+++ b/app/models/ci/build.rb
@@ -208,22 +208,31 @@ module Ci
       end
     end
 
+    def has_trace_file?
+      File.exist?(path_to_trace) || has_old_trace_file?
+    end
+
     def has_trace?
       raw_trace.present?
     end
 
     def raw_trace
-      if File.file?(path_to_trace)
-        File.read(path_to_trace)
-      elsif project.ci_id && File.file?(old_path_to_trace)
-        # Temporary fix for build trace data integrity
-        File.read(old_path_to_trace)
+      if File.exist?(trace_file_path)
+        File.read(trace_file_path)
       else
         # backward compatibility
         read_attribute :trace
       end
     end
 
+    ##
+    # Deprecated
+    #
+    # This is a hotfix for CI build data integrity, see #4246
+    def has_old_trace_file?
+      project.ci_id && File.exist?(old_path_to_trace)
+    end
+
     def trace
       trace = raw_trace
       if project && trace.present? && project.runners_token.present?
@@ -262,6 +271,14 @@ module Ci
       end
     end
 
+    def trace_file_path
+      if has_old_trace_file?
+        old_path_to_trace
+      else
+        path_to_trace
+      end
+    end
+
     def dir_to_trace
       File.join(
         Settings.gitlab_ci.builds_path,
diff --git a/app/views/projects/builds/_sidebar.html.haml b/app/views/projects/builds/_sidebar.html.haml
index 5b0b58e087b..49c8bd11634 100644
--- a/app/views/projects/builds/_sidebar.html.haml
+++ b/app/views/projects/builds/_sidebar.html.haml
@@ -100,7 +100,7 @@
         - elsif @build.runner
           \##{@build.runner.id}
       .btn-group.btn-group-justified{ role: :group }
-        - if @build.has_trace?
+        - if @build.has_trace_file?
           = link_to 'Raw', raw_namespace_project_build_path(@project.namespace, @project, @build), class: 'btn btn-sm btn-default'
         - if @build.active?
           = link_to "Cancel", cancel_namespace_project_build_path(@project.namespace, @project, @build), class: 'btn btn-sm btn-default', method: :post
diff --git a/spec/features/projects/builds_spec.rb b/spec/features/projects/builds_spec.rb
index 0cfeb2e57d8..d5d571e49bc 100644
--- a/spec/features/projects/builds_spec.rb
+++ b/spec/features/projects/builds_spec.rb
@@ -1,4 +1,5 @@
 require 'spec_helper'
+require 'tempfile'
 
 describe "Builds" do
   let(:artifacts_file) { fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif') }
@@ -6,7 +7,7 @@ describe "Builds" do
   before do
     login_as(:user)
     @commit = FactoryGirl.create :ci_pipeline
-    @build = FactoryGirl.create :ci_build, pipeline: @commit
+    @build = FactoryGirl.create :ci_build, :trace, pipeline: @commit
     @build2 = FactoryGirl.create :ci_build
     @project = @commit.project
     @project.team << [@user, :developer]
@@ -156,7 +157,6 @@ describe "Builds" do
     context 'Build raw trace' do
       before do
         @build.run!
-        @build.trace = 'BUILD TRACE'
         visit namespace_project_build_path(@project.namespace, @project, @build)
       end
 
@@ -255,35 +255,101 @@ describe "Builds" do
     end
   end
 
-  describe "GET /:project/builds/:id/raw" do
-    context "Build from project" do
-      before do
-        Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
-        @build.run!
-        @build.trace = 'BUILD TRACE'
-        visit namespace_project_build_path(@project.namespace, @project, @build)
-        page.within('.js-build-sidebar') { click_link 'Raw' }
+  describe 'GET /:project/builds/:id/raw' do
+    context 'access source' do
+      context 'build from project' do
+        before do
+          Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
+          @build.run!
+          visit namespace_project_build_path(@project.namespace, @project, @build)
+          page.within('.js-build-sidebar') { click_link 'Raw' }
+        end
+
+        it 'sends the right headers' do
+          expect(page.status_code).to eq(200)
+          expect(page.response_headers['Content-Type']).to eq('text/plain; charset=utf-8')
+          expect(page.response_headers['X-Sendfile']).to eq(@build.path_to_trace)
+        end
       end
 
-      it 'sends the right headers' do
-        expect(page.status_code).to eq(200)
-        expect(page.response_headers['Content-Type']).to eq('text/plain; charset=utf-8')
-        expect(page.response_headers['X-Sendfile']).to eq(@build.path_to_trace)
+      context 'build from other project' do
+        before do
+          Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
+          @build2.run!
+          visit raw_namespace_project_build_path(@project.namespace, @project, @build2)
+        end
+
+        it 'sends the right headers' do
+          expect(page.status_code).to eq(404)
+        end
       end
     end
 
-    context "Build from other project" do
-      before do
-        Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
-        @build2.run!
-        @build2.trace = 'BUILD TRACE'
-        visit raw_namespace_project_build_path(@project.namespace, @project, @build2)
-        puts page.status_code
-        puts current_url
+    context 'storage form' do
+      let(:existing_file) { Tempfile.new('existing-trace-file').path }
+      let(:non_existing_file) do
+        file = Tempfile.new('non-existing-trace-file')
+        path = file.path
+        file.unlink
+        path
       end
 
-      it 'sends the right headers' do
-        expect(page.status_code).to eq(404)
+      context 'when build has trace in file' do
+        before do
+          Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
+          @build.run!
+          visit namespace_project_build_path(@project.namespace, @project, @build)
+
+          allow_any_instance_of(Project).to receive(:ci_id).and_return(nil)
+          allow_any_instance_of(Ci::Build).to receive(:path_to_trace).and_return(existing_file)
+          allow_any_instance_of(Ci::Build).to receive(:old_path_to_trace).and_return(non_existing_file)
+
+          page.within('.js-build-sidebar') { click_link 'Raw' }
+        end
+
+        it 'sends the right headers' do
+          expect(page.status_code).to eq(200)
+          expect(page.response_headers['Content-Type']).to eq('text/plain; charset=utf-8')
+          expect(page.response_headers['X-Sendfile']).to eq(existing_file)
+        end
+      end
+
+      context 'when build has trace in old file' do
+        before do
+          Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
+          @build.run!
+          visit namespace_project_build_path(@project.namespace, @project, @build)
+
+          allow_any_instance_of(Project).to receive(:ci_id).and_return(999)
+          allow_any_instance_of(Ci::Build).to receive(:path_to_trace).and_return(non_existing_file)
+          allow_any_instance_of(Ci::Build).to receive(:old_path_to_trace).and_return(existing_file)
+
+          page.within('.js-build-sidebar') { click_link 'Raw' }
+        end
+
+        it 'sends the right headers' do
+          expect(page.status_code).to eq(200)
+          expect(page.response_headers['Content-Type']).to eq('text/plain; charset=utf-8')
+          expect(page.response_headers['X-Sendfile']).to eq(existing_file)
+        end
+      end
+
+      context 'when build has trace in DB' do
+        before do
+          Capybara.current_session.driver.header('X-Sendfile-Type', 'X-Sendfile')
+          @build.run!
+          visit namespace_project_build_path(@project.namespace, @project, @build)
+
+          allow_any_instance_of(Project).to receive(:ci_id).and_return(nil)
+          allow_any_instance_of(Ci::Build).to receive(:path_to_trace).and_return(non_existing_file)
+          allow_any_instance_of(Ci::Build).to receive(:old_path_to_trace).and_return(non_existing_file)
+
+          page.within('.js-build-sidebar') { click_link 'Raw' }
+        end
+
+        it 'sends the right headers' do
+          expect(page.status_code).to eq(404)
+        end
       end
     end
   end
diff --git a/spec/models/ci/build_spec.rb b/spec/models/ci/build_spec.rb
index 36d10636ae9..bce18b4e99e 100644
--- a/spec/models/ci/build_spec.rb
+++ b/spec/models/ci/build_spec.rb
@@ -19,4 +19,64 @@ describe Ci::Build, models: true do
       expect(build.trace).to eq(test_trace)
     end
   end
+
+  describe '#has_trace_file?' do
+    context 'when there is no trace' do
+      it { expect(build.has_trace_file?).to be_falsey }
+      it { expect(build.trace).to be_nil }
+    end
+
+    context 'when there is a trace' do
+      context 'when trace is stored in file' do
+        let(:build_with_trace) { create(:ci_build, :trace) }
+
+        it { expect(build_with_trace.has_trace_file?).to be_truthy }
+        it { expect(build_with_trace.trace).to eq('BUILD TRACE') }
+      end
+
+      context 'when trace is stored in old file' do
+        before do
+          allow(build.project).to receive(:ci_id).and_return(999)
+          allow(File).to receive(:exist?).with(build.path_to_trace).and_return(false)
+          allow(File).to receive(:exist?).with(build.old_path_to_trace).and_return(true)
+          allow(File).to receive(:read).with(build.old_path_to_trace).and_return(test_trace)
+        end
+
+        it { expect(build.has_trace_file?).to be_truthy }
+        it { expect(build.trace).to eq(test_trace) }
+      end
+
+      context 'when trace is stored in DB' do
+        before do
+          allow(build.project).to receive(:ci_id).and_return(nil)
+          allow(build).to receive(:read_attribute).with(:trace).and_return(test_trace)
+          allow(File).to receive(:exist?).with(build.path_to_trace).and_return(false)
+          allow(File).to receive(:exist?).with(build.old_path_to_trace).and_return(false)
+        end
+
+        it { expect(build.has_trace_file?).to be_falsey }
+        it { expect(build.trace).to eq(test_trace) }
+      end
+    end
+  end
+
+  describe '#trace_file_path' do
+    context 'when trace is stored in file' do
+      before do
+        allow(build).to receive(:has_trace_file?).and_return(true)
+        allow(build).to receive(:has_old_trace_file?).and_return(false)
+      end
+
+      it { expect(build.trace_file_path).to eq(build.path_to_trace) }
+    end
+
+    context 'when trace is stored in old file' do
+      before do
+        allow(build).to receive(:has_trace_file?).and_return(true)
+        allow(build).to receive(:has_old_trace_file?).and_return(true)
+      end
+
+      it { expect(build.trace_file_path).to eq(build.old_path_to_trace) }
+    end
+  end
 end