Skip to content

Commit

Permalink
StashApiClient: Improve exception handling
Browse files Browse the repository at this point in the history 
Don't ever print the stack trace. Don't throw RuntimeException. Don't log
a SEVERE message and throw for the same error.

Catch most specific exceptions. Don't use semicolon in place of colon in
error messages. Make sure there is space after colon. Improve wording.

Handle exceptions at the level where they can be reported with sufficient
context.

Report the whole status line (e.g. "404 Not found") in error messages.
Don't mention code 200 in error messages, the code accepts other codes as
success.
  • Loading branch information
@proski
proski committed Apr 10, 2019
1 parent b1237c0 commit a9cbb00
Showing 1 changed file with 56 additions and 61 deletions.
117 changes: 56 additions & 61 deletions src/main/java/stashpullrequestbuilder/stashpullrequestbuilder/stash/StashApiClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import java.util.Collections;
import java.util.List;
import java.util.concurrent.Callable;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.FutureTask;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.TimeoutException;
Expand Down Expand Up @@ -105,7 +106,7 @@ public List<StashPullRequestResponseValue> getPullRequests() {
}
return pullRequestResponseValues;
} catch (IOException e) {
logger.log(Level.WARNING, "invalid pull request response.", e);
logger.log(Level.WARNING, "Failed to get pull request list: ", e);
}
return Collections.emptyList();
}
Expand Down Expand Up @@ -137,15 +138,22 @@ public List<StashPullRequestComment> getPullRequestComments(
commentResponses.add(resp);
}
return extractComments(commentResponses);
} catch (Exception e) {
logger.log(Level.WARNING, "invalid pull request response.", e);
} catch (IOException e) {
logger.log(Level.WARNING, "Cannot get Stash comments for PR #" + pullRequestId + ": ", e);
}
return Collections.emptyList();
}

public void deletePullRequestComment(String pullRequestId, String commentId) {
String path = pullRequestPath(pullRequestId) + "/comments/" + commentId + "?version=0";
deleteRequest(path);
try {
deleteRequest(path);
} catch (IOException e) {
logger.log(
Level.WARNING,
"Cannot delete comment for PR #" + pullRequestId + " comment ID " + commentId + ": ",
e);
}
}

