From ac8f64cc65974b8d2a56c1b60277059b6c1efbc5 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Fri, 28 Jun 2019 16:07:10 +0100 Subject: [PATCH 1/5] More informative error state when pages are not found Some operations assume that finds are successful. This results in very unhelpful error messages (`NoMethodError` on `NilClass`). This change means that such operations throw instead, with an error message that names the page that could not be found. --- .../unreleased/update-page-check-page.yml | 4 +++ ruby/lib/gitlab/git/wiki.rb | 3 +- ruby/spec/lib/gitlab/git/wiki_spec.rb | 32 +++++++++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/update-page-check-page.yml diff --git a/changelogs/unreleased/update-page-check-page.yml b/changelogs/unreleased/update-page-check-page.yml new file mode 100644 index 0000000000..ca9784a359 --- /dev/null +++ b/changelogs/unreleased/update-page-check-page.yml @@ -0,0 +1,4 @@ +--- +title: More informative error states for missing pages +merge_request: 1340 +type: fixed diff --git a/ruby/lib/gitlab/git/wiki.rb b/ruby/lib/gitlab/git/wiki.rb index bb31492926..788cc455c7 100644 --- a/ruby/lib/gitlab/git/wiki.rb +++ b/ruby/lib/gitlab/git/wiki.rb @@ -3,6 +3,7 @@ module Gitlab class Wiki DuplicatePageError = Class.new(StandardError) OperationError = Class.new(StandardError) + PageNotFound = Class.new(StandardError) CommitDetails = Struct.new(:user_id, :username, :name, :email, :message) do def to_h @@ -135,7 +136,7 @@ module Gitlab page_name = Gollum::Page.canonicalize_filename(page_path) page_dir = File.split(page_path).first - gollum_wiki.paged(page_name, page_dir) + gollum_wiki.paged(page_name, page_dir) || (raise PageNotFound, "#{page_dir}/#{page_path}") end def gollum_write_page(name, format, content, commit_details) diff --git a/ruby/spec/lib/gitlab/git/wiki_spec.rb b/ruby/spec/lib/gitlab/git/wiki_spec.rb index 08292df97d..9069968adb 100644 --- a/ruby/spec/lib/gitlab/git/wiki_spec.rb +++ b/ruby/spec/lib/gitlab/git/wiki_spec.rb @@ -75,6 +75,35 @@ describe Gitlab::Git::Wiki do end end + describe '#update_page' do + let(:old_title) { 'page1' } + let(:new_content) { 'different content' } + let(:new_title) { 'new title' } + let(:deets) { commit_details('update') } + + before do + create_page(old_title, 'some content') + end + after do + destroy_page(new_title) + rescue Gitlab::Git::Wiki::PageNotFound + destroy_page(old_title) + end + + it 'can update the page' do + subject.update_page(old_title, new_title, :markdown, new_content, deets) + + expect(subject.pages.count).to eq 1 + expect(subject.pages.first.title).to eq new_title + expect(subject.pages.first.text_data).to eq new_content + end + + it 'raises PageNotFound when trying to access an unknown page' do + expect { subject.update_page('bad path', new_title, :markdown, new_content, deets) } + .to raise_error(Gitlab::Git::Wiki::PageNotFound) + end + end + def create_page(name, content) subject.write_page(name, :markdown, content, commit_details(name)) end @@ -85,6 +114,9 @@ describe Gitlab::Git::Wiki do def destroy_page(title, dir = '') page = subject.page(title: title, dir: dir) + + raise Gitlab::Git::Wiki::PageNotFound, title unless page + subject.delete_page(page.path, commit_details(title)) end end -- GitLab From 06de12271ca553b4d002a909e16ec278eb4b41bb Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 1 Jul 2019 11:02:36 +0100 Subject: [PATCH 2/5] Add golang service integration tests This tests for this scenario (not-found when updating the page), and checks that we get an appropriate message back. --- internal/service/wiki/update_page_test.go | 48 +++++++++++++++++++++++ ruby/lib/gitaly_server/wiki_service.rb | 2 + ruby/lib/gitlab/git/wiki.rb | 8 +++- 3 files changed, 56 insertions(+), 2 deletions(-) diff --git a/internal/service/wiki/update_page_test.go b/internal/service/wiki/update_page_test.go index 696b7d84e8..160feca731 100644 --- a/internal/service/wiki/update_page_test.go +++ b/internal/service/wiki/update_page_test.go @@ -11,6 +11,54 @@ import ( "google.golang.org/grpc/codes" ) +func TestFailedWikiUpdatePageDueToNotFound(t *testing.T) { + wikiRepo, _, cleanupFunc := setupWikiRepo(t) + defer cleanupFunc() + + server, serverSocketPath := runWikiServiceServer(t) + defer server.Stop() + + client, conn := newWikiClient(t, serverSocketPath) + defer conn.Close() + + pageName := "foo/Installing Gitaly" + content := []byte("Mock wiki page content") + commitDetails := &gitalypb.WikiCommitDetails{ + Name: []byte("Ahmad Sherif"), + Email: []byte("ahmad@gitlab.com"), + Message: []byte("Add " + pageName), + UserId: int32(1), + UserName: []byte("ahmad"), + } + + writeWikiPage(t, client, wikiRepo, createWikiPageOpts{title: pageName, content: content}) + + request := &gitalypb.WikiUpdatePageRequest{ + Repository: wikiRepo, + PagePath: []byte("/foo/Installing Gibaly"), + Title: []byte("Installing Gibaly"), + Format: "markdown", + CommitDetails: commitDetails, + Content: []byte("new content"), + } + + ctx, cancel := testhelper.Context() + defer cancel() + + stream, err := client.WikiUpdatePage(ctx) + require.NoError(t, err) + + require.NoError(t, stream.Send(request)) + + response, err := stream.CloseAndRecv() + require.NoError(t, err) + + expectedMessage := "page not found: /foo/Installing Gibaly" + + expectedResponse := &gitalypb.WikiUpdatePageResponse{Error: []byte(expectedMessage)} + require.Equal(t, expectedResponse, response, "mismatched response") +} + func TestSuccessfulWikiUpdatePageRequest(t *testing.T) { wikiRepo, wikiRepoPath, cleanupFunc := setupWikiRepo(t) defer cleanupFunc() diff --git a/ruby/lib/gitaly_server/wiki_service.rb b/ruby/lib/gitaly_server/wiki_service.rb index 76ab62b254..ac038906b8 100644 --- a/ruby/lib/gitaly_server/wiki_service.rb +++ b/ruby/lib/gitaly_server/wiki_service.rb @@ -177,6 +177,8 @@ module GitalyServer wiki.update_page(page_path, title, format.to_sym, content, commit_details) Gitaly::WikiUpdatePageResponse.new + rescue Gitlab::Git::Wiki::PageNotFound => e + Gitaly::WikiUpdatePageResponse.new(error: e.message.b) end def wiki_get_formatted_data(request, call) diff --git a/ruby/lib/gitlab/git/wiki.rb b/ruby/lib/gitlab/git/wiki.rb index 788cc455c7..5de7d7ad93 100644 --- a/ruby/lib/gitlab/git/wiki.rb +++ b/ruby/lib/gitlab/git/wiki.rb @@ -3,7 +3,11 @@ module Gitlab class Wiki DuplicatePageError = Class.new(StandardError) OperationError = Class.new(StandardError) - PageNotFound = Class.new(StandardError) + PageNotFound = Class.new(StandardError) do + def message + "page not found: #{super}" + end + end CommitDetails = Struct.new(:user_id, :username, :name, :email, :message) do def to_h @@ -136,7 +140,7 @@ module Gitlab page_name = Gollum::Page.canonicalize_filename(page_path) page_dir = File.split(page_path).first - gollum_wiki.paged(page_name, page_dir) || (raise PageNotFound, "#{page_dir}/#{page_path}") + gollum_wiki.paged(page_name, page_dir) || (raise PageNotFound, page_path) end def gollum_write_page(name, format, content, commit_details) -- GitLab From 8b7f79a458ea2c7c0d7b461b7dc64666451242b6 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 1 Jul 2019 12:20:16 +0100 Subject: [PATCH 3/5] use the GPRC::NotFound error, as appropriate --- internal/service/wiki/delete_page_test.go | 2 +- internal/service/wiki/update_page_test.go | 60 +++++------------------ ruby/lib/gitaly_server/wiki_service.rb | 4 +- ruby/lib/gitlab/git/wiki.rb | 7 +-- 4 files changed, 18 insertions(+), 55 deletions(-) diff --git a/internal/service/wiki/delete_page_test.go b/internal/service/wiki/delete_page_test.go index 85e8879bbc..4df9c81aac 100644 --- a/internal/service/wiki/delete_page_test.go +++ b/internal/service/wiki/delete_page_test.go @@ -111,7 +111,7 @@ func TestFailedWikiDeletePageDueToValidations(t *testing.T) { PagePath: []byte("does-not-exist"), CommitDetails: commitDetails, }, - code: codes.Unknown, + code: codes.NotFound, }, { desc: "empty page path", diff --git a/internal/service/wiki/update_page_test.go b/internal/service/wiki/update_page_test.go index 160feca731..903c719a8c 100644 --- a/internal/service/wiki/update_page_test.go +++ b/internal/service/wiki/update_page_test.go @@ -11,54 +11,6 @@ import ( "google.golang.org/grpc/codes" ) -func TestFailedWikiUpdatePageDueToNotFound(t *testing.T) { - wikiRepo, _, cleanupFunc := setupWikiRepo(t) - defer cleanupFunc() - - server, serverSocketPath := runWikiServiceServer(t) - defer server.Stop() - - client, conn := newWikiClient(t, serverSocketPath) - defer conn.Close() - - pageName := "foo/Installing Gitaly" - content := []byte("Mock wiki page content") - commitDetails := &gitalypb.WikiCommitDetails{ - Name: []byte("Ahmad Sherif"), - Email: []byte("ahmad@gitlab.com"), - Message: []byte("Add " + pageName), - UserId: int32(1), - UserName: []byte("ahmad"), - } - - writeWikiPage(t, client, wikiRepo, createWikiPageOpts{title: pageName, content: content}) - - request := &gitalypb.WikiUpdatePageRequest{ - Repository: wikiRepo, - PagePath: []byte("/foo/Installing Gibaly"), - Title: []byte("Installing Gibaly"), - Format: "markdown", - CommitDetails: commitDetails, - Content: []byte("new content"), - } - - ctx, cancel := testhelper.Context() - defer cancel() - - stream, err := client.WikiUpdatePage(ctx) - require.NoError(t, err) - - require.NoError(t, stream.Send(request)) - - response, err := stream.CloseAndRecv() - require.NoError(t, err) - - expectedMessage := "page not found: /foo/Installing Gibaly" - - expectedResponse := &gitalypb.WikiUpdatePageResponse{Error: []byte(expectedMessage)} - require.Equal(t, expectedResponse, response, "mismatched response") -} - func TestSuccessfulWikiUpdatePageRequest(t *testing.T) { wikiRepo, wikiRepoPath, cleanupFunc := setupWikiRepo(t) defer cleanupFunc() @@ -194,6 +146,18 @@ func TestFailedWikiUpdatePageDueToValidations(t *testing.T) { }, code: codes.InvalidArgument, }, + { + desc: "page does not exist", + request: &gitalypb.WikiUpdatePageRequest{ + Repository: wikiRepo, + PagePath: []byte("//Installing Gibaly"), + Title: []byte("Installing Gitaly"), + Format: "markdown", + CommitDetails: commitDetails, + Content: []byte(""), + }, + code: codes.NotFound, + }, { desc: "empty title", request: &gitalypb.WikiUpdatePageRequest{ diff --git a/ruby/lib/gitaly_server/wiki_service.rb b/ruby/lib/gitaly_server/wiki_service.rb index ac038906b8..ac84bed1a3 100644 --- a/ruby/lib/gitaly_server/wiki_service.rb +++ b/ruby/lib/gitaly_server/wiki_service.rb @@ -11,6 +11,8 @@ module GitalyServer wiki.delete_page(page_path, commit_details) Gitaly::WikiDeletePageResponse.new + rescue Gitlab::Git::Wiki::PageNotFound => e + raise GRPC::NotFound, e.message end def wiki_write_page(call) @@ -178,7 +180,7 @@ module GitalyServer Gitaly::WikiUpdatePageResponse.new rescue Gitlab::Git::Wiki::PageNotFound => e - Gitaly::WikiUpdatePageResponse.new(error: e.message.b) + raise GRPC::NotFound, e.message end def wiki_get_formatted_data(request, call) diff --git a/ruby/lib/gitlab/git/wiki.rb b/ruby/lib/gitlab/git/wiki.rb index 5de7d7ad93..025f7b940a 100644 --- a/ruby/lib/gitlab/git/wiki.rb +++ b/ruby/lib/gitlab/git/wiki.rb @@ -3,11 +3,7 @@ module Gitlab class Wiki DuplicatePageError = Class.new(StandardError) OperationError = Class.new(StandardError) - PageNotFound = Class.new(StandardError) do - def message - "page not found: #{super}" - end - end + PageNotFound = Class.new(StandardError) CommitDetails = Struct.new(:user_id, :username, :name, :email, :message) do def to_h @@ -136,6 +132,7 @@ module Gitlab offset: options[:offset]) end + # Retrieve the page at that `page_path`, raising an error if it does not exist def gollum_page_by_path(page_path) page_name = Gollum::Page.canonicalize_filename(page_path) page_dir = File.split(page_path).first -- GitLab From a51c63f73cdad0b8a6f383a32ea718286773c183 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Mon, 1 Jul 2019 12:26:19 +0100 Subject: [PATCH 4/5] Remove unused method definition There are no callers to this method, so it is best removed --- ruby/lib/gitlab/git/wiki.rb | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/ruby/lib/gitlab/git/wiki.rb b/ruby/lib/gitlab/git/wiki.rb index 025f7b940a..202234ef6d 100644 --- a/ruby/lib/gitlab/git/wiki.rb +++ b/ruby/lib/gitlab/git/wiki.rb @@ -51,19 +51,6 @@ module Gitlab gollum_find_file(name, version) end - # options: - # :page - The Integer page number. - # :per_page - The number of items per page. - # :limit - Total number of items to return. - def page_versions(page_path, options = {}) - current_page = gollum_page_by_path(page_path) - - commits_from_page(current_page, options).map do |gitlab_git_commit| - gollum_page = gollum_wiki.page(current_page.title, gitlab_git_commit.id) - Gitlab::Git::WikiPageVersion.new(gitlab_git_commit, gollum_page&.format) - end - end - def count_page_versions(page_path) @repository.count_commits(ref: 'HEAD', path: page_path) end -- GitLab From 1514838a9257bbfa5e913c9a12d08bd622cb9570 Mon Sep 17 00:00:00 2001 From: Alex Kalderimis Date: Tue, 2 Jul 2019 09:34:45 +0100 Subject: [PATCH 5/5] Inherit from GRPC::NotFound instead of StandardError This has the advantage of not having to explicitly catch this error in the service, and meaning that we will never get `UnknownError` if we throw PageNotFound. --- ruby/lib/gitaly_server/wiki_service.rb | 4 ---- ruby/lib/gitlab/git/wiki.rb | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/ruby/lib/gitaly_server/wiki_service.rb b/ruby/lib/gitaly_server/wiki_service.rb index ac84bed1a3..76ab62b254 100644 --- a/ruby/lib/gitaly_server/wiki_service.rb +++ b/ruby/lib/gitaly_server/wiki_service.rb @@ -11,8 +11,6 @@ module GitalyServer wiki.delete_page(page_path, commit_details) Gitaly::WikiDeletePageResponse.new - rescue Gitlab::Git::Wiki::PageNotFound => e - raise GRPC::NotFound, e.message end def wiki_write_page(call) @@ -179,8 +177,6 @@ module GitalyServer wiki.update_page(page_path, title, format.to_sym, content, commit_details) Gitaly::WikiUpdatePageResponse.new - rescue Gitlab::Git::Wiki::PageNotFound => e - raise GRPC::NotFound, e.message end def wiki_get_formatted_data(request, call) diff --git a/ruby/lib/gitlab/git/wiki.rb b/ruby/lib/gitlab/git/wiki.rb index 202234ef6d..69dddb01c6 100644 --- a/ruby/lib/gitlab/git/wiki.rb +++ b/ruby/lib/gitlab/git/wiki.rb @@ -3,7 +3,7 @@ module Gitlab class Wiki DuplicatePageError = Class.new(StandardError) OperationError = Class.new(StandardError) - PageNotFound = Class.new(StandardError) + PageNotFound = Class.new(GRPC::NotFound) CommitDetails = Struct.new(:user_id, :username, :name, :email, :message) do def to_h -- GitLab