diff --git a/app/models/wiki.rb b/app/models/wiki.rb index cd6914b19eddbfcba0ec3baa59c916965099ccd7..1c3440818c03233a259d5b116cb6dcffdbee1e69 100644 --- a/app/models/wiki.rb +++ b/app/models/wiki.rb @@ -193,7 +193,9 @@ def has_home_page? end def empty? - !repository_exists? || list_page_paths(limit: 1).empty? + capture_git_error(:empty, response_on_error: true) do + !repository_exists? || list_page_paths(limit: 1).empty? + end end def exists? @@ -217,28 +219,30 @@ def list_pages( limit: 0, offset: 0 ) - create_wiki_repository unless repository_exists? + capture_git_error(:list, response_on_error: []) do + create_wiki_repository unless repository_exists? - paths = list_page_paths(limit: limit, offset: offset) - return [] if paths.empty? + paths = list_page_paths(limit: limit, offset: offset) + next [] if paths.empty? + + pages = paths.map do |path| + page = Gitlab::Git::WikiPage.new( + url_path: strip_extension(path), + title: canonicalize_filename(path), + format: find_page_format(path), + path: path, + raw_data: '', + name: canonicalize_filename(path), + historical: false + ) + WikiPage.new(self, page) + end + sort_pages!(pages, direction) + pages = pages.take(limit) if limit > 0 + fetch_pages_content!(pages, size_limit: size_limit) if load_content - pages = paths.map do |path| - page = Gitlab::Git::WikiPage.new( - url_path: strip_extension(path), - title: canonicalize_filename(path), - format: find_page_format(path), - path: path, - raw_data: '', - name: canonicalize_filename(path), - historical: false - ) - WikiPage.new(self, page) + pages end - sort_pages!(pages, direction) - pages = pages.take(limit) if limit > 0 - fetch_pages_content!(pages, size_limit: size_limit) if load_content - - pages end # Finds a page within the repository based on a title @@ -249,29 +253,31 @@ def list_pages( # # Returns an initialized WikiPage instance or nil def find_page(title, version = nil, load_content: true) - create_wiki_repository unless repository_exists? - - version = version.presence || default_branch - path = find_matched_file(title, version) - return if path.blank? - - path = Gitlab::EncodingHelper.encode_utf8_no_detect(path) - blob_options = load_content ? {} : { limit: 0 } - blob = repository.blob_at(version, path, **blob_options) - commit = repository.commit(blob.commit_id) - format = find_page_format(path) - - page = Gitlab::Git::WikiPage.new( - url_path: strip_extension(path), - title: canonicalize_filename(path), - format: format, - path: path, - raw_data: blob.data, - name: canonicalize_filename(path), - historical: version == default_branch ? false : check_page_historical(path, commit), - version: Gitlab::Git::WikiPageVersion.new(commit, format) - ) - WikiPage.new(self, page) + capture_git_error(:find, response_on_error: nil) do + create_wiki_repository unless repository_exists? + + version = version.presence || default_branch + path = find_matched_file(title, version) + next if path.blank? + + path = Gitlab::EncodingHelper.encode_utf8_no_detect(path) + blob_options = load_content ? {} : { limit: 0 } + blob = repository.blob_at(version, path, **blob_options) + commit = repository.commit(blob.commit_id) + format = find_page_format(path) + + page = Gitlab::Git::WikiPage.new( + url_path: strip_extension(path), + title: canonicalize_filename(path), + format: format, + path: path, + raw_data: blob.data, + name: canonicalize_filename(path), + historical: version == default_branch ? false : check_page_historical(path, commit), + version: Gitlab::Git::WikiPageVersion.new(commit, format) + ) + WikiPage.new(self, page) + end end def find_sidebar(version = nil) @@ -280,7 +286,9 @@ def find_sidebar(version = nil) def find_file(name, version = default_branch, load_content: true) data_limit = load_content ? -1 : 0 - blobs = repository.blobs_at([[version, name]], blob_size_limit: data_limit) + blobs = capture_git_error(:blob, response_on_error: []) do + repository.blobs_at([[version, name]], blob_size_limit: data_limit) + end return if blobs.empty? @@ -290,13 +298,14 @@ def find_file(name, version = default_branch, load_content: true) def create_page(title, content, format = :markdown, message = nil) with_valid_format(format) do |default_extension| sanitized_path = sluggified_full_path(title, default_extension) - # cannot create two pages with: - # - the same title but different format - # - the same title but different capitalization - # - the same title, different capitalization, and different format - next duplicated_page_error(sanitized_path) if file_exists_by_regex?(title) capture_git_error(:created) do + # cannot create two pages with: + # - the same title but different format + # - the same title but different capitalization + # - the same title, different capitalization, and different format + next duplicated_page_error(sanitized_path) if file_exists_by_regex?(title) + create_wiki_repository unless repository_exists? sanitized_path = sluggified_full_path(title, default_extension) options = multi_commit_options(:created, message, title) @@ -401,7 +410,9 @@ def full_path override :default_branch def default_branch - super || Gitlab::DefaultBranch.value(object: container) + capture_git_error(:default_branch, response_on_error: 'main') do + super || Gitlab::DefaultBranch.value(object: container) + end end def wiki_base_path @@ -426,6 +437,8 @@ def cleanup @repository = nil end + private + def capture_git_error(action, response_on_error: false, &block) wrapped_gitaly_errors(&block) rescue Gitlab::Git::Index::IndexError, @@ -441,8 +454,6 @@ def capture_git_error(action, response_on_error: false, &block) response_on_error end - private - def update_redirection_actions(new_path, old_path = nil, **options) return [] unless old_path != new_path diff --git a/app/services/wiki_pages/create_service.rb b/app/services/wiki_pages/create_service.rb index 0e36a903086edc70e5c86593bd3f947895d5cdfb..ac91f15cfe198efde8b081130d20c0669a36bdc9 100644 --- a/app/services/wiki_pages/create_service.rb +++ b/app/services/wiki_pages/create_service.rb @@ -6,11 +6,7 @@ def execute wiki = Wiki.for_container(container, current_user) page = WikiPage.new(wiki) - wiki.capture_git_error(event_action) do - page.create(@params) - end - - if page.persisted? + if page.create(@params) && page.persisted? execute_hooks(page) ServiceResponse.success(payload: { page: page }) else diff --git a/app/services/wiki_pages/update_service.rb b/app/services/wiki_pages/update_service.rb index 62dec9a2122e04990c4ef58ec46d342807d43a09..c49c1e2a1859593532bfef63c1dc735f075ee4c1 100644 --- a/app/services/wiki_pages/update_service.rb +++ b/app/services/wiki_pages/update_service.rb @@ -8,7 +8,7 @@ def execute(page) # this class is not thread safe! @old_slug = page.slug - if page.wiki.capture_git_error(event_action) { page.update(@params) } + if page.update(@params) execute_hooks(page) ServiceResponse.success(payload: { page: page }) else diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index 6769945084edd3857a353b3551842466de6beff2..2ce4c5b177a9b86ccaafbba5e79db0475ff2e78f 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -419,6 +419,25 @@ def force_wiki_change_branch expect(wiki.find_page(title).content).to eq(page.content) end end + + context 'when the repository fails' do + it 'do not create the page if the repository raise an error' do + page = build_wiki_page(container) + + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + saved = page.create(attributes) + + # unstub + allow(Gitlab::GitalyClient).to receive(:call).and_call_original + + expect(saved).to be(false) + expect(page.errors.messages[:base]).to include(/Gitaly broken in this spec/) + expect(wiki.find_page(title)).to be_nil + end + end end describe "dot in the title" do @@ -661,6 +680,28 @@ def force_wiki_change_branch expect(page.content).to eq 'test content' end end + + context 'when the repository fails' do + it 'do not update the page if the repository raise an error' do + page = create_wiki_page(container) + + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + saved = page.update(content: "new content") + + # unstub + allow(Gitlab::GitalyClient).to receive(:call).and_call_original + + expect(saved).to be(false) + expect(page.errors.messages[:base]).to include(/Gitaly broken in this spec/) + + page_found = wiki.find_page(original_title) + + expect(page_found.content).to eq 'test content' + end + end end describe "#delete" do @@ -671,6 +712,25 @@ def force_wiki_change_branch expect(page.delete).to eq(true) end.to change { wiki.list_pages.length }.by(-1) end + + context 'when the repository fails' do + it 'do not delete the page if the repository raise an error' do + page = create_wiki_page(container) + + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + deleted = page.delete + + # unstub + allow(Gitlab::GitalyClient).to receive(:call).and_call_original + + expect(deleted).to be(false) + expect(wiki.error_message).to match(/Gitaly broken in this spec/) + expect(wiki.list_pages.length).to be(1) + end + end end describe "#versions" do diff --git a/spec/services/wiki_pages/create_service_spec.rb b/spec/services/wiki_pages/create_service_spec.rb index ca2d38ad70ddd13246928d678fce7ac83504b443..efb2bd975487394e7f8b058b79696b362198aa29 100644 --- a/spec/services/wiki_pages/create_service_spec.rb +++ b/spec/services/wiki_pages/create_service_spec.rb @@ -4,24 +4,4 @@ RSpec.describe WikiPages::CreateService, feature_category: :wiki do it_behaves_like 'WikiPages::CreateService#execute', :project - - describe '#execute' do - let_it_be(:project) { create(:project) } - - subject(:service) { described_class.new(container: project) } - - context 'when wiki create fails due to git error' do - let(:wiki_git_error) { 'Could not create wiki page' } - - it 'catches the thrown error and returns a ServiceResponse error' do - allow_next_instance_of(WikiPage) do |instance| - allow(instance).to receive(:create).and_raise(Gitlab::Git::CommandError.new(wiki_git_error)) - end - - result = service.execute - expect(result).to be_error - expect(result.message).to eq(wiki_git_error) - end - end - end end diff --git a/spec/services/wiki_pages/update_service_spec.rb b/spec/services/wiki_pages/update_service_spec.rb index 79b2b55907b2f7b9bb9986acc78a67ff2348af12..d9da974069f72ece75301d50f21f10e06fb8e3ea 100644 --- a/spec/services/wiki_pages/update_service_spec.rb +++ b/spec/services/wiki_pages/update_service_spec.rb @@ -4,26 +4,4 @@ RSpec.describe WikiPages::UpdateService, feature_category: :wiki do it_behaves_like 'WikiPages::UpdateService#execute', :project - - describe '#execute' do - let_it_be(:project) { create(:project) } - - let(:page) { create(:wiki_page, project: project) } - - subject(:service) { described_class.new(container: project) } - - context 'when wiki create fails due to git error' do - let(:wiki_git_error) { 'Could not update wiki page' } - - it 'catches the thrown error and returns a ServiceResponse error' do - allow_next_instance_of(WikiPage) do |instance| - allow(instance).to receive(:update).and_raise(Gitlab::Git::CommandError.new(wiki_git_error)) - end - - result = service.execute(page) - expect(result).to be_error - expect(result.message).to eq(wiki_git_error) - end - end - end end diff --git a/spec/support/shared_examples/models/wiki_shared_examples.rb b/spec/support/shared_examples/models/wiki_shared_examples.rb index 6abd37e9a0f3274a1e29d907dec093491a942d64..c1f116577e4c789d7e97e9fb7e302e623092b317 100644 --- a/spec/support/shared_examples/models/wiki_shared_examples.rb +++ b/spec/support/shared_examples/models/wiki_shared_examples.rb @@ -188,6 +188,19 @@ end end end + + context 'when the repository fails' do + let!(:page) { create(:wiki_page, wiki: subject, title: 'test page') } + + it 'returns true if the repository raise an error' do + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + expect(subject.empty?).to be(true) + expect(subject.error_message).to match(/Gitaly broken in this spec/) + end + end end describe '#list_pages' do @@ -301,6 +314,19 @@ expect(pages.map(&:content)).to eq(contents) end end + + context 'when the repository fails to list' do + let!(:page) { create(:wiki_page, wiki: subject, title: 'test page') } + + it 'returns an empty list if the repository raise an error' do + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + expect(wiki_pages.count).to eq(0) + expect(subject.error_message).to match(/Gitaly broken in this spec/) + end + end end it_behaves_like 'wiki model #list_pages' @@ -344,6 +370,19 @@ expect(page).to be_a WikiPage end + context 'when the repository fails to find' do + let!(:page) { create(:wiki_page, wiki: subject, title: 'test page') } + + it 'returns nil if the repository raise an error' do + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + expect(subject.find_page('index page')).to be_nil + expect(subject.error_message).to match(/Gitaly broken in this spec/) + end + end + # The repository can contain files that were not generated by Gollum Wiki # and these files can contain spaces in the path. context 'pages with spaces in the path' do @@ -578,6 +617,19 @@ expect(file.mime_type).to eq('image/png') end end + + context 'when the repository fails to find' do + let!(:page) { create(:wiki_page, wiki: subject, title: 'test page') } + + it 'returns nil if the repository raise an error' do + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + expect(subject.find_file('image.png')).to be_nil + expect(subject.error_message).to match(/Gitaly broken in this spec/) + end + end end describe '#create_page' do @@ -660,15 +712,6 @@ expect(subject.error_message).to match(/Duplicate page:/) end - it 'returns false if the repository raise an error' do - expect(subject.repository) - .to receive(:commit_files) - .and_raise(Gitlab::Git::CommandError) - .at_least(:once) - - expect(subject.create_page('test page', 'content')).to eq(false) - end - it 'returns false if it has an invalid format', :aggregate_failures do expect(subject.create_page('test page', 'content', :foobar)).to eq false expect(subject.error_message).to match(/Invalid format selected/) @@ -758,6 +801,19 @@ expect(subject.create_page(new_file, 'content', format)).to eq success end end + + context 'when the repository fails to create' do + let!(:page) { create(:wiki_page, wiki: subject, title: 'test page') } + + it 'returns false if the repository raise an error' do + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + expect(subject.create_page('test page', 'content')).to eq(false) + expect(subject.error_message).to match(/Gitaly broken in this spec/) + end + end end it_behaves_like 'create_page tests' @@ -916,7 +972,17 @@ def update_page context 'when the repository fails to update' do let!(:page) { create(:wiki_page, wiki: subject, title: 'test page') } - it 'returns false and sets error message', :aggregate_failures do + it 'returns false if the repository raise an error' do + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + expect(subject.update_page(page.page, content: 'new content', format: :markdown)) + .to eq(false) + expect(subject.error_message).to match(/Gitaly broken in this spec/) + end + + it 'returns false and sets error message if the repository raise an IndexError', :aggregate_failures do expect(subject.repository) .to receive(:commit_files) .and_raise(Gitlab::Git::Index::IndexError.new) @@ -1024,6 +1090,22 @@ def update_page expect(subject).to eq wiki.repository.root_ref end end + + context 'when the repository fails' do + before do + wiki.repository.create_if_not_exists + wiki.create_page('index', 'test content') + end + + it 'returns main branch if the repository raise an error' do + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + expect(subject).to eq 'main' + expect(wiki.error_message).to match(/Gitaly broken in this spec/) + end + end end describe '#create_wiki_repository' do diff --git a/spec/support/shared_examples/services/wiki_pages/create_service_shared_examples.rb b/spec/support/shared_examples/services/wiki_pages/create_service_shared_examples.rb index baddb5e9c253ac96c45148f9ba852211c557602a..8540b1961f0ff8963056da1df346f2f4b5155ab1 100644 --- a/spec/support/shared_examples/services/wiki_pages/create_service_shared_examples.rb +++ b/spec/support/shared_examples/services/wiki_pages/create_service_shared_examples.rb @@ -124,4 +124,19 @@ .and have_attributes(errors: be_present) end end + + context 'when wiki create fails due to git error' do + it 'catches the thrown error and returns a ServiceResponse error' do + container = create(container_type, :wiki_repo) + service = described_class.new(container: container, current_user: user, params: opts) + + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + result = service.execute + expect(result).to be_error + expect(result.message).to eq('Could not create wiki page') + end + end end diff --git a/spec/support/shared_examples/services/wiki_pages/destroy_service_shared_examples.rb b/spec/support/shared_examples/services/wiki_pages/destroy_service_shared_examples.rb index a369e396e619755dbdc105e375834a67136f473c..c2cdfdf15f66482ce5e2b9ccff043be8509d72ab 100644 --- a/spec/support/shared_examples/services/wiki_pages/destroy_service_shared_examples.rb +++ b/spec/support/shared_examples/services/wiki_pages/destroy_service_shared_examples.rb @@ -79,4 +79,20 @@ service.execute(page) end end + + context 'when wiki delete fails due to git error' do + it 'catches the thrown error and returns a ServiceResponse error' do + container = create(container_type, :wiki_repo) + page = create(:wiki_page, container: container) + service = described_class.new(container: container, current_user: user) + + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + result = service.execute(page) + expect(result).to be_error + expect(result.message).to eq('Could not delete wiki page') + end + end end diff --git a/spec/support/shared_examples/services/wiki_pages/update_service_shared_examples.rb b/spec/support/shared_examples/services/wiki_pages/update_service_shared_examples.rb index edf22868cdab4324b0df226c4fc68ebd15f8445a..0fd043e95e92c887e7072cd67864ea15eaca88cb 100644 --- a/spec/support/shared_examples/services/wiki_pages/update_service_shared_examples.rb +++ b/spec/support/shared_examples/services/wiki_pages/update_service_shared_examples.rb @@ -144,4 +144,20 @@ .and have_attributes(errors: be_present) end end + + context 'when wiki update fails due to git error' do + it 'catches the thrown error and returns a ServiceResponse error' do + container = create(container_type, :wiki_repo) + page = create(:wiki_page, container: container) + service = described_class.new(container: container, current_user: user, params: opts) + + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + result = service.execute(page) + expect(result).to be_error + expect(result.message).to eq('Could not update wiki page') + end + end end