@Nullable
Expand All @@ -156,7 +164,7 @@ public StashPullRequestComment postPullRequestComment(String pullRequestId, Stri
return parseSingleCommentJson(response);

} catch (IOException e) {
logger.log(Level.SEVERE, "Failed to post Stash PR comment " + path + " " + e);
logger.log(Level.WARNING, "Failed to post Stash PR comment " + path + " " + e);
}
return null;
}
Expand All @@ -176,7 +184,13 @@ public StashPullRequestMergeableResponse getPullRequestMergeStatus(String pullRe

public boolean mergePullRequest(String pullRequestId, String version) {
String path = pullRequestPath(pullRequestId) + "/merge?version=" + version;
String response = postRequest(path, null);
String response;
try {
response = postRequest(path, null);
} catch (IOException e) {
logger.log(Level.WARNING, "Cannot merge pull request: " + e);
return false;
}
return !response.equals(Integer.toString(HttpStatus.SC_CONFLICT));
}

Expand All @@ -203,7 +217,7 @@ private HttpContext getHttpContext(Credentials credentials) {
return context;
}

private HttpClient getHttpClient() {
private HttpClient getHttpClient() throws IOException {
HttpClientBuilder builder = HttpClientBuilder.create().useSystemProperties();
if (this.ignoreSsl) {
try {
Expand All @@ -214,22 +228,15 @@ private HttpClient getHttpClient() {
new SSLConnectionSocketFactory(
sslContextBuilder.build(), NoopHostnameVerifier.INSTANCE);
builder.setSSLSocketFactory(sslsf);
} catch (NoSuchAlgorithmException e) {
logger.log(Level.SEVERE, "Failing to setup the SSLConnectionFactory: " + e.toString());
throw new RuntimeException(e);
} catch (KeyStoreException e) {
logger.log(Level.SEVERE, "Failing to setup the SSLConnectionFactory: " + e.toString());
throw new RuntimeException(e);
} catch (KeyManagementException e) {
logger.log(Level.SEVERE, "Failing to setup the SSLConnectionFactory: " + e.toString());
throw new RuntimeException(e);
} catch (KeyManagementException | KeyStoreException | NoSuchAlgorithmException e) {
throw new IOException(e);
}
}
return builder.build();
}

private String getRequest(String path) {
logger.log(Level.FINEST, "PR-GET-REQUEST:" + path);
private String getRequest(String path) throws IOException {
logger.log(Level.FINEST, "GET path: " + path);
HttpClient client = getHttpClient();
HttpContext context = getHttpContext(credentials);

Expand All @@ -255,18 +262,17 @@ private String getRequest(String path) {
private HttpGet httpGet;

@Override
public String call() throws Exception {
public String call() throws IOException {
HttpResponse httpResponse = client.execute(httpGet, context);
int responseCode = httpResponse.getStatusLine().getStatusCode();
String response = httpResponse.getStatusLine().getReasonPhrase();
if (!validResponseCode(responseCode)) {
logger.log(
Level.SEVERE,
"Failing to get response from Stash PR GET" + httpGet.getURI().getPath());
throw new RuntimeException(
"Didn't get a 200 response from Stash PR GET! Response; '"
throw new IOException(
"Unexpected response for GET "
+ httpGet.getURI().getPath()
+ " - "
+ responseCode
+ "' with message; "
+ " "
+ response);
}
InputStream responseBodyAsStream = httpResponse.getEntity().getContent();
Expand All @@ -290,20 +296,18 @@ public Callable<String> init(
response = httpTask.get(StashApiClient.HTTP_REQUEST_TIMEOUT_SECONDS, TimeUnit.SECONDS);

} catch (TimeoutException e) {
e.printStackTrace();
httpGet.abort();
throw new RuntimeException(e);
} catch (Exception e) {
e.printStackTrace();
throw new RuntimeException(e);
throw new IOException(e);
} catch (ExecutionException | InterruptedException e) {
throw new IOException(e);
} finally {
httpGet.releaseConnection();
}
logger.log(Level.FINEST, "PR-GET-RESPONSE:" + response);
logger.log(Level.FINEST, "GET response: " + response);
return response;
}

public void deleteRequest(String path) {
public void deleteRequest(String path) throws IOException {
HttpClient client = getHttpClient();
HttpContext context = getHttpContext(this.credentials);

Expand All @@ -330,7 +334,7 @@ public void deleteRequest(String path) {
private HttpDelete httpDelete;

@Override
public Integer call() throws Exception {
public Integer call() throws IOException {
int res = -1;
res = client.execute(httpDelete, context).getStatusLine().getStatusCode();
return res;
Expand All @@ -349,21 +353,19 @@ public Callable<Integer> init(
res = httpTask.get(StashApiClient.HTTP_REQUEST_TIMEOUT_SECONDS, TimeUnit.SECONDS);

} catch (TimeoutException e) {
e.printStackTrace();
httpDelete.abort();
throw new RuntimeException(e);
} catch (Exception e) {
e.printStackTrace();
throw new RuntimeException(e);
throw new IOException(e);
} catch (ExecutionException | InterruptedException e) {
throw new IOException(e);
} finally {
httpDelete.releaseConnection();
}

logger.log(Level.FINE, "Delete comment {" + path + "} returned result code; " + res);
logger.log(Level.FINE, "DELETE on " + path + " returned result code: " + res);
}

private String postRequest(String path, String comment) {
logger.log(Level.FINEST, "PR-POST-REQUEST:" + path + " with: " + comment);
private String postRequest(String path, String comment) throws IOException {
logger.log(Level.FINEST, "POST on " + path + " with comment: " + comment);
HttpClient client = getHttpClient();
HttpContext context = getHttpContext(credentials);

Expand All @@ -378,12 +380,8 @@ private String postRequest(String path, String comment) {
ObjectNode node = mapper.getNodeFactory().objectNode();
node.put("text", comment);
StringEntity requestEntity = null;
try {
requestEntity =
new StringEntity(mapper.writeValueAsString(node), ContentType.APPLICATION_JSON);
} catch (IOException e) {
e.printStackTrace();
}
requestEntity =
new StringEntity(mapper.writeValueAsString(node), ContentType.APPLICATION_JSON);
httpPost.setEntity(requestEntity);
}

Expand All @@ -404,26 +402,25 @@ private String postRequest(String path, String comment) {
private HttpPost httpPost;

@Override
public String call() throws Exception {
public String call() throws IOException {

HttpResponse httpResponse = client.execute(httpPost, context);
int responseCode = httpResponse.getStatusLine().getStatusCode();
String response = httpResponse.getStatusLine().getReasonPhrase();
if (!validResponseCode(responseCode)) {
logger.log(
Level.SEVERE,
"Failing to get response from Stash PR POST" + httpPost.getURI().getPath());
throw new RuntimeException(
"Didn't get a 200 response from Stash PR POST! Response; '"
throw new IOException(
"Unexpected response for POST "
+ httpPost.getURI().getPath()
+ " - "
+ responseCode
+ "' with message; "
+ " "
+ response);
}
InputStream responseBodyAsStream = httpResponse.getEntity().getContent();
StringWriter stringWriter = new StringWriter();
IOUtils.copy(responseBodyAsStream, stringWriter, "UTF-8");
response = stringWriter.toString();
logger.log(Level.FINEST, "API Request Response: " + response);
logger.log(Level.FINEST, "POST response (in Callable): " + response);

return response;
}
Expand All @@ -441,17 +438,15 @@ public Callable<String> init(
response = httpTask.get(StashApiClient.HTTP_REQUEST_TIMEOUT_SECONDS, TimeUnit.SECONDS);

} catch (TimeoutException e) {
e.printStackTrace();
httpPost.abort();
throw new RuntimeException(e);
} catch (Exception e) {
e.printStackTrace();
throw new RuntimeException(e);
throw new IOException(e);
} catch (ExecutionException | InterruptedException e) {
throw new IOException(e);
} finally {
httpPost.releaseConnection();
}

logger.log(Level.FINEST, "PR-POST-RESPONSE:" + response);
logger.log(Level.FINEST, "POST response: " + response);

return response;
}
Expand Down

0 comments on commit a9cbb00

Please sign in to comment.