Merge branch 'dm-artifact-blob-viewer' into 'master'

Use blob viewer for job artifacts

Closes #31297

See merge request !11018

app/assets/javascripts/dispatcher.js

@@ -344,6 +344,9 @@ const ShortcutsBlob = require('./shortcuts_blob');
         case 'projects:artifacts:browse':
           new BuildArtifacts();
           break;
+        case 'projects:artifacts:file':
+          new BlobViewer();
+          break;
         case 'help:index':
           gl.VersionCheckImage.bindErrorEvent($('img.js-version-status-badge'));
           break;

app/controllers/projects/artifacts_controller.rb

 class Projects::ArtifactsController < Projects::ApplicationController
   include ExtractsPath
+  include RendersBlob
 
   layout 'project'
   before_action :authorize_read_build!
   before_action :authorize_update_build!, only: [:keep]
   before_action :extract_ref_name_and_path
   before_action :validate_artifacts!
+  before_action :set_path_and_entry, only: [:file, :raw]
 
   def download
     if artifacts_file.file_storage?
@@ -24,15 +26,24 @@ class Projects::ArtifactsController < Projects::ApplicationController
   end
 
   def file
-    entry = build.artifacts_metadata_entry(params[:path])
+    blob = @entry.blob
+    override_max_blob_size(blob)
 
-    if entry.exists?
-      send_artifacts_entry(build, entry)
-    else
-      render_404
+    respond_to do |format|
+      format.html do
+        render 'file'
+      end
+
+      format.json do
+        render_blob_json(blob)
+      end
     end
   end
 
+  def raw
+    send_artifacts_entry(build, @entry)
+  end
+
   def keep
     build.keep_artifacts!
     redirect_to namespace_project_build_path(project.namespace, project, build)
@@ -81,4 +92,11 @@ class Projects::ArtifactsController < Projects::ApplicationController
   def artifacts_file
     @artifacts_file ||= build.artifacts_file
   end
+
+  def set_path_and_entry
+    @path = params[:path]
+    @entry = build.artifacts_metadata_entry(@path)
+
+    render_404 unless @entry.exists?
+  end
 end

app/helpers/blob_helper.rb

@@ -119,7 +119,9 @@ module BlobHelper
   end
 
   def blob_raw_url
-    if @snippet
+    if @build && @entry
+      raw_namespace_project_build_artifacts_path(@project.namespace, @project, @build, path: @entry.path)
+    elsif @snippet
       if @snippet.project_id
         raw_namespace_project_snippet_path(@project.namespace, @project, @snippet)
       else
@@ -250,6 +252,8 @@ module BlobHelper
       case viewer.blob.external_storage
       when :lfs
         'it is stored in LFS'
+      when :build_artifact
+        'it is stored as a job artifact'
       else
         'it is stored externally'
       end

app/helpers/gitlab_routing_helper.rb

@@ -208,6 +208,8 @@ module GitlabRoutingHelper
       browse_namespace_project_build_artifacts_path(*args)
     when 'file'
       file_namespace_project_build_artifacts_path(*args)
+    when 'raw'
+      raw_namespace_project_build_artifacts_path(*args)
     end
   end
 

app/models/ci/artifact_blob.rb

+module Ci
+  class ArtifactBlob
+    include BlobLike
+
+    attr_reader :entry
+
+    def initialize(entry)
+      @entry = entry
+    end
+
+    delegate :name, :path, to: :entry
+
+    def id
+      Digest::SHA1.hexdigest(path)
+    end
+
+    def size
+      entry.metadata[:size]
+    end
+
+    def data
+      "Build artifact #{path}"
+    end
+
+    def mode
+      entry.metadata[:mode]
+    end
+
+    def external_storage
+      :build_artifact
+    end
+
+    alias_method :external_size, :size
+  end
+end

app/views/projects/artifacts/_tree_file.html.haml

 - path_to_file = file_namespace_project_build_artifacts_path(@project.namespace, @project, @build, path: file.path)
 
 %tr.tree-item{ 'data-link' => path_to_file }
+  - blob = file.blob
   %td.tree-item-file-name
-    = tree_icon('file', '664', file.name)
-    %span.str-truncated
-      = link_to file.name, path_to_file
+    = tree_icon('file', blob.mode, blob.name)
+    = link_to path_to_file do
+      %span.str-truncated= blob.name
   %td
-    = number_to_human_size(file.metadata[:size], precision: 2)
+    = number_to_human_size(blob.size, precision: 2)

app/views/projects/artifacts/file.html.haml

