Check if commits have already been part of an approved pull request

[Michael T.]

Hi,

For a Stash hook I want to check if the commits in a pull request have already been approved as part of another pull request. I have an draft implementation below but my question is if there isn't a more effective way than going through all merged pull requests in a repository to achieve this.

Thanks in advance,

Michael

public boolean hasChangeAlreadyBeenApprovedElsewhere(RefChange change) {
  Set<Changeset> changesToMerge = getChangesetsBetween(repository, change.getFromHash(), change.getToHash());
  for (PullRequest pr: findAlreadyMergedPullRequests(repository)) {
    Set<Changeset> csOfMergedPullRequest = getChangesetsOfPullRequest(repository, pr);
    if (changesToMerge.subsetOf(csOfMergedPullRequest)) {
      return true;
    }
  }
  return false;
}
 
private List<PullRequest> findAlreadyMergedPullRequests(repository: Repository) {
      return pullRequestService.search(new PullRequestSearchRequest.Builder()
        .state(PullRequestState.MERGED)
        .fromRepositoryId(repository.getId())
        .build(), new PageRequestImpl(0, 100); //FIXME: handle paging properly
}
 

private Set<Changeset> getChangesetsBetween(Repository repository, String fromHash, String toHash {
      return toSet(commitService.getChangesetsBetween(new ChangesetsBetweenRequest.Builder(repository)
        .exclude(fromHash)
        .include(toHash)
        .build(), new PageRequestImpl(0, 100)); //FIXME: handle paging properly
}
 
 
private Set<Changeset> getChangesetsOfPullRequest(Repository repository, PullRequest pullRequest) {
    return toSet(pullRequestService.getChangesets(repository.getId(), pullRequest.getId(), new PageRequestImpl(0, 100)); //FIXME: handle paging properly
}

[Michael Heemskerk · Accepted solution]

Hi Michael,

As you've probably already discovered, your draft implementation requires quite a bit of processing to discover whether a commit has already been reviewed. 

When you think about it, there's a lot of repetitive processing that you're doing here. Every time the hook is triggered, you spin through all merged pull requests and all commits contained in them. An alternative is to record that information once and then just retrieve the information when you need it in your hook.

You could:

• Add an @EventListener method for the PullRequestMergedEvent and then retrieve the commits that have been merged in (I'd probably use CommitService.streamCommitsBetween to avoid the paging. You only need to record the hash anyway). Then you can record that these commits have been 'approved' using CommitIndex.addProperty(<commit-hash>, "approved-in", pullRequest.getToRef().getRepository().getId() + ":" + pullRequest.getId())
• In your hook, you retrieve the "approved-in" property for the commits that you're interested in, using CommitIndex.getProperties. 
• You'll have to loop through all merged pull requests once to prime the CommitIndex, but after that your hook should run pretty fast.

[Michael T.]

Hi Michael, thanks, that worked greatly! The only thing I had to make sure is that I only add properties to commits that are already part of the commit index (commitIndex.getChangeset(changeset.getId) != null). Otherwise, merge commits from the pull request merge caused a NullPointerException in HibernateIndexedChagesetDao.addProperty.

[Michael T.]

Hi, I was a bit too fast saying that everything works. Adding the properties to the index didn't result in an exception, but it had no effect either when I have done it in the PullRequestMergedEvent handler (after commitIndex.addProperty I was not able to read it out again via commitIndex.getProperties). What worked fine though was the same processing in a scheduled job (com.atlassian.scheduler) which I use to loop through all merged pull requests initially at plug-in start. Do you have any idea why it matters if I do this processing in a event handler compared to at plug-in start in a scheduled job?

[Michael Heemskerk]

If I had to guess, I think it could be related to transaction management. Are you managing transactions in the event handler?

[Michael T.]

No, I'm not. Are you referring to ActiveObjects transactions? Something like ao.executeInTransaction(new TransactionCallback<Void>() // (1) { @Override public Void doInTransaction() { // addProperty ?

[Michael Heemskerk]

The ActiveObjects tx stuff should only be used around AO persistence operations. You'll want to use SAL's TransactionTemplate (com.atlassian.sal.api.transaction.TransactionTemplate). The API is very similar. You can always use SAL's transaction template, even when you use AO.

[Michael T.]

Running the code in the PullRequestMergedEvent handler inside a transaction template did the trick. thanks a lot!

[Michael T.]

Well, again, I was too early with reporting success. While the trick with a SAL transaction template made it work in Stash 3.0.0 (my lower bound), the same code failed to add properties with ChangesetIndex in Stash 3.7.0...

[Michael Heemskerk]

Hmmm... that is strange, the transaction management shouldn't have changed. Any chance you can share your plugin (e.g. on Bitbucket) so I can have a look and reproduce what's going on? My username on Bitbucket is mheemskerk.

[Michael T.]

Thanks for your offer. I was able to solve this issue now. The reason for my problems were that under Stash 3.7.0, PullRequestMergedEvent was not asynchronous anymore (which has been changed back for Stash 3.7.2 upwards) and therefore I was not able to add properties with the commit indexer as I was in a read-only transaction of the event handler. I've changed my code now so that it executes in an own thread and SAL transaction which did the trick finally.

[Michael Heemskerk]

I've checked the sources, but the @AsynchronousPreferred has not been added or removed from PullRequestMergedEvent (since 2012). What lead you to you conclude that the event wasn't asynchronous anymore?