From 0543e7606e80740041dbf1419514ec0dfd747e83 Mon Sep 17 00:00:00 2001 From: Dannyel Cardoso da Fonseca Date: Thu, 27 Jun 2024 13:03:41 +0200 Subject: [PATCH 01/10] Set #capture_git_error as private --- app/models/wiki.rb | 15 ++++++++------- app/services/wiki_pages/create_service.rb | 6 +----- app/services/wiki_pages/update_service.rb | 2 +- 3 files changed, 10 insertions(+), 13 deletions(-) diff --git a/app/models/wiki.rb b/app/models/wiki.rb index cd6914b19eddbf..cc2ff128c206ac 100644 --- a/app/models/wiki.rb +++ b/app/models/wiki.rb @@ -290,13 +290,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) @@ -426,6 +427,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 +444,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 0e36a903086edc..ac91f15cfe198e 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 62dec9a2122e04..c49c1e2a185959 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 -- GitLab From 69892d6b375da7b7715efad7f2a4e8fa35ee7f11 Mon Sep 17 00:00:00 2001 From: Dannyel Cardoso da Fonseca Date: Thu, 27 Jun 2024 13:04:24 +0200 Subject: [PATCH 02/10] Refactor wiki_pages services specs --- .../wiki_pages/create_service_spec.rb | 17 +++++++---- .../wiki_pages/destroy_service_spec.rb | 30 +++++++++++++++++++ .../wiki_pages/update_service_spec.rb | 18 +++++++---- 3 files changed, 54 insertions(+), 11 deletions(-) diff --git a/spec/services/wiki_pages/create_service_spec.rb b/spec/services/wiki_pages/create_service_spec.rb index ca2d38ad70ddd1..6c8673a1049d85 100644 --- a/spec/services/wiki_pages/create_service_spec.rb +++ b/spec/services/wiki_pages/create_service_spec.rb @@ -7,17 +7,22 @@ describe '#execute' do let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } - subject(:service) { described_class.new(container: project) } + let(:opts) do + { + title: 'Title', + content: 'Content for wiki page', + format: 'markdown' + } + end + + subject(:service) { described_class.new(container: project, current_user: user, params: opts) } - context 'when wiki create fails due to git error' do + context 'when wiki create fails due to git error', :broken_storage 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) diff --git a/spec/services/wiki_pages/destroy_service_spec.rb b/spec/services/wiki_pages/destroy_service_spec.rb index ff29fc59b3e3c8..74d8e95cb00073 100644 --- a/spec/services/wiki_pages/destroy_service_spec.rb +++ b/spec/services/wiki_pages/destroy_service_spec.rb @@ -4,4 +4,34 @@ RSpec.describe WikiPages::DestroyService, feature_category: :wiki do it_behaves_like 'WikiPages::DestroyService#execute', :project + + describe '#execute' do + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + let!(:page) { create(:wiki_page) } + let(:opts) do + { + title: 'Title', + content: 'Content for wiki page', + format: 'markdown' + } + end + + subject(:service) { described_class.new(container: project, current_user: user, params: opts) } + + context 'when wiki create fails due to git error' do + let(:wiki_git_error) { 'Could not delete wiki page' } + + it 'catches the thrown error and returns a ServiceResponse error' do + 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(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 79b2b55907b2f7..2cfd24e376e0a8 100644 --- a/spec/services/wiki_pages/update_service_spec.rb +++ b/spec/services/wiki_pages/update_service_spec.rb @@ -7,17 +7,25 @@ describe '#execute' do let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + + let!(:page) { create(:wiki_page) } + let(:opts) do + { + title: 'Title', + content: 'Content for wiki page', + format: 'markdown' + } + end - let(:page) { create(:wiki_page, project: project) } - - subject(:service) { described_class.new(container: project) } + subject(:service) { described_class.new(container: project, current_user: user, params: opts) } 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)) + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' end result = service.execute(page) -- GitLab From f4b6be4faada206acb64cec9092c95213507f93c Mon Sep 17 00:00:00 2001 From: Dannyel Cardoso da Fonseca Date: Thu, 27 Jun 2024 20:44:03 +0200 Subject: [PATCH 03/10] Wrap wiki's public methods with capture_git_error --- app/models/wiki.rb | 100 +++++++++-------- .../models/wiki_shared_examples.rb | 102 ++++++++++++++++-- 2 files changed, 147 insertions(+), 55 deletions(-) diff --git a/app/models/wiki.rb b/app/models/wiki.rb index cc2ff128c206ac..1c3440818c0323 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? @@ -402,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 diff --git a/spec/support/shared_examples/models/wiki_shared_examples.rb b/spec/support/shared_examples/models/wiki_shared_examples.rb index 6abd37e9a0f327..c1f116577e4c78 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 -- GitLab From 7294eb8b6788a38617e16e4fb37cb65efc9ee43e Mon Sep 17 00:00:00 2001 From: Dannyel Cardoso da Fonseca Date: Thu, 27 Jun 2024 20:58:43 +0200 Subject: [PATCH 04/10] Add tests to wiki_page --- spec/models/wiki_page_spec.rb | 62 +++++++++++++++++++++++++++++++++++ 1 file changed, 62 insertions(+) diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index 6769945084edd3..a83038132fb847 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 + let!(:container) { create(:project) } + + it 'do not create the page if the repository raise an error' do + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + saved = subject.create(attributes) + + # unstub + allow(Gitlab::GitalyClient).to receive(:call).and_call_original + + expect(saved).to be(false) + expect(subject.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 + let!(:container) { create(:project) } + + it 'do not update the page if the repository raise an error' do + allow(Gitlab::GitalyClient).to receive(:call) do + raise GRPC::Unavailable, 'Gitaly broken in this spec' + end + + saved = subject.update(content: "new content") + + # unstub + allow(Gitlab::GitalyClient).to receive(:call).and_call_original + + expect(saved).to be(false) + expect(subject.errors.messages[:base]).to include(/Gitaly broken in this spec/) + + page = wiki.find_page(original_title) + + expect(page.content).to eq 'test content' + end + end end describe "#delete" do @@ -671,6 +712,27 @@ 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 + let!(:container) { create(:project) } + + 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 -- GitLab From 6b5bcc14aa31a0f2391596a6a6ae9550d81b0d81 Mon Sep 17 00:00:00 2001 From: Dannyel Cardoso da Fonseca Date: Wed, 11 Sep 2024 22:53:13 +0200 Subject: [PATCH 05/10] Avoiding the use of `let!` --- spec/models/wiki_page_spec.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index a83038132fb847..c57c8151039947 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -421,9 +421,9 @@ def force_wiki_change_branch end context 'when the repository fails' do - let!(:container) { create(:project) } - it 'do not create the page if the repository raise an error' do + subject { build_wiki_page(container) } + allow(Gitlab::GitalyClient).to receive(:call) do raise GRPC::Unavailable, 'Gitaly broken in this spec' end @@ -682,9 +682,9 @@ def force_wiki_change_branch end context 'when the repository fails' do - let!(:container) { create(:project) } - it 'do not update the page if the repository raise an error' do + subject { build_wiki_page(container) } + allow(Gitlab::GitalyClient).to receive(:call) do raise GRPC::Unavailable, 'Gitaly broken in this spec' end @@ -714,8 +714,6 @@ def force_wiki_change_branch end context 'when the repository fails' do - let!(:container) { create(:project) } - it 'do not delete the page if the repository raise an error' do page = create_wiki_page(container) -- GitLab From 51d69d6625a002415b83e13ae9b7d9da2344c968 Mon Sep 17 00:00:00 2001 From: Dannyel Cardoso da Fonseca Date: Wed, 11 Sep 2024 22:58:22 +0200 Subject: [PATCH 06/10] Avoiding the use of the :broken_storage trait --- spec/services/wiki_pages/create_service_spec.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/spec/services/wiki_pages/create_service_spec.rb b/spec/services/wiki_pages/create_service_spec.rb index 6c8673a1049d85..f523c95e44c8d4 100644 --- a/spec/services/wiki_pages/create_service_spec.rb +++ b/spec/services/wiki_pages/create_service_spec.rb @@ -19,10 +19,14 @@ subject(:service) { described_class.new(container: project, current_user: user, params: opts) } - context 'when wiki create fails due to git error', :broken_storage do + 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(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(wiki_git_error) -- GitLab From c229901baa6664f7670f8203c01f53d7af3083da Mon Sep 17 00:00:00 2001 From: Dannyel Cardoso da Fonseca Date: Wed, 11 Sep 2024 23:56:41 +0200 Subject: [PATCH 07/10] Move tests to shared examples --- .../wiki_pages/create_service_spec.rb | 29 ------------------ .../wiki_pages/destroy_service_spec.rb | 30 ------------------- .../wiki_pages/update_service_spec.rb | 30 ------------------- .../create_service_shared_examples.rb | 17 +++++++++++ .../destroy_service_shared_examples.rb | 20 ++++++++++++- .../update_service_shared_examples.rb | 18 +++++++++++ 6 files changed, 54 insertions(+), 90 deletions(-) diff --git a/spec/services/wiki_pages/create_service_spec.rb b/spec/services/wiki_pages/create_service_spec.rb index f523c95e44c8d4..efb2bd97548739 100644 --- a/spec/services/wiki_pages/create_service_spec.rb +++ b/spec/services/wiki_pages/create_service_spec.rb @@ -4,33 +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) } - let_it_be(:user) { create(:user) } - - let(:opts) do - { - title: 'Title', - content: 'Content for wiki page', - format: 'markdown' - } - end - - subject(:service) { described_class.new(container: project, current_user: user, params: opts) } - - 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(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(wiki_git_error) - end - end - end end diff --git a/spec/services/wiki_pages/destroy_service_spec.rb b/spec/services/wiki_pages/destroy_service_spec.rb index 74d8e95cb00073..ff29fc59b3e3c8 100644 --- a/spec/services/wiki_pages/destroy_service_spec.rb +++ b/spec/services/wiki_pages/destroy_service_spec.rb @@ -4,34 +4,4 @@ RSpec.describe WikiPages::DestroyService, feature_category: :wiki do it_behaves_like 'WikiPages::DestroyService#execute', :project - - describe '#execute' do - let_it_be(:project) { create(:project) } - let_it_be(:user) { create(:user) } - - let!(:page) { create(:wiki_page) } - let(:opts) do - { - title: 'Title', - content: 'Content for wiki page', - format: 'markdown' - } - end - - subject(:service) { described_class.new(container: project, current_user: user, params: opts) } - - context 'when wiki create fails due to git error' do - let(:wiki_git_error) { 'Could not delete wiki page' } - - it 'catches the thrown error and returns a ServiceResponse error' do - 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(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 2cfd24e376e0a8..d9da974069f72e 100644 --- a/spec/services/wiki_pages/update_service_spec.rb +++ b/spec/services/wiki_pages/update_service_spec.rb @@ -4,34 +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_it_be(:user) { create(:user) } - - let!(:page) { create(:wiki_page) } - let(:opts) do - { - title: 'Title', - content: 'Content for wiki page', - format: 'markdown' - } - end - - subject(:service) { described_class.new(container: project, current_user: user, params: opts) } - - 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(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(wiki_git_error) - end - end - end end 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 baddb5e9c253ac..070972977d8b84 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,21 @@ .and have_attributes(errors: be_present) end end + + 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 + 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(wiki_git_error) + 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 a369e396e61975..56b09599764125 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 @@ -4,7 +4,7 @@ let(:container) { create(container_type) } # rubocop:disable Rails/SaveBang let(:user) { create(:user) } - let(:page) { create(:wiki_page) } + let(:page) { create(:wiki_page, container: container) } let!(:wiki_page_meta) { create(:wiki_page_meta, container: container) } subject(:service) { described_class.new(container: container, current_user: user) } @@ -79,4 +79,22 @@ service.execute(page) end end + + context 'when wiki delete fails due to git error' do + let(:wiki_git_error) { 'Could not delete wiki page' } + + 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(wiki_git_error) + 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 edf22868cdab43..14172b25807281 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,22 @@ .and have_attributes(errors: be_present) end end + + context 'when wiki update 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 + 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(wiki_git_error) + end + end end -- GitLab From 2ea6d5ae54b9ada4aa209e2aeec49b5416fe3cb1 Mon Sep 17 00:00:00 2001 From: Dannyel Cardoso da Fonseca Date: Tue, 8 Oct 2024 20:32:06 +0200 Subject: [PATCH 08/10] Avoid to overwrite `subject` object --- spec/models/wiki_page_spec.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/spec/models/wiki_page_spec.rb b/spec/models/wiki_page_spec.rb index c57c8151039947..2ce4c5b177a9b8 100644 --- a/spec/models/wiki_page_spec.rb +++ b/spec/models/wiki_page_spec.rb @@ -422,19 +422,19 @@ def force_wiki_change_branch context 'when the repository fails' do it 'do not create the page if the repository raise an error' do - subject { build_wiki_page(container) } + page = build_wiki_page(container) allow(Gitlab::GitalyClient).to receive(:call) do raise GRPC::Unavailable, 'Gitaly broken in this spec' end - saved = subject.create(attributes) + saved = page.create(attributes) # unstub allow(Gitlab::GitalyClient).to receive(:call).and_call_original expect(saved).to be(false) - expect(subject.errors.messages[:base]).to include(/Gitaly broken in this spec/) + expect(page.errors.messages[:base]).to include(/Gitaly broken in this spec/) expect(wiki.find_page(title)).to be_nil end end @@ -683,23 +683,23 @@ def force_wiki_change_branch context 'when the repository fails' do it 'do not update the page if the repository raise an error' do - subject { build_wiki_page(container) } + page = create_wiki_page(container) allow(Gitlab::GitalyClient).to receive(:call) do raise GRPC::Unavailable, 'Gitaly broken in this spec' end - saved = subject.update(content: "new content") + saved = page.update(content: "new content") # unstub allow(Gitlab::GitalyClient).to receive(:call).and_call_original expect(saved).to be(false) - expect(subject.errors.messages[:base]).to include(/Gitaly broken in this spec/) + expect(page.errors.messages[:base]).to include(/Gitaly broken in this spec/) - page = wiki.find_page(original_title) + page_found = wiki.find_page(original_title) - expect(page.content).to eq 'test content' + expect(page_found.content).to eq 'test content' end end end -- GitLab From 75ac88dc52c2cfb39999ad1f183073ec4b652827 Mon Sep 17 00:00:00 2001 From: Dannyel Cardoso da Fonseca Date: Tue, 8 Oct 2024 20:37:07 +0200 Subject: [PATCH 09/10] Replace variable with its direct value --- .../services/wiki_pages/create_service_shared_examples.rb | 4 +--- .../services/wiki_pages/destroy_service_shared_examples.rb | 4 +--- .../services/wiki_pages/update_service_shared_examples.rb | 4 +--- 3 files changed, 3 insertions(+), 9 deletions(-) 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 070972977d8b84..8540b1961f0ff8 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 @@ -126,8 +126,6 @@ end 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 container = create(container_type, :wiki_repo) service = described_class.new(container: container, current_user: user, params: opts) @@ -138,7 +136,7 @@ result = service.execute expect(result).to be_error - expect(result.message).to eq(wiki_git_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 56b09599764125..3eed964bfca4ca 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 @@ -81,8 +81,6 @@ end context 'when wiki delete fails due to git error' do - let(:wiki_git_error) { 'Could not delete wiki page' } - it 'catches the thrown error and returns a ServiceResponse error' do container = create(container_type, :wiki_repo) page = create(:wiki_page, container: container) @@ -94,7 +92,7 @@ result = service.execute(page) expect(result).to be_error - expect(result.message).to eq(wiki_git_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 14172b25807281..0fd043e95e92c8 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 @@ -146,8 +146,6 @@ end context 'when wiki update 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 container = create(container_type, :wiki_repo) page = create(:wiki_page, container: container) @@ -159,7 +157,7 @@ result = service.execute(page) expect(result).to be_error - expect(result.message).to eq(wiki_git_error) + expect(result.message).to eq('Could not update wiki page') end end end -- GitLab From f273334c869bc3b884d4a67dcfd325211b32f74e Mon Sep 17 00:00:00 2001 From: Dannyel Cardoso da Fonseca Date: Thu, 10 Oct 2024 23:20:20 +0200 Subject: [PATCH 10/10] Remove container in rspec wiki_page factory --- .../services/wiki_pages/destroy_service_shared_examples.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 3eed964bfca4ca..c2cdfdf15f6648 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 @@ -4,7 +4,7 @@ let(:container) { create(container_type) } # rubocop:disable Rails/SaveBang let(:user) { create(:user) } - let(:page) { create(:wiki_page, container: container) } + let(:page) { create(:wiki_page) } let!(:wiki_page_meta) { create(:wiki_page_meta, container: container) } subject(:service) { described_class.new(container: container, current_user: user) } -- GitLab