+- page_title @path, 'Artifacts', "#{@build.name} (##{@build.id})", 'Jobs'
+= render "projects/pipelines/head"
+
+= render "projects/builds/header", show_controls: false
+
+#tree-holder.tree-holder
+  .nav-block
+    %ul.breadcrumb.repo-breadcrumb
+      %li
+        = link_to 'Artifacts', browse_namespace_project_build_artifacts_path(@project.namespace, @project, @build)
+      - path_breadcrumbs do |title, path|
+        - title = truncate(title, length: 40)
+        %li
+          - if path == @path
+            = link_to file_namespace_project_build_artifacts_path(@project.namespace, @project, @build, path) do
+              %strong= title
+          - else
+            = link_to title, browse_namespace_project_build_artifacts_path(@project.namespace, @project, @build, path)
+
+
+  %article.file-holder
+    - blob = @entry.blob
+    .js-file-title.file-title-flex-parent
+      = render 'projects/blob/header_content', blob: blob
+
+      .file-actions.hidden-xs
+        = render 'projects/blob/viewer_switcher', blob: blob
+
+        .btn-group{ role: "group" }<
+          = copy_blob_source_button(blob)
+          = open_raw_blob_button(blob)
+
+    = render 'projects/blob/content', blob: blob

changelogs/unreleased/dm-artifact-blob-viewer.yml

+---
+title: Add artifact file page that uses the blob viewer
+merge_request:
+author:

config/routes/project.rb

@@ -183,6 +183,7 @@ constraints(ProjectUrlConstrainer.new) do
           get :download
           get :browse, path: 'browse(/*path)', format: false
           get :file, path: 'file/*path', format: false
+          get :raw, path: 'raw/*path', format: false
           post :keep
         end
       end

features/project/builds/artifacts.feature

@@ -46,13 +46,14 @@ Feature: Project Builds Artifacts
     And I navigate to parent directory of directory with invalid name
     Then I should not see directory with invalid name on the list
 
+  @javascript
   Scenario: I download a single file from build artifacts
     Given recent build has artifacts available
     And recent build has artifacts metadata available
     When I visit recent build details page
     And I click artifacts browse button
     And I click a link to file within build artifacts
-    Then download of a file extracted from build artifacts should start
+    Then I see a download link
 
   @javascript
   Scenario: I click on a row in an artifacts table

features/steps/project/builds/artifacts.rb

@@ -3,6 +3,7 @@ class Spinach::Features::ProjectBuildsArtifacts < Spinach::FeatureSteps
   include SharedProject
   include SharedBuilds
   include RepoHelpers
+  include WaitForAjax
 
   step 'I click artifacts download button' do
     click_link 'Download'
@@ -78,19 +79,11 @@ class Spinach::Features::ProjectBuildsArtifacts < Spinach::FeatureSteps
 
   step 'I click a link to file within build artifacts' do
     page.within('.tree-table') { find_link('ci_artifacts.txt').click }
+    wait_for_ajax
   end
 
-  step 'download of a file extracted from build artifacts should start' do
-    send_data = response_headers[Gitlab::Workhorse::SEND_DATA_HEADER]
-
-    expect(send_data).to start_with('artifacts-entry:')
-
-    base64_params = send_data.sub(/\Aartifacts\-entry:/, '')
-    params = JSON.parse(Base64.urlsafe_decode64(base64_params))
-
-    expect(params.keys).to eq(%w(Archive Entry))
-    expect(params['Archive']).to end_with('build_artifacts.zip')
-    expect(params['Entry']).to eq(Base64.encode64('ci_artifacts.txt'))
+  step 'I see a download link' do
+    expect(page).to have_link 'download it'
   end
 
   step 'I click a first row within build artifacts table' do

lib/gitlab/ci/build/artifacts/metadata/entry.rb

@@ -37,6 +37,12 @@ module Gitlab
           !directory?
         end
 
+        def blob
+          return unless file?
+
+          @blob ||= Blob.decorate(::Ci::ArtifactBlob.new(self), nil)
+        end
+
         def has_parent?
           nodes > 0
         end

spec/requests/projects/artifacts_controller_spec.rb → spec/controllers/projects/artifacts_controller_spec.rb

@@ -14,20 +14,91 @@ describe Projects::ArtifactsController do
 
   let(:build) { create(:ci_build, :success, :artifacts, pipeline: pipeline) }
 
-  describe 'GET /:project/builds/artifacts/:ref_name/browse?job=name' do
-    before do
-      project.team << [user, :developer]
+  before do
+    project.team << [user, :developer]
 
