Skip to content

Fix concurrency issues with GHIssue addLabels() and removeLabels() by using labels endpoint #1054

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 15, 2021
Merged

Fix concurrency issues with GHIssue addLabels() and removeLabels() by using labels endpoint #1054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1fefc77
Fix concurrency issues with GHIssue addLabels and removeLabels
gsmet Mar 5, 2021
a59ee6a
Add removeLabel() that throws when label missing
bitwiseman Mar 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 35 additions & 21 deletions src/main/java/org/kohsuke/github/GHIssue.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ public void assignTo(GHUser user) throws IOException {
}

/**
* Sets labels.
* Sets labels on the target to a specific list.
*
* @param labels
* the labels
Expand All @@ -326,6 +326,8 @@ public void setLabels(String... labels) throws IOException {
/**
* Adds labels to the issue.
*
* Labels that are already present on the target are ignored.
*
* @param names
* Names of the label
* @throws IOException
Expand All @@ -338,6 +340,8 @@ public void addLabels(String... names) throws IOException {
/**
* Add labels.
*
* Labels that are already present on the target are ignored.
*
* @param labels
* the labels
* @throws IOException
Expand All @@ -350,6 +354,8 @@ public void addLabels(GHLabel... labels) throws IOException {
/**
* Add labels.
*
* Labels that are already present on the target are ignored.
*
* @param labels
* the labels
* @throws IOException
Expand All @@ -360,21 +366,27 @@ public void addLabels(Collection<GHLabel> labels) throws IOException {
}

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

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]));
/**
* Remove a single label.
*
* Attempting to remove a label that is not present throws {@link GHFileNotFoundException}.
*
* @param name
* the name
* @throws IOException
* the io exception, throws {@link GHFileNotFoundException} if label was not present.
*/
public void removeLabel(String name) throws IOException {
root.createRequest().method("DELETE").withUrlPath(getIssuesApiRoute() + "/labels", name).send();
}

