RFR: 1385: Backport MR links to the wrong CSR request [v2]

[Erik Joelsson]

On Tue, 10 May 2022 14:25:05 GMT, Guoxiong Li <gli at openjdk.org> wrote:

>> jbs/src/main/java/org/openjdk/skara/jbs/IssueUtil.java line 45:
>> 
>>> 43:      * A "scratch" fixVersion is empty, "tbd.*", or "unknown".
>>> 44:      */
>>> 45:     public static Optional<Issue> findClosestIssue(List<Issue> issueList, JdkVersion fixVersion) {
>> 
>> I think this method belongs in the Backports class, close to Backports#findIssue. They do almost the same thing, so if we are to maintain two similar implementations of the version matching order, they should reside close to each other.
>> 
>> Could we move more of the common functionality to Backports. Something like `Backports.findCsr(Issue primary, JdkVersion jdkVersion)` that would encapsulate most of this repeated functionality?
>
>> I think this method belongs in the Backports class, close to Backports#findIssue. They do almost the same thing, so if we are to maintain two similar implementations of the version matching order, they should reside close to each other.
> 
> The `Backports` class mainly handles something about backport. But this method is only about issue instead of backport. 
> Actully, the method `Backports#findIssue` should delegate to a method such as `IssueUtil#findClosestIssue(List<Issue> issueList, JdkVersion fixVersion, boolean onlySearchMainFixVersion)` after getting the backport issues to find the closest issue by using the main fix version. Such refactor is good but I don't think it should be done at this patch. So I would like to keep the utilility method about issues at a seperate class so that we can do more refactor in the future. The class name `IssueUtil` may need to be adjusted to a better name.
> 
>> Could we move more of the common functionality to Backports. Something like Backports.findCsr(Issue primary, JdkVersion jdkVersion) that would encapsulate most of this repeated functionality?
> 
> I moved the method `findCsr` and `csrLink` to the class `IssueUtil`. Again, the class name `Backports` is not good to place such methods.

I see your point about having a static class with utility methods for issues, similar to the java.util.Collections or java.util.Arrays classes, and following that pattern, I think the name should be `Issues`. 

However, The `IssueUtil#findClosestIssue` method may look like it applies generally to issues, but in reality it does not. Looking for an issue with the best matching fixVersion is inherently tied to the concept of backports as we handle them in the OpenJDK development process. This is now expanded to finding the best matching CSR in a tree of backport issues, but it's still a tree of backports, and traversing a tree of backports should be handled by the Backports class. 

In your current patch `IssueUtils` calls into `Backports` to get all backports. If you truly want the separation between `IssueUtils` and `Backports`, then you can't really have `IssueUtils` call into `Backports`. If you would later refactor `Backports::findIssue` to call into `IssueUtils::findClosestIssue`, we would end up with both classes calling each other.

To me `Backports::findCsr` makes perfect sense, it's about finding the correct CSR in a tree of backports. `IssueUtils::findCsr` does not, as it's not clear to me that `IssueUtils` would have any knowledge about traversing backports.

-------------

PR: https://git.openjdk.java.net/skara/pull/1318