From 6b0a43aff36f0bbb9050b3c04155a3ccd9c1a75b Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 13 Jan 2016 21:17:28 +0100 Subject: Improve readability of artifacts browser `Entry` related code --- app/controllers/projects/artifacts_controller.rb | 10 +- app/models/ci/build.rb | 4 +- app/views/projects/artifacts/browse.html.haml | 12 +- features/steps/project/builds.rb | 2 +- lib/gitlab/ci/build/artifacts/metadata.rb | 5 +- lib/gitlab/ci/build/artifacts/metadata/entry.rb | 125 +++++++++++++++ lib/gitlab/ci/build/artifacts/metadata/path.rb | 124 --------------- .../ci/build/artifacts/metadata/entry_spec.rb | 170 +++++++++++++++++++++ .../ci/build/artifacts/metadata/path_spec.rb | 148 ------------------ .../lib/gitlab/ci/build/artifacts/metadata_spec.rb | 6 +- 10 files changed, 315 insertions(+), 291 deletions(-) create mode 100644 lib/gitlab/ci/build/artifacts/metadata/entry.rb delete mode 100644 lib/gitlab/ci/build/artifacts/metadata/path.rb create mode 100644 spec/lib/gitlab/ci/build/artifacts/metadata/entry_spec.rb delete mode 100644 spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb index 00daac0cb30..dff0732bdfe 100644 --- a/app/controllers/projects/artifacts_controller.rb +++ b/app/controllers/projects/artifacts_controller.rb @@ -18,17 +18,17 @@ class Projects::ArtifactsController < Projects::ApplicationController return render_404 unless build.artifacts? directory = params[:path] ? "#{params[:path]}/" : '' - @path = build.artifacts_metadata_path(directory) + @entry = build.artifacts_metadata_entry(directory) - return render_404 unless @path.exists? + return render_404 unless @entry.exists? end def file - file_path = build.artifacts_metadata_path(params[:path]) + entry = build.artifacts_metadata_entry(params[:path]) - if file_path.exists? + if entry.exists? render json: { archive: build.artifacts_file.path, - path: Base64.encode64(file_path.path) } + entry: Base64.encode64(entry.path) } else render json: {}, status: 404 end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index fef667f865e..6cc26abce66 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -347,8 +347,8 @@ module Ci artifacts? && artifacts_metadata.exists? end - def artifacts_metadata_path(path) - Gitlab::Ci::Build::Artifacts::Metadata.new(artifacts_metadata.path, path).to_path + def artifacts_metadata_entry(path) + Gitlab::Ci::Build::Artifacts::Metadata.new(artifacts_metadata.path, path).to_entry end private diff --git a/app/views/projects/artifacts/browse.html.haml b/app/views/projects/artifacts/browse.html.haml index fc161be2ffc..1add7ef6bfb 100644 --- a/app/views/projects/artifacts/browse.html.haml +++ b/app/views/projects/artifacts/browse.html.haml @@ -1,11 +1,11 @@ -- page_title "#{@build.name} (##{@build.id})", 'Build artifacts' -- header_title project_title(@project, "Build artifacts", namespace_project_build_path(@project.namespace, @project, @build)) +- page_title 'Artifacts', "#{@build.name} (##{@build.id})", 'Builds' += render 'projects/builds/header_title' #tree-holder.tree-holder .gray-content-block.top-block.clearfix .pull-right = link_to download_namespace_project_build_artifacts_path(@project.namespace, @project, @build), - class: 'btn btn-default', method: :get do + class: 'btn btn-default' do = icon('download') Download artifacts archive @@ -17,8 +17,8 @@ %th Name %th Size %th Download - = render partial: 'tree_directory', collection: @path.directories!, as: :directory - = render partial: 'tree_file', collection: @path.files, as: :file + = render partial: 'tree_directory', collection: @entry.directories(parent: true), as: :directory + = render partial: 'tree_file', collection: @entry.files, as: :file -- if @path.children.empty? +- if @entry.empty? .center Empty diff --git a/features/steps/project/builds.rb b/features/steps/project/builds.rb index 5702496ac84..28395281077 100644 --- a/features/steps/project/builds.rb +++ b/features/steps/project/builds.rb @@ -84,6 +84,6 @@ class Spinach::Features::ProjectBuilds < Spinach::FeatureSteps # this will be accelerated by Workhorse response_json = JSON.parse(page.body, symbolize_names: true) expect(response_json[:archive]).to end_with('build_artifacts.zip') - expect(response_json[:path]).to eq Base64.encode64('ci_artifacts.txt') + expect(response_json[:entry]).to eq Base64.encode64('ci_artifacts.txt') end end diff --git a/lib/gitlab/ci/build/artifacts/metadata.rb b/lib/gitlab/ci/build/artifacts/metadata.rb index e9ec8f1302c..bfdfc9a1d7d 100644 --- a/lib/gitlab/ci/build/artifacts/metadata.rb +++ b/lib/gitlab/ci/build/artifacts/metadata.rb @@ -34,8 +34,9 @@ module Gitlab end end - def to_path - Path.new(@path, *match!) + def to_entry + entires, metadata = match! + Entry.new(@path, entires, metadata) end private diff --git a/lib/gitlab/ci/build/artifacts/metadata/entry.rb b/lib/gitlab/ci/build/artifacts/metadata/entry.rb new file mode 100644 index 00000000000..2fb6c327729 --- /dev/null +++ b/lib/gitlab/ci/build/artifacts/metadata/entry.rb @@ -0,0 +1,125 @@ +module Gitlab + module Ci::Build::Artifacts + class Metadata + ## + # Class that represents an entry (path and metadata) to a file or + # directory in GitLab CI Build Artifacts binary file / archive + # + # This is IO-operations safe class, that does similar job to + # Ruby's Pathname but without the risk of accessing filesystem. + # + # This class is working only with UTF-8 encoded paths. + # + class Entry + attr_reader :path, :entires + attr_accessor :name + + def initialize(path, entires, metadata = []) + @path = path.force_encoding('UTF-8') + @entires = entires + @metadata = metadata + + if path.include?("\0") + raise ArgumentError, 'Path contains zero byte character!' + end + + unless path.valid_encoding? + raise ArgumentError, 'Path contains non-UTF-8 byte sequence!' + end + end + + def directory? + blank_node? || @path.end_with?('/') + end + + def file? + !directory? + end + + def has_parent? + nodes > 0 + end + + def parent + return nil unless has_parent? + new(@path.chomp(basename)) + end + + def basename + (directory? && !blank_node?) ? name + '/' : name + end + + def name + @name || @path.split('/').last.to_s + end + + def children + return [] unless directory? + return @children if @children + + child_pattern = %r{^#{Regexp.escape(@path)}[^/]+/?$} + @children = select_entires { |entry| entry =~ child_pattern } + end + + def directories(opts = {}) + return [] unless directory? + dirs = children.select(&:directory?) + return dirs unless has_parent? && opts[:parent] + + dotted_parent = parent + dotted_parent.name = '..' + dirs.prepend(dotted_parent) + end + + def files + return [] unless directory? + children.select(&:file?) + end + + def metadata + @index ||= @entires.index(@path) + @metadata[@index] || {} + end + + def nodes + @path.count('/') + (file? ? 1 : 0) + end + + def blank_node? + @path.empty? # "" is considered to be './' + end + + def exists? + blank_node? || @entires.include?(@path) + end + + def empty? + children.empty? + end + + def to_s + @path + end + + def ==(other) + @path == other.path && @entires == other.entires + end + + def inspect + "#{self.class.name}: #{@path}" + end + + private + + def new(path) + self.class.new(path, @entires, @metadata) + end + + def select_entires + selected = @entires.select { |entry| yield entry } + selected.map { |path| new(path) } + end + end + end + end +end diff --git a/lib/gitlab/ci/build/artifacts/metadata/path.rb b/lib/gitlab/ci/build/artifacts/metadata/path.rb deleted file mode 100644 index 6896aa936d5..00000000000 --- a/lib/gitlab/ci/build/artifacts/metadata/path.rb +++ /dev/null @@ -1,124 +0,0 @@ -module Gitlab - module Ci::Build::Artifacts - class Metadata - ## - # Class that represents a simplified path to a file or - # directory in GitLab CI Build Artifacts binary file / archive - # - # This is IO-operations safe class, that does similar job to - # Ruby's Pathname but without the risk of accessing filesystem. - # - # This class is working only with UTF-8 encoded paths. - # - class Path - attr_reader :path, :universe - attr_accessor :name - - def initialize(path, universe, metadata = []) - @path = path.force_encoding('UTF-8') - @universe = universe - @metadata = metadata - - if path.include?("\0") - raise ArgumentError, 'Path contains zero byte character!' - end - - unless path.valid_encoding? - raise ArgumentError, 'Path contains non-UTF-8 byte sequence!' - end - end - - def directory? - blank_node? || @path.end_with?('/') - end - - def file? - !directory? - end - - def has_parent? - nodes > 0 - end - - def parent - return nil unless has_parent? - new(@path.chomp(basename)) - end - - def basename - (directory? && !blank_node?) ? name + ::File::SEPARATOR : name - end - - def name - @name || @path.split(::File::SEPARATOR).last.to_s - end - - def children - return [] unless directory? - return @children if @children - - child_pattern = %r{^#{Regexp.escape(@path)}[^/]+/?$} - @children = select { |entry| entry =~ child_pattern } - end - - def directories - return [] unless directory? - children.select(&:directory?) - end - - def directories! - return directories unless has_parent? - - dotted_parent = parent - dotted_parent.name = '..' - directories.prepend(dotted_parent) - end - - def files - return [] unless directory? - children.select(&:file?) - end - - def metadata - @index ||= @universe.index(@path) - @metadata[@index] || {} - end - - def nodes - @path.count('/') + (file? ? 1 : 0) - end - - def exists? - blank_node? || @universe.include?(@path) - end - - def blank_node? - @path.empty? # "" is considered to be './' - end - - def to_s - @path - end - - def ==(other) - @path == other.path && @universe == other.universe - end - - def inspect - "#{self.class.name}: #{@path}" - end - - private - - def new(path) - self.class.new(path, @universe, @metadata) - end - - def select - selected = @universe.select { |entry| yield entry } - selected.map { |path| new(path) } - end - end - end - end -end diff --git a/spec/lib/gitlab/ci/build/artifacts/metadata/entry_spec.rb b/spec/lib/gitlab/ci/build/artifacts/metadata/entry_spec.rb new file mode 100644 index 00000000000..a728227f87c --- /dev/null +++ b/spec/lib/gitlab/ci/build/artifacts/metadata/entry_spec.rb @@ -0,0 +1,170 @@ +require 'spec_helper' + +describe Gitlab::Ci::Build::Artifacts::Metadata::Entry do + let(:entries) do + ['path/', + 'path/dir_1/', + 'path/dir_1/file_1', + 'path/dir_1/file_b', + 'path/dir_1/subdir/', + 'path/dir_1/subdir/subfile', + 'path/second_dir', + 'path/second_dir/dir_3/file_2', + 'path/second_dir/dir_3/file_3', + 'another_directory/', + 'another_file', + '/file/with/absolute_path'] + end + + def path(example) + string_path(example.metadata[:path]) + end + + def string_path(string_path) + described_class.new(string_path, entries) + end + + describe '/file/with/absolute_path', path: '/file/with/absolute_path' do + subject { |example| path(example) } + + it { is_expected.to be_file } + it { is_expected.to have_parent } + + describe '#basename' do + subject { |example| path(example).basename } + it { is_expected.to eq 'absolute_path' } + end + end + + describe 'path/dir_1/', path: 'path/dir_1/' do + subject { |example| path(example) } + it { is_expected.to have_parent } + it { is_expected.to be_directory } + + describe '#basename' do + subject { |example| path(example).basename } + it { is_expected.to eq 'dir_1/' } + end + + describe '#name' do + subject { |example| path(example).name } + it { is_expected.to eq 'dir_1' } + end + + describe '#parent' do + subject { |example| path(example).parent } + it { is_expected.to eq string_path('path/') } + end + + describe '#children' do + subject { |example| path(example).children } + + it { is_expected.to all(be_an_instance_of described_class) } + it do + is_expected.to contain_exactly string_path('path/dir_1/file_1'), + string_path('path/dir_1/file_b'), + string_path('path/dir_1/subdir/') + end + end + + describe '#files' do + subject { |example| path(example).files } + + it { is_expected.to all(be_file) } + it { is_expected.to all(be_an_instance_of described_class) } + it do + is_expected.to contain_exactly string_path('path/dir_1/file_1'), + string_path('path/dir_1/file_b') + end + end + + describe '#directories' do + context 'without options' do + subject { |example| path(example).directories } + + it { is_expected.to all(be_directory) } + it { is_expected.to all(be_an_instance_of described_class) } + it { is_expected.to contain_exactly string_path('path/dir_1/subdir/') } + end + + context 'with option parent: true' do + subject { |example| path(example).directories(parent: true) } + + it { is_expected.to all(be_directory) } + it { is_expected.to all(be_an_instance_of described_class) } + it do + is_expected.to contain_exactly string_path('path/dir_1/subdir/'), + string_path('path/') + end + end + + describe '#nodes' do + subject { |example| path(example).nodes } + it { is_expected.to eq 2 } + end + + describe '#exists?' do + subject { |example| path(example).exists? } + it { is_expected.to be true } + end + + describe '#empty?' do + subject { |example| path(example).empty? } + it { is_expected.to be false } + end + end + end + + describe 'empty path', path: '' do + subject { |example| path(example) } + it { is_expected.to_not have_parent } + + describe '#children' do + subject { |example| path(example).children } + it { expect(subject.count).to eq 3 } + end + + end + + describe 'path/dir_1/subdir/subfile', path: 'path/dir_1/subdir/subfile' do + describe '#nodes' do + subject { |example| path(example).nodes } + it { is_expected.to eq 4 } + end + end + + describe 'non-existent/', path: 'non-existent/' do + describe '#empty?' do + subject { |example| path(example).empty? } + it { is_expected.to be true } + end + + describe '#exists?' do + subject { |example| path(example).exists? } + it { is_expected.to be false } + end + end + + describe 'another_directory/', path: 'another_directory/' do + describe '#empty?' do + subject { |example| path(example).empty? } + it { is_expected.to be true } + end + end + + describe '#metadata' do + let(:entries) do + ['path/', 'path/file1', 'path/file2'] + end + + let(:metadata) do + [{ name: '/path/' }, { name: '/path/file1' }, { name: '/path/file2' }] + end + + subject do + described_class.new('path/file1', entries, metadata).metadata[:name] + end + + it { is_expected.to eq '/path/file1' } + end +end diff --git a/spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb b/spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb deleted file mode 100644 index 3b254c3ce2f..00000000000 --- a/spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb +++ /dev/null @@ -1,148 +0,0 @@ -require 'spec_helper' - -describe Gitlab::Ci::Build::Artifacts::Metadata::Path do - let(:universe) do - ['path/', - 'path/dir_1/', - 'path/dir_1/file_1', - 'path/dir_1/file_b', - 'path/dir_1/subdir/', - 'path/dir_1/subdir/subfile', - 'path/second_dir', - 'path/second_dir/dir_3/file_2', - 'path/second_dir/dir_3/file_3', - 'another_directory/', - 'another_file', - '/file/with/absolute_path'] - end - - def path(example) - string_path(example.metadata[:path]) - end - - def string_path(string_path) - described_class.new(string_path, universe) - end - - describe '/file/with/absolute_path', path: '/file/with/absolute_path' do - subject { |example| path(example) } - - it { is_expected.to be_file } - it { is_expected.to have_parent } - - describe '#basename' do - subject { |example| path(example).basename } - it { is_expected.to eq 'absolute_path' } - end - end - - describe 'path/dir_1/', path: 'path/dir_1/' do - subject { |example| path(example) } - it { is_expected.to have_parent } - it { is_expected.to be_directory } - - describe '#basename' do - subject { |example| path(example).basename } - it { is_expected.to eq 'dir_1/' } - end - - describe '#name' do - subject { |example| path(example).name } - it { is_expected.to eq 'dir_1' } - end - - describe '#parent' do - subject { |example| path(example).parent } - it { is_expected.to eq string_path('path/') } - end - - describe '#children' do - subject { |example| path(example).children } - - it { is_expected.to all(be_an_instance_of described_class) } - it do - is_expected.to contain_exactly string_path('path/dir_1/file_1'), - string_path('path/dir_1/file_b'), - string_path('path/dir_1/subdir/') - end - end - - describe '#files' do - subject { |example| path(example).files } - - it { is_expected.to all(be_file) } - it { is_expected.to all(be_an_instance_of described_class) } - it do - is_expected.to contain_exactly string_path('path/dir_1/file_1'), - string_path('path/dir_1/file_b') - end - end - - describe '#directories' do - subject { |example| path(example).directories } - - it { is_expected.to all(be_directory) } - it { is_expected.to all(be_an_instance_of described_class) } - it { is_expected.to contain_exactly string_path('path/dir_1/subdir/') } - end - - describe '#directories!' do - subject { |example| path(example).directories! } - - it { is_expected.to all(be_directory) } - it { is_expected.to all(be_an_instance_of described_class) } - it do - is_expected.to contain_exactly string_path('path/dir_1/subdir/'), - string_path('path/') - end - end - end - - describe 'empty path', path: '' do - subject { |example| path(example) } - it { is_expected.to_not have_parent } - - describe '#children' do - subject { |example| path(example).children } - it { expect(subject.count).to eq 3 } - end - end - - describe '#nodes', path: 'test' do - subject { |example| path(example).nodes } - it { is_expected.to eq 1 } - end - - describe '#nodes', path: 'test/' do - subject { |example| path(example).nodes } - it { is_expected.to eq 1 } - end - - describe '#metadata' do - let(:universe) do - ['path/', 'path/file1', 'path/file2'] - end - - let(:metadata) do - [{ name: '/path/' }, { name: '/path/file1' }, { name: '/path/file2' }] - end - - subject do - described_class.new('path/file1', universe, metadata).metadata[:name] - end - - it { is_expected.to eq '/path/file1' } - end - - describe '#exists?', path: 'another_file' do - subject { |example| path(example).exists? } - it { is_expected.to be true } - end - - describe '#exists?', path: './non_existent/' do - let(:universe) { ['./something'] } - subject { |example| path(example).exists? } - - it { is_expected.to be false } - end -end diff --git a/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb b/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb index 5c1a94974e8..42fbe40c890 100644 --- a/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb +++ b/spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb @@ -51,9 +51,9 @@ describe Gitlab::Ci::Build::Artifacts::Metadata do end end - describe '#to_path' do - subject { metadata('').to_path } - it { is_expected.to be_an_instance_of(Gitlab::Ci::Build::Artifacts::Metadata::Path) } + describe '#to_entry' do + subject { metadata('').to_entry } + it { is_expected.to be_an_instance_of(Gitlab::Ci::Build::Artifacts::Metadata::Entry) } end describe '#full_version' do -- cgit v1.2.1