#461: Correctly encode spaces in Azure DevOps URLs

Azure DevOps does not correctly resolve URLs that contains spaces encoded as a `+`. To allow projects containg spaces to be decorated, the `+` from an the encoded names is being replaced with a `%20` since Azure Devops resolves spaces encoded this way properly.

src/main/java/com/github/mc1arke/sonarqube/plugin/almclient/azuredevops/AzureDevopsRestClient.java

@@ -46,6 +46,7 @@
 import java.util.List;
 import java.util.Objects;
 import java.util.Optional;
+import java.util.function.Supplier;
 
 public class AzureDevopsRestClient implements AzureDevopsClient {
 
@@ -56,61 +57,67 @@ public class AzureDevopsRestClient implements AzureDevopsClient {
     private final String authToken;
     private final String apiUrl;
     private final ObjectMapper objectMapper;
+    private final Supplier<CloseableHttpClient> httpClientFactory;
 
     public AzureDevopsRestClient(String apiUrl, String authToken, ObjectMapper objectMapper) {
+        this(apiUrl, authToken, objectMapper, HttpClients::createSystem);
+    }
+
+    AzureDevopsRestClient(String apiUrl, String authToken, ObjectMapper objectMapper, Supplier<CloseableHttpClient> httpClientFactory) {
         super();
         this.apiUrl = apiUrl;
         this.authToken = authToken;
         this.objectMapper = objectMapper;
+        this.httpClientFactory = httpClientFactory;
     }
 
     @Override
     public void submitPullRequestStatus(String projectId, String repositoryName, int pullRequestId, GitPullRequestStatus status) throws IOException {
-        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/statuses?api-version=%s", apiUrl, URLEncoder.encode(projectId, StandardCharsets.UTF_8), URLEncoder.encode(repositoryName, StandardCharsets.UTF_8), pullRequestId, API_VERSION_PREVIEW);
+        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/statuses?api-version=%s", apiUrl, encode(projectId), encode(repositoryName), pullRequestId, API_VERSION_PREVIEW);
         execute(url, "post", objectMapper.writeValueAsString(status), null);
     }
 
     @Override
     public Repository getRepository(String projectId, String repositoryName) throws IOException {
-        String url = String.format("%s/%s/_apis/git/repositories/%s?api-version=%s", apiUrl, URLEncoder.encode(projectId, StandardCharsets.UTF_8), URLEncoder.encode(repositoryName, StandardCharsets.UTF_8), API_VERSION);
+        String url = String.format("%s/%s/_apis/git/repositories/%s?api-version=%s", apiUrl, encode(projectId), encode(repositoryName), API_VERSION);
         return execute(url, "get", null, Repository.class);
     }
 
     @Override
     public List<CommentThread> retrieveThreads(String projectId, String repositoryName, int pullRequestId) throws IOException {
-        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/threads?api-version=%s", apiUrl, URLEncoder.encode(projectId, StandardCharsets.UTF_8), URLEncoder.encode(repositoryName, StandardCharsets.UTF_8), pullRequestId, API_VERSION);
+        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/threads?api-version=%s", apiUrl, encode(projectId), encode(repositoryName), pullRequestId, API_VERSION);
         return Objects.requireNonNull(execute(url, "get", null, CommentThreadResponse.class)).getValue();
     }
 
     @Override
     public CommentThread createThread(String projectId, String repositoryName, int pullRequestId, CreateCommentThreadRequest thread) throws IOException {
-        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/threads?api-version=%s", apiUrl, URLEncoder.encode(projectId, StandardCharsets.UTF_8), URLEncoder.encode(repositoryName, StandardCharsets.UTF_8), pullRequestId, API_VERSION);
+        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/threads?api-version=%s", apiUrl, encode(projectId), encode(repositoryName), pullRequestId, API_VERSION);
         return execute(url, "post", objectMapper.writeValueAsString(thread), CommentThread.class);
     }
 
     @Override
     public void addCommentToThread(String projectId, String repositoryName, int pullRequestId, int threadId, CreateCommentRequest comment) throws IOException {
-        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/threads/%s/comments?api-version=%s", apiUrl, URLEncoder.encode(projectId, StandardCharsets.UTF_8), URLEncoder.encode(repositoryName, StandardCharsets.UTF_8), pullRequestId, threadId, API_VERSION);
+        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/threads/%s/comments?api-version=%s", apiUrl, encode(projectId), encode(repositoryName), pullRequestId, threadId, API_VERSION);
         execute(url, "post", objectMapper.writeValueAsString(comment), null);
     }
 
     @Override
     public void resolvePullRequestThread(String projectId, String repositoryName, int pullRequestId, int threadId) throws IOException {
-        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/threads/%s?api-version=%s", apiUrl, URLEncoder.encode(projectId, StandardCharsets.UTF_8), URLEncoder.encode(repositoryName, StandardCharsets.UTF_8), pullRequestId, threadId, API_VERSION);
+        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/threads/%s?api-version=%s", apiUrl, encode(projectId), encode(repositoryName), pullRequestId, threadId, API_VERSION);
 
         UpdateCommentThreadStatusRequest commentThread = new UpdateCommentThreadStatusRequest(CommentThreadStatus.CLOSED);
         execute(url, "patch", objectMapper.writeValueAsString(commentThread), null);
     }
 
     @Override
     public PullRequest retrievePullRequest(String projectId, String repositoryName, int pullRequestId) throws IOException {
-        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s?api-version=%s", apiUrl, URLEncoder.encode(projectId, StandardCharsets.UTF_8), URLEncoder.encode(repositoryName, StandardCharsets.UTF_8), pullRequestId, API_VERSION);
+        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s?api-version=%s", apiUrl, encode(projectId), encode(repositoryName), pullRequestId, API_VERSION);
         return execute(url, "get", null, PullRequest.class);
     }
 
     @Override
     public List<Commit> getPullRequestCommits(String projectId, String repositoryName, int pullRequestId) throws IOException {
-        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/commits?api-version=%s", apiUrl, URLEncoder.encode(projectId, StandardCharsets.UTF_8), URLEncoder.encode(repositoryName, StandardCharsets.UTF_8), pullRequestId, API_VERSION);
+        String url = String.format("%s/%s/_apis/git/repositories/%s/pullRequests/%s/commits?api-version=%s", apiUrl, encode(projectId), encode(repositoryName), pullRequestId, API_VERSION);
         return Objects.requireNonNull(execute(url, "get", null, Commits.class)).getValue();
     }
 
@@ -124,7 +131,7 @@ private <T> T execute(String url, String method, String content, Class<T> type)
         Optional.ofNullable(content).ifPresent(body -> requestBuilder.setEntity(new StringEntity(body, StandardCharsets.UTF_8)));
         Optional.ofNullable(type).ifPresent(responseType -> requestBuilder.addHeader("Accept", ContentType.APPLICATION_JSON.getMimeType()));
 
-        try (CloseableHttpClient httpClient = HttpClients.createSystem()) {
+        try (CloseableHttpClient httpClient = httpClientFactory.get()) {
             HttpResponse httpResponse = httpClient.execute(requestBuilder.build());
 
             validateResponse(httpResponse);
@@ -141,13 +148,15 @@ private static void validateResponse(HttpResponse httpResponse) {
             return;
         }
 
-        String responseContent;
-        try {
-            responseContent = EntityUtils.toString(httpResponse.getEntity(), StandardCharsets.UTF_8);
-        } catch (IOException ex) {
-            LOGGER.warn("Could not decode response entity", ex);
-            responseContent = "";
-        }
+        String responseContent = Optional.ofNullable(httpResponse.getEntity()).map(entity -> {
+            try {
+                return EntityUtils.toString(entity, StandardCharsets.UTF_8);
+            } catch (IOException ex) {
+                LOGGER.warn("Could not decode response entity", ex);
+                return "";
+            }
+        }).orElse("");
+
 
         LOGGER.error("Azure Devops response status did not match expected value. Expected: 200"
                 + System.lineSeparator()
@@ -157,4 +166,8 @@ private static void validateResponse(HttpResponse httpResponse) {
 
         throw new IllegalStateException("An unexpected response code was returned from the Azure Devops API - Expected: 200, Got: " + httpResponse.getStatusLine().getStatusCode());
     }
+
+    private static String encode(String input) {
+        return URLEncoder.encode(input, StandardCharsets.UTF_8).replace("+", "%20");
+    }
 }

src/test/java/com/github/mc1arke/sonarqube/plugin/almclient/azuredevops/AzureDevopsRestClientTest.java

@@ -0,0 +1,132 @@
+package com.github.mc1arke.sonarqube.plugin.almclient.azuredevops;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.CreateCommentRequest;
+import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.GitPullRequestStatus;
+import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.GitStatusContext;
+import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.PullRequest;
+import com.github.mc1arke.sonarqube.plugin.almclient.azuredevops.model.enums.GitStatusState;
+import org.apache.http.StatusLine;
+import org.apache.http.client.methods.CloseableHttpResponse;
+import org.apache.http.client.methods.HttpUriRequest;
+import org.apache.http.client.methods.RequestBuilder;
+import org.apache.http.entity.StringEntity;
+import org.apache.http.impl.client.CloseableHttpClient;
+import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentCaptor;
+
+import java.io.IOException;
+import java.net.URI;
+import java.nio.charset.StandardCharsets;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+class AzureDevopsRestClientTest {
+
+    private final ObjectMapper objectMapper = mock(ObjectMapper.class);
+    private final CloseableHttpClient closeableHttpClient = mock(CloseableHttpClient.class);
+
+    @Test
+    void checkErrorThrownOnNonSuccessResponseStatus() throws IOException {
+        AzureDevopsRestClient underTest = new AzureDevopsRestClient("http://url.test/api", "token", objectMapper, () -> closeableHttpClient);
+
+        CloseableHttpResponse closeableHttpResponse = mock(CloseableHttpResponse.class);
+        StatusLine statusLine = mock(StatusLine.class);
+        when(statusLine.getStatusCode()).thenReturn(500);
+        when(closeableHttpResponse.getStatusLine()).thenReturn(statusLine);
+        when(closeableHttpClient.execute(any())).thenReturn(closeableHttpResponse);
+        when(objectMapper.writeValueAsString(any())).thenReturn("json");
+
+        assertThatThrownBy(() -> underTest.submitPullRequestStatus("project", "repo", 101, new GitPullRequestStatus(GitStatusState.SUCCEEDED, "description", new GitStatusContext("name", "genre"), "url")))
+                .isExactlyInstanceOf(IllegalStateException.class)
+                .hasMessage("An unexpected response code was returned from the Azure Devops API - Expected: 200, Got: 500")
+                .hasNoCause();
+
+        ArgumentCaptor<HttpUriRequest> requestArgumentCaptor = ArgumentCaptor.forClass(HttpUriRequest.class);
+        verify(closeableHttpClient).execute(requestArgumentCaptor.capture());
+
+        RequestBuilder request = RequestBuilder.copy(requestArgumentCaptor.getValue());
+
+        assertThat(request.getMethod()).isEqualTo("post");
+        assertThat(request.getUri()).isEqualTo(URI.create("http://url.test/api/project/_apis/git/repositories/repo/pullRequests/101/statuses?api-version=4.1-preview"));
+        assertThat(request.getEntity()).usingRecursiveComparison().isEqualTo(new StringEntity("json", StandardCharsets.UTF_8));
+    }
+
+    @Test
+    void checkSubmitPullRequestStatusSubmitsCorrectContent() throws IOException {
+        AzureDevopsRestClient underTest = new AzureDevopsRestClient("http://url.test/api", "token", objectMapper, () -> closeableHttpClient);
+
+        CloseableHttpResponse closeableHttpResponse = mock(CloseableHttpResponse.class);
+        StatusLine statusLine = mock(StatusLine.class);
+        when(statusLine.getStatusCode()).thenReturn(200);
+        when(closeableHttpResponse.getStatusLine()).thenReturn(statusLine);
+        when(closeableHttpClient.execute(any())).thenReturn(closeableHttpResponse);
+        when(objectMapper.writeValueAsString(any())).thenReturn("json");
+
+        underTest.submitPullRequestStatus("project Id With Spaces", "repository Name With Spaces", 123, new GitPullRequestStatus(GitStatusState.SUCCEEDED, "description", new GitStatusContext("name", "genre"), "url"));
+
+        ArgumentCaptor<HttpUriRequest> requestArgumentCaptor = ArgumentCaptor.forClass(HttpUriRequest.class);
+        verify(closeableHttpClient).execute(requestArgumentCaptor.capture());
+
+        RequestBuilder request = RequestBuilder.copy(requestArgumentCaptor.getValue());
+
+        assertThat(request.getMethod()).isEqualTo("post");
+        assertThat(request.getUri()).isEqualTo(URI.create("http://url.test/api/project%20Id%20With%20Spaces/_apis/git/repositories/repository%20Name%20With%20Spaces/pullRequests/123/statuses?api-version=4.1-preview"));
+        assertThat(request.getEntity()).usingRecursiveComparison().isEqualTo(new StringEntity("json", StandardCharsets.UTF_8));
+    }
+
+    @Test
+    void checkAddCommentToThreadSubmitsCorrectContent() throws IOException {
+        AzureDevopsRestClient underTest = new AzureDevopsRestClient("http://test.url", "authToken", objectMapper, () -> closeableHttpClient);
+
+        CloseableHttpResponse closeableHttpResponse = mock(CloseableHttpResponse.class);
+        StatusLine statusLine = mock(StatusLine.class);
+        when(statusLine.getStatusCode()).thenReturn(200);
+        when(closeableHttpResponse.getStatusLine()).thenReturn(statusLine);
+        when(closeableHttpClient.execute(any())).thenReturn(closeableHttpResponse);
+        when(objectMapper.writeValueAsString(any())).thenReturn("json");
+
+        underTest.addCommentToThread("projectId", "repository Name", 123, 321, new CreateCommentRequest("comment"));
+
+        ArgumentCaptor<HttpUriRequest> requestArgumentCaptor = ArgumentCaptor.forClass(HttpUriRequest.class);
+        verify(closeableHttpClient).execute(requestArgumentCaptor.capture());
+
+        RequestBuilder request = RequestBuilder.copy(requestArgumentCaptor.getValue());
+
+        assertThat(request.getMethod()).isEqualTo("post");
+        assertThat(request.getUri()).isEqualTo(URI.create("http://test.url/projectId/_apis/git/repositories/repository%20Name/pullRequests/123/threads/321/comments?api-version=4.1"));
+        assertThat(request.getEntity()).usingRecursiveComparison().isEqualTo(new StringEntity("json", StandardCharsets.UTF_8));
+    }
+
+    @Test
+    void checkRetrievePullRequestReturnsCorrectContent() throws IOException {
+        AzureDevopsRestClient underTest = new AzureDevopsRestClient("http://test.url", "authToken", objectMapper, () -> closeableHttpClient);
+
+        CloseableHttpResponse closeableHttpResponse = mock(CloseableHttpResponse.class);
+        StatusLine statusLine = mock(StatusLine.class);
+        when(statusLine.getStatusCode()).thenReturn(200);
+        when(closeableHttpResponse.getStatusLine()).thenReturn(statusLine);
+        when(closeableHttpResponse.getEntity()).thenReturn(new StringEntity("content", StandardCharsets.UTF_8));
+        when(closeableHttpClient.execute(any())).thenReturn(closeableHttpResponse);
+        PullRequest pullRequest = mock(PullRequest.class);
+        when(objectMapper.readValue(any(String.class), eq(PullRequest.class))).thenReturn(pullRequest);
+
+        PullRequest result = underTest.retrievePullRequest("projectId", "repository Name", 123);
+
+        ArgumentCaptor<HttpUriRequest> requestArgumentCaptor = ArgumentCaptor.forClass(HttpUriRequest.class);
+        verify(closeableHttpClient).execute(requestArgumentCaptor.capture());
+
+        RequestBuilder request = RequestBuilder.copy(requestArgumentCaptor.getValue());
+
+        assertThat(request.getMethod()).isEqualTo("get");
+        assertThat(request.getUri()).isEqualTo(URI.create("http://test.url/projectId/_apis/git/repositories/repository%20Name/pullRequests/123?api-version=4.1"));
+        assertThat(request.getEntity()).isNull();
+        assertThat(result).isSameAs(pullRequest);
+    }
+}