Implement the correct linking behaviour in `WikiLinkFilter`.

Original Comments
=================

- Linking behaves as per rules documented here:
  https://gitlab.com/gitlab-org/gitlab-ce/blob/16568-document-wiki-linking-behavior/doc/markdown/wiki.md
- All links (to other wiki pages) are rewritten to be at the level of
  the app root. We can't use links relative to the current
  page ('./foo', 'foo', '../foo'), because they won't work in the
  markdown preview, where the current page is suffixed with `/edit`
- Move existing `WikiLinkFilter` specs to `WikiPipeline` spec. It makes
  sense to run these tests on the combined output of the pipeline,
  rather than a single filter, since we can catch issues with
  conflicting filters.
- Add more tests to cover the new linking

@rymai's Review
===============

- Classes nested under `WikiLinkFilter` should declare `WikiLinkFilter`'s
  inherit, so nothing changes if the nested class is loaded first.
- Add a blank line after a guard clause
- Use keyword arguments for the `Rewriter` constructor
- Invert a condition - use `if` instead of `unless`
- Inline a `let` in `WikiPipeline` spec - it was only used in a single place
- Change out of date spec names
- Add a comment for every rewrite rule in `Rewriter`

lib/banzai/filter/wiki_link_filter.rb

@@ -2,7 +2,8 @@ require 'uri'
 
 module Banzai
   module Filter
-    # HTML filter that "fixes" relative links to files in a repository.
+    # HTML filter that "fixes" links to pages/files in a wiki.
+    # Rewrite rules are documented in the `WikiPipeline` spec.
     #
     # Context options:
     #   :project_wiki
@@ -25,36 +26,15 @@ module Banzai
       end
 
       def process_link_attr(html_attr)
-        return if html_attr.blank? || file_reference?(html_attr) || hierarchical_link?(html_attr)
+        return if html_attr.blank?
 
-        uri = URI(html_attr.value)
-        if uri.relative? && uri.path.present?
-          html_attr.value = rebuild_wiki_uri(uri).to_s
-        end
+        html_attr.value = apply_rewrite_rules(html_attr.value)
       rescue URI::Error
         # noop
       end
 
-      def rebuild_wiki_uri(uri)
-        uri.path = ::File.join(project_wiki_base_path, uri.path)
-        uri
-      end
-
-      def project_wiki
-        context[:project_wiki]
-      end
-
-      def file_reference?(html_attr)
-        !File.extname(html_attr.value).blank?
-      end
-
-      # Of the form `./link`, `../link`, or similar
-      def hierarchical_link?(html_attr)
-        html_attr.value[0] == '.'
-      end
-
-      def project_wiki_base_path
-        project_wiki && project_wiki.wiki_base_path
+      def apply_rewrite_rules(link_string)
+        Rewriter.new(link_string, wiki: context[:project_wiki], slug: context[:page_slug]).apply_rules
       end
     end
   end

lib/banzai/filter/wiki_link_filter/rewriter.rb

+module Banzai
+  module Filter
+    class WikiLinkFilter < HTML::Pipeline::Filter
+      class Rewriter
+        def initialize(link_string, wiki:, slug:)
+          @uri = Addressable::URI.parse(link_string)
+          @wiki_base_path = wiki && wiki.wiki_base_path
+          @slug = slug
+        end
+
+        def apply_rules
+          apply_file_link_rules!
+          apply_hierarchical_link_rules!
+          apply_relative_link_rules!
+          @uri.to_s
+        end
+
+        private
+
+        # Of the form 'file.md'
+        def apply_file_link_rules!
+          @uri = Addressable::URI.join(@slug, @uri) if @uri.extname.present?
+        end
+
+        # Of the form `./link`, `../link`, or similar
+        def apply_hierarchical_link_rules!
+          @uri = Addressable::URI.join(@slug, @uri) if @uri.to_s[0] == '.'
+        end
+
+        # Any link _not_ of the form `http://example.com/`
+        def apply_relative_link_rules!
+          if @uri.relative? && @uri.path.present?
+            link = ::File.join(@wiki_base_path, @uri.path)
+            @uri = Addressable::URI.parse(link)
+          end
+        end
+      end
+    end
+  end
+end

