Clean up exception handling in StashApiClient

Stop using RuntimeException, it's an unchecked exception. The callers may
be unaware that they need to handle it.

Stop using Exception. Always use most specific exception types.

Always rethrow caught exceptions by wrapping them in StashApiClient. Let
the callers handle them, as they are in a better position to chose what
to do and emit meaningful logs.

Don't print stack trace in StashApiClient. Print it when the exception is
handled. Some callers log the stack trace to the build log, not to the
global Jenkins log.

Don't log error messages and throw StashApiException at the same time,
unless the log message contains information that is not added to the
exception.

Adjust "throws" declarations. Catch errors where appropriate and log
them.

Document functions that throw StashApiException. Add comments explaining
how StashApiException is handled.

Adjust unit tests for the new behavior.

Author: proski

src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashBuildListener.java

@@ -1,5 +1,7 @@
 package stashpullrequestbuilder.stashpullrequestbuilder;
 
+import static java.lang.String.format;
+
 import hudson.Extension;
 import hudson.Util;
 import hudson.model.AbstractBuild;
@@ -10,11 +12,11 @@
 import java.io.IOException;
 import java.io.PrintStream;
 import java.lang.invoke.MethodHandles;
-import java.util.logging.Level;
 import java.util.logging.Logger;
 import javax.annotation.Nonnull;
 import jenkins.model.JenkinsLocationConfiguration;
 import jenkins.model.ParameterizedJobMixIn;
+import stashpullrequestbuilder.stashpullrequestbuilder.stash.StashApiClient.StashApiException;
 
 /** Created by Nathan McCarthy */
 @Extension
