Simplify implementation of build artifacts browser (refactoring)

app/controllers/projects/artifacts_controller.rb

@@ -16,10 +16,16 @@ class Projects::ArtifactsController < Projects::ApplicationController
 
   def browse
     return render_404 unless build.artifacts?
-    @path = build.artifacts_metadata_string_path(params[:path] || './')
+    @path = build.artifacts_metadata_path(params[:path].to_s)
     return render_404 unless @path.exists?
   end
 
+  def file
+    # TODO, check if file exists in metadata
+    render json: { repository: build.artifacts_file.path,
+                   path: Base64.encode64(params[:path].to_s) }
+  end
+
   private
 
   def build

app/models/ci/build.rb

@@ -338,21 +338,19 @@ module Ci
     end
 
     def artifacts_browse_url
-      if artifacts?
+      if artifacts_browser_supported?
         browse_namespace_project_build_artifacts_path(project.namespace, project, self)
       end
     end
 
     def artifacts_browser_supported?
-      # TODO, since carrierwave 0.10.0 we will be able to check mime type here
-      #
       artifacts? && artifacts_file.path.end_with?('zip') && artifacts_metadata.exists?
     end
 
 
-    def artifacts_metadata_string_path(path)
-      file = artifacts_metadata.path
-      Gitlab::Ci::Build::Artifacts::Metadata.new(file, path).to_string_path
+    def artifacts_metadata_path(path)
+      metadata_file = artifacts_metadata.path
+      Gitlab::Ci::Build::Artifacts::Metadata.new(metadata_file, path).to_path
     end
 
     private

config/routes.rb

@@ -611,6 +611,7 @@ Rails.application.routes.draw do
           resource :artifacts, only: [] do
             get :download
             get 'browse(/*path)', action: :browse, as: :browse, format: false
+            get 'file/*path', action: :file, as: :file, format: false
           end
         end
 

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

@@ -6,65 +6,54 @@ module Gitlab
     module Build
       module Artifacts
         class Metadata
