From 9a780a75e30a7b4ccd474ac78e27f821d069b1d1 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 7 Jun 2018 11:11:20 +0900 Subject: [PATCH 1/3] DropWizardMetricMaker: Move sanitizeMetricName to MetricMaker base class The DropWizardMetricMaker#sanitizeMetricName method was added in change Ic0074b53f and used in change I332cb50b8 to prevent work queue metrics from being created with names that Dropwizard rejected. Those changes made the WorkQueue class dependent on the Dropwizard metrics implementation, which is contrary to the way the metrics system was designed: MetricMaker is the metrics interface, and DropWizardMetricMaker is the concrete implementation of it used in open source Gerrit. The reason it was done in this way was to allow Google to replace Dropwizard with their own metrics backend. Instead of calling the static DropWizardMetricMaker method, classes should refer to a method on the MetricMaker interface, so that it also works if the implementation is replaced with something else. Hoist sanitizeMetricName up to MetricMaker as a public method with a default implementation that only returns the input, and override it in the DropWizardMetricMaker. Change-Id: Iacaed3bbf36f17b87680255de8afb265ab73c06b --- .../com/google/gerrit/metrics/MetricMaker.java | 10 ++++++++++ .../dropwizard/DropWizardMetricMaker.java | 13 +++++++------ .../com/google/gerrit/server/git/WorkQueue.java | 4 +--- .../dropwizard/DropWizardMetricMakerTest.java | 16 +++++++++------- 4 files changed, 27 insertions(+), 16 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/metrics/MetricMaker.java b/gerrit-server/src/main/java/com/google/gerrit/metrics/MetricMaker.java index 977386952c..eee76fd04d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/metrics/MetricMaker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/metrics/MetricMaker.java @@ -167,4 +167,14 @@ public abstract class MetricMaker { } public abstract RegistrationHandle newTrigger(Set> metrics, Runnable trigger); + + /** + * Sanitize the given metric name. + * + * @param name the name to sanitize. + * @return sanitized version of the name. + */ + public String sanitizeMetricName(String name) { + return name; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java b/gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java index 37efb78701..fc53ee7fa1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java @@ -344,17 +344,18 @@ public class DropWizardMetricMaker extends MetricMaker { METRIC_NAME_PATTERN.pattern()); } - public static String sanitizeMetricName(String input) { - if (METRIC_NAME_PATTERN.matcher(input).matches()) { - return input; + @Override + public String sanitizeMetricName(String name) { + if (METRIC_NAME_PATTERN.matcher(name).matches()) { + return name; } - String first = input.substring(0, 1).replaceFirst("[^\\w-]", "_"); - if (input.length() == 1) { + String first = name.substring(0, 1).replaceFirst("[^\\w-]", "_"); + if (name.length() == 1) { return first; } - String result = first + input.substring(1).replaceAll("/[/]+", "/").replaceAll("[^\\w-/]", "_"); + String result = first + name.substring(1).replaceAll("/[/]+", "/").replaceAll("[^\\w-/]", "_"); if (result.endsWith("/")) { result = result.substring(0, result.length() - 1); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java index 4e77874bd7..4e9d937856 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/WorkQueue.java @@ -14,8 +14,6 @@ package com.google.gerrit.server.git; -import static com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker.sanitizeMetricName; - import com.google.common.base.CaseFormat; import com.google.common.base.Supplier; import com.google.gerrit.extensions.events.LifecycleListener; @@ -313,7 +311,7 @@ public class WorkQueue { CaseFormat.UPPER_CAMEL.to( CaseFormat.LOWER_UNDERSCORE, queueName.replaceFirst("SSH", "Ssh").replaceAll("-", "")); - return sanitizeMetricName(String.format("queue/%s/%s", name, metricName)); + return metrics.sanitizeMetricName(String.format("queue/%s/%s", name, metricName)); } public void unregisterWorkQueue() { diff --git a/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java b/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java index 26b98c6008..9b21bf6c01 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java @@ -15,28 +15,30 @@ package com.google.gerrit.metrics.dropwizard; import static com.google.common.truth.Truth.assertThat; -import static com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker.sanitizeMetricName; import org.junit.Test; public class DropWizardMetricMakerTest { + DropWizardMetricMaker metrics = + new DropWizardMetricMaker(null /* MetricRegistry unused in tests */); + @Test public void shouldSanitizeUnwantedChars() throws Exception { - assertThat(sanitizeMetricName("very+confusing$long#metric@net/name^1")) + assertThat(metrics.sanitizeMetricName("very+confusing$long#metric@net/name^1")) .isEqualTo("very_confusing_long_metric_net/name_1"); - assertThat(sanitizeMetricName("/metric/submetric")).isEqualTo("_metric/submetric"); + assertThat(metrics.sanitizeMetricName("/metric/submetric")).isEqualTo("_metric/submetric"); } @Test public void shouldReduceConsecutiveSlashesToOne() throws Exception { - assertThat(sanitizeMetricName("/metric//submetric1///submetric2/submetric3")) + assertThat(metrics.sanitizeMetricName("/metric//submetric1///submetric2/submetric3")) .isEqualTo("_metric/submetric1/submetric2/submetric3"); } @Test public void shouldNotFinishWithSlash() throws Exception { - assertThat(sanitizeMetricName("metric/")).isEqualTo("metric"); - assertThat(sanitizeMetricName("metric//")).isEqualTo("metric"); - assertThat(sanitizeMetricName("metric/submetric/")).isEqualTo("metric/submetric"); + assertThat(metrics.sanitizeMetricName("metric/")).isEqualTo("metric"); + assertThat(metrics.sanitizeMetricName("metric//")).isEqualTo("metric"); + assertThat(metrics.sanitizeMetricName("metric/submetric/")).isEqualTo("metric/submetric"); } } From 589af813ccca4c329bd4845852cd9d6d73d1d11d Mon Sep 17 00:00:00 2001 From: David Ostrovsky Date: Thu, 7 Jun 2018 06:39:38 +0200 Subject: [PATCH 2/3] Bazel: Remove unused commons-lang3 After removing Elasticsearch and Jest REST client library, commons lang3 is not used any more in Gerrit core. Given that we have forgotten to remove it and stil expose it in plugin API only confused the plugins that depend on it, because the build pass, but then the plugin cannot work, because the required dependency is missing. Change-Id: I7af2f9f6cb90e8e52d7ed2efaf4c3af85b47a9e2 --- WORKSPACE | 6 ------ gerrit-plugin-api/BUILD | 1 - lib/commons/BUILD | 6 ------ 3 files changed, 13 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 786b9ce584..b0ca3376b4 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -324,12 +324,6 @@ maven_jar( sha1 = "0ce1edb914c94ebc388f086c6827e8bdeec71ac2", ) -maven_jar( - name = "commons_lang3", - artifact = "org.apache.commons:commons-lang3:3.3.2", - sha1 = "90a3822c38ec8c996e84c16a3477ef632cbc87a3", -) - maven_jar( name = "commons_dbcp", artifact = "commons-dbcp:commons-dbcp:1.4", diff --git a/gerrit-plugin-api/BUILD b/gerrit-plugin-api/BUILD index 067b4cd38c..2a8a36cf98 100644 --- a/gerrit-plugin-api/BUILD +++ b/gerrit-plugin-api/BUILD @@ -22,7 +22,6 @@ EXPORTS = [ "//gerrit-server/src/main/prolog:common", "//lib/commons:dbcp", "//lib/commons:lang", - "//lib/commons:lang3", "//lib/dropwizard:dropwizard-core", "//lib/guice:guice", "//lib/guice:guice-assistedinject", diff --git a/lib/commons/BUILD b/lib/commons/BUILD index cc4de55bb3..9e9e0566f9 100644 --- a/lib/commons/BUILD +++ b/lib/commons/BUILD @@ -28,12 +28,6 @@ java_library( exports = ["@commons_lang//jar"], ) -java_library( - name = "lang3", - data = ["//lib:LICENSE-Apache2.0"], - exports = ["@commons_lang3//jar"], -) - java_library( name = "net", data = ["//lib:LICENSE-Apache2.0"], From bb412404dc125260e5883883b95022ac3751a6c0 Mon Sep 17 00:00:00 2001 From: Borui Tao Date: Wed, 30 May 2018 15:36:14 -0400 Subject: [PATCH 3/3] AbstractElasticIndex: fix index name of delete request and add coverage Previously the deletion of the index failed because of the improper index name. This change fixes the index name of the delete request so that the objects (i.e., groups, accounts or changes) that exist in the index can be deleted. This change also adds the test to delete a change from the index in order to test this fix and add coverage for the getDeleteActions method in ElasticChangeIndex. Bug: Issue 9040 Change-Id: I891bd4dc286f7bf0cfeb95de1c952d91ae631ba4 --- .../gerrit/elasticsearch/AbstractElasticIndex.java | 2 +- .../query/change/AbstractQueryChangesTest.java | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java index d3e6043b5b..45bd2b9477 100644 --- a/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java +++ b/gerrit-elasticsearch/src/main/java/com/google/gerrit/elasticsearch/AbstractElasticIndex.java @@ -157,7 +157,7 @@ abstract class AbstractElasticIndex implements Index { protected abstract String getId(V v); protected String delete(String type, K id) { - return new DeleteRequest(id.toString(), indexNameRaw, type).toString(); + return new DeleteRequest(id.toString(), indexName, type).toString(); } protected void addNamedElement(String name, JsonObject element, JsonArray array) { diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 7676e48de1..8f60067e50 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -2205,6 +2205,18 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { assertQuery("owner: \"" + nameEmail + "\"\\"); } + @Test + public void byDeletedChange() throws Exception { + TestRepository repo = createProject("repo"); + Change change = insert(repo, newChange(repo)); + + String query = "change:" + change.getId(); + assertQuery(query, change); + + gApi.changes().id(change.getChangeId()).delete(); + assertQuery(query); + } + protected ChangeInserter newChange(TestRepository repo) throws Exception { return newChange(repo, null, null, null, null); }