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

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

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

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,

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

spec/features/projects/builds_spec.rb

 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

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