spec/factories/wiki_pages.rb

@@ -2,7 +2,7 @@ require 'ostruct'
 
 FactoryGirl.define do
   factory :wiki_page do
-    page = OpenStruct.new(url_path: 'some-name')
+    page { OpenStruct.new(url_path: 'some-name') }
     association :wiki, factory: :project_wiki, strategy: :build
     initialize_with { new(wiki, page, true) }
   end

spec/lib/banzai/filter/wiki_link_filter_spec.rb

-require 'spec_helper'
-
-describe Banzai::Filter::WikiLinkFilter, lib: true do
-  include FilterSpecHelper
-
-  let(:namespace) { build_stubbed(:namespace, name: "wiki_link_ns") }
-  let(:project)   { build_stubbed(:empty_project, :public, name: "wiki_link_project", namespace: namespace) }
-  let(:user) { double }
-  let(:project_wiki) { ProjectWiki.new(project, user) }
-
-  describe "links within the wiki (relative)" do
-    describe "hierarchical links to the current directory" do
-      it "doesn't rewrite non-file links" do
-        link = "<a href='./page'>Link to Page</a>"
-        filtered_link = filter(link, project_wiki: project_wiki).children[0]
-
-        expect(filtered_link.attribute('href').value).to eq('./page')
-      end
-
-      it "doesn't rewrite file links" do
-        link = "<a href='./page.md'>Link to Page</a>"
-        filtered_link = filter(link, project_wiki: project_wiki).children[0]
-
-        expect(filtered_link.attribute('href').value).to eq('./page.md')
-      end
-    end
-
-    describe "hierarchical links to the parent directory" do
-      it "doesn't rewrite non-file links" do
-        link = "<a href='../page'>Link to Page</a>"
-        filtered_link = filter(link, project_wiki: project_wiki).children[0]
-
-        expect(filtered_link.attribute('href').value).to eq('../page')
-      end
-
-      it "doesn't rewrite file links" do
-        link = "<a href='../page.md'>Link to Page</a>"
-        filtered_link = filter(link, project_wiki: project_wiki).children[0]
-
-        expect(filtered_link.attribute('href').value).to eq('../page.md')
-      end
-    end
-
-    describe "hierarchical links to a sub-directory" do
-      it "doesn't rewrite non-file links" do
-        link = "<a href='./subdirectory/page'>Link to Page</a>"
-        filtered_link = filter(link, project_wiki: project_wiki).children[0]
-
-        expect(filtered_link.attribute('href').value).to eq('./subdirectory/page')
-      end
-
-      it "doesn't rewrite file links" do
-        link = "<a href='./subdirectory/page.md'>Link to Page</a>"
-        filtered_link = filter(link, project_wiki: project_wiki).children[0]
-
-        expect(filtered_link.attribute('href').value).to eq('./subdirectory/page.md')
-      end
-    end
-
-    describe "non-hierarchical links" do
-      it 'rewrites non-file links to be at the scope of the wiki root' do
-        link = "<a href='page'>Link to Page</a>"
-        filtered_link = filter(link, project_wiki: project_wiki).children[0]
-
-        expect(filtered_link.attribute('href').value).to match('/wiki_link_ns/wiki_link_project/wikis/page')
-      end
-
-      it "doesn't rewrite file links" do
-        link = "<a href='page.md'>Link to Page</a>"
-        filtered_link = filter(link, project_wiki: project_wiki).children[0]
-
-        expect(filtered_link.attribute('href').value).to eq('page.md')
-      end
-    end
-  end
-
-  describe "links outside the wiki (absolute)" do
-    it "doesn't rewrite links" do
-      link = "<a href='http://example.com/page'>Link to Page</a>"
-      filtered_link = filter(link, project_wiki: project_wiki).children[0]
-
-      expect(filtered_link.attribute('href').value).to eq('http://example.com/page')
-    end
-  end
-end

