Merge "Merge branch 'stable-3.5' into stable-3.6" into stable-3.6
diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt
index ca72f8b..11f85dd 100644
--- a/Documentation/dev-plugins.txt
+++ b/Documentation/dev-plugins.txt
@@ -2342,6 +2342,25 @@
will cause the metrics to be recorded under `my-metrics/${metric-name}`.
+Note that metric name can be expressed through a limited set of characters
+`[a-zA-Z0-9_-]+(/[a-zA-Z0-9_-]+)*` therefore one should either ensure it
+or call
+link:https://gerrit.googlesource.com/gerrit/+/master/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java[
+sanitizeMetricName,role=external,window=_blank] to have it enforced.
+
+The
+link:https://gerrit.googlesource.com/gerrit/+/master/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java[
+sanitizeMetricName,role=external,window=_blank] performs the following
+modifications:
+
+* leading `/` is replaced with `_`
+* doubled (or repeated more times) `/` are reduced to a single `/`
+* ending `/` is removed
+* all characters that are not `/a-zA-Z0-9_-` are replaced with
+ `\_0x[HEX CODE]_` (code is capitalized)
+* if the replacement prefix (`_0x`) is found then it is prepended with another
+ replacement prefix
+
See the replication metrics in the
link:https://gerrit.googlesource.com/plugins/replication/+/master/src/main/java/com/googlesource/gerrit/plugins/replication/ReplicationMetrics.java[
replication plugin,role=external,window=_blank] for an example of usage.
diff --git a/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java b/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java
index 32be18d..0821e86 100644
--- a/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java
+++ b/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java
@@ -60,6 +60,7 @@
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;
+import java.util.regex.Matcher;
import java.util.regex.Pattern;
/**
@@ -357,6 +358,8 @@
private static final Pattern METRIC_NAME_PATTERN =
Pattern.compile("[a-zA-Z0-9_-]+(/[a-zA-Z0-9_-]+)*");
+ private static final Pattern INVALID_CHAR_PATTERN = Pattern.compile("[^\\w-/]");
+ private static final String REPLACEMENT_PREFIX = "_0x";
private static void checkMetricName(String name) {
checkArgument(
@@ -366,24 +369,54 @@
METRIC_NAME_PATTERN.pattern());
}
+ /**
+ * Ensures that the sanitized metric name doesn't contain invalid characters and
+ * removes the risk of collision (between the sanitized metric names).
+ * Modifications to the input metric name:
+ * <ul>
+ * <li/> leading <code>/</code> is replaced with <code>_</code>
+ * <li/> doubled (or repeated more times) <code>/</code> are reduced to a single <code>/<code>
+ * <li/> ending <code>/</code> is removed
+ * <li/> all characters that are not <code>/a-zA-Z0-9_-</code> are replaced with <code>_0x[HEX CODE]_</code> (code is capitalized)
+ * <li/> the replacement prefix <code>_0x</code> is prepended with another replacement prefix
+ * </ul>
+ *
+ * @param name name of the metric to sanitize
+ * @return sanitized metric name
+ */
@Override
public String sanitizeMetricName(String name) {
- if (METRIC_NAME_PATTERN.matcher(name).matches()) {
+ if (METRIC_NAME_PATTERN.matcher(name).matches() && !name.contains(REPLACEMENT_PREFIX)) {
return name;
}
- String first = name.substring(0, 1).replaceFirst("[^\\w-]", "_");
+ StringBuilder sanitizedName =
+ new StringBuilder(name.substring(0, 1).replaceFirst("[^\\w-]", "_"));
if (name.length() == 1) {
- return first;
+ return sanitizedName.toString();
}
- String result = first + name.substring(1).replaceAll("/[/]+", "/").replaceAll("[^\\w-/]", "_");
-
- if (result.endsWith("/")) {
- result = result.substring(0, result.length() - 1);
+ String slashSanitizedName = name.substring(1).replaceAll("/[/]+", "/");
+ if (slashSanitizedName.endsWith("/")) {
+ slashSanitizedName = slashSanitizedName.substring(0, slashSanitizedName.length() - 1);
}
- return result;
+ String replacementPrefixSanitizedName =
+ slashSanitizedName.replaceAll(REPLACEMENT_PREFIX, REPLACEMENT_PREFIX + REPLACEMENT_PREFIX);
+
+ for (int i = 0; i < replacementPrefixSanitizedName.length(); i++) {
+ Character c = replacementPrefixSanitizedName.charAt(i);
+ Matcher matcher = INVALID_CHAR_PATTERN.matcher(c.toString());
+ if (matcher.matches()) {
+ sanitizedName.append(REPLACEMENT_PREFIX);
+ sanitizedName.append(Integer.toHexString(c).toUpperCase());
+ sanitizedName.append('_');
+ } else {
+ sanitizedName.append(c);
+ }
+ }
+
+ return sanitizedName.toString();
}
static String name(Description.FieldOrdering ordering, String codeName, String fieldValues) {
diff --git a/java/com/google/gerrit/server/restapi/project/ListProjects.java b/java/com/google/gerrit/server/restapi/project/ListProjects.java
index cd68a2f..7c431da 100644
--- a/java/com/google/gerrit/server/restapi/project/ListProjects.java
+++ b/java/com/google/gerrit/server/restapi/project/ListProjects.java
@@ -338,7 +338,8 @@
&& state != HIDDEN
&& isNullOrEmpty(matchPrefix)
&& isNullOrEmpty(matchRegex)
- && isNullOrEmpty(matchSubstring) // TODO: see Issue 10446
+ && isNullOrEmpty(
+ matchSubstring) // TODO: see https://issues.gerritcodereview.com/issues/40010295
&& type == FilterType.ALL
&& showBranch.isEmpty()
&& !showTree
diff --git a/javatests/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java b/javatests/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java
index 5777779..a50520ac 100644
--- a/javatests/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java
+++ b/javatests/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java
@@ -44,15 +44,33 @@
}
@Test
+ public void shouldNotSanitizeSafeName() throws Exception {
+ assertThat(metrics.sanitizeMetricName("metric/submetric1/submetric2/submetric3"))
+ .isEqualTo("metric/submetric1/submetric2/submetric3");
+ }
+
+ @Test
+ public void shouldNotCreateSimpleCollisions() throws Exception {
+ String sanitizedCase1 = metrics.sanitizeMetricName("foo_bar");
+ String sanitizedCase2 = metrics.sanitizeMetricName("foo+bar");
+ assertThat(sanitizedCase1).isNotEqualTo(sanitizedCase2);
+ }
+
+ @Test
+ public void shouldDoubleTheReplacementPrefix() throws Exception {
+ assertThat(metrics.sanitizeMetricName("foo_0x_bar")).isEqualTo("foo_0x_0x_bar");
+ }
+
+ @Test
public void shouldSanitizeUnwantedChars() throws Exception {
assertThat(metrics.sanitizeMetricName("very+confusing$long#metric@net/name^1"))
- .isEqualTo("very_confusing_long_metric_net/name_1");
+ .isEqualTo("very_0x2B_confusing_0x24_long_0x23_metric_0x40_net/name_0x5E_1");
assertThat(metrics.sanitizeMetricName("/metric/submetric")).isEqualTo("_metric/submetric");
}
@Test
public void shouldReduceConsecutiveSlashesToOne() throws Exception {
- assertThat(metrics.sanitizeMetricName("/metric//submetric1///submetric2/submetric3"))
+ assertThat(metrics.sanitizeMetricName("/metric///submetric1///submetric2/submetric3"))
.isEqualTo("_metric/submetric1/submetric2/submetric3");
}
diff --git a/tools/release_noter/release_noter.py b/tools/release_noter/release_noter.py
index 167f68a..184e089 100755
--- a/tools/release_noter/release_noter.py
+++ b/tools/release_noter/release_noter.py
@@ -96,7 +96,8 @@
CHANGE_URL = "/c/gerrit/+/"
COMMIT_URL = "/changes/?q=commit%3A"
GERRIT_URL = "https://gerrit-review.googlesource.com"
-ISSUE_URL = "https://bugs.chromium.org/p/gerrit/issues/detail?id="
+ISSUE_URL_MONORAIL = "https://bugs.chromium.org/p/gerrit/issues/detail?id="
+ISSUE_URL_TRACKER = "https://issues.gerritcodereview.com/issues/"
MARKDOWN = "release_noter"
GIT_COMMAND = "git"
@@ -301,7 +302,10 @@
def print_from(this_change, md):
md.write("\n*")
for issue in sorted(this_change.issues):
- md.write(f" [Issue {issue}]({ISSUE_URL}{issue});\n ")
+ if len(issue) > 5:
+ md.write(f" [Issue {issue}]({ISSUE_URL_TRACKER}{issue});\n ")
+ else:
+ md.write(f" [Issue {issue}]({ISSUE_URL_MONORAIL}{issue});\n ")
md.write(f" {this_change.subject}\n")
