From 5dd26d4e5a5a27ca93e6d55b4261c42f4f70e762 Mon Sep 17 00:00:00 2001 From: "Jacob Vosmaer (GitLab)" Date: Tue, 3 Oct 2017 16:58:33 +0000 Subject: [PATCH] Hide Gollum inside Gitlab::Git::Wiki --- app/controllers/projects/wikis_controller.rb | 16 +-- app/models/project_wiki.rb | 57 ++++----- app/models/wiki_page.rb | 10 +- app/views/projects/wikis/history.html.haml | 4 +- app/views/projects/wikis/show.html.haml | 2 +- lib/gitlab/git/wiki.rb | 115 ++++++++++++++++++ lib/gitlab/git/wiki_file.rb | 19 +++ lib/gitlab/git/wiki_page.rb | 39 ++++++ lib/gitlab/git/wiki_page_version.rb | 19 +++ .../wiki/user_views_wiki_page_spec.rb | 2 +- spec/models/project_wiki_spec.rb | 64 +++++----- spec/models/wiki_page_spec.rb | 14 +-- spec/services/projects/create_service_spec.rb | 11 +- .../single_repository_worker_spec.rb | 7 +- 14 files changed, 278 insertions(+), 101 deletions(-) create mode 100644 lib/gitlab/git/wiki.rb create mode 100644 lib/gitlab/git/wiki_file.rb create mode 100644 lib/gitlab/git/wiki_page.rb create mode 100644 lib/gitlab/git/wiki_page_version.rb diff --git a/app/controllers/projects/wikis_controller.rb b/app/controllers/projects/wikis_controller.rb index 968d880886c..a8ebdf5a4a9 100644 --- a/app/controllers/projects/wikis_controller.rb +++ b/app/controllers/projects/wikis_controller.rb @@ -18,16 +18,12 @@ class Projects::WikisController < Projects::ApplicationController response.headers['Content-Security-Policy'] = "default-src 'none'" response.headers['X-Content-Security-Policy'] = "default-src 'none'" - if file.on_disk? - send_file file.on_disk_path, disposition: 'inline' - else - send_data( - file.raw_data, - type: file.mime_type, - disposition: 'inline', - filename: file.name - ) - end + send_data( + file.raw_data, + type: file.mime_type, + disposition: 'inline', + filename: file.name + ) else return render('empty') unless can?(current_user, :create_wiki, @project) @page = WikiPage.new(@project_wiki) diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb index c4cc1c1cf22..bb7be29ef66 100644 --- a/app/models/project_wiki.rb +++ b/app/models/project_wiki.rb @@ -54,12 +54,15 @@ class ProjectWiki [Gitlab.config.gitlab.relative_url_root, '/', @project.full_path, '/wikis'].join('') end - # Returns the Gollum::Wiki object. + # Returns the Gitlab::Git::Wiki object. def wiki @wiki ||= begin - Gollum::Wiki.new(path_to_repo) - rescue Rugged::OSError - create_repo! + gl_repository = Gitlab::GlRepository.gl_repository(project, true) + raw_repository = Gitlab::Git::Repository.new(project.repository_storage, disk_path + '.git', gl_repository) + + create_repo!(raw_repository) unless raw_repository.exists? + + Gitlab::Git::Wiki.new(raw_repository) end end @@ -86,20 +89,14 @@ class ProjectWiki # Returns an initialized WikiPage instance or nil def find_page(title, version = nil) page_title, page_dir = page_title_and_dir(title) - if page = wiki.page(page_title, version, page_dir) + + if page = wiki.page(title: page_title, version: version, dir: page_dir) WikiPage.new(self, page, true) - else - nil end end - def find_file(name, version = nil, try_on_disk = true) - version = wiki.ref if version.nil? # Gollum::Wiki#file ? - if wiki_file = wiki.file(name, version, try_on_disk) - wiki_file - else - nil - end + def find_file(name, version = nil) + wiki.file(name, version) end def create_page(title, content, format = :markdown, message = nil) @@ -108,7 +105,7 @@ class ProjectWiki wiki.write_page(title, format.to_sym, content, commit) update_project_activity - rescue Gollum::DuplicatePageError => e + rescue Gitlab::Git::Wiki::DuplicatePageError => e @error_message = "Duplicate page: #{e.message}" return false end @@ -116,13 +113,13 @@ class ProjectWiki def update_page(page, content:, title: nil, format: :markdown, message: nil) commit = commit_details(:updated, message, page.title) - wiki.update_page(page, title || page.name, format.to_sym, content, commit) + wiki.update_page(page.path, title || page.name, format.to_sym, content, commit) update_project_activity end def delete_page(page, message = nil) - wiki.delete_page(page, commit_details(:deleted, message, page.title)) + wiki.delete_page(page.path, commit_details(:deleted, message, page.title)) update_project_activity end @@ -145,20 +142,8 @@ class ProjectWiki wiki.class.default_ref end - def create_repo! - if init_repo(disk_path) - wiki = Gollum::Wiki.new(path_to_repo) - else - raise CouldNotCreateWikiError - end - - repository.after_create - - wiki - end - def ensure_repository - create_repo! unless repository_exists? + raise CouldNotCreateWikiError unless wiki.repository_exists? end def hook_attrs @@ -173,24 +158,24 @@ class ProjectWiki private - def init_repo(disk_path) + def create_repo!(raw_repository) gitlab_shell.add_repository(project.repository_storage, disk_path) + + raise CouldNotCreateWikiError unless raw_repository.exists? + + repository.after_create end def commit_details(action, message = nil, title = nil) commit_message = message || default_message(action, title) - { email: @user.email, name: @user.name, message: commit_message } + Gitlab::Git::Wiki::CommitDetails.new(@user.name, @user.email, commit_message) end def default_message(action, title) "#{@user.username} #{action} page: #{title}" end - def path_to_repo - @path_to_repo ||= File.join(project.repository_storage_path, "#{disk_path}.git") - end - def update_project_activity @project.touch(:last_activity_at, :last_repository_updated_at) end diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index f2315bb3dbb..5f710961f95 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -50,7 +50,7 @@ class WikiPage # The Gitlab ProjectWiki instance. attr_reader :wiki - # The raw Gollum::Page instance. + # The raw Gitlab::Git::WikiPage instance. attr_reader :page # The attributes Hash used for storing and validating @@ -75,7 +75,7 @@ class WikiPage if @attributes[:slug].present? @attributes[:slug] else - wiki.wiki.preview_page(title, '', format).url_path + wiki.wiki.preview_slug(title, format) end end @@ -131,7 +131,7 @@ class WikiPage def versions return [] unless persisted? - @page.versions + wiki.wiki.page_versions(@page.path) end def commit @@ -264,8 +264,8 @@ class WikiPage end page_title, page_dir = wiki.page_title_and_dir(page_details) - gollum_wiki = wiki.wiki - @page = gollum_wiki.paged(page_title, page_dir) + gitlab_git_wiki = wiki.wiki + @page = gitlab_git_wiki.page(title: page_title, dir: page_dir) set_attributes @persisted = errors.blank? diff --git a/app/views/projects/wikis/history.html.haml b/app/views/projects/wikis/history.html.haml index bc1ab5065e4..9ee09262324 100644 --- a/app/views/projects/wikis/history.html.haml +++ b/app/views/projects/wikis/history.html.haml @@ -29,13 +29,13 @@ commit.id, index == 0) do = truncate_sha(commit.id) %td - = commit.author.name + = commit.author_name %td = commit.message %td #{time_ago_with_tooltip(version.authored_date)} %td %strong - = @page.page.wiki.page(@page.page.name, commit.id).try(:format) + = version.format = render 'sidebar' diff --git a/app/views/projects/wikis/show.html.haml b/app/views/projects/wikis/show.html.haml index 62c18cc4582..de15fc99eda 100644 --- a/app/views/projects/wikis/show.html.haml +++ b/app/views/projects/wikis/show.html.haml @@ -11,7 +11,7 @@ .nav-text %h2.wiki-page-title= @page.title.capitalize %span.wiki-last-edit-by - = (_("Last edited by %{name}") % { name: "#{@page.commit.author.name}" }).html_safe + = (_("Last edited by %{name}") % { name: "#{@page.commit.author_name}" }).html_safe #{time_ago_with_tooltip(@page.commit.authored_date)} .nav-controls diff --git a/lib/gitlab/git/wiki.rb b/lib/gitlab/git/wiki.rb new file mode 100644 index 00000000000..d651c931a38 --- /dev/null +++ b/lib/gitlab/git/wiki.rb @@ -0,0 +1,115 @@ +module Gitlab + module Git + class Wiki + DuplicatePageError = Class.new(StandardError) + + CommitDetails = Struct.new(:name, :email, :message) do + def to_h + { name: name, email: email, message: message } + end + end + + def self.default_ref + 'master' + end + + # Initialize with a Gitlab::Git::Repository instance + def initialize(repository) + @repository = repository + end + + def repository_exists? + @repository.exists? + end + + def write_page(name, format, content, commit_details) + assert_type!(format, Symbol) + assert_type!(commit_details, CommitDetails) + + gollum_wiki.write_page(name, format, content, commit_details.to_h) + + nil + rescue Gollum::DuplicatePageError => e + raise Gitlab::Git::Wiki::DuplicatePageError, e.message + end + + def delete_page(page_path, commit_details) + assert_type!(commit_details, CommitDetails) + + gollum_wiki.delete_page(gollum_page_by_path(page_path), commit_details.to_h) + nil + end + + def update_page(page_path, title, format, content, commit_details) + assert_type!(format, Symbol) + assert_type!(commit_details, CommitDetails) + + gollum_wiki.update_page(gollum_page_by_path(page_path), title, format, content, commit_details.to_h) + nil + end + + def pages + gollum_wiki.pages.map { |gollum_page| new_page(gollum_page) } + end + + def page(title:, version: nil, dir: nil) + if version + version = Gitlab::Git::Commit.find(@repository, version).id + end + + gollum_page = gollum_wiki.page(title, version, dir) + return unless gollum_page + + new_page(gollum_page) + end + + def file(name, version) + version ||= self.class.default_ref + gollum_file = gollum_wiki.file(name, version) + return unless gollum_file + + Gitlab::Git::WikiFile.new(gollum_file) + end + + def page_versions(page_path) + current_page = gollum_page_by_path(page_path) + current_page.versions.map do |gollum_git_commit| + gollum_page = gollum_wiki.page(current_page.title, gollum_git_commit.id) + new_version(gollum_page, gollum_git_commit.id) + end + end + + def preview_slug(title, format) + gollum_wiki.preview_page(title, '', format).url_path + end + + private + + def gollum_wiki + @gollum_wiki ||= Gollum::Wiki.new(@repository.path) + end + + def gollum_page_by_path(page_path) + page_name = Gollum::Page.canonicalize_filename(page_path) + page_dir = File.split(page_path).first + + gollum_wiki.paged(page_name, page_dir) + end + + def new_page(gollum_page) + Gitlab::Git::WikiPage.new(gollum_page, new_version(gollum_page, gollum_page.version.id)) + end + + def new_version(gollum_page, commit_id) + commit = Gitlab::Git::Commit.find(@repository, commit_id) + Gitlab::Git::WikiPageVersion.new(commit, gollum_page&.format) + end + + def assert_type!(object, klass) + unless object.is_a?(klass) + raise ArgumentError, "expected a #{klass}, got #{object.inspect}" + end + end + end + end +end diff --git a/lib/gitlab/git/wiki_file.rb b/lib/gitlab/git/wiki_file.rb new file mode 100644 index 00000000000..527f2a44dea --- /dev/null +++ b/lib/gitlab/git/wiki_file.rb @@ -0,0 +1,19 @@ +module Gitlab + module Git + class WikiFile + attr_reader :mime_type, :raw_data, :name + + # This class is meant to be serializable so that it can be constructed + # by Gitaly and sent over the network to GitLab. + # + # Because Gollum::File is not serializable we must get all the data from + # 'gollum_file' during initialization, and NOT store it in an instance + # variable. + def initialize(gollum_file) + @mime_type = gollum_file.mime_type + @raw_data = gollum_file.raw_data + @name = gollum_file.name + end + end + end +end diff --git a/lib/gitlab/git/wiki_page.rb b/lib/gitlab/git/wiki_page.rb new file mode 100644 index 00000000000..a06bac4414f --- /dev/null +++ b/lib/gitlab/git/wiki_page.rb @@ -0,0 +1,39 @@ +module Gitlab + module Git + class WikiPage + attr_reader :url_path, :title, :format, :path, :version, :raw_data, :name, :text_data, :historical + + # This class is meant to be serializable so that it can be constructed + # by Gitaly and sent over the network to GitLab. + # + # Because Gollum::Page is not serializable we must get all the data from + # 'gollum_page' during initialization, and NOT store it in an instance + # variable. + # + # Note that 'version' is a WikiPageVersion instance which it itself + # serializable. That means it's OK to store 'version' in an instance + # variable. + def initialize(gollum_page, version) + @url_path = gollum_page.url_path + @title = gollum_page.title + @format = gollum_page.format + @path = gollum_page.path + @raw_data = gollum_page.raw_data + @name = gollum_page.name + @historical = gollum_page.historical? + + @version = version + end + + def historical? + @historical + end + + def text_data + return @text_data if defined?(@text_data) + + @text_data = @raw_data && Gitlab::EncodingHelper.encode!(@raw_data.dup) + end + end + end +end diff --git a/lib/gitlab/git/wiki_page_version.rb b/lib/gitlab/git/wiki_page_version.rb new file mode 100644 index 00000000000..55f1afedcab --- /dev/null +++ b/lib/gitlab/git/wiki_page_version.rb @@ -0,0 +1,19 @@ +module Gitlab + module Git + class WikiPageVersion + attr_reader :commit, :format + + # This class is meant to be serializable so that it can be constructed + # by Gitaly and sent over the network to GitLab. + # + # Both 'commit' (a Gitlab::Git::Commit) and 'format' (a string) are + # serializable. + def initialize(commit, format) + @commit = commit + @format = format + end + + delegate :message, :sha, :id, :author_name, :authored_date, to: :commit + end + end +end diff --git a/spec/features/projects/wiki/user_views_wiki_page_spec.rb b/spec/features/projects/wiki/user_views_wiki_page_spec.rb index 49ba2969ef0..470391dc66b 100644 --- a/spec/features/projects/wiki/user_views_wiki_page_spec.rb +++ b/spec/features/projects/wiki/user_views_wiki_page_spec.rb @@ -83,7 +83,7 @@ describe 'User views a wiki page' do it 'shows a file stored in a page' do file = Gollum::File.new(project.wiki) - allow_any_instance_of(Gollum::Wiki).to receive(:file).with('image.jpg', 'master', true).and_return(file) + allow_any_instance_of(Gollum::Wiki).to receive(:file).with('image.jpg', 'master').and_return(file) allow_any_instance_of(Gollum::File).to receive(:mime_type).and_return('image/jpeg') expect(page).to have_xpath('//img[@data-src="image.jpg"]') diff --git a/spec/models/project_wiki_spec.rb b/spec/models/project_wiki_spec.rb index 953df7746eb..78fb2df884a 100644 --- a/spec/models/project_wiki_spec.rb +++ b/spec/models/project_wiki_spec.rb @@ -6,13 +6,10 @@ describe ProjectWiki do let(:user) { project.owner } let(:gitlab_shell) { Gitlab::Shell.new } let(:project_wiki) { described_class.new(project, user) } + let(:raw_repository) { Gitlab::Git::Repository.new(project.repository_storage, subject.disk_path + '.git', 'foo') } subject { project_wiki } - before do - project_wiki.wiki - end - describe "#path_with_namespace" do it "returns the project path with namespace with the .wiki extension" do expect(subject.path_with_namespace).to eq(project.full_path + '.wiki') @@ -61,8 +58,8 @@ describe ProjectWiki do end describe "#wiki" do - it "contains a Gollum::Wiki instance" do - expect(subject.wiki).to be_a Gollum::Wiki + it "contains a Gitlab::Git::Wiki instance" do + expect(subject.wiki).to be_a Gitlab::Git::Wiki end it "creates a new wiki repo if one does not yet exist" do @@ -70,20 +67,18 @@ describe ProjectWiki do end it "raises CouldNotCreateWikiError if it can't create the wiki repository" do - allow(project_wiki).to receive(:init_repo).and_return(false) - expect { project_wiki.send(:create_repo!) }.to raise_exception(ProjectWiki::CouldNotCreateWikiError) + # Create a fresh project which will not have a wiki + project_wiki = described_class.new(create(:project), user) + gitlab_shell = double(:gitlab_shell) + allow(gitlab_shell).to receive(:add_repository) + allow(project_wiki).to receive(:gitlab_shell).and_return(gitlab_shell) + + expect { project_wiki.send(:wiki) }.to raise_exception(ProjectWiki::CouldNotCreateWikiError) end end describe "#empty?" do context "when the wiki repository is empty" do - before do - allow_any_instance_of(Gitlab::Shell).to receive(:add_repository) do - create_temp_repo("#{Rails.root}/tmp/test-git-base-path/non-existant.wiki.git") - end - allow(project).to receive(:full_path).and_return("non-existant") - end - describe '#empty?' do subject { super().empty? } it { is_expected.to be_truthy } @@ -154,13 +149,13 @@ describe ProjectWiki do before do file = Gollum::File.new(subject.wiki) allow_any_instance_of(Gollum::Wiki) - .to receive(:file).with('image.jpg', 'master', true) + .to receive(:file).with('image.jpg', 'master') .and_return(file) allow_any_instance_of(Gollum::File) .to receive(:mime_type) .and_return('image/jpeg') allow_any_instance_of(Gollum::Wiki) - .to receive(:file).with('non-existant', 'master', true) + .to receive(:file).with('non-existant', 'master') .and_return(nil) end @@ -178,9 +173,9 @@ describe ProjectWiki do expect(subject.find_file('non-existant')).to eq(nil) end - it 'returns a Gollum::File instance' do + it 'returns a Gitlab::Git::WikiFile instance' do file = subject.find_file('image.jpg') - expect(file).to be_a Gollum::File + expect(file).to be_a Gitlab::Git::WikiFile end end @@ -222,9 +217,9 @@ describe ProjectWiki do describe "#update_page" do before do create_page("update-page", "some content") - @gollum_page = subject.wiki.paged("update-page") + @gitlab_git_wiki_page = subject.wiki.page(title: "update-page") subject.update_page( - @gollum_page, + @gitlab_git_wiki_page, content: "some other content", format: :markdown, message: "updated page" @@ -246,7 +241,7 @@ describe ProjectWiki do it 'updates project activity' do subject.update_page( - @gollum_page, + @gitlab_git_wiki_page, content: 'Yet more content', format: :markdown, message: 'Updated page again' @@ -262,7 +257,7 @@ describe ProjectWiki do describe "#delete_page" do before do create_page("index", "some content") - @page = subject.wiki.paged("index") + @page = subject.wiki.page(title: "index") end it "deletes the page" do @@ -282,27 +277,28 @@ describe ProjectWiki do describe '#create_repo!' do it 'creates a repository' do - expect(subject).to receive(:init_repo) - .with(subject.full_path) - .and_return(true) - + expect(raw_repository.exists?).to eq(false) expect(subject.repository).to receive(:after_create) - expect(subject.create_repo!).to be_an_instance_of(Gollum::Wiki) + subject.send(:create_repo!, raw_repository) + + expect(raw_repository.exists?).to eq(true) end end describe '#ensure_repository' do it 'creates the repository if it not exist' do - allow(subject).to receive(:repository_exists?).and_return(false) - - expect(subject).to receive(:create_repo!) + expect(raw_repository.exists?).to eq(false) + expect(subject).to receive(:create_repo!).and_call_original subject.ensure_repository + + expect(raw_repository.exists?).to eq(true) end it 'does not create the repository if it exists' do - allow(subject).to receive(:repository_exists?).and_return(true) + subject.wiki + expect(raw_repository.exists?).to eq(true) expect(subject).not_to receive(:create_repo!) @@ -329,7 +325,7 @@ describe ProjectWiki do end def commit_details - { name: user.name, email: user.email, message: "test commit" } + Gitlab::Git::Wiki::CommitDetails.new(user.name, user.email, "test commit") end def create_page(name, content) @@ -337,6 +333,6 @@ describe ProjectWiki do end def destroy_page(page) - subject.wiki.delete_page(page, commit_details) + subject.delete_page(page, commit_details) end end diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index 9ef8d117123..1f14d06997e 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -80,7 +80,7 @@ describe WikiPage do context "when initialized with an existing gollum page" do before do create_page("test page", "test content") - @page = wiki.wiki.paged("test page") + @page = wiki.wiki.page(title: "test page") @wiki_page = described_class.new(wiki, @page, true) end @@ -105,7 +105,7 @@ describe WikiPage do end it "sets the version attribute" do - expect(@wiki_page.version).to be_a Gollum::Git::Commit + expect(@wiki_page.version).to be_a Gitlab::Git::WikiPageVersion end end end @@ -321,14 +321,14 @@ describe WikiPage do end it 'returns true when requesting an old version' do - old_version = @page.versions.last.to_s + old_version = @page.versions.last.id old_page = wiki.find_page('Update', old_version) expect(old_page.historical?).to eq true end it 'returns false when requesting latest version' do - latest_version = @page.versions.first.to_s + latest_version = @page.versions.first.id latest_page = wiki.find_page('Update', latest_version) expect(latest_page.historical?).to eq false @@ -393,7 +393,7 @@ describe WikiPage do end def commit_details - { name: user.name, email: user.email, message: "test commit" } + Gitlab::Git::Wiki::CommitDetails.new(user.name, user.email, "test commit") end def create_page(name, content) @@ -401,8 +401,8 @@ describe WikiPage do end def destroy_page(title) - page = wiki.wiki.paged(title) - wiki.wiki.delete_page(page, commit_details) + page = wiki.wiki.page(title: title) + wiki.delete_page(page, commit_details) end def get_slugs(page_or_dir) diff --git a/spec/services/projects/create_service_spec.rb b/spec/services/projects/create_service_spec.rb index 2cc4643777e..dc89fdebce7 100644 --- a/spec/services/projects/create_service_spec.rb +++ b/spec/services/projects/create_service_spec.rb @@ -76,9 +76,8 @@ describe Projects::CreateService, '#execute' do context 'wiki_enabled true creates wiki repository directory' do it do project = create_project(user, opts) - path = ProjectWiki.new(project, user).send(:path_to_repo) - expect(File.exist?(path)).to be_truthy + expect(wiki_repo(project).exists?).to be_truthy end end @@ -86,11 +85,15 @@ describe Projects::CreateService, '#execute' do it do opts[:wiki_enabled] = false project = create_project(user, opts) - path = ProjectWiki.new(project, user).send(:path_to_repo) - expect(File.exist?(path)).to be_falsey + expect(wiki_repo(project).exists?).to be_falsey end end + + def wiki_repo(project) + relative_path = ProjectWiki.new(project).disk_path + '.git' + Gitlab::Git::Repository.new(project.repository_storage, relative_path, 'foobar') + end end context 'builds_enabled global setting' do diff --git a/spec/workers/repository_check/single_repository_worker_spec.rb b/spec/workers/repository_check/single_repository_worker_spec.rb index d2609d21546..1d9bbf2ca62 100644 --- a/spec/workers/repository_check/single_repository_worker_spec.rb +++ b/spec/workers/repository_check/single_repository_worker_spec.rb @@ -69,7 +69,12 @@ describe RepositoryCheck::SingleRepositoryWorker do end def break_wiki(project) - FileUtils.rm_rf(wiki_path(project) + '/objects') + objects_dir = wiki_path(project) + '/objects' + + # Replace the /objects directory with a file so that the repo is + # invalid, _and_ 'git init' cannot fix it. + FileUtils.rm_rf(objects_dir) + FileUtils.touch(objects_dir) if File.directory?(wiki_path(project)) end def wiki_path(project)