#540: Speedup checking issue state (#554)

exasol · Mar 19, 2024 · f1f164d · f1f164d
1 parent 997a142
commit f1f164d
Show file tree

Hide file tree

Showing 7 changed files with 65 additions and 53 deletions.
diff --git a/doc/changes/changes_4.2.1.md b/doc/changes/changes_4.2.1.md
@@ -10,6 +10,7 @@ Code name:
 * #542: Prefixed release letter on GitHub with version number
 * #545: Fix parsing of Go version numbers with a `v` prefix
 * #548: Skip release build when preconditions are not fulfilled
+* #540: Improved speed of validating mentioned issues in changes file
 
 ## Dependency Updates
 

diff --git a/project-keeper/error_code_config.yml b/project-keeper/error_code_config.yml
@@ -2,4 +2,4 @@ error-tags:
   PK-CORE:
     packages:
       - com.exasol.projectkeeper
-    highest-index: 190
+    highest-index: 192
diff --git a/...rc/main/java/com/exasol/projectkeeper/validators/release/ChangesFileReleaseValidator.java b/...rc/main/java/com/exasol/projectkeeper/validators/release/ChangesFileReleaseValidator.java
@@ -15,6 +15,7 @@
 import com.exasol.projectkeeper.validators.finding.SimpleValidationFinding;
 import com.exasol.projectkeeper.validators.finding.ValidationFinding;
 import com.exasol.projectkeeper.validators.release.github.GitHubAdapter;
