Skip to content

Commit 1f7f646

Browse files
bitwisemanbitwiseman
authored
Merge pull request hub4j#1054 from gsmet/fix-add-remove-labels-concurrency
Fix concurrency issues with GHIssue addLabels and removeLabels
2 parents 199eee4 + a59ee6a commit 1f7f646

71 files changed

Lines changed: 8001 additions & 23 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎src/main/java/org/kohsuke/github/GHIssue.java‎

Lines changed: 35 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -312,7 +312,7 @@ public void assignTo(GHUser user) throws IOException {
312312
}
313313

314314
/**
315-
* Sets labels.
315+
* Sets labels on the target to a specific list.
316316
*
317317
* @param labels
318318
* the labels
@@ -326,6 +326,8 @@ public void setLabels(String... labels) throws IOException {
326326
/**
327327
* Adds labels to the issue.
328328
*
329+
* Labels that are already present on the target are ignored.
330+
*
329331
* @param names
330332
* Names of the label
331333
* @throws IOException
@@ -338,6 +340,8 @@ public void addLabels(String... names) throws IOException {
338340
/**
339341
* Add labels.
340342
*
343+
* Labels that are already present on the target are ignored.
344+
*
341345
* @param labels
342346
* the labels
343347
* @throws IOException
@@ -350,6 +354,8 @@ public void addLabels(GHLabel... labels) throws IOException {
350354
/**
351355
* Add labels.
352356
*
357+
* Labels that are already present on the target are ignored.
358+
*
353359
* @param labels
354360
* the labels
355361
* @throws IOException
@@ -360,21 +366,27 @@ public void addLabels(Collection<GHLabel> labels) throws IOException {
360366
}
361367

362368
private void _addLabels(Collection<String> names) throws IOException {
363-
List<String> newLabels = new ArrayList<String>();
369+
root.createRequest().with("labels", names).method("POST").withUrlPath(getIssuesApiRoute() + "/labels").send();
370+
}
364371

365-
for (GHLabel label : getLabels()) {
366-
newLabels.add(label.getName());
367-
}
368-
for (String name : names) {
369-
if (!newLabels.contains(name)) {
370-
newLabels.add(name);
371-
}
372-
}
373-
setLabels(newLabels.toArray(new String[0]));
372+
/**
373+
* Remove a single label.
374+
*
375+
* Attempting to remove a label that is not present throws {@link GHFileNotFoundException}.
376+
*
377+
* @param name
378+
* the name
379+
* @throws IOException
380+
* the io exception, throws {@link GHFileNotFoundException} if label was not present.
381+
*/
382+
public void removeLabel(String name) throws IOException {
383+
root.createRequest().method("DELETE").withUrlPath(getIssuesApiRoute() + "/labels", name).send();
374384
}
375385

