From 344fa63b108d2de03dc42b8326079ca0d0492c50 Mon Sep 17 00:00:00 2001 From: Mikhail Golubev Date: Tue, 2 Sep 2014 15:09:45 +0400 Subject: [PATCH] Proper way to cancel test requests in repositories based on httpclient-4.x --- .../tasks/gitlab/GitlabRepository.java | 24 +----- .../httpclient/NewBaseRepositoryImpl.java | 42 ++++++++++ .../tasks/redmine/RedmineRepository.java | 76 +++++++++++-------- .../integration/RedmineIntegrationTest.java | 5 +- 4 files changed, 94 insertions(+), 53 deletions(-) diff --git a/plugins/tasks/tasks-core/src/com/intellij/tasks/gitlab/GitlabRepository.java b/plugins/tasks/tasks-core/src/com/intellij/tasks/gitlab/GitlabRepository.java index cc66448c94e9..9df0eb07b26e 100644 --- a/plugins/tasks/tasks-core/src/com/intellij/tasks/gitlab/GitlabRepository.java +++ b/plugins/tasks/tasks-core/src/com/intellij/tasks/gitlab/GitlabRepository.java @@ -4,7 +4,6 @@ import com.google.gson.Gson; import com.google.gson.reflect.TypeToken; import com.intellij.openapi.util.Comparing; import com.intellij.tasks.Task; -import com.intellij.tasks.TaskBundle; import com.intellij.tasks.TaskRepositoryType; import com.intellij.tasks.gitlab.model.GitlabIssue; import com.intellij.tasks.gitlab.model.GitlabProject; @@ -14,7 +13,9 @@ import com.intellij.util.Function; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.xmlb.annotations.Tag; import com.intellij.util.xmlb.annotations.Transient; -import org.apache.http.*; +import org.apache.http.HttpException; +import org.apache.http.HttpRequest; +import org.apache.http.HttpRequestInterceptor; import org.apache.http.client.ResponseHandler; import org.apache.http.client.methods.HttpGet; import org.apache.http.protocol.HttpContext; @@ -118,24 +119,7 @@ public class GitlabRepository extends NewBaseRepositoryImpl { @Nullable @Override public CancellableConnection createCancellableConnection() { - return new CancellableConnection() { - private HttpGet myRequest = new HttpGet(getIssuesUrl()); - - @Override - protected void doTest() throws Exception { - HttpResponse response = getHttpClient().execute(myRequest); - StatusLine statusLine = response.getStatusLine(); - if (statusLine != null && statusLine.getStatusCode() != HttpStatus.SC_OK) { - throw new Exception(TaskBundle.message("failure.http.error", statusLine.getStatusCode(), statusLine.getReasonPhrase())); - } - } - - // TODO: find more about proper request aborting in HttpClient4.x - @Override - public void cancel() { - myRequest.abort(); - } - }; + return new HttpTestConnection(new HttpGet(getIssuesUrl())); } /** diff --git a/plugins/tasks/tasks-core/src/com/intellij/tasks/impl/httpclient/NewBaseRepositoryImpl.java b/plugins/tasks/tasks-core/src/com/intellij/tasks/impl/httpclient/NewBaseRepositoryImpl.java index c1648fc8f490..082d49812d5b 100644 --- a/plugins/tasks/tasks-core/src/com/intellij/tasks/impl/httpclient/NewBaseRepositoryImpl.java +++ b/plugins/tasks/tasks-core/src/com/intellij/tasks/impl/httpclient/NewBaseRepositoryImpl.java @@ -3,6 +3,7 @@ package com.intellij.tasks.impl.httpclient; import com.intellij.tasks.TaskRepositoryType; import com.intellij.tasks.config.TaskSettings; import com.intellij.tasks.impl.BaseRepository; +import com.intellij.tasks.impl.RequestFailedException; import com.intellij.tasks.impl.TaskUtil; import com.intellij.util.net.HttpConfigurable; import com.intellij.util.net.ssl.CertificateManager; @@ -14,6 +15,7 @@ import org.apache.http.client.CredentialsProvider; import org.apache.http.client.HttpClient; import org.apache.http.client.config.AuthSchemes; import org.apache.http.client.config.RequestConfig; +import org.apache.http.client.methods.HttpRequestBase; import org.apache.http.client.protocol.HttpClientContext; import org.apache.http.conn.ssl.X509HostnameVerifier; import org.apache.http.impl.auth.BasicScheme; @@ -154,4 +156,44 @@ public abstract class NewBaseRepositoryImpl extends BaseRepository { } } } + + public class HttpTestConnection extends CancellableConnection { + + // Request can be changed during test + protected volatile HttpRequestBase myCurrentRequest; + + public HttpTestConnection(@NotNull HttpRequestBase request) { + myCurrentRequest = request; + } + + @Override + protected void doTest() throws Exception { + try { + test(); + } + catch (IOException e) { + // Depending on request state AbstractExecutionAwareRequest.abort() can cause either + // * RequestAbortedException if connection was not yet leased + // * InterruptedIOException before reading response + // * SocketException("Socket closed") during reading response + // However in all cases 'aborted' flag should be properly set + if (!myCurrentRequest.isAborted()) { + throw e; + } + } + } + + protected void test() throws Exception { + HttpResponse response = getHttpClient().execute(myCurrentRequest); + StatusLine statusLine = response.getStatusLine(); + if (statusLine != null && statusLine.getStatusCode() != HttpStatus.SC_OK) { + throw RequestFailedException.forStatusCode(statusLine.getStatusCode(), statusLine.getReasonPhrase()); + } + } + + @Override + public void cancel() { + myCurrentRequest.abort(); + } + } } diff --git a/plugins/tasks/tasks-core/src/com/intellij/tasks/redmine/RedmineRepository.java b/plugins/tasks/tasks-core/src/com/intellij/tasks/redmine/RedmineRepository.java index e63b2b365e60..a4b10386aeba 100644 --- a/plugins/tasks/tasks-core/src/com/intellij/tasks/redmine/RedmineRepository.java +++ b/plugins/tasks/tasks-core/src/com/intellij/tasks/redmine/RedmineRepository.java @@ -16,6 +16,7 @@ import com.intellij.util.xmlb.annotations.Tag; import com.intellij.util.xmlb.annotations.Transient; import org.apache.http.HttpResponse; import org.apache.http.HttpStatus; +import org.apache.http.StatusLine; import org.apache.http.client.HttpClient; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.utils.URIBuilder; @@ -23,6 +24,8 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.TestOnly; +import java.net.URI; +import java.net.URISyntaxException; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -107,28 +110,36 @@ public class RedmineRepository extends NewBaseRepositoryImpl { return new RedmineRepository(this); } + @Nullable @Override - public void testConnection() throws Exception { - // Strangely, Redmine doesn't return 401 or 403 error codes, if client sent wrong credentials, and instead - // merely returns empty array of issues with status code of 200. This means that we should attempt to fetch - // something more specific than issues to test proper configuration, e.g. current user information at - // /users/current.json. Unfortunately this endpoint may be unavailable on some old servers (see IDEA-122845) - // and in this case we have to come back to requesting issues in this case to test anything at all. - HttpClient client = getHttpClient(); - URIBuilder uriBuilder = new URIBuilder(getRestApiUrl("users", "current.json")); - if (isUseApiKeyAuthentication()) { - uriBuilder.addParameter("key", getAPIKey()); - } - HttpResponse response = client.execute(new HttpGet(uriBuilder.build())); - //TaskUtil.prettyFormatResponseToLog(LOG, response); - int code = response.getStatusLine().getStatusCode(); - if (code == HttpStatus.SC_NOT_FOUND) { - getIssues("", 0, 1, true); - return; - } - if (code != HttpStatus.SC_OK) { - throw RequestFailedException.forStatusCode(code); - } + public CancellableConnection createCancellableConnection() { + return new NewBaseRepositoryImpl.HttpTestConnection(new HttpGet()) { + @Override + protected void test() throws Exception { + // Strangely, Redmine doesn't return 401 or 403 error codes, if client sent wrong credentials, and instead + // merely returns empty array of issues with status code of 200. This means that we should attempt to fetch + // something more specific than issues to test proper configuration, e.g. current user information at + // /users/current.json. Unfortunately this endpoint may be unavailable on some old servers (see IDEA-122845) + // and in this case we have to come back to requesting issues in this case to test anything at all. + + URIBuilder uriBuilder = new URIBuilder(getRestApiUrl("users", "current.json")); + if (isUseApiKeyAuthentication()) { + uriBuilder.addParameter("key", getAPIKey()); + } + myCurrentRequest.setURI(uriBuilder.build()); + HttpClient client = getHttpClient(); + + HttpResponse httpResponse = client.execute(myCurrentRequest); + StatusLine statusLine = httpResponse.getStatusLine(); + if (statusLine != null && statusLine.getStatusCode() == HttpStatus.SC_NOT_FOUND) { + myCurrentRequest = new HttpGet(getIssuesUrl(0, 1, true)); + statusLine = client.execute(myCurrentRequest).getStatusLine(); + } + if (statusLine != null && statusLine.getStatusCode() != HttpStatus.SC_OK) { + throw RequestFailedException.forStatusCode(statusLine.getStatusCode(), statusLine.getReasonPhrase()); + } + } + }; } @Override @@ -144,12 +155,6 @@ public class RedmineRepository extends NewBaseRepositoryImpl { public List fetchIssues(String query, int offset, int limit, boolean withClosed) throws Exception { ensureProjectsDiscovered(); - URIBuilder builder = new URIBuilder(getRestApiUrl("issues.json")) - .addParameter("offset", String.valueOf(offset)) - .addParameter("limit", String.valueOf(limit)) - .addParameter("status_id", withClosed ? "*" : "open") - .addParameter("assigned_to_id", "me"); - // Legacy API, can't find proper documentation //if (StringUtil.isNotEmpty(query)) { // builder.addParameter("fields[]", "subject").addParameter("operators[subject]", "~").addParameter("values[subject][]", query); @@ -158,13 +163,22 @@ public class RedmineRepository extends NewBaseRepositoryImpl { //if (myCurrentProject != null && myCurrentProject != UNSPECIFIED_PROJECT) { // builder.addParameter("project_id", String.valueOf(myCurrentProject.getId())); //} + HttpClient client = getHttpClient(); + HttpGet method = new HttpGet(getIssuesUrl(offset, limit, withClosed)); + IssuesWrapper wrapper = client.execute(method, new GsonSingleObjectDeserializer(GSON, IssuesWrapper.class)); + return wrapper == null ? Collections.emptyList() : wrapper.getIssues(); + } + + private URI getIssuesUrl(int offset, int limit, boolean withClosed) throws URISyntaxException { + URIBuilder builder = new URIBuilder(getRestApiUrl("issues.json")) + .addParameter("offset", String.valueOf(offset)) + .addParameter("limit", String.valueOf(limit)) + .addParameter("status_id", withClosed ? "*" : "open") + .addParameter("assigned_to_id", "me"); if (isUseApiKeyAuthentication()) { builder.addParameter("key", myAPIKey); } - HttpClient client = getHttpClient(); - HttpGet method = new HttpGet(builder.toString()); - IssuesWrapper wrapper = client.execute(method, new GsonSingleObjectDeserializer(GSON, IssuesWrapper.class)); - return wrapper == null ? Collections.emptyList() : wrapper.getIssues(); + return builder.build(); } public List fetchProjects() throws Exception { diff --git a/plugins/tasks/tasks-tests/test/com/intellij/tasks/integration/RedmineIntegrationTest.java b/plugins/tasks/tasks-tests/test/com/intellij/tasks/integration/RedmineIntegrationTest.java index 941320ad8971..a14f48a4258e 100644 --- a/plugins/tasks/tasks-tests/test/com/intellij/tasks/integration/RedmineIntegrationTest.java +++ b/plugins/tasks/tasks-tests/test/com/intellij/tasks/integration/RedmineIntegrationTest.java @@ -64,8 +64,9 @@ public class RedmineIntegrationTest extends TaskManagerTestCase { public void testCredentialsCheck() throws Exception { myRepository.setPassword("wrong-password"); try { - myRepository.testConnection(); - fail("testConnection() should fails, when wrong credentials specified"); + //noinspection ConstantConditions + final Exception exception = myRepository.createCancellableConnection().call(); + assertNotNull("Test connection must fail when wrong credentials specified", exception); } catch (Exception e) { assertEquals(TaskBundle.message("failure.login"), e.getMessage());