From f1c16d98572efbf2993368d71fc3b8ba62208267 Mon Sep 17 00:00:00 2001 From: Sergiy Kulanov Date: Sat, 28 Mar 2026 16:15:48 +0200 Subject: [PATCH] EPMDEDP-16620: fix: Add proper 404/401 error handling Extract classifyGitHubError helper to centralize GitHub error mapping. Extract isBitbucketNotFound helper for type-safe 404 detection. Add 404>ErrNotFound and 401>ErrUnauthorized mappings to all providers so errors are properly wrapped as domain sentinels instead of raw API errors. This ensures API handlers can correctly map to HTTP status codes. Add 403 (Forbidden) checks to GitLab methods for token scope errors. Fix Bitbucket GetRepository to use typed error helper instead of fragile string comparison. Remove misleading ErrNotFound mapping from ListUserOrganizations since GET /groups never returns 404 in normal operation. Add comprehensive test coverage for 404/401 scenarios across all providers. Signed-off-by: Sergiy Kulanov --- internal/api/pullrequest_handler_test.go | 11 +++ internal/services/bitbucket/bitbucket.go | 31 ++++++- .../bitbucket/bitbucket_pullrequests_test.go | 70 ++++++++++++++- internal/services/github/github.go | 44 ++++++--- internal/services/github/github_pipelines.go | 14 +-- .../github/github_pullrequests_test.go | 90 +++++++++++++++++++ internal/services/gitlab/gitlab.go | 16 +++- .../gitlab/gitlab_pullrequests_test.go | 57 ++++++++++++ 8 files changed, 307 insertions(+), 26 deletions(-) diff --git a/internal/api/pullrequest_handler_test.go b/internal/api/pullrequest_handler_test.go index 0d984b2..c937da9 100644 --- a/internal/api/pullrequest_handler_test.go +++ b/internal/api/pullrequest_handler_test.go @@ -222,6 +222,17 @@ func TestPullRequestHandlerErrResponse(t *testing.T) { assert.Contains(t, errResp.Message, "unauthorized") }) + t.Run("not found error returns 404", func(t *testing.T) { + err := fmt.Errorf("project deleted-group/deleted-repo: %w", gferrors.ErrNotFound) + + resp := handler.errResponse(err) + + errResp, ok := resp.(ListPullRequests404JSONResponse) + require.True(t, ok, "expected ListPullRequests404JSONResponse") + assert.Equal(t, fmt.Sprintf("%d", http.StatusNotFound), errResp.Code) + assert.Contains(t, errResp.Message, "not found") + }) + t.Run("generic error returns 500", func(t *testing.T) { resp := handler.errResponse(errors.New("something went wrong")) diff --git a/internal/services/bitbucket/bitbucket.go b/internal/services/bitbucket/bitbucket.go index e54012b..84c6ecb 100644 --- a/internal/services/bitbucket/bitbucket.go +++ b/internal/services/bitbucket/bitbucket.go @@ -3,6 +3,7 @@ package bitbucket import ( "context" "encoding/base64" + "errors" "fmt" "net/http" "net/url" @@ -27,6 +28,18 @@ import ( // would require changes across all Bitbucket methods and the underlying library. const defaultBitbucketAPIURL = "https://api.bitbucket.org/2.0" +// isBitbucketNotFound checks whether the error from the go-bitbucket library +// indicates a 404 response. The library returns *UnexpectedResponseStatusError +// with Status set to the HTTP status text (e.g. "404 Not Found"). +func isBitbucketNotFound(err error) bool { + var statusErr *bitbucket.UnexpectedResponseStatusError + if errors.As(err, &statusErr) { + return strings.HasPrefix(statusErr.Status, "404") + } + + return false +} + type BitbucketService struct { httpClient *resty.Client } @@ -56,7 +69,7 @@ func (b *BitbucketService) GetRepository( repository, err := client.Repositories.Repository.Get(repoOptions) if err != nil { - if err.Error() == "404 Not Found" { + if isBitbucketNotFound(err) { return nil, fmt.Errorf("repository %s/%s %w", owner, repo, gferrors.ErrNotFound) } @@ -85,6 +98,10 @@ func (b *BitbucketService) ListRepositories( repositories, err := client.Repositories.ListForAccount(repoOptions) if err != nil { + if isBitbucketNotFound(err) { + return nil, fmt.Errorf("account %s: %w", account, gferrors.ErrNotFound) + } + return nil, fmt.Errorf("failed to list repositories for account %s: %w", account, err) } @@ -157,6 +174,10 @@ func (b *BitbucketService) ListBranches( for b, err := range scanBranches { if err != nil { + if isBitbucketNotFound(err) { + return nil, fmt.Errorf("repository %s/%s: %w", owner, repo, gferrors.ErrNotFound) + } + return nil, fmt.Errorf("failed to list branches: %w", err) } @@ -266,6 +287,14 @@ func (b *BitbucketService) ListPullRequests( return nil, fmt.Errorf("failed to list pull requests for %s/%s: %w", owner, repo, err) } + if resp.StatusCode() == http.StatusNotFound { + return nil, fmt.Errorf("repository %s/%s: %w", owner, repo, gferrors.ErrNotFound) + } + + if resp.StatusCode() == http.StatusUnauthorized || resp.StatusCode() == http.StatusForbidden { + return nil, fmt.Errorf("invalid credentials: %w", gferrors.ErrUnauthorized) + } + if resp.IsError() { return nil, fmt.Errorf("failed to list pull requests for %s/%s: status %d, body: %s", owner, repo, resp.StatusCode(), resp.String()) diff --git a/internal/services/bitbucket/bitbucket_pullrequests_test.go b/internal/services/bitbucket/bitbucket_pullrequests_test.go index 2bc74f0..a414d1a 100644 --- a/internal/services/bitbucket/bitbucket_pullrequests_test.go +++ b/internal/services/bitbucket/bitbucket_pullrequests_test.go @@ -4,6 +4,7 @@ import ( "context" "encoding/base64" "encoding/json" + "errors" "net/http" "net/http/httptest" "net/url" @@ -13,6 +14,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + gferrors "github.com/KubeRocketCI/gitfusion/internal/errors" "github.com/KubeRocketCI/gitfusion/internal/models" "github.com/KubeRocketCI/gitfusion/internal/services/krci" ) @@ -276,6 +278,7 @@ func TestBitbucketServiceListPullRequests(t *testing.T) { statusCode int wantErr bool wantErrContain string + wantSentinel error wantCount int wantState models.PullRequestState validateQuery func(t *testing.T, r *http.Request) @@ -378,7 +381,8 @@ func TestBitbucketServiceListPullRequests(t *testing.T) { responseBody: bitbucketPRResponse{}, statusCode: http.StatusUnauthorized, wantErr: true, - wantErrContain: "status 401", + wantErrContain: "unauthorized", + wantSentinel: gferrors.ErrUnauthorized, }, } @@ -424,6 +428,10 @@ func TestBitbucketServiceListPullRequests(t *testing.T) { assert.Contains(t, err.Error(), tt.wantErrContain) } + if tt.wantSentinel != nil { + assert.True(t, errors.Is(err, tt.wantSentinel), "error should wrap %v", tt.wantSentinel) + } + return } @@ -768,3 +776,63 @@ func TestBitbucketServiceListPullRequestsSupersededState(t *testing.T) { assert.Equal(t, models.PullRequestStateClosed, result.Data[0].State) assert.Equal(t, models.PullRequestStateClosed, result.Data[1].State) } + +func TestBitbucketServiceListPullRequestsNotFound(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"type":"error","error":{"message":"Repository not found"}}`)) + })) + defer server.Close() + + svc := &BitbucketService{ + httpClient: resty.New().SetTransport(&redirectTransport{ + target: server.URL, + wrapped: http.DefaultTransport, + }), + } + token := testBitbucketToken() + + result, err := svc.ListPullRequests( + context.Background(), + "deleted-owner", + "deleted-repo", + krci.GitServerSettings{Token: token}, + models.PullRequestListOptions{State: "open", Page: 1, PerPage: 20}, + ) + + assert.Error(t, err) + assert.Nil(t, result) + assert.True(t, errors.Is(err, gferrors.ErrNotFound), "error should wrap gferrors.ErrNotFound") + assert.Contains(t, err.Error(), "not found") +} + +func TestBitbucketServiceListPullRequestsUnauthorized(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"type":"error","error":{"message":"Unauthorized"}}`)) + })) + defer server.Close() + + svc := &BitbucketService{ + httpClient: resty.New().SetTransport(&redirectTransport{ + target: server.URL, + wrapped: http.DefaultTransport, + }), + } + token := testBitbucketToken() + + result, err := svc.ListPullRequests( + context.Background(), + "owner", + "repo", + krci.GitServerSettings{Token: token}, + models.PullRequestListOptions{State: "open", Page: 1, PerPage: 20}, + ) + + assert.Error(t, err) + assert.Nil(t, result) + assert.True(t, errors.Is(err, gferrors.ErrUnauthorized), "error should wrap gferrors.ErrUnauthorized") + assert.Contains(t, err.Error(), "unauthorized") +} diff --git a/internal/services/github/github.go b/internal/services/github/github.go index 66b1956..ba975ee 100644 --- a/internal/services/github/github.go +++ b/internal/services/github/github.go @@ -19,6 +19,24 @@ import ( "github.com/KubeRocketCI/gitfusion/pkg/xiter" ) +// classifyGitHubError maps a GitHub API error to a domain sentinel error. +// Returns nil if the error is not a recognized GitHub error response. +func classifyGitHubError(err error) error { + ghErr := &github.ErrorResponse{} + if !errors.As(err, &ghErr) { + return nil + } + + switch ghErr.Response.StatusCode { + case http.StatusNotFound: + return gferrors.ErrNotFound + case http.StatusUnauthorized: + return gferrors.ErrUnauthorized + default: + return nil + } +} + const ( stateOpen = "open" stateClosed = "closed" @@ -42,11 +60,8 @@ func (g *GitHubProvider) GetRepository( repository, _, err := client.Repositories.Get(ctx, owner, repo) if err != nil { - ghErr := &github.ErrorResponse{} - if errors.As(err, &ghErr) { - if ghErr.Response.StatusCode == http.StatusNotFound { - return nil, fmt.Errorf("repository %s/%s: %w", owner, repo, gferrors.ErrNotFound) - } + if sentinel := classifyGitHubError(err); sentinel != nil { + return nil, fmt.Errorf("repository %s/%s: %w", owner, repo, sentinel) } return nil, fmt.Errorf("failed to get repository %s/%s: %w", owner, repo, err) @@ -72,11 +87,8 @@ func (g *GitHubProvider) ListRepositories( for repo, err := range it { if err != nil { - ghErr := &github.ErrorResponse{} - if errors.As(err, &ghErr) { - if ghErr.Response.StatusCode == http.StatusNotFound { - return nil, fmt.Errorf("organization or user %s: %w", owner, gferrors.ErrNotFound) - } + if sentinel := classifyGitHubError(err); sentinel != nil { + return nil, fmt.Errorf("organization or user %s: %w", owner, sentinel) } return nil, fmt.Errorf("failed to list repositories for org %s: %w", owner, err) @@ -291,6 +303,10 @@ func (g *GitHubProvider) ListBranches( for b, err := range it { if err != nil { + if sentinel := classifyGitHubError(err); sentinel != nil { + return nil, fmt.Errorf("repository %s/%s: %w", owner, repo, sentinel) + } + return nil, fmt.Errorf("failed to list branches: %w", err) } @@ -339,6 +355,10 @@ func (g *GitHubProvider) listPullRequestsDirect( }, }) if err != nil { + if sentinel := classifyGitHubError(err); sentinel != nil { + return nil, fmt.Errorf("repository %s/%s: %w", owner, repo, sentinel) + } + return nil, fmt.Errorf("failed to list pull requests for %s/%s: %w", owner, repo, err) } @@ -415,6 +435,10 @@ func (g *GitHubProvider) listPullRequestsWithPostFilter( pagesQueried++ if err != nil { + if sentinel := classifyGitHubError(err); sentinel != nil { + return nil, fmt.Errorf("repository %s/%s: %w", owner, repo, sentinel) + } + return nil, fmt.Errorf("failed to list pull requests for %s/%s: %w", owner, repo, err) } diff --git a/internal/services/github/github_pipelines.go b/internal/services/github/github_pipelines.go index 6742888..0a856be 100644 --- a/internal/services/github/github_pipelines.go +++ b/internal/services/github/github_pipelines.go @@ -2,15 +2,12 @@ package github import ( "context" - "errors" "fmt" - "net/http" "strconv" "time" "github.com/google/go-github/v72/github" - gferrors "github.com/KubeRocketCI/gitfusion/internal/errors" "github.com/KubeRocketCI/gitfusion/internal/models" "github.com/KubeRocketCI/gitfusion/internal/services/common" "github.com/KubeRocketCI/gitfusion/internal/services/krci" @@ -56,15 +53,8 @@ func (g *GitHubProvider) ListPipelines( workflowRuns, _, err := client.Actions.ListRepositoryWorkflowRuns(ctx, owner, repo, ghOpts) if err != nil { - ghErr := &github.ErrorResponse{} - if errors.As(err, &ghErr) { - if ghErr.Response.StatusCode == http.StatusNotFound { - return nil, fmt.Errorf("project %s: %w", project, gferrors.ErrNotFound) - } - - if ghErr.Response.StatusCode == http.StatusUnauthorized { - return nil, fmt.Errorf("invalid credentials: %w", gferrors.ErrUnauthorized) - } + if sentinel := classifyGitHubError(err); sentinel != nil { + return nil, fmt.Errorf("project %s: %w", project, sentinel) } return nil, fmt.Errorf("failed to list pipelines for %s: %w", project, err) diff --git a/internal/services/github/github_pullrequests_test.go b/internal/services/github/github_pullrequests_test.go index 55baddd..3e56620 100644 --- a/internal/services/github/github_pullrequests_test.go +++ b/internal/services/github/github_pullrequests_test.go @@ -3,6 +3,7 @@ package github import ( "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "net/url" @@ -14,6 +15,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + gferrors "github.com/KubeRocketCI/gitfusion/internal/errors" "github.com/KubeRocketCI/gitfusion/internal/models" "github.com/KubeRocketCI/gitfusion/internal/services/krci" ) @@ -1186,3 +1188,91 @@ func TestGitHubProviderListPullRequestsContextCancellation(t *testing.T) { assert.Error(t, err) assert.Nil(t, result) } + +func TestGitHubProviderListPullRequestsNotFound(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/repos/deleted-owner/deleted-repo/pulls", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _ = json.NewEncoder(w).Encode(map[string]string{ + "message": "Not Found", + }) + }) + + server := httptest.NewServer(mux) + defer server.Close() + + provider := newTestProvider(server.URL) + + result, err := provider.ListPullRequests( + context.Background(), + "deleted-owner", + "deleted-repo", + krci.GitServerSettings{Token: "test-token"}, + models.PullRequestListOptions{State: "open", Page: 1, PerPage: 20}, + ) + + assert.Error(t, err) + assert.Nil(t, result) + assert.True(t, errors.Is(err, gferrors.ErrNotFound), "error should wrap gferrors.ErrNotFound") + assert.Contains(t, err.Error(), "not found") +} + +func TestGitHubProviderListPullRequestsNotFoundPostFilter(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/repos/deleted-owner/deleted-repo/pulls", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _ = json.NewEncoder(w).Encode(map[string]string{ + "message": "Not Found", + }) + }) + + server := httptest.NewServer(mux) + defer server.Close() + + provider := newTestProvider(server.URL) + + // Use "merged" state which triggers the post-filter code path + result, err := provider.ListPullRequests( + context.Background(), + "deleted-owner", + "deleted-repo", + krci.GitServerSettings{Token: "test-token"}, + models.PullRequestListOptions{State: "merged", Page: 1, PerPage: 20}, + ) + + assert.Error(t, err) + assert.Nil(t, result) + assert.True(t, errors.Is(err, gferrors.ErrNotFound), "error should wrap gferrors.ErrNotFound for post-filter path") + assert.Contains(t, err.Error(), "not found") +} + +func TestGitHubProviderListPullRequestsUnauthorized(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/repos/owner/repo/pulls", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _ = json.NewEncoder(w).Encode(map[string]string{ + "message": "Bad credentials", + }) + }) + + server := httptest.NewServer(mux) + defer server.Close() + + provider := newTestProvider(server.URL) + + result, err := provider.ListPullRequests( + context.Background(), + "owner", + "repo", + krci.GitServerSettings{Token: "bad-token"}, + models.PullRequestListOptions{State: "open", Page: 1, PerPage: 20}, + ) + + assert.Error(t, err) + assert.Nil(t, result) + assert.True(t, errors.Is(err, gferrors.ErrUnauthorized), "error should wrap gferrors.ErrUnauthorized") + assert.Contains(t, err.Error(), "unauthorized") +} diff --git a/internal/services/gitlab/gitlab.go b/internal/services/gitlab/gitlab.go index ebab8be..b33287e 100644 --- a/internal/services/gitlab/gitlab.go +++ b/internal/services/gitlab/gitlab.go @@ -157,6 +157,10 @@ func (g *GitlabProvider) ListBranches( for b, err := range it { if err != nil { + if errors.Is(err, gitlab.ErrNotFound) { + return nil, fmt.Errorf("project %s/%s: %w", owner, repo, gferrors.ErrNotFound) + } + return nil, fmt.Errorf("failed to list branches for %s/%s: %w", owner, repo, err) } @@ -198,7 +202,7 @@ func (g *GitlabProvider) TriggerPipeline( return nil, fmt.Errorf("project %s or ref %s: %w", project, ref, gferrors.ErrNotFound) } - if resp != nil && resp.StatusCode == http.StatusUnauthorized { + if resp != nil && (resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden) { return nil, fmt.Errorf("invalid credentials: %w", gferrors.ErrUnauthorized) } @@ -258,7 +262,7 @@ func (g *GitlabProvider) ListPipelines( return nil, fmt.Errorf("project %s: %w", project, gferrors.ErrNotFound) } - if resp != nil && resp.StatusCode == http.StatusUnauthorized { + if resp != nil && (resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden) { return nil, fmt.Errorf("invalid credentials: %w", gferrors.ErrUnauthorized) } @@ -440,6 +444,14 @@ func (g *GitlabProvider) ListPullRequests( gitlab.WithContext(ctx), ) if err != nil { + if errors.Is(err, gitlab.ErrNotFound) || (resp != nil && resp.StatusCode == http.StatusNotFound) { + return nil, fmt.Errorf("project %s/%s: %w", owner, repo, gferrors.ErrNotFound) + } + + if resp != nil && (resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden) { + return nil, fmt.Errorf("invalid credentials: %w", gferrors.ErrUnauthorized) + } + return nil, fmt.Errorf("failed to list merge requests for %s/%s: %w", owner, repo, err) } diff --git a/internal/services/gitlab/gitlab_pullrequests_test.go b/internal/services/gitlab/gitlab_pullrequests_test.go index 54fdc4f..9b37116 100644 --- a/internal/services/gitlab/gitlab_pullrequests_test.go +++ b/internal/services/gitlab/gitlab_pullrequests_test.go @@ -3,6 +3,7 @@ package gitlab import ( "context" "encoding/json" + "errors" "net/http" "net/http/httptest" "testing" @@ -11,6 +12,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + gferrors "github.com/KubeRocketCI/gitfusion/internal/errors" "github.com/KubeRocketCI/gitfusion/internal/models" "github.com/KubeRocketCI/gitfusion/internal/services/krci" ) @@ -307,3 +309,58 @@ func TestGitLabProviderListPullRequestsNilAuthor(t *testing.T) { require.Len(t, result.Data, 1) assert.Nil(t, result.Data[0].Author, "author should be nil when JSON author is null") } + +func TestGitLabProviderListPullRequestsNotFound(t *testing.T) { + mux := http.NewServeMux() + mrPath := "/api/v4/projects/deleted-group%2Fdeleted-repo/merge_requests" + mux.HandleFunc(mrPath, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte(`{"message":"404 Project Not Found"}`)) + }) + + server := httptest.NewServer(mux) + defer server.Close() + + provider := NewGitlabProvider() + + result, err := provider.ListPullRequests( + context.Background(), + "deleted-group", + "deleted-repo", + krci.GitServerSettings{Token: "test-token", Url: server.URL}, + models.PullRequestListOptions{State: "open", Page: 1, PerPage: 20}, + ) + + assert.Error(t, err) + assert.Nil(t, result) + assert.True(t, errors.Is(err, gferrors.ErrNotFound), "error should wrap gferrors.ErrNotFound") + assert.Contains(t, err.Error(), "not found") +} + +func TestGitLabProviderListPullRequestsUnauthorized(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("/api/v4/projects/owner%2Frepo/merge_requests", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(`{"message":"401 Unauthorized"}`)) + }) + + server := httptest.NewServer(mux) + defer server.Close() + + provider := NewGitlabProvider() + + result, err := provider.ListPullRequests( + context.Background(), + "owner", + "repo", + krci.GitServerSettings{Token: "bad-token", Url: server.URL}, + models.PullRequestListOptions{State: "open", Page: 1, PerPage: 20}, + ) + + assert.Error(t, err) + assert.Nil(t, result) + assert.True(t, errors.Is(err, gferrors.ErrUnauthorized), "error should wrap gferrors.ErrUnauthorized") + assert.Contains(t, err.Error(), "unauthorized") +}