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

gsmet · 2021-03-09T15:06:55Z

Description

Fixes concurrency issues when adding and removing labels by using the proper GitHub APIs.

Fixes #1049

Before submitting a PR:

We love getting PRs, but we hate asking people for the same basic changes every time.

Push your changes to a branch other than master. Create your PR from that branch.
Add JavaDocs and other comments
Write tests that run and pass in CI. See CONTRIBUTING.md for details on how to capture snapshot data.
Run mvn -D enable-ci clean install site locally. If this command doesn't succeed, your change will not pass CI.

When creating a PR:

Fill in the "Description" above.
Enable "Allow edits from maintainers".

gsmet · 2021-03-09T15:09:36Z

@bitwiseman I created this PR first as we have detected some issues with bridge methods and Mockito (we are mocking the API calls in our GH App framework) so I prefer to deal with the return values in another PR and check that things are OK for us in a second phase.

This fix is more urgent as our bot is removing labels that we just added due to this concurrency issue.

Thanks!

bitwiseman

One test change requested. Otherwise, look good to me.

bitwiseman · 2021-03-10T02:38:49Z

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

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


Ouch. I didn't notice this behavior. Not great for rate limit budget... But it is correct. Good job.

Yeah, I agree it's unfortunate they did it that way. It should have been symmetrical with adding labels IMHO.

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

bitwiseman · 2021-03-10T02:45:03Z

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

-        }
-        setLabels(newLabels.toArray(new String[0]));
+        root.createRequest()
+                .with("labels", names.toArray(new String[0]))


Wait... this should add a list of labels in one call, not only work for one label.
https://docs.github.com/en/rest/reference/issues#add-labels-to-an-issue

I think that's what it did anyway but I see you can pass directly the collection so that's what is done now.

gsmet · 2021-03-10T13:03:08Z

@bitwiseman I think I addressed all your concerns and I checked adding two labels only makes one API call.

Fixes hub4j#1049

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

bitwiseman · 2021-03-10T19:23:42Z

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

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


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?

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.

(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.

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.

bitwiseman

Sorry, a few more changes and questionsI didn't think of on the previous round.

bitwiseman

@gsmet
I updated removeLabels() to match the existing non-throwing behavior and added a removeLabel() that will behave like the GitHub API and throw.

This retains your fixes but also stays consistent with past behavior where possible.

What do you think?

gsmet · 2021-03-13T18:40:27Z

@bitwiseman sorry, I was extremely busy this week. I'll try to have a look next week!

gsmet · 2021-03-15T17:08:40Z

@bitwiseman I find it a bit weird that removeLabel and removeLabels don't behave the same way but I can live with it and we can adjust if people complain :).

So I think we should merge it!

gsmet force-pushed the fix-add-remove-labels-concurrency branch from d734663 to c742050 Compare March 9, 2021 15:13

bitwiseman requested changes Mar 10, 2021

View reviewed changes