RFR: 1602: Fix labels handling in GitLabMergeRequest

Erik Joelsson erikj at openjdk.org
Tue Sep 20 17:17:18 UTC 2022

On Tue, 20 Sep 2022 15:09:48 GMT, Magnus Ihse Bursie <ihse at openjdk.org> wrote:

>> A PullRequest exports two methods for getting all labels: `labels()` and `labelNames()`. The first returns `List<Label>` and the latter returns `List<String>`. The labelNames method has a default implementation that calls labels() and maps them to the name field. This is ok for most implementations, but not for GitLabMergeRequest. 
>> In GitLabMergeRequest, only the names of the labels are available from the merge_request REST object itself. In order to convert these to Label objects, it needs to make another REST call on the "project" (repository) to get the full label data. This extra call can add up to quite significant amounts of time when processing multiple PullRequest instances.
>> To make matters worse, the labels in GitLabMergeRequest are initialized in the constructor, so for every GitLabMergeRequest instance created, an extra REST call is made. There is also just a single use of PullRequest::labels() in the whole code base, and that is in the default implementation for labelNames(), so caller is ever interested in the full Label objects. This means that every instance of GitLabMergeRequest is making this extra REST call to initialize a cached set of data that is never used.
>> I'm solving this by only caching the label names instead of full Label objects. I've tried to retain the semantics of each method that deal with labels. The add/remove/set methods have been improved to update the cache instead of just clearing it.
> forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabMergeRequest.java line 666:
>> 664:                 // Avoid throwing NPE for unknown labels
>> 665:                 .filter(Objects::nonNull)
>> 666:                 .toList();
> You're losing `.sorted()`. Maybe it doesn't matter?

Hm, probably shouldn't matter, but I retained the sorted property on the `labels` field in the constructor, so should probably make sure all assignments to that field are made with sorted lists for consistency. Then we don't need to sort here.


PR: https://git.openjdk.org/skara/pull/1376

More information about the skara-dev mailing list