diff --git a/forge/src/main/java/org/openjdk/skara/forge/Forge.java b/forge/src/main/java/org/openjdk/skara/forge/Forge.java index 461959438..c8f46cf9f 100644 --- a/forge/src/main/java/org/openjdk/skara/forge/Forge.java +++ b/forge/src/main/java/org/openjdk/skara/forge/Forge.java @@ -22,6 +22,7 @@ */ package org.openjdk.skara.forge; +import java.time.Duration; import org.openjdk.skara.host.*; import org.openjdk.skara.json.JSONObject; import org.openjdk.skara.vcs.Hash; @@ -43,6 +44,18 @@ public interface Forge extends Host { Optional repository(String name); Optional search(Hash hash); + /** + * Some forges do not always update the "updated_at" fields of various objects + * when the object changes. This method returns a Duration indicating how long + * the shortest update interval is for the "updated_at" field. This is needed + * to be taken into account when running queries (typically by padding the + * timestamp by this duration to guarantee that no results are missed). The + * default returns 0 which means no special considerations are needed. + */ + default Duration minTimeStampUpdateInterval() { + return Duration.ZERO; + } + static Forge from(String name, URI uri, Credential credential, JSONObject configuration) { var factory = ForgeFactory.getForgeFactories().stream() .filter(f -> f.name().equals(name)) diff --git a/forge/src/main/java/org/openjdk/skara/forge/PullRequestPoller.java b/forge/src/main/java/org/openjdk/skara/forge/PullRequestPoller.java new file mode 100644 index 000000000..b9b0875cd --- /dev/null +++ b/forge/src/main/java/org/openjdk/skara/forge/PullRequestPoller.java @@ -0,0 +1,324 @@ +package org.openjdk.skara.forge; + +import java.time.Duration; +import java.time.Instant; +import java.time.ZonedDateTime; +import java.util.Comparator; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.logging.Logger; +import java.util.stream.Collectors; +import java.util.stream.Stream; +import org.openjdk.skara.issuetracker.Comment; +import org.openjdk.skara.issuetracker.Issue; +import org.openjdk.skara.vcs.Hash; + +/** + * A PullRequestPoller handles querying for new and updated pull requests. It + * guarantees that no pull request updates at the forge are missed and avoids + * returning the same update multiple times as much as possible. + *

+ * On the first call, all open PRs, and if configured, non-open PRs (up to a + * limit), are returned. After that only updated PRs should be included. + *

+ * After each call for updated pull requests, the result needs to be + * acknowledged once it has been processed by the caller. Failing to + * acknowledge makes the next call include everything from the last call + * again. This helps to avoid missing any updates due to errors. + *

+ * In addition to this, it's also possible to schedule PRs for retries with + * or without quarantine. A regular retry will not block the same PR if it + * gets returned by the regular query, but doing so will cancel the future + * retry. A quarantine type retry will completely block that PR until the + * quarantine is lifted. In both cases, the actual object returned will be + * the same one provided in the retry call, unless a newer instance has + * been returned by a query since then. + */ +public class PullRequestPoller { + + private static final Logger log = Logger.getLogger(PullRequestPoller.class.getName()); + + private static final Duration CLOSED_PR_AGE_LIMIT = Duration.ofDays(7); + + private final HostedRepository repository; + private final Duration queryPadding; + private final boolean includeClosed; + private final boolean trustUpdatedAt; + private final boolean checkComments; + private final boolean checkReviews; + + private record PullRequestRetry(PullRequest pr, Instant when) {} + private final Map retryMap = new HashMap<>(); + private final Map quarantineMap = new HashMap<>(); + + /** + * This record represents all the query results data needed to correctly figure + * out if future results have been updated or not. + */ + private record QueryResult(Map pullRequests, Map> comments, + Map> reviews, ZonedDateTime maxUpdatedAt, + Instant afterQuery, List result) {} + private QueryResult current; + private QueryResult prev; + + /** + * When enough time has past since the last time we actually received results + * padding the updatedAt query parameter is no longer needed. This is indicated + * using this boolean. + */ + private boolean paddingNeeded = true; + + public PullRequestPoller(HostedRepository repository, boolean includeClosed, boolean commentsRelevant, + boolean reviewsRelevant) { + this.repository = repository; + this.includeClosed = includeClosed; + queryPadding = repository.forge().minTimeStampUpdateInterval(); + if (!queryPadding.isZero()) { + trustUpdatedAt = false; + checkComments = commentsRelevant; + checkReviews = reviewsRelevant; + } else { + trustUpdatedAt = true; + checkComments = false; + checkReviews = false; + } + } + + /** + * The main API method. Call this go get updated PRs. When done processing the results + * call lastBatchHandled() to acknowledge that all the returned PRs have been handled + * and should not be included in the next call of this method. + */ + public List updatedPullRequests() { + var beforeQuery = Instant.now(); + List prs = queryPullRequests(); + + // If nothing was found. Update the paddingNeeded state if enough time + // has passed since last we found something. + if (prs.isEmpty()) { + if (prev != null && prev.afterQuery.isBefore(beforeQuery.minus(queryPadding))) { + paddingNeeded = false; + } + } else { + paddingNeeded = true; + } + var afterQuery = Instant.now(); + + // Convert the query result into a map + var pullRequestMap = prs.stream().collect(Collectors.toMap(PullRequest::id, pr -> pr)); + + // Find the max updatedAt value in the result set. Fall back on the previous + // value (happens if no results were returned), or null (if no results have + // been found at all so far). + var maxUpdatedAt = prs.stream() + .map(PullRequest::updatedAt) + .max(Comparator.naturalOrder()) + .orElseGet(() -> prev != null ? prev.maxUpdatedAt : null); + + // If checking comments, save the current state of comments for future + // comparisons. + var commentsMap = fetchComments(prs, maxUpdatedAt); + + // If checking reviews, save the current state of reviews for future + // comparisons. + var reviewsMap = fetchReviews(prs, maxUpdatedAt); + + // Filter the results + var filtered = prs.stream() + .filter(this::isUpdated) + .toList(); + + var withRetries = addRetries(filtered); + + var result = processQuarantined(withRetries); + + // Save the state of the current query results + current = new QueryResult(pullRequestMap, commentsMap, reviewsMap, maxUpdatedAt, afterQuery, result); + + log.info("Found " + result.size() + " updated pull requests for " + repository.name()); + return result; + } + + /** + * After calling getUpdatedPullRequests(), this method must be called to acknowledge + * that all the PRs returned have been handled. If not, the previous results will be + * included in the next call to getUpdatedPullRequests() again. + */ + public synchronized void lastBatchHandled() { + if (current != null) { + prev = current; + current = null; + // Remove any returned PRs from the retry/quarantine sets + prev.result.forEach(pr -> retryMap.remove(pr.id())); + prev.result.forEach(pr -> quarantineMap.remove(pr.id())); + } + } + + /** + * If handling of a pull request fails, call this to have it be included in the next + * update, regardless of if it was updated or not. + * @param pr PullRequest to retry + */ + public synchronized void retryPullRequest(PullRequest pr) { + retryPullRequest(pr, Instant.MIN); + } + + /** + * Schedules a pull request to be re-evaluated after a certain time, unless it is + * updated before that. + * @param pr PullRequest to retry + * @param at Time at which to process it + */ + public synchronized void retryPullRequest(PullRequest pr, Instant at) { + retryMap.put(pr.id(), new PullRequestRetry(pr, at)); + } + + /** + * Schedules a pull request to be retried after a quarantine period has passed. + * If a quarantined pull request is returned by a query, it will be removed from + * the result set until the quarantine time has passed. + * @param pr PullRequest to quarantine + * @param until Time at which the quarantine is lifted + */ + public synchronized void quarantinePullRequest(PullRequest pr, Instant until) { + quarantineMap.put(pr.id(), new PullRequestRetry(pr, until)); + } + + /** + * Queries the repository for pull requests. On the first round (or until any + * results have been received), get all pull requests. After that limit the + * results using the maxUpdatedAt value from the previous results. Use padding + * if needed to guarantee that we never miss an update. + */ + private List queryPullRequests() { + if (prev == null || prev.maxUpdatedAt == null) { + if (includeClosed) { + log.fine("Fetching all open and recent closed pull requests for " + repository.name()); + // We need to guarantee that all open PRs are always included in the first round. + // The pullRequests(ZonedDateTime) call has a size limit, so may leave some out. + // There may also be open PRs that haven't been updated since the closed age limit. + var openPrs = repository.openPullRequests(); + var allPrs = repository.pullRequestsAfter(ZonedDateTime.now().minus(CLOSED_PR_AGE_LIMIT)); + return Stream.concat(openPrs.stream(), allPrs.stream().filter(pr -> !pr.isOpen())).toList(); + } else { + log.fine("Fetching all open pull requests for " + repository.name()); + return repository.openPullRequests(); + } + } else { + var queryUpdatedAt = paddingNeeded ? prev.maxUpdatedAt.minus(queryPadding) : prev.maxUpdatedAt; + if (includeClosed) { + log.fine("Fetching open and closed pull requests updated after " + queryUpdatedAt + " for " + repository.name()); + return repository.pullRequestsAfter(queryUpdatedAt); + } else { + log.fine("Fetching open pull requests updated after " + queryUpdatedAt + " for " + repository.name()); + return repository.openPullRequestsAfter(queryUpdatedAt); + } + } + } + + private Map> fetchComments(List prs, ZonedDateTime maxUpdatedAt) { + if (checkComments) { + return prs.stream() + .filter(pr -> pr.updatedAt().isAfter(maxUpdatedAt.minus(queryPadding))) + .collect(Collectors.toMap(Issue::id, Issue::comments)); + } else { + return Map.of(); + } + } + + private Map> fetchReviews(List prs, ZonedDateTime maxUpdatedAt) { + if (checkReviews) { + return prs.stream() + .filter(pr -> pr.updatedAt().isAfter(maxUpdatedAt.minus(queryPadding))) + .collect(Collectors.toMap(Issue::id, PullRequest::reviews)); + } else { + return Map.of(); + } + } + + /** + * Evaluates if a PR has been updated since the previous query result. If we + * can trust updatedAt from the forge, it's a simple comparison, otherwise + * we need to compare the complete contents of the PR object, as well as + * comments and/or reviews as configured. + */ + private boolean isUpdated(PullRequest pr) { + if (prev == null) { + return true; + } + var prPrev = prev.pullRequests.get(pr.id()); + if (prPrev == null || pr.updatedAt().isAfter(prPrev.updatedAt())) { + return true; + } + if (!trustUpdatedAt) { + if (!pr.equals(prPrev)) { + return true; + } + if (checkComments && !pr.comments().equals(prev.comments.get(pr.id()))) { + return true; + } + if (checkReviews && !pr.reviews().equals(prev.reviews.get(pr.id()))) { + return true; + } + } + return false; + } + + /** + * Returns a list of all prs with retries added. + */ + private synchronized List addRetries(List prs) { + if (retryMap.isEmpty()) { + return prs; + } else { + // Find the retries that have passed their at time. + var now = Instant.now(); + var retries = retryMap.values().stream() + .filter(prRetry -> prRetry.when.isBefore(now)) + .filter(prRetry -> prs.stream().noneMatch(pr -> pr.id().equals(prRetry.pr.id()))) + .map(PullRequestRetry::pr) + .toList(); + if (retries.isEmpty()) { + return prs; + } else { + return Stream.concat(prs.stream(), retries.stream()).toList(); + } + } + } + + /** + * Returns a list of all prs with still quarantined prs removed and newly lifted + * prs added. + */ + private synchronized List processQuarantined(List prs) { + if (quarantineMap.isEmpty()) { + return prs; + } else { + var now = Instant.now(); + // Replace the PR instances in the quarantineMap with any freshly fetched PRs. + // By doing this, we will always return the most up-to-date version of the PR + // that we have seen so far. + prs.forEach(pr -> { + if (quarantineMap.containsKey(pr.id())) { + quarantineMap.put(pr.id(), new PullRequestRetry(pr, quarantineMap.get(pr.id()).when)); + } + }); + // Find all quarantined PRs that are now past the time + var pastQuarantine = quarantineMap.values().stream() + .filter(prRetry -> prRetry.when.isBefore(now)) + .filter(prRetry -> prs.stream().noneMatch(pr -> pr.id().equals(prRetry.pr.id()))) + .map(PullRequestRetry::pr) + .toList(); + // Find all still quarantined PRs + var stillQuarantined = quarantineMap.values().stream() + .filter(prRetry -> !prRetry.when.isBefore(now)) + .map(prRetry -> prRetry.pr.id()) + .collect(Collectors.toSet()); + return Stream.concat( + prs.stream().filter(pr -> !stillQuarantined.contains(pr.id())), + pastQuarantine.stream()) + .toList(); + } + } +} diff --git a/forge/src/main/java/org/openjdk/skara/forge/Review.java b/forge/src/main/java/org/openjdk/skara/forge/Review.java index 7d115ea56..719dda64d 100644 --- a/forge/src/main/java/org/openjdk/skara/forge/Review.java +++ b/forge/src/main/java/org/openjdk/skara/forge/Review.java @@ -22,6 +22,7 @@ */ package org.openjdk.skara.forge; +import java.util.Objects; import org.openjdk.skara.host.HostUser; import org.openjdk.skara.vcs.Hash; @@ -78,4 +79,26 @@ public enum Verdict { APPROVED, DISAPPROVED } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + Review review = (Review) o; + return id == review.id && + Objects.equals(createdAt, review.createdAt) && + Objects.equals(reviewer, review.reviewer) && + verdict == review.verdict && + Objects.equals(hash, review.hash) && + Objects.equals(body, review.body); + } + + @Override + public int hashCode() { + return Objects.hash(createdAt, reviewer, verdict, hash, id, body); + } } diff --git a/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabHost.java b/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabHost.java index 9a6f4e773..e93932a7e 100644 --- a/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabHost.java +++ b/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabHost.java @@ -22,6 +22,7 @@ */ package org.openjdk.skara.forge.gitlab; +import java.time.Duration; import org.openjdk.skara.forge.*; import org.openjdk.skara.host.*; import org.openjdk.skara.json.*; @@ -252,4 +253,9 @@ public String hostname() { URI getWebUri(String endpoint) { return URI.create(uri.toString() + endpoint); } + + @Override + public Duration minTimeStampUpdateInterval() { + return Duration.ofMinutes(1); + } } diff --git a/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabMergeRequest.java b/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabMergeRequest.java index 942dc30f6..5809624fd 100644 --- a/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabMergeRequest.java +++ b/forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabMergeRequest.java @@ -827,4 +827,25 @@ public Optional lastForcePushTime() { public Optional findIntegratedCommitHash() { return findIntegratedCommitHash(List.of(repository.forge().currentUser().id())); } + + /** + * Equality for a GitLabMergeRequest is based on the data snapshot retrieved + * when the instance was created. + */ + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + GitLabMergeRequest that = (GitLabMergeRequest) o; + return json.equals(that.json); + } + + @Override + public int hashCode() { + return Objects.hash(json); + } } diff --git a/forge/src/test/java/org/openjdk/skara/forge/PullRequestPollerTests.java b/forge/src/test/java/org/openjdk/skara/forge/PullRequestPollerTests.java new file mode 100644 index 000000000..669729028 --- /dev/null +++ b/forge/src/test/java/org/openjdk/skara/forge/PullRequestPollerTests.java @@ -0,0 +1,347 @@ +package org.openjdk.skara.forge; + +import java.io.IOException; +import java.time.Duration; +import java.time.Instant; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestInfo; +import org.openjdk.skara.issuetracker.Issue; +import org.openjdk.skara.test.HostCredentials; +import org.openjdk.skara.test.TestHost; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class PullRequestPollerTests { + + @Test + void onlyOpen(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo)) { + var repo = credentials.getHostedRepository(); + var prPoller = new PullRequestPoller(repo, false, true, true); + + // Create closed PR that should never be returned + var prClosed = credentials.createPullRequest(repo, null, null, "Foo"); + prClosed.setState(Issue.State.CLOSED); + var prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Create a PR and poll for it + var pr = credentials.createPullRequest(repo, null, null, "Foo"); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + + // Poll for it again without calling lastBatchHandled(), it should be returned again + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Poll for it again after calling lastBatchHandled(), it should not be returned + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Add a label and poll again. + pr.addLabel("foo"); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + } + } + + @Test + void includeClosed(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo)) { + var repo = credentials.getHostedRepository(); + var prPoller = new PullRequestPoller(repo, true, true, true); + + // Create a and an open closed PR, that should both be returned + var prClosed = credentials.createPullRequest(repo, null, null, "Foo"); + var prOpen = credentials.createPullRequest(repo, null, null, "Foo"); + prClosed.setState(Issue.State.CLOSED); + var prs = prPoller.updatedPullRequests(); + assertEquals(2, prs.size()); + + // Poll for it again without calling lastBatchHandled(), both should be returned again + prs = prPoller.updatedPullRequests(); + assertEquals(2, prs.size()); + prPoller.lastBatchHandled(); + + // Poll for it again after calling lastBatchHandled(), none should not be returned + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Add a label to the closed PR and poll again. + prClosed.addLabel("foo"); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + assertEquals(prClosed.id(), prs.get(0).id()); + prPoller.lastBatchHandled(); + + // Add a label to the open PR and poll again. + prOpen.addLabel("foo"); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + assertEquals(prOpen.id(), prs.get(0).id()); + prPoller.lastBatchHandled(); + } + } + + /** + * Tests polling with padding needed, with comments and reviews irrelevant. + * Uses a closed PR to cover that case in our of the queryPadding tests. + */ + @Test + void queryPaddingLabel(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo)) { + var repo = credentials.getHostedRepository(); + var forge = repo.forge(); + ((TestHost) forge).setMinTimeStampUpdateInterval(Duration.ofDays(1)); + var prPoller = new PullRequestPoller(repo, true, false, false); + + // Create a closed PR and poll for it + var pr = credentials.createPullRequest(repo, "master", "master", "Foo"); + pr.setState(Issue.State.CLOSED); + var prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Poll for it again + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Add a new label but make sure the updatedAt time was not updated. This should trigger an update. + var prevUpdatedAt = pr.updatedAt(); + pr.addLabel("foo"); + pr.store().setLastUpdate(prevUpdatedAt); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Add comment while keeping the updatedAt time unchanged. This should not trigger an update + prevUpdatedAt = pr.updatedAt(); + pr.addComment("foo"); + pr.store().setLastUpdate(prevUpdatedAt); + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Add review while keeping the updatedAt time unchanged. This should not trigger an update + prevUpdatedAt = pr.updatedAt(); + pr.addReview(Review.Verdict.APPROVED, "foo"); + pr.store().setLastUpdate(prevUpdatedAt); + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + } + } + + /** + * Tests polling with padding needed and creating/modifying comments + */ + @Test + void queryPaddingComment(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo)) { + var repo = credentials.getHostedRepository(); + var forge = repo.forge(); + ((TestHost) forge).setMinTimeStampUpdateInterval(Duration.ofDays(1)); + var prPoller = new PullRequestPoller(repo, true, true, false); + + // Create a PR and poll for it + var pr = credentials.createPullRequest(repo, "master", "master", "Foo"); + var prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Poll for it again + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Add a new comment but make sure the updatedAt time was not updated. This should trigger an update. + var prevUpdatedAt = pr.updatedAt(); + pr.addLabel("foo"); + pr.store().setLastUpdate(prevUpdatedAt); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Add comment while keeping updatedAt unchanged. This should trigger an update. + prevUpdatedAt = pr.updatedAt(); + pr.addComment("foo"); + pr.store().setLastUpdate(prevUpdatedAt); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Update comment while keeping updatedAt unchanged. This should trigger an update. + prevUpdatedAt = pr.updatedAt(); + pr.updateComment(pr.comments().get(0).id(), "bar"); + pr.store().setLastUpdate(prevUpdatedAt); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Add review while keeping updatedAt unchanged. This should not trigger an update. + prevUpdatedAt = pr.updatedAt(); + pr.addReview(Review.Verdict.APPROVED, "foo"); + pr.store().setLastUpdate(prevUpdatedAt); + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + } + } + + /** + * Tests polling with padding needed and creating/modifying reviews + */ + @Test + void queryPaddingReview(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo)) { + var repo = credentials.getHostedRepository(); + var forge = repo.forge(); + ((TestHost) forge).setMinTimeStampUpdateInterval(Duration.ofDays(1)); + var prPoller = new PullRequestPoller(repo, true, false, true); + + // Create a PR and poll for it + var pr = credentials.createPullRequest(repo, "master", "master", "Foo"); + var prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Poll for it again + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Add a label but make sure the updatedAt time was not updated. This should trigger an update. + var prevUpdatedAt = pr.updatedAt(); + pr.addLabel("foo"); + pr.store().setLastUpdate(prevUpdatedAt); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Add comment while keeping updatedAt unchanged. This should not trigger an update + prevUpdatedAt = pr.updatedAt(); + pr.addComment("foo"); + pr.store().setLastUpdate(prevUpdatedAt); + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Add review while keeping updatedAt unchanged. This should trigger an update + prevUpdatedAt = pr.updatedAt(); + pr.addReview(Review.Verdict.APPROVED, "foo"); + pr.store().setLastUpdate(prevUpdatedAt); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + } + } + + @Test + void retries(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo)) { + var repo = credentials.getHostedRepository(); + var prPoller = new PullRequestPoller(repo, false, true, true); + + // Create PR + var pr1 = credentials.createPullRequest(repo, null, null, "Foo"); + var prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Create another PR and mark the first PR for retry + var pr2 = credentials.createPullRequest(repo, null, null, "Foo"); + prPoller.retryPullRequest(pr1); + prs = prPoller.updatedPullRequests(); + assertEquals(2, prs.size()); + prPoller.lastBatchHandled(); + + // Poll again, nothing should not be returned + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Just mark a PR for retry + prPoller.retryPullRequest(pr2); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + + // Call again without calling .lastBatchHandled, the retry should be included again + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Mark a PR for retry far in the future, it should not be included + prPoller.retryPullRequest(pr2, Instant.now().plus(Duration.ofDays(1))); + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Update PR and add it as retry, only one copy should be returned + pr1.addLabel("foo"); + prPoller.retryPullRequest(pr1); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + } + } + + @Test + void quarantine(TestInfo testInfo) throws IOException { + try (var credentials = new HostCredentials(testInfo)) { + var repo = credentials.getHostedRepository(); + var prPoller = new PullRequestPoller(repo, false, false, false); + + // Create PR + var pr1 = credentials.createPullRequest(repo, null, null, "Foo"); + var prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + var returnedPr1 = prs.get(0); + prPoller.lastBatchHandled(); + + // Mark it for quarantine far in the future, it should not be returned + prPoller.quarantinePullRequest(returnedPr1, Instant.now().plus(Duration.ofDays(1))); + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Touch it, it should not be returned + pr1.addLabel("foo"); + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + + // Change the quarantine time to sometime in the past + prPoller.quarantinePullRequest(returnedPr1, Instant.now().minus(Duration.ofMinutes(1))); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + + // Call again without marking as handled + prPoller.quarantinePullRequest(pr1, Instant.now().minus(Duration.ofMinutes(1))); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + prPoller.lastBatchHandled(); + + // Quarantine to sometime in the past and touch it + prPoller.quarantinePullRequest(returnedPr1, Instant.now().minus(Duration.ofMinutes(1))); + pr1.addLabel("bar"); + prs = prPoller.updatedPullRequests(); + assertEquals(1, prs.size()); + // Check that the returned PR object is updated with the label + assertTrue(prs.get(0).labelNames().contains("bar")); + + // Add PR both for retry and quarantine in the future, quarantine should win + prPoller.retryPullRequest(pr1, Instant.now().plus(Duration.ofDays(1))); + prPoller.quarantinePullRequest(pr1, Instant.now().plus(Duration.ofDays(1))); + prs = prPoller.updatedPullRequests(); + assertEquals(0, prs.size()); + prPoller.lastBatchHandled(); + } + } +} diff --git a/issuetracker/src/main/java/org/openjdk/skara/issuetracker/Comment.java b/issuetracker/src/main/java/org/openjdk/skara/issuetracker/Comment.java index 966cc8870..5fdb80940 100644 --- a/issuetracker/src/main/java/org/openjdk/skara/issuetracker/Comment.java +++ b/issuetracker/src/main/java/org/openjdk/skara/issuetracker/Comment.java @@ -71,11 +71,11 @@ public boolean equals(Object o) { return false; } Comment comment = (Comment) o; - return id.equals(comment.id) && - body.equals(comment.body) && - author.equals(comment.author) && - createdAt.equals(comment.createdAt) && - updatedAt.equals(comment.updatedAt); + return Objects.equals(id, comment.id) && + Objects.equals(body, comment.body) && + Objects.equals(author, comment.author) && + Objects.equals(createdAt, comment.createdAt) && + Objects.equals(updatedAt, comment.updatedAt); } @Override diff --git a/json/src/main/java/org/openjdk/skara/json/JSONArray.java b/json/src/main/java/org/openjdk/skara/json/JSONArray.java index dd9717467..225a65b44 100644 --- a/json/src/main/java/org/openjdk/skara/json/JSONArray.java +++ b/json/src/main/java/org/openjdk/skara/json/JSONArray.java @@ -159,4 +159,21 @@ public Stream stream() { public Iterator iterator() { return values.iterator(); } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + JSONArray that = (JSONArray) o; + return values.equals(that.values); + } + + @Override + public int hashCode() { + return Objects.hash(values); + } } diff --git a/json/src/main/java/org/openjdk/skara/json/JSONBoolean.java b/json/src/main/java/org/openjdk/skara/json/JSONBoolean.java index 939de630c..ce9e0d3f4 100644 --- a/json/src/main/java/org/openjdk/skara/json/JSONBoolean.java +++ b/json/src/main/java/org/openjdk/skara/json/JSONBoolean.java @@ -22,6 +22,8 @@ */ package org.openjdk.skara.json; +import java.util.Objects; + public class JSONBoolean implements JSONValue { boolean value; @@ -43,4 +45,21 @@ public boolean asBoolean() { public String toString() { return value ? "true" : "false"; } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + JSONBoolean that = (JSONBoolean) o; + return value == that.value; + } + + @Override + public int hashCode() { + return Objects.hash(value); + } } diff --git a/json/src/main/java/org/openjdk/skara/json/JSONDecimal.java b/json/src/main/java/org/openjdk/skara/json/JSONDecimal.java index 32c648a46..82e44c564 100644 --- a/json/src/main/java/org/openjdk/skara/json/JSONDecimal.java +++ b/json/src/main/java/org/openjdk/skara/json/JSONDecimal.java @@ -22,6 +22,8 @@ */ package org.openjdk.skara.json; +import java.util.Objects; + public class JSONDecimal implements JSONValue { double value; @@ -43,4 +45,21 @@ public double asDouble() { public String toString() { return Double.toString(value); } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + JSONDecimal that = (JSONDecimal) o; + return Double.compare(that.value, value) == 0; + } + + @Override + public int hashCode() { + return Objects.hash(value); + } } diff --git a/json/src/main/java/org/openjdk/skara/json/JSONNull.java b/json/src/main/java/org/openjdk/skara/json/JSONNull.java index 6daa36c39..37d0fcd8b 100644 --- a/json/src/main/java/org/openjdk/skara/json/JSONNull.java +++ b/json/src/main/java/org/openjdk/skara/json/JSONNull.java @@ -23,7 +23,9 @@ package org.openjdk.skara.json; public class JSONNull implements JSONValue { - public JSONNull() { + static JSONNull instance = new JSONNull(); + + private JSONNull() { } @Override diff --git a/json/src/main/java/org/openjdk/skara/json/JSONNumber.java b/json/src/main/java/org/openjdk/skara/json/JSONNumber.java index 0e555d6fb..5ca0f83f1 100644 --- a/json/src/main/java/org/openjdk/skara/json/JSONNumber.java +++ b/json/src/main/java/org/openjdk/skara/json/JSONNumber.java @@ -22,6 +22,8 @@ */ package org.openjdk.skara.json; +import java.util.Objects; + class JSONNumber implements JSONValue { long value; @@ -57,4 +59,21 @@ public long asLong() { public String toString() { return Long.toString(value); } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + JSONNumber that = (JSONNumber) o; + return value == that.value; + } + + @Override + public int hashCode() { + return Objects.hash(value); + } } diff --git a/json/src/main/java/org/openjdk/skara/json/JSONObject.java b/json/src/main/java/org/openjdk/skara/json/JSONObject.java index d498ced2c..cde3ac799 100644 --- a/json/src/main/java/org/openjdk/skara/json/JSONObject.java +++ b/json/src/main/java/org/openjdk/skara/json/JSONObject.java @@ -148,4 +148,21 @@ public String toString() { builder.append("}"); return builder.toString(); } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + JSONObject that = (JSONObject) o; + return value.equals(that.value); + } + + @Override + public int hashCode() { + return Objects.hash(value); + } } diff --git a/json/src/main/java/org/openjdk/skara/json/JSONParser.java b/json/src/main/java/org/openjdk/skara/json/JSONParser.java index e5a2f9397..3fd787d25 100644 --- a/json/src/main/java/org/openjdk/skara/json/JSONParser.java +++ b/json/src/main/java/org/openjdk/skara/json/JSONParser.java @@ -285,7 +285,7 @@ public JSONNull parseNull() { expect('l'); expect('l'); advance(); - return new JSONNull(); + return JSONNull.instance; } public JSONObject parseObject() { diff --git a/json/src/main/java/org/openjdk/skara/json/JSONString.java b/json/src/main/java/org/openjdk/skara/json/JSONString.java index c03e199dc..6c2895393 100644 --- a/json/src/main/java/org/openjdk/skara/json/JSONString.java +++ b/json/src/main/java/org/openjdk/skara/json/JSONString.java @@ -22,6 +22,8 @@ */ package org.openjdk.skara.json; +import java.util.Objects; + class JSONString implements JSONValue { String value; @@ -81,4 +83,21 @@ public String toString() { builder.append("\""); return builder.toString(); } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + JSONString that = (JSONString) o; + return Objects.equals(value, that.value); + } + + @Override + public int hashCode() { + return Objects.hash(value); + } } diff --git a/json/src/main/java/org/openjdk/skara/json/JSONValue.java b/json/src/main/java/org/openjdk/skara/json/JSONValue.java index 89b18c3fd..2146520c9 100644 --- a/json/src/main/java/org/openjdk/skara/json/JSONValue.java +++ b/json/src/main/java/org/openjdk/skara/json/JSONValue.java @@ -131,6 +131,6 @@ static JSONValue from(String s) { } static JSONValue fromNull() { - return new JSONNull(); + return JSONNull.instance; } } diff --git a/test/src/main/java/org/openjdk/skara/test/TestHost.java b/test/src/main/java/org/openjdk/skara/test/TestHost.java index f391daf67..ad36fb424 100644 --- a/test/src/main/java/org/openjdk/skara/test/TestHost.java +++ b/test/src/main/java/org/openjdk/skara/test/TestHost.java @@ -54,6 +54,10 @@ public class TestHost implements Forge, IssueTracker { private final Logger log = Logger.getLogger("org.openjdk.skara.test"); // Setting this field doesn't change the behavior of the TestHost, but it changes // what the associated method returns, which triggers different code paths in + // dependent code for testing. + private Duration minTimeStampUpdateInterval = Duration.ZERO; + // Setting this field doesn't change the behavior of the TestHost, but it changes + // what the associated method returns, which triggers different code paths in // dependent test code. private Duration timeStampQueryPrecision = Duration.ZERO; @@ -274,6 +278,15 @@ Optional getLastUpdatedIssue(TestIssueProject issueProject) { .max(Comparator.comparing(TestIssue::updatedAt)); } + public void setMinTimeStampUpdateInterval(Duration minTimeStampUpdateInterval) { + this.minTimeStampUpdateInterval = minTimeStampUpdateInterval; + } + + @Override + public Duration minTimeStampUpdateInterval() { + return minTimeStampUpdateInterval; + } + public void setTimeStampQueryPrecision(Duration timeStampQueryPrecision) { this.timeStampQueryPrecision = timeStampQueryPrecision; }