From 8e71c19a6940b8d82c70ee9b2550b62b5169eb54 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 8 Jun 2016 10:29:20 +0530 Subject: [PATCH 1/3] 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 | 32 +----- .../filter/wiki_link_filter/rewriter.rb | 40 +++++++ spec/factories/wiki_pages.rb | 2 +- .../banzai/filter/wiki_link_filter_spec.rb | 85 -------------- .../lib/banzai/pipeline/wiki_pipeline_spec.rb | 108 ++++++++++++++++++ 5 files changed, 155 insertions(+), 112 deletions(-) create mode 100644 lib/banzai/filter/wiki_link_filter/rewriter.rb delete mode 100644 spec/lib/banzai/filter/wiki_link_filter_spec.rb diff --git a/lib/banzai/filter/wiki_link_filter.rb b/lib/banzai/filter/wiki_link_filter.rb index 7dc771afd71..37a2779d453 100644 --- a/lib/banzai/filter/wiki_link_filter.rb +++ b/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 diff --git a/lib/banzai/filter/wiki_link_filter/rewriter.rb b/lib/banzai/filter/wiki_link_filter/rewriter.rb new file mode 100644 index 00000000000..2e2c8da311e --- /dev/null +++ b/lib/banzai/filter/wiki_link_filter/rewriter.rb @@ -0,0 +1,40 @@ +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 diff --git a/spec/factories/wiki_pages.rb b/spec/factories/wiki_pages.rb index 938ccf2306b..efa6cbe5bb1 100644 --- a/spec/factories/wiki_pages.rb +++ b/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 diff --git a/spec/lib/banzai/filter/wiki_link_filter_spec.rb b/spec/lib/banzai/filter/wiki_link_filter_spec.rb deleted file mode 100644 index 185abbb2108..00000000000 --- a/spec/lib/banzai/filter/wiki_link_filter_spec.rb +++ /dev/null @@ -1,85 +0,0 @@ -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 = "Link to Page" - 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 = "Link to Page" - 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 = "Link to Page" - 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 = "Link to Page" - 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 = "Link to Page" - 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 = "Link to Page" - 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 = "Link to Page" - 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 = "Link to Page" - 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 = "Link to Page" - 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 diff --git a/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb b/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb index 7aa1b4a3bf6..ea4ab2c852e 100644 --- a/spec/lib/banzai/pipeline/wiki_pipeline_spec.rb +++ b/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 From e6b1d1669b362ad4cea27ac44e89e73f4d6e92fd Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 8 Jun 2016 10:32:50 +0530 Subject: [PATCH 2/3] Hook up the updated `WikiLinkFilter` to the wiki controllers. - Need to pass in a `page_slug` to the filter, so it can rewrite based on the current page (all links are rewritten to the level of the app root). - The earlier `markdown_preview` endpoint was at the level of the wiki. We need to know the current page (for rewriting, as above), so this commit moves the endpoint to the level of a wiki page. - Fix all tests --- app/controllers/projects/wikis_controller.rb | 2 +- app/helpers/gitlab_markdown_helper.rb | 2 +- app/views/layouts/project.html.haml | 4 ++-- config/routes.rb | 2 +- features/steps/project/wiki.rb | 4 ++-- spec/features/markdown_spec.rb | 3 ++- spec/helpers/gitlab_markdown_helper_spec.rb | 3 ++- spec/support/markdown_feature.rb | 4 ++++ 8 files changed, 15 insertions(+), 9 deletions(-) diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb index 4b404eb03fa..2aa6bed0724 100644 --- a/app/controllers/projects/wikis_controller.rb +++ b/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) } diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 0a1b48af219..067a00660aa 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/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 diff --git a/app/views/layouts/project.html.haml b/app/views/layouts/project.html.haml index 20d6cdf7246..2049b204956 100644 --- a/app/views/layouts/project.html.haml +++ b/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 diff --git a/config/routes.rb b/config/routes.rb index 428302d0fd7..34126bbe913 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -591,7 +591,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 @@ -600,6 +599,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 diff --git a/features/steps/project/wiki.rb b/features/steps/project/wiki.rb index 9f6aed1c5b9..3cbf832c728 100644 --- a/features/steps/project/wiki.rb +++ b/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 diff --git a/spec/features/markdown_spec.rb b/spec/features/markdown_spec.rb index 1d892fe1a55..07275bfb3d8 100644 --- a/spec/features/markdown_spec.rb +++ b/spec/features/markdown_spec.rb @@ -231,13 +231,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' diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 13de88e2f21..ade5c3b02d9 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/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 diff --git a/spec/support/markdown_feature.rb b/spec/support/markdown_feature.rb index 7fc6d6fcc5e..a79386b5db9 100644 --- a/spec/support/markdown_feature.rb +++ b/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 From 19b91e749a6320d12fb299d33f1f6440777e0e26 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 9 Jun 2016 10:16:46 +0530 Subject: [PATCH 3/3] Add #18019 to the CHANGELOG. --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index d1cde40c1c7..1e51bbff4e0 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -16,6 +16,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