From b8e66a5552a10dd13ef91d6806111bf0d946a93a Mon Sep 17 00:00:00 2001 From: Earl Warren Date: Sat, 28 Jun 2025 18:48:37 +0200 Subject: [PATCH] feat: improve API /repos/{owner}/{repo}/compare/{basehead} error messages --- routers/api/v1/repo/pull.go | 11 ++-- tests/integration/api_repo_compare_test.go | 68 ++++++++++++++++++++++ 2 files changed, 73 insertions(+), 6 deletions(-) diff --git a/routers/api/v1/repo/pull.go b/routers/api/v1/repo/pull.go index cb66b4d09d..20f718d6c2 100644 --- a/routers/api/v1/repo/pull.go +++ b/routers/api/v1/repo/pull.go @@ -1084,7 +1084,6 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) err error ) - // If there is no head repository, it means pull request between same repository. headInfos := strings.Split(form.Head, ":") if len(headInfos) == 1 { isSameRepo = true @@ -1094,7 +1093,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) headUser, err = user_model.GetUserByName(ctx, headInfos[0]) if err != nil { if user_model.IsErrUserNotExist(err) { - ctx.NotFound("GetUserByName") + ctx.NotFound(fmt.Errorf("the owner %s does not exist", headInfos[0])) } else { ctx.Error(http.StatusInternalServerError, "GetUserByName", err) } @@ -1104,7 +1103,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) // The head repository can also point to the same repo isSameRepo = ctx.Repo.Owner.ID == headUser.ID } else { - ctx.NotFound() + ctx.NotFound(fmt.Errorf("the head part of {basehead} %s must contain zero or one colon (:) but contains %d", form.Head, len(headInfos)-1)) return nil, nil, nil, "", "" } @@ -1116,7 +1115,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) baseIsBranch := ctx.Repo.GitRepo.IsBranchExist(baseBranch) baseIsTag := ctx.Repo.GitRepo.IsTagExist(baseBranch) if !baseIsCommit && !baseIsBranch && !baseIsTag { - ctx.NotFound("BaseNotExist") + ctx.NotFound(fmt.Errorf("could not find '%s' to be a commit, branch or tag in the base repository %s/%s", baseBranch, baseRepo.Owner.Name, baseRepo.Name)) return nil, nil, nil, "", "" } @@ -1130,7 +1129,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) if baseRepo.BaseRepo == nil || baseRepo.BaseRepo.OwnerID != headUser.ID { log.Trace("parseCompareInfo[%d]: does not have fork or in same repository", baseRepo.ID) - ctx.NotFound("GetBaseRepo") + ctx.NotFound(fmt.Errorf("%[1]s does not have a fork of %[2]s/%[3]s and %[2]s/%[3]s is not a fork of a repository from %[1]s", headUser.Name, baseRepo.Owner.Name, baseRepo.Name)) return nil, nil, nil, "", "" } headRepo = baseRepo.BaseRepo @@ -1198,7 +1197,7 @@ func parseCompareInfo(ctx *context.APIContext, form api.CreatePullRequestOption) headIsBranch := headGitRepo.IsBranchExist(headBranch) headIsTag := headGitRepo.IsTagExist(headBranch) if !headIsCommit && !headIsBranch && !headIsTag { - ctx.NotFound("IsRefExist", nil) + ctx.NotFound(fmt.Errorf("could not find '%s' to be a commit, branch or tag in the head repository %s/%s", headBranch, headRepo.Owner.Name, headRepo.Name)) return nil, nil, nil, "", "" } diff --git a/tests/integration/api_repo_compare_test.go b/tests/integration/api_repo_compare_test.go index e4e85fc742..1724924fdc 100644 --- a/tests/integration/api_repo_compare_test.go +++ b/tests/integration/api_repo_compare_test.go @@ -7,7 +7,9 @@ import ( "encoding/base64" "fmt" "net/http" + "net/http/httptest" "net/url" + "strings" "testing" "time" @@ -50,6 +52,28 @@ func testAPICompareCommits(t *testing.T, objectFormat git.ObjectFormat) { } } + requireErrorContains := func(t *testing.T, resp *httptest.ResponseRecorder, expected string) { + t.Helper() + + type response struct { + Message string `json:"message"` + Errors []string `json:"errors"` + } + var bodyResp response + DecodeJSON(t, resp, &bodyResp) + + if strings.Contains(bodyResp.Message, expected) { + return + } + for _, error := range bodyResp.Errors { + if strings.Contains(error, expected) { + return + } + } + t.Log(fmt.Sprintf("expected %s in %+v", expected, bodyResp)) + t.Fail() + } + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) user2repo := "repoA" user2Ctx := NewAPITestContext(t, user2.Name, user2repo, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) @@ -123,6 +147,14 @@ func testAPICompareCommits(t *testing.T, objectFormat git.ObjectFormat) { user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) user4Ctx := NewAPITestContext(t, user4.Name, user2repo, auth_model.AccessTokenScopeWriteRepository, auth_model.AccessTokenScopeWriteUser) + + t.Run("ForkNotFound", func(t *testing.T) { + req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/compare/%s...%s:%s", user2.Name, user2repo, "master", user4.Name, user2branchName). + AddTokenAuth(user2Ctx.Token) + resp := MakeRequest(t, req, http.StatusNotFound) + requireErrorContains(t, resp, "user4 does not have a fork of user2/repoA and user2/repoA is not a fork of a repository from user4") + }) + t.Run("User4ForksUser2Repository", doAPIForkRepository(user4Ctx, user2.Name)) user4branchName := "user4branch" t.Run("CreateUser4RepositoryBranch", newBranchAndFile(user4Ctx, user4, user4branchName, "user4branchfilename.txt")) @@ -199,5 +231,41 @@ func testAPICompareCommits(t *testing.T, objectFormat git.ObjectFormat) { assert.Len(t, apiResp.Files, 1) }) } + + t.Run("ForkUserDoesNotExist", func(t *testing.T) { + notUser := "notauser" + req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/compare/master...%s:branchname", user2.Name, user2repo, notUser). + AddTokenAuth(user2Ctx.Token) + resp := MakeRequest(t, req, http.StatusNotFound) + requireErrorContains(t, resp, fmt.Sprintf("the owner %s does not exist", notUser)) + }) + + t.Run("HeadHasTooManyColon", func(t *testing.T) { + req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/compare/master...one:two:many", user2.Name, user2repo). + AddTokenAuth(user2Ctx.Token) + resp := MakeRequest(t, req, http.StatusNotFound) + requireErrorContains(t, resp, fmt.Sprintf("must contain zero or one colon (:) but contains 2")) + }) + + for _, testCase := range []struct { + what string + baseHead string + }{ + { + what: "base", + baseHead: "notexists...master", + }, + { + what: "head", + baseHead: "master...notexists", + }, + } { + t.Run("BaseHeadNotExists "+testCase.what, func(t *testing.T) { + req := NewRequestf(t, "GET", "/api/v1/repos/%s/%s/compare/%s", user2.Name, user2repo, testCase.baseHead). + AddTokenAuth(user2Ctx.Token) + resp := MakeRequest(t, req, http.StatusNotFound) + requireErrorContains(t, resp, fmt.Sprintf("could not find 'notexists' to be a commit, branch or tag in the %s", testCase.what)) + }) + } }) }