-      login_as(user)
+    sign_in(user)
+  end
+
+  describe 'GET download' do
+    it 'sends the artifacts file' do
+      expect(controller).to receive(:send_file).with(build.artifacts_file.path, disposition: 'attachment').and_call_original
+
+      get :download, namespace_id: project.namespace, project_id: project, build_id: build
+    end
+  end
+
+  describe 'GET browse' do
+    context 'when the directory exists' do
+      it 'renders the browse view' do
+        get :browse, namespace_id: project.namespace, project_id: project, build_id: build, path: 'other_artifacts_0.1.2'
+
+        expect(response).to render_template('projects/artifacts/browse')
+      end
+    end
+
+    context 'when the directory does not exist' do
+      it 'responds Not Found' do
+        get :browse, namespace_id: project.namespace, project_id: project, build_id: build, path: 'unknown'
+
+        expect(response).to be_not_found
+      end
+    end
+  end
+
+  describe 'GET file' do
+    context 'when the file exists' do
+      it 'renders the file view' do
+        get :file, namespace_id: project.namespace, project_id: project, build_id: build, path: 'ci_artifacts.txt'
+
+        expect(response).to render_template('projects/artifacts/file')
+      end
     end
 
-    def path_from_ref(
-      ref = pipeline.ref, job = build.name, path = 'browse')
-      latest_succeeded_namespace_project_artifacts_path(
-        project.namespace,
-        project,
-        [ref, path].join('/'),
-        job: job)
+    context 'when the file does not exist' do
+      it 'responds Not Found' do
+        get :file, namespace_id: project.namespace, project_id: project, build_id: build, path: 'unknown'
+
+        expect(response).to be_not_found
+      end
+    end
+  end
+
+  describe 'GET raw' do
+    context 'when the file exists' do
+      it 'serves the file using workhorse' do
+        get :raw, namespace_id: project.namespace, project_id: project, build_id: build, path: 'ci_artifacts.txt'
+
+        send_data = response.headers[Gitlab::Workhorse::SEND_DATA_HEADER]
+
+        expect(send_data).to start_with('artifacts-entry:')
+
+        base64_params = send_data.sub(/\Aartifacts\-entry:/, '')
+        params = JSON.parse(Base64.urlsafe_decode64(base64_params))
+
+        expect(params.keys).to eq(%w(Archive Entry))
+        expect(params['Archive']).to end_with('build_artifacts.zip')
+        expect(params['Entry']).to eq(Base64.encode64('ci_artifacts.txt'))
+      end
+    end
+
+    context 'when the file does not exist' do
+      it 'responds Not Found' do
+        get :raw, namespace_id: project.namespace, project_id: project, build_id: build, path: 'unknown'
+
+        expect(response).to be_not_found
+      end
+    end
+  end
+
+  describe 'GET latest_succeeded' do
+    def params_from_ref(ref = pipeline.ref, job = build.name, path = 'browse')
+      {
+        namespace_id: project.namespace,
+        project_id: project,
+        ref_name_and_path: File.join(ref, path),
+        job: job
+      }
     end
 
     context 'cannot find the build' do
@@ -37,7 +108,7 @@ describe Projects::ArtifactsController do
 
       context 'has no such ref' do
         before do
-          get path_from_ref('TAIL', build.name)
+          get :latest_succeeded, params_from_ref('TAIL', build.name)
         end
 
         it_behaves_like 'not found'
@@ -45,7 +116,7 @@ describe Projects::ArtifactsController do
 
       context 'has no such build' do
         before do
-          get path_from_ref(pipeline.ref, 'NOBUILD')
+          get :latest_succeeded, params_from_ref(pipeline.ref, 'NOBUILD')
         end
 
         it_behaves_like 'not found'
@@ -53,7 +124,7 @@ describe Projects::ArtifactsController do
 
       context 'has no path' do
         before do
-          get path_from_ref(pipeline.sha, build.name, '')
+          get :latest_succeeded, params_from_ref(pipeline.sha, build.name, '')
         end
 
         it_behaves_like 'not found'
@@ -77,7 +148,7 @@ describe Projects::ArtifactsController do
           pipeline.update(ref: 'master',
                           sha: project.commit('master').sha)
 
-          get path_from_ref('master')
+          get :latest_succeeded, params_from_ref('master')
         end
 
         it_behaves_like 'redirect to the build'