/**
* Remove a given label by name from this issue.
* Remove a collection of labels.
*
* Attempting to remove labels that are not present on the target are ignored.
*
* @param names
* the names
Expand All @@ -386,7 +398,9 @@ public void removeLabels(String... names) throws IOException {
}

/**
* Remove labels.
* Remove a collection of labels.
*
* Attempting to remove labels that are not present on the target are ignored.
*
* @param labels
* the labels
Expand All @@ -399,7 +413,9 @@ public void removeLabels(GHLabel... labels) throws IOException {
}

/**
* Remove labels.
* Remove a collection of labels.
*
* Attempting to remove labels that are not present on the target are ignored.
*
* @param labels
* the labels
Expand All @@ -411,15 +427,13 @@ 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) {
try {
removeLabel(name);
} catch (GHFileNotFoundException e) {
// when trying to remove multiple labels, we ignore already removed
}
}

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

/**
Expand Down
2 changes: 1 addition & 1 deletion src/test/java/org/kohsuke/github/GHIssueEventTest.java
View file
Open in desktop
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
84 changes: 83 additions & 1 deletion src/test/java/org/kohsuke/github/GHPullRequestTest.java
View file
Open in desktop
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,88 @@ 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 addedLabel1 = "addLabels_label_name_1";
String addedLabel2 = "addLabels_label_name_2";
String addedLabel3 = "addLabels_label_name_3";

p.addLabels(addedLabel1);

int requestCount = mockGitHub.getRequestCount();
p.addLabels(addedLabel2, addedLabel3);
// multiple labels can be added with one api call
assertThat(mockGitHub.getRequestCount(), equalTo(requestCount + 1));

Collection<GHLabel> labels = getRepository().getPullRequest(p.getNumber()).getLabels();
assertEquals(3, labels.size());
assertThat(labels,
containsInAnyOrder(hasProperty("name", equalTo(addedLabel1)),
hasProperty("name", equalTo(addedLabel2)),
hasProperty("name", equalTo(addedLabel3))));

// Adding a label which is already present does not throw an error
p.addLabels(addedLabel1);
}

@Test
// Requires push access to the test repo to pass
public void addLabelsConcurrencyIssue() throws Exception {
String addedLabel1 = "addLabelsConcurrencyIssue_label_name_1";
String addedLabel2 = "addLabelsConcurrencyIssue_label_name_2";

GHPullRequest p1 = getRepository()
.createPullRequest("addLabelsConcurrencyIssue", "test/stable", "master", "## test");
p1.getLabels();

GHPullRequest p2 = getRepository().getPullRequest(p1.getNumber());
p2.addLabels(addedLabel2);

p1.addLabels(addedLabel1);

Collection<GHLabel> labels = getRepository().getPullRequest(p1.getNumber()).getLabels();
assertEquals(2, labels.size());
assertThat(labels,
containsInAnyOrder(hasProperty("name", equalTo(addedLabel1)),
hasProperty("name", equalTo(addedLabel2))));
}

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

int requestCount = mockGitHub.getRequestCount();
p.removeLabels(label2, label3);
// each label deleted is a separate api call
assertThat(mockGitHub.getRequestCount(), equalTo(requestCount + 2));

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

// Removing some labels that are not present does not throw
// This is consistent with earlier behavior and with addLabels()
p.removeLabels(label3);

// Calling removeLabel() on label that is not present will throw
try {
p.removeLabel(label3);
fail("Expected GHFileNotFoundException");
} catch (GHFileNotFoundException e) {
assertThat(e.getMessage(), containsString("Label does not exist"));
}
}
Copy link
Member

@bitwiseman bitwiseman Mar 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hm, if you add the following above this line, you'll get a GHFileNotFoundException.

        p.removeLabels(label2);

Need to test this scenario. Also, it seems like this could still result in concurrency issues. Two clients go to remove the same label and one gets an error while the other doesn't. Would you rather merge this change with this or work on a solution?

Copy link
Contributor Author

@gsmet gsmet Mar 10, 2021

Choose a reason for hiding this comment

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

if you... what? There's a word missing :)

I don't see how the case you're talking about is a concurrency issue? For sure, if you have several clients removing the same labels at the same time, you will have errors. But that's how the APIs are designed and that's expected in my opinion.

What's not acceptable is the issue that this PR fixes e.g. your use of the API is perfectly fine and you lose data.

Copy link
Member

@bitwiseman bitwiseman Mar 10, 2021

Choose a reason for hiding this comment

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

(Edited my previous message).

I'm not saying the you have not properly fixed the issue in question. I'm just raising your awareness that you may still run into unexpected errors but only when removing labels.

When you call addLabels() and a label is already present, GitHub returns no error. Multiple clients can add the same label and none will error.

When you call removeLabels() and a label is not present you get an error. Multiple clients cannot remove the same label without all but one of them erroring. The behavior is reasonable but asymmetric. Also, we're providing a single method call to wrap multiple API calls. If adding failed for some reason, no labels will be added. If removeLabels() fails, some label will be removed and others will not. Also, the old method would not throw if you removed a label that was not present.

What you've written simply passes the errors returned from GitHub to the user. Good and fine, but it is a more significant change not just a fix to incorrect behavior.

Copy link
Member

@bitwiseman bitwiseman Mar 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We probably want to have removeLabels(...) match the old method behavior and not throw if any of the label s in the list are not present.
And then add a removeLabel(String) that does throw if the specified label isn't present.


@Test
// Requires push access to the test repo to pass
public void setAssignee() throws Exception {
Expand Down
50 changes: 50 additions & 0 deletions ...rg/kohsuke/github/GHPullRequestTest/wiremock/addLabels/__files/orgs_hub4j-test-org-2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
{
"login": "hub4j-test-org",
"id": 7544739,
"node_id": "MDEyOk9yZ2FuaXphdGlvbjc1NDQ3Mzk=",
"url": "https://api.github.com/orgs/hub4j-test-org",
"repos_url": "https://api.github.com/orgs/hub4j-test-org/repos",
"events_url": "https://api.github.com/orgs/hub4j-test-org/events",
"hooks_url": "https://api.github.com/orgs/hub4j-test-org/hooks",
"issues_url": "https://api.github.com/orgs/hub4j-test-org/issues",
"members_url": "https://api.github.com/orgs/hub4j-test-org/members{/member}",
"public_members_url": "https://api.github.com/orgs/hub4j-test-org/public_members{/member}",
"avatar_url": "https://avatars.githubusercontent.com/u/7544739?v=4",
"description": "Hub4j Test Org Description (this could be null or blank too)",
"name": "Hub4j Test Org Name (this could be null or blank too)",
"company": null,
"blog": "https://hub4j.url.io/could/be/null",
"location": "Hub4j Test Org Location (this could be null or blank too)",
"email": "hub4jtestorgemail@could.be.null.com",
"twitter_username": null,
"is_verified": false,
"has_organization_projects": true,
"has_repository_projects": true,
"public_repos": 12,
"public_gists": 0,
"followers": 0,
"following": 0,
"html_url": "https://github.com/hub4j-test-org",
"created_at": "2014-05-10T19:39:11Z",
"updated_at": "2020-06-04T05:56:10Z",
"type": "Organization",
"total_private_repos": 2,
"owned_private_repos": 2,
"private_gists": 0,
"disk_usage": 154,
"collaborators": 0,
"billing_email": "kk@kohsuke.org",
"default_repository_permission": "none",
"members_can_create_repositories": false,
"two_factor_requirement_enabled": false,
"members_can_create_pages": true,
"members_can_create_public_pages": true,
"members_can_create_private_pages": true,
"plan": {
"name": "free",
"space": 976562499,
"private_repos": 10000,
"filled_seats": 22,
"seats": 3
}
}
Loading