Merge branch '18019-fix-wiki-linking' into 'master'

Fix wiki linking behavior for markdown wiki pages

Related to #18019 

- As per the documentation in !4372 

## TODO
- [ ] !4432 Have wiki linking behave as per the documentation
    - [x] Move `WikiLinkFilter` specs to the pipeline level
    - [x] Verify current behavior on wiki `show` page
    - [x] Fix current behavior on wiki `show` page
    - [x] Verify current behaviour on wiki preview
    - [x] Fix current behaviour on wiki preview
        - [x] Rewrite all links and get preview links working
        - [x] Make sure all links are on-par with the wiki `show` page
        - [x] TDD `WikiLinkFilter` and get it working
        - [x] Hook `WikiLinkFilter` up
    - [x] Fix tests 
        - [x] Fix `markdown_spec`
        - [x] Fix `wiki` spinach feature
        - [x] Wait for [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/4f50dd2/builds) to pass
        - [x] Make sure all wiki-related pages are working as expected (history, all pages, etc.)
    - [x] Test in different ruby versions
    - [x] GitLab instances hosted on a relative URL
    - [x] Non-markdown rendering formats?
        - [x] RDoc
        - [x] ASCIIDoc
        - [x] Create issues to fix things for RDoc and ASCIIDoc
    - [x] Gauge performance impact
    - [x] Refactor
    - [x] Re-organize commits
    - [x] Make sure [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/f860e9a8dcabe7d5f160c32fc549807c98baa4a1/builds) passes
    - [x] Respond to @rymai's comments
        - [x] `class WikiLinkFilter < HTML::Pipeline::Filter`
        - [x] blank line after guard clause
        - [x] keyword arguments for `wiki` and `slug`
        - [x] invert the condition
        - [x] inline `user` in spec
        - [x] Make sure spec names are not out of date
        - [x] Comment for each rewrite rule
    - [x] Add CHANGELOG entry
    - [x] Reorganize commits
    - [x] Make sure [build](https://gitlab.com/gitlab-org/gitlab-ce/commit/19b91e749a6320d12fb299d33f1f6440777e0e26/builds) passes
    - [ ] Wait for merge

See merge request !4432

CHANGELOG

@@ -28,6 +28,7 @@ v 8.9.0 (unreleased)
   - Add DB index on users.state
   - Add rake task 'gitlab:db:configure' for conditionally seeding or migrating the database
   - Changed the Slack build message to use the singular duration if necessary (Aran Koning)
+  - Links from a wiki page to other wiki pages should be rewritten as expected
   - Fix issues filter when ordering by milestone
   - Todos will display target state if issuable target is 'Closed' or 'Merged'
   - Fix bug when sorting issues by milestone due date and filtering by two or more labels

app/controllers/projects/wikis_controller.rb

@@ -95,7 +95,7 @@ class Projects::WikisController < Projects::ApplicationController
     ext.analyze(text, author: current_user)
 
     render json: {
-      body: view_context.markdown(text, pipeline: :wiki, project_wiki: @project_wiki),
+      body: view_context.markdown(text, pipeline: :wiki, project_wiki: @project_wiki, page_slug: params[:id]),
       references: {
         users: ext.users.map(&:username)
       }

app/helpers/gitlab_markdown_helper.rb

@@ -108,7 +108,7 @@ module GitlabMarkdownHelper
   def render_wiki_content(wiki_page)
     case wiki_page.format
     when :markdown
-      markdown(wiki_page.content, pipeline: :wiki, project_wiki: @project_wiki)
+      markdown(wiki_page.content, pipeline: :wiki, project_wiki: @project_wiki, page_slug: wiki_page.slug)
     when :asciidoc
       asciidoc(wiki_page.content)
     else

app/views/layouts/project.html.haml

@@ -5,8 +5,8 @@
 
 - content_for :scripts_body_top do
   - project = @target_project || @project
-  - if @project_wiki
-    - markdown_preview_path = namespace_project_wikis_markdown_preview_path(project.namespace, project)
+  - if @project_wiki && @page
+    - markdown_preview_path = namespace_project_wiki_markdown_preview_path(project.namespace, project, params[:id])
   - else
     - markdown_preview_path = markdown_preview_namespace_project_path(project.namespace, project)
   - if current_user

config/routes.rb

@@ -616,7 +616,6 @@ Rails.application.routes.draw do
           # Order matters to give priority to these matches
           get '/wikis/git_access', to: 'wikis#git_access'
           get '/wikis/pages', to: 'wikis#pages', as: 'wiki_pages'
-          post '/wikis/markdown_preview', to:'wikis#markdown_preview'
           post '/wikis', to: 'wikis#create'
 
           get '/wikis/*id/history', to: 'wikis#history', as: 'wiki_history', constraints: WIKI_SLUG_ID
@@ -625,6 +624,7 @@ Rails.application.routes.draw do
           get '/wikis/*id', to: 'wikis#show', as: 'wiki', constraints: WIKI_SLUG_ID
           delete '/wikis/*id', to: 'wikis#destroy', constraints: WIKI_SLUG_ID
           put '/wikis/*id', to: 'wikis#update', constraints: WIKI_SLUG_ID
+          post '/wikis/*id/markdown_preview', to:'wikis#markdown_preview', constraints: WIKI_SLUG_ID, as: 'wiki_markdown_preview'
         end
 
         resource :repository, only: [:show, :create] do

features/steps/project/wiki.rb

@@ -97,7 +97,7 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps
     file = Gollum::File.new(wiki.wiki)
     Gollum::Wiki.any_instance.stub(:file).with("image.jpg", "master", true).and_return(file)
     Gollum::File.any_instance.stub(:mime_type).and_return("image/jpeg")
-    expect(page).to have_link('image', href: "image.jpg")
+    expect(page).to have_link('image', href: "#{wiki.wiki_base_path}/image.jpg")
     click_on "image"
   end
 
@@ -113,7 +113,7 @@ class Spinach::Features::ProjectWiki < Spinach::FeatureSteps
   end
 
   step 'I click on image link' do
-    expect(page).to have_link('image', href: "image.jpg")
+    expect(page).to have_link('image', href: "#{wiki.wiki_base_path}/image.jpg")
     click_on "image"
   end
 

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/features/markdown_spec.rb

@@ -241,13 +241,14 @@ describe 'GitLab Markdown', feature: true do
   context 'wiki pipeline' do
     before do
       @project_wiki = @feat.project_wiki
+      @project_wiki_page = @feat.project_wiki_page
 
       file = Gollum::File.new(@project_wiki.wiki)
       expect(file).to receive(:path).and_return('images/example.jpg')
       expect(@project_wiki).to receive(:find_file).with('images/example.jpg').and_return(file)
       allow(@project_wiki).to receive(:wiki_base_path) { '/namespace1/gitlabhq/wikis' }
 
-      @html = markdown(@feat.raw_markdown, { pipeline: :wiki, project_wiki: @project_wiki })
+      @html = markdown(@feat.raw_markdown, { pipeline: :wiki, project_wiki: @project_wiki, page_slug: @project_wiki_page.slug })
     end
 
     it_behaves_like 'all pipelines'

spec/helpers/gitlab_markdown_helper_spec.rb

@@ -121,13 +121,14 @@ describe GitlabMarkdownHelper do
     before do
       @wiki = double('WikiPage')
       allow(@wiki).to receive(:content).and_return('wiki content')
+      allow(@wiki).to receive(:slug).and_return('nested/page')
       helper.instance_variable_set(:@project_wiki, @wiki)
     end
 
     it "should use Wiki pipeline for markdown files" do
       allow(@wiki).to receive(:format).and_return(:markdown)
 
-      expect(helper).to receive(:markdown).with('wiki content', pipeline: :wiki, project_wiki: @wiki)
+      expect(helper).to receive(:markdown).with('wiki content', pipeline: :wiki, project_wiki: @wiki, page_slug: "nested/page")
 
       helper.render_wiki_content(@wiki)
     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

spec/support/markdown_feature.rb

@@ -32,6 +32,10 @@ class MarkdownFeature
     @project_wiki ||= ProjectWiki.new(project, user)
   end
 
+  def project_wiki_page
+    @project_wiki_page ||= build(:wiki_page, wiki: project_wiki)
+  end
+
   def issue
     @issue ||= create(:issue, project: project)
   end