-          def initialize(file, path)
-            @file = file
-
-            @path = path.sub(/^\.\//, '')
-            @path << '/' unless path.end_with?('/')
-          end
-
-          def exists?
-            File.exists?(@file)
-          end
+          VERSION_PATTERN = '[\w\s]+(\d+\.\d+\.\d+)'
+          attr_reader :file, :path, :full_version
 
-          def full_version
-            gzip do|gz|
-              read_string(gz) do |size|
-                raise StandardError, 'Artifacts metadata file empty!' unless size
-              end
-            end
+          def initialize(file, path)
+            @file, @path = file, path
+            @full_version = read_version
+            @path << '/' unless path.end_with?('/') || path.empty?
           end
 
           def version
-            full_version.match(/\w+ (\d+\.\d+\.\d+)/).captures.first
+            @full_version.match(/#{VERSION_PATTERN}/).captures.first
           end
 
           def errors
             gzip do|gz|
               read_string(gz) # version
-              JSON.parse(read_string(gz))
+              errors = read_string(gz)
+              raise StandardError, 'Errors field not found!' unless errors
+              JSON.parse(errors)
             end
           end
 
           def match!
-            raise StandardError, 'Metadata file not found !' unless exists?
-
             gzip do |gz|
-              read_string(gz) # version field
-              read_string(gz) # errors field
-              iterate_entries(gz)
+              2.times { read_string(gz) } # version and errors fields
+              match_entries(gz)
             end
           end
 
-          def to_string_path
-            universe, metadata = match!
-            ::Gitlab::StringPath.new(@path, universe, metadata)
+          def to_path
+            Path.new(@path, *match!)
           end
 
           private
 
-          def iterate_entries(gz)
+          def match_entries(gz)
             paths, metadata = [], []
+            child_pattern = %r{^#{Regexp.escape(@path)}[^/\s]*/?$}
 
             until gz.eof? do
               begin
                 path = read_string(gz)
                 meta = read_string(gz)
                
-                next unless path =~ %r{^#{Regexp.escape(@path)}[^/\s]*/?$}
+                next unless path =~ child_pattern
 
                 paths.push(path)
-                metadata.push(JSON.parse(meta, symbolize_names: true))
+                metadata.push(JSON.parse(meta.chomp, symbolize_names: true))
               rescue JSON::ParserError
                 next
               end
@@ -73,16 +62,31 @@ module Gitlab
             [paths, metadata]
           end
 
-          def read_string_size(gz)
+          def read_version
+            gzip do|gz|
+              version_string = read_string(gz)
+
+              unless version_string
+                raise StandardError, 'Artifacts metadata file empty!'
+              end
+
+              unless version_string =~ /^#{VERSION_PATTERN}/
+                raise StandardError, 'Invalid version!'
+              end
+
+              version_string.chomp
+            end
+          end
+
+          def read_uint32(gz)
             binary = gz.read(4)
             binary.unpack('L>')[0] if binary
           end
 
           def read_string(gz)
-            string_size = read_string_size(gz)
-            yield string_size if block_given?
+            string_size = read_uint32(gz)
             return false unless string_size
-            gz.read(string_size).chomp
+            gz.read(string_size)
           end
 
           def gzip

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

+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.
+      #
+      class Path
+        attr_reader :path, :universe
+        attr_accessor :name
+
+        def initialize(path, universe, metadata = [])
+          @path = path
+          @universe = universe
+          @metadata = metadata
+
+          if path.include?("\0")
+            raise ArgumentError, 'Path contains zero byte character!'
+          end
+        end
+
+        def directory?
+          @path.end_with?('/') || @path.blank?
+        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? ? name + ::File::SEPARATOR : name
+        end
+
+        def name
+          @name || @path.split(::File::SEPARATOR).last
+        end
+
+        def children
+          return [] unless directory?
+          return @children if @children
+
+          child_pattern = %r{^#{Regexp.escape(@path)}[^/\s]+/?$}
+          @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?
+          @path.blank? || @universe.include?(@path)
+        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

lib/gitlab/string_path.rb

-module Gitlab
-  ## 
-  # Class that represents a simplified path to a file or directory
-  #
-  # This is IO-operations safe class, that does similar job to 
-  # Ruby's Pathname but without the risk of accessing filesystem.
-  #
-  class StringPath
-    attr_reader :path, :universe
-    attr_accessor :name
-
-    def initialize(path, universe, metadata = [])
-      @path = sanitize(path)
-      @universe = universe.map { |entry| sanitize(entry) }
-      @metadata = metadata
-    end
-
-    def to_s
-      @path
-    end
-
-    def absolute?
-      @path.start_with?('/')
-    end
-
-    def relative?
-      !absolute?
-    end
-
-    def directory?
-      @path.end_with?('/')
-    end
-
-    def file?
-      !directory?
-    end
-
-    def has_parent?
-      nodes > 1
-    end
-
-    def parent
-      return nil unless has_parent?
-      new(@path.sub(basename, ''))
-    end
-
-    def basename
-      directory? ? name + ::File::SEPARATOR : name
-    end
-
-    def name
-      @name || @path.split(::File::SEPARATOR).last
-    end
-
-    def has_descendants?
-      descendants.any?
-    end
-
-    def descendants
-      return [] unless directory?
-      select { |entry| entry =~ /^#{Regexp.escape(@path)}.+/ }
-    end
-
-    def children
-      return [] unless directory?
-      return @children if @children
-
-      @children = select do |entry|
-        entry =~ %r{^#{Regexp.escape(@path)}[^/\s]+/?$}
-      end
-    end
-
-    def directories
-      return [] unless directory?
-      children.select(&:directory?)
-    end
-
-    def directories!
-      return directories unless has_parent? && directory?
-
-      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?
-      @path == './' || @universe.include?(@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
-
-    def sanitize(path)
-      self.class.sanitize(path)
-    end
-
-    def self.sanitize(path)
-      # It looks like Pathname#new doesn't touch a file system,
-      # neither Pathname#cleanpath does, so it is, hopefully, filesystem safe
-
-      clean_path = Pathname.new(path).cleanpath.to_s
-      raise ArgumentError, 'Invalid path' if clean_path.start_with?('../')
-
-      prefix = './' unless clean_path =~ %r{^[\.|/]}
-      suffix = '/' if path.end_with?('/') || ['.', '..'].include?(clean_path)
-      prefix.to_s + clean_path + suffix.to_s
-    end
-  end
-end

spec/lib/gitlab/string_path_spec.rb → spec/lib/gitlab/ci/build/artifacts/metadata/path_spec.rb

 require 'spec_helper'
 
-describe Gitlab::StringPath do
+describe Gitlab::Ci::Build::Artifacts::Metadata::Path do
   let(:universe) do
     ['path/',
      'path/dir_1/',
@@ -27,30 +27,19 @@ describe Gitlab::StringPath do
   describe '/file/with/absolute_path', path: '/file/with/absolute_path' do
     subject { |example| path(example) }
 
-    it { is_expected.to be_absolute } 
-    it { is_expected.to_not be_relative }
     it { is_expected.to be_file }
     it { is_expected.to have_parent }
-    it { is_expected.to_not have_descendants }
-    it { is_expected.to exist }
 
     describe '#basename' do
       subject { |example| path(example).basename }
-
       it { is_expected.to eq 'absolute_path' }
     end
   end
 
-  describe 'path/', path: 'path/' do
-    subject { |example| path(example) }
-
-    it { is_expected.to be_directory }
-    it { is_expected.to be_relative }
-  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 }
@@ -67,19 +56,6 @@ describe Gitlab::StringPath do
       it { is_expected.to eq string_path('path/') }
     end
 
-    describe '#descendants' do
-      subject { |example| path(example).descendants }
-
-      it { is_expected.to be_an_instance_of Array }
-      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/'),
-                                       string_path('path/dir_1/subdir/subfile')
-      end
-    end
-
     describe '#children' do
       subject { |example| path(example).children }
 
@@ -117,23 +93,14 @@ describe Gitlab::StringPath do
       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/dir_1/../')
+                                       string_path('path/')
       end
     end
   end
 
-  describe './', path: './' do
+  describe 'empty path', path: '' do
     subject { |example| path(example) }
-
     it { is_expected.to_not have_parent }
-    it { is_expected.to have_descendants }
-
-    describe '#descendants' do
-      subject { |example| path(example).descendants }
-
-      it { expect(subject.count).to eq universe.count - 1 }
-      it { is_expected.to_not include string_path('./') }
-    end
 
     describe '#children' do
       subject { |example| path(example).children }
@@ -141,11 +108,6 @@ describe Gitlab::StringPath do
     end
   end
 
-  describe '#nodes', path: './' 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 2 }

spec/lib/gitlab/ci/build/artifacts/metadata_spec.rb

@@ -10,13 +10,8 @@ describe Gitlab::Ci::Build::Artifacts::Metadata do
   end
 
   context 'metadata file exists' do
-    describe '#exists?' do
-      subject { metadata.exists? }
-      it { is_expected.to be true }
-    end
-
-    describe '#match! ./' do
-      subject { metadata('./').match! }
+    describe '#match! empty string' do
+      subject { metadata('').match! }
 
       it 'matches correct paths' do
         expect(subject.first).to contain_exactly 'ci_artifacts.txt',
@@ -55,9 +50,9 @@ describe Gitlab::Ci::Build::Artifacts::Metadata do
       end
     end
 
-    describe '#to_string_path' do
-      subject { metadata('').to_string_path }
-      it { is_expected.to be_an_instance_of(Gitlab::StringPath) }
+    describe '#to_path' do
+      subject { metadata('').to_path }
+      it { is_expected.to be_an_instance_of(Gitlab::Ci::Build::Artifacts::Metdata::Path) }
     end
 
     describe '#full_version' do
@@ -79,14 +74,9 @@ describe Gitlab::Ci::Build::Artifacts::Metadata do
   context 'metadata file does not exist' do
     let(:metadata_file_path) { '' }
 
-    describe '#exists?' do
-      subject { metadata.exists? }
-      it { is_expected.to be false }
-    end
-
     describe '#match!' do
       it 'raises error' do
-        expect { metadata.match! }.to raise_error(StandardError, /Metadata file not found/)
+        expect { metadata.match! }.to raise_error(Errno::ENOENT)
       end
     end
   end