@@ -42,6 +44,8 @@ public void onStarted(Run<?, ?> run, TaskListener listener) {
 
   @Override
   public void onCompleted(Run<?, ?> run, @Nonnull TaskListener listener) {
+    PrintStream buildLogger = listener.getLogger();
+
     StashCause cause = run.getCause(StashCause.class);
     if (cause == null) {
       return;
@@ -65,7 +69,19 @@ public void onCompleted(Run<?, ?> run, @Nonnull TaskListener listener) {
     } else {
       buildUrl = rootUrl + run.getUrl();
     }
-    repository.deletePullRequestComment(cause.getPullRequestId(), cause.getBuildStartCommentId());
+
+    // Delete the "Build Started" comment, as it gets replaced with a comment
+    // about the build result. Failure to delete the comment is not fatal, it's
+    // reported to the build log.
+    try {
+      repository.deletePullRequestComment(cause.getPullRequestId(), cause.getBuildStartCommentId());
+    } catch (StashApiException e) {
+      buildLogger.println(
+          format(
+              "%s: cannot delete Build Start comment for pull request %s",
+              run.getParent().getDisplayName(), cause.getPullRequestId()));
+      e.printStackTrace(buildLogger);
+    }
 
     String additionalComment = "";
     StashPostBuildComment comments = null;
@@ -103,31 +119,30 @@ public void onCompleted(Run<?, ?> run, @Nonnull TaskListener listener) {
         additionalComment,
         duration);
 
-    // Merge PR
+    // Request the server to merge the pull request on success if that option is
+    // enabled. Log the result to the build log. Handle exceptions here, report
+    // them to the build log as well.
     if (trigger.getMergeOnSuccess() && run.getResult() == Result.SUCCESS) {
-      boolean mergeStat =
-          repository.mergePullRequest(cause.getPullRequestId(), cause.getPullRequestVersion());
-      if (mergeStat == true) {
-        String logmsg =
-            "Merged pull request "
-                + cause.getPullRequestId()
-                + "("
-                + cause.getSourceBranch()
-                + ") to branch "
-                + cause.getTargetBranch();
-        logger.log(Level.INFO, logmsg);
-        listener.getLogger().println(logmsg);
-      } else {
-        String logmsg =
-            "Failed to merge pull request "
-                + cause.getPullRequestId()
-                + "("
-                + cause.getSourceBranch()
-                + ") to branch "
-                + cause.getTargetBranch()
-                + " because it's out of date";
-        logger.log(Level.INFO, logmsg);
-        listener.getLogger().println(logmsg);
+      try {
+        boolean mergeStat =
+            repository.mergePullRequest(cause.getPullRequestId(), cause.getPullRequestVersion());
+        if (mergeStat == true) {
+          buildLogger.println(
+              format(
+                  "Successfully merged pull request %s (%s) to branch %s",
+                  cause.getPullRequestId(), cause.getSourceBranch(), cause.getTargetBranch()));
+        } else {
+          buildLogger.println(
+              format(
+                  "Failed to merge pull request %s (%s) to branch %s, it may be out of date",
+                  cause.getPullRequestId(), cause.getSourceBranch(), cause.getTargetBranch()));
+        }
+      } catch (StashApiException e) {
+        buildLogger.println(
+            format(
+                "Failed to merge pull request %s (%s) to branch %s",
+                cause.getPullRequestId(), cause.getSourceBranch(), cause.getTargetBranch()));
+        e.printStackTrace(buildLogger);
       }
     }
   }

src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/StashRepository.java

@@ -94,6 +94,9 @@ public Collection<StashPullRequestResponseValue> getTargetPullRequests() {
     List<StashPullRequestResponseValue> targetPullRequests =
         new ArrayList<StashPullRequestResponseValue>();
 
+    // Fetch "OPEN" pull requests from the server. Failure to get the list will
+    // prevent builds from being scheduled. However, the call will be retried
+    // during the next cycle, as determined by the cron settings.
     List<StashPullRequestResponseValue> pullRequests;
     try {
       pullRequests = client.getPullRequests();
@@ -110,7 +113,15 @@ public Collection<StashPullRequestResponseValue> getTargetPullRequests() {
     return targetPullRequests;
   }
 
-  public String postBuildStartCommentTo(StashPullRequestResponseValue pullRequest) {
+  /**
+   * Post "BuildStarted" comment to Bitbucket Server
+   *
+   * @param pullRequest pull request
+   * @return comment ID
+   * @throws StashApiException if posting the comment fails
+   */
+  public String postBuildStartCommentTo(StashPullRequestResponseValue pullRequest)
+      throws StashApiException {
     String sourceCommit = pullRequest.getFromRef().getLatestCommit();
     String destinationCommit = pullRequest.getToRef().getLatestCommit();
     String comment =
@@ -269,6 +280,9 @@ public void addFutureBuildTasks(Collection<StashPullRequestResponseValue> pullRe
     for (StashPullRequestResponseValue pullRequest : pullRequests) {
       Map<String, String> additionalParameters;
 
+      // Get parameter values for the build from the pull request comments.
+      // Failure to do so causes the build to be skipped, as we would not run
+      // the build with incorrect parameters.
       try {
         additionalParameters = getAdditionalParameters(pullRequest);
       } catch (StashApiException e) {
@@ -281,6 +295,8 @@ public void addFutureBuildTasks(Collection<StashPullRequestResponseValue> pullRe
         continue;
       }
 
+      // Delete comments about previous build results, if that option is
+      // enabled. Run the build even if those comments cannot be deleted.
       if (trigger.getDeletePreviousBuildFinishComments()) {
         try {
           deletePreviousBuildFinishedComments(pullRequest);
@@ -294,7 +310,25 @@ public void addFutureBuildTasks(Collection<StashPullRequestResponseValue> pullRe
         }
       }
 
-      String commentId = postBuildStartCommentTo(pullRequest);
+      // Post a comment indicating the build start. Strictly speaking, we are
+      // just adding the build to the queue, it will start after the quiet time
+      // expires and there are executors available. Failure to post the comment
+      // prevents the build for safety reasons. If the plugin cannot post this
+      // comment, chances are it won't be able to post the build results, which
+      // would trigger the build again and again, wasting Jenkins resources.
+      String commentId;
+      try {
+        commentId = postBuildStartCommentTo(pullRequest);
+      } catch (StashApiException e) {
+        logger.log(
+            Level.INFO,
+            format(
+                "%s: cannot post Build Start comment for pull request %s, not building",
+                job.getName(), pullRequest.getId()),
+            e);
+        continue;
+      }
+
       StashCause cause =
           new StashCause(
               trigger.getStashHost(),
@@ -315,7 +349,15 @@ public void addFutureBuildTasks(Collection<StashPullRequestResponseValue> pullRe
     }
   }
 
-  public void deletePullRequestComment(String pullRequestId, String commentId) {
+  /**
+   * Deletes pull request comment from Bitbucket Server
+   *
+   * @param pullRequestId pull request ID
+   * @param commentId comment to be deleted
+   * @throws StashApiException if deleting the comment fails
+   */
+  public void deletePullRequestComment(String pullRequestId, String commentId)
+      throws StashApiException {
     this.client.deletePullRequestComment(pullRequestId, commentId);
   }
 
@@ -359,14 +401,41 @@ public void postFinishedComment(
 
     comment = comment.concat(additionalComment);
 
-    this.client.postPullRequestComment(pullRequestId, comment);
+    // Post the "Build Finished" comment. Failure to post it can lead to
+    // scheduling another build for the pull request unnecessarily.
+    try {
+      this.client.postPullRequestComment(pullRequestId, comment);
+    } catch (StashApiException e) {
+      logger.log(
+          Level.WARNING,
+          format(
+              "%s: cannot post Build Finished comment for pull request %s",
+              job.getDisplayName(), pullRequestId),
+          e);
+    }
   }
 
-  public boolean mergePullRequest(String pullRequestId, String version) {
+  /**
+   * Instructs Bitbucket Server to merge pull request
+   *
+   * @param pullRequestId pull request ID
+   * @param version pull request version
+   * @return true if the merge succeeds, false if the server reports an error
+   * @throws StashApiException if cannot communicate to the server
+   */
+  public boolean mergePullRequest(String pullRequestId, String version) throws StashApiException {
     return this.client.mergePullRequest(pullRequestId, version);
   }
 
-  private boolean isPullRequestMergeable(StashPullRequestResponseValue pullRequest) {
+  /**
+   * Inquiries Bitbucket Server whether the pull request can be merged
+   *
+   * @param pullRequest pull request
+   * @return true if the merge is allowed, false otherwise
+   * @throws StashApiException if cannot communicate to the server
+   */
+  private boolean isPullRequestMergeable(StashPullRequestResponseValue pullRequest)
+      throws StashApiException {
     if (trigger.getCheckMergeable()
         || trigger.getCheckNotConflicted()
         || trigger.getCheckProbeMergeStatus()) {
@@ -454,8 +523,21 @@ private boolean isBuildTarget(StashPullRequestResponseValue pullRequest) {
       return false;
     }
 
-    if (!isPullRequestMergeable(pullRequest)) {
-      logger.info("Skipping PR: " + pullRequest.getId() + " as cannot be merged");
+    // Check whether the pull request can be merged and whether it's in the
+    // "conflicted" state. If that information cannot be retrieved, don't build
+    // the pull request in this cycle.
+    try {
+      if (!isPullRequestMergeable(pullRequest)) {
+        logger.info("Skipping PR: " + pullRequest.getId() + " as cannot be merged");
+        return false;
+      }
+    } catch (StashApiException e) {
+      logger.log(
+          Level.INFO,
+          format(
+              "%s: cannot determine if pull request %s can be merged, skipping",
+              job.getDisplayName(), pullRequest.getId()),
+          e);
       return false;
     }
 
@@ -473,6 +555,10 @@ private boolean isBuildTarget(StashPullRequestResponseValue pullRequest) {
     String destinationCommit = destination.getLatestCommit();
 
     String id = pullRequest.getId();
+
+    // Fetch all comments for the pull request. If it fails, don't build the
+    // pull request in this cycle, as it cannot be determined if it should be
+    // built without checking the comments.
     List<StashPullRequestComment> comments;
     try {
       comments = client.getPullRequestComments(owner, repositoryName, id);