From b2c5fe9fe6d958d9da6c978680fd820d2e5acc9d Mon Sep 17 00:00:00 2001 From: corebonts Date: Thu, 28 Jan 2021 22:12:28 +0100 Subject: [PATCH] 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 --- .../sputnik/configuration/GeneralOption.java | 3 + .../connector/gerrit/GerritFacade.java | 8 ++- .../connector/gerrit/GerritFacadeBuilder.java | 28 +++++++-- .../connector/gerrit/GerritOptions.java | 34 ++++++++++ .../gerrit/GerritFacadeBuilderTest.java | 62 +++++++++++++++++++ .../gerrit/GerritFacadeExceptionTest.java | 35 ++++++++++- .../connector/gerrit/GerritFacadeTest.java | 60 ++++++++++++++---- .../connector/gerrit/GerritOptionsTest.java | 50 +++++++++++++++ .../gerrit/ReviewInputBuilderTest.java | 2 +- 9 files changed, 262 insertions(+), 20 deletions(-) create mode 100644 src/main/java/pl/touk/sputnik/connector/gerrit/GerritOptions.java create mode 100644 src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilderTest.java create mode 100644 src/test/java/pl/touk/sputnik/connector/gerrit/GerritOptionsTest.java diff --git a/src/main/java/pl/touk/sputnik/configuration/GeneralOption.java b/src/main/java/pl/touk/sputnik/configuration/GeneralOption.java index 7f81c9b5..ec93c80c 100644 --- a/src/main/java/pl/touk/sputnik/configuration/GeneralOption.java +++ b/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), diff --git a/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacade.java b/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacade.java index 5b49208c..b8055789 100644 --- a/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacade.java +++ b/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()) diff --git a/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilder.java b/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilder.java index ea4cc489..ce907916 100644 --- a/src/main/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilder.java +++ b/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("~")); } } diff --git a/src/main/java/pl/touk/sputnik/connector/gerrit/GerritOptions.java b/src/main/java/pl/touk/sputnik/connector/gerrit/GerritOptions.java new file mode 100644 index 00000000..e92b61f8 --- /dev/null +++ b/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); + } +} diff --git a/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilderTest.java b/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeBuilderTest.java new file mode 100644 index 00000000..026b59bf --- /dev/null +++ b/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); + } +} diff --git a/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeExceptionTest.java b/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeExceptionTest.java index 8b4cbb1f..e7ab5271 100644 --- a/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeExceptionTest.java +++ b/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"); + } } diff --git a/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeTest.java b/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeTest.java index 02abd07a..4a96c573 100644 --- a/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeTest.java +++ b/src/test/java/pl/touk/sputnik/connector/gerrit/GerritFacadeTest.java @@ -5,6 +5,7 @@ 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; @@ -12,20 +13,28 @@ 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 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 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); } - -} \ No newline at end of file +} diff --git a/src/test/java/pl/touk/sputnik/connector/gerrit/GerritOptionsTest.java b/src/test/java/pl/touk/sputnik/connector/gerrit/GerritOptionsTest.java new file mode 100644 index 00000000..2a37c787 --- /dev/null +++ b/src/test/java/pl/touk/sputnik/connector/gerrit/GerritOptionsTest.java @@ -0,0 +1,50 @@ +package pl.touk.sputnik.connector.gerrit; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static pl.touk.sputnik.configuration.GeneralOption.GERRIT_OMIT_DUPLICATE_COMMENTS; +import static pl.touk.sputnik.configuration.GeneralOption.GERRIT_USE_HTTP_PASSWORD; + +import org.junit.jupiter.api.Test; + +import pl.touk.sputnik.configuration.Configuration; +import pl.touk.sputnik.configuration.GeneralOption; + +class GerritOptionsTest { + + @Test + void shouldHttpPasswordOptionBeConsidered() { + assertThat(optionsFrom(GERRIT_USE_HTTP_PASSWORD, "false").isUseHttpPassword()) + .isFalse(); + assertThat(optionsFrom(GERRIT_USE_HTTP_PASSWORD, "true").isUseHttpPassword()) + .isTrue(); + } + + @Test + void shouldOmitDumplicateCommentsOptionBeConsidered() { + assertThat(optionsFrom(GERRIT_OMIT_DUPLICATE_COMMENTS, "false").isOmitDuplicateComments()) + .isFalse(); + assertThat(optionsFrom(GERRIT_OMIT_DUPLICATE_COMMENTS, "true").isOmitDuplicateComments()) + .isTrue(); + } + + @Test + void shouldDisableOptionalFeaturesByDefault() { + GerritOptions defaultOptions = GerritOptions.from(mock(Configuration.class)); + + assertThat(defaultOptions.isOmitDuplicateComments()).isFalse(); + assertThat(defaultOptions.isUseHttpPassword()).isFalse(); + } + + private GerritOptions optionsFrom(GeneralOption option, String value) { + return GerritOptions.from(configure(option, value)); + } + + private Configuration configure(GeneralOption option, String value) { + Configuration configuration = mock(Configuration.class); + when(configuration.getProperty(option)).thenReturn(value); + return configuration; + } + +} diff --git a/src/test/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilderTest.java b/src/test/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilderTest.java index f0591866..4799769f 100644 --- a/src/test/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilderTest.java +++ b/src/test/java/pl/touk/sputnik/connector/gerrit/ReviewInputBuilderTest.java @@ -18,7 +18,7 @@ class ReviewInputBuilderTest { private static final String TAG = "ci"; - private ReviewInputBuilder reviewInputBuilder = new ReviewInputBuilder(); + private final ReviewInputBuilder reviewInputBuilder = new ReviewInputBuilder(); @Test void shouldBuildReviewInput() {