376386
/**
377-
* Remove a given label by name from this issue.
387+
* Remove a collection of labels.
388+
*
389+
* Attempting to remove labels that are not present on the target are ignored.
378390
*
379391
* @param names
380392
* the names
@@ -386,7 +398,9 @@ public void removeLabels(String... names) throws IOException {
386398
}
387399

388400
/**
389-
* Remove labels.
401+
* Remove a collection of labels.
402+
*
403+
* Attempting to remove labels that are not present on the target are ignored.
390404
*
391405
* @param labels
392406
* the labels
@@ -399,7 +413,9 @@ public void removeLabels(GHLabel... labels) throws IOException {
399413
}
400414

401415
/**
402-
* Remove labels.
416+
* Remove a collection of labels.
417+
*
418+
* Attempting to remove labels that are not present on the target are ignored.
403419
*
404420
* @param labels
405421
* the labels
@@ -411,15 +427,13 @@ public void removeLabels(Collection<GHLabel> labels) throws IOException {
411427
}
412428

413429
private void _removeLabels(Collection<String> names) throws IOException {
414-
List<String> newLabels = new ArrayList<String>();
415-
416-
for (GHLabel l : getLabels()) {
417-
if (!names.contains(l.getName())) {
418-
newLabels.add(l.getName());
430+
for (String name : names) {
431+
try {
432+
removeLabel(name);
433+
} catch (GHFileNotFoundException e) {
434+
// when trying to remove multiple labels, we ignore already removed
419435
}
420436
}
421-
422-
setLabels(newLabels.toArray(new String[0]));
423437
}
424438

425439
/**

‎src/test/java/org/kohsuke/github/GHIssueEventTest.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ public void testEventsForSingleIssue() throws Exception {
1818
GHIssue issue = builder.create();
1919

2020
// Generate some events.
21-
issue.addLabels("test-label");
21+
issue.setLabels("test-label");
2222

2323
// Test that the events are present.
2424
List<GHIssueEvent> list = issue.listEvents().toList();

‎src/test/java/org/kohsuke/github/GHPullRequestTest.java‎

Lines changed: 83 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
import java.util.Collections;
1010
import java.util.List;
1111

12-
import static org.hamcrest.CoreMatchers.*;
12+
import static org.hamcrest.Matchers.*;
1313

1414
/**
1515
* @author Kohsuke Kawaguchi
@@ -421,6 +421,88 @@ public void setLabels() throws Exception {
421421
assertEquals(label, labels.iterator().next().getName());
422422
}
423423

424+
@Test
425+
// Requires push access to the test repo to pass
426+
public void addLabels() throws Exception {
427+
GHPullRequest p = getRepository().createPullRequest("addLabels", "test/stable", "master", "## test");
428+
String addedLabel1 = "addLabels_label_name_1";
429+
String addedLabel2 = "addLabels_label_name_2";
430+
String addedLabel3 = "addLabels_label_name_3";
431+
432+
p.addLabels(addedLabel1);
433+
434+
int requestCount = mockGitHub.getRequestCount();
435+
p.addLabels(addedLabel2, addedLabel3);
436+
// multiple labels can be added with one api call
437+
assertThat(mockGitHub.getRequestCount(), equalTo(requestCount + 1));
438+
439+
Collection<GHLabel> labels = getRepository().getPullRequest(p.getNumber()).getLabels();
440+
assertEquals(3, labels.size());
441+
assertThat(labels,
442+
containsInAnyOrder(hasProperty("name", equalTo(addedLabel1)),
443+
hasProperty("name", equalTo(addedLabel2)),
444+
hasProperty("name", equalTo(addedLabel3))));
445+
446+
// Adding a label which is already present does not throw an error
447+
p.addLabels(addedLabel1);
448+
}
449+
450+
@Test
451+
// Requires push access to the test repo to pass
452+
public void addLabelsConcurrencyIssue() throws Exception {
453+
String addedLabel1 = "addLabelsConcurrencyIssue_label_name_1";
454+
String addedLabel2 = "addLabelsConcurrencyIssue_label_name_2";
455+
456+
GHPullRequest p1 = getRepository()
457+
.createPullRequest("addLabelsConcurrencyIssue", "test/stable", "master", "## test");
458+
p1.getLabels();
459+
460+
GHPullRequest p2 = getRepository().getPullRequest(p1.getNumber());
461+
p2.addLabels(addedLabel2);
462+
463+
p1.addLabels(addedLabel1);
464+
465+
Collection<GHLabel> labels = getRepository().getPullRequest(p1.getNumber()).getLabels();
466+
assertEquals(2, labels.size());
467+
assertThat(labels,
468+
containsInAnyOrder(hasProperty("name", equalTo(addedLabel1)),
469+
hasProperty("name", equalTo(addedLabel2))));
470+
}
471+
472+
@Test
473+
// Requires push access to the test repo to pass
474+
public void removeLabels() throws Exception {
475+
GHPullRequest p = getRepository().createPullRequest("removeLabels", "test/stable", "master", "## test");
476+
String label1 = "removeLabels_label_name_1";
477+
String label2 = "removeLabels_label_name_2";
478+
String label3 = "removeLabels_label_name_3";
479+
p.setLabels(label1, label2, label3);
480+
481+
Collection<GHLabel> labels = getRepository().getPullRequest(p.getNumber()).getLabels();
482+
assertEquals(3, labels.size());
483+
484+
int requestCount = mockGitHub.getRequestCount();
485+
p.removeLabels(label2, label3);
486+
// each label deleted is a separate api call
487+
assertThat(mockGitHub.getRequestCount(), equalTo(requestCount + 2));
488+
489+
labels = getRepository().getPullRequest(p.getNumber()).getLabels();
490+
assertEquals(1, labels.size());
491+
assertEquals(label1, labels.iterator().next().getName());
492+
493+
// Removing some labels that are not present does not throw
494+
// This is consistent with earlier behavior and with addLabels()
495+
p.removeLabels(label3);
496+
497+
// Calling removeLabel() on label that is not present will throw
498+
try {
499+
p.removeLabel(label3);
500+
fail("Expected GHFileNotFoundException");
501+
} catch (GHFileNotFoundException e) {
502+
assertThat(e.getMessage(), containsString("Label does not exist"));
503+
}
504+
}
505+
424506
@Test
425507
// Requires push access to the test repo to pass
426508
public void setAssignee() throws Exception {

‎src/test/resources/org/kohsuke/github/GHPullRequestTest/wiremock/addLabels/__files/orgs_hub4j-test-org-2.json‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
{
2+
"login": "hub4j-test-org",
3+
"id": 7544739,
4+
"node_id": "MDEyOk9yZ2FuaXphdGlvbjc1NDQ3Mzk=",
5+
"url": "https://api.github.com/orgs/hub4j-test-org",
6+
"repos_url": "https://api.github.com/orgs/hub4j-test-org/repos",
7+
"events_url": "https://api.github.com/orgs/hub4j-test-org/events",
8+
"hooks_url": "https://api.github.com/orgs/hub4j-test-org/hooks",
9+
"issues_url": "https://api.github.com/orgs/hub4j-test-org/issues",
10+
"members_url": "https://api.github.com/orgs/hub4j-test-org/members{/member}",
11+
"public_members_url": "https://api.github.com/orgs/hub4j-test-org/public_members{/member}",
12+
"avatar_url": "https://avatars.githubusercontent.com/u/7544739?v=4",
13+
"description": "Hub4j Test Org Description (this could be null or blank too)",
14+
"name": "Hub4j Test Org Name (this could be null or blank too)",
15+
"company": null,
16+
"blog": "https://hub4j.url.io/could/be/null",
17+
"location": "Hub4j Test Org Location (this could be null or blank too)",
18+
"email": "hub4jtestorgemail@could.be.null.com",
19+
"twitter_username": null,
20+
"is_verified": false,
21+
"has_organization_projects": true,
22+
"has_repository_projects": true,
23+
"public_repos": 12,
24+
"public_gists": 0,
25+
"followers": 0,
26+
"following": 0,
27+
"html_url": "https://github.com/hub4j-test-org",
28+
"created_at": "2014-05-10T19:39:11Z",
29+
"updated_at": "2020-06-04T05:56:10Z",
30+
"type": "Organization",
31+
"total_private_repos": 2,
32+
"owned_private_repos": 2,
33+
"private_gists": 0,
34+
"disk_usage": 154,
35+
"collaborators": 0,
36+
"billing_email": "kk@kohsuke.org",
37+
"default_repository_permission": "none",
38+
"members_can_create_repositories": false,
39+
"two_factor_requirement_enabled": false,
40+
"members_can_create_pages": true,
41+
"members_can_create_public_pages": true,
42+
"members_can_create_private_pages": true,
43+
"plan": {
44+
"name": "free",
45+
"space": 976562499,
46+
"private_repos": 10000,
47+
"filled_seats": 22,
48+
"seats": 3
49+
}
50+
}

0 commit comments

Comments
 (0)