Concurrency issues when adding labels to an issue/PR

[gsmet]

Hi,

Some context: we are using the Hub4j GitHub API for our Quarkus Bot (it's a GitHub App based on Quarkus and running as a GraalVM native executable).

The bot can automatically add labels based on triage rules but we started seeing concurrency issues when someone adds labels when the bot is also adding some. Believe it or not, this happened in real life.

AFAICS, the issue is that the GHIssue#addLabels() method doesn't really add the labels but set them. My understanding is that it should use this method: https://docs.github.com/en/rest/reference/issues#add-labels-to-an-issue and that's not what is done here:

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

Lines 362 to 374 in 6d86cfb

	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]));
	}

: you set the labels based on the existing ones + the added ones instead of adding them, which can result in data loss if you're not lucky.

The issue occurs when a label is added manually in the UI between the call at line 365 and the call at line 373: the labels added in the UI are lost, which is very unfortunate given they are critical to our workflow. You can see an example of this issue here: quarkusio/quarkus#15402 (comment) - Georgios added the triage/backport? label in the UI and the bot removed it while our bot is only adding labels.

Does it make any sense? Any chance this could be fixed?

Thanks!

[gsmet]

BTW, removeLabels() has exactly the same issue.

[bitwiseman]

@gsmet
Makes perfect sense.
In terms of labels, it is actually the same endpoint, except PUT method sets labels, POST adds labels, and DELETE removes labels.

However, just to be confusing you can also set the list of labels from this endpoint using the PATCH method which is what the setLabels() method currently does, along with all the other set*() methods on GHIssue.

This is definitely a bug. The addLabels/removeLabels methods should use the endpoint you mentioned.

You can submit a PR to fix this. Tests will need to be added (looks like these methods are not currently tested).

[bitwiseman]

If someone fixes this issue, it would be great if they also look at doing #1050 at the same time, but it is not required.

[gsmet]

I started working on that one.

https://github.com/hub4j/github-api/blob/master/CONTRIBUTING.md#running-tests-against-your-personal-github-user-account says I need access to the hub4j test org to record the requests/responses. Could you add me there?

Thanks!

[bitwiseman]

@gsmet
You are now invited.

[gsmet]

Thanks! I'll try to wrap up this work during the week-end!

[bitwiseman]

@gsmet
Great!

By the way, I've done a little testing and fixing #1050 should be as easy as adding @WithBridgeMethods(void.class) to the methods and then having them return List instead of void.