Git/gitea-tea
1
0 Fork 0
mirror of https://gitea.com/gitea/tea.git synced 2026-10-26 19:11:25 +01:00
Code Issues Packages Projects Releases Wiki Activity

refactor: drop the URL-based error classifier and hint lookups

Browse Source 
Errors are named by the service call that failed: Classify,
NotFoundError and ConflictError at the call site. Remove the endpoint
parser that guessed the resource from the request path, and the
follow-up reads the explainer made at error time (repository state,
issue-vs-pull-request, the existing pull request's URL) together with
the credentialed transport each API error carried for them.

Calls that the endpoint parser covered and the services did not now
classify themselves: issues, pull requests, reviews, comments,
releases, branches, wiki pages, repositories, organizations and SSH
keys. Unclassified failures fall back to the generic
"not found (HTTP 404 GET ...)" text.

Signed-off-by: bircni <bircni@icloud.com>
This commit is contained in:
bircni committed 2026-10-12 10:40:45 +02:00
1 parent ea13c9e4f5
commit 27e39ec4be
20 files changed
+129 -495
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

internal/app/errors.go
+11 -85
View File
@@ -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
internal/app/errors_test.go
+26 -32
View File
@@ -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)
}
}
internal/app/issues.go
+1 -1
View File
@@ -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.
internal/app/ssh_keys.go
+1 -1
View File
@@ -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)