From 5fd4be98037d7b8dff7fa6e7d9e08d8a17d743a4 Mon Sep 17 00:00:00 2001 From: bircni Date: Mon, 12 Oct 2026 09:57:12 +0200 Subject: [PATCH] refactor(errors): explain failed requests through the SDK The hints for a missing repository, a pull request number that names an issue and an existing pull request read the API with raw GETs from the app layer. Read them through SDK lookups in services, which reuse the failed request's instance and credential, and let transport only hand out that client. Signed-off-by: bircni --- internal/app/errors.go | 32 ++++-------- internal/services/lookups.go | 81 +++++++++++++++++++++++++++++++ internal/transport/client.go | 37 ++++---------- internal/transport/client_test.go | 15 +++--- 4 files changed, 106 insertions(+), 59 deletions(-) create mode 100644 internal/services/lookups.go diff --git a/internal/app/errors.go b/internal/app/errors.go index 51cbca9b..bc86df6e 100644 --- a/internal/app/errors.go +++ b/internal/app/errors.go @@ -10,7 +10,7 @@ import ( "fmt" "net" "net/http" - "net/url" + "strconv" "strings" "syscall" "time" @@ -116,18 +116,10 @@ func explainable(err error, apiErr *transport.APIError) bool { // 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 struct { - Empty bool `json:"empty"` - HasIssues bool `json:"has_issues"` - HasPullRequests bool `json:"has_pull_requests"` - HasWiki bool `json:"has_wiki"` - HasReleases bool `json:"has_releases"` - HasActions bool `json:"has_actions"` - HasProjects bool `json:"has_projects"` - } + var state services.RepositoryState status := http.StatusNotFound if resource.Kind != "repository" { - status = apiErr.Lookup(ctx, "repos/"+repo, &state) + state, status = services.LookupRepository(ctx, apiErr, repo) } switch status { case http.StatusNotFound: @@ -140,9 +132,9 @@ func explainRepositoryNotFound(ctx context.Context, apiErr *transport.APIError, name string enabled bool }{ - "issues": {"issues are", state.HasIssues}, "pulls": {"pull requests are", state.HasPullRequests}, - "wiki": {"the wiki is", state.HasWiki}, "releases": {"releases are", state.HasReleases}, - "actions": {"Actions are", state.HasActions}, "projects": {"projects are", state.HasProjects}, + "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 @@ -151,10 +143,7 @@ func explainRepositoryNotFound(ctx context.Context, apiErr *transport.APIError, return repo + " is empty; it has no pull requests" } if number, ok := strings.CutPrefix(resource.Key, "#"); ok && resource.Kind == "pull request" { - var issue struct { - PullRequest any `json:"pull_request"` - } - if apiErr.Lookup(ctx, "repos/"+repo+"/issues/"+number, &issue) == http.StatusOK && issue.PullRequest == nil { + 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) } } @@ -204,11 +193,8 @@ func existsHint(ctx context.Context, apiErr *transport.APIError, conflict *servi if !ok || conflict.Scope == "" { return "" } - var pull struct { - URL string `json:"html_url"` - } - if apiErr.Lookup(ctx, "repos/"+conflict.Scope+"/pulls/"+url.PathEscape(base)+"/"+url.PathEscape(head), &pull) == http.StatusOK && pull.URL != "" { - return "see " + pull.URL + 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 diff --git a/internal/services/lookups.go b/internal/services/lookups.go new file mode 100644 index 00000000..81145ce4 --- /dev/null +++ b/internal/services/lookups.go @@ -0,0 +1,81 @@ +// 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/transport/client.go b/internal/transport/client.go index 234f453d..f5b687c5 100644 --- a/internal/transport/client.go +++ b/internal/transport/client.go @@ -35,9 +35,8 @@ type APIError struct { // Instance is the installation URL the failed request was sent to, when it was recorded // from a request. Instance string - // apiBase and lookup repeat reads on the failed request's instance with its credential. - apiBase string - lookup http.RoundTripper + // lookup repeats reads on the failed request's instance with its credential. + lookup http.RoundTripper } func (e *APIError) Error() string { @@ -70,46 +69,28 @@ func (e *APIError) request() string { } // bind records the instance of the request to u and the credentialed transport rt, so the -// failure can later be explained with Lookup. +// 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 } instance := url.URL{Scheme: u.Scheme, Host: u.Host, Path: prefix} - e.Instance = instance.String() - instance.Path += "/api/v1/" - e.apiBase, e.lookup = instance.String(), rt + e.Instance, e.lookup = instance.String(), rt } -// Lookup reads endpoint, a path below the API base such as "repos/alice/project", with the -// instance and credential of the request that failed, and decodes a successful JSON response -// into out. It returns the response status, or 0 when the error was not recorded from a -// request or the lookup itself failed. -func (e *APIError) Lookup(ctx context.Context, endpoint string, out any) int { +// 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 0 + return nil } - req, err := http.NewRequestWithContext(ctx, http.MethodGet, e.apiBase+strings.TrimLeft(endpoint, "/"), nil) - if err != nil { - return 0 - } - req.Header.Set("Accept", "application/json") - resp, err := e.lookup.RoundTrip(req) - if err != nil { - return 0 - } - defer resp.Body.Close() - if resp.StatusCode >= 200 && resp.StatusCode < 300 && out != nil && json.NewDecoder(io.LimitReader(resp.Body, maxLookupBody)).Decode(out) != nil { - return 0 - } - return resp.StatusCode + return &http.Client{Transport: e.lookup, CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} } const ( maxErrorBody = 4 << 10 maxErrorMessage = 300 - maxLookupBody = 1 << 20 ) // newAPIError keeps only a bounded, printable JSON "message" field of a failed response body. diff --git a/internal/transport/client_test.go b/internal/transport/client_test.go index 3363f4c5..cd198887 100644 --- a/internal/transport/client_test.go +++ b/internal/transport/client_test.go @@ -232,12 +232,11 @@ func TestAPIErrorLookupRepeatsReadsWithTheFailedCredential(t *testing.T) { var apiErr *APIError require.ErrorAs(t, err, &apiErr) require.Equal(t, server.URL+"/base", apiErr.Instance) - var repo struct { - FullName string `json:"full_name"` - HasIssues bool `json:"has_issues"` - } - require.Equal(t, http.StatusOK, apiErr.Lookup(context.Background(), "repos/a/b", &repo)) - require.Equal(t, "a/b", repo.FullName) - require.Equal(t, http.StatusNotFound, apiErr.Lookup(context.Background(), "repos/a/missing", nil)) - require.Zero(t, (&APIError{Status: http.StatusNotFound}).Lookup(context.Background(), "repos/a/b", nil), "errors built without a request cannot look up") + 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") }