diff --git a/internal/app/errors.go b/internal/app/errors.go index bc86df6e..0ce3a69b 100644 --- a/internal/app/errors.go +++ b/internal/app/errors.go @@ -10,7 +10,6 @@ import ( "fmt" "net" "net/http" - "strconv" "strings" "syscall" "time" @@ -19,9 +18,6 @@ import ( "gitea.dev/tea/internal/transport" ) -// lookupTimeout bounds the follow-up reads that explain a failed request. -const lookupTimeout = 5 * time.Second - // explainedError replaces the message of err and keeps err reachable for exit codes. type explainedError struct { text string @@ -36,16 +32,15 @@ func explain(err error, format string, args ...any) error { return &explainedError{text: fmt.Sprintf(format, args...), err: err} } -// Explain words a command failure for people. It names the resource a failed request addressed -// ("issue #9 not found in alice/x"), tells a missing or inaccessible repository from a disabled -// feature, adds the next step to refusals Gitea explains, and words rejected credentials, -// unreachable servers and expired timeouts. Exit codes are unchanged. +// Explain words a command failure for people. It adds the next step to refusals Gitea explains, +// and words rejected credentials, unreachable servers and expired timeouts. Exit codes are +// unchanged. func Explain(ctx context.Context, deps Dependencies, err error) error { if err == nil || errors.As(err, new(*explainedError)) { return err } if apiErr, ok := errors.AsType[*transport.APIError](err); ok { - return explainAPIError(ctx, deps, err, apiErr) + return explainAPIError(deps, err, apiErr) } if failure, ok := errors.AsType[*transport.RequestFailure](err); ok { if reason := unreachable(failure.Err); reason != "" && failure.Server != "" { @@ -58,7 +53,7 @@ func Explain(ctx context.Context, deps Dependencies, err error) error { return err } -func explainAPIError(ctx context.Context, deps Dependencies, err error, apiErr *transport.APIError) error { +func explainAPIError(deps Dependencies, err error, apiErr *transport.APIError) error { if !explainable(err, apiErr) { return err } @@ -69,27 +64,11 @@ func explainAPIError(ctx context.Context, deps Dependencies, err error, apiErr * } return explain(err, "authentication rejected by %s; run tea auth login", instance) } - // Errors a service classified keep its wording; others are classified by their endpoint. - classified := err - if !errors.As(err, new(*services.NotFoundError)) && !errors.As(err, new(*services.ConflictError)) { - classified = services.ClassifyRequest(err) - } - ctx, cancel := context.WithTimeout(ctx, lookupTimeout) - defer cancel() - resource := services.RequestResource(apiErr.Endpoint) - if apiErr.Status == http.StatusNotFound && resource.Repository != "" { - if explained := explainRepositoryNotFound(ctx, apiErr, resource); explained != "" { - return explain(err, "%s", explained) - } - } - if conflict, ok := errors.AsType[*services.ConflictError](classified); ok && conflict.Hint == "" { - if hint := conflictHint(ctx, apiErr, conflict); hint != "" { + if conflict, ok := errors.AsType[*services.ConflictError](err); ok && conflict.Hint == "" { + if hint := conflictHint(apiErr, conflict); hint != "" { return explain(err, "%s; %s", conflict.Error(), hint) } } - if classified != err { - return explain(err, "%s", classified.Error()) - } return err } @@ -111,47 +90,8 @@ func explainable(err error, apiErr *transport.APIError) bool { return apiErr.Error() == text } -// explainRepositoryNotFound tells why a request on a repository found nothing: the repository is -// missing or hidden, the feature is disabled, the repository is empty, or a pull request number -// names an issue. It returns "" when the resource itself is missing. -func explainRepositoryNotFound(ctx context.Context, apiErr *transport.APIError, resource services.Resource) string { - repo := resource.Repository - var state services.RepositoryState - status := http.StatusNotFound - if resource.Kind != "repository" { - state, status = services.LookupRepository(ctx, apiErr, repo) - } - switch status { - case http.StatusNotFound: - return fmt.Sprintf("repository %s not found on %s (or you don't have access)", repo, cmp.Or(apiErr.Instance, "the instance")) - case http.StatusOK: - default: - return "" - } - features := map[string]struct { - name string - enabled bool - }{ - "issues": {"issues are", state.Issues}, "pulls": {"pull requests are", state.PullRequests}, - "wiki": {"the wiki is", state.Wiki}, "releases": {"releases are", state.Releases}, - "actions": {"Actions are", state.Actions}, "projects": {"projects are", state.Projects}, - } - if feature, ok := features[resource.Section]; ok && !feature.enabled { - return feature.name + " disabled for " + repo - } - if state.Empty && resource.Section == "pulls" { - return repo + " is empty; it has no pull requests" - } - if number, ok := strings.CutPrefix(resource.Key, "#"); ok && resource.Kind == "pull request" { - if index, err := strconv.ParseInt(number, 10, 64); err == nil && services.LookupIsIssue(ctx, apiErr, repo, index) { - return fmt.Sprintf("#%s in %s is an issue, not a pull request; view it with: tea issue view %s", number, repo, number) - } - } - return "" -} - // conflictHint returns the next step for a refused request, or "". -func conflictHint(ctx context.Context, apiErr *transport.APIError, conflict *services.ConflictError) string { +func conflictHint(apiErr *transport.APIError, conflict *services.ConflictError) string { number := strings.TrimPrefix(conflict.Key, "#") switch conflict.Reason { case services.ReasonArchived: @@ -159,7 +99,9 @@ func conflictHint(ctx context.Context, apiErr *transport.APIError, conflict *ser return "unarchive it with: tea repo unarchive " + conflict.Scope } case services.ReasonExists: - return existsHint(ctx, apiErr, conflict) + if conflict.Kind == "wiki page" { + return "edit it with: tea wiki edit " + conflict.Key + } case services.ReasonMerged: if apiErr.Method == http.MethodPatch { return "its state can no longer change" @@ -186,22 +128,6 @@ func conflictHint(ctx context.Context, apiErr *transport.APIError, conflict *ser return "" } -func existsHint(ctx context.Context, apiErr *transport.APIError, conflict *services.ConflictError) string { - switch conflict.Kind { - case "pull request": - head, base, ok := strings.Cut(strings.TrimPrefix(conflict.Key, "for "), " into ") - if !ok || conflict.Scope == "" { - return "" - } - if pullURL := services.LookupPullURL(ctx, apiErr, conflict.Scope, base, head); pullURL != "" { - return "see " + pullURL - } - case "wiki page": - return "edit it with: tea wiki edit " + conflict.Key - } - return "" -} - // unreachable words a failure to connect, or returns "" for other request failures. func unreachable(err error) string { var dnsErr *net.DNSError diff --git a/internal/app/errors_test.go b/internal/app/errors_test.go index e5f72b84..3dc1d8de 100644 --- a/internal/app/errors_test.go +++ b/internal/app/errors_test.go @@ -13,6 +13,7 @@ import ( "time" "gitea.dev/tea/internal/clierr" + "gitea.dev/tea/internal/services" "gitea.dev/tea/internal/transport" "github.com/stretchr/testify/require" ) @@ -36,19 +37,15 @@ func fakeGitea(t *testing.T, responses map[string][2]string) *httptest.Server { return server } -func TestExplainNamesResourcesAndTellsWhyRepositoriesFail(t *testing.T) { +func TestExplainKeepsClassifiedErrorsAndAddsHints(t *testing.T) { server := fakeGitea(t, map[string][2]string{ - "GET /api/v1/user": {"200", `{"login":"alice"}`}, - "GET /api/v1/repos/alice/project": {"200", `{"full_name":"alice/project","has_issues":true,"has_pull_requests":true}`}, - "GET /api/v1/repos/alice/project/issues/1": {"200", `{"number":1,"pull_request":null}`}, - "GET /api/v1/repos/bob/project": {"200", `{"full_name":"bob/project","has_issues":false,"has_pull_requests":true}`}, - "GET /api/v1/repos/alice/empty": {"200", `{"full_name":"alice/empty","empty":true,"has_issues":true,"has_pull_requests":true}`}, - "GET /api/v1/repos/alice/old": {"200", `{"full_name":"alice/old","archived":true,"has_issues":true}`}, - "POST /api/v1/repos/alice/old/issues": {"423", `{"message":"repo is archived"}`}, - "POST /api/v1/repos/alice/project/pulls": {"409", `{"message":"pull request already exists for these targets [id: 16, issue_id: 3, head_repo_id: 1, base_repo_id: 1, head_branch: feat, base_branch: main]"}`}, - "GET /api/v1/repos/alice/project/pulls/main/feat": {"200", `{"number":3,"html_url":"https://example.org/alice/project/pulls/3"}`}, - "GET /api/v1/repos/alice/project/pulls/3": {"200", `{"number":3,"state":"open","mergeable":false,"head":{"sha":"abc"}}`}, - "POST /api/v1/repos/alice/project/pulls/3/merge": {"405", `{"message":"Please try again later"}`}, + "GET /api/v1/user": {"200", `{"login":"alice"}`}, + "GET /api/v1/repos/alice/project": {"200", `{"full_name":"alice/project","has_issues":true,"has_pull_requests":true}`}, + "GET /api/v1/repos/alice/old": {"200", `{"full_name":"alice/old","archived":true,"has_issues":true}`}, + "POST /api/v1/repos/alice/old/issues": {"423", `{"message":"repo is archived"}`}, + "POST /api/v1/repos/alice/project/pulls": {"409", `{"message":"pull request already exists for these targets [id: 16, issue_id: 3, head_repo_id: 1, base_repo_id: 1, head_branch: feat, base_branch: main]"}`}, + "GET /api/v1/repos/alice/project/pulls/3": {"200", `{"number":3,"state":"open","mergeable":false,"head":{"sha":"abc"}}`}, + "POST /api/v1/repos/alice/project/pulls/3/merge": {"405", `{"message":"Please try again later"}`}, }) for _, tc := range []struct { name string @@ -56,13 +53,9 @@ func TestExplainNamesResourcesAndTellsWhyRepositoriesFail(t *testing.T) { want string }{ {"missing issue", []string{"issue", "view", "999", "-R", "alice/project"}, "issue #999 not found in alice/project"}, - {"missing repository", []string{"issue", "list", "-R", "alice/nope"}, "repository alice/nope not found on " + server.URL + " (or you don't have access)"}, - {"disabled issues", []string{"issue", "list", "-R", "bob/project"}, "issues are disabled for bob/project"}, - {"issue as pull request", []string{"pr", "view", "1", "-R", "alice/project"}, "#1 in alice/project is an issue, not a pull request; view it with: tea issue view 1"}, {"missing pull request", []string{"pr", "view", "999", "-R", "alice/project"}, "pull request #999 not found in alice/project"}, - {"empty repository", []string{"pr", "list", "-R", "alice/empty"}, "alice/empty is empty; it has no pull requests"}, {"archived repository", []string{"issue", "create", "-R", "alice/old", "--title", "x", "--body", "y"}, "alice/old is archived (read-only); unarchive it with: tea repo unarchive alice/old"}, - {"existing pull request", []string{"pr", "create", "-R", "alice/project", "--head", "feat", "--base", "main", "--title", "x", "--body", "y"}, "pull request for feat into main already exists in alice/project; see https://example.org/alice/project/pulls/3"}, + {"existing pull request", []string{"pr", "create", "-R", "alice/project", "--head", "feat", "--base", "main", "--title", "x", "--body", "y"}, "pull request for feat into main already exists in alice/project"}, {"context added by the command", []string{"pr", "merge", "3", "-R", "alice/project", "--yes"}, "pull request #3 has conflicting files or is otherwise not mergeable: Gitea API returned HTTP 405 for POST /api/v1/repos/alice/project/pulls/3/merge: Please try again later"}, {"raw api response", []string{"api", "users/nobody"}, "not found (HTTP 404 GET /api/v1/users/nobody)"}, } { @@ -79,23 +72,24 @@ func TestExplainNamesResourcesAndTellsWhyRepositoriesFail(t *testing.T) { func TestExplainAddsTheNextStepToRefusals(t *testing.T) { for _, tc := range []struct { - method, endpoint, message string - status int - want string + method, kind, key, message string + status int + want string }{ - {"PATCH", "/api/v1/repos/alice/x/pulls/3", "cannot change state of this pull request, it was already merged", 412, "pull request #3 is already merged; its state can no longer change"}, - {"POST", "/api/v1/repos/alice/x/pulls/3/merge", "The PR is already merged", 405, "pull request #3 is already merged"}, - {"POST", "/api/v1/repos/alice/x/pulls/5/merge", "Work in progress PRs cannot be merged", 405, "pull request #5 is a draft; mark it ready with: tea pr ready 5"}, - {"POST", "/api/v1/repos/alice/x/pulls/1/reviews", "approve your own pull is not allowed", 422, "you can't approve or request changes on your own pull request; leave a comment instead"}, - {"POST", "/api/v1/repos/alice/x/pulls", "Invalid PullRequest: There are no changes between the head and the base", 422, "no commits between the base and head branches; check out your feature branch or pass --head"}, - {"POST", "/api/v1/repos/alice/x/actions/workflows/nightly.yaml/dispatches", "workflow is disabled", 403, "workflow nightly.yaml is disabled; enable it with: tea workflow enable nightly.yaml"}, - {"DELETE", "/api/v1/repos/alice/x/actions/runs/37", "this workflow run is not done", 400, "run 37 has not finished; cancel it first: tea run cancel 37"}, - {"POST", "/api/v1/repos/alice/x/wiki/new", "wiki page already exists [title: Home.md]", 400, `wiki page "Home" already exists in alice/x; edit it with: tea wiki edit "Home"`}, - {"DELETE", "/api/v1/admin/users/uxadm1", "user still has ownership of repositories [uid: 7]", 422, `user "uxadm1" still owns repositories; transfer or delete them first`}, - {"POST", "/api/v1/user/keys", "Key content has been used as non-deploy key", 422, "this SSH key is already registered"}, + {"PATCH", "pull request", "#3", "cannot change state of this pull request, it was already merged", 412, "pull request #3 is already merged; its state can no longer change"}, + {"POST", "pull request", "#3", "The PR is already merged", 405, "pull request #3 is already merged"}, + {"POST", "pull request", "#5", "Work in progress PRs cannot be merged", 405, "pull request #5 is a draft; mark it ready with: tea pr ready 5"}, + {"POST", "pull request", "#1", "approve your own pull is not allowed", 422, "you can't approve or request changes on your own pull request; leave a comment instead"}, + {"POST", "pull request", "", "Invalid PullRequest: There are no changes between the head and the base", 422, "no commits between the base and head branches; check out your feature branch or pass --head"}, + {"POST", "workflow", "nightly.yaml", "workflow is disabled", 403, "workflow nightly.yaml is disabled; enable it with: tea workflow enable nightly.yaml"}, + {"DELETE", "run", "37", "this workflow run is not done", 400, "run 37 has not finished; cancel it first: tea run cancel 37"}, + {"POST", "wiki page", `"Home"`, "wiki page already exists [title: Home.md]", 400, `wiki page "Home" already exists in alice/x; edit it with: tea wiki edit "Home"`}, + {"POST", "issue", "", "repo is archived", 423, "alice/x is archived (read-only); unarchive it with: tea repo unarchive alice/x"}, + {"DELETE", "user", `"uxadm1"`, "user still has ownership of repositories [uid: 7]", 422, `user "uxadm1" still owns repositories; transfer or delete them first`}, + {"POST", "SSH key", "", "Key content has been used as non-deploy key", 422, "this SSH key is already registered"}, } { - apiErr := &transport.APIError{Status: tc.status, Method: tc.method, Endpoint: tc.endpoint, Message: tc.message, Instance: "http://localhost:3001"} - require.EqualError(t, Explain(context.Background(), Dependencies{}, apiErr), tc.want, tc.endpoint) + apiErr := &transport.APIError{Status: tc.status, Method: tc.method, Endpoint: "/api/v1/repos/alice/x/endpoint", Message: tc.message, Instance: "http://localhost:3001"} + require.EqualError(t, Explain(context.Background(), Dependencies{}, services.Classify(apiErr, tc.kind, tc.key, "alice/x")), tc.want, tc.message) } } diff --git a/internal/app/issues.go b/internal/app/issues.go index 7f6722bf..e22fdc20 100644 --- a/internal/app/issues.go +++ b/internal/app/issues.go @@ -512,7 +512,7 @@ func itemError(err error, scope string) error { if apiErr, ok := errors.AsType[*transport.APIError](err); ok && apiErr.Status == http.StatusNotFound { return explain(err, "not found in %s", scope) } - return services.ClassifyRequest(err) + return err } // nameFilter splits a comma-separated filter of kind, trimming entries and rejecting blanks. diff --git a/internal/app/ssh_keys.go b/internal/app/ssh_keys.go index 04299cb0..e87d01df 100644 --- a/internal/app/ssh_keys.go +++ b/internal/app/ssh_keys.go @@ -174,7 +174,7 @@ $ cat key.pub | tea ssh-key add`, service := services.SSHKeys{Client: target.client} created, err := service.Create(cmd.Context(), title, strings.TrimSpace(string(data))) if err != nil { - return a.registeredKey(cmd.Context(), service, key, services.ClassifyRequest(err)) + return a.registeredKey(cmd.Context(), service, key, err) } a.done(cmd, "Added %s", sshKeyLabel(created)) return a.jsonResult(cmd, created) diff --git a/internal/services/branches.go b/internal/services/branches.go index 6961c31b..4b53a5f6 100644 --- a/internal/services/branches.go +++ b/internal/services/branches.go @@ -64,9 +64,10 @@ func (s Branches) ListPage(ctx context.Context, repo string, limit, page int) (P // View retrieves a remote branch. func (s Branches) View(ctx context.Context, repo, branch string) (Branch, error) { - return repoConvert(ctx, s.Client, repo, branchFrom, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Branch, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, repo, branchFrom, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Branch, *gitea.Response, error) { return c.Repositories.GetRepoBranch(ctx, owner, name, branch) }) + return item, Classify(err, "branch", branch, repo) } // Create creates a branch from source, a branch, tag or commit, or from the default branch. @@ -98,10 +99,11 @@ func (s Branches) Rename(ctx context.Context, repo, oldName, newName string) err // Delete removes a remote branch. func (s Branches) Delete(ctx context.Context, repo, branch string) error { - return repoDo(ctx, s.Client, repo, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Response, error) { + err := repoDo(ctx, s.Client, repo, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Response, error) { _, resp, err := c.Repositories.DeleteRepoBranch(ctx, owner, name, branch) return resp, err }) + return Classify(err, "branch", branch, repo) } // Protection is a branch protection rule. diff --git a/internal/services/comments.go b/internal/services/comments.go index 6351c23e..b165bdf8 100644 --- a/internal/services/comments.go +++ b/internal/services/comments.go @@ -71,28 +71,32 @@ func (s Comments) Page(ctx context.Context, n int64, limit, page int) ([]Comment // View retrieves a comment by its repository-scoped ID. func (s Comments) View(ctx context.Context, id int64) (Comment, error) { - return repoConvert(ctx, s.Client, s.Repository, commentFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Comment, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, s.Repository, commentFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Comment, *gitea.Response, error) { return c.Issues.GetIssueComment(ctx, owner, repo, id) }) + return item, Classify(err, "comment", strconv.FormatInt(id, 10), s.Repository) } // Create adds a comment to an issue or pull request. func (s Comments) Create(ctx context.Context, n int64, body string) (Comment, error) { - return repoConvert(ctx, s.Client, s.Repository, commentFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Comment, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, s.Repository, commentFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Comment, *gitea.Response, error) { return c.Issues.CreateIssueComment(ctx, owner, repo, n, gitea.CreateIssueCommentOption{Body: body}) }) + return item, Classify(err, "issue", "#"+strconv.FormatInt(n, 10), s.Repository) } // Edit replaces the body of a repository comment. func (s Comments) Edit(ctx context.Context, id int64, body string) (Comment, error) { - return repoConvert(ctx, s.Client, s.Repository, commentFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Comment, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, s.Repository, commentFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Comment, *gitea.Response, error) { return c.Issues.EditIssueComment(ctx, owner, repo, id, gitea.EditIssueCommentOption{Body: body}) }) + return item, Classify(err, "comment", strconv.FormatInt(id, 10), s.Repository) } // Delete removes a repository comment. func (s Comments) Delete(ctx context.Context, id int64) error { - return repoDo(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Response, error) { + err := repoDo(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Response, error) { return c.Issues.DeleteIssueComment(ctx, owner, repo, id) }) + return Classify(err, "comment", strconv.FormatInt(id, 10), s.Repository) } diff --git a/internal/services/errors.go b/internal/services/errors.go index b3778bd4..413775bd 100644 --- a/internal/services/errors.go +++ b/internal/services/errors.go @@ -212,170 +212,3 @@ func refusal(apiErr *transport.APIError) string { } return "" } - -// ClassifyRequest is Classify for a failure no service classified: it infers the resource from -// the failed request's endpoint, so a 404 for GET /api/v1/repos/alice/x/issues/9 reports -// "issue #9 not found in alice/x". -func ClassifyRequest(err error) error { - apiErr, ok := errors.AsType[*transport.APIError](err) - if !ok { - return err - } - resource := RequestResource(apiErr.Endpoint) - kind, key := resource.Kind, resource.Key - // A refused POST to a collection concerns the resource it would have created. - if apiErr.Status != http.StatusNotFound && resource.Collection != "" { - kind, key = resource.Collection, "" - } - return Classify(err, kind, key, resource.Scope) -} - -// Resource is what a Gitea API endpoint addresses. -type Resource struct { - // Kind and Key name one resource, such as "issue" and "#9"; both are empty for collections. - Kind, Key string - // Collection names the kind of resource a collection endpoint holds, such as "branch" for - // POST /repos/{owner}/{repo}/branches. - Collection string - // Scope is the repository or organization the resource belongs to. - Scope string - // Repository is the owner/name of a repository endpoint, else empty. - Repository string - // Section is the first path segment below the repository, such as "issues" or "pulls". - Section string -} - -// RequestResource reports what endpoint, an API path such as /api/v1/repos/alice/x/issues/9, -// addresses. -func RequestResource(endpoint string) Resource { - _, rest, ok := strings.Cut(endpoint, "/api/v1/") - if !ok { - return Resource{} - } - parts := strings.Split(strings.Trim(rest, "/"), "/") - if len(parts) < 3 || parts[0] != "repos" { - return instanceResource(parts) - } - repo := parts[1] + "/" + parts[2] - if len(parts) == 3 { - return Resource{Kind: "repository", Key: repo, Repository: repo} - } - resource := repositoryResource(parts[3:]) - resource.Repository, resource.Scope, resource.Section = repo, repo, parts[3] - return resource -} - -// collections names the resources that POST to a repository collection creates. -var collections = map[string]string{ - "branches": "branch", "tags": "tag", "releases": "release", "labels": "label", "milestones": "milestone", - "pulls": "pull request", "issues": "issue", "hooks": "webhook", "keys": "deploy key", "wiki": "wiki page", -} - -// repositoryResource matches the path below /repos/{owner}/{repo}. -func repositoryResource(parts []string) Resource { - at := func(i int) string { - if i < len(parts) { - return parts[i] - } - return "" - } - rest := func(i int) string { return strings.Join(parts[min(i, len(parts)):], "/") } - switch { - case at(0) == "issues" && at(1) == "comments" && at(2) != "": - return Resource{Kind: "comment", Key: at(2)} - case (at(0) == "issues" || at(0) == "pulls") && isNumber(at(1)): - kind := "issue" - if at(0) == "pulls" { - kind = "pull request" - } - return Resource{Kind: kind, Key: "#" + at(1)} - case at(0) == "releases" && at(1) == "tags" && at(2) != "": - return Resource{Kind: "release", Key: rest(2)} - case at(0) == "releases" && at(2) == "assets" && at(3) != "": - return Resource{Kind: "release asset", Key: at(3)} - case at(0) == "releases" && isNumber(at(1)): - return Resource{Kind: "release", Key: at(1)} - case at(0) == "labels" && at(1) != "": - return Resource{Kind: "label", Key: at(1)} - case at(0) == "milestones" && at(1) != "": - return Resource{Kind: "milestone", Key: nameKey(at(1))} - case at(0) == "branches" && at(1) != "": - return Resource{Kind: "branch", Key: rest(1)} - case at(0) == "branch_protections" && at(1) != "": - return Resource{Kind: "branch protection", Key: rest(1)} - case at(0) == "tags" && at(1) != "": - return Resource{Kind: "tag", Key: rest(1)} - case at(0) == "wiki" && (at(1) == "page" || at(1) == "revisions") && at(2) != "": - return Resource{Kind: "wiki page", Key: strconv.Quote(at(2))} - case at(0) == "hooks" && isNumber(at(1)): - return Resource{Kind: "webhook", Key: at(1)} - case at(0) == "collaborators" && at(1) != "": - return Resource{Kind: "collaborator", Key: strconv.Quote(at(1))} - case at(0) == "contents" && at(1) != "": - return Resource{Kind: "file", Key: rest(1)} - case at(0) == "actions": - return actionsResource(at(1), at(2)) - } - return Resource{Collection: collections[at(0)]} -} - -func actionsResource(collection, key string) Resource { - kinds := map[string]string{"runs": "run", "jobs": "job", "artifacts": "artifact", "workflows": "workflow", "secrets": "secret", "variables": "variable"} - kind := kinds[collection] - if key == "" { - return Resource{Collection: kind} - } - return Resource{Kind: kind, Key: key} -} - -// instanceResource matches endpoints outside a repository, such as /users/{name}. -func instanceResource(parts []string) Resource { - at := func(i int) string { - if i < len(parts) { - return parts[i] - } - return "" - } - switch { - case at(0) == "orgs" && at(1) != "" && (at(2) == "members" || at(2) == "public_members") && at(3) != "": - return Resource{Kind: "member", Key: strconv.Quote(at(3)), Scope: at(1)} - case at(0) == "orgs" && at(1) != "" && at(2) == "repos": - return Resource{Kind: "organization", Key: strconv.Quote(at(1)), Collection: "repository"} - case at(0) == "orgs" && at(1) != "" && at(2) == "teams": - return Resource{Kind: "organization", Key: strconv.Quote(at(1)), Collection: "team"} - case at(0) == "orgs" && at(1) != "": - return Resource{Kind: "organization", Key: strconv.Quote(at(1))} - case at(0) == "orgs": - return Resource{Collection: "organization"} - case at(0) == "users" && at(1) != "": - return Resource{Kind: "user", Key: strconv.Quote(at(1))} - case at(0) == "admin" && at(1) == "users" && at(2) != "": - return Resource{Kind: "user", Key: strconv.Quote(at(2))} - case at(0) == "admin" && at(1) == "users": - return Resource{Collection: "user"} - case at(0) == "teams" && isNumber(at(1)): - return Resource{Kind: "team", Key: at(1)} - case at(0) == "user" && at(1) == "keys" && at(2) != "": - return Resource{Kind: "SSH key", Key: at(2)} - case at(0) == "user" && at(1) == "keys": - return Resource{Collection: "SSH key"} - case at(0) == "user" && at(1) == "repos": - return Resource{Collection: "repository"} - case at(0) == "notifications" && at(1) == "threads" && at(2) != "": - return Resource{Kind: "notification", Key: at(2)} - } - return Resource{} -} - -// nameKey quotes a name, but not a numeric ID. -func nameKey(value string) string { - if isNumber(value) { - return value - } - return strconv.Quote(value) -} - -func isNumber(value string) bool { - n, err := strconv.ParseInt(value, 10, 64) - return err == nil && n > 0 -} diff --git a/internal/services/errors_test.go b/internal/services/errors_test.go index 97138934..e52293e1 100644 --- a/internal/services/errors_test.go +++ b/internal/services/errors_test.go @@ -13,60 +13,35 @@ import ( "github.com/stretchr/testify/require" ) -func TestClassifyRequestNamesTheResource(t *testing.T) { +func TestClassifyWordsRefusals(t *testing.T) { for _, tc := range []struct { - name, method, endpoint, message string - status int - want string + name, message, kind, key string + status int + want string }{ - {"issue", "GET", "/api/v1/repos/alice/project/issues/999", "not found", 404, "issue #999 not found in alice/project"}, - {"issue comments", "GET", "/api/v1/repos/alice/project/issues/9/comments", "not found", 404, "issue #9 not found in alice/project"}, - {"comment", "DELETE", "/api/v1/repos/alice/project/issues/comments/81", "comment does not exist [id: 81, issue_id: 0]", 404, "comment 81 not found in alice/project"}, - {"pull request", "GET", "/api/v1/repos/alice/project/pulls/7/reviews", "not found", 404, "pull request #7 not found in alice/project"}, - {"release tag", "GET", "/api/v1/repos/alice/project/releases/tags/v9", "not found", 404, "release v9 not found in alice/project"}, - {"branch with slashes", "GET", "/api/v1/repos/alice/project/branches/docs/guide", "not found", 404, "branch docs/guide not found in alice/project"}, - {"milestone title", "GET", "/api/v1/repos/alice/project/milestones/v2.0", "not found", 404, `milestone "v2.0" not found in alice/project`}, - {"milestone id", "GET", "/api/v1/repos/alice/project/milestones/3", "not found", 404, "milestone 3 not found in alice/project"}, - {"wiki page", "GET", "/api/v1/repos/alice/project/wiki/page/Nope", "not found", 404, `wiki page "Nope" not found in alice/project`}, - {"run", "GET", "/api/v1/repos/alice/project/actions/runs/9999/jobs", "run with id 9999: resource does not exist", 404, "run 9999 not found in alice/project"}, - {"secret", "DELETE", "/api/v1/repos/alice/project/actions/secrets/NOPE", "secret was not found [name: ]", 404, "secret NOPE not found in alice/project"}, - {"repository", "GET", "/api/v1/repos/nobody/none", "not found", 404, "repository nobody/none not found"}, - {"user", "GET", "/api/v1/users/nobody/repos", "user redirect does not exist [name: nobody]", 404, `user "nobody" not found`}, - {"organization", "GET", "/api/v1/orgs/nope-org", "not found", 404, `organization "nope-org" not found`}, - {"org member", "DELETE", "/api/v1/orgs/acme/members/carol", "not found", 404, `member "carol" not found in acme`}, - {"team", "GET", "/api/v1/teams/99", "not found", 404, "team 99 not found"}, - {"ssh key", "DELETE", "/api/v1/user/keys/5", "not found", 404, "SSH key 5 not found"}, - {"archived 423", "POST", "/api/v1/repos/alice/old-stuff/issues", "repo is archived", 423, "alice/old-stuff is archived (read-only)"}, - {"archived 500", "POST", "/api/v1/repos/alice/old-stuff/releases", " is archived", 500, "alice/old-stuff is archived (read-only)"}, - {"archived repository edit", "PATCH", "/api/v1/repos/alice/old-stuff", "repo is archived", 423, "alice/old-stuff is archived (read-only)"}, - {"pull request exists", "POST", "/api/v1/repos/alice/x/pulls", "pull request already exists for these targets [id: 16, issue_id: 1, head_repo_id: 44, base_repo_id: 44, head_branch: feat, base_branch: main]", 409, "pull request for feat into main already exists in alice/x"}, - {"no changes", "POST", "/api/v1/repos/alice/x/pulls", "Invalid PullRequest: There are no changes between the head and the base", 422, "no commits between the base and head branches"}, - {"merged", "PATCH", "/api/v1/repos/alice/x/pulls/3", "cannot change state of this pull request, it was already merged", 412, "pull request #3 is already merged"}, - {"merge merged", "POST", "/api/v1/repos/alice/x/pulls/3/merge", "The PR is already merged", 405, "pull request #3 is already merged"}, - {"draft merge", "POST", "/api/v1/repos/alice/x/pulls/5/merge", "Work in progress PRs cannot be merged", 405, "pull request #5 is a draft"}, - {"own approval", "POST", "/api/v1/repos/alice/x/pulls/1/reviews", "approve your own pull is not allowed", 422, "you can't approve or request changes on your own pull request"}, - {"own rejection", "POST", "/api/v1/repos/alice/x/pulls/1/reviews", "reject your own pull is not allowed", 422, "you can't approve or request changes on your own pull request"}, - {"branch exists", "POST", "/api/v1/repos/alice/x/branches", "The branch already exists.", 409, "branch already exists in alice/x"}, - {"release exists", "POST", "/api/v1/repos/alice/x/releases", "Release is has no Tag", 409, "release already exists in alice/x"}, - {"wiki page exists", "POST", "/api/v1/repos/alice/x/wiki/new", "wiki page already exists [title: Home.md]", 400, `wiki page "Home" already exists in alice/x`}, - {"repository exists", "POST", "/api/v1/user/repos", "The repository with the same name already exists.", 409, "repository already exists"}, - {"org repository exists", "POST", "/api/v1/orgs/acme/repos", "The repository with the same name already exists.", 409, "repository already exists"}, - {"team exists", "POST", "/api/v1/orgs/acme/teams", "team already exists [org_id: 3, name: devs]", 422, "team already exists"}, - {"user exists", "POST", "/api/v1/admin/users", "user already exists [name: bob]", 422, "user already exists"}, - {"organization exists", "POST", "/api/v1/orgs", "user already exists [name: acme]", 422, "organization already exists"}, - {"archived label is no repository", "POST", "/api/v1/repos/alice/x/issues/1/labels", "label is archived", 422, "Gitea API returned HTTP 422 for POST /api/v1/repos/alice/x/issues/1/labels: label is archived"}, - {"run completed", "POST", "/api/v1/repos/alice/x/actions/runs/35/cancel", "run is already completed", 409, "run 35 is already completed"}, - {"run not done", "DELETE", "/api/v1/repos/alice/x/actions/runs/37", "this workflow run is not done", 400, "run 37 has not finished"}, - {"workflow disabled", "POST", "/api/v1/repos/alice/x/actions/workflows/nightly.yaml/dispatches", "workflow is disabled", 403, "workflow nightly.yaml is disabled"}, - {"key in use", "POST", "/api/v1/user/keys", "Key content has been used as non-deploy key", 422, "this SSH key is already registered"}, - {"unknown assignee", "POST", "/api/v1/repos/alice/x/issues", "Assignee does not exist: [name: user does not exist [uid: 0, name: nobody123]]", 422, `user "nobody123" not found`}, - {"unknown poster filter", "GET", "/api/v1/repos/alice/x/pulls", "user does not exist [uid: 0, name: nobody]", 400, `user "nobody" not found`}, - {"unrecognized endpoint", "GET", "/api/v1/repos/alice/x/issues", "not found", 404, "not found (HTTP 404 GET /api/v1/repos/alice/x/issues)"}, - {"server error", "GET", "/api/v1/repos/alice/x/issues/1", "boom", 500, "Gitea API returned HTTP 500 for GET /api/v1/repos/alice/x/issues/1: boom"}, + {"missing issue", "not found", "issue", "#999", 404, "issue #999 not found in alice/x"}, + {"archived 423", "repo is archived", "issue", "", 423, "alice/x is archived (read-only)"}, + {"archived 500", " is archived", "release", "", 500, "alice/x is archived (read-only)"}, + {"pull request exists", "pull request already exists for these targets [id: 16, issue_id: 1, head_repo_id: 44, base_repo_id: 44, head_branch: feat, base_branch: main]", "pull request", "", 409, "pull request for feat into main already exists in alice/x"}, + {"no changes", "Invalid PullRequest: There are no changes between the head and the base", "pull request", "", 422, "no commits between the base and head branches"}, + {"merged", "cannot change state of this pull request, it was already merged", "pull request", "#3", 412, "pull request #3 is already merged"}, + {"draft merge", "Work in progress PRs cannot be merged", "pull request", "#5", 405, "pull request #5 is a draft"}, + {"own approval", "approve your own pull is not allowed", "pull request", "#1", 422, "you can't approve or request changes on your own pull request"}, + {"own rejection", "reject your own pull is not allowed", "pull request", "#1", 422, "you can't approve or request changes on your own pull request"}, + {"branch exists", "The branch already exists.", "branch", "", 409, "branch already exists in alice/x"}, + {"release exists", "Release is has no Tag", "release", "", 409, "release already exists in alice/x"}, + {"wiki page exists", "wiki page already exists [title: Home.md]", "wiki page", `"Home"`, 400, `wiki page "Home" already exists in alice/x`}, + {"archived label is no repository", "label is archived", "issue", "#1", 422, "Gitea API returned HTTP 422 for POST /api/v1/repos/alice/x/endpoint: label is archived"}, + {"run completed", "run is already completed", "run", "35", 409, "run 35 is already completed"}, + {"run not done", "this workflow run is not done", "run", "37", 400, "run 37 has not finished"}, + {"workflow disabled", "workflow is disabled", "workflow", "nightly.yaml", 403, "workflow nightly.yaml is disabled"}, + {"key in use", "Key content has been used as non-deploy key", "SSH key", "", 422, "this SSH key is already registered"}, + {"unknown assignee", "Assignee does not exist: [name: user does not exist [uid: 0, name: nobody123]]", "issue", "", 422, `user "nobody123" not found`}, + {"server error", "boom", "issue", "#1", 500, "Gitea API returned HTTP 500 for POST /api/v1/repos/alice/x/endpoint: boom"}, } { t.Run(tc.name, func(t *testing.T) { - apiErr := &transport.APIError{Status: tc.status, Method: tc.method, Endpoint: tc.endpoint, Message: tc.message} - err := ClassifyRequest(apiErr) + apiErr := &transport.APIError{Status: tc.status, Method: "POST", Endpoint: "/api/v1/repos/alice/x/endpoint", Message: tc.message} + err := Classify(apiErr, tc.kind, tc.key, "alice/x") require.EqualError(t, err, tc.want) require.ErrorIs(t, err, apiErr, "the API error stays reachable for exit codes and --debug") }) diff --git a/internal/services/issues.go b/internal/services/issues.go index b4e5e39e..5f1e8d23 100644 --- a/internal/services/issues.go +++ b/internal/services/issues.go @@ -6,6 +6,7 @@ package services import ( "context" "net/http" + "strconv" "strings" "time" @@ -144,19 +145,21 @@ func filterValues(value string) []string { // View retrieves one issue. func (s Issues) View(ctx context.Context, number int64) (Issue, error) { - return repoConvert(ctx, s.Client, s.Repository, discussionFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, s.Repository, discussionFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { return c.Issues.GetIssue(ctx, owner, repo, number) }) + return item, Classify(err, "issue", "#"+strconv.FormatInt(number, 10), s.Repository) } // Create opens an issue. func (s Issues) Create(ctx context.Context, options IssueOptions) (Issue, error) { - return repoConvert(ctx, s.Client, s.Repository, discussionFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, s.Repository, discussionFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { return c.Issues.CreateIssue(ctx, owner, repo, gitea.CreateIssueOption{ Title: options.Title, Body: options.Body, Ref: options.Ref, Assignees: options.Assignees, Deadline: options.Deadline, Milestone: options.Milestone, Labels: options.Labels, }) }) + return item, Classify(err, "issue", "", s.Repository) } // SetState updates only the selected issue state. @@ -166,7 +169,8 @@ func (s Issues) SetState(ctx context.Context, number int64, state string) (Issue } func (s Issues) edit(ctx context.Context, number int64, opts gitea.EditIssueOption) (Issue, error) { - return repoConvert(ctx, s.Client, s.Repository, discussionFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, s.Repository, discussionFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Issue, *gitea.Response, error) { return c.Issues.EditIssue(ctx, owner, repo, number, opts) }) + return item, Classify(err, "issue", "#"+strconv.FormatInt(number, 10), s.Repository) } diff --git a/internal/services/lookups.go b/internal/services/lookups.go deleted file mode 100644 index 81145ce4..00000000 --- a/internal/services/lookups.go +++ /dev/null @@ -1,81 +0,0 @@ -// Copyright 2026 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package services - -import ( - "context" - "net/http" - - gitea "gitea.dev/sdk" - "gitea.dev/tea/internal/transport" -) - -// lookup repeats a read on the instance of the failed request apiErr, with its credential, to -// explain the failure. It returns the response status, or 0 when apiErr was not recorded from a -// request, the read got no response, or a successful response did not decode. -func lookup[T any](ctx context.Context, apiErr *transport.APIError, repository string, op repoOperation[T]) (T, int) { - var zero T - httpClient := apiErr.LookupClient() - if httpClient == nil { - return zero, 0 - } - owner, repo, err := splitRepository(repository) - if err != nil { - return zero, 0 - } - sdk, err := gitea.NewClient(apiErr.Instance, gitea.SetHTTPClient(httpClient), gitea.SetGiteaVersion("")) - if err != nil { - return zero, 0 - } - result, resp, err := op(ctx, sdk, owner, repo) - switch { - case resp == nil || resp.Response == nil: - return zero, 0 - case err == nil: - return result, resp.StatusCode - case resp.StatusCode >= 200 && resp.StatusCode < 300: - return zero, 0 - } - return zero, resp.StatusCode -} - -// RepositoryState holds the repository facts that explain a failed request on it. -type RepositoryState struct { - Empty, Issues, PullRequests, Wiki, Releases, Actions, Projects bool -} - -// LookupRepository reads the state of repo for the failed request apiErr; see lookup for the status. -func LookupRepository(ctx context.Context, apiErr *transport.APIError, repo string) (RepositoryState, int) { - item, status := lookup(ctx, apiErr, repo, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Repository, *gitea.Response, error) { - return c.Repositories.GetRepo(ctx, owner, name) - }) - if item == nil { - return RepositoryState{}, status - } - return RepositoryState{ - Empty: item.Empty, Issues: item.HasIssues, PullRequests: item.HasPullRequests, Wiki: item.HasWiki, - Releases: item.HasReleases, Actions: item.HasActions, Projects: item.HasProjects, - }, status -} - -// LookupIsIssue reports whether number names an issue rather than a pull request in repo, for the -// failed request apiErr. -func LookupIsIssue(ctx context.Context, apiErr *transport.APIError, repo string, number int64) bool { - item, status := lookup(ctx, apiErr, repo, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Issue, *gitea.Response, error) { - return c.Issues.GetIssue(ctx, owner, name, number) - }) - return status == http.StatusOK && item != nil && item.PullRequest == nil -} - -// LookupPullURL returns the web URL of the pull request from head into base in repo, for the failed -// request apiErr, or "". -func LookupPullURL(ctx context.Context, apiErr *transport.APIError, repo, base, head string) string { - item, status := lookup(ctx, apiErr, repo, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.PullRequest, *gitea.Response, error) { - return c.PullRequests.GetPullRequestByBaseHead(ctx, owner, name, base, head) - }) - if status != http.StatusOK || item == nil { - return "" - } - return item.HTMLURL -} diff --git a/internal/services/organizations.go b/internal/services/organizations.go index a01bc65e..7432ced6 100644 --- a/internal/services/organizations.go +++ b/internal/services/organizations.go @@ -9,6 +9,7 @@ import ( "fmt" "net/http" "net/url" + "strconv" "sync" gitea "gitea.dev/sdk" @@ -94,7 +95,7 @@ func (s Organizations) View(ctx context.Context, name string) (Organization, err return c.Organizations.GetOrg(ctx, name) }) if err != nil { - return Organization{}, err + return Organization{}, Classify(err, "organization", strconv.Quote(name), "") } return organizationFrom(org), nil } @@ -159,7 +160,7 @@ func (s Organizations) Create(ctx context.Context, opts OrganizationOptions) (Or }) }) if err != nil { - return Organization{}, err + return Organization{}, Classify(err, "organization", "", "") } return organizationFrom(org), nil } @@ -176,9 +177,10 @@ func (s Organizations) Edit(ctx context.Context, name string, opts OrganizationO // Delete deletes the named organization. func (s Organizations) Delete(ctx context.Context, name string) error { - return do(ctx, s.Client, func(ctx context.Context, c *gitea.Client) (*gitea.Response, error) { + err := do(ctx, s.Client, func(ctx context.Context, c *gitea.Client) (*gitea.Response, error) { return c.Organizations.DeleteOrg(ctx, name) }) + return Classify(err, "organization", strconv.Quote(name), "") } // OrganizationMember identifies a member without exposing private profile fields. diff --git a/internal/services/pulls.go b/internal/services/pulls.go index bbfac283..5c676c32 100644 --- a/internal/services/pulls.go +++ b/internal/services/pulls.go @@ -237,9 +237,10 @@ func (s Pulls) View(ctx context.Context, n int64) (Pull, error) { } func (s Pulls) get(ctx context.Context, n int64) (*gitea.PullRequest, error) { - return repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.PullRequest, *gitea.Response, error) { + item, err := repoCall(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.PullRequest, *gitea.Response, error) { return c.PullRequests.GetPullRequest(ctx, owner, name, n) }) + return item, Classify(err, "pull request", "#"+strconv.FormatInt(n, 10), s.Repository) } // ListOpen returns every open pull request in the repository. @@ -255,7 +256,7 @@ func (s Pulls) Create(ctx context.Context, opts PullOptions) (Pull, error) { // TODO(sdk): CreatePullRequestOption lacks allow_maintainer_edit. var pull gitea.PullRequest if err := sendJSON(ctx, s.Client, http.MethodPost, "repos/"+s.Repository+"/pulls", opts, &pull); err != nil { - return Pull{}, err + return Pull{}, Classify(err, "pull request", "", s.Repository) } return pullFrom(&pull), nil } @@ -308,9 +309,10 @@ func (s Pulls) Edit(ctx context.Context, n int64, opts PullEdit) (Pull, error) { } return result, nil } - return repoConvert(ctx, s.Client, s.Repository, pullFrom, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.PullRequest, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, s.Repository, pullFrom, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.PullRequest, *gitea.Response, error) { return c.PullRequests.EditPullRequest(ctx, owner, name, n, editPullOption(opts)) }) + return item, Classify(err, "pull request", "#"+strconv.FormatInt(n, 10), s.Repository) } func editPullOption(opts PullEdit) gitea.EditPullRequestOption { @@ -438,8 +440,9 @@ func (s Pulls) Merge(ctx context.Context, n int64, opts MergeOptions) error { _, resp, err := c.PullRequests.MergePullRequest(ctx, owner, name, n, option) return resp, err }) + key := "#" + strconv.FormatInt(n, 10) if refused, ok := errors.AsType[*transport.APIError](err); !ok || refused.Status != http.StatusMethodNotAllowed { - return err + return Classify(err, "pull request", key, s.Repository) } var reason string switch { @@ -452,7 +455,7 @@ func (s Pulls) Merge(ctx context.Context, n int64, opts MergeOptions) error { case !pull.Mergeable: reason = "has conflicting files or is otherwise not mergeable" default: - return err + return Classify(err, "pull request", key, s.Repository) } return fmt.Errorf("pull request #%d %s: %w", n, reason, err) } diff --git a/internal/services/releases.go b/internal/services/releases.go index 78dda95e..f4dc892c 100644 --- a/internal/services/releases.go +++ b/internal/services/releases.go @@ -80,9 +80,10 @@ func (s Releases) ListPage(ctx context.Context, limit, page int) (Page[Release], // View retrieves a release by its tag, including numeric tags. func (s Releases) View(ctx context.Context, tag string) (Release, error) { - return repoConvert(ctx, s.Client, s.Repository, releaseFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Release, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, s.Repository, releaseFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Release, *gitea.Response, error) { return c.Releases.GetReleaseByTag(ctx, owner, repo, tag) }) + return item, Classify(err, "release", tag, s.Repository) } // Latest retrieves the most recent release that is neither a draft nor a prerelease. @@ -102,12 +103,13 @@ func (s Releases) Archive(ctx context.Context, ref, format string) (io.ReadClose // Create creates a release and lets Gitea create a missing tag at the supplied target. func (s Releases) Create(ctx context.Context, opts ReleaseOptions) (Release, error) { - return repoConvert(ctx, s.Client, s.Repository, releaseFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Release, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, s.Repository, releaseFrom, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Release, *gitea.Response, error) { return c.Releases.CreateRelease(ctx, owner, repo, gitea.CreateReleaseOption{ TagName: deref(opts.Tag), Target: deref(opts.Target), Title: deref(opts.Title), Note: deref(opts.Body), IsDraft: deref(opts.Draft), IsPrerelease: deref(opts.Prerelease), }) }) + return item, Classify(err, "release", deref(opts.Tag), s.Repository) } // Edit changes only supplied release fields; Gitea ignores empty text fields. diff --git a/internal/services/repositories.go b/internal/services/repositories.go index e033d57f..6302a9e8 100644 --- a/internal/services/repositories.go +++ b/internal/services/repositories.go @@ -93,9 +93,10 @@ func (s Repositories) View(ctx context.Context, repo string) (Repository, error) } func (s Repositories) get(ctx context.Context, repo string) (*gitea.Repository, error) { - return repoCall(ctx, s.Client, repo, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Repository, *gitea.Response, error) { + item, err := repoCall(ctx, s.Client, repo, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Repository, *gitea.Response, error) { return c.Repositories.GetRepo(ctx, owner, name) }) + return item, Classify(err, "repository", repo, "") } // readmeNames are the README file names that Gitea shows, in order of preference. @@ -195,7 +196,7 @@ func (s Repositories) Create(ctx context.Context, org string, opts RepositoryOpt return c.Repositories.CreateRepo(ctx, options) }) if err != nil { - return Repository{}, err + return Repository{}, Classify(err, "repository", "", "") } return repositoryFrom(item), nil } @@ -207,7 +208,7 @@ func (s Repositories) Edit(ctx context.Context, repo string, opts RepositoryEdit style := gitea.MergeStyle(*opts.MergeStyle) mergeStyle = &style } - return repoConvert(ctx, s.Client, repo, repositoryFrom, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Repository, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, repo, repositoryFrom, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Repository, *gitea.Response, error) { return c.Repositories.EditRepo(ctx, owner, name, gitea.EditRepoOption{ HasActions: opts.ActionsEnabled, Name: opts.Name, Website: opts.Website, DefaultBranch: opts.DefaultBranch, Description: opts.Description, Private: opts.Private, Archived: opts.Archived, Template: opts.Template, @@ -216,6 +217,7 @@ func (s Repositories) Edit(ctx context.Context, repo string, opts RepositoryEdit DefaultDeleteBranchAfterMerge: opts.DeleteBranch, }) }) + return item, Classify(err, "repository", repo, "") } // AddTopic adds a topic to the repository. @@ -234,9 +236,10 @@ func (s Repositories) RemoveTopic(ctx context.Context, repo, topic string) error // Delete removes the selected repository. func (s Repositories) Delete(ctx context.Context, repo string) error { - return repoDo(ctx, s.Client, repo, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Response, error) { + err := repoDo(ctx, s.Client, repo, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.Response, error) { return c.Repositories.DeleteRepo(ctx, owner, name) }) + return Classify(err, "repository", repo, "") } // ForkOptions places a fork: under Organization instead of the account, and named Name instead diff --git a/internal/services/reviews.go b/internal/services/reviews.go index 89dbf751..8b505de0 100644 --- a/internal/services/reviews.go +++ b/internal/services/reviews.go @@ -11,6 +11,7 @@ import ( "net/http" "slices" "sort" + "strconv" "strings" gitea "gitea.dev/sdk" @@ -135,9 +136,10 @@ func (s Reviews) Create(ctx context.Context, n int64, opts ReviewOptions) (Revie Path: comment.Path, Body: comment.Body, OldLineNum: comment.OldLine, NewLineNum: comment.NewLine, }) } - return repoConvert(ctx, s.Client, s.Repository, reviewFrom, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.PullReview, *gitea.Response, error) { + item, err := repoConvert(ctx, s.Client, s.Repository, reviewFrom, func(ctx context.Context, c *gitea.Client, owner, name string) (*gitea.PullReview, *gitea.Response, error) { return c.PullRequests.CreatePullReview(ctx, owner, name, n, option) }) + return item, Classify(err, "pull request", "#"+strconv.FormatInt(n, 10), s.Repository) } // Submit publishes an existing pending review with explicitly collected input. diff --git a/internal/services/ssh_keys.go b/internal/services/ssh_keys.go index eb7052c9..c268d988 100644 --- a/internal/services/ssh_keys.go +++ b/internal/services/ssh_keys.go @@ -6,6 +6,7 @@ package services import ( "context" "net/http" + "strconv" gitea "gitea.dev/sdk" "gitea.dev/tea/internal/transport" @@ -45,7 +46,7 @@ func (s SSHKeys) View(ctx context.Context, id int64) (SSHKey, error) { return c.Users.GetPublicKey(ctx, id) }) if err != nil { - return SSHKey{}, err + return SSHKey{}, Classify(err, "SSH key", strconv.FormatInt(id, 10), "") } return sshKeyFrom(key), nil } @@ -56,14 +57,15 @@ func (s SSHKeys) Create(ctx context.Context, title, key string) (SSHKey, error) return c.Users.CreatePublicKey(ctx, gitea.CreateKeyOption{Title: title, Key: key}) }) if err != nil { - return SSHKey{}, err + return SSHKey{}, Classify(err, "SSH key", "", "") } return sshKeyFrom(created), nil } // Delete removes only the selected public key. func (s SSHKeys) Delete(ctx context.Context, id int64) error { - return do(ctx, s.Client, func(ctx context.Context, c *gitea.Client) (*gitea.Response, error) { + err := do(ctx, s.Client, func(ctx context.Context, c *gitea.Client) (*gitea.Response, error) { return c.Users.DeletePublicKey(ctx, id) }) + return Classify(err, "SSH key", strconv.FormatInt(id, 10), "") } diff --git a/internal/services/wiki.go b/internal/services/wiki.go index 302d2d52..d63c59ec 100644 --- a/internal/services/wiki.go +++ b/internal/services/wiki.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "net/http" + "strconv" gitea "gitea.dev/sdk" "gitea.dev/tea/internal/transport" @@ -121,9 +122,10 @@ func (s Wiki) ListPage(ctx context.Context, limit, page int) (Page[WikiPage], er // View retrieves and decodes a wiki page. func (s Wiki) View(ctx context.Context, name string) (WikiPage, error) { - return s.page(ctx, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.WikiPage, *gitea.Response, error) { + item, err := s.page(ctx, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.WikiPage, *gitea.Response, error) { return c.Wiki.GetPage(ctx, owner, repo, name) }) + return item, Classify(err, "wiki page", strconv.Quote(name), s.Repository) } func wikiOptions(opts WikiOptions) gitea.CreateWikiPageOptions { @@ -132,9 +134,10 @@ func wikiOptions(opts WikiOptions) gitea.CreateWikiPageOptions { // Create creates one wiki page, initializing the wiki if necessary. func (s Wiki) Create(ctx context.Context, opts WikiOptions) (WikiPage, error) { - return s.page(ctx, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.WikiPage, *gitea.Response, error) { + item, err := s.page(ctx, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.WikiPage, *gitea.Response, error) { return c.Wiki.CreatePage(ctx, owner, repo, wikiOptions(opts)) }) + return item, Classify(err, "wiki page", strconv.Quote(opts.Title), s.Repository) } // Edit replaces page content and optionally changes its title. @@ -146,16 +149,18 @@ func (s Wiki) Edit(ctx context.Context, name string, opts WikiOptions) (WikiPage } opts.Title = current.Title } - return s.page(ctx, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.WikiPage, *gitea.Response, error) { + item, err := s.page(ctx, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.WikiPage, *gitea.Response, error) { return c.Wiki.EditPage(ctx, owner, repo, name, wikiOptions(opts)) }) + return item, Classify(err, "wiki page", strconv.Quote(name), s.Repository) } // Delete removes a named wiki page. func (s Wiki) Delete(ctx context.Context, name string) error { - return repoDo(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Response, error) { + err := repoDo(ctx, s.Client, s.Repository, func(ctx context.Context, c *gitea.Client, owner, repo string) (*gitea.Response, error) { return c.Wiki.DeletePage(ctx, owner, repo, name) }) + return Classify(err, "wiki page", strconv.Quote(name), s.Repository) } // Revisions returns one server-paginated page of wiki commits. diff --git a/internal/transport/api.go b/internal/transport/api.go index 248dd199..22911535 100644 --- a/internal/transport/api.go +++ b/internal/transport/api.go @@ -55,7 +55,7 @@ func (t captureTransport) RoundTrip(req *http.Request) (*http.Response, error) { } data, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBody)) failure := newAPIError(resp.StatusCode, req.Method, req.URL.Path, t.token, data) - failure.bind(req.URL, t.base) + failure.bind(req.URL) holder.Store(failure) resp.Body = struct { io.Reader diff --git a/internal/transport/client.go b/internal/transport/client.go index f5b687c5..4d902188 100644 --- a/internal/transport/client.go +++ b/internal/transport/client.go @@ -35,8 +35,6 @@ type APIError struct { // Instance is the installation URL the failed request was sent to, when it was recorded // from a request. Instance string - // lookup repeats reads on the failed request's instance with its credential. - lookup http.RoundTripper } func (e *APIError) Error() string { @@ -68,24 +66,11 @@ func (e *APIError) request() string { return strings.TrimSpace(fmt.Sprintf("HTTP %d %s %s", e.Status, e.Method, e.Endpoint)) } -// bind records the instance of the request to u and the credentialed transport rt, so the -// failure can later be explained with LookupClient. -func (e *APIError) bind(u *url.URL, rt http.RoundTripper) { - prefix, _, ok := strings.Cut(u.Path, "/api/v1/") - if !ok { - return +// bind records the instance of the request to u. +func (e *APIError) bind(u *url.URL) { + if prefix, _, ok := strings.Cut(u.Path, "/api/v1/"); ok { + e.Instance = (&url.URL{Scheme: u.Scheme, Host: u.Host, Path: prefix}).String() } - instance := url.URL{Scheme: u.Scheme, Host: u.Host, Path: prefix} - e.Instance, e.lookup = instance.String(), rt -} - -// LookupClient returns an HTTP client that repeats reads on the instance of the failed request, -// Instance, with its credential, or nil when the error was not recorded from a request. -func (e *APIError) LookupClient() *http.Client { - if e.lookup == nil { - return nil - } - return &http.Client{Transport: e.lookup, CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} } const ( diff --git a/internal/transport/client_test.go b/internal/transport/client_test.go index cd198887..5212d219 100644 --- a/internal/transport/client_test.go +++ b/internal/transport/client_test.go @@ -213,30 +213,3 @@ func TestDebugRedactsOnlySensitiveQueryValues(t *testing.T) { require.Equal(t, tc.redacted, sensitiveQuery(tc.path, tc.key), "%s %s", tc.path, tc.key) } } - -func TestAPIErrorLookupRepeatsReadsWithTheFailedCredential(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - require.Equal(t, "token secret", r.Header.Get("Authorization")) - require.Equal(t, "bob", r.Header.Get("Sudo")) - if r.URL.Path == "/base/api/v1/repos/a/b" { - _, _ = w.Write([]byte(`{"full_name":"a/b","has_issues":false}`)) - return - } - w.WriteHeader(http.StatusNotFound) - _, _ = w.Write([]byte(`{"message":"not found"}`)) - })) - defer server.Close() - client := New(server.URL+"/base", "secret", false) - client.Sudo = "bob" - _, err := client.Do(context.Background(), http.MethodGet, "repos/a/b/issues/9", nil, nil) - var apiErr *APIError - require.ErrorAs(t, err, &apiErr) - require.Equal(t, server.URL+"/base", apiErr.Instance) - lookup := apiErr.LookupClient() - require.NotNil(t, lookup) - resp, err := lookup.Get(server.URL + "/base/api/v1/repos/a/b") - require.NoError(t, err) - _ = resp.Body.Close() - require.Equal(t, http.StatusOK, resp.StatusCode) - require.Nil(t, (&APIError{Status: http.StatusNotFound}).LookupClient(), "errors built without a request cannot look up") -}