Merge pull request #9276 from jirutka/relative_link_filter

Extract handling of relative file links to its own HTML filter

app/helpers/gitlab_markdown_helper.rb

@@ -72,146 +72,6 @@ module GitlabMarkdownHelper
     end
   end
 
-  # TODO (rspeicher): This should be its own filter
-  def create_relative_links(text)
-    paths = extract_paths(text)
-
-    paths.uniq.each do |file_path|
-      # If project does not have repository
-      # its nothing to rebuild
-      #
-      # TODO: pass project variable to markdown helper instead of using
-      # instance variable. Right now it generates invalid path for pages out
-      # of project scope. Example: search results where can be rendered markdown
-      # from different projects
-      if @repository && @repository.exists? && !@repository.empty?
-        new_path = rebuild_path(file_path)
-        # Finds quoted path so we don't replace other mentions of the string
-        # eg. "doc/api" will be replaced and "/home/doc/api/text" won't
-        text.gsub!("\"#{file_path}\"", "\"/#{new_path}\"")
-      end
-    end
-
-    text
-  end
-
-  def extract_paths(text)
-    links = substitute_links(text)
-    image_links = substitute_image_links(text)
-    links + image_links
-  end
-
-  def substitute_links(text)
-    links = text.scan(/<a href=\"([^"]*)\">/)
-    relative_links = links.flatten.reject{ |link| link_to_ignore? link }
-    relative_links
-  end
-
-  def substitute_image_links(text)
-    links = text.scan(/<img src=\"([^"]*)\"/)
-    relative_links = links.flatten.reject{ |link| link_to_ignore? link }
-    relative_links
-  end
-
-  def link_to_ignore?(link)
-    if link =~ /\A\#\w+/
-      # ignore anchors like <a href="#my-header">
-      true
-    else
-      ignored_protocols.map{ |protocol| link.include?(protocol) }.any?
-    end
-  end
-
-  def ignored_protocols
-    ["http://","https://", "ftp://", "mailto:", "smb://"]
-  end
-
-  def rebuild_path(file_path)
-    file_path = file_path.dup
-    file_path.gsub!(/(#.*)/, "")
-    id = $1 || ""
-    file_path = relative_file_path(file_path)
-    file_path = sanitize_slashes(file_path)
-
-    [
-      Gitlab.config.gitlab.relative_url_root,
-      @project.path_with_namespace,
-      path_with_ref(file_path),
-      file_path
-    ].compact.join("/").gsub(/\A\/*|\/*\z/, '') + id
-  end
-
-  def sanitize_slashes(path)
-    path[0] = "" if path.start_with?("/")
-    path.chop if path.end_with?("/")
-    path
-  end
-
-  def relative_file_path(path)
-    requested_path = @path
-    nested_path = build_nested_path(path, requested_path)
-    return nested_path if file_exists?(nested_path)
-    path
-  end
-
-  # Covering a special case, when the link is referencing file in the same directory eg:
-  # If we are at doc/api/README.md and the README.md contains relative links like [Users](users.md)
-  # this takes the request path(doc/api/README.md), and replaces the README.md with users.md so the path looks like doc/api/users.md
-  # If we are at doc/api and the README.md shown in below the tree view
-  # this takes the request path(doc/api) and adds users.md so the path looks like doc/api/users.md
-  def build_nested_path(path, request_path)
-    return request_path if path == ""
-    return path unless request_path
-    if local_path(request_path) == "tree"
-      base = request_path.split("/").push(path)
-      base.join("/")
-    else
-      base = request_path.split("/")
-      base.pop
-      base.push(path).join("/")
-    end
-  end
-
-  # Checks if the path exists in the repo
-  # eg. checks if doc/README.md exists, if not then link to blob
-  def path_with_ref(path)
-    if file_exists?(path)
-      "#{local_path(path)}/#{correct_ref}"
-    else
-      "blob/#{correct_ref}"
-    end
-  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
-
-  # Check if the path is pointing to a directory(tree) or a file(blob)
-  # eg. doc/api is directory and doc/README.md is file
-  def local_path(path)
-    return "tree" if @repository.tree(current_sha, path).entries.any?
-    return "raw" if @repository.blob_at(current_sha, path).image?
-    "blob"
-  end
-
-  def current_sha
-    if @commit
-      @commit.id
-    elsif @repository && !@repository.empty?
-      if @ref
-        @repository.commit(@ref).try(:sha)
-      else
-        @repository.head_commit.sha
-      end
-    end
-  end
-
-  # We will assume that if no ref exists we can point to master
-  def correct_ref
-    @ref ? @ref : "master"
-  end
-
   private
 
   # Return +text+, truncated to +max_chars+ characters, excluding any HTML

lib/gitlab/markdown.rb

@@ -15,6 +15,7 @@ module Gitlab
     autoload :IssueReferenceFilter,         'gitlab/markdown/issue_reference_filter'
     autoload :LabelReferenceFilter,         'gitlab/markdown/label_reference_filter'
     autoload :MergeRequestReferenceFilter,  'gitlab/markdown/merge_request_reference_filter'
+    autoload :RelativeLinkFilter,           'gitlab/markdown/relative_link_filter'
     autoload :SanitizationFilter,           'gitlab/markdown/sanitization_filter'
     autoload :SnippetReferenceFilter,       'gitlab/markdown/snippet_reference_filter'
     autoload :TableOfContentsFilter,        'gitlab/markdown/table_of_contents_filter'
@@ -64,7 +65,12 @@ module Gitlab
         current_user:    current_user,
         only_path:       options[:reference_only_path],
         project:         project,
-        reference_class: html_options[:class]
+        reference_class: html_options[:class],
+
+        # RelativeLinkFilter
+        ref: @ref,
+        requested_path: @path,
+        project_wiki: @project_wiki
       }
 
       result = pipeline.call(text, context)
@@ -91,6 +97,7 @@ module Gitlab
       [
         Gitlab::Markdown::SanitizationFilter,
 
+        Gitlab::Markdown::RelativeLinkFilter,
         Gitlab::Markdown::EmojiFilter,
         Gitlab::Markdown::TableOfContentsFilter,
         Gitlab::Markdown::AutolinkFilter,

lib/gitlab/markdown/relative_link_filter.rb

+require 'html/pipeline/filter'
+require 'uri'
+
+module Gitlab
+  module Markdown
+    # HTML filter that "fixes" relative links to files in a repository.
+    #
+    # Context options:
+    #   :commit
+    #   :project
+    #   :project_wiki
+    #   :requested_path
+    #   :ref
+    class RelativeLinkFilter < HTML::Pipeline::Filter
+
+      def call
+        if linkable_files?
+          doc.search('a').each do |el|
+            process_link_attr el.attribute('href')
+          end
+
+          doc.search('img').each do |el|
+            process_link_attr el.attribute('src')
+          end
+        end
+
+        doc
+      end
+
+      protected
+
+      def linkable_files?
+        context[:project_wiki].nil? && repository.try(:exists?) && !repository.empty?
+      end
+
+      def process_link_attr(html_attr)
+        return if html_attr.blank?
+
+        uri = URI(html_attr.value)
+        if uri.relative? && uri.path.present?
+          html_attr.value = rebuild_relative_uri(uri).to_s
+        end
+      end
+
+      def rebuild_relative_uri(uri)
+        file_path = relative_file_path(uri.path)
+
+        uri.path = [
+          relative_url_root,
+          context[:project].path_with_namespace,
+          path_type(file_path),
+          ref || 'master',  # assume that if no ref exists we can point to master
+          file_path
+        ].compact.join('/').squeeze('/').chomp('/')
+
+        uri
+      end
+
+      def relative_file_path(path)
+        nested_path = build_nested_path(path, context[:requested_path])
+        file_exists?(nested_path) ? nested_path : path
+      end
+
+      # Covering a special case, when the link is referencing file in the same
+      # directory.
+      # If we are at doc/api/README.md and the README.md contains relative
+      # links like [Users](users.md), this takes the request
+      # path(doc/api/README.md) and replaces the README.md with users.md so the
+      # path looks like doc/api/users.md.
+      # If we are at doc/api and the README.md shown in below the tree view
+      # this takes the request path(doc/api) and adds users.md so the path
+      # looks like doc/api/users.md
+      def build_nested_path(path, request_path)
+        return request_path if path.empty?
+        return path unless request_path
+
+        parts = request_path.split('/')
+        parts.pop if path_type(request_path) != 'tree'
+        parts.push(path).join('/')
+      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
+
+      # Check if the path is pointing to a directory(tree) or a file(blob)
+      # eg. doc/api is directory and doc/README.md is file.
+      def path_type(path)
+        return 'tree' if repository.tree(current_sha, path).entries.any?
+        return 'raw' if repository.blob_at(current_sha, path).try(:image?)
+        'blob'
+      end
+
+      def current_sha
+        context[:commit].try(:id) ||
+          ref ? repository.commit(ref).try(:sha) : repository.head_commit.sha
+      end
+
+      def relative_url_root
+        Gitlab.config.gitlab.relative_url_root.presence || '/'
+      end
+
+      def ref
+        context[:ref]
+      end
+
+      def repository
+        context[:project].try(:repository)
+      end
+    end
+  end
+end

lib/redcarpet/render/gitlab_html.rb

@@ -36,10 +36,6 @@ class Redcarpet::Render::GitlabHTML < Redcarpet::Render::HTML
   end
 
   def postprocess(full_document)
-    unless @template.instance_variable_get("@project_wiki") || @project.nil?
-      full_document = h.create_relative_links(full_document)
-    end
-
     h.gfm_with_options(full_document, @options)
   end
 end

spec/helpers/gitlab_markdown_helper_spec.rb

@@ -96,79 +96,6 @@ describe GitlabMarkdownHelper do
     end
   end
 
-  describe "#markdown" do
-    # TODO (rspeicher): These belong in a relative link filter spec
-    context 'relative links' do
-      context 'with a valid repository' do
-        before do
-          @repository = project.repository
-          @ref = 'markdown'
-        end
-
-        it "should handle relative urls for a file in master" do
-          actual = "[GitLab API doc](doc/api/README.md)\n"
-          expected = "<p><a href=\"/#{project.path_with_namespace}/blob/#{@ref}/doc/api/README.md\">GitLab API doc</a></p>\n"
-          expect(markdown(actual)).to match(expected)
-        end
-
-        it "should handle relative urls for a file in master with an anchor" do
-          actual = "[GitLab API doc](doc/api/README.md#section)\n"
-          expected = "<p><a href=\"/#{project.path_with_namespace}/blob/#{@ref}/doc/api/README.md#section\">GitLab API doc</a></p>\n"
-          expect(markdown(actual)).to match(expected)
-        end
-
-        it "should not handle relative urls for the current file with an anchor" do
-          actual = "[GitLab API doc](#section)\n"
-          expected = "<p><a href=\"#section\">GitLab API doc</a></p>\n"
-          expect(markdown(actual)).to match(expected)
-        end
-
-        it "should handle relative urls for a directory in master" do
-          actual = "[GitLab API doc](doc/api)\n"
-          expected = "<p><a href=\"/#{project.path_with_namespace}/tree/#{@ref}/doc/api\">GitLab API doc</a></p>\n"
-          expect(markdown(actual)).to match(expected)
-        end
-
-        it "should handle absolute urls" do
-          actual = "[GitLab](https://www.gitlab.com)\n"
-          expected = "<p><a href=\"https://www.gitlab.com\">GitLab</a></p>\n"
-          expect(markdown(actual)).to match(expected)
-        end
-
-        it "should handle relative urls in reference links for a file in master" do
-          actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n"
-          expected = "<p><a href=\"/#{project.path_with_namespace}/blob/#{@ref}/doc/api/README.md\">GitLab API doc</a></p>\n"
-          expect(markdown(actual)).to match(expected)
-        end
-
-        it "should handle relative urls in reference links for a directory in master" do
-          actual = "[GitLab API doc directory][GitLab readmes]\n [GitLab readmes]: doc/api/\n"
-          expected = "<p><a href=\"/#{project.path_with_namespace}/tree/#{@ref}/doc/api\">GitLab API doc directory</a></p>\n"
-          expect(markdown(actual)).to match(expected)
-        end
-
-        it "should not handle malformed relative urls in reference links for a file in master" do
-          actual = "[GitLab readme]: doc/api/README.md\n"
-          expected = ""
-          expect(markdown(actual)).to match(expected)
-        end
-      end
-
-      context 'with an empty repository' do
-        before do
-          @project = create(:empty_project)
-          @repository = @project.repository
-        end
-
-        it "should not touch relative urls" do
-          actual = "[GitLab API doc][GitLab readme]\n [GitLab readme]: doc/api/README.md\n"
-          expected = "<p><a href=\"doc/api/README.md\">GitLab API doc</a></p>\n"
-          expect(markdown(actual)).to match(expected)
-        end
-      end
-    end
-  end
-
   describe '#render_wiki_content' do
     before do
       @wiki = double('WikiPage')

spec/lib/gitlab/markdown/relative_link_filter_spec.rb

+require 'spec_helper'
+
+module Gitlab::Markdown
+  describe RelativeLinkFilter do
+    include ActionView::Helpers::TagHelper
+
+    let!(:project) { create(:project) }
+
+    let(:commit) { project.commit }
+    let(:project_path) { project.path_with_namespace }
+    let(:repository) { project.repository }
+    let(:ref) { 'markdown' }
+
+    let(:project_wiki) { nil }
+    let(:requested_path) { '/' }
+    let(:blob) { RepoHelpers.sample_blob }
+
+    let(:context) do
+      {
+        commit: commit,
+        project: project,
+        project_wiki: project_wiki,
+        requested_path: requested_path,
+        ref: ref
+      }
+    end
+
+
+    shared_examples :preserve_unchanged do
+
+      it "should not modify any relative url in anchor" do
+        doc = tag(:a, href: 'README.md')
+        expect( filter(doc) ).to match '"README.md"'
+      end
+
+      it "should not modify any relative url in image" do
+        doc = tag(:img, src: 'files/images/logo-black.png')
+        expect( filter(doc) ).to match '"files/images/logo-black.png"'
+      end
+    end
+
+    shared_examples :relative_to_requested do
+
+      it "should rebuild url relative to the requested path" do
+        expect( filter(tag(:a, href: 'users.md')) ).to \
+          match %("/#{project_path}/blob/#{ref}/doc/api/users.md")
+      end
+    end
+
+
+    context "with a project_wiki" do
+      let(:project_wiki) { double('ProjectWiki') }
+
+      include_examples :preserve_unchanged
+    end
+
+    context "without a repository" do
+      let!(:project) { create(:empty_project) }
+
+      include_examples :preserve_unchanged
+    end
+
+    context "with an empty repository" do
+      let!(:project) { create(:project_empty_repo) }
+
+      include_examples :preserve_unchanged
+    end
+
+
+    context "with a valid repository" do
+
+      it "should rebuild relative url for a file in the repo" do
+        expect( filter(tag(:a, href: 'doc/api/README.md')) ).to \
+          match %("/#{project_path}/blob/#{ref}/doc/api/README.md")
+      end
+
+      it "should rebuild relative url for a file in the repo with an anchor" do
+        expect( filter(tag(:a, href: 'README.md#section')) ).to \
+          match %("/#{project_path}/blob/#{ref}/README.md#section")
+      end
+
+      it "should rebuild relative url for a directory in the repo" do
+        expect( filter(tag(:a, href: 'doc/api/')) ).to \
+          match %("/#{project_path}/tree/#{ref}/doc/api")
+      end
+
+      it "should rebuild relative url for an image in the repo" do
+        expect( filter(tag(:img, src: 'files/images/logo-black.png')) ).to \
+          match %("/#{project_path}/raw/#{ref}/files/images/logo-black.png")
+      end
+
+      it "should not modify relative url with an anchor only" do
+        doc = tag(:a, href: '#section-1')
+        expect( filter(doc) ).to match %("#section-1")
+      end
+
+      it "should not modify absolute url" do
+        expect( filter(tag(:a, href: 'http://example.org')) ).to \
+          match %("http://example.org")
+      end
+
+      context "when requested path is a file in the repo" do
+        let(:requested_path) { 'doc/api/README.md' }
+
+        include_examples :relative_to_requested
+      end
+
+      context "when requested path is a directory in the repo" do
+        let(:requested_path) { 'doc/api' }
+
+        include_examples :relative_to_requested
+      end
+    end
+
+
+    def filter(doc)
+      described_class.call(doc, context).to_s
+    end
+  end
+end