From 83eaad4232c1ece42959776d1591f17457a8df3a Mon Sep 17 00:00:00 2001 From: Borui Tao Date: Mon, 14 May 2018 11:34:13 -0400 Subject: [PATCH 01/36] AllChangesIndexer: Don't abort when failing to open repository Previously when opening project repositories during the reindexing process, the program would be aborted if it failed to open the repository. Instead, just log an error message and continue with remaining projects. Change-Id: I571dec2639f8f22bde0ec0460304a1f27f8d8fe9 --- .../google/gerrit/server/index/change/AllChangesIndexer.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java index 638ff4c78f..54d769f086 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java @@ -118,15 +118,13 @@ public class AllChangesIndexer extends SiteIndexer projects = new TreeSet<>(); int changeCount = 0; - Stopwatch sw = Stopwatch.createStarted(); for (Project.NameKey name : projectCache.all()) { try (Repository repo = repoManager.openRepository(name)) { int size = ChangeNotes.Factory.scan(repo).size(); changeCount += size; projects.add(new ProjectHolder(name, size)); } catch (IOException e) { - log.error("Error collecting projects", e); - return new Result(sw, false, 0, 0); + log.error("Error collecting changes for project {}", name, e); } pm.update(1); } From 594a4632e33b73d8510a2e0723be7d528650660b Mon Sep 17 00:00:00 2001 From: Eryk Szymanski Date: Wed, 16 May 2018 12:12:07 +0200 Subject: [PATCH 02/36] DropWizardMetricMaker: Improve error message when metric name is invalid In case the metric name is invalid, the stack trace only contains the required pattern but not the name itself. Add the metric name to the message so that the stack trace is more informative. Change-Id: If1750a46e7d1e513d3d372212b8a95c89f682da0 Signed-off-by: Eryk Szymanski --- .../gerrit/metrics/dropwizard/DropWizardMetricMaker.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) 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 118ca03129..1e075aa60a 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 @@ -339,7 +339,8 @@ public class DropWizardMetricMaker extends MetricMaker { private static void checkMetricName(String name) { checkArgument( METRIC_NAME_PATTERN.matcher(name).matches(), - "metric name must match %s", + "invalid metric name '%s': must match pattern '%s'", + name, METRIC_NAME_PATTERN.pattern()); } From 104ea59499ca76c2b1c583a68accc60ea95ec72d Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 16 May 2018 19:28:00 +0900 Subject: [PATCH 03/36] DropWizardMetricMaker: Improve error messages for invalid arguments - When the metric name is already defined, wrap the name in quotes in the error message. - Adjust error messages about invalid counter consistent to use singular for consitency with other similar error messages. Change-Id: If7298b5c6553f54b3422a83c613a888895cd7342 --- .../gerrit/metrics/dropwizard/DropWizardMetricMaker.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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 1e075aa60a..af5350e5a4 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 @@ -151,8 +151,8 @@ public class DropWizardMetricMaker extends MetricMaker { private static void checkCounterDescription(String name, Description desc) { checkMetricName(name); - checkArgument(!desc.isConstant(), "counters must not be constant"); - checkArgument(!desc.isGauge(), "counters must not be gauge"); + checkArgument(!desc.isConstant(), "counter must not be constant"); + checkArgument(!desc.isGauge(), "counter must not be gauge"); } CounterImpl newCounterImpl(String name, boolean isRate) { @@ -326,7 +326,7 @@ public class DropWizardMetricMaker extends MetricMaker { if (!desc.getAnnotations() .get(Description.DESCRIPTION) .equals(annotations.get(Description.DESCRIPTION))) { - throw new IllegalStateException(String.format("metric %s already defined", name)); + throw new IllegalStateException(String.format("metric '%s' already defined", name)); } } else { descriptions.put(name, desc.getAnnotations()); From 7d16b055e95bdbd9fb9ceb3b1e9440006689425a Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 16 May 2018 16:34:27 +0200 Subject: [PATCH 04/36] Remove some logs for errors that are rethrown "log and throw" is considered a poor practice. The logging is uneeded since the thrown exception will be logged somewhere else. Change-Id: I82c210fe1e053a4ffd7a58b7fc7de6056f9875a7 Signed-off-by: Edwin Kempin --- .../java/com/google/gerrit/gpg/server/PostGpgKeys.java | 1 - .../com/google/gerrit/pgm/http/jetty/JettyServer.java | 9 ++------- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java index 88d24ea94b..2d5da510d5 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java +++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java @@ -262,7 +262,6 @@ public class PostGpgKeys implements RestModifyView { msg.append("GPG key ").append(extIdKey.get()).append(" associated with multiple accounts: "); Joiner.on(", ") .appendTo(msg, Lists.transform(accountStates, AccountState.ACCOUNT_ID_FUNCTION)); - log.error(msg.toString()); throw new IllegalStateException(msg.toString()); } diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java index 18ddc86e76..0103aaecbf 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/JettyServer.java @@ -69,13 +69,9 @@ import org.eclipse.jetty.util.ssl.SslContextFactory; import org.eclipse.jetty.util.thread.QueuedThreadPool; import org.eclipse.jetty.util.thread.ThreadPool; import org.eclipse.jgit.lib.Config; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; @Singleton public class JettyServer { - private static final Logger log = LoggerFactory.getLogger(JettyServer.class); - static class Lifecycle implements LifecycleListener { private final JettyServer server; private final Config cfg; @@ -426,9 +422,8 @@ public class JettyServer { "/*", EnumSet.of(DispatcherType.REQUEST, DispatcherType.ASYNC)); } catch (Throwable e) { - String errorMessage = "Unable to instantiate front-end HTTP Filter " + filterClassName; - log.error(errorMessage, e); - throw new IllegalArgumentException(errorMessage, e); + throw new IllegalArgumentException( + "Unable to instantiate front-end HTTP Filter " + filterClassName, e); } } From 2311ef273ed4071697703f14a34f3f4b8f5e228d Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 16 May 2018 16:38:33 +0200 Subject: [PATCH 05/36] PostGpgKeys: Remove unneeded use of Joiner We can just rely on the toString formatting of the List. This makes the code more readable. Change-Id: I98f0da6b5180198b9fcf0e45271e12e9ab182a52 Signed-off-by: Edwin Kempin --- .../java/com/google/gerrit/gpg/server/PostGpgKeys.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java index 2d5da510d5..0e3fb971fd 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java +++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/PostGpgKeys.java @@ -259,9 +259,10 @@ public class PostGpgKeys implements RestModifyView { if (accountStates.size() > 1) { StringBuilder msg = new StringBuilder(); - msg.append("GPG key ").append(extIdKey.get()).append(" associated with multiple accounts: "); - Joiner.on(", ") - .appendTo(msg, Lists.transform(accountStates, AccountState.ACCOUNT_ID_FUNCTION)); + msg.append("GPG key ") + .append(extIdKey.get()) + .append(" associated with multiple accounts: ") + .append(Lists.transform(accountStates, AccountState.ACCOUNT_ID_FUNCTION)); throw new IllegalStateException(msg.toString()); } From 510dc555903874e01dab97581cf14ad7c198a417 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 16 May 2018 14:49:03 +0200 Subject: [PATCH 06/36] GitwebServlet: Log unexpected errors on error level Especially it's seems odd that copyStderrToLog logs the input on error level but failing to do so results only in a debug log. Change-Id: I52302be34e8f4a62639015acffe26d0a11a5c8da Signed-off-by: Edwin Kempin --- .../java/com/google/gerrit/httpd/gitweb/GitwebServlet.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java index 89a12684d7..8fcb8c6a01 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java @@ -635,7 +635,7 @@ class GitwebServlet extends HttpServlet { dst.close(); } } catch (IOException e) { - log.debug("Unexpected error copying input to CGI", e); + log.error("Unexpected error copying input to CGI", e); } }, "Gitweb-InputFeeder") @@ -652,7 +652,7 @@ class GitwebServlet extends HttpServlet { log.error("CGI: " + line); } } catch (IOException e) { - log.debug("Unexpected error copying stderr from CGI", e); + log.error("Unexpected error copying stderr from CGI", e); } }, "Gitweb-ErrorLogger") From 5fa97fcdd051b000dbd71f173e4e6a6c2aad903d Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 16 May 2018 15:07:57 +0200 Subject: [PATCH 07/36] GitwebServlet: Write only one log entry for CGI errors If a separate log statement is written for each line then the entries in the log file can be interleaved with log entries from other threads, which makes them less readable. Change-Id: I038a0d8bc906746cc23b8f3bfb31d6c4d98b53c0 Signed-off-by: Edwin Kempin --- .../java/com/google/gerrit/httpd/gitweb/GitwebServlet.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java index 8fcb8c6a01..c5907c932d 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/gitweb/GitwebServlet.java @@ -645,12 +645,17 @@ class GitwebServlet extends HttpServlet { private void copyStderrToLog(final InputStream in) { new Thread( () -> { + StringBuilder b = new StringBuilder(); try (BufferedReader br = new BufferedReader(new InputStreamReader(in, ISO_8859_1.name()))) { String line; while ((line = br.readLine()) != null) { - log.error("CGI: " + line); + if (b.length() > 0) { + b.append('\n'); + } + b.append("CGI: ").append(line); } + log.error(b.toString()); } catch (IOException e) { log.error("Unexpected error copying stderr from CGI", e); } From e70929836361bd7ab09dd2af933fffc24222c574 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 16 May 2018 14:43:38 +0200 Subject: [PATCH 08/36] BazelBuild: Fix exception message when command was interrupted We used ProcessBuilder#toString() but ProcessBuilder doesn't implement the toString() method, hence calling toString() would return the instance identity (e.g. 'java.lang.ProcessBuilder@4488aabb'). Instead include the command which is more useful. This issue was detected by ErrorProne. Change-Id: I780f21bbfeae8e7c1b3f4467d789bcf148293324 Signed-off-by: Edwin Kempin --- .../src/main/java/com/google/gerrit/httpd/raw/BazelBuild.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/BazelBuild.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/BazelBuild.java index 85453fbf53..f52792caf9 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/BazelBuild.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/BazelBuild.java @@ -17,6 +17,7 @@ package com.google.gerrit.httpd.raw; import static com.google.common.base.MoreObjects.firstNonNull; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.base.Joiner; import com.google.common.escape.Escaper; import com.google.common.html.HtmlEscapers; import com.google.common.io.ByteStreams; @@ -62,7 +63,8 @@ public class BazelBuild { try { status = rebuild.waitFor(); } catch (InterruptedException e) { - throw new InterruptedIOException("interrupted waiting for " + proc.toString()); + throw new InterruptedIOException( + "interrupted waiting for: " + Joiner.on(' ').join(proc.command())); } if (status != 0) { log.warn("build failed: " + new String(out, UTF_8)); From b492df1f56f4d8cf4b9f37674b4dd28bcc7f8c87 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 16 May 2018 16:00:59 +0200 Subject: [PATCH 09/36] ProjectBasicAuthFilter: Add comment why cause is not logged Change-Id: I5c738557f50034a8ed0bff3af750e698d7c52604 Signed-off-by: Edwin Kempin --- .../java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java index 57ec9c5182..335897642b 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/ProjectBasicAuthFilter.java @@ -164,6 +164,8 @@ class ProjectBasicAuthFilter implements Filter { rsp.sendError(SC_UNAUTHORIZED); return false; } catch (AuthenticationFailedException e) { + // This exception is thrown if the user provided wrong credentials, we don't need to log a + // stacktrace for it. log.warn("Authentication failed for " + username + ": " + e.getMessage()); rsp.sendError(SC_UNAUTHORIZED); return false; From 45b4c87a7c1340129c6b0f7397c5e41d5670e003 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 16 May 2018 15:20:16 +0200 Subject: [PATCH 10/36] VersionedAccountDestinations: Remove unused createSink(String) method Change-Id: I914c263e2f2855c6278887d23a6c582d6ee7b8ef Signed-off-by: Edwin Kempin --- .../gerrit/server/account/VersionedAccountDestinations.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java index 2bb4bb75a2..1e1e54a701 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java @@ -69,10 +69,6 @@ public class VersionedAccountDestinations extends VersionedMetaData { } } - public ValidationError.Sink createSink(String file) { - return ValidationError.createLoggerSink(file, log); - } - @Override protected boolean onSave(CommitBuilder commit) throws IOException, ConfigInvalidException { throw new UnsupportedOperationException("Cannot yet save destinations"); From d7a17e19a17fd5b1509a1648479ed240c1579968 Mon Sep 17 00:00:00 2001 From: Jacek Centkowski Date: Wed, 16 May 2018 12:15:04 +0200 Subject: [PATCH 11/36] DropWizardMetricMaker: Introduce method to sanitize metric name The new method sanitizeMetricName can be used to create metric names that confirm to the naming rules. Change-Id: Ic0074b53f7d25edfc4f366cc45f3ad3a47e25057 Signed-off-by: Jacek Centkowski Signed-off-by: David Pursehouse --- .../dropwizard/DropWizardMetricMaker.java | 19 +++++++++ .../dropwizard/DropWizardMetricMakerTest.java | 42 +++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java 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 af5350e5a4..37efb78701 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,6 +344,25 @@ public class DropWizardMetricMaker extends MetricMaker { METRIC_NAME_PATTERN.pattern()); } + public static String sanitizeMetricName(String input) { + if (METRIC_NAME_PATTERN.matcher(input).matches()) { + return input; + } + + String first = input.substring(0, 1).replaceFirst("[^\\w-]", "_"); + if (input.length() == 1) { + return first; + } + + String result = first + input.substring(1).replaceAll("/[/]+", "/").replaceAll("[^\\w-/]", "_"); + + if (result.endsWith("/")) { + result = result.substring(0, result.length() - 1); + } + + return result; + } + static String name(Description.FieldOrdering ordering, String codeName, String fieldValues) { if (ordering == FieldOrdering.PREFIX_FIELDS_BASENAME) { int s = codeName.lastIndexOf('/'); 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 new file mode 100644 index 0000000000..26b98c6008 --- /dev/null +++ b/gerrit-server/src/test/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java @@ -0,0 +1,42 @@ +// Copyright (C) 2018 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +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 { + @Test + public void shouldSanitizeUnwantedChars() throws Exception { + assertThat(sanitizeMetricName("very+confusing$long#metric@net/name^1")) + .isEqualTo("very_confusing_long_metric_net/name_1"); + assertThat(sanitizeMetricName("/metric/submetric")).isEqualTo("_metric/submetric"); + } + + @Test + public void shouldReduceConsecutiveSlashesToOne() throws Exception { + assertThat(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"); + } +} From fccd70ed65fd1003060b505070f5a48b93356b92 Mon Sep 17 00:00:00 2001 From: Jacek Centkowski Date: Wed, 16 May 2018 12:27:35 +0200 Subject: [PATCH 12/36] WorkQueue: Sanitize metric name when queue is created This is a follow up to I3f92b840b and makes sure that whatever metric is created from WorkQueue will conform to the naming rules. Change-Id: I332cb50b80bcf80333dad9b4105e599bbcdb01c4 Signed-off-by: Jacek Centkowski Signed-off-by: David Pursehouse --- .../src/main/java/com/google/gerrit/server/git/WorkQueue.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 dc6aa83244..07948483cd 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,6 +14,8 @@ 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; @@ -284,7 +286,7 @@ public class WorkQueue { CaseFormat.UPPER_CAMEL.to( CaseFormat.LOWER_UNDERSCORE, queueName.replaceFirst("SSH", "Ssh").replaceAll("-", "")); - return String.format("queue/%s/%s", name, metricName); + return sanitizeMetricName(String.format("queue/%s/%s", name, metricName)); } public void unregisterWorkQueue() { From 4e97e35b7c3423ff4a85a02d4b03722f903d6348 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 17 May 2018 08:17:59 +0900 Subject: [PATCH 13/36] WorkQueue: Don't fail when queue metric already exists It is possible to create mutiple queues with the same name, however the metric maker implementation (in this case dropwizard, but it may be the same case for other implementations) disallows to create a metric if one with the given name already exist. A specific case where a queue with the same name may be created is when a plugin creates a queue. When the plugin is reloaded, a queue with the same name gets created. Handle the exception raised by the metric maker and if it was due to the queue already existing, log a warning rather than failing. Change-Id: I74a0c8b8363ba3774b1cad0b0c27ad437271c81d --- .../java/com/google/gerrit/server/git/WorkQueue.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) 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 07948483cd..ffa22940f1 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 @@ -207,7 +207,15 @@ public class WorkQueue { corePoolSize + 4 // concurrency level ); queueName = prefix; - buildMetrics(queueName, metrics); + try { + buildMetrics(queueName, metrics); + } catch (IllegalArgumentException e) { + if (e.getMessage().contains("already")) { + log.warn("Not creating metrics for queue '{}': already exists", queueName); + } else { + throw e; + } + } } private void buildMetrics(String queueName, MetricMaker metric) { From ef899cc67a6b6207da335d5e1e567cf57c06814f Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 17 May 2018 13:40:51 +0900 Subject: [PATCH 14/36] Revert "AllChangesIndexer: Don't abort when failing to open repository" This was done differently on stable-2.15 with change I282ddae9a. Revert this so we can more easily backport the other implementation. This reverts commit 83eaad4232c1ece42959776d1591f17457a8df3a. Change-Id: I5300ba11dbe5da2dd34a70f3b57cb488bbc3a1ca --- .../google/gerrit/server/index/change/AllChangesIndexer.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java index 54d769f086..638ff4c78f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java @@ -118,13 +118,15 @@ public class AllChangesIndexer extends SiteIndexer projects = new TreeSet<>(); int changeCount = 0; + Stopwatch sw = Stopwatch.createStarted(); for (Project.NameKey name : projectCache.all()) { try (Repository repo = repoManager.openRepository(name)) { int size = ChangeNotes.Factory.scan(repo).size(); changeCount += size; projects.add(new ProjectHolder(name, size)); } catch (IOException e) { - log.error("Error collecting changes for project {}", name, e); + log.error("Error collecting projects", e); + return new Result(sw, false, 0, 0); } pm.update(1); } From 3cee96f94c0461a988feee3f8701162e6e65a865 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Sun, 11 Mar 2018 22:03:19 +0000 Subject: [PATCH 15/36] Do not abort indexing if < 50% projects failed Run the project reindexing even if some of the projects could not be loaded. It is more important to keep the reindexing operation and leave to the Gerrit admin the troubleshooting of the failing projects. Log as error the project name that failed to be collected for indexing. Change-Id: I282ddae9a3211470d3fc8d9d26045bf7a8d3ac5c --- .../gerrit/server/index/change/AllChangesIndexer.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java index 638ff4c78f..58969d717b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/AllChangesIndexer.java @@ -119,20 +119,24 @@ public class AllChangesIndexer extends SiteIndexer projects = new TreeSet<>(); int changeCount = 0; Stopwatch sw = Stopwatch.createStarted(); + int projectsFailed = 0; for (Project.NameKey name : projectCache.all()) { try (Repository repo = repoManager.openRepository(name)) { int size = ChangeNotes.Factory.scan(repo).size(); changeCount += size; projects.add(new ProjectHolder(name, size)); } catch (IOException e) { - log.error("Error collecting projects", e); - return new Result(sw, false, 0, 0); + log.error("Error collecting project {}", name, e); + projectsFailed++; + if (projectsFailed > projects.size() / 2) { + log.error("Over 50% of the projects could not be collected: aborted"); + return new Result(sw, false, 0, 0); + } } pm.update(1); } pm.endTask(); setTotalWork(changeCount); - return indexAll(index, projects); } From 7d1dfc7bc728f304c41804ef2ffab08e8fb01952 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 17 May 2018 14:19:34 +0900 Subject: [PATCH 16/36] ProjectConfig: Don't use JGit's StringUtils to convert to lower case Instead use Java's String built-in toLowerCase method. Change-Id: Ic5bd099356b7ad08b6ba70d25cfb1043e84c272e --- .../main/java/com/google/gerrit/server/git/ProjectConfig.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java index e3299d9c7f..7b90408735 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ProjectConfig.java @@ -65,6 +65,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashMap; import java.util.List; +import java.util.Locale; import java.util.Map; import java.util.Map.Entry; import java.util.Set; @@ -75,7 +76,6 @@ import org.eclipse.jgit.lib.CommitBuilder; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.transport.RefSpec; -import org.eclipse.jgit.util.StringUtils; public class ProjectConfig extends VersionedMetaData implements ValidationError.Sink { public static final String COMMENTLINK = "commentlink"; @@ -1181,7 +1181,7 @@ public class ProjectConfig extends VersionedMetaData implements ValidationError. List types = Lists.newArrayListWithCapacity(4); for (NotifyType t : NotifyType.values()) { if (nc.isNotify(t)) { - types.add(StringUtils.toLowerCase(t.name())); + types.add(t.name().toLowerCase(Locale.US)); } } rc.setStringList(NOTIFY, nc.getName(), KEY_TYPE, types); From fcbdca1e3019c06f7d41f7ae179304a93f9161f1 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 17 May 2018 14:23:35 +0900 Subject: [PATCH 17/36] HttpPluginServlet: Don't trim leading whitespace from about.md content Trimming leading whitespace prevents preformatted code blocks from being rendered properly. Change-Id: I97f0fab63d128a11320cabe9e13ff5b6e80fc139 --- gerrit-httpd/BUILD | 1 + .../com/google/gerrit/httpd/plugins/HttpPluginServlet.java | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/gerrit-httpd/BUILD b/gerrit-httpd/BUILD index 9b1b610e03..a5494bcf9d 100644 --- a/gerrit-httpd/BUILD +++ b/gerrit-httpd/BUILD @@ -40,6 +40,7 @@ java_library( "//lib:soy", "//lib/auto:auto-value", "//lib/commons:codec", + "//lib/commons:lang", "//lib/guice", "//lib/guice:guice-assistedinject", "//lib/guice:guice-servlet", diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java index e5b3d86f47..55fa0cb29e 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java @@ -87,6 +87,7 @@ import javax.servlet.ServletResponse; import javax.servlet.http.HttpServlet; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.apache.commons.lang.StringUtils; import org.eclipse.jgit.lib.Config; import org.eclipse.jgit.util.IO; import org.eclipse.jgit.util.RawParseUtils; @@ -477,7 +478,7 @@ class HttpPluginServlet extends HttpServlet implements StartPluginListener, Relo try (BufferedReader reader = new BufferedReader(isr)) { String line; while ((line = reader.readLine()) != null) { - line = line.trim(); + line = StringUtils.stripEnd(line, null); if (line.isEmpty()) { aboutContent.append("\n"); } else { From 702463fcda6ad64918a6cad9f1190335375f2b96 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 16 May 2018 16:24:20 +0200 Subject: [PATCH 18/36] OperatingSystemMXBeanProvider: Log exception for ReflectiveOperationException Having the stacktrace when debugging this seems to be useful. Change-Id: Ifa298ffb74eabdf0c6522c9831c533dd2ebd02eb Signed-off-by: Edwin Kempin --- .../gerrit/metrics/proc/OperatingSystemMXBeanProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/metrics/proc/OperatingSystemMXBeanProvider.java b/gerrit-server/src/main/java/com/google/gerrit/metrics/proc/OperatingSystemMXBeanProvider.java index 7256e8cb37..bc2846a200 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/metrics/proc/OperatingSystemMXBeanProvider.java +++ b/gerrit-server/src/main/java/com/google/gerrit/metrics/proc/OperatingSystemMXBeanProvider.java @@ -41,7 +41,7 @@ class OperatingSystemMXBeanProvider { return new OperatingSystemMXBeanProvider(sys); } } catch (ReflectiveOperationException e) { - log.debug(String.format("No implementation for %s: %s", name, e.getMessage())); + log.debug("No implementation for {}", name, e); } } log.warn("No implementation of UnixOperatingSystemMXBean found"); From 7f5212239678b73b918cb2e458eaf1b541322234 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 16 May 2018 16:12:25 +0200 Subject: [PATCH 19/36] LdapLoginServlet: Improve exception handling * Handle AuthenticationFailedException separately: This exception is a subclass of AccountException that is thrown if the user provides wrong credentials. For this exception we want to return "Invalid username or password." as message to the client. * Return a more general message for other AccountExceptions: Likely they are not caused by invalid username or password since this would cause a AuthenticationFailedException which we handle before. * Increase log level to warning: This is the log level that we use for these exceptions in other places (e.g. ProjectBasicAuthFilter). Make it consistent. * Log the stacktrace for AccountExceptions: We do this everywhere else (e.g. ProjectBasicAuthFilter, HttpLoginServlet). Make it consistent. Change-Id: Ie34687d087b5a6cd102bf8cebd0f9830f54c9c1c Signed-off-by: Edwin Kempin --- .../gerrit/httpd/auth/ldap/LdapLoginServlet.java | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java index 24ba4ac114..46714756fb 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/auth/ldap/LdapLoginServlet.java @@ -30,6 +30,7 @@ import com.google.gerrit.server.account.AccountManager; import com.google.gerrit.server.account.AccountUserNameException; import com.google.gerrit.server.account.AuthRequest; import com.google.gerrit.server.account.AuthResult; +import com.google.gerrit.server.account.AuthenticationFailedException; import com.google.gerrit.server.auth.AuthenticationUnavailableException; import com.google.gwtexpui.server.CacheHeaders; import com.google.inject.Inject; @@ -126,10 +127,16 @@ class LdapLoginServlet extends HttpServlet { } catch (AuthenticationUnavailableException e) { sendForm(req, res, "Authentication unavailable at this time."); return; - } catch (AccountException e) { - log.info(String.format("'%s' failed to sign in: %s", username, e.getMessage())); + } catch (AuthenticationFailedException e) { + // This exception is thrown if the user provided wrong credentials, we don't need to log a + // stacktrace for it. + log.warn("'{}' failed to sign in: {}", username, e.getMessage()); sendForm(req, res, "Invalid username or password."); return; + } catch (AccountException e) { + log.warn("'{}' failed to sign in", username, e); + sendForm(req, res, "Authentication failed."); + return; } catch (RuntimeException e) { log.error("LDAP authentication failed", e); sendForm(req, res, "Authentication unavailable at this time."); From ba6fd0f3b117108057a7bdc3288704f8ee1c04bd Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Wed, 16 May 2018 15:34:33 +0200 Subject: [PATCH 20/36] Remove ValidationError#createLoggerSink to avoid passing around loggers Passing loggers between classes is bad and we should avoid this if possible. ValidationError#createLoggerSink required to pass in a logger just to return it back to the caller wrapped in a lambda. Remove this method and instead create the lambda directly in the caller. This brings us minimal code duplication for formatting the message, but it's better than passing the logger around. Change-Id: I6b475de50fb2a1745c16ac424e0a95d358aaaf7d Signed-off-by: Edwin Kempin --- .../pgm/init/api/AllProjectsConfig.java | 3 +-- .../account/VersionedAccountDestinations.java | 8 +++---- .../account/VersionedAccountQueries.java | 4 +++- .../com/google/gerrit/server/git/TabFile.java | 5 ---- .../gerrit/server/git/ValidationError.java | 24 ++++++++++++------- 5 files changed, 24 insertions(+), 20 deletions(-) diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java index 349cd056b0..8c013edf80 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/init/api/AllProjectsConfig.java @@ -30,7 +30,6 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; public class AllProjectsConfig extends VersionedMetaDataOnInit { - private static final Logger log = LoggerFactory.getLogger(AllProjectsConfig.class); private Config cfg; @@ -65,7 +64,7 @@ public class AllProjectsConfig extends VersionedMetaDataOnInit { return GroupList.parse( new Project.NameKey(project), readUTF8(GroupList.FILE_NAME), - GroupList.createLoggerSink(GroupList.FILE_NAME, log)); + error -> log.error("Error parsing file {}: {}", GroupList.FILE_NAME, error.getMessage())); } public void save(String pluginName, String message) throws IOException, ConfigInvalidException { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java index 1e1e54a701..7808edda43 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountDestinations.java @@ -17,8 +17,6 @@ package com.google.gerrit.server.account; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.server.git.DestinationList; -import com.google.gerrit.server.git.TabFile; -import com.google.gerrit.server.git.ValidationError; import com.google.gerrit.server.git.VersionedMetaData; import java.io.IOException; import org.eclipse.jgit.errors.ConfigInvalidException; @@ -62,8 +60,10 @@ public class VersionedAccountDestinations extends VersionedMetaData { String path = p.path; if (path.startsWith(prefix)) { String label = path.substring(prefix.length()); - ValidationError.Sink errors = TabFile.createLoggerSink(path, log); - destinations.parseLabel(label, readUTF8(path), errors); + destinations.parseLabel( + label, + readUTF8(path), + error -> log.error("Error parsing file {}: {}", path, error.getMessage())); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountQueries.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountQueries.java index e1fa8d6b69..af0463a3fe 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountQueries.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/VersionedAccountQueries.java @@ -52,7 +52,9 @@ public class VersionedAccountQueries extends VersionedMetaData { protected void onLoad() throws IOException, ConfigInvalidException { queryList = QueryList.parse( - readUTF8(QueryList.FILE_NAME), QueryList.createLoggerSink(QueryList.FILE_NAME, log)); + readUTF8(QueryList.FILE_NAME), + error -> + log.error("Error parsing file {}: {}", QueryList.FILE_NAME, error.getMessage())); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/TabFile.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/TabFile.java index ea041f4f3b..c4179651be 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/TabFile.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/TabFile.java @@ -23,7 +23,6 @@ import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; -import org.slf4j.Logger; public class TabFile { public interface Parser { @@ -145,8 +144,4 @@ public class TabFile { } return r.toString(); } - - public static ValidationError.Sink createLoggerSink(String file, Logger log) { - return ValidationError.createLoggerSink("Error parsing file " + file + ": ", log); - } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ValidationError.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ValidationError.java index 41851417f0..28d5171945 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ValidationError.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ValidationError.java @@ -14,7 +14,7 @@ package com.google.gerrit.server.git; -import org.slf4j.Logger; +import java.util.Objects; /** Indicates a problem with Git based data. */ public class ValidationError { @@ -45,12 +45,20 @@ public class ValidationError { void error(ValidationError error); } - public static Sink createLoggerSink(final String message, final Logger log) { - return new ValidationError.Sink() { - @Override - public void error(ValidationError error) { - log.error(message + error.getMessage()); - } - }; + @Override + public boolean equals(Object o) { + if (o == this) { + return true; + } + if (o instanceof ValidationError) { + ValidationError that = (ValidationError) o; + return Objects.equals(this.message, that.message); + } + return false; + } + + @Override + public int hashCode() { + return Objects.hashCode(message); } } From 480c638ba31bad68ef289f7618b42f88d8cb6a54 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 17 May 2018 08:15:26 +0200 Subject: [PATCH 21/36] ldap.Helper: Use local logger and make logger in LdapRealm private Loggers should not be shared between classes. Change-Id: I043df22de41b00afa93e1c1596b19576d2a3c3dc Signed-off-by: Edwin Kempin --- .../google/gerrit/server/auth/ldap/Helper.java | 16 ++++++++++------ .../gerrit/server/auth/ldap/LdapRealm.java | 3 ++- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java index 1acd647d33..d2499c0688 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/Helper.java @@ -52,9 +52,13 @@ import javax.security.auth.Subject; import javax.security.auth.login.LoginContext; import javax.security.auth.login.LoginException; import org.eclipse.jgit.lib.Config; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; @Singleton class Helper { + private static final Logger log = LoggerFactory.getLogger(Helper.class); + static final String LDAP_UUID = "ldap:"; private final Cache> parentGroups; @@ -151,7 +155,7 @@ class Helper { } catch (PrivilegedActionException e) { Throwables.throwIfInstanceOf(e.getException(), NamingException.class); Throwables.throwIfInstanceOf(e.getException(), RuntimeException.class); - LdapRealm.log.warn("Internal error", e.getException()); + log.warn("Internal error", e.getException()); return null; } finally { ctx.logout(); @@ -297,7 +301,7 @@ class Helper { } } } catch (NamingException e) { - LdapRealm.log.warn("Could not find group " + groupDN, e); + log.warn("Could not find group {}", groupDN, e); } cachedParentsDNs = dns.build(); parentGroups.put(groupDN, cachedParentsDNs); @@ -430,10 +434,10 @@ class Helper { try { return LdapType.guessType(ctx); } catch (NamingException e) { - LdapRealm.log.warn( - "Cannot discover type of LDAP server at " - + server - + ", assuming the server is RFC 2307 compliant.", + log.warn( + "Cannot discover type of LDAP server at {}," + + " assuming the server is RFC 2307 compliant.", + server, e); return LdapType.RFC_2307; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java index 66b279f945..49ff6e85f5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java @@ -59,7 +59,8 @@ import org.slf4j.LoggerFactory; @Singleton class LdapRealm extends AbstractRealm { - static final Logger log = LoggerFactory.getLogger(LdapRealm.class); + private static final Logger log = LoggerFactory.getLogger(LdapRealm.class); + static final String LDAP = "com.sun.jndi.ldap.LdapCtxFactory"; static final String USERNAME = "username"; From d0e0309b89f97004b419e41f7d0236eccf110fea Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 17 May 2018 11:53:38 +0900 Subject: [PATCH 22/36] Update git submodules * Update plugins/hooks from branch 'stable-2.14' to 0c4a90f7ca3479cb4a4e58d06421ad001f83d500 - Annotate RefUpdate and CommitReceived as Singleton These should be singletons so that they are not instantiated every time a hook is executed. Change-Id: I32e67a85c24138148c1cff97d76144030942930e - Reword documentation of ref-update and commit-received hooks There has been confusion about the purpose of these hooks. Rewrite the documentation to hopefully make it clearer. Change-Id: I86c80072d2d9f05c9d926f0a6a5014590af45b05 - Improve hooks documentation structure Split synchronous and asyncronous hooks into separate sections, and move the duplicated paragraphs about syncronous hook behavior into the top of the syncronous hooks section. Replace legacy section heading formatting (===== and ----- forms), which only supports 2 levels, with newer formatting (starting the line with one or more # marks) which supports further levels. Add a table of contents. Change-Id: Iacf7c44860ec4d77e44c9816fc04ea641a2c8b43 --- plugins/hooks | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/hooks b/plugins/hooks index 1370e0d678..0c4a90f7ca 160000 --- a/plugins/hooks +++ b/plugins/hooks @@ -1 +1 @@ -Subproject commit 1370e0d678579efe09678d33b3ee26c6f9428dac +Subproject commit 0c4a90f7ca3479cb4a4e58d06421ad001f83d500 From 6a5d6ed03ec96b4b187013fa956acf5966d44d4c Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 16 May 2018 16:12:52 +0900 Subject: [PATCH 23/36] Update git submodules * Update plugins/hooks from branch 'stable-2.14' to 0fe206dea9f652c3b087fb65c5efc4ac3f0e370f - Add submit hook The submit hook is invoked synchronously when a change is submitted. If it returns a non-zero exit status, a MergeValidationException is thrown and the submit is prevented. This adds back the ability to block submit by a hook. Previously this was possible with the ref-update hook until its purpose was changed and only invoked on ref updates such as branch creation, deletion, or fast- forward by direct push. Change-Id: Ie4efb90df645ecac01638b23305dc2ffb547192e - Module: Bind listeners in alphabetical order Most of them are already in alphabetical order. Reorder the couple that aren't. Change-Id: I0722c357992bf855baa0a9b8997078036f28cdc8 --- plugins/hooks | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/hooks b/plugins/hooks index 0c4a90f7ca..0fe206dea9 160000 --- a/plugins/hooks +++ b/plugins/hooks @@ -1 +1 @@ -Subproject commit 0c4a90f7ca3479cb4a4e58d06421ad001f83d500 +Subproject commit 0fe206dea9f652c3b087fb65c5efc4ac3f0e370f From e1153ded6021b24c09b9e31d93e427dc9b695ff1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Thu, 17 May 2018 08:55:30 -0400 Subject: [PATCH 24/36] Update git submodules * Update plugins/hooks from branch 'stable-2.15' to 01c3fb71412e2b0a532e7f9cae389765a8e982ca - Merge branch 'stable-2.14' into stable-2.15 * stable-2.14: Add submit hook Module: Bind listeners in alphabetical order Annotate RefUpdate and CommitReceived as Singleton Reword documentation of ref-update and commit-received hooks Improve hooks documentation structure Change-Id: I5f5eed398e182c8542b4b53722d942fb2b6bde5a - Add submit hook The submit hook is invoked synchronously when a change is submitted. If it returns a non-zero exit status, a MergeValidationException is thrown and the submit is prevented. This adds back the ability to block submit by a hook. Previously this was possible with the ref-update hook until its purpose was changed and only invoked on ref updates such as branch creation, deletion, or fast- forward by direct push. Change-Id: Ie4efb90df645ecac01638b23305dc2ffb547192e - Module: Bind listeners in alphabetical order Most of them are already in alphabetical order. Reorder the couple that aren't. Change-Id: I0722c357992bf855baa0a9b8997078036f28cdc8 - Annotate RefUpdate and CommitReceived as Singleton These should be singletons so that they are not instantiated every time a hook is executed. Change-Id: I32e67a85c24138148c1cff97d76144030942930e - Reword documentation of ref-update and commit-received hooks There has been confusion about the purpose of these hooks. Rewrite the documentation to hopefully make it clearer. Change-Id: I86c80072d2d9f05c9d926f0a6a5014590af45b05 - Improve hooks documentation structure Split synchronous and asyncronous hooks into separate sections, and move the duplicated paragraphs about syncronous hook behavior into the top of the syncronous hooks section. Replace legacy section heading formatting (===== and ----- forms), which only supports 2 levels, with newer formatting (starting the line with one or more # marks) which supports further levels. Add a table of contents. Change-Id: Iacf7c44860ec4d77e44c9816fc04ea641a2c8b43 --- plugins/hooks | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/hooks b/plugins/hooks index e51f704687..01c3fb7141 160000 --- a/plugins/hooks +++ b/plugins/hooks @@ -1 +1 @@ -Subproject commit e51f704687f7aa44b448b5af62da3cabfab8a062 +Subproject commit 01c3fb71412e2b0a532e7f9cae389765a8e982ca From 65a1ae50eb8af57b1e29e8bda34d25f736968869 Mon Sep 17 00:00:00 2001 From: Borui Tao Date: Fri, 4 May 2018 15:11:48 -0400 Subject: [PATCH 25/36] ChangeQueryBuilder: Allow ownerin predicate to be evaluated by the index The previous implementation of ownerin predicate was not efficient as it first found all the matched changes and then checked the membership of the owner for each one of them. This change modifies this predicate such that for each account in the group, it finds all the matched changes that belong to that account, which can be evaluated by the index. Note that for external groups (e.g., ldap groups), it falls back to the previous implementation of this ownerin predicate, as the account lists of external groups are not available in the index. Bug: Issue 8916 Change-Id: I57b0a33c824f103d8b0e5e533af30e7cb23aa41d --- .../query/change/ChangeQueryBuilder.java | 54 ++++++++++++------- 1 file changed, 36 insertions(+), 18 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index 17f52dce30..63ec45c174 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.primitives.Ints; +import com.google.gerrit.common.data.GroupDescription; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.common.errors.NotSignedInException; @@ -742,23 +743,8 @@ public class ChangeQueryBuilder extends QueryBuilder { } } - // expand a group predicate into multiple user predicates if (group != null) { - Set allMembers = - args.listMembers - .get() - .setRecursive(true) - .apply(group) - .stream() - .map(a -> new Account.Id(a._accountId)) - .collect(toSet()); - int maxTerms = args.indexConfig.maxTerms(); - if (allMembers.size() > maxTerms) { - // limit the number of query terms otherwise Gerrit will barf - accounts = ImmutableSet.copyOf(Iterables.limit(allMembers, maxTerms)); - } else { - accounts = allMembers; - } + accounts = getMembers(group); } // If the vote piece looks like Code-Review=NEED with a valid non-numeric @@ -923,12 +909,24 @@ public class ChangeQueryBuilder extends QueryBuilder { } @Operator - public Predicate ownerin(String group) throws QueryParseException { + public Predicate ownerin(String group) throws QueryParseException, OrmException { GroupReference g = GroupBackends.findBestSuggestion(args.groupBackend, group); if (g == null) { throw error("Group " + group + " not found"); } - return new OwnerinPredicate(args.userFactory, g.getUUID()); + + AccountGroup.UUID groupId = g.getUUID(); + GroupDescription.Basic groupDescription = args.groupBackend.get(groupId); + if (!(groupDescription instanceof GroupDescription.Internal)) { + return new OwnerinPredicate(args.userFactory, groupId); + } + + Set accounts = getMembers(groupId); + List p = Lists.newArrayListWithCapacity(accounts.size()); + for (Account.Id id : accounts) { + p.add(new OwnerPredicate(id)); + } + return Predicate.or(p); } @Operator @@ -1134,6 +1132,26 @@ public class ChangeQueryBuilder extends QueryBuilder { return Predicate.or(predicates); } + private Set getMembers(AccountGroup.UUID g) throws OrmException { + Set accounts; + Set allMembers = + args.listMembers + .get() + .setRecursive(true) + .apply(g) + .stream() + .map(a -> new Account.Id(a._accountId)) + .collect(toSet()); + int maxTerms = args.indexConfig.maxTerms(); + if (allMembers.size() > maxTerms) { + // limit the number of query terms otherwise Gerrit will barf + accounts = ImmutableSet.copyOf(Iterables.limit(allMembers, maxTerms)); + } else { + accounts = allMembers; + } + return accounts; + } + private Set parseAccount(String who) throws QueryParseException, OrmException { if ("self".equals(who)) { return Collections.singleton(self()); From fe270ddd54c0f4c2d20edc74fa1e890080bd0630 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 18 May 2018 19:58:48 +0900 Subject: [PATCH 26/36] WorkQueue.Executor#buildMetrics: Remove MetricMaker parameter It's not necessary to pass a MetricMaker; the member variable can be used. Change-Id: I466ac5e9a6d18d14f91d6bb81448755b1dfdf7fc --- .../com/google/gerrit/server/git/WorkQueue.java | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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 ffa22940f1..3b8e4c441d 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 @@ -208,7 +208,7 @@ public class WorkQueue { ); queueName = prefix; try { - buildMetrics(queueName, metrics); + buildMetrics(queueName); } catch (IllegalArgumentException e) { if (e.getMessage().contains("already")) { log.warn("Not creating metrics for queue '{}': already exists", queueName); @@ -218,8 +218,8 @@ public class WorkQueue { } } - private void buildMetrics(String queueName, MetricMaker metric) { - metric.newCallbackMetric( + private void buildMetrics(String queueName) { + metrics.newCallbackMetric( getMetricName(queueName, "max_pool_size"), Long.class, new Description("Maximum allowed number of threads in the pool") @@ -231,7 +231,7 @@ public class WorkQueue { return (long) getMaximumPoolSize(); } }); - metric.newCallbackMetric( + metrics.newCallbackMetric( getMetricName(queueName, "pool_size"), Long.class, new Description("Current number of threads in the pool").setGauge().setUnit("threads"), @@ -241,7 +241,7 @@ public class WorkQueue { return (long) getPoolSize(); } }); - metric.newCallbackMetric( + metrics.newCallbackMetric( getMetricName(queueName, "active_threads"), Long.class, new Description("Number number of threads that are actively executing tasks") @@ -253,7 +253,7 @@ public class WorkQueue { return (long) getActiveCount(); } }); - metric.newCallbackMetric( + metrics.newCallbackMetric( getMetricName(queueName, "scheduled_tasks"), Integer.class, new Description("Number of scheduled tasks in the queue").setGauge().setUnit("tasks"), @@ -263,7 +263,7 @@ public class WorkQueue { return getQueue().size(); } }); - metric.newCallbackMetric( + metrics.newCallbackMetric( getMetricName(queueName, "total_scheduled_tasks_count"), Long.class, new Description("Total number of tasks that have been scheduled for execution") @@ -275,7 +275,7 @@ public class WorkQueue { return (long) getTaskCount(); } }); - metric.newCallbackMetric( + metrics.newCallbackMetric( getMetricName(queueName, "total_completed_tasks_count"), Long.class, new Description("Total number of tasks that have completed execution") From 8f708b8cc06fac3607ac13d522ac8e957a165735 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Fri, 18 May 2018 19:38:31 +0900 Subject: [PATCH 27/36] Update git submodules * Update plugins/hooks from branch 'stable-2.14' to 534cd3254c7e43648d36043b0d3f48ab4ae34a27 - HookQueue: Rename constructor 'queue' argument to avoid confusion The `queue` argument gets assigned to the `workQueue` member, but there is also a `queue` member. Rename the argument to `workQueue` to match the member that it gets assigned to, and prevent possible confusion with the other. Change-Id: I3fc278e096028f06f0f91b5551e6e435c66fff93 --- plugins/hooks | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/hooks b/plugins/hooks index 0fe206dea9..534cd3254c 160000 --- a/plugins/hooks +++ b/plugins/hooks @@ -1 +1 @@ -Subproject commit 0fe206dea9f652c3b087fb65c5efc4ac3f0e370f +Subproject commit 534cd3254c7e43648d36043b0d3f48ab4ae34a27 From a50ded0aae2bfa08e3df55124d542a738e7aac78 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 21 May 2018 08:28:53 +0900 Subject: [PATCH 28/36] Update git submodules * Update plugins/hooks from branch 'stable-2.15' to db007bb49807cc39080753075efce31a5df54a57 - Merge branch 'stable-2.14' into stable-2.15 * stable-2.14: HookQueue: Rename constructor 'queue' argument to avoid confusion Change-Id: I7d4911d2a77ce1726ef243a42ec9e54d420cf5ad - HookQueue: Rename constructor 'queue' argument to avoid confusion The `queue` argument gets assigned to the `workQueue` member, but there is also a `queue` member. Rename the argument to `workQueue` to match the member that it gets assigned to, and prevent possible confusion with the other. Change-Id: I3fc278e096028f06f0f91b5551e6e435c66fff93 --- plugins/hooks | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/hooks b/plugins/hooks index 01c3fb7141..db007bb498 160000 --- a/plugins/hooks +++ b/plugins/hooks @@ -1 +1 @@ -Subproject commit 01c3fb71412e2b0a532e7f9cae389765a8e982ca +Subproject commit db007bb49807cc39080753075efce31a5df54a57 From 3912a9d5ab66211bc22e4b7998752b72404ab3aa Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 21 May 2018 12:45:53 +0900 Subject: [PATCH 29/36] GroupCacheImpl: Fix log message when UUID is not found Change-Id: Ia20c8bfdba6bbf6469acc770c4a415b89e0a14ed --- .../java/com/google/gerrit/server/account/GroupCacheImpl.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java index 5c8e3e9f3c..effaa8dbc4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java @@ -144,7 +144,7 @@ public class GroupCacheImpl implements GroupCache { try { return byUUID.get(uuid.get()).orElse(null); } catch (ExecutionException e) { - log.warn(String.format("Cannot lookup group %s by name", uuid.get()), e); + log.warn(String.format("Cannot lookup group %s by uuid", uuid.get()), e); return null; } } From 5872780a5a321a9b29d645bc6e82fd8f29e40e9c Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 10 May 2018 17:17:12 +0100 Subject: [PATCH 30/36] Fix permissions checks on Gerrit API on current user When the current user is using GerritApi, a new user object with the same user-id of CurrentUser gets created on-the-fly. Even though the user accountId is the same, the user instances are different and will fail all the REST API permissions checks. Turn instance checks with accountId checks to allow user to execute GerritApi on themselves without the requirement of being Gerrit administrators. NOTE: GerritApi are mostly used in plugins, so this change allows other plugins to function properly. Change-Id: Iaeb204dda3791eb2757d89fe6bce6994c6305e04 --- .../java/com/google/gerrit/gpg/server/GpgKeys.java | 2 +- .../java/com/google/gerrit/server/CurrentUser.java | 13 +++++++++++++ .../com/google/gerrit/server/IdentifiedUser.java | 5 +++++ .../com/google/gerrit/server/account/AddSshKey.java | 3 ++- .../google/gerrit/server/account/Capabilities.java | 3 ++- .../google/gerrit/server/account/CreateEmail.java | 3 ++- .../google/gerrit/server/account/DeleteEmail.java | 3 ++- .../gerrit/server/account/DeleteExternalIds.java | 2 +- .../google/gerrit/server/account/DeleteSshKey.java | 3 ++- .../server/account/DeleteWatchedProjects.java | 3 ++- .../gerrit/server/account/GetCapabilities.java | 3 ++- .../gerrit/server/account/GetDiffPreferences.java | 3 ++- .../gerrit/server/account/GetEditPreferences.java | 3 ++- .../gerrit/server/account/GetExternalIds.java | 2 +- .../google/gerrit/server/account/GetOAuthToken.java | 2 +- .../gerrit/server/account/GetPreferences.java | 3 ++- .../google/gerrit/server/account/GetSshKeys.java | 3 ++- .../gerrit/server/account/GetWatchedProjects.java | 3 ++- .../com/google/gerrit/server/account/Index.java | 3 ++- .../gerrit/server/account/PostWatchedProjects.java | 3 ++- .../google/gerrit/server/account/PutAgreement.java | 2 +- .../gerrit/server/account/PutHttpPassword.java | 6 ++++-- .../com/google/gerrit/server/account/PutName.java | 3 ++- .../google/gerrit/server/account/PutPreferred.java | 3 ++- .../com/google/gerrit/server/account/PutStatus.java | 3 ++- .../google/gerrit/server/account/PutUsername.java | 3 ++- .../gerrit/server/account/SetDiffPreferences.java | 3 ++- .../gerrit/server/account/SetEditPreferences.java | 3 ++- .../gerrit/server/account/SetPreferences.java | 3 ++- .../com/google/gerrit/server/account/SshKeys.java | 3 ++- .../gerrit/server/account/StarredChanges.java | 6 +++--- .../com/google/gerrit/server/account/Stars.java | 6 +++--- 32 files changed, 77 insertions(+), 35 deletions(-) diff --git a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/GpgKeys.java b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/GpgKeys.java index 819ad9641f..9f2acd2464 100644 --- a/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/GpgKeys.java +++ b/gerrit-gpg/src/main/java/com/google/gerrit/gpg/server/GpgKeys.java @@ -219,7 +219,7 @@ public class GpgKeys implements ChildCollection { if (!BouncyCastleUtil.havePGP()) { throw new ResourceNotFoundException("GPG not enabled"); } - if (self.get() != rsrc.getUser()) { + if (!self.get().hasSameAccountId(rsrc.getUser())) { throw new ResourceNotFoundException(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java index 029b54d4ae..c6f10d2cc5 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/CurrentUser.java @@ -158,4 +158,17 @@ public abstract class CurrentUser { public ExternalId.Key getLastLoginExternalIdKey() { return get(lastLoginExternalIdPropertyKey); } + + /** + * Checks if the current user has the same account id of another. + * + *

Provide a generic interface for allowing subclasses to define whether two accounts represent + * the same account id. + * + * @param other user to compare + * @return true if the two users have the same account id + */ + public boolean hasSameAccountId(CurrentUser other) { + return false; + } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java index 2c4c61ca0e..41b7c67717 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/IdentifiedUser.java @@ -498,6 +498,11 @@ public class IdentifiedUser extends CurrentUser { realUser); } + @Override + public boolean hasSameAccountId(CurrentUser other) { + return getAccountId().get() == other.getAccountId().get(); + } + private String guessHost() { String host = null; SocketAddress remotePeer = null; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/AddSshKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/AddSshKey.java index 8c10c737c3..44b632af2e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/AddSshKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/AddSshKey.java @@ -69,7 +69,8 @@ public class AddSshKey implements RestModifyView { @Override public Response apply(AccountResource rsrc, Input input) throws AuthException, BadRequestException, OrmException, IOException, ConfigInvalidException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canAdministrateServer()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("not allowed to add SSH keys"); } return apply(rsrc.getUser(), input); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/Capabilities.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/Capabilities.java index d35656c46d..e53b7d000a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/Capabilities.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/Capabilities.java @@ -51,7 +51,8 @@ class Capabilities implements ChildCollection throws AuthException, BadRequestException, ResourceConflictException, ResourceNotFoundException, OrmException, EmailException, MethodNotAllowedException, IOException, ConfigInvalidException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canModifyAccount()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canModifyAccount()) { throw new AuthException("not allowed to add email address"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteEmail.java index bcbf794899..79edaa786a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteEmail.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteEmail.java @@ -60,7 +60,8 @@ public class DeleteEmail implements RestModifyView public Response apply(AccountResource.Email rsrc, Input input) throws AuthException, ResourceNotFoundException, ResourceConflictException, MethodNotAllowedException, OrmException, IOException, ConfigInvalidException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canModifyAccount()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canModifyAccount()) { throw new AuthException("not allowed to delete email address"); } return apply(rsrc.getUser(), rsrc.getEmail()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteExternalIds.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteExternalIds.java index 42726dcff4..7ab8aaf197 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteExternalIds.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteExternalIds.java @@ -60,7 +60,7 @@ public class DeleteExternalIds implements RestModifyView apply(AccountResource resource, List externalIds) throws RestApiException, IOException, OrmException, ConfigInvalidException { - if (self.get() != resource.getUser()) { + if (!self.get().hasSameAccountId(resource.getUser())) { throw new AuthException("not allowed to delete external IDs"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteSshKey.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteSshKey.java index 3d5d38e6d0..abb01182bc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteSshKey.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteSshKey.java @@ -50,7 +50,8 @@ public class DeleteSshKey implements RestModifyView apply(AccountResource.SshKey rsrc, Input input) throws AuthException, OrmException, RepositoryNotFoundException, IOException, ConfigInvalidException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canAdministrateServer()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("not allowed to delete SSH keys"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteWatchedProjects.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteWatchedProjects.java index 97102a2428..8cd979f6e4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteWatchedProjects.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteWatchedProjects.java @@ -52,7 +52,8 @@ public class DeleteWatchedProjects public Response apply(AccountResource rsrc, List input) throws AuthException, UnprocessableEntityException, OrmException, IOException, ConfigInvalidException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canAdministrateServer()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("It is not allowed to edit project watches of other users"); } if (input == null) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java index cd3c0c81f8..fa36d1d43e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetCapabilities.java @@ -78,7 +78,8 @@ class GetCapabilities implements RestReadView { @Override public Object apply(AccountResource resource) throws AuthException { - if (self.get() != resource.getUser() && !self.get().getCapabilities().canAdministrateServer()) { + if (!self.get().hasSameAccountId(resource.getUser()) + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("restricted to administrator"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetDiffPreferences.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetDiffPreferences.java index 0edff4f576..c2f7b8f66f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetDiffPreferences.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetDiffPreferences.java @@ -57,7 +57,8 @@ public class GetDiffPreferences implements RestReadView { @Override public DiffPreferencesInfo apply(AccountResource rsrc) throws AuthException, ConfigInvalidException, IOException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canAdministrateServer()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("restricted to administrator"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetEditPreferences.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetEditPreferences.java index e385020182..e795f8322a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetEditPreferences.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetEditPreferences.java @@ -49,7 +49,8 @@ public class GetEditPreferences implements RestReadView { @Override public EditPreferencesInfo apply(AccountResource rsrc) throws AuthException, IOException, ConfigInvalidException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canModifyAccount()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canModifyAccount()) { throw new AuthException("requires Modify Account capability"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetExternalIds.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetExternalIds.java index 6ea911f080..c926cff930 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetExternalIds.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetExternalIds.java @@ -49,7 +49,7 @@ public class GetExternalIds implements RestReadView { @Override public List apply(AccountResource resource) throws RestApiException, OrmException { - if (self.get() != resource.getUser()) { + if (!self.get().hasSameAccountId(resource.getUser())) { throw new AuthException("not allowed to get external IDs"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetOAuthToken.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetOAuthToken.java index 4bbb5d4c9a..61f5b8444e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetOAuthToken.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetOAuthToken.java @@ -50,7 +50,7 @@ class GetOAuthToken implements RestReadView { @Override public OAuthTokenInfo apply(AccountResource rsrc) throws AuthException, ResourceNotFoundException { - if (self.get() != rsrc.getUser()) { + if (!self.get().hasSameAccountId(rsrc.getUser())) { throw new AuthException("not allowed to get access token"); } Account a = rsrc.getUser().getAccount(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetPreferences.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetPreferences.java index 77cdbd451d..95b115f43d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetPreferences.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetPreferences.java @@ -36,7 +36,8 @@ public class GetPreferences implements RestReadView { @Override public GeneralPreferencesInfo apply(AccountResource rsrc) throws AuthException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canModifyAccount()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canModifyAccount()) { throw new AuthException("requires Modify Account capability"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetSshKeys.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetSshKeys.java index 980d880173..a169f6f48c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetSshKeys.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetSshKeys.java @@ -47,7 +47,8 @@ public class GetSshKeys implements RestReadView { public List apply(AccountResource rsrc) throws AuthException, OrmException, RepositoryNotFoundException, IOException, ConfigInvalidException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canModifyAccount()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canModifyAccount()) { throw new AuthException("not allowed to get SSH keys"); } return apply(rsrc.getUser()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetWatchedProjects.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetWatchedProjects.java index e0aeee03e0..d8580eb72f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GetWatchedProjects.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GetWatchedProjects.java @@ -51,7 +51,8 @@ public class GetWatchedProjects implements RestReadView { @Override public List apply(AccountResource rsrc) throws OrmException, AuthException, IOException, ConfigInvalidException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canAdministrateServer()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("It is not allowed to list project watches of other users"); } Account.Id accountId = rsrc.getUser().getAccountId(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/Index.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/Index.java index 1666c708cd..238241c4cc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/Index.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/Index.java @@ -39,7 +39,8 @@ public class Index implements RestModifyView { @Override public Response apply(AccountResource rsrc, Input input) throws IOException, AuthException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canModifyAccount()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canModifyAccount()) { throw new AuthException("not allowed to index account"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PostWatchedProjects.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PostWatchedProjects.java index 55ba912dcc..7a4e0ec7a8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PostWatchedProjects.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PostWatchedProjects.java @@ -63,7 +63,8 @@ public class PostWatchedProjects @Override public List apply(AccountResource rsrc, List input) throws OrmException, RestApiException, IOException, ConfigInvalidException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canAdministrateServer()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("not allowed to edit project watches"); } Account.Id accountId = rsrc.getUser().getAccountId(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutAgreement.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutAgreement.java index 423d5a1be5..e622575b3c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutAgreement.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutAgreement.java @@ -72,7 +72,7 @@ public class PutAgreement implements RestModifyView { String newPassword; if (input.generate) { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canAdministrateServer()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("not allowed to generate HTTP password"); } newPassword = generate(); } else if (input.httpPassword == null) { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canAdministrateServer()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("not allowed to clear HTTP password"); } newPassword = null; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutName.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutName.java index 443a549729..a00e2ad1b6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutName.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutName.java @@ -61,7 +61,8 @@ public class PutName implements RestModifyView { public Response apply(AccountResource rsrc, Input input) throws AuthException, MethodNotAllowedException, ResourceNotFoundException, OrmException, IOException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canModifyAccount()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canModifyAccount()) { throw new AuthException("not allowed to change name"); } return apply(rsrc.getUser(), input); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutPreferred.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutPreferred.java index ec60fb31de..d86a312bb4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutPreferred.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutPreferred.java @@ -50,7 +50,8 @@ public class PutPreferred implements RestModifyView apply(AccountResource.Email rsrc, Input input) throws AuthException, ResourceNotFoundException, OrmException, IOException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canModifyAccount()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canModifyAccount()) { throw new AuthException("not allowed to set preferred email address"); } return apply(rsrc.getUser(), rsrc.getEmail()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutStatus.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutStatus.java index ff541fdae4..c16d8da0f9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutStatus.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutStatus.java @@ -58,7 +58,8 @@ public class PutStatus implements RestModifyView { @Override public Response apply(AccountResource rsrc, Input input) throws AuthException, ResourceNotFoundException, OrmException, IOException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canModifyAccount()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canModifyAccount()) { throw new AuthException("not allowed to set status"); } return apply(rsrc.getUser(), input); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutUsername.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutUsername.java index e3a3c12adf..21b17205a7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/PutUsername.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/PutUsername.java @@ -59,7 +59,8 @@ public class PutUsername implements RestModifyView { public String apply(AccountResource rsrc, Input input) throws AuthException, MethodNotAllowedException, UnprocessableEntityException, ResourceConflictException, OrmException, IOException, ConfigInvalidException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canAdministrateServer()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canAdministrateServer()) { throw new AuthException("not allowed to set username"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/SetDiffPreferences.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/SetDiffPreferences.java index ac0cc963fb..c72ff0241e 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/SetDiffPreferences.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/SetDiffPreferences.java @@ -59,7 +59,8 @@ public class SetDiffPreferences implements RestModifyView apply(AccountResource rsrc, EmptyInput in) throws AuthException, OrmException, IOException { - if (self.get() != rsrc.getUser()) { + if (!self.get().hasSameAccountId(rsrc.getUser())) { throw new AuthException("not allowed to add starred change"); } try { @@ -159,7 +159,7 @@ public class StarredChanges @Override public Response apply(AccountResource.StarredChange rsrc, EmptyInput in) throws AuthException { - if (self.get() != rsrc.getUser()) { + if (!self.get().hasSameAccountId(rsrc.getUser())) { throw new AuthException("not allowed update starred changes"); } return Response.none(); @@ -180,7 +180,7 @@ public class StarredChanges @Override public Response apply(AccountResource.StarredChange rsrc, EmptyInput in) throws AuthException, OrmException, IOException { - if (self.get() != rsrc.getUser()) { + if (!self.get().hasSameAccountId(rsrc.getUser())) { throw new AuthException("not allowed remove starred change"); } starredChangesUtil.star( diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/Stars.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/Stars.java index 52c6cdfa9f..cf43a21c64 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/Stars.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/Stars.java @@ -97,7 +97,7 @@ public class Stars implements ChildCollection apply(AccountResource rsrc) throws BadRequestException, AuthException, OrmException { - if (self.get() != rsrc.getUser()) { + if (!self.get().hasSameAccountId(rsrc.getUser())) { throw new AuthException("not allowed to list stars of another account"); } QueryChanges query = changes.list(); @@ -119,7 +119,7 @@ public class Stars implements ChildCollection apply(AccountResource.Star rsrc) throws AuthException, OrmException { - if (self.get() != rsrc.getUser()) { + if (!self.get().hasSameAccountId(rsrc.getUser())) { throw new AuthException("not allowed to get stars of another account"); } return starredChangesUtil.getLabels(self.get().getAccountId(), rsrc.getChange().getId()); @@ -140,7 +140,7 @@ public class Stars implements ChildCollection apply(AccountResource.Star rsrc, StarsInput in) throws AuthException, BadRequestException, OrmException { - if (self.get() != rsrc.getUser()) { + if (!self.get().hasSameAccountId(rsrc.getUser())) { throw new AuthException("not allowed to update stars of another account"); } try { From b801a367757397cfb9d5da3706d266d66aea3e04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Wed, 16 May 2018 18:39:55 +0000 Subject: [PATCH 31/36] Revert "WorkQueue: Add metrics" Some plugins are creating a work queue on load which will create the metrics for that queue. If plugin is reloaded, the work queue is created again and it will try to create metrics again which will fail because metrics already exists thus causing the plugin to not load. This reverts commit fe270ddd54c0f4c2d20edc74fa1e890080bd0630. This reverts commit 4e97e35b7c3423ff4a85a02d4b03722f903d6348. This reverts commit fccd70ed65fd1003060b505070f5a48b93356b92. This reverts commit eb2f45c84ebb650cf653e40c3926e9fe953646f0. Change-Id: I0cef3831470d919c8efea4c019943af69a85281e --- Documentation/metrics.txt | 11 -- .../google/gerrit/server/git/WorkQueue.java | 102 +----------------- 2 files changed, 3 insertions(+), 110 deletions(-) diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt index 9f315b8fdd..091dbb3a5f 100644 --- a/Documentation/metrics.txt +++ b/Documentation/metrics.txt @@ -53,17 +53,6 @@ objects needing finalization. * `query/query_latency`: Successful query latency, accumulated over the life of the process. -=== Queue - -The metrics below are per queue. - -* `queue//pool_size`: Current number of threads in the pool -* `queue//max_pool_size`: Maximum allowed number of threads in the pool -* `queue//active_threads`: Number of threads that are actively executing tasks -* `queue//scheduled_tasks`: Number of scheduled tasks in the queue -* `queue//total_scheduled_tasks_count`: Total number of tasks that have been scheduled -* `queue//total_completed_tasks_count`: Total number of tasks that have completed execution - === SSH sessions * `sshd/sessions/connected`: Number of currently connected SSH sessions. 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 3b8e4c441d..40ebc6661e 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,14 +14,8 @@ 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; import com.google.gerrit.lifecycle.LifecycleModule; -import com.google.gerrit.metrics.Description; -import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.util.IdGenerator; @@ -88,19 +82,17 @@ public class WorkQueue { } }; - private final MetricMaker metrics; private final Executor defaultQueue; private final IdGenerator idGenerator; private final CopyOnWriteArrayList queues; @Inject - WorkQueue(MetricMaker metrics, IdGenerator idGenerator, @GerritServerConfig Config cfg) { - this(metrics, idGenerator, cfg.getInt("execution", "defaultThreadPoolSize", 1)); + WorkQueue(IdGenerator idGenerator, @GerritServerConfig Config cfg) { + this(idGenerator, cfg.getInt("execution", "defaultThreadPoolSize", 1)); } /** Constructor to allow binding the WorkQueue more explicitly in a vhost setup. */ - public WorkQueue(MetricMaker metrics, IdGenerator idGenerator, int defaultThreadPoolSize) { - this.metrics = metrics; + public WorkQueue(IdGenerator idGenerator, int defaultThreadPoolSize) { this.idGenerator = idGenerator; this.queues = new CopyOnWriteArrayList<>(); this.defaultQueue = createQueue(defaultThreadPoolSize, "WorkQueue"); @@ -207,94 +199,6 @@ public class WorkQueue { corePoolSize + 4 // concurrency level ); queueName = prefix; - try { - buildMetrics(queueName); - } catch (IllegalArgumentException e) { - if (e.getMessage().contains("already")) { - log.warn("Not creating metrics for queue '{}': already exists", queueName); - } else { - throw e; - } - } - } - - private void buildMetrics(String queueName) { - metrics.newCallbackMetric( - getMetricName(queueName, "max_pool_size"), - Long.class, - new Description("Maximum allowed number of threads in the pool") - .setGauge() - .setUnit("threads"), - new Supplier() { - @Override - public Long get() { - return (long) getMaximumPoolSize(); - } - }); - metrics.newCallbackMetric( - getMetricName(queueName, "pool_size"), - Long.class, - new Description("Current number of threads in the pool").setGauge().setUnit("threads"), - new Supplier() { - @Override - public Long get() { - return (long) getPoolSize(); - } - }); - metrics.newCallbackMetric( - getMetricName(queueName, "active_threads"), - Long.class, - new Description("Number number of threads that are actively executing tasks") - .setGauge() - .setUnit("threads"), - new Supplier() { - @Override - public Long get() { - return (long) getActiveCount(); - } - }); - metrics.newCallbackMetric( - getMetricName(queueName, "scheduled_tasks"), - Integer.class, - new Description("Number of scheduled tasks in the queue").setGauge().setUnit("tasks"), - new Supplier() { - @Override - public Integer get() { - return getQueue().size(); - } - }); - metrics.newCallbackMetric( - getMetricName(queueName, "total_scheduled_tasks_count"), - Long.class, - new Description("Total number of tasks that have been scheduled for execution") - .setCumulative() - .setUnit("tasks"), - new Supplier() { - @Override - public Long get() { - return (long) getTaskCount(); - } - }); - metrics.newCallbackMetric( - getMetricName(queueName, "total_completed_tasks_count"), - Long.class, - new Description("Total number of tasks that have completed execution") - .setCumulative() - .setUnit("tasks"), - new Supplier() { - @Override - public Long get() { - return (long) getCompletedTaskCount(); - } - }); - } - - private String getMetricName(String queueName, String metricName) { - String name = - CaseFormat.UPPER_CAMEL.to( - CaseFormat.LOWER_UNDERSCORE, - queueName.replaceFirst("SSH", "Ssh").replaceAll("-", "")); - return sanitizeMetricName(String.format("queue/%s/%s", name, metricName)); } public void unregisterWorkQueue() { From adfefda09412ed935ae76a85b45d8481f69067d7 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 22 May 2018 16:03:42 +0900 Subject: [PATCH 32/36] Fix more comparisons of current user Change-Id: I3a7c46dfa705ae6fa088735af7f031a840816957 --- .../java/com/google/gerrit/server/account/DeleteActive.java | 2 +- .../src/main/java/com/google/gerrit/server/account/Emails.java | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteActive.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteActive.java index d013120119..8b713cad91 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteActive.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/DeleteActive.java @@ -53,7 +53,7 @@ public class DeleteActive implements RestModifyView { @Override public Response apply(AccountResource rsrc, Input input) throws RestApiException, OrmException, IOException { - if (self.get() == rsrc.getUser()) { + if (self.get().hasSameAccountId(rsrc.getUser())) { throw new ResourceConflictException("cannot deactivate own account"); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/Emails.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/Emails.java index b894f5673b..8cfb66c867 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/Emails.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/Emails.java @@ -56,7 +56,8 @@ public class Emails @Override public AccountResource.Email parse(AccountResource rsrc, IdString id) throws ResourceNotFoundException { - if (self.get() != rsrc.getUser() && !self.get().getCapabilities().canAdministrateServer()) { + if (!self.get().hasSameAccountId(rsrc.getUser()) + && !self.get().getCapabilities().canAdministrateServer()) { throw new ResourceNotFoundException(); } From cbecb73ac1bb9f50fec9172de548da91920ca7a3 Mon Sep 17 00:00:00 2001 From: Kasper Nilsson Date: Thu, 15 Feb 2018 13:02:34 -0800 Subject: [PATCH 33/36] Allow some sections of the change list to overflow On screens < 90em (approximately 1600px with default font size) wide, the `Owner`, `Branch`, `Assignee`, and `Project` sections should overflow with ellipses. `Project` is handled as a special case -- because most long project names usually resemble a directory structure, they are truncated to just the last two parts of the path, with ellipsees appearing before the leading slash. Bug: Issue 4552 Change-Id: Icf882cbcecb741fb9e46141a1fa88f5987a3e674 (cherry picked from commit e420236caab5fb1e46d07df4dac4010512a6b4de) --- .../gr-path-list-behavior.html | 14 +++++++++----- .../gr-path-list-behavior_test.html | 13 +++++++++++++ .../gr-change-list-item.html | 10 ++++++++-- .../gr-change-list-item/gr-change-list-item.js | 6 ++++++ .../app/styles/gr-change-list-styles.html | 18 ++++++++++++++++++ 5 files changed, 54 insertions(+), 7 deletions(-) diff --git a/polygerrit-ui/app/behaviors/gr-path-list-behavior/gr-path-list-behavior.html b/polygerrit-ui/app/behaviors/gr-path-list-behavior/gr-path-list-behavior.html index f3db039b9c..d3491c79a4 100644 --- a/polygerrit-ui/app/behaviors/gr-path-list-behavior/gr-path-list-behavior.html +++ b/polygerrit-ui/app/behaviors/gr-path-list-behavior/gr-path-list-behavior.html @@ -93,16 +93,20 @@ limitations under the License. * Example * // returns 'text.html' * util.truncatePath.('text.html'); + * + * @param {string} path + * @param {number=} opt_threshold * @return {string} Returns the truncated value of a URL. */ - truncatePath(path) { + truncatePath(path, opt_threshold) { + const threshold = opt_threshold || 1; const pathPieces = path.split('/'); - if (pathPieces.length < 2) { - return path; - } + if (pathPieces.length <= threshold) { return path; } + + const index = pathPieces.length - threshold; // Character is an ellipsis. - return '\u2026/' + pathPieces[pathPieces.length - 1]; + return `\u2026/${pathPieces.slice(index).join('/')}`; }, }; })(window); diff --git a/polygerrit-ui/app/behaviors/gr-path-list-behavior/gr-path-list-behavior_test.html b/polygerrit-ui/app/behaviors/gr-path-list-behavior/gr-path-list-behavior_test.html index e0b1b7ec1a..f48fb989b3 100644 --- a/polygerrit-ui/app/behaviors/gr-path-list-behavior/gr-path-list-behavior_test.html +++ b/polygerrit-ui/app/behaviors/gr-path-list-behavior/gr-path-list-behavior_test.html @@ -66,6 +66,19 @@ limitations under the License. assert.equal(shortenedPath, expectedPath); }); + test('truncatePath with opt_threshold', () => { + const truncatePath = Gerrit.PathListBehavior.truncatePath; + let path = 'level1/level2/level3/level4/file.js'; + let shortenedPath = truncatePath(path, 2); + // The expected path is truncated with an ellipsis. + const expectedPath = '\u2026/level4/file.js'; + assert.equal(shortenedPath, expectedPath); + + path = 'level2/file.js'; + shortenedPath = truncatePath(path, 2); + assert.equal(shortenedPath, path); + }); + test('truncatePath with short path should not add ellipsis', () => { const truncatePath = Gerrit.PathListBehavior.truncatePath; const path = 'file.js'; diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html index a2c785337f..0fab4afdb8 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html +++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.html @@ -15,6 +15,7 @@ limitations under the License. --> + @@ -145,7 +146,12 @@ limitations under the License. - [[change.project]] + + [[change.project]] + + + [[_computeTruncatedProject(change.project)]] + @@ -154,7 +160,7 @@ limitations under the License. diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js index 79f06fe4cf..d8fced21f0 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js +++ b/polygerrit-ui/app/elements/change-list/gr-change-list-item/gr-change-list-item.js @@ -39,6 +39,7 @@ behaviors: [ Gerrit.BaseUrlBehavior, Gerrit.ChangeTableBehavior, + Gerrit.PathListBehavior, Gerrit.RESTClientBehavior, Gerrit.URLEncodingBehavior, ], @@ -115,5 +116,10 @@ if (!change.topic) { return ''; } return Gerrit.Nav.getUrlForTopic(change.topic); }, + + _computeTruncatedProject(project) { + if (!project) { return ''; } + return this.truncatePath(project, 2); + }, }); })(); diff --git a/polygerrit-ui/app/styles/gr-change-list-styles.html b/polygerrit-ui/app/styles/gr-change-list-styles.html index e082aabce5..a0bba9044d 100644 --- a/polygerrit-ui/app/styles/gr-change-list-styles.html +++ b/polygerrit-ui/app/styles/gr-change-list-styles.html @@ -62,6 +62,24 @@ limitations under the License. .label { text-align: center; } + .truncatedProject { + display: none; + } + @media only screen and (max-width: 90em) { + .assignee, + .branch, + .owner { + overflow: hidden; + max-width: 10rem; + text-overflow: ellipsis; + } + .truncatedProject { + display: inline-block; + } + .fullProject { + display: none; + } + } @media only screen and (max-width: 50em) { :host { font-size: 14px; From 1d56cd75d179dc3a856ff20f61561a14edf2b3ac Mon Sep 17 00:00:00 2001 From: Orgad Shaneh Date: Wed, 23 May 2018 09:48:12 +0300 Subject: [PATCH 34/36] Doc: Fix code example in JS API open is an array. Change-Id: Id942f522d5a671248d65aeb3617aa23dfd9795fe --- Documentation/js-api.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/js-api.txt b/Documentation/js-api.txt index f3b84d2705..8a19720941 100644 --- a/Documentation/js-api.txt +++ b/Documentation/js-api.txt @@ -769,7 +769,7 @@ Gerrit.get(url, callback) ---- Gerrit.get('/changes/?q=status:open', function (open) { for (var i = 0; i < open.length; i++) { - console.log(open.get(i).change_id); + console.log(open[i].change_id); } }); ---- From 46af8d137d10ba632ffb11dfe3c97ab871b6e7b8 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 21 May 2018 12:45:29 +0900 Subject: [PATCH 35/36] Use Logger's built-in string formatting where possible Change-Id: Ie1d03cbd17660a777a15961333dbd8c7fbd2e453 --- .../httpd/plugins/HttpPluginServlet.java | 23 +++--- .../httpd/plugins/LfsPluginServlet.java | 4 +- .../httpd/plugins/PluginServletContext.java | 2 +- .../gerrit/httpd/raw/ResourceServlet.java | 6 +- .../gerrit/httpd/restapi/RestApiServlet.java | 2 +- .../google/gerrit/pgm/SwitchSecureStore.java | 5 +- .../pgm/http/jetty/HiddenErrorHandler.java | 2 +- .../com/google/gerrit/server/WebLinks.java | 4 +- .../gerrit/server/account/GroupCacheImpl.java | 6 +- .../server/auth/ldap/LdapGroupBackend.java | 2 +- .../server/auth/ldap/LdapGroupMembership.java | 2 +- .../gerrit/server/auth/ldap/LdapRealm.java | 2 +- .../gerrit/server/change/AbandonUtil.java | 2 +- .../gerrit/server/events/EventFactory.java | 6 +- .../server/extensions/events/EventUtil.java | 8 +-- .../server/git/GarbageCollectionRunner.java | 2 +- .../git/LocalDiskRepositoryManager.java | 10 ++- .../gerrit/server/git/ReceiveCommits.java | 2 +- .../google/gerrit/server/index/Schema.java | 2 +- .../server/index/change/StalenessChecker.java | 2 +- .../server/mail/receive/MailProcessor.java | 34 +++++---- .../server/mail/send/CommentSender.java | 21 +++--- .../gerrit/server/notedb/ChangeNotes.java | 2 +- .../server/plugins/JarPluginProvider.java | 2 +- .../gerrit/server/plugins/PluginLoader.java | 70 +++++++++---------- .../gerrit/server/project/PutConfig.java | 12 ++-- 26 files changed, 114 insertions(+), 121 deletions(-) diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java index 55fa0cb29e..b64b3b3821 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java @@ -190,7 +190,7 @@ class HttpPluginServlet extends HttpServlet implements StartPluginListener, Relo try { filter = plugin.getHttpInjector().getInstance(GuiceFilter.class); } catch (RuntimeException e) { - log.warn(String.format("Plugin %s cannot load GuiceFilter", name), e); + log.warn("Plugin {} cannot load GuiceFilter", name, e); return null; } @@ -198,7 +198,7 @@ class HttpPluginServlet extends HttpServlet implements StartPluginListener, Relo ServletContext ctx = PluginServletContext.create(plugin, wrapper.getFullPath(name)); filter.init(new WrappedFilterConfig(ctx)); } catch (ServletException e) { - log.warn(String.format("Plugin %s failed to initialize HTTP", name), e); + log.warn("Plugin {} failed to initialize HTTP", name, e); return null; } @@ -429,10 +429,11 @@ class HttpPluginServlet extends HttpServlet implements StartPluginListener, Relo && size.isPresent()) { if (size.get() <= 0 || size.get() > SMALL_RESOURCE) { log.warn( - String.format( - "Plugin %s: %s omitted from document index. " - + "Size %d out of range (0,%d).", - pluginName, name.substring(prefix.length()), size.get(), SMALL_RESOURCE)); + "Plugin {}: {} omitted from document index. Size {} out of range (0,{}).", + pluginName, + name.substring(prefix.length()), + size.get(), + SMALL_RESOURCE); return false; } return true; @@ -455,9 +456,9 @@ class HttpPluginServlet extends HttpServlet implements StartPluginListener, Relo about = entry; } else { log.warn( - String.format( - "Plugin %s: Multiple 'about' documents found; using %s", - pluginName, about.getName().substring(prefix.length()))); + "Plugin {}: Multiple 'about' documents found; using {}", + pluginName, + about.getName().substring(prefix.length())); } } else { docs.add(entry); @@ -731,9 +732,7 @@ class HttpPluginServlet extends HttpServlet implements StartPluginListener, Relo } return def; } catch (IOException e) { - log.warn( - String.format("Error getting %s for plugin %s, using default", attr, plugin.getName()), - e); + log.warn("Error getting {} for plugin {}, using default", attr, plugin.getName(), e); return null; } } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/LfsPluginServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/LfsPluginServlet.java index 7e013e6c21..e13ea959d2 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/LfsPluginServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/LfsPluginServlet.java @@ -139,7 +139,7 @@ public class LfsPluginServlet extends HttpServlet try { guiceFilter = plugin.getHttpInjector().getInstance(GuiceFilter.class); } catch (RuntimeException e) { - log.warn(String.format("Plugin %s cannot load GuiceFilter", name), e); + log.warn("Plugin {} cannot load GuiceFilter", name, e); return null; } @@ -147,7 +147,7 @@ public class LfsPluginServlet extends HttpServlet ServletContext ctx = PluginServletContext.create(plugin, "/"); guiceFilter.init(new WrappedFilterConfig(ctx)); } catch (ServletException e) { - log.warn(String.format("Plugin %s failed to initialize HTTP", name), e); + log.warn("Plugin {} failed to initialize HTTP", name, e); return null; } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/PluginServletContext.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/PluginServletContext.java index 53b49a4a03..8f64d9ff74 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/PluginServletContext.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/plugins/PluginServletContext.java @@ -155,7 +155,7 @@ class PluginServletContext { @Override public void log(String msg, Throwable reason) { - log.warn(String.format("[plugin %s] %s", plugin.getName(), msg), reason); + log.warn("[plugin {}] {}", plugin.getName(), msg, reason); } @Override diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java index 2b34f81613..3ca1878206 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/raw/ResourceServlet.java @@ -161,7 +161,7 @@ public abstract class ResourceServlet extends HttpServlet { r = cache.get(p, newLoader(p)); } } catch (ExecutionException e) { - log.warn("Cannot load static resource " + req.getPathInfo(), e); + log.warn("Cannot load static resource {}", req.getPathInfo(), e); CacheHeaders.setNotCacheable(rsp); rsp.setStatus(SC_INTERNAL_SERVER_ERROR); return; @@ -214,12 +214,12 @@ public abstract class ResourceServlet extends HttpServlet { try { Path p = getResourcePath(name); if (p == null) { - log.warn(String.format("Path doesn't exist %s", name)); + log.warn("Path doesn't exist {}", name); return null; } return cache.get(p, newLoader(p)); } catch (ExecutionException | IOException e) { - log.warn(String.format("Cannot load static resource %s", name), e); + log.warn("Cannot load static resource {}", name, e); return null; } } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java index d9dd5d4269..bfa91d6d37 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/restapi/RestApiServlet.java @@ -1094,7 +1094,7 @@ public class RestApiServlet extends HttpServlet { if (!Strings.isNullOrEmpty(req.getQueryString())) { uri += "?" + req.getQueryString(); } - log.error(String.format("Error in %s %s", req.getMethod(), uri), err); + log.error("Error in {} {}", req.getMethod(), uri, err); if (!res.isCommitted()) { res.reset(); diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/SwitchSecureStore.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/SwitchSecureStore.java index 58876ce688..1a2216224c 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/SwitchSecureStore.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/SwitchSecureStore.java @@ -68,7 +68,7 @@ public class SwitchSecureStore extends SiteProgram { SitePaths sitePaths = new SitePaths(getSitePath()); Path newSecureStorePath = Paths.get(newSecureStoreLib); if (!Files.exists(newSecureStorePath)) { - log.error(String.format("File %s doesn't exist", newSecureStorePath.toAbsolutePath())); + log.error("File {} doesn't exist", newSecureStorePath.toAbsolutePath()); return -1; } @@ -77,8 +77,7 @@ public class SwitchSecureStore extends SiteProgram { if (currentSecureStoreName.equals(newSecureStore)) { log.error( - "Old and new SecureStore implementation names " - + "are the same. Migration will not work"); + "Old and new SecureStore implementation names are the same. Migration will not work"); return -1; } diff --git a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/HiddenErrorHandler.java b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/HiddenErrorHandler.java index 4c2455a22d..2fbbb97282 100644 --- a/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/HiddenErrorHandler.java +++ b/gerrit-pgm/src/main/java/com/google/gerrit/pgm/http/jetty/HiddenErrorHandler.java @@ -79,7 +79,7 @@ class HiddenErrorHandler extends ErrorHandler { if (!Strings.isNullOrEmpty(req.getQueryString())) { uri += "?" + req.getQueryString(); } - log.error(String.format("Error in %s %s", req.getMethod(), uri), err); + log.error("Error in {} {}", req.getMethod(), uri, err); } } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/WebLinks.java b/gerrit-server/src/main/java/com/google/gerrit/server/WebLinks.java index 0566804dff..4ca77bc3c9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/WebLinks.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/WebLinks.java @@ -49,7 +49,7 @@ public class WebLinks { if (link == null) { return false; } else if (Strings.isNullOrEmpty(link.name) || Strings.isNullOrEmpty(link.url)) { - log.warn(String.format("%s is missing name and/or url", link.getClass().getName())); + log.warn("{} is missing name and/or url", link.getClass().getName()); return false; } return true; @@ -60,7 +60,7 @@ public class WebLinks { if (link == null) { return false; } else if (Strings.isNullOrEmpty(link.name) || Strings.isNullOrEmpty(link.url)) { - log.warn(String.format("%s is missing name and/or url", link.getClass().getName())); + log.warn("{} is missing name and/or url", link.getClass().getName()); return false; } return true; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java index effaa8dbc4..06a46805e9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/account/GroupCacheImpl.java @@ -92,7 +92,7 @@ public class GroupCacheImpl implements GroupCache { Optional g = byId.get(groupId); return g.isPresent() ? g.get() : missing(groupId); } catch (ExecutionException e) { - log.warn("Cannot load group " + groupId, e); + log.warn("Cannot load group {}", groupId, e); return missing(groupId); } } @@ -131,7 +131,7 @@ public class GroupCacheImpl implements GroupCache { try { return byName.get(name.get()).orElse(null); } catch (ExecutionException e) { - log.warn(String.format("Cannot lookup group %s by name", name.get()), e); + log.warn("Cannot lookup group {} by name", name.get(), e); return null; } } @@ -144,7 +144,7 @@ public class GroupCacheImpl implements GroupCache { try { return byUUID.get(uuid.get()).orElse(null); } catch (ExecutionException e) { - log.warn(String.format("Cannot lookup group %s by uuid", uuid.get()), e); + log.warn("Cannot lookup group {} by uuid", uuid.get(), e); return null; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupBackend.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupBackend.java index 7feb745fb7..c748ddccd6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupBackend.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupBackend.java @@ -126,7 +126,7 @@ public class LdapGroupBackend implements GroupBackend { return null; } } catch (ExecutionException e) { - log.warn(String.format("Cannot lookup group %s in LDAP", groupDn), e); + log.warn("Cannot lookup group {} in LDAP", groupDn, e); return null; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupMembership.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupMembership.java index 7bef2e7e06..39c8a2fdd4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupMembership.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapGroupMembership.java @@ -65,7 +65,7 @@ class LdapGroupMembership implements GroupMembership { try { membership = new ListGroupMembership(membershipCache.get(id)); } catch (ExecutionException e) { - LdapGroupBackend.log.warn(String.format("Cannot lookup membershipsOf %s in LDAP", id), e); + LdapGroupBackend.log.warn("Cannot lookup membershipsOf {} in LDAP", id, e); membership = GroupMembership.EMPTY; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java index 49ff6e85f5..122d4bc2a4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/auth/ldap/LdapRealm.java @@ -313,7 +313,7 @@ class LdapRealm extends AbstractRealm { Optional id = usernameCache.get(accountName); return id != null ? id.orElse(null) : null; } catch (ExecutionException e) { - log.warn(String.format("Cannot lookup account %s in LDAP", accountName), e); + log.warn("Cannot lookup account {} in LDAP", accountName, e); return null; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java index 7c408c86c1..1dd321f083 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/AbandonUtil.java @@ -96,7 +96,7 @@ public class AbandonUtil { log.error(msg.toString(), e); } } - log.info(String.format("Auto-Abandoned %d of %d changes.", count, changesToAbandon.size())); + log.info("Auto-Abandoned {} of {} changes.", count, changesToAbandon.size()); } catch (QueryParseException | OrmException e) { log.error("Failed to query inactive open changes for auto-abandoning.", e); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java index 2584559d47..5c3da331b4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java @@ -150,7 +150,7 @@ public class EventFactory { try { a.commitMessage = changeDataFactory.create(db, change).commitMessage(); } catch (Exception e) { - log.error("Error while getting full commit message for change " + a.number, e); + log.error("Error while getting full commit message for change {}", a.number, e); } a.url = getChangeUrl(change); a.owner = asAccountAttribute(change.getOwner()); @@ -498,9 +498,9 @@ public class EventFactory { } p.kind = changeKindCache.getChangeKind(db, change, patchSet); } catch (IOException e) { - log.error("Cannot load patch set data for " + patchSet.getId(), e); + log.error("Cannot load patch set data for {}", patchSet.getId(), e); } catch (PatchListNotAvailableException e) { - log.error(String.format("Cannot get size information for %s.", pId), e); + log.error("Cannot get size information for {}.", pId, e); } return p; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/extensions/events/EventUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/extensions/events/EventUtil.java index 531d95a6b9..57382f6bbf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/extensions/events/EventUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/extensions/events/EventUtil.java @@ -118,9 +118,9 @@ public class EventUtil { public void logEventListenerError(Object event, Object listener, Exception error) { if (log.isDebugEnabled()) { log.debug( - String.format( - "Error in event listener %s for event %s", - listener.getClass().getName(), event.getClass().getName()), + "Error in event listener {} for event {}", + listener.getClass().getName(), + event.getClass().getName(), error); } else { log.warn( @@ -133,7 +133,7 @@ public class EventUtil { public static void logEventListenerError(Object listener, Exception error) { if (log.isDebugEnabled()) { - log.debug(String.format("Error in event listener %s", listener.getClass().getName()), error); + log.debug("Error in event listener {}", listener.getClass().getName(), error); } else { log.warn("Error in event listener {}: {}", listener.getClass().getName(), error.getMessage()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/GarbageCollectionRunner.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/GarbageCollectionRunner.java index ea93f96f92..4d8a61f47d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/GarbageCollectionRunner.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/GarbageCollectionRunner.java @@ -52,7 +52,7 @@ public class GarbageCollectionRunner implements Runnable { if (delay == MISSING_CONFIG && interval == MISSING_CONFIG) { log.info("Ignoring missing gc schedule configuration"); } else if (delay < 0 || interval <= 0) { - log.warn(String.format("Ignoring invalid gc schedule configuration: %s", scheduleConfig)); + log.warn("Ignoring invalid gc schedule configuration: {}", scheduleConfig); } else { @SuppressWarnings("unused") Future possiblyIgnoredError = diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java index ca19d47e03..5f836ae2b7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/LocalDiskRepositoryManager.java @@ -100,7 +100,7 @@ public class LocalDiskRepositoryManager implements GitRepositoryManager { } else { desc = String.format("%d", limit); } - log.info(String.format("Defaulting core.streamFileThreshold to %s", desc)); + log.info("Defaulting core.streamFileThreshold to {}", desc); cfg.setStreamFileThreshold(limit); } cfg.install(); @@ -226,9 +226,7 @@ public class LocalDiskRepositoryManager implements GitRepositoryManager { // File metaConfigLog = new File(db.getDirectory(), "logs/" + RefNames.REFS_CONFIG); if (!metaConfigLog.getParentFile().mkdirs() || !metaConfigLog.createNewFile()) { - log.error( - String.format( - "Failed to create ref log for %s in repository %s", RefNames.REFS_CONFIG, name)); + log.error("Failed to create ref log for {} in repository {}", RefNames.REFS_CONFIG, name); } onCreateProject(name); @@ -304,7 +302,7 @@ public class LocalDiskRepositoryManager implements GitRepositoryManager { Integer.MAX_VALUE, visitor); } catch (IOException e) { - log.error("Error walking repository tree " + visitor.startFolder.toAbsolutePath(), e); + log.error("Error walking repository tree {}", visitor.startFolder.toAbsolutePath(), e); } } @@ -359,7 +357,7 @@ public class LocalDiskRepositoryManager implements GitRepositoryManager { Project.NameKey nameKey = getProjectName(startFolder, p); if (getBasePath(nameKey).equals(startFolder)) { if (isUnreasonableName(nameKey)) { - log.warn("Ignoring unreasonably named repository " + p.toAbsolutePath()); + log.warn("Ignoring unreasonably named repository {}", p.toAbsolutePath()); } else { found.add(nameKey); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index e6529359ec..79e3c12583 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -650,7 +650,7 @@ public class ReceiveCommits { try { repo.setGitwebDescription(ps.getProject().getDescription()); } catch (IOException e) { - log.warn("cannot update description of " + project.getName(), e); + log.warn("cannot update description of {}", project.getName(), e); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java index 5e06242b66..c7e5755422 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/Schema.java @@ -186,7 +186,7 @@ public class Schema { try { v = f.get(obj, fillArgs); } catch (OrmException e) { - log.error(String.format("error getting field %s of %s", f.getName(), obj), e); + log.error("error getting field {} of {}", f.getName(), obj, e); return null; } if (v == null) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java index 872dfaf135..0c43c85973 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/StalenessChecker.java @@ -206,7 +206,7 @@ public class StalenessChecker { } return false; } catch (IOException e) { - log.warn(String.format("error checking staleness of %s in %s", id, project), e); + log.warn("error checking staleness of {} in {}", id, project, e); return true; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java index accf7ba0e4..07cbc20719 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/receive/MailProcessor.java @@ -128,9 +128,10 @@ public class MailProcessor { for (DynamicMap.Entry filter : mailFilters) { if (!filter.getProvider().get().shouldProcessMessage(message)) { log.warn( - String.format( - "Message %s filtered by plugin %s %s. Will delete message.", - message.id(), filter.getPluginName(), filter.getExportName())); + "Message {} filtered by plugin {} {}. Will delete message.", + message.id(), + filter.getPluginName(), + filter.getExportName()); return; } } @@ -138,23 +139,24 @@ public class MailProcessor { MailMetadata metadata = MetadataParser.parse(message); if (!metadata.hasRequiredFields()) { log.error( - String.format( - "Message %s is missing required metadata, have %s. Will delete message.", - message.id(), metadata)); + "Message {} is missing required metadata, have {}. Will delete message.", + message.id(), + metadata); return; } Set accounts = accountByEmailCache.get(metadata.author); if (accounts.size() != 1) { log.error( - String.format( - "Address %s could not be matched to a unique account. It was matched to %s. Will delete message.", - metadata.author, accounts)); + "Address {} could not be matched to a unique account. It was matched to {}." + + " Will delete message.", + metadata.author, + accounts); return; } Account.Id account = accounts.iterator().next(); if (!accountCache.get(account).getAccount().isActive()) { - log.warn(String.format("Mail: Account %s is inactive. Will delete message.", account)); + log.warn("Mail: Account {} is inactive. Will delete message.", account); return; } @@ -163,14 +165,16 @@ public class MailProcessor { queryProvider.get().byLegacyChangeId(new Change.Id(metadata.changeNumber)); if (changeDataList.size() != 1) { log.error( - String.format( - "Message %s references unique change %s, but there are %d matching changes in the index. Will delete message.", - message.id(), metadata.changeNumber, changeDataList.size())); + "Message {} references unique change {}, but there are {} matching changes in " + + "the index. Will delete message.", + message.id(), + metadata.changeNumber, + changeDataList.size()); return; } ChangeData cd = changeDataList.get(0); if (existingMessageIds(cd).contains(message.id())) { - log.info("Message " + message.id() + " was already processed. Will delete message."); + log.info("Message {} was already processed. Will delete message.", message.id()); return; } // Get all comments; filter and sort them to get the original list of @@ -193,7 +197,7 @@ public class MailProcessor { } if (parsedComments.isEmpty()) { - log.warn("Could not parse any comments from " + message.id() + ". Will delete message."); + log.warn("Could not parse any comments from {}. Will delete message.", message.id()); return; } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java index b572e8d179..d3c456c340 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/send/CommentSender.java @@ -254,11 +254,10 @@ public class CommentSender extends ReplyToChangeSender { currentGroup.fileData = new PatchFile(repo, patchList, c.key.filename); } catch (IOException e) { log.warn( - String.format( - "Cannot load %s from %s in %s", - c.key.filename, - patchList.getNewId().name(), - projectState.getProject().getName()), + "Cannot load {} from {} in {}", + c.key.filename, + patchList.getNewId().name(), + projectState.getProject().getName(), e); currentGroup.fileData = null; } @@ -352,10 +351,10 @@ public class CommentSender extends ReplyToChangeSender { maxLines = currentFileData.getLineCount(side); } catch (IOException err) { // The file could not be read, leave the max as is. - log.warn(String.format("Failed to read file %s on side %d", comment.key.filename, side), err); + log.warn("Failed to read file {} on side {}", comment.key.filename, side, err); } catch (NoSuchEntityException err) { // The file could not be read, leave the max as is. - log.warn(String.format("Side %d of file %s didn't exist", side, comment.key.filename), err); + log.warn("Side {} of file {} didn't exist", side, comment.key.filename, err); } int startLine = Math.max(1, lineNbr - contextLines + 1); @@ -404,7 +403,7 @@ public class CommentSender extends ReplyToChangeSender { try { return commentsUtil.get(args.db.get(), changeData.notes(), key); } catch (OrmException e) { - log.warn("Could not find the parent of this comment: " + child.toString()); + log.warn("Could not find the parent of this comment: {}", child.toString()); return Optional.empty(); } } @@ -595,16 +594,16 @@ public class CommentSender extends ReplyToChangeSender { return fileInfo.getLine(side, lineNbr); } catch (IOException err) { // Default to the empty string if the file cannot be safely read. - log.warn(String.format("Failed to read file on side %d", side), err); + log.warn("Failed to read file on side {}", side, err); return ""; } catch (IndexOutOfBoundsException err) { // Default to the empty string if the given line number does not appear // in the file. - log.debug(String.format("Failed to get line number of file on side %d", side), err); + log.debug("Failed to get line number of file on side {}", side, err); return ""; } catch (NoSuchEntityException err) { // Default to the empty string if the side cannot be found. - log.warn(String.format("Side %d of file didn't exist", side), err); + log.warn("Side {} of file didn't exist", side, err); return ""; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index d35fd12d8f..d821a78661 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java @@ -140,7 +140,7 @@ public class ChangeNotes extends AbstractChangeNotes { throw new NoSuchChangeException(changeId); } if (changes.size() != 1) { - log.error(String.format("Multiple changes found for %d", changeId.get())); + log.error("Multiple changes found for {}", changeId.get()); throw new NoSuchChangeException(changeId); } return changes.get(0).notes(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/JarPluginProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/JarPluginProvider.java index 06a1a683ae..b10d8abd2c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/JarPluginProvider.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/JarPluginProvider.java @@ -131,7 +131,7 @@ public class JarPluginProvider implements ServerPluginProvider { if (overlay != null) { Path classes = Paths.get(overlay).resolve(name).resolve("main"); if (Files.isDirectory(classes)) { - log.info(String.format("plugin %s: including %s", name, classes)); + log.info("plugin {}: including {}", name, classes); urls.add(classes.toUri().toURL()); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginLoader.java b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginLoader.java index 9b37bad969..c99b797d9d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginLoader.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginLoader.java @@ -195,9 +195,9 @@ public class PluginLoader implements LifecycleListener { String name = MoreObjects.firstNonNull(getGerritPluginName(tmp), nameOf(fileName)); if (!originalName.equals(name)) { log.warn( - String.format( - "Plugin provides its own name: <%s>, use it instead of the input name: <%s>", - name, originalName)); + "Plugin provides its own name: <{}>, use it instead of the input name: <{}>", + name, + originalName); } String fileExtension = getExtension(fileName); @@ -206,7 +206,7 @@ public class PluginLoader implements LifecycleListener { Plugin active = running.get(name); if (active != null) { fileName = active.getSrcFile().getFileName().toString(); - log.info(String.format("Replacing plugin %s", active.getName())); + log.info("Replacing plugin {}", active.getName()); Path old = pluginsDir.resolve(".last_" + fileName); Files.deleteIfExists(old); Files.move(active.getSrcFile(), old); @@ -217,7 +217,7 @@ public class PluginLoader implements LifecycleListener { try { Plugin plugin = runPlugin(name, dst, active); if (active == null) { - log.info(String.format("Installed plugin %s", plugin.getName())); + log.info("Installed plugin {}", plugin.getName()); } } catch (PluginInstallException e) { Files.deleteIfExists(dst); @@ -247,7 +247,7 @@ public class PluginLoader implements LifecycleListener { private synchronized void unloadPlugin(Plugin plugin) { persistentCacheFactory.onStop(plugin); String name = plugin.getName(); - log.info(String.format("Unloading plugin %s, version %s", name, plugin.getVersion())); + log.info("Unloading plugin {}, version {}", name, plugin.getVersion()); plugin.stop(env); env.onStopPlugin(plugin); running.remove(name); @@ -257,7 +257,7 @@ public class PluginLoader implements LifecycleListener { public void disablePlugins(Set names) { if (!isRemoteAdminEnabled()) { - log.warn("Remote plugin administration is disabled, ignoring disablePlugins(" + names + ")"); + log.warn("Remote plugin administration is disabled, ignoring disablePlugins({})", names); return; } @@ -268,7 +268,7 @@ public class PluginLoader implements LifecycleListener { continue; } - log.info(String.format("Disabling plugin %s", active.getName())); + log.info("Disabling plugin {}", active.getName()); Path off = active.getSrcFile().resolveSibling(active.getSrcFile().getFileName() + ".disabled"); try { @@ -288,7 +288,7 @@ public class PluginLoader implements LifecycleListener { disabled.put(name, offPlugin); } catch (Throwable e) { // This shouldn't happen, as the plugin was loaded earlier. - log.warn(String.format("Cannot load disabled plugin %s", active.getName()), e.getCause()); + log.warn("Cannot load disabled plugin {}", active.getName(), e.getCause()); } } cleanInBackground(); @@ -297,7 +297,7 @@ public class PluginLoader implements LifecycleListener { public void enablePlugins(Set names) throws PluginInstallException { if (!isRemoteAdminEnabled()) { - log.warn("Remote plugin administration is disabled, ignoring enablePlugins(" + names + ")"); + log.warn("Remote plugin administration is disabled, ignoring enablePlugins({})", names); return; } @@ -308,7 +308,7 @@ public class PluginLoader implements LifecycleListener { continue; } - log.info(String.format("Enabling plugin %s", name)); + log.info("Enabling plugin {}", name); String n = off.getSrcFile().toFile().getName(); if (n.endsWith(".disabled")) { n = n.substring(0, n.lastIndexOf('.')); @@ -317,7 +317,7 @@ public class PluginLoader implements LifecycleListener { try { Files.move(off.getSrcFile(), on); } catch (IOException e) { - log.error("Failed to move plugin " + name + " into place", e); + log.error("Failed to move plugin {} into place", name, e); continue; } disabled.remove(name); @@ -337,25 +337,23 @@ public class PluginLoader implements LifecycleListener { }; try (DirectoryStream files = Files.newDirectoryStream(tempDir, filter)) { for (Path file : files) { - log.info("Removing stale plugin file: " + file.toFile().getName()); + log.info("Removing stale plugin file: {}", file.toFile().getName()); try { Files.delete(file); } catch (IOException e) { log.error( - String.format( - "Failed to remove stale plugin file %s: %s", - file.toFile().getName(), e.getMessage())); + "Failed to remove stale plugin file {}: {}", file.toFile().getName(), e.getMessage()); } } } catch (IOException e) { - log.warn("Unable to discover stale plugin files: " + e.getMessage()); + log.warn("Unable to discover stale plugin files: {}", e.getMessage()); } } @Override public synchronized void start() { removeStalePluginFiles(); - log.info("Loading plugins from " + pluginsDir.toAbsolutePath()); + log.info("Loading plugins from {}", pluginsDir.toAbsolutePath()); srvInfoImpl.state = ServerInformation.State.STARTUP; rescan(); srvInfoImpl.state = ServerInformation.State.RUNNING; @@ -404,13 +402,11 @@ public class PluginLoader implements LifecycleListener { for (Plugin active : reload) { String name = active.getName(); try { - log.info(String.format("Reloading plugin %s", name)); + log.info("Reloading plugin {}", name); Plugin newPlugin = runPlugin(name, active.getSrcFile(), active); - log.info( - String.format( - "Reloaded plugin %s, version %s", newPlugin.getName(), newPlugin.getVersion())); + log.info("Reloaded plugin {}, version {}", newPlugin.getName(), newPlugin.getVersion()); } catch (PluginInstallException e) { - log.warn(String.format("Cannot reload plugin %s", name), e.getCause()); + log.warn("Cannot reload plugin {}", name, e.getCause()); throw e; } } @@ -448,21 +444,20 @@ public class PluginLoader implements LifecycleListener { } if (active != null) { - log.info(String.format("Reloading plugin %s", active.getName())); + log.info("Reloading plugin {}", active.getName()); } try { Plugin loadedPlugin = runPlugin(name, path, active); if (!loadedPlugin.isDisabled()) { log.info( - String.format( - "%s plugin %s, version %s", - active == null ? "Loaded" : "Reloaded", - loadedPlugin.getName(), - loadedPlugin.getVersion())); + "{} plugin {}, version {}", + active == null ? "Loaded" : "Reloaded", + loadedPlugin.getName(), + loadedPlugin.getVersion()); } } catch (PluginInstallException e) { - log.warn(String.format("Cannot load plugin %s", name), e.getCause()); + log.warn("Cannot load plugin {}", name, e.getCause()); } } @@ -716,18 +711,19 @@ public class PluginLoader implements LifecycleListener { Collection elementsToAdd = new ArrayList<>(); for (Path loser : Iterables.skip(enabled, 1)) { log.warn( - String.format( - "Plugin <%s> was disabled, because" - + " another plugin <%s>" - + " with the same name <%s> already exists", - loser, winner, plugin)); + "Plugin <{}> was disabled, because" + + " another plugin <{}>" + + " with the same name <{}> already exists", + loser, + winner, + plugin); Path disabledPlugin = Paths.get(loser + ".disabled"); elementsToAdd.add(disabledPlugin); elementsToRemove.add(loser); try { Files.move(loser, disabledPlugin); } catch (IOException e) { - log.warn("Failed to fully disable plugin " + loser, e); + log.warn("Failed to fully disable plugin {}", loser, e); } } Iterables.removeAll(files, elementsToRemove); @@ -740,7 +736,7 @@ public class PluginLoader implements LifecycleListener { try { return listPlugins(pluginsDir); } catch (IOException e) { - log.error("Cannot list " + pluginsDir.toAbsolutePath(), e); + log.error("Cannot list {}", pluginsDir.toAbsolutePath(), e); return ImmutableList.of(); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/PutConfig.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/PutConfig.java index 59e004486c..2132ef4adb 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/PutConfig.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/PutConfig.java @@ -168,7 +168,7 @@ public class PutConfig implements RestModifyView { throw new ResourceConflictException( "Cannot update " + projectName + ": " + e.getCause().getMessage()); } - log.warn(String.format("Failed to update config of project %s.", projectName), e); + log.warn("Failed to update config of project {}.", projectName, e); throw new ResourceConflictException("Cannot update " + projectName); } @@ -202,9 +202,7 @@ public class PutConfig implements RestModifyView { ProjectConfigEntry projectConfigEntry = pluginConfigEntries.get(pluginName, v.getKey()); if (projectConfigEntry != null) { if (!isValidParameterName(v.getKey())) { - log.warn( - String.format( - "Parameter name '%s' must match '^[a-zA-Z0-9]+[a-zA-Z0-9-]*$'", v.getKey())); + log.warn("Parameter name '{}' must match '^[a-zA-Z0-9]+[a-zA-Z0-9-]*$'", v.getKey()); continue; } String oldValue = cfg.getString(v.getKey()); @@ -253,9 +251,9 @@ public class PutConfig implements RestModifyView { break; default: log.warn( - String.format( - "The type '%s' of parameter '%s' is not supported.", - projectConfigEntry.getType().name(), v.getKey())); + "The type '{}' of parameter '{}' is not supported.", + projectConfigEntry.getType().name(), + v.getKey()); } } catch (NumberFormatException ex) { throw new BadRequestException( From 1a2c8037bedcbc0ca261ceda9c135cf1d13da5bc Mon Sep 17 00:00:00 2001 From: Gerrit Code Review Date: Wed, 23 May 2018 11:05:28 +0000 Subject: [PATCH 36/36] Update git submodules * Update plugins/download-commands from branch 'stable-2.15' to cfed9533017919eba8ae89e27929651c7d37c0a5 - Merge branch 'stable-2.14' into stable-2.15 * stable-2.14: GitDownloadCommand: Use Logger's built-in string formatting Change-Id: I4ae4ba2d1de673419682f9965b2838593c3d9515 - GitDownloadCommand: Use Logger's built-in string formatting Change-Id: Id08fb3af477607c7a0237672d56fe448bdce3b55 * Update plugins/reviewnotes from branch 'stable-2.15' to eb09af38459c25ba3056b4f4ee0ef181f42915b2 - Merge branch 'stable-2.14' into stable-2.15 * stable-2.14: CreateReviewNotes: Use Logger's built-in string formatting Change-Id: I2d2dd1d24734cc43997cceb7efd57fc72b415d06 - CreateReviewNotes: Use Logger's built-in string formatting Change-Id: Iffb19f9a9d370dcc0b476421028ec608d715d0df --- plugins/download-commands | 2 +- plugins/reviewnotes | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/download-commands b/plugins/download-commands index 43656ce165..cfed953301 160000 --- a/plugins/download-commands +++ b/plugins/download-commands @@ -1 +1 @@ -Subproject commit 43656ce1658a56834cfe62760eafbb9b17c7ad53 +Subproject commit cfed9533017919eba8ae89e27929651c7d37c0a5 diff --git a/plugins/reviewnotes b/plugins/reviewnotes index 5e6d0fb102..eb09af3845 160000 --- a/plugins/reviewnotes +++ b/plugins/reviewnotes @@ -1 +1 @@ -Subproject commit 5e6d0fb102a2d5486c94a0ea7463efd1508f5ade +Subproject commit eb09af38459c25ba3056b4f4ee0ef181f42915b2