Limit comment scan on closed PRs

github-pullrequest-plugin/src/main/java/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEvent.java

@@ -2,14 +2,21 @@
 
 import com.github.kostyasha.github.integration.generic.GitHubPRDecisionContext;
 import hudson.Extension;
+import hudson.model.Job;
 import hudson.model.TaskListener;
+import hudson.scheduler.CronTab;
+import hudson.scheduler.CronTabList;
+import hudson.scheduler.Hash;
 import org.jenkinsci.Symbol;
 import org.jenkinsci.plugins.github.pullrequest.GitHubPRCause;
 import org.jenkinsci.plugins.github.pullrequest.GitHubPRPullRequest;
+import org.jenkinsci.plugins.github.pullrequest.GitHubPRTrigger;
+import org.jenkinsci.plugins.github.pullrequest.GitHubPRTriggerMode;
 import org.jenkinsci.plugins.github.pullrequest.events.GitHubPREvent;
 import org.jenkinsci.plugins.github.pullrequest.events.GitHubPREventDescriptor;
 import org.jenkinsci.plugins.github.pullrequest.restrictions.GitHubPRUserRestriction;
 import org.kohsuke.github.GHIssueComment;
+import org.kohsuke.github.GHIssueState;
 import org.kohsuke.github.GHPullRequest;
 import org.kohsuke.stapler.DataBoundConstructor;
 import org.slf4j.Logger;
@@ -18,6 +25,10 @@
 import edu.umd.cs.findbugs.annotations.NonNull;
 import java.io.IOException;
 import java.io.PrintStream;
