Minor RelativeLinkFilter cleanuprs-relative-link-filter

3 files changed, 94 insertions, 85 deletions

diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb
index cc68416d5fc..89abd521b1e 100644
--- a/lib/gitlab/markdown.rb
+++ b/lib/gitlab/markdown.rb
@@ -68,9 +68,9 @@ module Gitlab
         reference_class: html_options[:class],
 
         # RelativeLinkFilter
-        ref: @ref,
+        ref:            @ref,
         requested_path: @path,
-        project_wiki: @project_wiki
+        project_wiki:   @project_wiki
       }
 
       result = pipeline.call(text, context)
diff --git a/lib/gitlab/markdown/relative_link_filter.rb b/lib/gitlab/markdown/relative_link_filter.rb
index deb302c88e1..9de2b24a9da 100644
--- a/lib/gitlab/markdown/relative_link_filter.rb
+++ b/lib/gitlab/markdown/relative_link_filter.rb
@@ -9,19 +9,18 @@ module Gitlab
     #   :commit
     #   :project
     #   :project_wiki
-    #   :requested_path
     #   :ref
+    #   :requested_path
     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
+        return doc unless 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
 
         doc
@@ -40,6 +39,8 @@ module Gitlab
         if uri.relative? && uri.path.present?
           html_attr.value = rebuild_relative_uri(uri).to_s
         end
+      rescue URI::Error
+        # noop
       end
 
       def rebuild_relative_uri(uri)
@@ -85,12 +86,25 @@ module Gitlab
           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.
+      # 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)
-        return 'tree' if repository.tree(current_sha, path).entries.any?
-        return 'raw' if repository.blob_at(current_sha, path).try(:image?)
-        'blob'
+        if repository.tree(current_sha, path).entries.any?
+          'tree'
+        elsif repository.blob_at(current_sha, path).try(:image?)
+          'raw'
+        else
+          'blob'
+        end
       end
 
       def current_sha
diff --git a/spec/lib/gitlab/markdown/relative_link_filter_spec.rb b/spec/lib/gitlab/markdown/relative_link_filter_spec.rb
index 38cf567d73f..5ee5310825d 100644
--- a/spec/lib/gitlab/markdown/relative_link_filter_spec.rb
+++ b/spec/lib/gitlab/markdown/relative_link_filter_spec.rb
@@ -2,119 +2,114 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe RelativeLinkFilter do
-    include ActionView::Helpers::TagHelper
-
-    let!(:project) { create(:project) }
+    def filter(doc)
+      described_class.call(doc, {
+        commit:         project.commit,
+        project:        project,
+        project_wiki:   project_wiki,
+        ref:            ref,
+        requested_path: requested_path
+      })
+    end
 
-    let(:commit) { project.commit }
-    let(:project_path) { project.path_with_namespace }
-    let(:repository) { project.repository }
-    let(:ref) { 'markdown' }
+    def image(path)
+      %(<img src="#{path}" />)
+    end
 
-    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
-      }
+    def link(path)
+      %(<a href="#{path}">#{path}</a>)
     end
 
+    let(:project)        { create(:project) }
+    let(:project_path)   { project.path_with_namespace }
+    let(:ref)            { 'markdown' }
+    let(:project_wiki)   { nil }
+    let(:requested_path) { '/' }
 
     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"'
+      it 'does not modify any relative URL in anchor' do
+        doc = filter(link('README.md'))
+        expect(doc.at_css('a')['href']).to eq '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"'
+      it 'does not modify any relative URL in image' do
+        doc = filter(image('files/images/logo-black.png'))
+        expect(doc.at_css('img')['src']).to eq '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")
+      it 'rebuilds URL relative to the requested path' do
+        doc = filter(link('users.md'))
+        expect(doc.at_css('a')['href']).
+          to eq "/#{project_path}/blob/#{ref}/doc/api/users.md"
       end
     end
 
-
-    context "with a project_wiki" do
+    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) }
-
+    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) }
-
+    context 'with an empty repository' do
+      let(:project) { create(:project_empty_repo) }
       include_examples :preserve_unchanged
     end
 
+    it 'does not raise an exception on invalid URIs' do
+      act = link("://foo")
+      expect { filter(act) }.not_to raise_error
+    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")
+    context 'with a valid repository' do
+      it 'rebuilds relative URL for a file in the repo' do
+        doc = filter(link('doc/api/README.md'))
+        expect(doc.at_css('a')['href']).
+          to eq "/#{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")
+      it 'rebuilds relative URL for a file in the repo with an anchor' do
+        doc = filter(link('README.md#section'))
+        expect(doc.at_css('a')['href']).
+          to eq "/#{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")
+      it 'rebuilds relative URL for a directory in the repo' do
+        doc = filter(link('doc/api/'))
+        expect(doc.at_css('a')['href']).
+          to eq "/#{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")
+      it 'rebuilds relative URL for an image in the repo' do
+        doc = filter(link('files/images/logo-black.png'))
+        expect(doc.at_css('a')['href']).
+          to eq "/#{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")
+      it 'does not modify relative URL with an anchor only' do
+        doc = filter(link('#section-1'))
+        expect(doc.at_css('a')['href']).to eq '#section-1'
       end
 
-      it "should not modify absolute url" do
-        expect( filter(tag(:a, href: 'http://example.org')) ).to \
-          match %("http://example.org")
+      it 'does not modify absolute URL' do
+        doc = filter(link('http://example.com'))
+        expect(doc.at_css('a')['href']).to eq 'http://example.com'
       end
 
-      context "when requested path is a file in the repo" do
+      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
+      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