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

@@ -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)

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) {

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
 }

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

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
 }

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 {