Avoid Gitaly calls when a document has only upload links

Now that we have the UploadLinkFilter, the RepositoryLinkFilter should
be a no-op (in particular, making no Gitaly calls) when it doesn't have
anything to do. To ensure that happens:

1. Remove upload handling from RepositoryLinkFilter.
2. Add a spec to ensure that this case generates no Gitaly calls.
3. Move common code to a mixin.

changelogs/unreleased/split-up-relativelinkfilter.yml

+---
+title: Avoid making Gitaly calls when some Markdown text links to an uploaded file
+merge_request: 22631
+author:
+type: performance

lib/banzai/filter/base_relative_link_filter.rb

+# frozen_string_literal: true
+
+require 'uri'
+
+module Banzai
+  module Filter
+    class BaseRelativeLinkFilter < HTML::Pipeline::Filter
+      include Gitlab::Utils::StrongMemoize
+
+      protected
+
+      def linkable_attributes
+        strong_memoize(:linkable_attributes) do
+          attrs = []
+
+          attrs += doc.search('a:not(.gfm)').map do |el|
+            el.attribute('href')
+          end
+
+          attrs += doc.search('img:not(.gfm), video:not(.gfm), audio:not(.gfm)').flat_map do |el|
+            [el.attribute('src'), el.attribute('data-src')]
+          end
+
+          attrs.reject do |attr|
+            attr.blank? || attr.value.start_with?('//')
+          end
+        end
+      end
+
+      def relative_url_root
+        Gitlab.config.gitlab.relative_url_root.presence || '/'
+      end
+
+      def project
+        context[:project]
+      end
+
+      private
+
+      def unescape_and_scrub_uri(uri)
+        Addressable::URI.unescape(uri).scrub
+      end
+    end
+  end
+end

lib/banzai/filter/repository_link_filter.rb

@@ -8,15 +8,13 @@ module Banzai
     #
     # Context options:
     #   :commit
-    #   :group
     #   :current_user
     #   :project
     #   :project_wiki
     #   :ref
     #   :requested_path
-    class RepositoryLinkFilter < HTML::Pipeline::Filter
-      include Gitlab::Utils::StrongMemoize
-
+    #   :system_note
+    class RepositoryLinkFilter < BaseRelativeLinkFilter
       def call
         return doc if context[:system_note]
 
@@ -26,7 +24,9 @@ module Banzai
         load_uri_types
 
         linkable_attributes.each do |attr|
-          process_link_attr(attr)
+          if linkable_files? && repo_visible_to_user?
+            process_link_to_repository_attr(attr)
+          end
         end
 
         doc
@@ -35,8 +35,8 @@ module Banzai
       protected
 
       def load_uri_types
-        return unless linkable_files?
         return unless linkable_attributes.present?
+        return unless linkable_files?
         return {} unless repository
 
         @uri_types = request_path.present? ? get_uri_types([request_path]) : {}
@@ -57,24 +57,6 @@ module Banzai
         end
       end
 
-      def linkable_attributes
-        strong_memoize(:linkable_attributes) do
-          attrs = []
-
-          attrs += doc.search('a:not(.gfm)').map do |el|
-            el.attribute('href')
-          end
-
-          attrs += doc.search('img:not(.gfm), video:not(.gfm), audio:not(.gfm)').flat_map do |el|
-            [el.attribute('src'), el.attribute('data-src')]
-          end
-
-          attrs.reject do |attr|
-            attr.blank? || attr.value.start_with?('//')
-          end
-        end
-      end
-
       def get_uri_types(paths)
         return {} if paths.empty?
 
@@ -107,39 +89,6 @@ module Banzai
       rescue URI::Error, Addressable::URI::InvalidURIError
       end
 
