Skip to content

Commit

Permalink
Fix concurrency issues with GHIssue addLabels and removeLabels
Browse files Browse the repository at this point in the history
  • Loading branch information
gsmet committed Mar 5, 2021
1 parent 6d86cfb commit 776086b
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 21 deletions.
29 changes: 10 additions & 19 deletions src/main/java/org/kohsuke/github/GHIssue.java
Original file line number Diff line number Diff line change
Expand Up @@ -360,17 +360,11 @@ public void addLabels(Collection<GHLabel> labels) throws IOException {
}

private void _addLabels(Collection<String> names) throws IOException {
List<String> newLabels = new ArrayList<String>();

for (GHLabel label : getLabels()) {
newLabels.add(label.getName());
}
for (String name : names) {
if (!newLabels.contains(name)) {
newLabels.add(name);
}
}
setLabels(newLabels.toArray(new String[0]));
root.createRequest()
.with("labels", names.toArray(new String[0]))
.method("POST")
.withUrlPath(getIssuesApiRoute() + "/labels")
.send();
}

/**
Expand Down Expand Up @@ -411,15 +405,12 @@ public void removeLabels(Collection<GHLabel> labels) throws IOException {
}

private void _removeLabels(Collection<String> names) throws IOException {
List<String> newLabels = new ArrayList<String>();

for (GHLabel l : getLabels()) {
if (!names.contains(l.getName())) {
newLabels.add(l.getName());
}
for (String name : names) {
root.createRequest()
.method("DELETE")
.withUrlPath(getIssuesApiRoute() + "/labels", name)
.send();
}

setLabels(newLabels.toArray(new String[0]));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/kohsuke/github/GHIssueEventTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ public void testEventsForSingleIssue() throws Exception {
GHIssue issue = builder.create();

// Generate some events.
issue.addLabels("test-label");
issue.setLabels("test-label");

// Test that the events are present.
List<GHIssueEvent> list = issue.listEvents().toList();
Expand Down
36 changes: 35 additions & 1 deletion src/test/java/org/kohsuke/github/GHPullRequestTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import java.util.Collections;
import java.util.List;

import static org.hamcrest.CoreMatchers.*;
import static org.hamcrest.Matchers.*;

/**
* @author Kohsuke Kawaguchi
Expand Down Expand Up @@ -421,6 +421,40 @@ public void setLabels() throws Exception {
assertEquals(label, labels.iterator().next().getName());
}

@Test
// Requires push access to the test repo to pass
public void addLabels() throws Exception {
GHPullRequest p = getRepository().createPullRequest("addLabels", "test/stable", "master", "## test");
String initialLabel = "setLabels_label_name";
p.setLabels(initialLabel);

This comment has been minimized.

Copy link
@bitwiseman

bitwiseman Mar 5, 2021

Why use set at all? Use add, check result, use add again.

This comment has been minimized.

Copy link
@gsmet

gsmet Mar 5, 2021

Author Owner

Yeah I can do that too.


String addedLabel = "addLabels_label_name";
p.addLabels(addedLabel);

Collection<GHLabel> labels = getRepository().getPullRequest(p.getNumber()).getLabels();
assertEquals(2, labels.size());
assertThat(labels, containsInAnyOrder(initialLabel, addedLabel));
}

@Test
// Requires push access to the test repo to pass
public void removeLabels() throws Exception {
GHPullRequest p = getRepository().createPullRequest("removeLabels", "test/stable", "master", "## test");
String label1 = "removeLabels_label_name_1";
String label2 = "removeLabels_label_name_2";
String label3 = "removeLabels_label_name_3";
p.setLabels(label1, label2, label3);

Collection<GHLabel> labels = getRepository().getPullRequest(p.getNumber()).getLabels();
assertEquals(3, labels.size());

p.removeLabels(label2, label3);

labels = getRepository().getPullRequest(p.getNumber()).getLabels();
assertEquals(1, labels.size());
assertEquals(label1, labels.iterator().next().getName());
}

@Test
// Requires push access to the test repo to pass
public void setAssignee() throws Exception {
Expand Down

1 comment on commit 776086b

@bitwiseman
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great!

Please be sure to write tests that would fail with the old behavior and pass with the new behavior. I think you can do it by just storing two copies of the the created pull request and calling add/remove for different labels on each instance.

Please sign in to comment.