From 99d3f6d45cb3ddfcb113f312947091a028037670 Mon Sep 17 00:00:00 2001 From: Taylor Jones Date: Wed, 3 Jan 2018 20:38:38 -0500 Subject: [PATCH] Optimizing PR search requests (#53) * Using larger page size for open PR searches * Using PR commit stream instead of list --- .../plugins/PullRequestListener.java | 45 ++++++++++++------- .../plugins/PullRequestListenerTest.java | 18 +++++++- 2 files changed, 46 insertions(+), 17 deletions(-) diff --git a/src/main/java/com/monitorjbl/plugins/PullRequestListener.java b/src/main/java/com/monitorjbl/plugins/PullRequestListener.java index 214eb5a..629c358 100644 --- a/src/main/java/com/monitorjbl/plugins/PullRequestListener.java +++ b/src/main/java/com/monitorjbl/plugins/PullRequestListener.java @@ -1,7 +1,6 @@ package com.monitorjbl.plugins; import com.atlassian.bitbucket.build.BuildStatusSetEvent; -import com.atlassian.bitbucket.commit.Commit; import com.atlassian.bitbucket.event.pull.PullRequestParticipantStatusUpdatedEvent; import com.atlassian.bitbucket.permission.Permission; import com.atlassian.bitbucket.pull.PullRequest; @@ -12,6 +11,7 @@ import com.atlassian.bitbucket.repository.Repository; import com.atlassian.bitbucket.user.SecurityService; import com.atlassian.bitbucket.util.Page; +import com.atlassian.bitbucket.util.PageRequest; import com.atlassian.bitbucket.util.PageRequestImpl; import com.atlassian.event.api.EventListener; import com.monitorjbl.plugins.AsyncProcessor.TaskProcessor; @@ -19,11 +19,13 @@ import com.monitorjbl.plugins.config.ConfigDao; import java.io.Serializable; +import java.util.concurrent.atomic.AtomicBoolean; public class PullRequestListener { private static final String PR_APPROVE_BUCKET = "AUTOMERGE_PR_APPROVAL"; private static final String BUILD_APPROVE_BUCKET = "AUTOMERGE_BUILD_APPROVAL"; public static final int MAX_COMMITS = 1048576; + public static final int SEARCH_PAGE_SIZE = 50; private final AsyncProcessor asyncProcessor; private final ConfigDao configDao; @@ -69,21 +71,33 @@ void automergePullRequest(PullRequest pr) { } PullRequest findPRByCommitId(String commitId) { - int start = 0; - Page requests = null; - while(requests == null || requests.getSize() > 0) { - requests = prService.search(new PullRequestSearchRequest.Builder() - .state(PullRequestState.OPEN) - .build(), new PageRequestImpl(start, 10)); - for(PullRequest pr : requests.getValues()) { - Page commits = prService.getCommits(pr.getToRef().getRepository().getId(), pr.getId(), new PageRequestImpl(0, MAX_COMMITS)); - for(Commit c : commits.getValues()) { - if(c.getId().equals(commitId)) { - return pr; - } - } + PullRequestSearchRequest request = new PullRequestSearchRequest.Builder() + .state(PullRequestState.OPEN) + .withProperties(false) + .build(); + PageRequest nextPage = new PageRequestImpl(0, SEARCH_PAGE_SIZE); + do { + Page page = prService.search(request, nextPage); + PullRequest pr = searchForPR(page, commitId); + if(pr != null) { + return pr; + } else { + nextPage = page.getNextPageRequest(); + } + } while(nextPage != null); + return null; + } + + private PullRequest searchForPR(Page requests, String commitId) { + for(PullRequest pr : requests.getValues()) { + AtomicBoolean found = new AtomicBoolean(false); + prService.streamCommits(pr.getToRef().getRepository().getId(), pr.getId(), commit -> { + found.set(commit.getId().equals(commitId)); + return !found.get(); + }); + if(found.get()) { + return pr; } - start += 10; } return null; } @@ -130,4 +144,5 @@ public ApprovalTask(Long pullRequestId, Integer repositoryId, String commitId) { this.commitId = commitId; } } + } diff --git a/src/test/java/ut/com/monitorjbl/plugins/PullRequestListenerTest.java b/src/test/java/ut/com/monitorjbl/plugins/PullRequestListenerTest.java index ae1ae95..beefbb5 100644 --- a/src/test/java/ut/com/monitorjbl/plugins/PullRequestListenerTest.java +++ b/src/test/java/ut/com/monitorjbl/plugins/PullRequestListenerTest.java @@ -6,9 +6,11 @@ import com.atlassian.bitbucket.pull.PullRequest; import com.atlassian.bitbucket.pull.PullRequestMergeRequest; import com.atlassian.bitbucket.pull.PullRequestMergeability; +import com.atlassian.bitbucket.pull.PullRequestParticipant; import com.atlassian.bitbucket.pull.PullRequestRef; import com.atlassian.bitbucket.pull.PullRequestService; import com.atlassian.bitbucket.repository.Repository; +import com.atlassian.bitbucket.user.ApplicationUser; import com.atlassian.bitbucket.user.EscalatedSecurityContext; import com.atlassian.bitbucket.user.SecurityService; import com.atlassian.bitbucket.util.Operation; @@ -16,7 +18,6 @@ import com.monitorjbl.plugins.config.Config; import com.monitorjbl.plugins.config.ConfigDao; import org.junit.Before; -import org.junit.Ignore; import org.junit.Test; import org.mockito.InjectMocks; import org.mockito.Mock; @@ -35,7 +36,6 @@ import static org.mockito.Mockito.when; @SuppressWarnings("unchecked") -@Ignore public class PullRequestListenerTest { @Mock private ConfigDao configDao; @@ -66,16 +66,25 @@ public class PullRequestListenerTest { PullRequestRef fromRef; @Mock PullRequestMergeability mergeability; + @Mock + PullRequestParticipant author; + @Mock + ApplicationUser authorUser; + @Mock + EscalatedSecurityContext securityContext; @Before public void setUp() throws Exception { MockitoAnnotations.initMocks(this); when(securityService.withPermission(any(Permission.class), anyString())).thenReturn(new MockSecurityContext()); when(openedEvent.getPullRequest()).thenReturn(pr); + when(authorUser.getSlug()).thenReturn("someguy"); + when(author.getUser()).thenReturn(authorUser); when(pr.getToRef()).thenReturn(toRef); when(pr.getFromRef()).thenReturn(fromRef); when(pr.getId()).thenReturn(10L); when(pr.getVersion()).thenReturn(10384); + when(pr.getAuthor()).thenReturn(author); when(toRef.getRepository()).thenReturn(toRepo); when(toRef.getId()).thenReturn(RegexUtils.REFS_PREFIX + "master"); when(fromRef.getRepository()).thenReturn(fromRepo); @@ -118,6 +127,11 @@ public void testAutomerge_canMerge() throws Exception { .requiredReviewers(newArrayList("user1")) .requiredReviews(1) .build()); + when(securityService.impersonating(any(), any())).thenReturn(securityContext); + when(securityContext.call(any())).then(inv -> { + ((Operation) inv.getArguments()[0]).perform(); + return null; + }); sut.automergePullRequest(pr); verify(prService, times(1)).merge(any(PullRequestMergeRequest.class)); }