Merge branch '18590-banzai-filter-relativelinkfilter-is-slow' into 'master'

Optimize Banzai::Filter::RelativeLinkFilter

See merge request !4813

5 files changed, 55 insertions, 42 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 482696431b7..98817834e3a 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -9,6 +9,7 @@ v 8.9.0 (unreleased)
   - Bulk assign/unassign labels to issues.
   - Ability to prioritize labels !4009 / !3205 (Thijs Wouters)
   - Show Star and Fork buttons on mobile.
+  - Performance improvements on RelativeLinkFilter
   - Fix endless redirections when accessing user OAuth applications when they are disabled
   - Allow enabling wiki page events from Webhook management UI
   - Bump rouge to 1.11.0
diff --git a/app/models/commit.rb b/app/models/commit.rb
index d69d518fadd..174ccbaea6c 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -271,6 +271,32 @@ class Commit
     merged_merge_request ? 'merge request' : 'commit'
   end
 
+  # Get the URI type of the given path
+  #
+  # Used to build URLs to files in the repository in GFM.
+  #
+  # path - String path to check
+  #
+  # Examples:
+  #
+  #   uri_type('doc/README.md') # => :blob
+  #   uri_type('doc/logo.png')  # => :raw
+  #   uri_type('doc/api')       # => :tree
+  #   uri_type('not/found')     # => :nil
+  #
+  # Returns a symbol
+  def uri_type(path)
+    entry = @raw.tree.path(path)
+    if entry[:type] == :blob
+      blob = Gitlab::Git::Blob.new(name: entry[:name])
+      blob.image? ? :raw : :blob
+    else
+      entry[:type]
+    end
+  rescue Rugged::TreeError
+    nil
+  end
+
   private
 
   def repo_changes
diff --git a/lib/banzai/filter/relative_link_filter.rb b/lib/banzai/filter/relative_link_filter.rb
index ea21c7b041c..c78da404607 100644
--- a/lib/banzai/filter/relative_link_filter.rb
+++ b/lib/banzai/filter/relative_link_filter.rb
@@ -14,6 +14,8 @@ module Banzai
       def call
         return doc unless linkable_files?
 
+        @uri_types = {}
+
         doc.search('a:not(.gfm)').each do |el|
           process_link_attr el.attribute('href')
         end
@@ -48,7 +50,7 @@ module Banzai
         uri.path = [
           relative_url_root,
           context[:project].path_with_namespace,
-          path_type(file_path),
+          uri_type(file_path),
           ref || context[:project].default_branch,  # if no ref exists, point to the default branch
           file_path
         ].compact.join('/').squeeze('/').chomp('/')
@@ -87,7 +89,7 @@ module Banzai
         return path unless request_path
 
         parts = request_path.split('/')
-        parts.pop if path_type(request_path) != 'tree'
+        parts.pop if uri_type(request_path) != :tree
 
         while path.start_with?('../')
           parts.pop
@@ -98,45 +100,20 @@ module Banzai
       end
 
       def file_exists?(path)
-        return false if path.nil?
-        repository.blob_at(current_sha, path).present? ||
-          repository.tree(current_sha, path).entries.any?
-      end
-
-      # Get the type of the given path
-      #
-      # path - String path to check
-      #
-      # Examples:
-      #
-      #   path_type('doc/README.md') # => 'blob'
-      #   path_type('doc/logo.png')  # => 'raw'
-      #   path_type('doc/api')       # => 'tree'
-      #
-      # Returns a String
-      def path_type(path)
-        unescaped_path = Addressable::URI.unescape(path)
-
-        if tree?(unescaped_path)
-          'tree'
-        elsif image?(unescaped_path)
-          'raw'
-        else
-          'blob'
-        end
+        path.present? && !!uri_type(path)
       end
 
-      def tree?(path)
-        repository.tree(current_sha, path).entries.any?
-      end
+      def uri_type(path)
+        @uri_types[path] ||= begin
+          unescaped_path = Addressable::URI.unescape(path)
 
-      def image?(path)
-        repository.blob_at(current_sha, path).try(:image?)
+          current_commit.uri_type(unescaped_path)
+        end
       end
 
-      def current_sha
-        context[:commit].try(:id) ||
-          ref ? repository.commit(ref).try(:sha) : repository.head_commit.sha
+      def current_commit
+        @current_commit ||= context[:commit] ||
+          ref ? repository.commit(ref) : repository.head_commit
       end
 
       def relative_url_root
@@ -148,7 +125,7 @@ module Banzai
       end
 
       def repository
-        context[:project].try(:repository)
+        @repository ||= context[:project].try(:repository)
       end
     end
   end
diff --git a/spec/lib/banzai/filter/relative_link_filter_spec.rb b/spec/lib/banzai/filter/relative_link_filter_spec.rb
index 0e6685f0ffb..b9e4a4eaf0e 100644
--- a/spec/lib/banzai/filter/relative_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/relative_link_filter_spec.rb
@@ -132,11 +132,8 @@ describe Banzai::Filter::RelativeLinkFilter, lib: true do
       path = 'files/images/한글.png'
       escaped = Addressable::URI.escape(path)
 
-      # Stub these methods so the file doesn't actually need to be in the repo
-      allow_any_instance_of(described_class).
-        to receive(:file_exists?).and_return(true)
-      allow_any_instance_of(described_class).
-        to receive(:image?).with(path).and_return(true)
+      # Stub this method so the file doesn't actually need to be in the repo
+      allow_any_instance_of(described_class).to receive(:uri_type).and_return(:raw)
 
       doc = filter(image(escaped))
       expect(doc.at_css('img')['src']).to match '/raw/'
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index beca8708c9d..ba02d5fe977 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -207,4 +207,16 @@ eos
       expect(commit.participants).to include(note1.author, note2.author)
     end
   end
+
+  describe '#uri_type' do
+    it 'returns the URI type at the given path' do
+      expect(commit.uri_type('files/html')).to be(:tree)
+      expect(commit.uri_type('files/images/logo-black.png')).to be(:raw)
+      expect(commit.uri_type('files/js/application.js')).to be(:blob)
+    end
+
+    it "returns nil if the path doesn't exists" do
+      expect(commit.uri_type('this/path/doesnt/exist')).to be_nil
+    end
+  end
 end