Improve Gerrit connector usability (#230)

* Allow usage of Gerrit HTTP password tokens (#220)

This makes it possible to use http password token that is defined in
Settings -> HTTP Password page instead of the regular site password.

https://gerrit-review.googlesource.com/Documentation/rest-api.html#authentication

* URL encode changeId when creating GerritPatchset (#221)

When a cherry-pick is done and there are more commits with the same
changeId, an exended id can be used to distinguish between them.

It has a tilde separated format:
project/subproject~branch/subbranch~changeId

If this is not encoded but just put in the URL, an error message comes
from the server like:
Request not successful. Message: Not Found. Status-Code: 404.
Content: Not found: project

* Support gerrit configuration to omit duplicate comments (#108)

As it's a Gerrit feature and not a client side deduplication, it should
not have a large overhead, therefore it's enabled by default.

* Improve test coverage of Gerrit connector

* Use Gerrit API's URL encoding in GerritFacadeBuilder

This actually does the same as the old 'safeUrlEncode(String)'.
Also a reference is now added for the ID encoding method.

* Clean up Gerrit connector tests to better fit the project style

Co-authored-by: Gabor Garancsi <[email protected]>

src/main/java/pl/touk/sputnik/configuration/GeneralOption.java

@@ -82,6 +82,9 @@ public enum GeneralOption implements ConfigurationOption {
 
     GITHUB_API_KEY("github.api.key", "Personal access tokens for Github", ""),
 
+    GERRIT_USE_HTTP_PASSWORD("gerrit.useHttpPassword", "Use Gerrit's internal password token.", "false"),
+    GERRIT_OMIT_DUPLICATE_COMMENTS("gerrit.omitDuplicateComments", "Avoid publishing same comments for the same patchset.", "true"),
+
     JAVA_SRC_DIR("java.src.dir", "Java root source directory", Paths.SRC_MAIN),
     JAVA_TEST_DIR("java.test.dir", "Java root test directory", Paths.SRC_TEST),
 

src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacade.java

@@ -1,5 +1,6 @@
 package pl.touk.sputnik.connector.gerrit;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.gerrit.extensions.api.GerritApi;
 import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.api.changes.RevisionApi;
@@ -26,7 +27,10 @@ public class GerritFacade implements ConnectorFacade, ReviewPublisher {
     private static final String COMMIT_MSG = "/COMMIT_MSG";
 
     private final GerritApi gerritApi;
-    private final GerritPatchset gerritPatchset;
+    @VisibleForTesting
+    final GerritPatchset gerritPatchset;
+    @VisibleForTesting
+    final GerritOptions options;
 
     @NotNull
     @Override
@@ -63,6 +67,8 @@ public void publish(@NotNull Review review) {
         try {
             log.debug("Set review in Gerrit: {}", review);
             ReviewInput reviewInput = new ReviewInputBuilder().toReviewInput(review, gerritPatchset.getTag());
+            reviewInput.omitDuplicateComments = options.isOmitDuplicateComments();
+
             gerritApi.changes()
                     .id(gerritPatchset.getChangeId())
                     .revision(gerritPatchset.getRevisionId())

src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilder.java

@@ -1,10 +1,13 @@
 package pl.touk.sputnik.connector.gerrit;
 
+import com.google.common.base.Splitter;
 import com.google.common.base.Strings;
 import com.google.gerrit.extensions.api.GerritApi;
+import com.google.gerrit.extensions.restapi.Url;
 import com.urswolfer.gerrit.client.rest.GerritAuthData;
 import com.urswolfer.gerrit.client.rest.GerritRestApiFactory;
 import com.urswolfer.gerrit.client.rest.http.HttpClientBuilderExtension;
+
 import lombok.extern.slf4j.Slf4j;
 import org.apache.http.impl.client.HttpClientBuilder;
 import org.jetbrains.annotations.NotNull;
@@ -16,10 +19,13 @@
 
 import static org.apache.commons.lang3.Validate.notBlank;
 
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+
 @Slf4j
 public class GerritFacadeBuilder {
 
-    private HttpHelper httpHelper = new HttpHelper();
+    private final HttpHelper httpHelper = new HttpHelper();
 
     @NotNull
     public GerritFacade build(Configuration configuration) {
@@ -32,9 +38,12 @@ public GerritFacade build(Configuration configuration) {
             hostUri += connectorDetails.getPath();
         }
 
+        GerritOptions gerritOptions = GerritOptions.from(configuration);
+
         log.info("Using Gerrit URL: {}", hostUri);
         GerritAuthData.Basic authData = new GerritAuthData.Basic(hostUri,
-                connectorDetails.getUsername(), connectorDetails.getPassword());
+                connectorDetails.getUsername(), connectorDetails.getPassword(),
+                gerritOptions.isUseHttpPassword());
         GerritApi gerritApi = gerritRestApiFactory.create(authData, new HttpClientBuilderExtension() {
             @Override
             public HttpClientBuilder extend(HttpClientBuilder httpClientBuilder, GerritAuthData authData) {
@@ -44,7 +53,7 @@ public HttpClientBuilder extend(HttpClientBuilder httpClientBuilder, GerritAuthD
             }
         });
 
-        return new GerritFacade(gerritApi, gerritPatchset);
+        return new GerritFacade(gerritApi, gerritPatchset, gerritOptions);
     }
 
     @NotNull
@@ -56,6 +65,17 @@ private GerritPatchset buildGerritPatchset(Configuration configuration) {
         notBlank(changeId, "You must provide non blank Gerrit change Id");
         notBlank(revisionId, "You must provide non blank Gerrit revision Id");
 
-        return new GerritPatchset(changeId, revisionId, tag);
+        return new GerritPatchset(urlEncodeChangeId(changeId), revisionId, tag);
+    }
+
+    public static String urlEncodeChangeId(String changeId) {
+        if (changeId.indexOf('%') >= 0) {
+            // ChangeID is already encoded (otherwise why it would have a '%' character?)
+            return changeId;
+        }
+        // To keep the changeId readable, we don't encode '~' (it is not needed according to RFC2396)
+        // See also: ChangesRestClient.id(String, String) and ChangesRestClient.id(String, String, String)
+        return StreamSupport.stream(Splitter.on('~').split(changeId).spliterator(), false)
+                .map(Url::encode).collect(Collectors.joining("~"));
     }
 }

src/main/java/pl/touk/sputnik/connector/gerrit/GerritOptions.java

@@ -0,0 +1,34 @@
+package pl.touk.sputnik.connector.gerrit;
+
+import com.google.common.annotations.VisibleForTesting;
+
+import lombok.AccessLevel;
+import lombok.AllArgsConstructor;
+import lombok.Data;
+import pl.touk.sputnik.configuration.Configuration;
+import pl.touk.sputnik.configuration.GeneralOption;
+
+@Data
+@AllArgsConstructor(access = AccessLevel.PRIVATE)
+public class GerritOptions {
+    /**
+     * Indicates whether to use Gerrit's internal password token.
+     */
+    private final boolean useHttpPassword;
+    /**
+     * Indicates whether to avoid publishing the same comment again when the review is retriggered
+     * for the same revision.
+     */
+    private final boolean omitDuplicateComments;
+
+    static GerritOptions from(Configuration configuration) {
+        return new GerritOptions(
+                Boolean.parseBoolean(configuration.getProperty(GeneralOption.GERRIT_USE_HTTP_PASSWORD)),
+                Boolean.parseBoolean(configuration.getProperty(GeneralOption.GERRIT_OMIT_DUPLICATE_COMMENTS)));
+    }
+
+    @VisibleForTesting
+    static GerritOptions empty() {
+        return new GerritOptions(false, false);
+    }
+}

src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilderTest.java

@@ -0,0 +1,62 @@
+package pl.touk.sputnik.connector.gerrit;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.when;
+
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import pl.touk.sputnik.configuration.CliOption;
+import pl.touk.sputnik.configuration.Configuration;
+import pl.touk.sputnik.configuration.ConfigurationOption;
+import pl.touk.sputnik.configuration.GeneralOption;
+
+@ExtendWith(MockitoExtension.class)
+class GerritFacadeBuilderTest {
+    private static final String CHANGE_ID_WITH_SLASH = "project/subproject~branch/subbranch~changeId";
+    private static final String REVISION_ID = "changeId";
+
+    @Mock
+    private Configuration configuration;
+
+    private GerritFacadeBuilder gerritFacadeBuilder;
+
+    @BeforeEach
+    void setup() {
+        when(configuration.getProperty(any()))
+                .then(invocation -> ((ConfigurationOption)invocation.getArgument(0)).getDefaultValue());
+        configure(CliOption.CHANGE_ID, CHANGE_ID_WITH_SLASH);
+        configure(CliOption.REVISION_ID, REVISION_ID);
+
+        gerritFacadeBuilder = new GerritFacadeBuilder();
+    }
+
+    @Test
+    void shouldEscapeChangeIdWithSlash() {
+        GerritFacade connector = gerritFacadeBuilder.build(configuration);
+
+        assertThat(connector.gerritPatchset.getChangeId())
+                .isEqualTo("project%2Fsubproject~branch%2Fsubbranch~changeId");
+        assertThat(connector.gerritPatchset.getRevisionId())
+                .isEqualTo(REVISION_ID);
+    }
+
+    @Test
+    void shouldBuildWithCorrectOptions() {
+        configure(GeneralOption.GERRIT_USE_HTTP_PASSWORD, "true");
+        configure(GeneralOption.GERRIT_OMIT_DUPLICATE_COMMENTS, "false");
+
+        GerritFacade connector = gerritFacadeBuilder.build(configuration);
+
+        assertThat(connector.options.isUseHttpPassword()).isTrue();
+        assertThat(connector.options.isOmitDuplicateComments()).isFalse();
+    }
+
+    private void configure(ConfigurationOption option, String value) {
+        when(configuration.getProperty(option)).thenReturn(value);
+    }
+}

src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeExceptionTest.java

@@ -2,6 +2,8 @@
 
 import com.google.gerrit.extensions.api.GerritApi;
 import com.google.gerrit.extensions.api.changes.Changes;
+import com.google.gerrit.extensions.restapi.RestApiException;
+
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.mockito.Mock;
@@ -11,6 +13,10 @@
 import static org.assertj.core.api.Assertions.catchThrowable;
 import static org.mockito.Mockito.when;
 
+import java.util.ArrayList;
+
+import pl.touk.sputnik.review.Review;
+
 @ExtendWith(MockitoExtension.class)
 class GerritFacadeExceptionTest {
 
@@ -25,10 +31,10 @@ class GerritFacadeExceptionTest {
     private Changes changes;
 
     @Test
-    void shouldWrapConnectorException() throws Exception {
+    void listFiles_shouldWrapConnectorException() throws Exception {
         when(gerritApi.changes()).thenReturn(changes);
         when(changes.id(CHANGE_ID)).thenThrow(new RuntimeException("Connection refused"));
-        GerritFacade gerritFacade = new GerritFacade(gerritApi, new GerritPatchset(CHANGE_ID, REVISION_ID, TAG));
+        GerritFacade gerritFacade = new GerritFacade(gerritApi, new GerritPatchset(CHANGE_ID, REVISION_ID, TAG), GerritOptions.empty());
 
         Throwable thrown = catchThrowable(gerritFacade::listFiles);
 
@@ -37,4 +43,29 @@ void shouldWrapConnectorException() throws Exception {
                 .hasMessageContaining("Error when listing files");
     }
 
+    @Test
+    void publish_shouldWrapConnectorException() throws Exception {
+        when(gerritApi.changes()).thenReturn(changes);
+        when(changes.id(CHANGE_ID)).thenThrow(new RuntimeException("Connection refused"));
+        GerritFacade gerritFacade = new GerritFacade(gerritApi, new GerritPatchset(CHANGE_ID, REVISION_ID, TAG), GerritOptions.empty());
+
+        Throwable thrown = catchThrowable(() -> gerritFacade.publish(new Review(new ArrayList<>(), null)));
+
+        assertThat(thrown)
+                .isInstanceOf(GerritException.class)
+                .hasMessageContaining("Error when setting review");
+    }
+
+    @Test
+    void getRevision_shouldWrapRestApiException() throws Exception {
+        when(gerritApi.changes()).thenReturn(changes);
+        when(changes.id(CHANGE_ID)).thenThrow(RestApiException.wrap("Connection refused", new RuntimeException("Something bad happened")));
+        GerritFacade gerritFacade = new GerritFacade(gerritApi, new GerritPatchset(CHANGE_ID, REVISION_ID, TAG), GerritOptions.empty());
+
+        Throwable thrown = catchThrowable(gerritFacade::getRevision);
+
+        assertThat(thrown)
+                .isInstanceOf(GerritException.class)
+                .hasMessageContaining("Error when retrieve modified lines");
+    }
 }

src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeTest.java

@@ -5,27 +5,36 @@
 import com.google.gerrit.extensions.api.GerritApi;
 import com.google.gerrit.extensions.api.changes.ChangeApi;
 import com.google.gerrit.extensions.api.changes.Changes;
+import com.google.gerrit.extensions.api.changes.ReviewInput;
 import com.google.gerrit.extensions.api.changes.RevisionApi;
 import com.google.gerrit.extensions.common.FileInfo;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gson.Gson;
 import com.google.gson.JsonElement;
 import com.google.gson.JsonParser;
 import com.urswolfer.gerrit.client.rest.http.changes.FileInfoParser;
-import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.ArgumentCaptor;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
+
+import pl.touk.sputnik.configuration.Configuration;
+import pl.touk.sputnik.review.Review;
 import pl.touk.sputnik.review.ReviewFile;
+import pl.touk.sputnik.review.ReviewFormatter;
 
 import java.io.IOException;
+import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
+import static org.mockito.Mockito.withSettings;
 
 @ExtendWith(MockitoExtension.class)
 class GerritFacadeTest {
@@ -36,13 +45,10 @@ class GerritFacadeTest {
 
     @Mock
     private GerritApi gerritApi;
-
-    private GerritFacade gerritFacade;
-
-    @BeforeEach
-    void setUp() {
-        gerritFacade = new GerritFacade(gerritApi, null);
-    }
+    @Mock
+    private GerritOptions gerritOptions;
+    @Mock
+    private Configuration configuration;
 
     @Test
     void shouldParseListFilesResponse() throws IOException, RestApiException {
@@ -56,7 +62,38 @@ void shouldNotListDeletedFiles() throws IOException, RestApiException {
         assertThat(reviewFiles).hasSize(1);
     }
 
+    @Test
+    void shouldCallGerritApiOnPublish() throws IOException, RestApiException {
+        when(gerritOptions.isOmitDuplicateComments()).thenReturn(true);
+        Review review = new Review(new ArrayList<>(), new ReviewFormatter(configuration));
+        ArgumentCaptor<ReviewInput> reviewInputCaptor = ArgumentCaptor.forClass(ReviewInput.class);
+
+        createGerritFacade().publish(review);
+
+        verify(gerritApi.changes().id(CHANGE_ID).revision(REVISION_ID)).review(reviewInputCaptor.capture());
+        assertThat(reviewInputCaptor.getValue().omitDuplicateComments).isTrue();
+        assertThat(reviewInputCaptor.getValue().tag).isEqualTo(TAG);
+    }
+
+    @Test
+    void shouldRevisionApiBeConfigured() throws IOException, RestApiException {
+        GerritFacade gerritFacade = createGerritFacade();
+
+        assertThat(gerritFacade.getRevision())
+                .isEqualTo(gerritApi.changes().id(CHANGE_ID).revision(REVISION_ID));
+    }
+
+    @Test
+    void shouldReviewDelegateToPublish() throws IOException, RestApiException {
+        Review review = new Review(new ArrayList<>(), new ReviewFormatter(configuration));
+
+        createGerritFacade().setReview(review);
+
+        verify(gerritApi.changes().id(CHANGE_ID).revision(REVISION_ID)).review(any());
+    }
+
     private GerritFacade createGerritFacade() throws IOException, RestApiException {
+        @SuppressWarnings("UnstableApiUsage")
         String listFilesJson = Resources.toString(Resources.getResource("json/gerrit-listfiles.json"), Charsets.UTF_8);
         JsonElement jsonElement = new JsonParser().parse(listFilesJson);
         Map<String, FileInfo> fileInfoMap = new FileInfoParser(new Gson()).parseFileInfos(jsonElement);
@@ -65,10 +102,9 @@ private GerritFacade createGerritFacade() throws IOException, RestApiException {
         when(gerritApi.changes()).thenReturn(changes);
         ChangeApi changeApi = mock(ChangeApi.class);
         when(changes.id(CHANGE_ID)).thenReturn(changeApi);
-        RevisionApi revisionApi = mock(RevisionApi.class);
+        RevisionApi revisionApi = mock(RevisionApi.class, withSettings().lenient());
         when(changeApi.revision(REVISION_ID)).thenReturn(revisionApi);
         when(revisionApi.files()).thenReturn(fileInfoMap);
-        return new GerritFacade(gerritApi, new GerritPatchset(CHANGE_ID, REVISION_ID, TAG));
+        return new GerritFacade(gerritApi, new GerritPatchset(CHANGE_ID, REVISION_ID, TAG), gerritOptions);
     }
-
-}
+}