spec/lib/banzai/pipeline/wiki_pipeline_spec.rb

@@ -50,4 +50,112 @@ describe Banzai::Pipeline::WikiPipeline do
       end
     end
   end
+
+  describe "Links" do
+    let(:namespace) { build_stubbed(:namespace, name: "wiki_link_ns") }
+    let(:project)   { build_stubbed(:empty_project, :public, name: "wiki_link_project", namespace: namespace) }
+    let(:project_wiki) { ProjectWiki.new(project, double(:user)) }
+    let(:page) { build(:wiki_page, wiki: project_wiki, page: OpenStruct.new(url_path: 'nested/twice/start-page')) }
+
+    { "when GitLab is hosted at a root URL" => '/',
+      "when GitLab is hosted at a relative URL" => '/nested/relative/gitlab' }.each do |test_name, relative_url_root|
+
+      context test_name do
+        before do
+          allow(Gitlab.config.gitlab).to receive(:relative_url_root).and_return(relative_url_root)
+        end
+
+        describe "linking to pages within the wiki" do
+          context "when creating hierarchical links to the current directory" do
+            it "rewrites non-file links to be at the scope of the current directory" do
+              markdown = "[Page](./page)"
+              output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
+
+              expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/twice/page\"")
+            end
+
+            it "rewrites file links to be at the scope of the current directory" do
+              markdown = "[Link to Page](./page.md)"
+              output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
+
+              expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/twice/page.md\"")
+            end
+          end
+
+          context "when creating hierarchical links to the parent directory" do
+            it "rewrites non-file links to be at the scope of the parent directory" do
+              markdown = "[Link to Page](../page)"
+              output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
+
+              expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/page\"")
+            end
+
+            it "rewrites file links to be at the scope of the parent directory" do
+              markdown = "[Link to Page](../page.md)"
+              output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
+
+              expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/page.md\"")
+            end
+          end
+
+          context "when creating hierarchical links to a sub-directory" do
+            it "rewrites non-file links to be at the scope of the sub-directory" do
+              markdown = "[Link to Page](./subdirectory/page)"
+              output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
+
+              expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/twice/subdirectory/page\"")
+            end
+
+            it "rewrites file links to be at the scope of the sub-directory" do
+              markdown = "[Link to Page](./subdirectory/page.md)"
+              output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
+
+              expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/twice/subdirectory/page.md\"")
+            end
+          end
+
+          describe "when creating non-hierarchical links" do
+            it 'rewrites non-file links to be at the scope of the wiki root' do
+              markdown = "[Link to Page](page)"
+              output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
+
+              expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/page\"")
+            end
+
+            it "rewrites file links to be at the scope of the current directory" do
+              markdown = "[Link to Page](page.md)"
+              output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
+
+              expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/nested/twice/page.md\"")
+            end
+          end
+
+          describe "when creating root links" do
+            it 'rewrites non-file links to be at the scope of the wiki root' do
+              markdown = "[Link to Page](/page)"
+              output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
+
+              expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/page\"")
+            end
+
+            it 'rewrites file links to be at the scope of the wiki root' do
+              markdown = "[Link to Page](/page.md)"
+              output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
+
+              expect(output).to include("href=\"#{relative_url_root}/wiki_link_ns/wiki_link_project/wikis/page.md\"")
+            end
+          end
+        end
+
+        describe "linking to pages outside the wiki (absolute)" do
+          it "doesn't rewrite links" do
+            markdown = "[Link to Page](http://example.com/page)"
+            output = described_class.to_html(markdown, project: project, project_wiki: project_wiki, page_slug: page.slug)
+
+            expect(output).to include('href="http://example.com/page"')
+          end
+        end
+      end
+    end
+  end
 end