-      def process_link_attr(html_attr)
-        if html_attr.value.start_with?('/uploads/')
-          process_link_to_upload_attr(html_attr)
-        elsif linkable_files? && repo_visible_to_user?
-          process_link_to_repository_attr(html_attr)
-        end
-      end
-
-      def process_link_to_upload_attr(html_attr)
-        path_parts = [unescape_and_scrub_uri(html_attr.value)]
-
-        if project
-          path_parts.unshift(relative_url_root, project.full_path)
-        elsif group
-          path_parts.unshift(relative_url_root, 'groups', group.full_path, '-')
-        else
-          path_parts.unshift(relative_url_root)
-        end
-
-        begin
-          path = Addressable::URI.escape(File.join(*path_parts))
-        rescue Addressable::URI::InvalidURIError
-          return
-        end
-
-        html_attr.value =
-          if context[:only_path]
-            path
-          else
-            Addressable::URI.join(Gitlab.config.gitlab.base_url, path).to_s
-          end
-      end
-
       def process_link_to_repository_attr(html_attr)
         uri = URI(html_attr.value)
 
@@ -239,10 +188,6 @@ module Banzai
         @current_commit ||= context[:commit] || repository.commit(ref)
       end
 
-      def relative_url_root
-        Gitlab.config.gitlab.relative_url_root.presence || '/'
-      end
-
       def repo_visible_to_user?
         project && Ability.allowed?(current_user, :download_code, project)
       end
@@ -251,14 +196,6 @@ module Banzai
         context[:ref] || project.default_branch
       end
 
-      def group
-        context[:group]
-      end
-
-      def project
-        context[:project]
-      end
-
       def current_user
         context[:current_user]
       end
@@ -266,12 +203,6 @@ module Banzai
       def repository
         @repository ||= project&.repository
       end
-
-      private
-
-      def unescape_and_scrub_uri(uri)
-        Addressable::URI.unescape(uri).scrub
-      end
     end
   end
 end

lib/banzai/filter/upload_link_filter.rb

@@ -11,17 +11,12 @@ module Banzai
     #   :only_path
     #   :project
     #   :system_note
-    class UploadLinkFilter < HTML::Pipeline::Filter
-      include Gitlab::Utils::StrongMemoize
-
+    class UploadLinkFilter < BaseRelativeLinkFilter
       def call
         return doc if context[:system_note]
 
         linkable_attributes.each do |attr|
-          if attr.value.start_with?('/uploads/')
-            process_link_to_upload_attr(attr)
-            attr.parent.add_class('gfm')
-          end
+          process_link_to_upload_attr(attr)
         end
 
         doc
@@ -29,25 +24,9 @@ module Banzai
 
       protected
 
-      def linkable_attributes
-        strong_memoize(:linkable_attributes) do
-          attrs = []
-
-          attrs += doc.search('a:not(.gfm)').map do |el|
-            el.attribute('href')
-          end
-
-          attrs += doc.search('img:not(.gfm), video:not(.gfm), audio:not(.gfm)').flat_map do |el|
-            [el.attribute('src'), el.attribute('data-src')]
-          end
-
-          attrs.reject do |attr|
-            attr.blank? || attr.value.start_with?('//')
-          end
-        end
-      end
-
       def process_link_to_upload_attr(html_attr)
+        return unless html_attr.value.start_with?('/uploads/')
+
         path_parts = [unescape_and_scrub_uri(html_attr.value)]
 
         if project
@@ -70,25 +49,13 @@ module Banzai
           else
             Addressable::URI.join(Gitlab.config.gitlab.base_url, path).to_s
           end
-      end
 
-      def relative_url_root
-        Gitlab.config.gitlab.relative_url_root.presence || '/'
+        html_attr.parent.add_class('gfm')
       end
 
       def group
         context[:group]
       end
-
-      def project
-        context[:project]
-      end
-
-      private
-
-      def unescape_and_scrub_uri(uri)
-        Addressable::URI.unescape(uri).scrub
-      end
     end
   end
 end

spec/lib/banzai/pipeline/post_process_pipeline_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Banzai::Pipeline::PostProcessPipeline do
+  context 'when a document only has upload links' do
+    it 'does not make any Gitaly calls', :request_store do
+      markdown = <<-MARKDOWN.strip_heredoc
+        [Relative Upload Link](/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg)
+
+        ![Relative Upload Image](/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg)
+      MARKDOWN
+
+      context = {
+        project: create(:project, :public, :repository),
+        ref: 'master'
+      }
+
+      Gitlab::GitalyClient.reset_counts
+
+      described_class.call(markdown, context)
+
+      expect(Gitlab::GitalyClient.get_request_count).to eq(0)
+    end
+  end
+end