From e2f3b24f003887e5a2e59ca468ab6b571a2af987 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Sat, 14 Sep 2024 07:19:39 -0700 Subject: [PATCH] forge: ChangeIsMerged -> ChangesAreMerged (#399) Replace ChangeIsMerged with a bulk ChangesAreMerged API that takes a list of ChangeIDs and returns a list of booleans in the same order. With this in place, we can change 'repo sync' to make a single request for all submitted branches instead of one request per branch. [skip changelog]: No user facing changes in this branch. --- internal/forge/forge.go | 2 +- internal/forge/github/integration_test.go | 20 +++--- internal/forge/github/merged.go | 46 ++++++++---- ...tegration_Repository_ChangesAreMerged.yaml | 37 ++++++++++ internal/forge/shamhub/merge.go | 71 ++++++++++++------- repo_sync.go | 4 +- 6 files changed, 125 insertions(+), 55 deletions(-) create mode 100644 internal/forge/github/testdata/fixtures/TestIntegration_Repository_ChangesAreMerged.yaml diff --git a/internal/forge/forge.go b/internal/forge/forge.go index b66c5150..162db203 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -133,7 +133,7 @@ type Repository interface { EditChange(ctx context.Context, id ChangeID, opts EditChangeOptions) error FindChangesByBranch(ctx context.Context, branch string, opts FindChangesOptions) ([]*FindChangeItem, error) FindChangeByID(ctx context.Context, id ChangeID) (*FindChangeItem, error) - ChangeIsMerged(ctx context.Context, id ChangeID) (bool, error) + ChangesAreMerged(ctx context.Context, ids []ChangeID) ([]bool, error) // Post and update comments on changes. PostChangeComment(context.Context, ChangeID, string) (ChangeCommentID, error) diff --git a/internal/forge/github/integration_test.go b/internal/forge/github/integration_test.go index 38155140..b2b4db68 100644 --- a/internal/forge/github/integration_test.go +++ b/internal/forge/github/integration_test.go @@ -199,24 +199,22 @@ func TestIntegration_Repository_FindChangesByBranch(t *testing.T) { }) } -func TestIntegration_Repository_IsMerged(t *testing.T) { +func TestIntegration_Repository_ChangesAreMerged(t *testing.T) { ctx := context.Background() rec := newRecorder(t, t.Name()) ghc := githubv4.NewClient(rec.GetDefaultClient()) repo, err := github.NewRepository(ctx, new(github.Forge), "abhinav", "git-spice", logtest.New(t), ghc, _gitSpiceRepoID) require.NoError(t, err) - t.Run("false", func(t *testing.T) { - ok, err := repo.ChangeIsMerged(ctx, &github.PR{Number: 144}) - require.NoError(t, err) - assert.False(t, ok) - }) - - t.Run("true", func(t *testing.T) { - ok, err := repo.ChangeIsMerged(ctx, &github.PR{Number: 141}) - require.NoError(t, err) - assert.True(t, ok) + merged, err := repo.ChangesAreMerged(ctx, []forge.ChangeID{ + &github.PR{Number: 196, GQLID: "PR_kwDOJ2BQKs5ylEYu"}, // merged + &github.PR{Number: 387, GQLID: "PR_kwDOJ2BQKs56wX01"}, // open (not merged) + &github.PR{Number: 144, GQLID: "PR_kwDOJ2BQKs5xNeqO"}, // merged + // Explicit GQL ID means we don't need to be in the same repo. + &github.PR{Number: 4, GQLID: githubv4.ID("PR_kwDOMVd0xs51N_9r")}, // closed (not merged) }) + require.NoError(t, err) + assert.Equal(t, []bool{true, false, true, false}, merged) } func TestIntegration_Repository_ListChangeTemplates(t *testing.T) { diff --git a/internal/forge/github/merged.go b/internal/forge/github/merged.go index 9cf906db..0e4dbf33 100644 --- a/internal/forge/github/merged.go +++ b/internal/forge/github/merged.go @@ -8,25 +8,41 @@ import ( "go.abhg.dev/gs/internal/forge" ) -// ChangeIsMerged reports whether a change has been merged. -func (r *Repository) ChangeIsMerged(ctx context.Context, id forge.ChangeID) (bool, error) { - // TODO: Bulk ChangesAreMerged that takes a list of IDs. - +// ChangesAreMerged reports whether the given changes have been merged. +// The returned slice is in the same order as the input slice. +func (r *Repository) ChangesAreMerged(ctx context.Context, ids []forge.ChangeID) ([]bool, error) { var q struct { - Repository struct { + Nodes []struct { PullRequest struct { Merged bool `graphql:"merged"` - } `graphql:"pullRequest(number: $number)"` - } `graphql:"repository(owner: $owner, name: $repo)"` + } `graphql:"... on PullRequest"` + } `graphql:"nodes(ids: $ids)"` + } + + gqlIDs := make([]githubv4.ID, len(ids)) + for i, id := range ids { + // Since before the first stable v0.1.0, + // the data store has tracked the GraphQL ID of each change, + // so this won't actually make a network request. + // + // However, if a [PR] was constructed in-process + // and not from the data store, we need to resolve it, + // and that will make a network request. + var err error + gqlIDs[i], err = r.graphQLID(ctx, mustPR(id)) + if err != nil { + return nil, fmt.Errorf("resolve ID %v: %w", id, err) + } } - err := r.client.Query(ctx, &q, map[string]any{ - "owner": githubv4.String(r.owner), - "repo": githubv4.String(r.repo), - "number": githubv4.Int(mustPR(id).Number), - }) - if err != nil { - return false, fmt.Errorf("query failed: %w", err) + + if err := r.client.Query(ctx, &q, map[string]any{"ids": gqlIDs}); err != nil { + return nil, fmt.Errorf("query failed: %w", err) + } + + merged := make([]bool, len(ids)) + for i, pr := range q.Nodes { + merged[i] = pr.PullRequest.Merged } - return q.Repository.PullRequest.Merged, nil + return merged, nil } diff --git a/internal/forge/github/testdata/fixtures/TestIntegration_Repository_ChangesAreMerged.yaml b/internal/forge/github/testdata/fixtures/TestIntegration_Repository_ChangesAreMerged.yaml new file mode 100644 index 00000000..9681c622 --- /dev/null +++ b/internal/forge/github/testdata/fixtures/TestIntegration_Repository_ChangesAreMerged.yaml @@ -0,0 +1,37 @@ +--- +version: 2 +interactions: + - id: 0 + request: + proto: HTTP/1.1 + proto_major: 1 + proto_minor: 1 + content_length: 187 + transfer_encoding: [] + trailer: {} + host: api.github.com + remote_addr: "" + request_uri: "" + body: | + {"query":"query($ids:[ID!]!){nodes(ids: $ids){... on PullRequest{merged}}}","variables":{"ids":["PR_kwDOJ2BQKs5ylEYu","PR_kwDOJ2BQKs56wX01","PR_kwDOJ2BQKs5xNeqO","PR_kwDOMVd0xs51N_9r"]}} + form: {} + headers: + Content-Type: + - application/json + url: https://api.github.com/graphql + method: POST + response: + proto: HTTP/2.0 + proto_major: 2 + proto_minor: 0 + transfer_encoding: [] + trailer: {} + content_length: -1 + uncompressed: true + body: '{"data":{"nodes":[{"merged":true},{"merged":false},{"merged":true},{"merged":false}]}}' + headers: + Content-Type: + - application/json; charset=utf-8 + status: 200 OK + code: 200 + duration: 269.167291ms diff --git a/internal/forge/shamhub/merge.go b/internal/forge/shamhub/merge.go index 070bf977..3c3a2cbb 100644 --- a/internal/forge/shamhub/merge.go +++ b/internal/forge/shamhub/merge.go @@ -7,7 +7,6 @@ import ( "net/http" "os" "os/exec" - "strconv" "strings" "time" @@ -16,57 +15,77 @@ import ( "go.abhg.dev/gs/internal/ioutil" ) -type isMergedResponse struct { - Merged bool `json:"merged"` +type areMergedRequest struct { + IDs []ChangeID `json:"ids"` } -var _ = shamhubHandler("GET /{owner}/{repo}/change/{number}/merged", (*ShamHub).handleIsMerged) +type areMergedResponse struct { + Merged []bool `json:"merged"` +} + +var _ = shamhubHandler("POST /{owner}/{repo}/change/merged", (*ShamHub).handleAreMerged) -func (sh *ShamHub) handleIsMerged(w http.ResponseWriter, r *http.Request) { - owner, repo, numStr := r.PathValue("owner"), r.PathValue("repo"), r.PathValue("number") - if owner == "" || repo == "" || numStr == "" { +func (sh *ShamHub) handleAreMerged(w http.ResponseWriter, r *http.Request) { + owner, repo := r.PathValue("owner"), r.PathValue("repo") + if owner == "" || repo == "" { http.Error(w, "owner, repo, and number are required", http.StatusBadRequest) return } - num, err := strconv.Atoi(numStr) - if err != nil { + var req areMergedRequest + if err := json.NewDecoder(r.Body).Decode(&req); err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } + changeNumToIdx := make(map[int]int, len(req.IDs)) + for i, id := range req.IDs { + changeNumToIdx[int(id)] = i + } + sh.mu.RLock() - var ( - merged bool - found bool - ) + merged := make([]bool, len(sh.changes)) for _, c := range sh.changes { - if c.Owner == owner && c.Repo == repo && c.Number == num { - merged = c.State == shamChangeMerged - found = true - break + if c.Owner == owner && c.Repo == repo { + idx, ok := changeNumToIdx[c.Number] + if !ok { + continue + } + merged[idx] = c.State == shamChangeMerged + delete(changeNumToIdx, c.Number) + + if len(changeNumToIdx) == 0 { + break + } } } sh.mu.RUnlock() - if !found { - http.Error(w, "change not found", http.StatusNotFound) + if len(changeNumToIdx) > 0 { + w.WriteHeader(http.StatusNotFound) + fmt.Fprintf(w, "changes not found: %v", changeNumToIdx) return } enc := json.NewEncoder(w) enc.SetIndent("", " ") - if err := enc.Encode(isMergedResponse{Merged: merged}); err != nil { + if err := enc.Encode(areMergedResponse{Merged: merged}); err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) } } -func (f *forgeRepository) ChangeIsMerged(ctx context.Context, fid forge.ChangeID) (bool, error) { - id := fid.(ChangeID) - u := f.apiURL.JoinPath(f.owner, f.repo, "change", strconv.Itoa(int(id)), "merged") - var res isMergedResponse - if err := f.client.Get(ctx, u.String(), &res); err != nil { - return false, fmt.Errorf("is merged: %w", err) +func (f *forgeRepository) ChangesAreMerged(ctx context.Context, fids []forge.ChangeID) ([]bool, error) { + ids := make([]ChangeID, len(fids)) + for i, fid := range fids { + ids[i] = fid.(ChangeID) + } + + u := f.apiURL.JoinPath(f.owner, f.repo, "change", "merged") + req := areMergedRequest{IDs: ids} + + var res areMergedResponse + if err := f.client.Post(ctx, u.String(), req, &res); err != nil { + return nil, fmt.Errorf("are merged: %w", err) } return res.Merged, nil } diff --git a/repo_sync.go b/repo_sync.go index 1dc23de0..25a21cf4 100644 --- a/repo_sync.go +++ b/repo_sync.go @@ -272,13 +272,13 @@ func (cmd *repoSyncCmd) deleteMergedBranches( // TODO: Once we're recording GraphQL IDs in the store, // we can combine all submitted PRs into one query. - merged, err := remoteRepo.ChangeIsMerged(ctx, b.Change) + merged, err := remoteRepo.ChangesAreMerged(ctx, []forge.ChangeID{b.Change}) if err != nil { log.Error("Failed to query CR status", "change", b.Change, "error", err) continue } - b.Merged = merged + b.Merged = merged[0] case b, ok := <-trachedch: if !ok {