+import com.exasol.projectkeeper.validators.release.github.IssueState;
 
 class ChangesFileReleaseValidator implements Validator {
     private static final ZoneId UTC_ZONE = ZoneId.of("UTC");
@@ -74,12 +75,14 @@ private List<ValidationFinding> validateIssuesClosed() {
     private List<Integer> getIssuesWronglyMarkedAsClosed() {
         final Set<Integer> mentionedTickets = changesFile.getFixedIssues().stream().map(FixedIssue::issueNumber)
                 .collect(toSet());
-        if (mentionedTickets.isEmpty()) {
-            return emptyList();
+        final Set<Integer> stillOpenIssues = new HashSet<>();
+        for (final Integer issue : mentionedTickets) {
+            final IssueState state = gitHubAdapter.getIssueState(issue);
+            if (state != IssueState.CLOSED) {
+                stillOpenIssues.add(issue);
+            }
         }
-        final Set<Integer> wrongIssues = new HashSet<>(mentionedTickets);
-        wrongIssues.removeAll(gitHubAdapter.getClosedIssues());
-        return sort(wrongIssues);
+        return sort(stillOpenIssues);
     }
 
     private List<Integer> sort(final Set<Integer> numbers) {

diff --git a/...eeper/src/main/java/com/exasol/projectkeeper/validators/release/github/GitHubAdapter.java b/...eeper/src/main/java/com/exasol/projectkeeper/validators/release/github/GitHubAdapter.java
@@ -1,19 +1,12 @@
 package com.exasol.projectkeeper.validators.release.github;
 
-import static java.util.stream.Collectors.toSet;
-
-import java.time.Duration;
-import java.time.Instant;
-import java.util.EnumMap;
-import java.util.Set;
+import java.io.IOException;
+import java.io.UncheckedIOException;
 import java.util.logging.Logger;
-import java.util.stream.StreamSupport;
 
+import com.exasol.errorreporting.ExaError;
 import com.jcabi.github.Coordinates;
 import com.jcabi.github.Issue;
-import com.jcabi.github.Issues.Qualifier;
-import com.jcabi.github.Issues.Sort;
-import com.jcabi.github.Search.Order;
 
 /**
  * This class allows executing queries against the GitHub API.
@@ -43,28 +36,33 @@ public static GitHubAdapter connect(final String repoName) {
     }
 
     /**
-     * Get the {@link Set} of closed issues for the given repository.
-     *
-     * @return issue numbers
+     * Get the state of an issue.
+     * 
+     * @param issueNumber issue number
+     * @return state of the issue
      */
     // [impl->dsn~verify-release-mode.verify-issues-closed~1]
-    public Set<Integer> getClosedIssues() {
-        LOG.fine(() -> "Fetching closed issues for repo " + owner + "/" + repoName + "...");
-        final Instant start = Instant.now();
-        final Set<Integer> issues = StreamSupport.stream(gitHubConnectionProvider.connect() //
-                .repos().get(new Coordinates.Simple(owner, repoName)) //
-                .issues().search(Sort.CREATED, Order.ASC, stateClosed()) //
-                .spliterator(), false) //
-                .map(Issue::number) //
-                .collect(toSet());
-        LOG.fine(() -> "Found " + issues.size() + " issues for repo " + owner + "/" + repoName + " in "
-                + Duration.between(start, Instant.now()));
-        return issues;
-    }
-
-    private EnumMap<Qualifier, String> stateClosed() {
-        final EnumMap<Qualifier, String> qualifiers = new EnumMap<>(Qualifier.class);
-        qualifiers.put(Qualifier.STATE, "closed");
-        return qualifiers;
+    public IssueState getIssueState(final int issueNumber) {
+        final Issue issue = gitHubConnectionProvider.connect().repos().get(new Coordinates.Simple(owner, repoName))
+                .issues().get(issueNumber);
+        final Issue.Smart smartIssue = new Issue.Smart(issue);
+        try {
+            if (!smartIssue.exists()) {
+                LOG.fine(() -> "GitHub issue #" + issueNumber + " does not exist.");
+                return IssueState.MISSING;
+            }
+            if (smartIssue.isOpen()) {
+                LOG.fine(() -> "GitHub issue #" + issueNumber + " is still open.");
+                return IssueState.OPEN;
+            } else {
+                LOG.fine(() -> "GitHub issue #" + issueNumber + " is closed.");
+                return IssueState.CLOSED;
+            }
+        } catch (final IOException exception) {
+            throw new UncheckedIOException(ExaError.messageBuilder("E-PK-CORE-192")
+                    .message("Failed to get status of GitHub issue #{{issue number}}: {{cause}}", issueNumber,
+                            exception.getMessage())
+                    .toString(), exception);
+        }
     }
 }
diff --git a/...t-keeper/src/main/java/com/exasol/projectkeeper/validators/release/github/IssueState.java b/...t-keeper/src/main/java/com/exasol/projectkeeper/validators/release/github/IssueState.java
@@ -0,0 +1,13 @@
+package com.exasol.projectkeeper.validators.release.github;
+
+/**
+ * This enum represents the state of a GitHub issue.
+ */
+public enum IssueState {
+    /** The issue is open. */
+    OPEN,
+    /** The issue is closed. */
+    CLOSED,
+    /** The issue does not exist. */
+    MISSING;
+}
diff --git a/...est/java/com/exasol/projectkeeper/validators/release/ChangesFileReleaseValidatorTest.java b/...est/java/com/exasol/projectkeeper/validators/release/ChangesFileReleaseValidatorTest.java
@@ -1,14 +1,12 @@
 package com.exasol.projectkeeper.validators.release;
 
-import static java.util.Collections.emptySet;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.*;
 import static org.mockito.Mockito.when;
 
 import java.nio.file.Path;
 import java.time.*;
 import java.util.List;
-import java.util.Set;
 
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
@@ -21,6 +19,7 @@
 import com.exasol.projectkeeper.validators.changesfile.ChangesFileSection;
 import com.exasol.projectkeeper.validators.finding.SimpleValidationFinding;
 import com.exasol.projectkeeper.validators.release.github.GitHubAdapter;
+import com.exasol.projectkeeper.validators.release.github.IssueState;
 
 @ExtendWith(MockitoExtension.class)
 class ChangesFileReleaseValidatorTest {
@@ -56,9 +55,10 @@ void closedIssuesNoIssuesMentioned() {
     }
 
     // [utest->dsn~verify-release-mode.verify-issues-closed~1]
-    @Test
-    void closedIssuesIssueNotClosed() {
-        when(gitHubAdapterMock.getClosedIssues()).thenReturn(emptySet());
+    @ParameterizedTest
+    @CsvSource({ "OPEN", "MISSING" })
+    void closedIssuesIssueNotClosed(final IssueState state) {
+        when(gitHubAdapterMock.getIssueState(42)).thenReturn(state);
         final List<String> findings = findings(ChangesFile.builder()
                 .addSection(ChangesFileSection.builder("## Bugfixes").addLine("- #42: Fixed a bug").build())
                 .releaseDate(TODAY));
@@ -68,7 +68,7 @@ void closedIssuesIssueNotClosed() {
 
     @Test
     void closedIssuesIssueClosed() {
-        when(gitHubAdapterMock.getClosedIssues()).thenReturn(Set.of(17, 42));
+        when(gitHubAdapterMock.getIssueState(42)).thenReturn(IssueState.CLOSED);
         final List<String> findings = findings(ChangesFile.builder()
                 .addSection(ChangesFileSection.builder("## Bugfixes").addLine("- #42: Fixed a bug").build())
                 .releaseDate(TODAY));

diff --git a/...per/src/test/java/com/exasol/projectkeeper/validators/release/github/GitHubAdapterIT.java b/...per/src/test/java/com/exasol/projectkeeper/validators/release/github/GitHubAdapterIT.java
@@ -1,14 +1,12 @@
 package com.exasol.projectkeeper.validators.release.github;
 
 import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.*;
-import static org.junit.jupiter.api.Assertions.assertAll;
+import static org.hamcrest.Matchers.equalTo;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
-import java.util.Set;
-
 import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
 
 class GitHubAdapterIT {
 
@@ -21,10 +19,9 @@ static void createAdapter() {
     }
 
     // [itest->dsn~verify-release-mode.verify-issues-closed~1]
-    @Test
-    void getClosedIssues() {
-        final Set<Integer> closedIssues = adapter.getClosedIssues();
-        assertAll(() -> assertThat(closedIssues, hasSize(allOf(greaterThan(520), lessThan(10000000)))),
-                () -> assertThat(closedIssues, allOf(hasItem(1), hasItem(2), hasItem(3))));
+    @ParameterizedTest
+    @CsvSource({ "1, CLOSED", "0, MISSING", "-1, MISSING" })
+    void getIssueState(final int issueNumber, final IssueState expectedState) {
+        assertThat(adapter.getIssueState(issueNumber), equalTo(expectedState));
     }
 }