Skip to content

Commit c509a3d

Browse files
mislavmislav
authored
Merge pull request cli#4861 from despreston/4835-labels-race
fix race condition when updating labels
2 parents 43ae0e5 + db50b54 commit c509a3d

File tree

5 files changed

+156
-49
lines changed

5 files changed

+156
-49
lines changed

‎pkg/cmd/issue/edit/edit_test.go‎

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"path/filepath"
99
"testing"
1010

11+
"github.com/cli/cli/v2/api"
1112
"github.com/cli/cli/v2/internal/ghrepo"
1213
prShared "github.com/cli/cli/v2/pkg/cmd/pr/shared"
1314
"github.com/cli/cli/v2/pkg/cmdutil"
@@ -286,13 +287,19 @@ func Test_editRun(t *testing.T) {
286287
Value: "GA",
287288
Edited: true,
288289
},
290+
Metadata: api.RepoMetadataResult{
291+
Labels: []api.RepoLabel{
292+
{Name: "docs", ID: "DOCSID"},
293+
},
294+
},
289295
},
290296
FetchOptions: prShared.FetchOptions,
291297
},
292298
httpStubs: func(t *testing.T, reg *httpmock.Registry) {
293299
mockIssueGet(t, reg)
294300
mockRepoMetadata(t, reg)
295301
mockIssueUpdate(t, reg)
302+
mockIssueUpdateLabels(t, reg)
296303
},
297304
stdout: "https://github.com/OWNER/REPO/issue/123\n",
298305
},
@@ -386,7 +393,8 @@ func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry) {
386393
"nodes": [
387394
{ "name": "feature", "id": "FEATUREID" },
388395
{ "name": "TODO", "id": "TODOID" },
389-
{ "name": "bug", "id": "BUGID" }
396+
{ "name": "bug", "id": "BUGID" },
397+
{ "name": "docs", "id": "DOCSID" }
390398
],
391399
"pageInfo": { "hasNextPage": false }
392400
} } } }
@@ -429,9 +437,22 @@ func mockIssueUpdate(t *testing.T, reg *httpmock.Registry) {
429437
reg.Register(
430438
httpmock.GraphQL(`mutation IssueUpdate\b`),
431439
httpmock.GraphQLMutation(`
432-
{ "data": { "updateIssue": { "issue": {
433-
"id": "123"
434-
} } } }`,
440+
{ "data": { "updateIssue": { "__typename": "" } } }`,
441+
func(inputs map[string]interface{}) {}),
442+
)
443+
}
444+
445+
func mockIssueUpdateLabels(t *testing.T, reg *httpmock.Registry) {
446+
reg.Register(
447+
httpmock.GraphQL(`mutation LabelAdd\b`),
448+
httpmock.GraphQLMutation(`
449+
{ "data": { "addLabelsToLabelable": { "__typename": "" } } }`,
450+
func(inputs map[string]interface{}) {}),
451+
)
452+
reg.Register(
453+
httpmock.GraphQL(`mutation LabelRemove\b`),
454+
httpmock.GraphQLMutation(`
455+
{ "data": { "removeLabelsFromLabelable": { "__typename": "" } } }`,
435456
func(inputs map[string]interface{}) {}),
436457
)
437458
}

‎pkg/cmd/pr/edit/edit.go‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"github.com/cli/cli/v2/pkg/iostreams"
1414
"github.com/shurcooL/githubv4"
1515
"github.com/spf13/cobra"
16+
"golang.org/x/sync/errgroup"
1617
)
1718

1819
type EditOptions struct {
@@ -214,16 +215,19 @@ func editRun(opts *EditOptions) error {
214215
}
215216

216217
func updatePullRequest(httpClient *http.Client, repo ghrepo.Interface, id string, editable shared.Editable) error {
217-
if err := shared.UpdateIssue(httpClient, repo, id, true, editable); err != nil {
218-
return err
218+
var wg errgroup.Group
219+
wg.Go(func() error {
220+
return shared.UpdateIssue(httpClient, repo, id, true, editable)
221+
})
222+
if editable.Reviewers.Edited {
223+
wg.Go(func() error {
224+
return updatePullRequestReviews(httpClient, repo, id, editable)
225+
})
219226
}
220-
return updatePullRequestReviews(httpClient, repo, id, editable)
227+
return wg.Wait()
221228
}
222229

223230
func updatePullRequestReviews(httpClient *http.Client, repo ghrepo.Interface, id string, editable shared.Editable) error {
224-
if !editable.Reviewers.Edited {
225-
return nil
226-
}
227231
userIds, teamIds, err := editable.ReviewerIds()
228232
if err != nil {
229233
return err

‎pkg/cmd/pr/edit/edit_test.go‎

Lines changed: 20 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -357,6 +357,7 @@ func Test_editRun(t *testing.T) {
357357
mockRepoMetadata(t, reg, false)
358358
mockPullRequestUpdate(t, reg)
359359
mockPullRequestReviewersUpdate(t, reg)
360+
mockPullRequestUpdateLabels(t, reg)
360361
},
361362
stdout: "https://github.com/OWNER/REPO/pull/123\n",
362363
},
@@ -387,7 +388,7 @@ func Test_editRun(t *testing.T) {
387388
Edited: true,
388389
},
389390
Labels: shared.EditableSlice{
390-
Value: []string{"feature", "TODO", "bug"},
391+
Add: []string{"feature", "TODO", "bug"},
391392
Remove: []string{"docs"},
392393
Edited: true,
393394
},
@@ -406,6 +407,7 @@ func Test_editRun(t *testing.T) {
406407
httpStubs: func(t *testing.T, reg *httpmock.Registry) {
407408
mockRepoMetadata(t, reg, true)
408409
mockPullRequestUpdate(t, reg)
410+
mockPullRequestUpdateLabels(t, reg)
409411
},
410412
stdout: "https://github.com/OWNER/REPO/pull/123\n",
411413
},
@@ -490,7 +492,8 @@ func mockRepoMetadata(_ *testing.T, reg *httpmock.Registry, skipReviewers bool)
490492
"nodes": [
491493
{ "name": "feature", "id": "FEATUREID" },
492494
{ "name": "TODO", "id": "TODOID" },
493-
{ "name": "bug", "id": "BUGID" }
495+
{ "name": "bug", "id": "BUGID" },
496+
{ "name": "docs", "id": "DOCSID" }
494497
],
495498
"pageInfo": { "hasNextPage": false }
496499
} } } }
@@ -554,6 +557,21 @@ func mockPullRequestReviewersUpdate(t *testing.T, reg *httpmock.Registry) {
554557
httpmock.StringResponse(`{}`))
555558
}
556559

560+
func mockPullRequestUpdateLabels(t *testing.T, reg *httpmock.Registry) {
561+
reg.Register(
562+
httpmock.GraphQL(`mutation LabelAdd\b`),
563+
httpmock.GraphQLMutation(`
564+
{ "data": { "addLabelsToLabelable": { "__typename": "" } } }`,
565+
func(inputs map[string]interface{}) {}),
566+
)
567+
reg.Register(
568+
httpmock.GraphQL(`mutation LabelRemove\b`),
569+
httpmock.GraphQLMutation(`
570+
{ "data": { "removeLabelsFromLabelable": { "__typename": "" } } }`,
571+
func(inputs map[string]interface{}) {}),
572+
)
573+
}
574+
557575
type testFetcher struct{}
558576
type testSurveyor struct {
559577
skipReviewers bool

‎pkg/cmd/pr/shared/editable.go‎

Lines changed: 13 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -120,21 +120,6 @@ func (e Editable) AssigneeIds(client *api.Client, repo ghrepo.Interface) (*[]str
120120
return &a, err
121121
}
122122

123-
func (e Editable) LabelIds() (*[]string, error) {
124-
if !e.Labels.Edited {
125-
return nil, nil
126-
}
127-
if len(e.Labels.Add) != 0 || len(e.Labels.Remove) != 0 {
128-
s := set.NewStringSet()
129-
s.AddValues(e.Labels.Default)
130-
s.AddValues(e.Labels.Add)
131-
s.RemoveValues(e.Labels.Remove)
132-
e.Labels.Value = s.ToSlice()
133-
}
134-
l, err := e.Metadata.LabelsToIDs(e.Labels.Value)
135-
return &l, err
136-
}
137-
138123
func (e Editable) ProjectIds() (*[]string, error) {
139124
if !e.Projects.Edited {
140125
return nil, nil
@@ -189,10 +174,22 @@ func EditFieldsSurvey(editable *Editable, editorCommand string) error {
189174
}
190175
}
191176
if editable.Labels.Edited {
192-
editable.Labels.Value, err = multiSelectSurvey("Labels", editable.Labels.Default, editable.Labels.Options)
177+
editable.Labels.Add, err = multiSelectSurvey("Labels", editable.Labels.Default, editable.Labels.Options)
193178
if err != nil {
194179
return err
195180
}
181+
for _, prev := range editable.Labels.Default {
182+
var found bool
183+
for _, selected := range editable.Labels.Add {
184+
if prev == selected {
185+
found = true
186+
break
187+
}
188+
}
189+
if !found {
190+
editable.Labels.Remove = append(editable.Labels.Remove, prev)
191+
}
192+
}
196193
}
197194
if editable.Projects.Edited {
198195
editable.Projects.Value, err = multiSelectSurvey("Projects", editable.Projects.Default, editable.Projects.Options)

‎pkg/cmd/pr/shared/editable_http.go‎

Lines changed: 88 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -9,19 +9,47 @@ import (
99
"github.com/cli/cli/v2/internal/ghrepo"
1010
graphql "github.com/cli/shurcooL-graphql"
1111
"github.com/shurcooL/githubv4"
12+
"golang.org/x/sync/errgroup"
1213
)
1314

1415
func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR bool, options Editable) error {
15-
title := ghString(options.TitleValue())
16-
body := ghString(options.BodyValue())
16+
var wg errgroup.Group
1717

18-
apiClient := api.NewClientFromHTTP(httpClient)
19-
assigneeIds, err := options.AssigneeIds(apiClient, repo)
20-
if err != nil {
21-
return err
18+
// Labels are updated through discrete mutations to avoid having to replace the entire list of labels
19+
// and risking race conditions.
20+
if options.Labels.Edited {
21+
if len(options.Labels.Add) > 0 {
22+
wg.Go(func() error {
23+
addedLabelIds, err := options.Metadata.LabelsToIDs(options.Labels.Add)
24+
if err != nil {
25+
return err
26+
}
27+
return addLabels(httpClient, id, repo, addedLabelIds)
28+
})
29+
}
30+
if len(options.Labels.Remove) > 0 {
31+
wg.Go(func() error {
32+
removeLabelIds, err := options.Metadata.LabelsToIDs(options.Labels.Remove)
33+
if err != nil {
34+
return err
35+
}
36+
return removeLabels(httpClient, id, repo, removeLabelIds)
37+
})
38+
}
39+
}
40+
41+
if dirtyExcludingLabels(options) {
42+
wg.Go(func() error {
43+
return replaceIssueFields(httpClient, repo, id, isPR, options)
44+
})
2245
}
2346

24-
labelIds, err := options.LabelIds()
47+
return wg.Wait()
48+
}
49+
50+
func replaceIssueFields(httpClient *http.Client, repo ghrepo.Interface, id string, isPR bool, options Editable) error {
51+
apiClient := api.NewClientFromHTTP(httpClient)
52+
assigneeIds, err := options.AssigneeIds(apiClient, repo)
2553
if err != nil {
2654
return err
2755
}
@@ -39,10 +67,9 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR
3967
if isPR {
4068
params := githubv4.UpdatePullRequestInput{
4169
PullRequestID: id,
42-
Title: title,
43-
Body: body,
70+
Title: ghString(options.TitleValue()),
71+
Body: ghString(options.BodyValue()),
4472
AssigneeIDs: ghIds(assigneeIds),
45-
LabelIDs: ghIds(labelIds),
4673
ProjectIDs: ghIds(projectIds),
4774
MilestoneID: ghId(milestoneId),
4875
}
@@ -52,23 +79,65 @@ func UpdateIssue(httpClient *http.Client, repo ghrepo.Interface, id string, isPR
5279
return updatePullRequest(httpClient, repo, params)
5380
}
5481

55-
return updateIssue(httpClient, repo, githubv4.UpdateIssueInput{
82+
params := githubv4.UpdateIssueInput{
5683
ID: id,
57-
Title: title,
58-
Body: body,
84+
Title: ghString(options.TitleValue()),
85+
Body: ghString(options.BodyValue()),
5986
AssigneeIDs: ghIds(assigneeIds),
60-
LabelIDs: ghIds(labelIds),
6187
ProjectIDs: ghIds(projectIds),
6288
MilestoneID: ghId(milestoneId),
63-
})
89+
}
90+
return updateIssue(httpClient, repo, params)
91+
}
92+
93+
func dirtyExcludingLabels(e Editable) bool {
94+
return e.Title.Edited ||
95+
e.Body.Edited ||
96+
e.Base.Edited ||
97+
e.Reviewers.Edited ||
98+
e.Assignees.Edited ||
99+
e.Projects.Edited ||
100+
e.Milestone.Edited
101+
}
102+
103+
func addLabels(httpClient *http.Client, id string, repo ghrepo.Interface, labels []string) error {
104+
params := githubv4.AddLabelsToLabelableInput{
105+
LabelableID: id,
106+
LabelIDs: *ghIds(&labels),
107+
}
108+
109+
var mutation struct {
110+
AddLabelsToLabelable struct {
111+
Typename string `graphql:"__typename"`
112+
} `graphql:"addLabelsToLabelable(input: $input)"`
113+
}
114+
115+
variables := map[string]interface{}{"input": params}
116+
gql := graphql.NewClient(ghinstance.GraphQLEndpoint(repo.RepoHost()), httpClient)
117+
return gql.MutateNamed(context.Background(), "LabelAdd", &mutation, variables)
118+
}
119+
120+
func removeLabels(httpClient *http.Client, id string, repo ghrepo.Interface, labels []string) error {
121+
params := githubv4.RemoveLabelsFromLabelableInput{
122+
LabelableID: id,
123+
LabelIDs: *ghIds(&labels),
124+
}
125+
126+
var mutation struct {
127+
RemoveLabelsFromLabelable struct {
128+
Typename string `graphql:"__typename"`
129+
} `graphql:"removeLabelsFromLabelable(input: $input)"`
130+
}
131+
132+
variables := map[string]interface{}{"input": params}
133+
gql := graphql.NewClient(ghinstance.GraphQLEndpoint(repo.RepoHost()), httpClient)
134+
return gql.MutateNamed(context.Background(), "LabelRemove", &mutation, variables)
64135
}
65136

66137
func updateIssue(httpClient *http.Client, repo ghrepo.Interface, params githubv4.UpdateIssueInput) error {
67138
var mutation struct {
68139
UpdateIssue struct {
69-
Issue struct {
70-
ID string
71-
}
140+
Typename string `graphql:"__typename"`
72141
} `graphql:"updateIssue(input: $input)"`
73142
}
74143
variables := map[string]interface{}{"input": params}
@@ -79,9 +148,7 @@ func updateIssue(httpClient *http.Client, repo ghrepo.Interface, params githubv4
79148
func updatePullRequest(httpClient *http.Client, repo ghrepo.Interface, params githubv4.UpdatePullRequestInput) error {
80149
var mutation struct {
81150
UpdatePullRequest struct {
82-
PullRequest struct {
83-
ID string
84-
}
151+
Typename string `graphql:"__typename"`
85152
} `graphql:"updatePullRequest(input: $input)"`
86153
}
87154
variables := map[string]interface{}{"input": params}

0 commit comments

Comments
 (0)