+import java.util.ArrayList;
+import java.util.Calendar;
+import java.util.Date;
+import java.util.List;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -32,6 +43,7 @@
 public class GitHubPRCommentEvent extends GitHubPREvent {
     private static final String DISPLAY_NAME = "Comment matched to pattern";
     private static final Logger LOG = LoggerFactory.getLogger(GitHubPRCommentEvent.class);
+    private static final long CLOSED_PR_COMMENT_GRACE_MILLIS = 10 * 1000L;
 
     private String comment = "";
 
@@ -53,8 +65,18 @@ public GitHubPRCause check(@NonNull GitHubPRDecisionContext prDecisionContext) {
         final GitHubPRUserRestriction prUserRestriction = prDecisionContext.getPrUserRestriction();
 
         GitHubPRCause cause = null;
+        final boolean isClosedWithoutLocalState = isNull(localPR)
+                && GHIssueState.CLOSED.equals(remotePR.getState());
+        final Date closedPrCommentCutoff =
+                resolveClosedPrCommentCutoff(prDecisionContext, llog, isClosedWithoutLocalState);
         try {
             for (GHIssueComment issueComment : remotePR.getComments()) {
+                if (isClosedWithoutLocalState) {
+                    Date commentUpdatedAt = resolveCommentUpdatedAt(issueComment);
+                    if (isNull(commentUpdatedAt) || commentUpdatedAt.before(closedPrCommentCutoff)) {
+                        continue;
+                    }
+                }
                 if (isNull(localPR) // test all comments for trigger word even if we never saw PR before
                         || isNull(localPR.getLastCommentCreatedAt()) // PR was created but had no comments
                         // don't check comments that we saw before
@@ -81,6 +103,129 @@ public GitHubPRCause check(@NonNull GitHubPRDecisionContext prDecisionContext) {
         return cause;
     }
 
+    private Date resolveClosedPrCommentCutoff(GitHubPRDecisionContext prDecisionContext,
+                                              PrintStream llog,
+                                              boolean shouldResolve) {
+        if (!shouldResolve) {
+            return null;
+        }
+        long pollingIntervalMillis = resolvePollingIntervalMillis(prDecisionContext, llog);
+        long cutoffMillis = System.currentTimeMillis() - pollingIntervalMillis - CLOSED_PR_COMMENT_GRACE_MILLIS;
+        Date cutoff = new Date(cutoffMillis);
+        llog.println(DISPLAY_NAME + ": closed PR scan limited to comments updated since " + cutoff);
+        return cutoff;
+    }
+
+    private long resolvePollingIntervalMillis(GitHubPRDecisionContext prDecisionContext, PrintStream llog) {
+        GitHubPRTrigger trigger = prDecisionContext.getTrigger();
+        if (isNull(trigger)) {
+            return 0L;
+        }
+
+        GitHubPRTriggerMode triggerMode = trigger.getTriggerMode();
+        if (triggerMode == GitHubPRTriggerMode.HEAVY_HOOKS
+                || triggerMode == GitHubPRTriggerMode.LIGHT_HOOKS) {
+            return 0L;
+        }
+
+        String spec = trigger.getSpec();
+        if (isNull(spec) || spec.trim().isEmpty()) {
+            llog.println(DISPLAY_NAME + ": empty cron spec, using 0s polling window for closed PR scan");
+            return 0L;
+        }
+
+        Job<?, ?> job = trigger.getJob();
+        String seed = job == null || job.getFullName() == null ? "github-pullrequest-trigger" : job.getFullName();
+        List<CronTab> tabs = parseCronTabs(spec, Hash.from(seed), llog);
+        if (tabs.isEmpty()) {
+            llog.println(DISPLAY_NAME + ": no valid cron entries, using 0s polling window");
+            return 0L;
+        }
+        try {
+            Calendar next = nextScheduledAfter(tabs, System.currentTimeMillis() + 1000L);
+            if (next == null) {
+                llog.println(DISPLAY_NAME + ": unable to resolve cron interval, using 0s polling window");
+                return 0L;
+            }
+            Calendar nextAfter = nextScheduledAfter(tabs, next.getTimeInMillis() + 1000L);
+            if (nextAfter == null) {
+                llog.println(DISPLAY_NAME + ": unable to resolve cron interval, using 0s polling window");
+                return 0L;
+            }
+            long intervalMillis = nextAfter.getTimeInMillis() - next.getTimeInMillis();
+            if (intervalMillis <= 0L) {
+                llog.println(DISPLAY_NAME + ": non-positive cron interval, using 0s polling window");
+                return 0L;
+            }
+            return intervalMillis;
+        } catch (IllegalArgumentException e) {
+            LOG.warn("Invalid cron spec '{}' while resolving polling interval for closed PR scan", spec, e);
+            llog.println(DISPLAY_NAME + ": invalid cron spec, using 0s polling window");
+            return 0L;
+        }
+    }
+
+    private List<CronTab> parseCronTabs(String spec, Hash hash, PrintStream llog) {
+        List<CronTab> tabs = new ArrayList<>();
+        String timezone = null;
+        int lineNumber = 0;
+        for (String line : spec.split("\\r?\\n")) {
+            lineNumber++;
+            String trimmed = line.trim();
+            if (lineNumber == 1 && trimmed.startsWith("TZ=")) {
+                String tz = trimmed.replace("TZ=", "");
+                timezone = CronTabList.getValidTimezone(tz);
+                if (timezone == null) {
+                    llog.println(DISPLAY_NAME + ": invalid cron timezone, using 0s polling window");
+                    return new ArrayList<>();
+                }
+                continue;
+            }
+            if (trimmed.isEmpty() || trimmed.startsWith("#")) {
+                continue;
+            }
+            try {
+                tabs.add(new CronTab(trimmed, lineNumber, hash, timezone));
+            } catch (IllegalArgumentException e) {
+                LOG.warn("Invalid cron entry '{}' while resolving polling interval for closed PR scan", trimmed, e);
+                llog.println(DISPLAY_NAME + ": invalid cron entry, using 0s polling window");
+                return new ArrayList<>();
+            }
+        }
+        return tabs;
+    }
+
+    private static Calendar nextScheduledAfter(List<CronTab> tabs, long baseMillis) {
+        Calendar next = null;
+        for (CronTab tab : tabs) {
+            Calendar base = calendarFor(tab, baseMillis);
+            Calendar candidate = tab.ceil(base);
+            if (candidate == null) {
+                continue;
+            }
+            if (next == null || candidate.before(next)) {
+                next = candidate;
+            }
+        }
+        return next;
+    }
+
+    private static Calendar calendarFor(CronTab tab, long baseMillis) {
+        Calendar calendar = tab.getTimeZone() == null
+                ? Calendar.getInstance()
+                : Calendar.getInstance(tab.getTimeZone());
+        calendar.setTimeInMillis(baseMillis);
+        return calendar;
+    }
+
+    private static Date resolveCommentUpdatedAt(GHIssueComment issueComment) throws IOException {
+        Date updatedAt = issueComment.getUpdatedAt();
+        if (nonNull(updatedAt)) {
+            return updatedAt;
+        }
+        return issueComment.getCreatedAt();
+    }
+
     private GitHubPRCause checkComment(GitHubPRDecisionContext prDecisionContext,
                                        GHIssueComment issueComment,
                                        GitHubPRUserRestriction userRestriction,

github-pullrequest-plugin/src/main/resources/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEvent/help.html

@@ -1,4 +1,6 @@
 <div>
     Triggers when a comment matching the pattern is posted in a pull request.<br/>
+    For closed or merged pull requests without local state, only comments updated within the polling interval
+    (or 0 for hook-only triggers) plus 10 seconds are evaluated to avoid reprocessing historic comments.<br/>
     For example <b>Test ([A-Za-z0-9 ,!]+) tags please.</b>
 </div>

github-pullrequest-plugin/src/test/java/org/jenkinsci/plugins/github/pullrequest/events/impl/GitHubPRCommentEventTest.java

@@ -1,10 +1,15 @@
 package org.jenkinsci.plugins.github.pullrequest.events.impl;
 
+import hudson.model.Job;
 import hudson.model.TaskListener;
+import hudson.scheduler.CronTab;
+import hudson.scheduler.CronTabList;
+import hudson.scheduler.Hash;
 import org.jenkinsci.plugins.github.pullrequest.GitHubPRCause;
 import org.jenkinsci.plugins.github.pullrequest.GitHubPRLabel;
 import org.jenkinsci.plugins.github.pullrequest.GitHubPRPullRequest;
 import org.jenkinsci.plugins.github.pullrequest.GitHubPRTrigger;
+import org.jenkinsci.plugins.github.pullrequest.GitHubPRTriggerMode;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.kohsuke.github.GHCommitPointer;
@@ -21,7 +26,9 @@
 import java.io.IOException;
 import java.io.PrintStream;
 import java.util.ArrayList;
+import java.util.Calendar;
 import java.util.Date;
+import java.util.List;
 import java.util.Set;
 
 import static com.github.kostyasha.github.integration.generic.GitHubPRDecisionContext.newGitHubPRDecisionContext;
@@ -41,6 +48,7 @@
  */
 @RunWith(MockitoJUnitRunner.class)
 public class GitHubPRCommentEventTest {
+    private static final long CLOSED_PR_COMMENT_GRACE_MILLIS = 10 * 1000L;
 
     @Mock
     private GHPullRequest remotePr;
@@ -214,6 +222,104 @@ public void testNullLocalPR() throws IOException {
         assertNotNull(cause);
     }
 
+    @Test
+    public void testClosedPrSkipsHistoricComments() throws IOException {
+        commonExpectations(emptySet());
+        when(remotePr.getState()).thenReturn(GHIssueState.CLOSED);
+        when(trigger.getTriggerMode()).thenReturn(GitHubPRTriggerMode.HEAVY_HOOKS);
+
+        Date oldCommentDate = new Date(System.currentTimeMillis() - CLOSED_PR_COMMENT_GRACE_MILLIS - 20000L);
+        when(comment.getCreatedAt()).thenReturn(oldCommentDate);
+        when(comment.getUpdatedAt()).thenReturn(oldCommentDate);
+        when(comment.getBody()).thenReturn("test foo, bar tags please.");
+
+        final ArrayList<GHIssueComment> ghIssueComments = new ArrayList<>();
+        ghIssueComments.add(comment);
+        when(remotePr.getComments()).thenReturn(ghIssueComments);
+
+        GitHubPRCause cause = new GitHubPRCommentEvent("test ([A-Za-z0-9 ,!]+) tags please.")
+                .check(newGitHubPRDecisionContext()
+                        .withPrTrigger(trigger)
+                        .withRemotePR(remotePr)
+                        .withListener(listener)
+                        .build()
+                ); // localPR is null and PR is closed
+
+        assertNull(cause);
+    }
+
+    @Test
+    public void testClosedPrMatchesLatestCommentUpdate() throws IOException {
+        commonExpectations(emptySet());
+        causeCreationExpectations();
+        when(remotePr.getState()).thenReturn(GHIssueState.CLOSED);
+        when(trigger.getTriggerMode()).thenReturn(GitHubPRTriggerMode.HEAVY_HOOKS);
+
+        Date commentDate = new Date(System.currentTimeMillis() - 1000L);
+        when(comment.getCreatedAt()).thenReturn(new Date(System.currentTimeMillis() - CLOSED_PR_COMMENT_GRACE_MILLIS));
+        when(comment.getUpdatedAt()).thenReturn(commentDate);
+
+        final String body = "test foo, bar tags please.";
+        when(comment.getBody()).thenReturn(body);
+
+        final ArrayList<GHIssueComment> ghIssueComments = new ArrayList<>();
+        ghIssueComments.add(comment);
+        when(remotePr.getComments()).thenReturn(ghIssueComments);
+
+        GitHubPRCause cause = new GitHubPRCommentEvent("test ([A-Za-z0-9 ,!]+) tags please.")
+                .check(newGitHubPRDecisionContext()
+                        .withPrTrigger(trigger)
+                        .withRemotePR(remotePr)
+                        .withListener(listener)
+                        .build()
+                ); // localPR is null and PR is closed
+
+        assertThat(cause.getCommentAuthorName(), is("commentOwnerName"));
+        assertThat(cause.getCommentAuthorEmail(), is("commentOwner@email.com"));
+        assertThat(cause.getCommentBody(), is(body));
+        assertThat(cause.getCommentBodyMatch(), is("foo, bar"));
+        assertNotNull(cause);
+    }
+
+    @Test
+    public void testClosedPrCronWindowAcceptsRecentComment() throws IOException {
+        commonExpectations(emptySet());
+        causeCreationExpectations();
+        when(remotePr.getState()).thenReturn(GHIssueState.CLOSED);
+        when(trigger.getTriggerMode()).thenReturn(GitHubPRTriggerMode.CRON);
+        when(trigger.getSpec()).thenReturn("H/5 * * * *");
+
+        Job<?, ?> job = mock(Job.class);
+        when(job.getFullName()).thenReturn("pr-comment-cron-window");
+        when(trigger.getJob()).thenReturn(job);
+
+        long intervalMillis = computeCronIntervalMillis("H/5 * * * *", "pr-comment-cron-window");
+        Date recentCommentDate = new Date(System.currentTimeMillis() - intervalMillis + 2000L);
+        when(comment.getCreatedAt()).thenReturn(new Date(System.currentTimeMillis() - intervalMillis - 60000L));
+        when(comment.getUpdatedAt()).thenReturn(recentCommentDate);
+
+        final String body = "test foo, bar tags please.";
+        when(comment.getBody()).thenReturn(body);
+
+        final ArrayList<GHIssueComment> ghIssueComments = new ArrayList<>();
+        ghIssueComments.add(comment);
+        when(remotePr.getComments()).thenReturn(ghIssueComments);
+
+        GitHubPRCause cause = new GitHubPRCommentEvent("test ([A-Za-z0-9 ,!]+) tags please.")
+                .check(newGitHubPRDecisionContext()
+                        .withPrTrigger(trigger)
+                        .withRemotePR(remotePr)
+                        .withListener(listener)
+                        .build()
+                ); // localPR is null and PR is closed
+
+        assertThat(cause.getCommentAuthorName(), is("commentOwnerName"));
+        assertThat(cause.getCommentAuthorEmail(), is("commentOwner@email.com"));
+        assertThat(cause.getCommentBody(), is(body));
+        assertThat(cause.getCommentBodyMatch(), is("foo, bar"));
+        assertNotNull(cause);
+    }
+
     private void commonExpectations(Set<String> localLabels) throws IOException {
         when(labels.getLabelsSet()).thenReturn(localLabels);
         when(localPR.getLabels()).thenReturn(localLabels);
@@ -238,4 +344,57 @@ private void causeCreationExpectations() throws IOException {
         when(remotePr.getHead()).thenReturn(mockPointer);
         when(remotePr.getBase()).thenReturn(mockPointer);
     }
+
+    private long computeCronIntervalMillis(String spec, String seed) {
+        List<CronTab> tabs = parseCronTabs(spec, Hash.from(seed));
+        Calendar next = nextScheduledAfter(tabs, System.currentTimeMillis() + 1000L);
+        Calendar nextAfter = nextScheduledAfter(tabs, next.getTimeInMillis() + 1000L);
+        return nextAfter.getTimeInMillis() - next.getTimeInMillis();
+    }
+
+    private List<CronTab> parseCronTabs(String spec, Hash hash) {
+        List<CronTab> tabs = new ArrayList<>();
+        String timezone = null;
+        int lineNumber = 0;
+        for (String line : spec.split("\\r?\\n")) {
+            lineNumber++;
+            String trimmed = line.trim();
+            if (lineNumber == 1 && trimmed.startsWith("TZ=")) {
+                String tz = trimmed.replace("TZ=", "");
+                timezone = CronTabList.getValidTimezone(tz);
+                if (timezone == null) {
+                    throw new IllegalArgumentException("Invalid cron timezone");
+                }
+                continue;
+            }
+            if (trimmed.isEmpty() || trimmed.startsWith("#")) {
+                continue;
+            }
+            tabs.add(new CronTab(trimmed, lineNumber, hash, timezone));
+        }
+        return tabs;
+    }
+
+    private Calendar nextScheduledAfter(List<CronTab> tabs, long baseMillis) {
+        Calendar next = null;
+        for (CronTab tab : tabs) {
+            Calendar base = calendarFor(tab, baseMillis);
+            Calendar candidate = tab.ceil(base);
+            if (candidate == null) {
+                continue;
+            }
+            if (next == null || candidate.before(next)) {
+                next = candidate;
+            }
+        }
+        return next;
+    }
+
+    private Calendar calendarFor(CronTab tab, long baseMillis) {
+        Calendar calendar = tab.getTimeZone() == null
+                ? Calendar.getInstance()
+                : Calendar.getInstance(tab.getTimeZone());
+        calendar.setTimeInMillis(baseMillis);
+        return calendar;
+    }
 }