@@ -88,7 +159,7 @@ describe Projects::ArtifactsController do
           pipeline.update(ref: 'improve/awesome',
                           sha: project.commit('improve/awesome').sha)
 
-          get path_from_ref('improve/awesome')
+          get :latest_succeeded, params_from_ref('improve/awesome')
         end
 
         it_behaves_like 'redirect to the build'
@@ -99,7 +170,7 @@ describe Projects::ArtifactsController do
           pipeline.update(ref: 'improve/awesome',
                           sha: project.commit('improve/awesome').sha)
 
-          get path_from_ref('improve/awesome', build.name, 'file/README.md')
+          get :latest_succeeded, params_from_ref('improve/awesome', build.name, 'file/README.md')
         end
 
         it 'redirects' do

spec/features/projects/artifacts/file_spec.rb

+require 'spec_helper'
+
+feature 'Artifact file', :js, feature: true do
+  let(:project) { create(:project, :public) }
+  let(:pipeline) { create(:ci_empty_pipeline, project: project, sha: project.commit.sha, ref: 'master') }
+  let(:build) { create(:ci_build, :artifacts, pipeline: pipeline) }
+
+  def visit_file(path)
+    visit file_namespace_project_build_artifacts_path(project.namespace, project, build, path)
+  end
+
+  context 'Text file' do
+    before do
+      visit_file('other_artifacts_0.1.2/doc_sample.txt')
+
+      wait_for_ajax
+    end
+
+    it 'displays an error' do
+      aggregate_failures do
+        # shows an error message
+        expect(page).to have_content('The source could not be displayed because it is stored as a job artifact. You can download it instead.')
+
+        # does not show a viewer switcher
+        expect(page).not_to have_selector('.js-blob-viewer-switcher')
+
+        # does not show a copy button
+        expect(page).not_to have_selector('.js-copy-blob-source-btn')
+
+        # shows a download button
+        expect(page).to have_link('Download')
+      end
+    end
+  end
+
+  context 'JPG file' do
+    before do
+      visit_file('rails_sample.jpg')
+
+      wait_for_ajax
+    end
+
+    it 'displays the blob' do
+      aggregate_failures do
+        # shows rendered image
+        expect(page).to have_selector('.image_file img')
+
+        # does not show a viewer switcher
+        expect(page).not_to have_selector('.js-blob-viewer-switcher')
+
+        # does not show a copy button
+        expect(page).not_to have_selector('.js-copy-blob-source-btn')
+
+        # shows a download button
+        expect(page).to have_link('Download')
+      end
+    end
+  end
+end

spec/lib/gitlab/ci/build/artifacts/metadata/entry_spec.rb

@@ -135,6 +135,17 @@ describe Gitlab::Ci::Build::Artifacts::Metadata::Entry do
       subject { |example| path(example).nodes }
       it { is_expected.to eq 4 }
     end
+
+    describe '#blob' do
+      let(:file_entry) { |example| path(example) }
+      subject { file_entry.blob }
+
+      it 'returns a blob representing the entry data' do
+        expect(subject).to be_a(Blob)
+        expect(subject.path).to eq(file_entry.path)
+        expect(subject.size).to eq(file_entry.metadata[:size])
+      end
+    end
   end
 
   describe 'non-existent/', path: 'non-existent/' do

spec/models/ci/artifact_blob_spec.rb

+require 'spec_helper'
+
+describe Ci::ArtifactBlob, models: true do
+  let(:build) { create(:ci_build, :artifacts) }
+  let(:entry) { build.artifacts_metadata_entry('other_artifacts_0.1.2/another-subdirectory/banana_sample.gif') }
+
+  subject { described_class.new(entry) }
+
+  describe '#id' do
+    it 'returns a hash of the path' do
+      expect(subject.id).to eq(Digest::SHA1.hexdigest(entry.path))
+    end
+  end
+
+  describe '#name' do
+    it 'returns the entry name' do
+      expect(subject.name).to eq(entry.name)
+    end
+  end
+
+  describe '#path' do
+    it 'returns the entry path' do
+      expect(subject.path).to eq(entry.path)
+    end
+  end
+
+  describe '#size' do
+    it 'returns the entry size' do
+      expect(subject.size).to eq(entry.metadata[:size])
+    end
+  end
+
+  describe '#mode' do
+    it 'returns the entry mode' do
+      expect(subject.mode).to eq(entry.metadata[:mode])
+    end
+  end
+
+  describe '#external_storage' do
+    it 'returns :build_artifact' do
+      expect(subject.external_storage).to eq(:build_artifact)
+    end
+  end
+end