From 02457482a589760cc10c46611b600c40fcf0e139 Mon Sep 17 00:00:00 2001 From: Kynan Ware <47394200+BagToad@users.noreply.github.com> Date: Tue, 12 May 2026 19:38:55 -0600 Subject: [PATCH] Apply deferred update mutations in parallel for gh issue edit The Issues 2.0 mutations (issue type, parent set/remove, sub-issues, blocked-by, blocking) are deferred until after the main UpdateIssue because they target IDs that the standard mutation does not handle. Move them behind a shared api.DeferredUpdateIssue orchestrator that fans them out in parallel and joins all errors so a single failure does not abort the rest. editRun no longer carries its own applyEditParent / applyEditSubIssues / applyEditRelationships helpers; the per-issue goroutine resolves refs to node IDs via a small deferredUpdateIssueOptions builder, then hands the populated DeferredUpdateIssueOptions to api.DeferredUpdateIssue. Also moves ResolveIssueRef and ResolveIssueTypeName from the deleted resolve.go into lookup.go. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- api/queries_issue.go | 109 +++++++++++++++++++ pkg/cmd/issue/edit/edit.go | 186 ++++++++++++-------------------- pkg/cmd/issue/edit/edit_test.go | 62 +++++++---- pkg/cmd/issue/shared/lookup.go | 39 +++++++ pkg/cmd/issue/shared/resolve.go | 47 -------- 5 files changed, 261 insertions(+), 182 deletions(-) delete mode 100644 pkg/cmd/issue/shared/resolve.go diff --git a/api/queries_issue.go b/api/queries_issue.go index 70b69bcff..497528fee 100644 --- a/api/queries_issue.go +++ b/api/queries_issue.go @@ -2,7 +2,9 @@ package api import ( "encoding/json" + "errors" "fmt" + "sync" "time" "github.com/cli/cli/v2/internal/ghrepo" @@ -606,6 +608,113 @@ func RemoveBlockedBy(client *Client, hostname string, issueID string, blockingIs return client.Mutate(hostname, "RemoveBlockedBy", &mutation, variables) } +// DeferredUpdateIssueOptions updates an issue with mutations unsupported by the +// standard issue update mutations. All ID fields are node IDs. +type DeferredUpdateIssueOptions struct { + IssueID string + Hostname string + + IssueTypeID string + + ParentID string + ReplaceExistingParent bool + RemoveParentID string + + AddSubIssueIDs []string + RemoveSubIssueIDs []string + + AddBlockedByIDs []string + RemoveBlockedByIDs []string + + // AddBlockingIDs / RemoveBlockingIDs name issues that this issue + // blocks. They are applied via the addBlockedBy / removeBlockedBy + // mutations with the arguments swapped. + AddBlockingIDs []string + RemoveBlockingIDs []string +} + +// DeferredUpdateIssue runs issue mutations described by opts in +// parallel and returns any failures as a single joined error so a single +// failure does not abort the rest. +func DeferredUpdateIssue(client *Client, opts DeferredUpdateIssueOptions) error { + var mutations []func() error + + if opts.IssueTypeID != "" { + mutations = append(mutations, func() error { + return UpdateIssueIssueType(client, opts.Hostname, opts.IssueID, opts.IssueTypeID) + }) + } + + if opts.ParentID != "" { + mutations = append(mutations, func() error { + return AddSubIssue(client, opts.Hostname, opts.ParentID, opts.IssueID, opts.ReplaceExistingParent) + }) + } else if opts.RemoveParentID != "" { + mutations = append(mutations, func() error { + return RemoveSubIssue(client, opts.Hostname, opts.RemoveParentID, opts.IssueID) + }) + } + + for _, id := range opts.AddSubIssueIDs { + mutations = append(mutations, func() error { + return AddSubIssue(client, opts.Hostname, opts.IssueID, id, false) + }) + } + for _, id := range opts.RemoveSubIssueIDs { + mutations = append(mutations, func() error { + return RemoveSubIssue(client, opts.Hostname, opts.IssueID, id) + }) + } + + for _, id := range opts.AddBlockedByIDs { + mutations = append(mutations, func() error { + return AddBlockedBy(client, opts.Hostname, opts.IssueID, id) + }) + } + for _, id := range opts.RemoveBlockedByIDs { + mutations = append(mutations, func() error { + return RemoveBlockedBy(client, opts.Hostname, opts.IssueID, id) + }) + } + + for _, id := range opts.AddBlockingIDs { + mutations = append(mutations, func() error { + // blocking is the inverse of blocked-by: this issue blocks `id`, + // expressed as `id` is blocked by this issue. + return AddBlockedBy(client, opts.Hostname, id, opts.IssueID) + }) + } + for _, id := range opts.RemoveBlockingIDs { + mutations = append(mutations, func() error { + return RemoveBlockedBy(client, opts.Hostname, id, opts.IssueID) + }) + } + + if len(mutations) == 0 { + return nil + } + + errCh := make(chan error, len(mutations)) + var wg sync.WaitGroup + for _, m := range mutations { + wg.Add(1) + go func(m func() error) { + defer wg.Done() + if err := m(); err != nil { + errCh <- err + } + }(m) + } + wg.Wait() + close(errCh) + + var errs []error + for err := range errCh { + errs = append(errs, err) + } + return errors.Join(errs...) +} + // RepoIssueTypes fetches the available issue types for a repository. func RepoIssueTypes(client *Client, repo ghrepo.Interface) ([]IssueType, error) { query := ` diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index 4a02fa9eb..0361fead3 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -395,37 +395,18 @@ func editRun(opts *EditOptions) error { go func(issue *api.Issue) { defer g.Done() - err := prShared.UpdateIssue(httpClient, baseRepo, issue.ID, issue.IsPullRequest(), editable) - if err != nil { + if err := prShared.UpdateIssue(httpClient, baseRepo, issue.ID, issue.IsPullRequest(), editable); err != nil { failedIssueChan <- fmt.Sprintf("failed to update %s: %s", issue.URL, err) return } - // Issue type mutation - if editable.IssueType.Edited && editable.IssueType.Value != "" { - if err := api.UpdateIssueIssueType(apiClient, baseRepo.RepoHost(), issue.ID, issueTypeID); err != nil { - failedIssueChan <- fmt.Sprintf("failed to update type for %s: %s", issue.URL, err) - return - } - } - - // Parent mutation - if editable.Parent.Edited { - if err := applyEditParent(apiClient, baseRepo, issue, editable.Parent.Value); err != nil { - failedIssueChan <- fmt.Sprintf("failed to update parent for %s: %s", issue.URL, err) - return - } - } - - // Sub-issue mutations - if err := applyEditSubIssues(apiClient, baseRepo, issue, opts); err != nil { - failedIssueChan <- fmt.Sprintf("failed to update sub-issues for %s: %s", issue.URL, err) + mutations, err := deferredUpdateIssueOptions(apiClient, baseRepo, issue, opts, &editable, issueTypeID) + if err != nil { + failedIssueChan <- fmt.Sprintf("failed to update %s: %s", issue.URL, err) return } - - // Relationship mutations - if err := applyEditRelationships(apiClient, baseRepo, issue, opts); err != nil { - failedIssueChan <- fmt.Sprintf("failed to update relationships for %s: %s", issue.URL, err) + if err := api.DeferredUpdateIssue(apiClient, mutations); err != nil { + failedIssueChan <- fmt.Sprintf("failed to update %s:\n%s", issue.URL, err) return } @@ -484,101 +465,72 @@ func lookupIssueTypeID(editable *prShared.Editable) (string, error) { return id, nil } -func applyEditParent(client *api.Client, baseRepo ghrepo.Interface, issue *api.Issue, parentRef string) error { - hostname := baseRepo.RepoHost() +func deferredUpdateIssueOptions(client *api.Client, baseRepo ghrepo.Interface, issue *api.Issue, editOpts *EditOptions, editable *prShared.Editable, issueTypeID string) (api.DeferredUpdateIssueOptions, error) { + updateOpts := api.DeferredUpdateIssueOptions{ + IssueID: issue.ID, + Hostname: baseRepo.RepoHost(), + IssueTypeID: issueTypeID, + ReplaceExistingParent: true, + } - if parentRef == "" { - // Remove parent - use the parent's ID from the fetched issue data - if issue.Parent == nil { - return nil // no parent to remove + if editable.Parent.Edited { + if editable.Parent.Value == "" { + if issue.Parent != nil { + updateOpts.RemoveParentID = issue.Parent.ID + } + } else { + parentID, err := issueShared.ResolveIssueRef(client, baseRepo, editable.Parent.Value) + if err != nil { + return updateOpts, fmt.Errorf("resolving --set-parent reference %q: %w", editable.Parent.Value, err) + } + updateOpts.ParentID = parentID } - return api.RemoveSubIssue(client, hostname, issue.Parent.ID, issue.ID) } - // Set parent with replaceParent=true - parentID, err := issueShared.ResolveIssueRef(client, baseRepo, parentRef) - if err != nil { - return fmt.Errorf("resolving parent: %w", err) + for _, ref := range editOpts.AddSubIssues { + id, err := issueShared.ResolveIssueRef(client, baseRepo, ref) + if err != nil { + return updateOpts, fmt.Errorf("resolving --add-sub-issue reference %q: %w", ref, err) + } + updateOpts.AddSubIssueIDs = append(updateOpts.AddSubIssueIDs, id) } - return api.AddSubIssue(client, hostname, parentID, issue.ID, true) -} - -func applyEditSubIssues(client *api.Client, baseRepo ghrepo.Interface, issue *api.Issue, opts *EditOptions) error { - hostname := baseRepo.RepoHost() - - for _, ref := range opts.AddSubIssues { - subID, err := issueShared.ResolveIssueRef(client, baseRepo, ref) - if err != nil { - return fmt.Errorf("resolving --add-sub-issue reference %q: %w", ref, err) - } - if err := api.AddSubIssue(client, hostname, issue.ID, subID, false); err != nil { - return err - } - } - - for _, ref := range opts.RemoveSubIssues { - subID, err := issueShared.ResolveIssueRef(client, baseRepo, ref) - if err != nil { - return fmt.Errorf("resolving --remove-sub-issue reference %q: %w", ref, err) - } - if err := api.RemoveSubIssue(client, hostname, issue.ID, subID); err != nil { - return err - } - } - - return nil -} - -func applyEditRelationships(client *api.Client, baseRepo ghrepo.Interface, issue *api.Issue, opts *EditOptions) error { - hasRelationshipFlags := len(opts.AddBlockedBy) > 0 || len(opts.RemoveBlockedBy) > 0 || - len(opts.AddBlocking) > 0 || len(opts.RemoveBlocking) > 0 - if !hasRelationshipFlags { - return nil - } - - hostname := baseRepo.RepoHost() - - for _, ref := range opts.AddBlockedBy { - blockingID, err := issueShared.ResolveIssueRef(client, baseRepo, ref) - if err != nil { - return fmt.Errorf("resolving --add-blocked-by reference %q: %w", ref, err) - } - if err := api.AddBlockedBy(client, hostname, issue.ID, blockingID); err != nil { - return err - } - } - - for _, ref := range opts.RemoveBlockedBy { - blockingID, err := issueShared.ResolveIssueRef(client, baseRepo, ref) - if err != nil { - return fmt.Errorf("resolving --remove-blocked-by reference %q: %w", ref, err) - } - if err := api.RemoveBlockedBy(client, hostname, issue.ID, blockingID); err != nil { - return err - } - } - - for _, ref := range opts.AddBlocking { - // --add-blocking swaps args: the OTHER issue is blocked by THIS issue - blockedID, err := issueShared.ResolveIssueRef(client, baseRepo, ref) - if err != nil { - return fmt.Errorf("resolving --add-blocking reference %q: %w", ref, err) - } - if err := api.AddBlockedBy(client, hostname, blockedID, issue.ID); err != nil { - return err - } - } - - for _, ref := range opts.RemoveBlocking { - // --remove-blocking swaps args: the OTHER issue is no longer blocked by THIS issue - blockedID, err := issueShared.ResolveIssueRef(client, baseRepo, ref) - if err != nil { - return fmt.Errorf("resolving --remove-blocking reference %q: %w", ref, err) - } - if err := api.RemoveBlockedBy(client, hostname, blockedID, issue.ID); err != nil { - return err - } - } - - return nil + for _, ref := range editOpts.RemoveSubIssues { + id, err := issueShared.ResolveIssueRef(client, baseRepo, ref) + if err != nil { + return updateOpts, fmt.Errorf("resolving --remove-sub-issue reference %q: %w", ref, err) + } + updateOpts.RemoveSubIssueIDs = append(updateOpts.RemoveSubIssueIDs, id) + } + + for _, ref := range editOpts.AddBlockedBy { + id, err := issueShared.ResolveIssueRef(client, baseRepo, ref) + if err != nil { + return updateOpts, fmt.Errorf("resolving --add-blocked-by reference %q: %w", ref, err) + } + updateOpts.AddBlockedByIDs = append(updateOpts.AddBlockedByIDs, id) + } + for _, ref := range editOpts.RemoveBlockedBy { + id, err := issueShared.ResolveIssueRef(client, baseRepo, ref) + if err != nil { + return updateOpts, fmt.Errorf("resolving --remove-blocked-by reference %q: %w", ref, err) + } + updateOpts.RemoveBlockedByIDs = append(updateOpts.RemoveBlockedByIDs, id) + } + + for _, ref := range editOpts.AddBlocking { + id, err := issueShared.ResolveIssueRef(client, baseRepo, ref) + if err != nil { + return updateOpts, fmt.Errorf("resolving --add-blocking reference %q: %w", ref, err) + } + updateOpts.AddBlockingIDs = append(updateOpts.AddBlockingIDs, id) + } + for _, ref := range editOpts.RemoveBlocking { + id, err := issueShared.ResolveIssueRef(client, baseRepo, ref) + if err != nil { + return updateOpts, fmt.Errorf("resolving --remove-blocking reference %q: %w", ref, err) + } + updateOpts.RemoveBlockingIDs = append(updateOpts.RemoveBlockingIDs, id) + } + + return updateOpts, nil } diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index 4b8999d9f..094a6bdda 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -2,7 +2,9 @@ package edit import ( "bytes" + "encoding/json" "fmt" + "io" "net/http" "os" "path/filepath" @@ -1052,34 +1054,31 @@ func Test_editRun(t *testing.T) { httpStubs: func(t *testing.T, reg *httpmock.Registry) { mockIssueNumberGet(t, reg, 100) reg.Register( - httpmock.GraphQL(`query IssueNodeID\b`), - httpmock.StringResponse(` - { "data": { "repository": { "issue": { "id": "SUB_123_ID" } } } } - `), + issueNodeIDByNumberMatcher(123), + httpmock.StringResponse(`{ "data": { "repository": { "issue": { "id": "SUB_123_ID" } } } }`), ) reg.Register( - httpmock.GraphQL(`mutation AddSubIssue\b`), - httpmock.GraphQLMutation(` - { "data": { "addSubIssue": { "issue": { "id": "100" } } } }`, + issueNodeIDByNumberMatcher(124), + httpmock.StringResponse(`{ "data": { "repository": { "issue": { "id": "SUB_124_ID" } } } }`), + ) + reg.Register( + httpmock.GraphQLMutationMatcher(`mutation AddSubIssue\b`, func(input map[string]interface{}) bool { + return input["subIssueId"] == "SUB_123_ID" + }), + httpmock.GraphQLMutation(`{ "data": { "addSubIssue": { "issue": { "id": "100" } } } }`, func(inputs map[string]interface{}) { assert.Equal(t, "100", inputs["issueId"]) - assert.Equal(t, "SUB_123_ID", inputs["subIssueId"]) assert.Equal(t, false, inputs["replaceParent"]) }), ) reg.Register( - httpmock.GraphQL(`query IssueNodeID\b`), - httpmock.StringResponse(` - { "data": { "repository": { "issue": { "id": "SUB_124_ID" } } } } - `), - ) - reg.Register( - httpmock.GraphQL(`mutation AddSubIssue\b`), - httpmock.GraphQLMutation(` - { "data": { "addSubIssue": { "issue": { "id": "100" } } } }`, + httpmock.GraphQLMutationMatcher(`mutation AddSubIssue\b`, func(input map[string]interface{}) bool { + return input["subIssueId"] == "SUB_124_ID" + }), + httpmock.GraphQLMutation(`{ "data": { "addSubIssue": { "issue": { "id": "100" } } } }`, func(inputs map[string]interface{}) { assert.Equal(t, "100", inputs["issueId"]) - assert.Equal(t, "SUB_124_ID", inputs["subIssueId"]) + assert.Equal(t, false, inputs["replaceParent"]) }), ) }, @@ -1732,3 +1731,30 @@ func TestProjectsV1Deprecation(t *testing.T) { reg.Verify(t) }) } + +// issueNodeIDByNumberMatcher matches an IssueNodeID GraphQL query whose +// number variable equals the given value. Used by tests that issue +// multiple IssueNodeID lookups and need stubs to route by issue number +// rather than by registration order. +func issueNodeIDByNumberMatcher(number int) httpmock.Matcher { + queryMatcher := httpmock.GraphQL(`query IssueNodeID\b`) + return func(req *http.Request) bool { + if !queryMatcher(req) { + return false + } + body, err := io.ReadAll(req.Body) + if err != nil { + return false + } + req.Body = io.NopCloser(bytes.NewReader(body)) + var b struct { + Variables struct { + Number int `json:"number"` + } `json:"variables"` + } + if err := json.Unmarshal(body, &b); err != nil { + return false + } + return b.Variables.Number == number + } +} diff --git a/pkg/cmd/issue/shared/lookup.go b/pkg/cmd/issue/shared/lookup.go index 63efd61f7..8501bfcfa 100644 --- a/pkg/cmd/issue/shared/lookup.go +++ b/pkg/cmd/issue/shared/lookup.go @@ -208,3 +208,42 @@ func FindIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, f return resp.Repository.Issue, nil } + +// ResolveIssueRef parses an issue reference (number or URL) and returns its +// node ID. References that point at a different host than baseRepo are +// rejected because relationship mutations require IDs from the base host. +func ResolveIssueRef(client *api.Client, baseRepo ghrepo.Interface, ref string) (string, error) { + number, repo, err := ParseIssueFromArg(ref) + if err != nil { + return "", err + } + + targetRepo := baseRepo + if r, ok := repo.Value(); ok { + if r.RepoHost() != baseRepo.RepoHost() { + return "", fmt.Errorf("issue reference %q belongs to a different host (%s) than the current repository (%s)", ref, r.RepoHost(), baseRepo.RepoHost()) + } + targetRepo = r + } + + return api.IssueNodeID(client, targetRepo, number) +} + +// ResolveIssueTypeName resolves an issue type name to its node ID by +// fetching the repository's available types. +func ResolveIssueTypeName(client *api.Client, repo ghrepo.Interface, typeName string) (string, error) { + issueTypes, err := api.RepoIssueTypes(client, repo) + if err != nil { + return "", err + } + + typeNames := make([]string, len(issueTypes)) + for i, t := range issueTypes { + typeNames[i] = t.Name + if strings.EqualFold(t.Name, typeName) { + return t.ID, nil + } + } + + return "", fmt.Errorf("type %q not found; available types: %s", typeName, strings.Join(typeNames, ", ")) +} diff --git a/pkg/cmd/issue/shared/resolve.go b/pkg/cmd/issue/shared/resolve.go deleted file mode 100644 index f83c3e51f..000000000 --- a/pkg/cmd/issue/shared/resolve.go +++ /dev/null @@ -1,47 +0,0 @@ -package shared - -import ( - "fmt" - "strings" - - "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghrepo" -) - -// ResolveIssueRef parses an issue reference (number or URL) and returns its node ID. -func ResolveIssueRef(client *api.Client, baseRepo ghrepo.Interface, ref string) (string, error) { - number, repo, err := ParseIssueFromArg(ref) - if err != nil { - return "", err - } - - targetRepo := baseRepo - if r, ok := repo.Value(); ok { - if r.RepoHost() != baseRepo.RepoHost() { - return "", fmt.Errorf("issue reference %q belongs to a different host (%s) than the current repository (%s)", ref, r.RepoHost(), baseRepo.RepoHost()) - } - targetRepo = r - } - - return api.IssueNodeID(client, targetRepo, number) -} - -// ResolveIssueTypeName resolves an issue type name to its ID by fetching -// available types from the repository. Returns an error listing available -// types if the name is not found. -func ResolveIssueTypeName(client *api.Client, repo ghrepo.Interface, typeName string) (string, error) { - issueTypes, err := api.RepoIssueTypes(client, repo) - if err != nil { - return "", err - } - - typeNames := make([]string, len(issueTypes)) - for i, t := range issueTypes { - typeNames[i] = t.Name - if strings.EqualFold(t.Name, typeName) { - return t.ID, nil - } - } - - return "", fmt.Errorf("type %q not found; available types: %s", typeName, strings.Join(typeNames, ", ")) -}