diff options
122 files changed, 2692 insertions, 462 deletions
diff --git a/.bazelproject b/.bazelproject index a7f54502e9..ad7b022328 100644 --- a/.bazelproject +++ b/.bazelproject @@ -16,7 +16,7 @@ directories: targets: //...:all -java_language_level: 8 +java_language_level: 11 workspace_type: java diff --git a/Documentation/cmd-index.txt b/Documentation/cmd-index.txt index ce3a024fe3..7f1a6e818c 100644 --- a/Documentation/cmd-index.txt +++ b/Documentation/cmd-index.txt @@ -58,6 +58,9 @@ link:cmd-apropos.html[gerrit apropos]:: link:cmd-ban-commit.html[gerrit ban-commit]:: Bans a commit from a project's repository. +link:cmd-copy-approvals.html[gerrit copy-approvals]:: + Copy all inferred approvals labels to the latest patch-set. + link:cmd-create-branch.html[gerrit create-branch]:: Create a new project branch. diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index be801f3e00..565d3702e0 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -3197,6 +3197,25 @@ advantage of new search features without restarting the server. + Defaults to true. +[[index.paginationType]]index.paginationType:: ++ +The pagination type to use when index queries are repeated to +obtain the next set of results. Supported values are: ++ +* `OFFSET` ++ +Index queries are repeated with a non-zero offset to obtain the +next set of results. ++ +* `SEARCH_AFTER` ++ +Index queries are repeated using a search-after object. Index +backends can provide their custom implementations for search-after. +Note that, `SEARCH_AFTER` does not impact using offsets in Gerrit +query APIs. ++ +Defaults to `OFFSET`. + [[index.maxLimit]]index.maxLimit:: + Maximum limit to allow for search queries. Requesting results above this @@ -3215,6 +3234,44 @@ this threshold times the requested limit will result in an error. Set to + Defaults to no limit. +[[index.pageSizeMultiplier]]index.pageSizeMultiplier:: ++ +When index queries are repeated to obtain more results, this multiplier +will be used to determine the limit for the next query. Using a page +multiplier allows queries to start off small and thus provide good +latency for queries which may end up only having very few results, and +then scaling up to have better throughput to handle queries with larger +result sets without incurring the overhead of making as many queries as +would be required with a smaller limit. This strategy of using a multiplier +attempts to create a balance between latency and throughput by dynamically +adjusting the query size to the number of results being returned by each +query in the pagination. ++ +The larger the multiplier, the better the throughput on large queries, and +it also improves latency on large queries by scaling up quickly. However, a +larger multiplier can hurt latencies a bit by making the "last" query in a +series longer than needed. The impact of this depends on how much the backend +latency goes up when specifying a large limit and few results are returned. +Setting link:#index.maxPageSize[index.maxPageSize] that isn't too large, can +likely help reduce the impacts of this. ++ +For example, if the limit of the previous query was 500 and pageSizeMultiplier +is configured to 5, the next query will have a limit of 2500. ++ +Defaults to 1 which effectively turns this feature off. + +[[index.maxPageSize]]index.maxPageSize:: ++ +Maximum size to allow when index queries are repeated to obtain more results. Note +that, link:#index.maxLimit[index.maxLimit] will be used to limit page size if it +is configured to a value lower than maxPageSize. ++ +For example, if the limit of previous query was 500, pageSizeMultiplier is +configured to 5 and maxPageSize to 2000, the next query will have a limit of +2000 (instead of 2500). ++ +Defaults to no limit. + [[index.maxTerms]]index.maxTerms:: + Maximum number of leaf terms to allow in a query. Too-large queries may @@ -3928,6 +3985,46 @@ If set to true, log files are rotated daily at midnight (GMT). + Defaults to true. +[[metrics]] +=== Section metrics + +[[metrics.reservoir]]metrics.reservoir:: ++ +The type of data reservoir used by the metrics system to calculate the percentile +values for timers and histograms. +It can be set to one of the following values: ++ +* ExponentiallyDecaying: An exponentially-decaying random reservoir based on + Cormode et al's forward-decaying priority reservoir sampling method to produce + a statistically representative sampling reservoir, exponentially biased towards + newer entries. +* SlidingTimeWindowArray: A sliding window that stores only the measurements made + in the last window using chunks of 512 samples. +* SlidingTimeWindow: A sliding window that stores only the measurements made in + the last window using a skip list. +* SlidingWindow: A sliding window that stores only the last measurements. +* Uniform: A random sampling reservoir that uses Vitter's Algorithm R to produce + a statistically representative sample. ++ +Defaults to ExponentiallyDecaying. + +[[metrics.ExponentiallyDecaying.alpha]]metrics.ExponentiallyDecaying.alpha:: ++ +The exponential decay factor; the higher this is, the more biased the reservoir +will be towards newer values. + +[[metrics.reservoirType.size]]metrics.<reservoirType>.size:: ++ +The number of samples to keep in the reservoir. Applies to all reservoir types +except the sliding time-based ones. ++ +Defaults to 1028. + +[[metrics.reservoirType.window]]metrics.<reservoirType>.window:: ++ +The window of time for keeping data in the reservoir. It only applies to sliding +time-based reservoir types. + [[mimetype]] === Section mimetype diff --git a/Documentation/rest-api-projects.txt b/Documentation/rest-api-projects.txt index 6ddc8bdfa7..82b6553724 100644 --- a/Documentation/rest-api-projects.txt +++ b/Documentation/rest-api-projects.txt @@ -3970,7 +3970,7 @@ A boolean value that can also be inherited. |`value` || The effective boolean value. |`configured_value` || -The configured value, can be `TRUE`, `FALSE` or `INHERITED`. +The configured value, can be `TRUE`, `FALSE` or `INHERIT`. |`inherited_value` |optional| The boolean value inherited from the parent. + Not set if there is no parent. diff --git a/Documentation/user-privacy.txt b/Documentation/user-privacy.txt index d61ee76a82..afedb7e13e 100644 --- a/Documentation/user-privacy.txt +++ b/Documentation/user-privacy.txt @@ -8,7 +8,7 @@ This page documents how Gerrit handles user data. |=== | Note: Gerrit has extensive support for link:config-plugins.html[plugins] which extend Gerrits functionality, and these plugins could access, export, or - maniuplate user data. This document only focuses on the behavior of Gerrit + manipulate user data. This document only focuses on the behavior of Gerrit core and its link:dev-core-plugins.html[core plugins]. |=== @@ -98,11 +98,6 @@ identifiable information. Notably, Gerrit cannot: * Remove a user's e-mail from all existing commits * Remove a user's username -There is also a known -link:https://bugs.chromium.org/p/gerrit/issues/detail?id=14185[bug] where a -user's username is stored in metadata for link:user-attention-set.html[Attention -Set]. - ## Open Source Software Limitations @@ -110,4 +105,4 @@ Gerrit is open-source software licensed under the Apache 2.0 license. 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.
\ No newline at end of file +language governing permissions and limitations under the License. diff --git a/Documentation/user-search.txt b/Documentation/user-search.txt index cc8d813441..f07a5042f5 100644 --- a/Documentation/user-search.txt +++ b/Documentation/user-search.txt @@ -531,7 +531,7 @@ Mergeability of abandoned changes is not computed. This operator will not find any abandoned but mergeable changes. + This operator only works if Gerrit indexes 'mergeable'. See -link:config-gerrit.html#index.change.indexMergeable[indexMergeable] +link:config-gerrit.html#change.mergeabilityComputationBehavior[change.mergeabilityComputationBehavior] for details. [[ignored]] diff --git a/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java b/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java index 373246ae0d..9a652e3946 100644 --- a/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java +++ b/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java @@ -21,11 +21,13 @@ import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.events.LifecycleListener; import com.google.gerrit.lifecycle.LifecycleModule; import com.google.gerrit.metrics.MetricMaker; +import com.google.gerrit.metrics.MetricsReservoirConfig; import com.google.gerrit.server.config.AllProjectsConfigProvider; import com.google.gerrit.server.config.FileBasedAllProjectsConfigProvider; import com.google.gerrit.server.config.FileBasedGlobalPluginConfigProvider; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.config.GlobalPluginConfigProvider; +import com.google.gerrit.server.config.MetricsReservoirConfigImpl; import com.google.gerrit.server.config.SitePath; import com.google.gerrit.server.config.SitePaths; import com.google.gerrit.server.config.TrackingFooters; @@ -36,6 +38,7 @@ import com.google.gerrit.server.schema.SchemaModule; import com.google.gerrit.testing.InMemoryRepositoryManager; import com.google.inject.Inject; import com.google.inject.ProvisionException; +import com.google.inject.Scopes; import java.io.IOException; import java.nio.file.Files; import java.nio.file.Path; @@ -69,6 +72,7 @@ class InMemoryTestingDatabaseModule extends LifecycleModule { bind(InMemoryRepositoryManager.class).in(SINGLETON); } + bind(MetricsReservoirConfig.class).to(MetricsReservoirConfigImpl.class).in(Scopes.SINGLETON); bind(MetricMaker.class).to(TestMetricMaker.class); listener().to(CreateSchema.class); diff --git a/java/com/google/gerrit/acceptance/SshSessionJsch.java b/java/com/google/gerrit/acceptance/SshSessionJsch.java index a86c2d64da..fa4d1d11fd 100644 --- a/java/com/google/gerrit/acceptance/SshSessionJsch.java +++ b/java/com/google/gerrit/acceptance/SshSessionJsch.java @@ -40,9 +40,9 @@ import java.util.Scanner; import org.bouncycastle.openssl.jcajce.JcaPEMWriter; import org.bouncycastle.openssl.jcajce.JcaPKCS8Generator; import org.bouncycastle.util.io.pem.PemObject; -import org.eclipse.jgit.transport.JschConfigSessionFactory; -import org.eclipse.jgit.transport.OpenSshConfig.Host; import org.eclipse.jgit.transport.SshSessionFactory; +import org.eclipse.jgit.transport.ssh.jsch.JschConfigSessionFactory; +import org.eclipse.jgit.transport.ssh.jsch.OpenSshConfig.Host; import org.eclipse.jgit.util.FS; public class SshSessionJsch extends SshSession { diff --git a/java/com/google/gerrit/asciidoctor/DocIndexer.java b/java/com/google/gerrit/asciidoctor/DocIndexer.java index 513bdd70b3..6cbe2a96a3 100644 --- a/java/com/google/gerrit/asciidoctor/DocIndexer.java +++ b/java/com/google/gerrit/asciidoctor/DocIndexer.java @@ -107,11 +107,19 @@ public class DocIndexer { String title; try (BufferedReader titleReader = Files.newBufferedReader(file.toPath(), UTF_8)) { - title = titleReader.readLine(); - if (title != null && title.startsWith("[[")) { + while ((title = titleReader.readLine()) != null) { // Generally the first line of the txt is the title. In a few cases the - // first line is a "[[tag]]" and the second line is the title. - title = titleReader.readLine(); + // first lines are "[[tag]]" and or ":attribute:" and the next line + // after those is the title. + if (title.startsWith("[[")) { + continue; + } + // Skip attributes such as :linkattrs: + if (title.startsWith(":") && title.endsWith(":")) { + continue; + } + // We found the title + break; } } Matcher matcher = SECTION_HEADER.matcher(title); diff --git a/java/com/google/gerrit/server/util/GuiceRequestScopePropagator.java b/java/com/google/gerrit/httpd/GuiceRequestScopePropagator.java index 99dd8bfc63..7c8094a7ea 100644 --- a/java/com/google/gerrit/server/util/GuiceRequestScopePropagator.java +++ b/java/com/google/gerrit/httpd/GuiceRequestScopePropagator.java @@ -12,11 +12,13 @@ // See the License for the specific language governing permissions and // limitations under the License. -package com.google.gerrit.server.util; +package com.google.gerrit.httpd; import com.google.gerrit.common.Nullable; import com.google.gerrit.server.RemotePeer; import com.google.gerrit.server.config.CanonicalWebUrl; +import com.google.gerrit.server.util.RequestScopePropagator; +import com.google.gerrit.server.util.ThreadLocalRequestContext; import com.google.inject.Inject; import com.google.inject.Key; import com.google.inject.Provider; @@ -29,21 +31,25 @@ import java.net.SocketAddress; import java.util.HashMap; import java.util.Map; import java.util.concurrent.Callable; +import javax.servlet.http.HttpServletRequest; /** Propagator for Guice's built-in servlet scope. */ public class GuiceRequestScopePropagator extends RequestScopePropagator { private final String url; private final SocketAddress peer; + private final Provider<HttpServletRequest> request; @Inject GuiceRequestScopePropagator( @CanonicalWebUrl @Nullable Provider<String> urlProvider, @RemotePeer Provider<SocketAddress> remotePeerProvider, - ThreadLocalRequestContext local) { + ThreadLocalRequestContext local, + Provider<HttpServletRequest> request) { super(ServletScopes.REQUEST, local); this.url = urlProvider != null ? urlProvider.get() : null; this.peer = remotePeerProvider.get(); + this.request = request; } /** @see RequestScopePropagator#wrap(Callable) */ @@ -64,6 +70,11 @@ public class GuiceRequestScopePropagator extends RequestScopePropagator { seedMap.put(Key.get(typeOfProvider(SocketAddress.class), RemotePeer.class), Providers.of(peer)); seedMap.put(Key.get(SocketAddress.class, RemotePeer.class), peer); + Key<?> webSessionAttrKey = Key.get(WebSession.class); + Object webSessionAttrValue = request.get().getAttribute(webSessionAttrKey.toString()); + seedMap.put(webSessionAttrKey, webSessionAttrValue); + seedMap.put(Key.get(typeOfProvider(WebSession.class)), Providers.of(webSessionAttrValue)); + return ServletScopes.continueRequest(callable, seedMap); } diff --git a/java/com/google/gerrit/httpd/WebModule.java b/java/com/google/gerrit/httpd/WebModule.java index da485cc0f0..79dde85c66 100644 --- a/java/com/google/gerrit/httpd/WebModule.java +++ b/java/com/google/gerrit/httpd/WebModule.java @@ -29,7 +29,6 @@ import com.google.gerrit.server.config.GerritOptions; import com.google.gerrit.server.config.GerritRequestModule; import com.google.gerrit.server.config.GitwebCgiConfig; import com.google.gerrit.server.git.receive.AsyncReceiveCommits.AsyncReceiveCommitsModule; -import com.google.gerrit.server.util.GuiceRequestScopePropagator; import com.google.gerrit.server.util.RequestScopePropagator; import com.google.inject.Inject; import com.google.inject.ProvisionException; diff --git a/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java b/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java index ef37fc5bc8..97752a004a 100644 --- a/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java +++ b/java/com/google/gerrit/httpd/plugins/HttpPluginServlet.java @@ -491,7 +491,7 @@ class HttpPluginServlet extends HttpServlet implements StartPluginListener, Relo } if (toc != null) { - appendPageAsSection(scanner, toc, "Documentaion", md); + appendPageAsSection(scanner, toc, "Documentation", md); } else { appendEntriesSection(scanner, docs, "Documentation", md, prefix, 0); appendEntriesSection(scanner, servlets, "Servlets", md, prefix, "servlet-".length()); diff --git a/java/com/google/gerrit/index/IndexConfig.java b/java/com/google/gerrit/index/IndexConfig.java index 8676fb2f78..c21f32eb5c 100644 --- a/java/com/google/gerrit/index/IndexConfig.java +++ b/java/com/google/gerrit/index/IndexConfig.java @@ -30,6 +30,7 @@ import org.eclipse.jgit.lib.Config; @AutoValue public abstract class IndexConfig { private static final int DEFAULT_MAX_TERMS = 1024; + private static final int DEFAULT_PAGE_SIZE_MULTIPLIER = 1; public static IndexConfig createDefault() { return builder().build(); @@ -40,7 +41,10 @@ public abstract class IndexConfig { setIfPresent(cfg, "maxLimit", b::maxLimit); setIfPresent(cfg, "maxPages", b::maxPages); setIfPresent(cfg, "maxTerms", b::maxTerms); + setIfPresent(cfg, "pageSizeMultiplier", b::pageSizeMultiplier); + setIfPresent(cfg, "maxPageSize", b::maxPageSize); setTypeOrDefault(cfg, b::type); + setPaginationTypeOrDefault(cfg, b::paginationType); return b; } @@ -56,13 +60,21 @@ public abstract class IndexConfig { setter.accept(new IndexType(type).toString()); } + private static void setPaginationTypeOrDefault(Config cfg, Consumer<PaginationType> setter) { + setter.accept( + cfg != null ? cfg.getEnum("index", null, "paginationType", PaginationType.OFFSET) : null); + } + public static Builder builder() { return new AutoValue_IndexConfig.Builder() .maxLimit(Integer.MAX_VALUE) .maxPages(Integer.MAX_VALUE) .maxTerms(DEFAULT_MAX_TERMS) + .pageSizeMultiplier(DEFAULT_PAGE_SIZE_MULTIPLIER) + .maxPageSize(Integer.MAX_VALUE) .type(IndexType.getDefault()) - .separateChangeSubIndexes(false); + .separateChangeSubIndexes(false) + .paginationType(PaginationType.OFFSET); } @AutoValue.Builder @@ -85,6 +97,12 @@ public abstract class IndexConfig { public abstract Builder separateChangeSubIndexes(boolean separate); + public abstract Builder paginationType(PaginationType type); + + public abstract Builder pageSizeMultiplier(int pageSizeMultiplier); + + public abstract Builder maxPageSize(int maxPageSize); + abstract IndexConfig autoBuild(); public IndexConfig build() { @@ -92,6 +110,8 @@ public abstract class IndexConfig { checkLimit(cfg.maxLimit(), "maxLimit"); checkLimit(cfg.maxPages(), "maxPages"); checkLimit(cfg.maxTerms(), "maxTerms"); + checkLimit(cfg.pageSizeMultiplier(), "pageSizeMultiplier"); + checkLimit(cfg.maxPageSize(), "maxPageSize"); return cfg; } } @@ -124,4 +144,21 @@ public abstract class IndexConfig { * Returns whether different subsets of changes may be stored in different physical sub-indexes. */ public abstract boolean separateChangeSubIndexes(); + + /** + * Returns pagination type to use when index queries are repeated to obtain the next set of + * results. + */ + public abstract PaginationType paginationType(); + + /** + * Returns multiplier to be used to determine the limit when queries are repeated to obtain the + * next set of results. + */ + public abstract int pageSizeMultiplier(); + + /** + * Returns maximum allowed limit when repeating index queries to obtain the next set of results. + */ + public abstract int maxPageSize(); } diff --git a/java/com/google/gerrit/index/PaginationType.java b/java/com/google/gerrit/index/PaginationType.java new file mode 100644 index 0000000000..e7e34fdc0a --- /dev/null +++ b/java/com/google/gerrit/index/PaginationType.java @@ -0,0 +1,29 @@ +// Copyright (C) 2022 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.index; + +public enum PaginationType { + /** Index queries are restarted at a non-zero offset to obtain the next set of results */ + OFFSET, + + /** + * Index queries are restarted using a search-after object. Supported index backends can provide + * their custom implementations for search-after. + * + * <p>For example, Lucene implementation uses the last doc from the previous search as + * search-after object and uses the IndexSearcher.searchAfter API to get the next set of results. + */ + SEARCH_AFTER +} diff --git a/java/com/google/gerrit/index/QueryOptions.java b/java/com/google/gerrit/index/QueryOptions.java index 0401dab067..91c8d1a5e5 100644 --- a/java/com/google/gerrit/index/QueryOptions.java +++ b/java/com/google/gerrit/index/QueryOptions.java @@ -19,15 +19,52 @@ import static com.google.common.base.Preconditions.checkArgument; import com.google.auto.value.AutoValue; import com.google.common.collect.ImmutableSet; import com.google.common.primitives.Ints; +import com.google.gerrit.common.Nullable; import java.util.Set; import java.util.function.Function; @AutoValue public abstract class QueryOptions { public static QueryOptions create(IndexConfig config, int start, int limit, Set<String> fields) { + return create(config, start, null, limit, config.pageSizeMultiplier(), limit, fields); + } + + public static QueryOptions create( + IndexConfig config, int start, int pageSize, int limit, Set<String> fields) { + return create(config, start, null, pageSize, config.pageSizeMultiplier(), limit, fields); + } + + public static QueryOptions create( + IndexConfig config, + int start, + int pageSize, + int pageSizeMultiplier, + int limit, + Set<String> fields) { + return create(config, start, null, pageSize, pageSizeMultiplier, limit, fields); + } + + public static QueryOptions create( + IndexConfig config, + int start, + Object searchAfter, + int pageSize, + int pageSizeMultiplier, + int limit, + Set<String> fields) { checkArgument(start >= 0, "start must be nonnegative: %s", start); checkArgument(limit > 0, "limit must be positive: %s", limit); - return new AutoValue_QueryOptions(config, start, limit, ImmutableSet.copyOf(fields)); + if (searchAfter != null) { + checkArgument(start == 0, "start must be 0 when searchAfter is specified: %s", start); + } + return new AutoValue_QueryOptions( + config, + start, + searchAfter, + pageSize, + pageSizeMultiplier, + limit, + ImmutableSet.copyOf(fields)); } public QueryOptions convertForBackend() { @@ -36,26 +73,56 @@ public abstract class QueryOptions { int backendLimit = config().maxLimit(); int limit = Ints.saturatedCast((long) limit() + start()); limit = Math.min(limit, backendLimit); - return create(config(), 0, limit, fields()); + int pageSize = Math.min(Ints.saturatedCast((long) pageSize() + start()), backendLimit); + return create(config(), 0, null, pageSize, pageSizeMultiplier(), limit, fields()); } public abstract IndexConfig config(); public abstract int start(); + @Nullable + public abstract Object searchAfter(); + + public abstract int pageSize(); + + public abstract int pageSizeMultiplier(); + public abstract int limit(); public abstract ImmutableSet<String> fields(); + public QueryOptions withPageSize(int pageSize) { + return create( + config(), start(), searchAfter(), pageSize, pageSizeMultiplier(), limit(), fields()); + } + public QueryOptions withLimit(int newLimit) { - return create(config(), start(), newLimit, fields()); + return create( + config(), start(), searchAfter(), pageSize(), pageSizeMultiplier(), newLimit, fields()); } public QueryOptions withStart(int newStart) { - return create(config(), newStart, limit(), fields()); + return create( + config(), newStart, searchAfter(), pageSize(), pageSizeMultiplier(), limit(), fields()); + } + + public QueryOptions withSearchAfter(Object newSearchAfter) { + // Index search-after APIs don't use 'start', so set it to 0 to be safe. ElasticSearch for + // example, expects it to be 0 when using search-after APIs. + return create( + config(), start(), newSearchAfter, pageSize(), pageSizeMultiplier(), limit(), fields()) + .withStart(0); } public QueryOptions filterFields(Function<QueryOptions, Set<String>> filter) { - return create(config(), start(), limit(), filter.apply(this)); + return create( + config(), + start(), + searchAfter(), + pageSize(), + pageSizeMultiplier(), + limit(), + filter.apply(this)); } } diff --git a/java/com/google/gerrit/index/query/AndPredicate.java b/java/com/google/gerrit/index/query/AndPredicate.java index ae13fb3f8b..098bccbde6 100644 --- a/java/com/google/gerrit/index/query/AndPredicate.java +++ b/java/com/google/gerrit/index/query/AndPredicate.java @@ -15,15 +15,19 @@ package com.google.gerrit.index.query; import static com.google.common.base.Preconditions.checkState; +import static com.google.common.collect.ImmutableList.toImmutableList; +import com.google.common.collect.ImmutableList; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Collections; +import java.util.Comparator; import java.util.List; /** Requires all predicates to be true. */ -public class AndPredicate<T> extends Predicate<T> implements Matchable<T> { +public class AndPredicate<T> extends Predicate<T> + implements Matchable<T>, Comparator<Predicate<T>> { private final List<Predicate<T>> children; private final int cost; @@ -35,7 +39,7 @@ public class AndPredicate<T> extends Predicate<T> implements Matchable<T> { protected AndPredicate(Collection<? extends Predicate<T>> that) { List<Predicate<T>> t = new ArrayList<>(that.size()); int c = 0; - for (Predicate<T> p : that) { + for (Predicate<T> p : sort(that)) { if (getClass() == p.getClass()) { for (Predicate<T> gp : p.getChildren()) { t.add(gp); @@ -114,6 +118,28 @@ public class AndPredicate<T> extends Predicate<T> implements Matchable<T> { && getChildren().equals(((Predicate<?>) other).getChildren()); } + private ImmutableList<Predicate<T>> sort(Collection<? extends Predicate<T>> that) { + return that.stream().sorted(this).collect(toImmutableList()); + } + + @Override + public int compare(Predicate<T> a, Predicate<T> b) { + int ai = a instanceof DataSource ? 0 : 1; + int bi = b instanceof DataSource ? 0 : 1; + int cmp = ai - bi; + + if (cmp == 0) { + cmp = a.estimateCost() - b.estimateCost(); + } + + if (cmp == 0 && a instanceof DataSource && b instanceof DataSource) { + DataSource<?> as = (DataSource<?>) a; + DataSource<?> bs = (DataSource<?>) b; + cmp = as.getCardinality() - bs.getCardinality(); + } + return cmp; + } + @Override public String toString() { final StringBuilder r = new StringBuilder(); diff --git a/java/com/google/gerrit/index/query/AndSource.java b/java/com/google/gerrit/index/query/AndSource.java index 538e11b783..2a04051d2a 100644 --- a/java/com/google/gerrit/index/query/AndSource.java +++ b/java/com/google/gerrit/index/query/AndSource.java @@ -15,56 +15,58 @@ package com.google.gerrit.index.query; import static com.google.common.base.Preconditions.checkArgument; -import static com.google.common.collect.ImmutableList.toImmutableList; -import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableList; -import com.google.common.collect.Iterables; -import com.google.gerrit.exceptions.StorageException; -import java.util.ArrayList; +import com.google.gerrit.index.IndexConfig; import java.util.Collection; -import java.util.Comparator; import java.util.List; -public class AndSource<T> extends AndPredicate<T> - implements DataSource<T>, Comparator<Predicate<T>> { +public class AndSource<T> extends AndPredicate<T> implements DataSource<T> { protected final DataSource<T> source; private final IsVisibleToPredicate<T> isVisibleToPredicate; private final int start; private final int cardinality; + private final IndexConfig indexConfig; - public AndSource(Collection<? extends Predicate<T>> that) { - this(that, null, 0); + public AndSource(Collection<? extends Predicate<T>> that, IndexConfig indexConfig) { + this(that, null, 0, indexConfig); } - public AndSource(Predicate<T> that, IsVisibleToPredicate<T> isVisibleToPredicate) { - this(that, isVisibleToPredicate, 0); + public AndSource( + Predicate<T> that, IsVisibleToPredicate<T> isVisibleToPredicate, IndexConfig indexConfig) { + this(that, isVisibleToPredicate, 0, indexConfig); } - public AndSource(Predicate<T> that, IsVisibleToPredicate<T> isVisibleToPredicate, int start) { - this(ImmutableList.of(that), isVisibleToPredicate, start); + public AndSource( + Predicate<T> that, + IsVisibleToPredicate<T> isVisibleToPredicate, + int start, + IndexConfig indexConfig) { + this(ImmutableList.of(that), isVisibleToPredicate, start, indexConfig); } public AndSource( Collection<? extends Predicate<T>> that, IsVisibleToPredicate<T> isVisibleToPredicate, - int start) { + int start, + IndexConfig indexConfig) { super(that); checkArgument(start >= 0, "negative start: %s", start); this.isVisibleToPredicate = isVisibleToPredicate; this.start = start; + this.indexConfig = indexConfig; int c = Integer.MAX_VALUE; DataSource<T> s = null; int minCost = Integer.MAX_VALUE; - for (Predicate<T> p : sort(getChildren())) { + for (Predicate<T> p : getChildren()) { if (p instanceof DataSource) { c = Math.min(c, ((DataSource<?>) p).getCardinality()); int cost = p.estimateCost(); if (cost < minCost) { - s = toDataSource(p); + s = toPaginatingSource(p); minCost = cost; } } @@ -75,64 +77,12 @@ public class AndSource<T> extends AndPredicate<T> @Override public ResultSet<T> read() { - if (source == null) { - throw new StorageException("No DataSource: " + this); - } - - // ResultSets are lazy. Calling #read here first and then dealing with ResultSets only when - // requested allows the index to run asynchronous queries. - ResultSet<T> resultSet = source.read(); - return new LazyResultSet<>( - () -> { - List<T> r = new ArrayList<>(); - T last = null; - int nextStart = 0; - boolean skipped = false; - for (T data : buffer(resultSet)) { - if (!isMatchable() || match(data)) { - r.add(data); - } else { - skipped = true; - } - last = data; - nextStart++; - } - - if (skipped && last != null && source instanceof Paginated) { - // If our source is a paginated source and we skipped at - // least one of its results, we may not have filled the full - // limit the caller wants. Restart the source and continue. - // - @SuppressWarnings("unchecked") - Paginated<T> p = (Paginated<T>) source; - while (skipped && r.size() < p.getOptions().limit() + start) { - skipped = false; - ResultSet<T> next = p.restart(nextStart); - - for (T data : buffer(next)) { - if (match(data)) { - r.add(data); - } else { - skipped = true; - } - nextStart++; - } - } - } - - if (start >= r.size()) { - return ImmutableList.of(); - } else if (start > 0) { - return ImmutableList.copyOf(r.subList(start, r.size())); - } - return ImmutableList.copyOf(r); - }); + return source.read(); } @Override public ResultSet<FieldBundle> readRaw() { - // TOOD(hiesel): Implement - throw new UnsupportedOperationException("not implemented"); + return source.readRaw(); } @Override @@ -153,11 +103,6 @@ public class AndSource<T> extends AndPredicate<T> return true; } - private Iterable<T> buffer(ResultSet<T> scanner) { - return FluentIterable.from(Iterables.partition(scanner, 50)) - .transformAndConcat(this::transformBuffer); - } - protected List<T> transformBuffer(List<T> buffer) { return buffer; } @@ -167,30 +112,18 @@ public class AndSource<T> extends AndPredicate<T> return cardinality; } - private ImmutableList<Predicate<T>> sort(Collection<? extends Predicate<T>> that) { - return that.stream().sorted(this).collect(toImmutableList()); - } - - @Override - public int compare(Predicate<T> a, Predicate<T> b) { - int ai = a instanceof DataSource ? 0 : 1; - int bi = b instanceof DataSource ? 0 : 1; - int cmp = ai - bi; - - if (cmp == 0) { - cmp = a.estimateCost() - b.estimateCost(); - } - - if (cmp == 0 && a instanceof DataSource && b instanceof DataSource) { - DataSource<?> as = (DataSource<?>) a; - DataSource<?> bs = (DataSource<?>) b; - cmp = as.getCardinality() - bs.getCardinality(); - } - return cmp; - } - @SuppressWarnings("unchecked") - private DataSource<T> toDataSource(Predicate<T> pred) { - return (DataSource<T>) pred; + private PaginatingSource<T> toPaginatingSource(Predicate<T> pred) { + return new PaginatingSource<T>((DataSource<T>) pred, start, indexConfig) { + @Override + protected boolean match(T object) { + return AndSource.this.match(object); + } + + @Override + protected boolean isMatchable() { + return AndSource.this.isMatchable(); + } + }; } } diff --git a/java/com/google/gerrit/index/query/IndexedQuery.java b/java/com/google/gerrit/index/query/IndexedQuery.java index d9e33ea2dc..df14e295cc 100644 --- a/java/com/google/gerrit/index/query/IndexedQuery.java +++ b/java/com/google/gerrit/index/query/IndexedQuery.java @@ -87,19 +87,15 @@ public class IndexedQuery<I, T> extends Predicate<T> implements DataSource<T>, P } @Override - public ResultSet<T> restart(int start) { - opts = opts.withStart(start); - try { - source = index.getSource(pred, opts); - } catch (QueryParseException e) { - // Don't need to show this exception to the user; the only thing that - // changed about pred was its start, and any other QPEs that might happen - // should have already thrown from the constructor. - throw new StorageException(e); - } - // Don't convert start to a limit, since the caller of this method (see - // AndSource) has calculated the actual number to skip. - return read(); + public ResultSet<T> restart(int start, int pageSize) { + opts = opts.withStart(start).withPageSize(pageSize); + return search(); + } + + @Override + public ResultSet<T> restart(Object searchAfter, int pageSize) { + opts = opts.withSearchAfter(searchAfter).withPageSize(pageSize); + return search(); } @Override @@ -125,4 +121,18 @@ public class IndexedQuery<I, T> extends Predicate<T> implements DataSource<T>, P public String toString() { return MoreObjects.toStringHelper("index").add("p", pred).add("opts", opts).toString(); } + + private ResultSet<T> search() { + try { + source = index.getSource(pred, opts); + } catch (QueryParseException e) { + // Don't need to show this exception to the user; the only thing that + // changed about pred was its start, and any other QPEs that might happen + // should have already thrown from the constructor. + throw new StorageException(e); + } + // Don't convert start to a limit, since the caller of this method (see + // AndSource) has calculated the actual number to skip. + return read(); + } } diff --git a/java/com/google/gerrit/index/query/LazyResultSet.java b/java/com/google/gerrit/index/query/LazyResultSet.java index f3fab5f5b5..a7d71f003a 100644 --- a/java/com/google/gerrit/index/query/LazyResultSet.java +++ b/java/com/google/gerrit/index/query/LazyResultSet.java @@ -53,4 +53,9 @@ public class LazyResultSet<T> implements ResultSet<T> { @Override public void close() {} + + @Override + public Object searchAfter() { + return null; + } } diff --git a/java/com/google/gerrit/index/query/ListResultSet.java b/java/com/google/gerrit/index/query/ListResultSet.java index 9d7eadf393..f09fda0cd2 100644 --- a/java/com/google/gerrit/index/query/ListResultSet.java +++ b/java/com/google/gerrit/index/query/ListResultSet.java @@ -54,4 +54,9 @@ public class ListResultSet<T> implements ResultSet<T> { public void close() { results = null; } + + @Override + public Object searchAfter() { + return null; + } } diff --git a/java/com/google/gerrit/index/query/Paginated.java b/java/com/google/gerrit/index/query/Paginated.java index e61dd53234..552199093b 100644 --- a/java/com/google/gerrit/index/query/Paginated.java +++ b/java/com/google/gerrit/index/query/Paginated.java @@ -19,5 +19,7 @@ import com.google.gerrit.index.QueryOptions; public interface Paginated<T> { QueryOptions getOptions(); - ResultSet<T> restart(int start); + ResultSet<T> restart(int start, int pageSize); + + ResultSet<T> restart(Object searchAfter, int pageSize); } diff --git a/java/com/google/gerrit/index/query/PaginatingSource.java b/java/com/google/gerrit/index/query/PaginatingSource.java new file mode 100644 index 0000000000..fd3a2187e6 --- /dev/null +++ b/java/com/google/gerrit/index/query/PaginatingSource.java @@ -0,0 +1,148 @@ +// Copyright (C) 2022 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.index.query; + +import static com.google.common.base.Preconditions.checkArgument; + +import com.google.common.collect.FluentIterable; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Iterables; +import com.google.common.collect.Ordering; +import com.google.gerrit.exceptions.StorageException; +import com.google.gerrit.index.IndexConfig; +import com.google.gerrit.index.PaginationType; +import com.google.gerrit.index.QueryOptions; +import java.util.ArrayList; +import java.util.List; + +public class PaginatingSource<T> implements DataSource<T> { + protected final DataSource<T> source; + private final int start; + private final int cardinality; + private final IndexConfig indexConfig; + + public PaginatingSource(DataSource<T> source, int start, IndexConfig indexConfig) { + checkArgument(start >= 0, "negative start: %s", start); + this.source = source; + this.start = start; + this.cardinality = source.getCardinality(); + this.indexConfig = indexConfig; + } + + @Override + public ResultSet<T> read() { + if (source == null) { + throw new StorageException("No DataSource: " + this); + } + + // ResultSets are lazy. Calling #read here first and then dealing with ResultSets only when + // requested allows the index to run asynchronous queries. + ResultSet<T> resultSet = source.read(); + return new LazyResultSet<>( + () -> { + List<T> r = new ArrayList<>(); + T last = null; + int pageResultSize = 0; + for (T data : buffer(resultSet)) { + if (!isMatchable() || match(data)) { + r.add(data); + } + last = data; + pageResultSize++; + } + + if (last != null && source instanceof Paginated) { + // Restart source and continue if we have not filled the + // full limit the caller wants. + // + @SuppressWarnings("unchecked") + Paginated<T> p = (Paginated<T>) source; + QueryOptions opts = p.getOptions(); + final int limit = opts.limit(); + int pageSize = opts.pageSize(); + int pageSizeMultiplier = opts.pageSizeMultiplier(); + Object searchAfter = resultSet.searchAfter(); + int nextStart = pageResultSize; + while (pageResultSize == pageSize && r.size() <= limit) { // get 1 more than the limit + pageSize = getNextPageSize(pageSize, pageSizeMultiplier); + ResultSet<T> next = + indexConfig.paginationType().equals(PaginationType.SEARCH_AFTER) + ? p.restart(searchAfter, pageSize) + : p.restart(nextStart, pageSize); + pageResultSize = 0; + for (T data : buffer(next)) { + if (match(data)) { + r.add(data); + } + pageResultSize++; + } + nextStart += pageResultSize; + searchAfter = next.searchAfter(); + } + } + + if (start >= r.size()) { + return ImmutableList.of(); + } else if (start > 0) { + return ImmutableList.copyOf(r.subList(start, r.size())); + } + return ImmutableList.copyOf(r); + }); + } + + @Override + public ResultSet<FieldBundle> readRaw() { + // TOOD(hiesel): Implement + throw new UnsupportedOperationException("not implemented"); + } + + private Iterable<T> buffer(ResultSet<T> scanner) { + return FluentIterable.from(Iterables.partition(scanner, 50)) + .transformAndConcat(this::transformBuffer); + } + + protected boolean match(T object) { + return true; + } + + protected boolean isMatchable() { + return true; + } + + protected List<T> transformBuffer(List<T> buffer) { + return buffer; + } + + @Override + public int getCardinality() { + return cardinality; + } + + private int getNextPageSize(int pageSize, int pageSizeMultiplier) { + List<Integer> possiblePageSizes = new ArrayList<>(3); + try { + possiblePageSizes.add(Math.multiplyExact(pageSize, pageSizeMultiplier)); + } catch (ArithmeticException e) { + possiblePageSizes.add(Integer.MAX_VALUE); + } + if (indexConfig.maxPageSize() > 0) { + possiblePageSizes.add(indexConfig.maxPageSize()); + } + if (indexConfig.maxLimit() > 0) { + possiblePageSizes.add(indexConfig.maxLimit()); + } + return Ordering.natural().min(possiblePageSizes); + } +} diff --git a/java/com/google/gerrit/index/query/QueryProcessor.java b/java/com/google/gerrit/index/query/QueryProcessor.java index ea23d91a36..ec90d2145a 100644 --- a/java/com/google/gerrit/index/query/QueryProcessor.java +++ b/java/com/google/gerrit/index/query/QueryProcessor.java @@ -79,7 +79,7 @@ public abstract class QueryProcessor<T> { private final IndexCollection<?, T, ? extends Index<?, T>> indexes; private final IndexRewriter<T> rewriter; private final String limitField; - private final IntSupplier permittedLimit; + private final IntSupplier userQueryLimit; private final CallerFinder callerFinder; // This class is not generally thread-safe, but programmer error may result in it being shared @@ -100,14 +100,14 @@ public abstract class QueryProcessor<T> { IndexCollection<?, T, ? extends Index<?, T>> indexes, IndexRewriter<T> rewriter, String limitField, - IntSupplier permittedLimit) { + IntSupplier userQueryLimit) { this.metrics = new Metrics(metricMaker); this.schemaDef = schemaDef; this.indexConfig = indexConfig; this.indexes = indexes; this.rewriter = rewriter; this.limitField = limitField; - this.permittedLimit = permittedLimit; + this.userQueryLimit = userQueryLimit; this.used = new AtomicBoolean(false); this.callerFinder = CallerFinder.builder() @@ -230,9 +230,10 @@ public abstract class QueryProcessor<T> { checkSupportedForQueries(q); int limit = getEffectiveLimit(q); limits.add(limit); + int initialPageSize = getInitialPageSize(limit); - if (limit == getBackendSupportedLimit()) { - limit--; + if (initialPageSize == getBackendSupportedLimit()) { + initialPageSize--; } int page = (start / limit) + 1; @@ -241,10 +242,31 @@ public abstract class QueryProcessor<T> { "Cannot go beyond page " + indexConfig.maxPages() + " of results"); } - // Always bump limit by 1, even if this results in exceeding the permitted - // max for this user. The only way to see if there are more entities is to - // ask for one more result from the query. - QueryOptions opts = createOptions(indexConfig, start, limit + 1, getRequestedFields()); + // Always bump initial page size by 1, even if this results in exceeding the + // permitted max for this user. The only way to see if there are more entities + // is to ask for one more result from the query. + try { + initialPageSize = Math.addExact(initialPageSize, 1); + } catch (ArithmeticException e) { + initialPageSize = Integer.MAX_VALUE; + } + + // If pageSizeMultiplier is set to 1 (default), update it to 10 for no-limit queries as + // it helps improve performance and also prevents no-limit queries from severely degrading + // when pagination type is OFFSET. + int pageSizeMultiplier = indexConfig.pageSizeMultiplier(); + if (isNoLimit && pageSizeMultiplier == 1) { + pageSizeMultiplier = 10; + } + + QueryOptions opts = + createOptions( + indexConfig, + start, + initialPageSize, + pageSizeMultiplier, + limit, + getRequestedFields()); logger.atFine().log("Query options: " + opts); Predicate<T> pred = rewriter.rewrite(q, opts); if (enforceVisibility) { @@ -259,6 +281,9 @@ public abstract class QueryProcessor<T> { @SuppressWarnings("unchecked") DataSource<T> s = (DataSource<T>) pred; + if (initialPageSize < limit && !(pred instanceof AndSource)) { + s = new PaginatingSource<T>(s, start, indexConfig); + } sources.add(s); } @@ -318,8 +343,14 @@ public abstract class QueryProcessor<T> { } protected QueryOptions createOptions( - IndexConfig indexConfig, int start, int limit, Set<String> requestedFields) { - return QueryOptions.create(indexConfig, start, limit, requestedFields); + IndexConfig indexConfig, + int start, + int pageSize, + int pageSizeMultiplier, + int limit, + Set<String> requestedFields) { + return QueryOptions.create( + indexConfig, start, pageSize, pageSizeMultiplier, limit, requestedFields); } /** @@ -357,14 +388,14 @@ public abstract class QueryProcessor<T> { } private int getPermittedLimit() { - return enforceVisibility ? permittedLimit.getAsInt() : Integer.MAX_VALUE; + return enforceVisibility ? userQueryLimit.getAsInt() : Integer.MAX_VALUE; } private int getBackendSupportedLimit() { return indexConfig.maxLimit(); } - private int getEffectiveLimit(Predicate<T> p) { + public int getEffectiveLimit(Predicate<T> p) { if (isNoLimit == true) { return Integer.MAX_VALUE; } @@ -383,6 +414,7 @@ public abstract class QueryProcessor<T> { int result = Ordering.natural().min(possibleLimits); // Should have short-circuited from #query or thrown some other exception before getting here. checkState(result > 0, "effective limit should be positive"); + return result; } @@ -393,5 +425,17 @@ public abstract class QueryProcessor<T> { .findFirst(); } + protected IntSupplier getUserQueryLimit() { + return userQueryLimit; + } + + protected int getInitialPageSize(int queryLimit) { + return queryLimit; + } + protected abstract String formatForLogging(T t); + + protected abstract int getIndexSize(); + + protected abstract int getBatchSize(); } diff --git a/java/com/google/gerrit/index/query/ResultSet.java b/java/com/google/gerrit/index/query/ResultSet.java index 65fcd4593c..b4bd19e9a7 100644 --- a/java/com/google/gerrit/index/query/ResultSet.java +++ b/java/com/google/gerrit/index/query/ResultSet.java @@ -49,4 +49,6 @@ public interface ResultSet<T> extends Iterable<T> { * the iterator has finished. */ void close(); + + Object searchAfter(); } diff --git a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java index 6ece45afe8..4197c64011 100644 --- a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java +++ b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java @@ -19,6 +19,7 @@ import static com.google.common.collect.ImmutableList.toImmutableList; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableListMultimap; import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Iterables; import com.google.gerrit.entities.Account; import com.google.gerrit.entities.AccountGroup; import com.google.gerrit.entities.Change; @@ -52,6 +53,9 @@ import java.util.Comparator; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.IntStream; +import java.util.stream.Stream; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.lib.Config; /** @@ -69,12 +73,14 @@ public abstract class AbstractFakeIndex<K, V, D> implements Index<K, V> { private final String indexName; private final Map<K, D> indexedDocuments; + private int queryCount; AbstractFakeIndex(Schema<V> schema, SitePaths sitePaths, String indexName) { this.schema = schema; this.sitePaths = sitePaths; this.indexName = indexName; this.indexedDocuments = new HashMap<>(); + this.queryCount = 0; } @Override @@ -108,18 +114,33 @@ public abstract class AbstractFakeIndex<K, V, D> implements Index<K, V> { } } + public int getQueryCount() { + return queryCount; + } + @Override public DataSource<V> getSource(Predicate<V> p, QueryOptions opts) { List<V> results; synchronized (indexedDocuments) { - results = + Stream<V> valueStream = indexedDocuments.values().stream() .map(doc -> valueFor(doc)) .filter(doc -> p.asMatchable().match(doc)) - .sorted(sortingComparator()) - .skip(opts.start()) - .limit(opts.limit()) - .collect(toImmutableList()); + .sorted(sortingComparator()); + if (opts.searchAfter() != null) { + ImmutableList<V> valueList = valueStream.collect(toImmutableList()); + int fromIndex = + IntStream.range(0, valueList.size()) + .filter(i -> keyFor(valueList.get(i)).equals(opts.searchAfter())) + .findFirst() + .orElse(-1) + + 1; + int toIndex = Math.min(fromIndex + opts.pageSize(), valueList.size()); + results = valueList.subList(fromIndex, toIndex); + } else { + results = valueStream.skip(opts.start()).limit(opts.pageSize()).collect(toImmutableList()); + } + queryCount++; } return new DataSource<V>() { @Override @@ -129,12 +150,19 @@ public abstract class AbstractFakeIndex<K, V, D> implements Index<K, V> { @Override public ResultSet<V> read() { - return new ListResultSet<>(results); + return new ListResultSet<>(results) { + @Override + public Object searchAfter() { + @Nullable V last = Iterables.getLast(results, null); + return last != null ? keyFor(last) : null; + } + }; } @Override public ResultSet<FieldBundle> readRaw() { ImmutableList.Builder<FieldBundle> fieldBundles = ImmutableList.builder(); + K searchAfter = null; for (V result : results) { ImmutableListMultimap.Builder<String, Object> fields = ImmutableListMultimap.builder(); for (FieldDef<V, ?> field : getSchema().getFields().values()) { @@ -148,8 +176,16 @@ public abstract class AbstractFakeIndex<K, V, D> implements Index<K, V> { } } fieldBundles.add(new FieldBundle(fields.build())); + searchAfter = keyFor(result); } - return new ListResultSet<>(fieldBundles.build()); + ImmutableList<FieldBundle> resultSet = fieldBundles.build(); + K finalSearchAfter = searchAfter; + return new ListResultSet<>(resultSet) { + @Override + public Object searchAfter() { + return finalSearchAfter; + } + }; } }; } diff --git a/java/com/google/gerrit/lucene/AbstractLuceneIndex.java b/java/com/google/gerrit/lucene/AbstractLuceneIndex.java index 1e6ccac89a..6cede89aa5 100644 --- a/java/com/google/gerrit/lucene/AbstractLuceneIndex.java +++ b/java/com/google/gerrit/lucene/AbstractLuceneIndex.java @@ -535,20 +535,31 @@ public abstract class AbstractLuceneIndex<K, V> implements Index<K, V> { private <T> ResultSet<T> readImpl(Function<Document, T> mapper) { IndexSearcher searcher = null; + ScoreDoc scoreDoc = null; try { searcher = acquire(); - int realLimit = opts.start() + opts.limit(); - TopFieldDocs docs = searcher.search(query, realLimit, sort); + int realLimit = opts.start() + opts.pageSize(); + TopFieldDocs docs = + opts.searchAfter() != null + ? searcher.searchAfter( + (ScoreDoc) opts.searchAfter(), query, realLimit, sort, false, false) + : searcher.search(query, realLimit, sort); ImmutableList.Builder<T> b = ImmutableList.builderWithExpectedSize(docs.scoreDocs.length); for (int i = opts.start(); i < docs.scoreDocs.length; i++) { - ScoreDoc sd = docs.scoreDocs[i]; - Document doc = searcher.doc(sd.doc, opts.fields()); + scoreDoc = docs.scoreDocs[i]; + Document doc = searcher.doc(scoreDoc.doc, opts.fields()); T mapperResult = mapper.apply(doc); if (mapperResult != null) { b.add(mapperResult); } } - return new ListResultSet<>(b.build()); + ScoreDoc searchAfter = scoreDoc; + return new ListResultSet<T>(b.build()) { + @Override + public Object searchAfter() { + return searchAfter; + } + }; } catch (IOException e) { throw new StorageException(e); } finally { diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java index e7d47d0778..efbe0e8f4a 100644 --- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java +++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java @@ -40,6 +40,7 @@ import com.google.gerrit.entities.converter.ChangeProtoConverter; import com.google.gerrit.entities.converter.ProtoConverter; import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.index.FieldDef; +import com.google.gerrit.index.PaginationType; import com.google.gerrit.index.QueryOptions; import com.google.gerrit.index.Schema; import com.google.gerrit.index.query.FieldBundle; @@ -64,8 +65,11 @@ import com.google.protobuf.MessageLite; import java.io.IOException; import java.nio.file.Path; import java.util.ArrayList; +import java.util.Arrays; +import java.util.HashMap; import java.util.Iterator; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; @@ -319,6 +323,7 @@ public class LuceneChangeIndex implements ChangeIndex { private final QueryOptions opts; private final Sort sort; private final Function<Document, FieldBundle> rawDocumentMapper; + private final boolean isSearchAfterPagination; private QuerySource( List<ChangeSubIndex> indexes, @@ -333,6 +338,8 @@ public class LuceneChangeIndex implements ChangeIndex { this.opts = opts; this.sort = sort; this.rawDocumentMapper = rawDocumentMapper; + this.isSearchAfterPagination = + opts.config().paginationType().equals(PaginationType.SEARCH_AFTER); } @Override @@ -360,9 +367,9 @@ public class LuceneChangeIndex implements ChangeIndex { final Set<String> fields = IndexUtils.changeFields(opts, schema.useLegacyNumericFields()); return new ChangeDataResults( executor.submit( - new Callable<List<Document>>() { + new Callable<Results>() { @Override - public List<Document> call() throws IOException { + public Results call() throws IOException { return doRead(fields); } @@ -377,8 +384,12 @@ public class LuceneChangeIndex implements ChangeIndex { @Override public ResultSet<FieldBundle> readRaw() { List<Document> documents; + Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex; + try { - documents = doRead(IndexUtils.changeFields(opts, schema.useLegacyNumericFields())); + Results r = doRead(IndexUtils.changeFields(opts, schema.useLegacyNumericFields())); + documents = r.docs; + searchAfterBySubIndex = r.searchAfterBySubIndex; } catch (IOException e) { throw new StorageException(e); } @@ -399,29 +410,57 @@ public class LuceneChangeIndex implements ChangeIndex { public void close() { // Do nothing. } + + @Override + public Object searchAfter() { + return searchAfterBySubIndex; + } }; } - private List<Document> doRead(Set<String> fields) throws IOException { + private Results doRead(Set<String> fields) throws IOException { IndexSearcher[] searchers = new IndexSearcher[indexes.size()]; + Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex = new HashMap<>(); try { - int realLimit = opts.start() + opts.limit(); - if (Integer.MAX_VALUE - opts.limit() < opts.start()) { - realLimit = Integer.MAX_VALUE; + int realPageSize = opts.start() + opts.pageSize(); + if (Integer.MAX_VALUE - opts.pageSize() < opts.start()) { + realPageSize = Integer.MAX_VALUE; } - TopFieldDocs[] hits = new TopFieldDocs[indexes.size()]; + List<TopFieldDocs> hits = new ArrayList<>(); + int searchAfterHitsCount = 0; for (int i = 0; i < indexes.size(); i++) { - searchers[i] = indexes.get(i).acquire(); - hits[i] = searchers[i].search(query, realLimit, sort); + ChangeSubIndex subIndex = indexes.get(i); + searchers[i] = subIndex.acquire(); + if (isSearchAfterPagination) { + ScoreDoc searchAfter = getSearchAfter(subIndex); + int maxRemainingHits = realPageSize - searchAfterHitsCount; + if (maxRemainingHits > 0) { + TopFieldDocs subIndexHits = + searchers[i].searchAfter( + searchAfter, + query, + maxRemainingHits, + sort, + /* doDocScores= */ false, + /* doMaxScore= */ false); + searchAfterHitsCount += subIndexHits.scoreDocs.length; + hits.add(subIndexHits); + searchAfterBySubIndex.put( + subIndex, Iterables.getLast(Arrays.asList(subIndexHits.scoreDocs), searchAfter)); + } + } else { + hits.add(searchers[i].search(query, realPageSize, sort)); + } } - TopDocs docs = TopDocs.merge(sort, realLimit, hits); + TopDocs docs = + TopDocs.merge(sort, realPageSize, hits.stream().toArray(TopFieldDocs[]::new)); List<Document> result = new ArrayList<>(docs.scoreDocs.length); for (int i = opts.start(); i < docs.scoreDocs.length; i++) { ScoreDoc sd = docs.scoreDocs[i]; result.add(searchers[sd.shardIndex].doc(sd.doc, fields)); } - return result; + return new Results(result, searchAfterBySubIndex); } finally { for (int i = 0; i < indexes.size(); i++) { if (searchers[i] != null) { @@ -434,13 +473,41 @@ public class LuceneChangeIndex implements ChangeIndex { } } } + + /** + * Returns null for the first page or when pagination type is not {@link + * PaginationType#SEARCH_AFTER search-after}, otherwise returns the last doc from previous + * search on the given change sub-index. + * + * @param subIndex change sub-index + * @return the score doc that can be used to page result sets + */ + private ScoreDoc getSearchAfter(ChangeSubIndex subIndex) { + if (isSearchAfterPagination + && opts.searchAfter() != null + && opts.searchAfter() instanceof Map) { + return ((Map<ChangeSubIndex, ScoreDoc>) opts.searchAfter()).get(subIndex); + } + return null; + } + } + + private static class Results { + List<Document> docs; + Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex; + + public Results(List<Document> docs, Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex) { + this.docs = docs; + this.searchAfterBySubIndex = searchAfterBySubIndex; + } } private class ChangeDataResults implements ResultSet<ChangeData> { - private final Future<List<Document>> future; + private final Future<Results> future; private final Set<String> fields; + private Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex; - ChangeDataResults(Future<List<Document>> future, Set<String> fields) { + ChangeDataResults(Future<Results> future, Set<String> fields) { this.future = future; this.fields = fields; } @@ -453,7 +520,9 @@ public class LuceneChangeIndex implements ChangeIndex { @Override public ImmutableList<ChangeData> toList() { try { - List<Document> docs = future.get(); + Results r = future.get(); + List<Document> docs = r.docs; + searchAfterBySubIndex = r.searchAfterBySubIndex; ImmutableList.Builder<ChangeData> result = ImmutableList.builderWithExpectedSize(docs.size()); for (Document doc : docs) { @@ -473,6 +542,11 @@ public class LuceneChangeIndex implements ChangeIndex { public void close() { future.cancel(false /* do not interrupt Lucene */); } + + @Override + public Object searchAfter() { + return searchAfterBySubIndex; + } } private static ListMultimap<String, IndexableField> fields(Document doc, Set<String> fields) { diff --git a/java/com/google/gerrit/metrics/MetricMaker.java b/java/com/google/gerrit/metrics/MetricMaker.java index 42ec8a09a9..618d421e2e 100644 --- a/java/com/google/gerrit/metrics/MetricMaker.java +++ b/java/com/google/gerrit/metrics/MetricMaker.java @@ -78,14 +78,15 @@ public abstract class MetricMaker { * @param name unique name of the metric. * @param value only value of the metric. * @param desc description of the metric. + * @return registration handle */ - public <V> void newConstantMetric(String name, V value, Description desc) { + public <V> RegistrationHandle newConstantMetric(String name, V value, Description desc) { desc.setConstant(); @SuppressWarnings("unchecked") Class<V> type = (Class<V>) value.getClass(); CallbackMetric0<V> metric = newCallbackMetric(name, type, desc); - newTrigger(metric, () -> metric.set(value)); + return newTrigger(metric, () -> metric.set(value)); } /** @@ -107,11 +108,12 @@ public abstract class MetricMaker { * @param valueClass type of value recorded by the metric. * @param desc description of the metric. * @param trigger function to compute the value of the metric. + * @return registration handle */ - public <V> void newCallbackMetric( + public <V> RegistrationHandle newCallbackMetric( String name, Class<V> valueClass, Description desc, Supplier<V> trigger) { CallbackMetric0<V> metric = newCallbackMetric(name, valueClass, desc); - newTrigger(metric, () -> metric.set(trigger.get())); + return newTrigger(metric, () -> metric.set(trigger.get())); } /** diff --git a/java/com/google/gerrit/metrics/MetricsReservoirConfig.java b/java/com/google/gerrit/metrics/MetricsReservoirConfig.java new file mode 100644 index 0000000000..ca4cb09e1e --- /dev/null +++ b/java/com/google/gerrit/metrics/MetricsReservoirConfig.java @@ -0,0 +1,33 @@ +// Copyright (C) 2022 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; + +import java.time.Duration; + +/** Configuration of the Metrics' reservoir type and size. */ +public interface MetricsReservoirConfig { + + /** Returns the reservoir type. */ + ReservoirType reservoirType(); + + /** Returns the reservoir window duration. */ + Duration reservoirWindow(); + + /** Returns the number of samples that the reservoir can contain */ + int reservoirSize(); + + /** Returns the alpha parameter of the ExponentiallyDecaying reservoir */ + double reservoirAlpha(); +} diff --git a/java/com/google/gerrit/metrics/ReservoirType.java b/java/com/google/gerrit/metrics/ReservoirType.java new file mode 100644 index 0000000000..fe8975205f --- /dev/null +++ b/java/com/google/gerrit/metrics/ReservoirType.java @@ -0,0 +1,24 @@ +// Copyright (C) 2022 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; + +/** Type of reservoir for collecting metrics into. */ +public enum ReservoirType { + ExponentiallyDecaying, + SlidingTimeWindowArray, + SlidingTimeWindow, + SlidingWindow, + Uniform; +} diff --git a/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java b/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java index fcba0eeb3c..32be18d981 100644 --- a/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java +++ b/java/com/google/gerrit/metrics/dropwizard/DropWizardMetricMaker.java @@ -18,8 +18,10 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.gerrit.metrics.dropwizard.MetricResource.METRIC_KIND; import static com.google.gerrit.server.config.ConfigResource.CONFIG_KIND; +import com.codahale.metrics.Histogram; import com.codahale.metrics.Metric; import com.codahale.metrics.MetricRegistry; +import com.codahale.metrics.Timer; import com.google.common.collect.FluentIterable; import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; @@ -41,6 +43,7 @@ import com.google.gerrit.metrics.Histogram1; import com.google.gerrit.metrics.Histogram2; import com.google.gerrit.metrics.Histogram3; import com.google.gerrit.metrics.MetricMaker; +import com.google.gerrit.metrics.MetricsReservoirConfig; import com.google.gerrit.metrics.Timer0; import com.google.gerrit.metrics.Timer1; import com.google.gerrit.metrics.Timer2; @@ -48,10 +51,12 @@ import com.google.gerrit.metrics.Timer3; import com.google.gerrit.metrics.proc.JGitMetricModule; import com.google.gerrit.metrics.proc.ProcMetricModule; import com.google.gerrit.server.cache.CacheMetrics; +import com.google.gerrit.server.config.MetricsReservoirConfigImpl; import com.google.inject.Inject; import com.google.inject.Scopes; import com.google.inject.Singleton; import java.util.Map; +import java.util.Optional; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.TimeUnit; @@ -65,8 +70,25 @@ import java.util.regex.Pattern; @Singleton public class DropWizardMetricMaker extends MetricMaker { public static class ApiModule extends RestApiModule { + private final Optional<MetricsReservoirConfig> metricsReservoirConfig; + + public ApiModule(MetricsReservoirConfig metricsReservoirConfig) { + this.metricsReservoirConfig = Optional.of(metricsReservoirConfig); + } + + public ApiModule() { + this.metricsReservoirConfig = Optional.empty(); + } + @Override protected void configure() { + if (metricsReservoirConfig.isPresent()) { + bind(MetricsReservoirConfig.class).toInstance(metricsReservoirConfig.get()); + } else { + bind(MetricsReservoirConfig.class) + .to(MetricsReservoirConfigImpl.class) + .in(Scopes.SINGLETON); + } bind(MetricRegistry.class).in(Scopes.SINGLETON); bind(DropWizardMetricMaker.class).in(Scopes.SINGLETON); bind(MetricMaker.class).to(DropWizardMetricMaker.class); @@ -89,12 +111,14 @@ public class DropWizardMetricMaker extends MetricMaker { private final MetricRegistry registry; private final Map<String, BucketedMetric> bucketed; private final Map<String, ImmutableMap<String, String>> descriptions; + private final MetricsReservoirConfig reservoirConfig; @Inject - DropWizardMetricMaker(MetricRegistry registry) { + DropWizardMetricMaker(MetricRegistry registry, MetricsReservoirConfig reservoirConfig) { this.registry = registry; this.bucketed = new ConcurrentHashMap<>(); this.descriptions = new ConcurrentHashMap<>(); + this.reservoirConfig = reservoirConfig; } Iterable<String> getMetricNames() { @@ -222,7 +246,9 @@ public class DropWizardMetricMaker extends MetricMaker { } TimerImpl newTimerImpl(String name) { - return new TimerImpl(name, registry.timer(name)); + return new TimerImpl( + name, + registry.timer(name, () -> new Timer(DropWizardReservoirProvider.get(reservoirConfig)))); } @Override @@ -271,7 +297,10 @@ public class DropWizardMetricMaker extends MetricMaker { } HistogramImpl newHistogramImpl(String name) { - return new HistogramImpl(name, registry.histogram(name)); + return new HistogramImpl( + name, + registry.histogram( + name, () -> new Histogram(DropWizardReservoirProvider.get(reservoirConfig)))); } @Override diff --git a/java/com/google/gerrit/metrics/dropwizard/DropWizardReservoirProvider.java b/java/com/google/gerrit/metrics/dropwizard/DropWizardReservoirProvider.java new file mode 100644 index 0000000000..30890689cb --- /dev/null +++ b/java/com/google/gerrit/metrics/dropwizard/DropWizardReservoirProvider.java @@ -0,0 +1,52 @@ +// Copyright (C) 2022 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 com.codahale.metrics.ExponentiallyDecayingReservoir; +import com.codahale.metrics.Reservoir; +import com.codahale.metrics.SlidingTimeWindowArrayReservoir; +import com.codahale.metrics.SlidingTimeWindowReservoir; +import com.codahale.metrics.SlidingWindowReservoir; +import com.codahale.metrics.UniformReservoir; +import com.google.gerrit.metrics.MetricsReservoirConfig; +import com.google.gerrit.metrics.ReservoirType; +import java.util.concurrent.TimeUnit; + +class DropWizardReservoirProvider { + + private DropWizardReservoirProvider() {} + + static Reservoir get(MetricsReservoirConfig config) { + ReservoirType reservoirType = config.reservoirType(); + switch (reservoirType) { + case ExponentiallyDecaying: + return new ExponentiallyDecayingReservoir(config.reservoirSize(), config.reservoirAlpha()); + case SlidingTimeWindowArray: + return new SlidingTimeWindowArrayReservoir( + config.reservoirWindow().toMillis(), TimeUnit.MILLISECONDS); + case SlidingTimeWindow: + return new SlidingTimeWindowReservoir( + config.reservoirWindow().toMillis(), TimeUnit.MILLISECONDS); + case SlidingWindow: + return new SlidingWindowReservoir(config.reservoirSize()); + case Uniform: + return new UniformReservoir(config.reservoirSize()); + + default: + throw new IllegalArgumentException( + "Unsupported metrics reservoir type " + reservoirType.name()); + } + } +} diff --git a/java/com/google/gerrit/server/LibModuleLoader.java b/java/com/google/gerrit/server/LibModuleLoader.java index 36765e9e92..f7e04b4409 100644 --- a/java/com/google/gerrit/server/LibModuleLoader.java +++ b/java/com/google/gerrit/server/LibModuleLoader.java @@ -18,6 +18,7 @@ import static java.util.stream.Collectors.toList; import com.google.common.flogger.FluentLogger; import com.google.gerrit.server.config.GerritServerConfig; +import com.google.gerrit.server.index.options.AutoFlush; import com.google.inject.Injector; import com.google.inject.Key; import com.google.inject.Module; @@ -55,9 +56,14 @@ public class LibModuleLoader { try { Method m = - clazz.getMethod("singleVersionWithExplicitVersions", Map.class, int.class, boolean.class); + clazz.getMethod( + "singleVersionWithExplicitVersions", + Map.class, + int.class, + boolean.class, + AutoFlush.class); - Module module = (Module) m.invoke(null, versions, threads, replica); + Module module = (Module) m.invoke(null, versions, threads, replica, AutoFlush.DISABLED); logger.atInfo().log("Installed module %s", className); return module; } catch (Exception e) { diff --git a/java/com/google/gerrit/server/account/AccountAttributeLoader.java b/java/com/google/gerrit/server/account/AccountAttributeLoader.java new file mode 100644 index 0000000000..ae579415ee --- /dev/null +++ b/java/com/google/gerrit/server/account/AccountAttributeLoader.java @@ -0,0 +1,49 @@ +// Copyright (C) 2022 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.server.account; + +import com.google.gerrit.common.Nullable; +import com.google.gerrit.entities.Account; +import com.google.gerrit.server.data.AccountAttribute; +import com.google.inject.Inject; +import java.util.HashMap; +import java.util.Map; + +public class AccountAttributeLoader { + + public interface Factory { + AccountAttributeLoader create(); + } + + private final InternalAccountDirectory directory; + private final Map<Account.Id, AccountAttribute> created = new HashMap<>(); + + @Inject + AccountAttributeLoader(InternalAccountDirectory directory) { + this.directory = directory; + } + + @Nullable + public synchronized AccountAttribute get(@Nullable Account.Id id) { + if (id == null) { + return null; + } + return created.computeIfAbsent(id, k -> new AccountAttribute(k.get())); + } + + public void fill() { + directory.fillAccountAttributeInfo(created.values()); + } +} diff --git a/java/com/google/gerrit/server/account/AccountDirectory.java b/java/com/google/gerrit/server/account/AccountDirectory.java index 98b2ca9937..10aecd300b 100644 --- a/java/com/google/gerrit/server/account/AccountDirectory.java +++ b/java/com/google/gerrit/server/account/AccountDirectory.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.account; import com.google.gerrit.extensions.common.AccountInfo; +import com.google.gerrit.server.data.AccountAttribute; import com.google.gerrit.server.permissions.PermissionBackendException; import java.util.Set; @@ -24,7 +25,7 @@ import java.util.Set; * <p>Implementations supply data to Gerrit about user accounts. */ public abstract class AccountDirectory { - /** Fields to be populated for a REST API response. */ + /** Fields to be populated for SSH or REST API response. */ public enum FillOptions { /** Full name or username. */ NAME, @@ -59,4 +60,6 @@ public abstract class AccountDirectory { public abstract void fillAccountInfo(Iterable<? extends AccountInfo> in, Set<FillOptions> options) throws PermissionBackendException; + + public abstract void fillAccountAttributeInfo(Iterable<? extends AccountAttribute> in); } diff --git a/java/com/google/gerrit/server/account/InternalAccountDirectory.java b/java/com/google/gerrit/server/account/InternalAccountDirectory.java index 130fa444c2..f7b0b60964 100644 --- a/java/com/google/gerrit/server/account/InternalAccountDirectory.java +++ b/java/com/google/gerrit/server/account/InternalAccountDirectory.java @@ -32,6 +32,7 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.externalids.ExternalId; import com.google.gerrit.server.avatar.AvatarProvider; +import com.google.gerrit.server.data.AccountAttribute; import com.google.gerrit.server.permissions.GlobalPermission; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; @@ -52,6 +53,9 @@ import java.util.stream.Stream; @Singleton public class InternalAccountDirectory extends AccountDirectory { static final Set<FillOptions> ID_ONLY = Collections.unmodifiableSet(EnumSet.of(FillOptions.ID)); + static final Set<FillOptions> ALL_ACCOUNT_ATTRIBUTES = + Collections.unmodifiableSet( + EnumSet.of(FillOptions.NAME, FillOptions.EMAIL, FillOptions.USERNAME)); public static class InternalAccountDirectoryModule extends AbstractModule { @Override @@ -129,6 +133,44 @@ public class InternalAccountDirectory extends AccountDirectory { } } + @Override + public void fillAccountAttributeInfo(Iterable<? extends AccountAttribute> in) { + Set<Account.Id> ids = stream(in).map(a -> Account.id(a.accountId)).collect(toSet()); + Map<Account.Id, AccountState> accountStates = accountCache.get(ids); + for (AccountAttribute accountAttribute : in) { + Account.Id id = Account.id(accountAttribute.accountId); + AccountState accountState = accountStates.get(id); + if (accountState != null) { + fill(accountAttribute, accountState, ALL_ACCOUNT_ATTRIBUTES); + } else { + accountAttribute.accountId = null; + } + } + } + + private void fill( + AccountAttribute accountAttribute, AccountState accountState, Set<FillOptions> options) { + Account account = accountState.account(); + if (options.contains(FillOptions.NAME)) { + accountAttribute.name = Strings.emptyToNull(account.fullName()); + if (accountAttribute.name == null) { + accountAttribute.name = accountState.userName().orElse(null); + } + } + if (options.contains(FillOptions.EMAIL)) { + accountAttribute.email = account.preferredEmail(); + } + if (options.contains(FillOptions.USERNAME)) { + accountAttribute.username = accountState.userName().orElse(null); + } + if (options.contains(FillOptions.ID)) { + accountAttribute.accountId = account.id().get(); + } else { + // Was previously set to look up account for filling. + accountAttribute.accountId = null; + } + } + private void fill(AccountInfo info, AccountState accountState, Set<FillOptions> options) { Account account = accountState.account(); if (options.contains(FillOptions.ID)) { diff --git a/java/com/google/gerrit/server/approval/RecursiveApprovalCopier.java b/java/com/google/gerrit/server/approval/RecursiveApprovalCopier.java index 87df46523e..0926dafd52 100644 --- a/java/com/google/gerrit/server/approval/RecursiveApprovalCopier.java +++ b/java/com/google/gerrit/server/approval/RecursiveApprovalCopier.java @@ -101,7 +101,7 @@ public class RecursiveApprovalCopier { @Override public boolean updateChange(ChangeContext ctx) throws IOException { Change change = ctx.getChange(); - ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId()); + ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId(), change.getLastUpdatedOn()); approvalsUtil.persistCopiedApprovals( ctx.getNotes(), ctx.getNotes().getCurrentPatchSet(), diff --git a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java index 23caca7aee..8e06294eeb 100644 --- a/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java +++ b/java/com/google/gerrit/server/cache/mem/DefaultMemoryCacheFactory.java @@ -158,7 +158,7 @@ class DefaultMemoryCacheFactory implements MemoryCacheFactory { Duration refreshAfterWrite = def.refreshAfterWrite(); if (has(def.configKey(), "refreshAfterWrite")) { - builder.expireAfterAccess( + builder.refreshAfterWrite( ConfigUtil.getTimeUnit( cfg, "cache", diff --git a/java/com/google/gerrit/server/config/MetricsReservoirConfigImpl.java b/java/com/google/gerrit/server/config/MetricsReservoirConfigImpl.java new file mode 100644 index 0000000000..ac3c53acf7 --- /dev/null +++ b/java/com/google/gerrit/server/config/MetricsReservoirConfigImpl.java @@ -0,0 +1,96 @@ +// Copyright (C) 2022 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.server.config; + +import com.google.gerrit.metrics.MetricsReservoirConfig; +import com.google.gerrit.metrics.ReservoirType; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.time.Duration; +import java.util.Optional; +import java.util.concurrent.TimeUnit; +import org.eclipse.jgit.lib.Config; + +/** Define metrics reservoir settings based on gerrit.config */ +@Singleton +public class MetricsReservoirConfigImpl implements MetricsReservoirConfig { + private static final double RESERVOIR_ALPHA_DEFAULT = 0.015; + private static final int METRICS_RESERVOIR_SIZE_DEFAULT = 1028; + private static final long METRICS_RESERVOIR_WINDOW_MSEC_DEFAULT = 60000L; + private static final String METRICS_SECTION = "metrics"; + private static final String METRICS_RESERVOIR = "reservoir"; + + private final ReservoirType reservoirType; + + private final Duration reservoirWindow; + private final int reservoirSize; + private final double reservoirAlpha; + + @Inject + MetricsReservoirConfigImpl(@GerritServerConfig Config gerritConfig) { + this.reservoirType = + gerritConfig.getEnum( + METRICS_SECTION, null, METRICS_RESERVOIR, ReservoirType.ExponentiallyDecaying); + + reservoirWindow = + Duration.ofMillis( + ConfigUtil.getTimeUnit( + gerritConfig, + METRICS_SECTION, + reservoirType.name(), + "window", + METRICS_RESERVOIR_WINDOW_MSEC_DEFAULT, + TimeUnit.MILLISECONDS)); + reservoirSize = + gerritConfig.getInt( + METRICS_SECTION, reservoirType.name(), "size", METRICS_RESERVOIR_SIZE_DEFAULT); + reservoirAlpha = + Optional.ofNullable(gerritConfig.getString(METRICS_SECTION, reservoirType.name(), "alpha")) + .map(Double::parseDouble) + .orElse(RESERVOIR_ALPHA_DEFAULT); + } + + /* (non-Javadoc) + * @see com.google.gerrit.server.config.MetricsConfig#reservoirType() + */ + @Override + public ReservoirType reservoirType() { + return reservoirType; + } + + /* (non-Javadoc) + * @see com.google.gerrit.server.config.MetricsConfig#reservoirWindow() + */ + @Override + public Duration reservoirWindow() { + return reservoirWindow; + } + + /* (non-Javadoc) + * @see com.google.gerrit.server.config.MetricsConfig#reservoirSize() + */ + @Override + public int reservoirSize() { + return reservoirSize; + } + + /* (non-Javadoc) + * @see com.google.gerrit.server.config.MetricsConfig#reservoirAlpha() + */ + @Override + public double reservoirAlpha() { + return reservoirAlpha; + } +} diff --git a/java/com/google/gerrit/server/data/AccountAttribute.java b/java/com/google/gerrit/server/data/AccountAttribute.java index 19605a2c7b..9be221b632 100644 --- a/java/com/google/gerrit/server/data/AccountAttribute.java +++ b/java/com/google/gerrit/server/data/AccountAttribute.java @@ -18,4 +18,11 @@ public class AccountAttribute { public String name; public String email; public String username; + public Integer accountId; + + public AccountAttribute(Integer id) { + this.accountId = id; + } + + public AccountAttribute() {} } diff --git a/java/com/google/gerrit/server/events/EventFactory.java b/java/com/google/gerrit/server/events/EventFactory.java index a7fea3c1c0..8b19ecb585 100644 --- a/java/com/google/gerrit/server/events/EventFactory.java +++ b/java/com/google/gerrit/server/events/EventFactory.java @@ -36,6 +36,7 @@ import com.google.gerrit.exceptions.StorageException; import com.google.gerrit.extensions.registration.DynamicItem; import com.google.gerrit.index.IndexConfig; import com.google.gerrit.server.GerritPersonIdent; +import com.google.gerrit.server.account.AccountAttributeLoader; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.account.Emails; @@ -120,7 +121,7 @@ public class EventFactory { this.accountTemplateUtil = accountTemplateUtil; } - public ChangeAttribute asChangeAttribute(Change change) { + public ChangeAttribute asChangeAttribute(Change change, AccountAttributeLoader accountLoader) { ChangeAttribute a = new ChangeAttribute(); a.project = change.getProject().get(); a.branch = change.getDest().shortName(); @@ -128,15 +129,9 @@ public class EventFactory { a.id = change.getKey().get(); a.number = change.getId().get(); a.subject = change.getSubject(); - try { - a.commitMessage = changeDataFactory.create(change).commitMessage(); - } catch (Exception e) { - logger.atSevere().withCause(e).log( - "Error while getting full commit message for change %d", a.number); - } a.url = getChangeUrl(change); - a.owner = asAccountAttribute(change.getOwner()); - a.assignee = asAccountAttribute(change.getAssignee()); + a.owner = asAccountAttribute(change.getOwner(), accountLoader); + a.assignee = asAccountAttribute(change.getAssignee(), accountLoader); a.status = change.getStatus(); a.createdOn = change.getCreatedOn().getTime() / 1000L; a.wip = change.isWorkInProgress() ? true : null; @@ -150,12 +145,9 @@ public class EventFactory { /** Create a {@link ChangeAttribute} instance from the specified change. */ public ChangeAttribute asChangeAttribute(Change change, ChangeNotes notes) { - ChangeAttribute a = asChangeAttribute(change); - Set<String> hashtags = notes.load().getHashtags(); - if (!hashtags.isEmpty()) { - a.hashtags = new ArrayList<>(hashtags.size()); - a.hashtags.addAll(hashtags); - } + ChangeAttribute a = asChangeAttribute(change, (AccountAttributeLoader) null); + addHashTags(a, notes); + addCommitMessage(a, notes); return a; } /** @@ -179,25 +171,27 @@ public class EventFactory { } /** Add allReviewers to an existing {@link ChangeAttribute}. */ - public void addAllReviewers(ChangeAttribute a, ChangeNotes notes) { + public void addAllReviewers( + ChangeAttribute a, ChangeNotes notes, AccountAttributeLoader accountLoader) { Collection<Account.Id> reviewers = approvalsUtil.getReviewers(notes).all(); if (!reviewers.isEmpty()) { a.allReviewers = Lists.newArrayListWithCapacity(reviewers.size()); for (Account.Id id : reviewers) { - a.allReviewers.add(asAccountAttribute(id)); + a.allReviewers.add(asAccountAttribute(id, accountLoader)); } } } /** Add submitRecords to an existing {@link ChangeAttribute}. */ - public void addSubmitRecords(ChangeAttribute ca, List<SubmitRecord> submitRecords) { + public void addSubmitRecords( + ChangeAttribute ca, List<SubmitRecord> submitRecords, AccountAttributeLoader accountLoader) { ca.submitRecords = new ArrayList<>(); for (SubmitRecord submitRecord : submitRecords) { SubmitRecordAttribute sa = new SubmitRecordAttribute(); sa.status = submitRecord.status.name(); if (submitRecord.status != SubmitRecord.Status.RULE_ERROR) { - addSubmitRecordLabels(submitRecord, sa); + addSubmitRecordLabels(submitRecord, sa, accountLoader); addSubmitRecordRequirements(submitRecord, sa); } ca.submitRecords.add(sa); @@ -208,7 +202,8 @@ public class EventFactory { } } - private void addSubmitRecordLabels(SubmitRecord submitRecord, SubmitRecordAttribute sa) { + private void addSubmitRecordLabels( + SubmitRecord submitRecord, SubmitRecordAttribute sa, AccountAttributeLoader accountLoader) { if (submitRecord.labels != null && !submitRecord.labels.isEmpty()) { sa.labels = new ArrayList<>(); for (SubmitRecord.Label lbl : submitRecord.labels) { @@ -216,7 +211,7 @@ public class EventFactory { la.label = lbl.label; la.status = lbl.status.name(); if (lbl.appliedBy != null) { - la.by = asAccountAttribute(lbl.appliedBy); + la.by = asAccountAttribute(lbl.appliedBy, accountLoader); } sa.labels.add(la); } @@ -351,13 +346,23 @@ public class EventFactory { a.commitMessage = commitMessage; } + private void addCommitMessage(ChangeAttribute changeAttribute, ChangeNotes notes) { + try { + addCommitMessage(changeAttribute, changeDataFactory.create(notes).commitMessage()); + } catch (Exception e) { + logger.atSevere().withCause(e).log( + "Error while getting full commit message for change %d", changeAttribute.number); + } + } + public void addPatchSets( RevWalk revWalk, ChangeAttribute ca, Collection<PatchSet> ps, Map<PatchSet.Id, Collection<PatchSetApproval>> approvals, - LabelTypes labelTypes) { - addPatchSets(revWalk, ca, ps, approvals, false, null, labelTypes); + LabelTypes labelTypes, + AccountAttributeLoader accountLoader) { + addPatchSets(revWalk, ca, ps, approvals, false, null, labelTypes, accountLoader); } public void addPatchSets( @@ -367,13 +372,14 @@ public class EventFactory { Map<PatchSet.Id, Collection<PatchSetApproval>> approvals, boolean includeFiles, Change change, - LabelTypes labelTypes) { + LabelTypes labelTypes, + AccountAttributeLoader accountLoader) { if (!ps.isEmpty()) { ca.patchSets = new ArrayList<>(ps.size()); for (PatchSet p : ps) { - PatchSetAttribute psa = asPatchSetAttribute(revWalk, change, p); + PatchSetAttribute psa = asPatchSetAttribute(revWalk, change, p, accountLoader); if (approvals != null) { - addApprovals(psa, p.id(), approvals, labelTypes); + addApprovals(psa, p.id(), approvals, labelTypes, accountLoader); } ca.patchSets.add(psa); if (includeFiles) { @@ -384,13 +390,15 @@ public class EventFactory { } public void addPatchSetComments( - PatchSetAttribute patchSetAttribute, Collection<HumanComment> comments) { + PatchSetAttribute patchSetAttribute, + Collection<HumanComment> comments, + AccountAttributeLoader accountLoader) { for (HumanComment comment : comments) { if (comment.key.patchSetId == patchSetAttribute.number) { if (patchSetAttribute.comments == null) { patchSetAttribute.comments = new ArrayList<>(); } - patchSetAttribute.comments.add(asPatchSetLineAttribute(comment)); + patchSetAttribute.comments.add(asPatchSetLineAttribute(comment, accountLoader)); } } } @@ -420,22 +428,30 @@ public class EventFactory { } } - public void addComments(ChangeAttribute ca, Collection<ChangeMessage> messages) { + public void addComments( + ChangeAttribute ca, + Collection<ChangeMessage> messages, + AccountAttributeLoader accountLoader) { if (!messages.isEmpty()) { ca.comments = new ArrayList<>(); for (ChangeMessage message : messages) { - ca.comments.add(asMessageAttribute(message)); + ca.comments.add(asMessageAttribute(message, accountLoader)); } } } - /** Create a PatchSetAttribute for the given patchset suitable for serialization to JSON. */ public PatchSetAttribute asPatchSetAttribute(RevWalk revWalk, Change change, PatchSet patchSet) { + return asPatchSetAttribute(revWalk, change, patchSet, null); + } + + /** Create a PatchSetAttribute for the given patchset suitable for serialization to JSON. */ + public PatchSetAttribute asPatchSetAttribute( + RevWalk revWalk, Change change, PatchSet patchSet, AccountAttributeLoader accountLoader) { PatchSetAttribute p = new PatchSetAttribute(); p.revision = patchSet.commitId().name(); p.number = patchSet.number(); p.ref = patchSet.refName(); - p.uploader = asAccountAttribute(patchSet.uploader()); + p.uploader = asAccountAttribute(patchSet.uploader(), accountLoader); p.createdOn = patchSet.createdOn().getTime() / 1000L; PatchSet.Id pId = patchSet.id(); try { @@ -452,7 +468,7 @@ public class EventFactory { p.author.name = author.getName(); p.author.username = ""; } else { - p.author = asAccountAttribute(author.getAccount()); + p.author = asAccountAttribute(author.getAccount(), accountLoader); } Map<String, FileDiffOutput> modifiedFiles = @@ -475,20 +491,24 @@ public class EventFactory { PatchSetAttribute p, PatchSet.Id id, Map<PatchSet.Id, Collection<PatchSetApproval>> all, - LabelTypes labelTypes) { + LabelTypes labelTypes, + AccountAttributeLoader accountLoader) { Collection<PatchSetApproval> list = all.get(id); if (list != null) { - addApprovals(p, list, labelTypes); + addApprovals(p, list, labelTypes, accountLoader); } } public void addApprovals( - PatchSetAttribute p, Collection<PatchSetApproval> list, LabelTypes labelTypes) { + PatchSetAttribute p, + Collection<PatchSetApproval> list, + LabelTypes labelTypes, + AccountAttributeLoader accountLoader) { if (!list.isEmpty()) { p.approvals = new ArrayList<>(list.size()); for (PatchSetApproval a : list) { if (a.value() != 0) { - p.approvals.add(asApprovalAttribute(a, labelTypes)); + p.approvals.add(asApprovalAttribute(a, labelTypes, accountLoader)); } } if (p.approvals.isEmpty()) { @@ -497,6 +517,10 @@ public class EventFactory { } } + public AccountAttribute asAccountAttribute(Account.Id id, AccountAttributeLoader accountLoader) { + return accountLoader != null ? accountLoader.get(id) : asAccountAttribute(id); + } + /** Create an AuthorAttribute for the given account suitable for serialization to JSON. */ public AccountAttribute asAccountAttribute(Account.Id id) { if (id == null) { @@ -528,11 +552,12 @@ public class EventFactory { * @param labelTypes label types for the containing project * @return object suitable for serialization to JSON */ - public ApprovalAttribute asApprovalAttribute(PatchSetApproval approval, LabelTypes labelTypes) { + public ApprovalAttribute asApprovalAttribute( + PatchSetApproval approval, LabelTypes labelTypes, AccountAttributeLoader accountLoader) { ApprovalAttribute a = new ApprovalAttribute(); a.type = approval.labelId().get(); a.value = Short.toString(approval.value()); - a.by = asAccountAttribute(approval.accountId()); + a.by = asAccountAttribute(approval.accountId(), accountLoader); a.grantedOn = approval.granted().getTime() / 1000L; a.oldValue = null; @@ -541,20 +566,22 @@ public class EventFactory { return a; } - public MessageAttribute asMessageAttribute(ChangeMessage message) { + public MessageAttribute asMessageAttribute( + ChangeMessage message, AccountAttributeLoader accountLoader) { MessageAttribute a = new MessageAttribute(); a.timestamp = message.getWrittenOn().getTime() / 1000L; a.reviewer = message.getAuthor() != null - ? asAccountAttribute(message.getAuthor()) + ? asAccountAttribute(message.getAuthor(), accountLoader) : asAccountAttribute(myIdent.get()); a.message = accountTemplateUtil.replaceTemplates(message.getMessage()); return a; } - public PatchSetCommentAttribute asPatchSetLineAttribute(HumanComment c) { + public PatchSetCommentAttribute asPatchSetLineAttribute( + HumanComment c, AccountAttributeLoader accountLoader) { PatchSetCommentAttribute a = new PatchSetCommentAttribute(); - a.reviewer = asAccountAttribute(c.author.getId()); + a.reviewer = asAccountAttribute(c.author.getId(), accountLoader); a.file = c.key.filename; a.line = c.lineNbr; a.message = c.message; @@ -568,4 +595,12 @@ public class EventFactory { } return null; } + + private void addHashTags(ChangeAttribute changeAttribute, ChangeNotes notes) { + Set<String> hashtags = notes.load().getHashtags(); + if (!hashtags.isEmpty()) { + changeAttribute.hashtags = new ArrayList<>(hashtags.size()); + changeAttribute.hashtags.addAll(hashtags); + } + } } diff --git a/java/com/google/gerrit/server/events/EventTypes.java b/java/com/google/gerrit/server/events/EventTypes.java index 5498ec8eb1..229ef86603 100644 --- a/java/com/google/gerrit/server/events/EventTypes.java +++ b/java/com/google/gerrit/server/events/EventTypes.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.events; +import com.google.common.collect.ImmutableMap; import java.util.HashMap; import java.util.Map; @@ -61,4 +62,15 @@ public class EventTypes { public static Class<?> getClass(String type) { return typesByString.get(type); } + + /** + * Get a copy of all currently registered events. + * + * <p>The key is the one given to the evenType parameter of the {@link #register} method. + * + * @return ImmutableMap of event types, Event classes. + */ + public static Map<String, Class<?>> getRegisteredEvents() { + return ImmutableMap.copyOf(typesByString); + } } diff --git a/java/com/google/gerrit/server/git/DelegateRepository.java b/java/com/google/gerrit/server/git/DelegateRepository.java index 9ead038fc2..9046d9d4c2 100644 --- a/java/com/google/gerrit/server/git/DelegateRepository.java +++ b/java/com/google/gerrit/server/git/DelegateRepository.java @@ -65,7 +65,8 @@ public class DelegateRepository extends Repository { this.delegate = delegate; } - Repository delegate() { + /** Returns the wrapped {@link Repository} instance. */ + public Repository delegate() { return delegate; } @@ -214,12 +215,12 @@ public class DelegateRepository extends Repository { } @Override - public Set<ObjectId> getAdditionalHaves() { + public Set<ObjectId> getAdditionalHaves() throws IOException { return delegate.getAdditionalHaves(); } @Override - public Map<AnyObjectId, Set<Ref>> getAllRefsByPeeledObjectId() { + public Map<AnyObjectId, Set<Ref>> getAllRefsByPeeledObjectId() throws IOException { return delegate.getAllRefsByPeeledObjectId(); } diff --git a/java/com/google/gerrit/server/git/HookUtil.java b/java/com/google/gerrit/server/git/HookUtil.java index fd29c8debc..cafa18ef79 100644 --- a/java/com/google/gerrit/server/git/HookUtil.java +++ b/java/com/google/gerrit/server/git/HookUtil.java @@ -43,12 +43,12 @@ public class HookUtil { refs = rp.getRepository().getRefDatabase().getRefs().stream() .collect(toMap(Ref::getName, r -> r)); + rp.setAdvertisedRefs(refs, rp.getAdvertisedObjects()); } catch (ServiceMayNotContinueException e) { throw e; } catch (IOException e) { throw new ServiceMayNotContinueException(e); } - rp.setAdvertisedRefs(refs, rp.getAdvertisedObjects()); return refs; } @@ -70,12 +70,12 @@ public class HookUtil { refs = up.getRepository().getRefDatabase().getRefs().stream() .collect(toMap(Ref::getName, r -> r)); + up.setAdvertisedRefs(refs); } catch (ServiceMayNotContinueException e) { throw e; } catch (IOException e) { throw new ServiceMayNotContinueException(e); } - up.setAdvertisedRefs(refs); return refs; } diff --git a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java index 85d7db0801..aa4e88e71d 100644 --- a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java @@ -36,6 +36,7 @@ import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.metrics.Timer1; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.PublishCommentsOp; +import com.google.gerrit.server.cache.PerThreadCache; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.config.ConfigUtil; import com.google.gerrit.server.config.GerritServerConfig; @@ -387,7 +388,7 @@ public class AsyncReceiveCommits { () -> { String oldName = Thread.currentThread().getName(); Thread.currentThread().setName(oldName + "-for-" + currentThreadName); - try { + try (PerThreadCache threadLocalCache = PerThreadCache.create()) { return receiveCommits.processCommands(commands, monitor); } finally { Thread.currentThread().setName(oldName); diff --git a/java/com/google/gerrit/server/git/receive/HackPushNegotiateHook.java b/java/com/google/gerrit/server/git/receive/HackPushNegotiateHook.java index 72483af881..12666f943d 100644 --- a/java/com/google/gerrit/server/git/receive/HackPushNegotiateHook.java +++ b/java/com/google/gerrit/server/git/receive/HackPushNegotiateHook.java @@ -80,13 +80,13 @@ public class HackPushNegotiateHook implements AdvertiseRefsHook { r = rp.getRepository().getRefDatabase().getRefs().stream() .collect(toMap(Ref::getName, x -> x)); + rp.setAdvertisedRefs(r, history(r.values(), rp)); } catch (ServiceMayNotContinueException e) { throw e; } catch (IOException e) { throw new ServiceMayNotContinueException(e); } } - rp.setAdvertisedRefs(r, history(r.values(), rp)); } private Set<ObjectId> history(Collection<Ref> refs, ReceivePack rp) { diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 55d7af00f6..9a59f4682d 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -1446,7 +1446,7 @@ class ReceiveCommits { // Must pass explicit user instead of injecting a provider into CreateRefControl, since // Provider<CurrentUser> within ReceiveCommits will always return anonymous. createRefControl.checkCreateRef( - Providers.of(user), receivePack.getRepository(), branch, obj); + Providers.of(user), receivePack.getRepository(), branch, obj, /* forPush= */ true); } catch (AuthException denied) { rejectProhibited(cmd, denied); return; @@ -3472,7 +3472,7 @@ class ReceiveCommits { rw.markStart(newTip); rw.markUninteresting(rw.parseCommit(cmd.getOldId())); - Map<Change.Key, ChangeNotes> byKey = null; + Map<Change.Key, ChangeData> changeDataByKey = null; List<ReplaceRequest> replaceAndClose = new ArrayList<>(); int existingPatchSets = 0; @@ -3508,8 +3508,8 @@ class ReceiveCommits { for (String changeId : ChangeUtil.getChangeIdsFromFooter(c, urlFormatter.get())) { - if (byKey == null) { - byKey = + if (changeDataByKey == null) { + changeDataByKey = retryHelper .changeIndexQuery( "queryOpenChangesByKeyByBranch", @@ -3517,14 +3517,15 @@ class ReceiveCommits { .call(); } - ChangeNotes onto = byKey.get(Change.key(changeId.trim())); + ChangeData onto = changeDataByKey.get(Change.key(changeId.trim())); if (onto != null) { newPatchSets++; // Hold onto this until we're done with the walk, as the call to // req.validate below calls isMergedInto which resets the walk. + ChangeNotes ontoNotes = onto.notes(); ReplaceRequest req = - new ReplaceRequest(onto.getChangeId(), c, cmd, false); - req.notes = onto; + new ReplaceRequest(ontoNotes.getChangeId(), c, cmd, false); + req.notes = ontoNotes; replaceAndClose.add(req); continue COMMIT; } @@ -3597,14 +3598,17 @@ class ReceiveCommits { } } - private Map<Change.Key, ChangeNotes> openChangesByKeyByBranch( + private Map<Change.Key, ChangeData> openChangesByKeyByBranch( InternalChangeQuery internalChangeQuery, BranchNameKey branch) { try (TraceTimer traceTimer = newTimer("openChangesByKeyByBranch", Metadata.builder().branchName(branch.branch()))) { - Map<Change.Key, ChangeNotes> r = new HashMap<>(); + Map<Change.Key, ChangeData> r = new HashMap<>(); for (ChangeData cd : internalChangeQuery.byBranchOpen(branch)) { try { - r.put(cd.change().getKey(), cd.notes()); + // ChangeData is not materialised into a ChangeNotes for avoiding + // to load a potentially large number of changes meta-data into memory + // which would cause unnecessary disk I/O, CPU and heap utilisation. + r.put(cd.change().getKey(), cd); } catch (NoSuchChangeException e) { // Ignore deleted change } diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java index cc203ad43a..e545c707f6 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommitsAdvertiseRefsHook.java @@ -91,7 +91,11 @@ public class ReceiveCommitsAdvertiseRefsHook implements AdvertiseRefsHook { .filter(ReceiveCommitsAdvertiseRefsHook::skip) .collect(toImmutableList()) .forEach(r -> advertisedRefs.remove(r)); - rp.setAdvertisedRefs(advertisedRefs, advertiseOpenChanges(rp.getRepository())); + try { + rp.setAdvertisedRefs(advertisedRefs, advertiseOpenChanges(rp.getRepository())); + } catch (IOException e) { + throw new ServiceMayNotContinueException(e); + } } private Set<ObjectId> advertiseOpenChanges(Repository repo) diff --git a/java/com/google/gerrit/server/git/validators/AccountValidator.java b/java/com/google/gerrit/server/git/validators/AccountValidator.java index 4755f5ff56..c2a504715b 100644 --- a/java/com/google/gerrit/server/git/validators/AccountValidator.java +++ b/java/com/google/gerrit/server/git/validators/AccountValidator.java @@ -92,7 +92,7 @@ public class AccountValidator { return ImmutableList.of(String.format("account '%s' does not exist", accountId.get())); } - if (accountId.equals(self.get().getAccountId()) && !newAccount.get().isActive()) { + if (!newAccount.get().isActive() && accountId.equals(self.get().getAccountId())) { messages.add("cannot deactivate own account"); } diff --git a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java index 601ac59689..12d8c936d6 100644 --- a/java/com/google/gerrit/server/group/testing/TestGroupBackend.java +++ b/java/com/google/gerrit/server/group/testing/TestGroupBackend.java @@ -118,6 +118,13 @@ public class TestGroupBackend implements GroupBackend { @Override public Collection<GroupReference> suggest(String name, ProjectState project) { + AccountGroup.UUID uuid = AccountGroup.uuid(name); + if (handles(uuid)) { + GroupDescription.Basic g = get(uuid); + if (g != null) { + return ImmutableList.of(GroupReference.forGroup(g)); + } + } return ImmutableList.of(); } diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java index 63c52977a6..35d167a5ce 100644 --- a/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java +++ b/java/com/google/gerrit/server/index/change/ChangeIndexRewriter.java @@ -281,7 +281,7 @@ public class ChangeIndexRewriter implements IndexRewriter<ChangeData> { private Predicate<ChangeData> copy(Predicate<ChangeData> in, List<Predicate<ChangeData>> all) { if (in instanceof AndPredicate) { - return new AndChangeSource(all); + return new AndChangeSource(all, config); } else if (in instanceof OrPredicate) { return new OrSource(all); } diff --git a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java index 57a2091390..1d9bbcd4a3 100644 --- a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java +++ b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java @@ -34,6 +34,7 @@ import com.google.gerrit.index.query.QueryParseException; import com.google.gerrit.index.query.ResultSet; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeDataSource; +import com.google.gerrit.server.query.change.ChangeIndexPostFilterPredicate; import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; @@ -51,24 +52,40 @@ import java.util.Set; public class IndexedChangeQuery extends IndexedQuery<Change.Id, ChangeData> implements ChangeDataSource, Matchable<ChangeData> { public static QueryOptions oneResult() { - return createOptions(IndexConfig.createDefault(), 0, 1, ImmutableSet.of()); + IndexConfig config = IndexConfig.createDefault(); + return createOptions(config, 0, 1, config.pageSizeMultiplier(), 1, ImmutableSet.of()); } public static QueryOptions createOptions( IndexConfig config, int start, int limit, Set<String> fields) { + return createOptions(config, start, limit, config.pageSizeMultiplier(), limit, fields); + } + + public static QueryOptions createOptions( + IndexConfig config, + int start, + int pageSize, + int pageSizeMultiplier, + int limit, + Set<String> fields) { // Always include project since it is needed to load the change from NoteDb. if (!fields.contains(CHANGE.getName()) && !fields.contains(PROJECT.getName())) { fields = new HashSet<>(fields); fields.add(PROJECT.getName()); } - return QueryOptions.create(config, start, limit, fields); + return QueryOptions.create(config, start, pageSize, pageSizeMultiplier, limit, fields); } @VisibleForTesting static QueryOptions convertOptions(QueryOptions opts) { opts = opts.convertForBackend(); return IndexedChangeQuery.createOptions( - opts.config(), opts.start(), opts.limit(), opts.fields()); + opts.config(), + opts.start(), + opts.pageSize(), + opts.pageSizeMultiplier(), + opts.limit(), + opts.fields()); } private final Map<ChangeData, DataSource<ChangeData>> fromSource; @@ -109,16 +126,38 @@ public class IndexedChangeQuery extends IndexedQuery<Change.Id, ChangeData> public void close() { rs.close(); } + + @Override + public Object searchAfter() { + return rs.searchAfter(); + } }; } + public boolean postIndexMatch(Predicate<ChangeData> pred, ChangeData cd) { + if (pred instanceof ChangeIndexPostFilterPredicate) { + checkState( + pred.isMatchable(), + "match invoked, but child predicate %s doesn't implement %s", + pred, + Matchable.class.getName()); + return pred.asMatchable().match(cd); + } + for (int i = 0; i < pred.getChildCount(); i++) { + if (!postIndexMatch(pred.getChild(i), cd)) { + return false; + } + } + return true; + } + @Override public boolean match(ChangeData cd) { - if (source != null && fromSource.get(cd) == source) { + Predicate<ChangeData> pred = getChild(0); + if (source != null && fromSource.get(cd) == source && postIndexMatch(pred, cd)) { return true; } - Predicate<ChangeData> pred = getChild(0); checkState( pred.isMatchable(), "match invoked, but child predicate %s doesn't implement %s", diff --git a/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java b/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java index 90070b6779..8b0f1f8e93 100644 --- a/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java +++ b/java/com/google/gerrit/server/index/group/IndexedGroupQuery.java @@ -34,13 +34,13 @@ public class IndexedGroupQuery extends IndexedQuery<AccountGroup.UUID, InternalG implements DataSource<InternalGroup> { public static QueryOptions createOptions( - IndexConfig config, int start, int limit, Set<String> fields) { + IndexConfig config, int start, int pageSize, int limit, Set<String> fields) { // Always include GroupField.UUID since it is needed to load the group from NoteDb. if (!fields.contains(GroupField.UUID.getName())) { fields = new HashSet<>(fields); fields.add(GroupField.UUID.getName()); } - return QueryOptions.create(config, start, limit, fields); + return QueryOptions.create(config, start, pageSize, limit, fields); } public IndexedGroupQuery( diff --git a/java/com/google/gerrit/server/index/group/StalenessChecker.java b/java/com/google/gerrit/server/index/group/StalenessChecker.java index 4ce3f5b744..4e992cb659 100644 --- a/java/com/google/gerrit/server/index/group/StalenessChecker.java +++ b/java/com/google/gerrit/server/index/group/StalenessChecker.java @@ -66,7 +66,7 @@ public class StalenessChecker { } Optional<FieldBundle> result = - i.getRaw(uuid, IndexedGroupQuery.createOptions(indexConfig, 0, 1, FIELDS)); + i.getRaw(uuid, IndexedGroupQuery.createOptions(indexConfig, 0, 1, 1, FIELDS)); if (!result.isPresent()) { // The document is missing in the index. try (Repository repo = repoManager.openRepository(allUsers)) { diff --git a/java/com/google/gerrit/server/notedb/RepoSequence.java b/java/com/google/gerrit/server/notedb/RepoSequence.java index 47e12ff4bd..d743921362 100644 --- a/java/com/google/gerrit/server/notedb/RepoSequence.java +++ b/java/com/google/gerrit/server/notedb/RepoSequence.java @@ -340,4 +340,16 @@ public class RepoSequence { counterLock.unlock(); } } + + /** + * Retrieves the last returned sequence number. + * + * <p>Explicitly calls {@link #next()} if this instance didn't return sequence number until now. + */ + public int last() { + if (counter == 0) { + next(); + } + return counter - 1; + } } diff --git a/java/com/google/gerrit/server/notedb/Sequences.java b/java/com/google/gerrit/server/notedb/Sequences.java index 7ae987788e..b42253eced 100644 --- a/java/com/google/gerrit/server/notedb/Sequences.java +++ b/java/com/google/gerrit/server/notedb/Sequences.java @@ -55,6 +55,9 @@ public class Sequences { private final RepoSequence changeSeq; private final RepoSequence groupSeq; private final Timer2<SequenceType, Boolean> nextIdLatency; + private final int accountBatchSize; + private final int changeBatchSize; + private final int groupBatchSize = 1; @Inject public Sequences( @@ -65,7 +68,7 @@ public class Sequences { AllUsersName allUsers, MetricMaker metrics) { - int accountBatchSize = + accountBatchSize = cfg.getInt( SECTION_NOTEDB, NAME_ACCOUNTS, @@ -80,7 +83,7 @@ public class Sequences { () -> FIRST_ACCOUNT_ID, accountBatchSize); - int changeBatchSize = + changeBatchSize = cfg.getInt( SECTION_NOTEDB, NAME_CHANGES, @@ -95,7 +98,6 @@ public class Sequences { () -> FIRST_CHANGE_ID, changeBatchSize); - int groupBatchSize = 1; groupSeq = new RepoSequence( repoManager, @@ -147,6 +149,18 @@ public class Sequences { } } + public int changeBatchSize() { + return changeBatchSize; + } + + public int groupBatchSize() { + return groupBatchSize; + } + + public int accountBatchSize() { + return accountBatchSize; + } + public int currentChangeId() { return changeSeq.current(); } @@ -159,6 +173,18 @@ public class Sequences { return groupSeq.current(); } + public int lastChangeId() { + return changeSeq.last(); + } + + public int lastGroupId() { + return groupSeq.last(); + } + + public int lastAccountId() { + return accountSeq.last(); + } + public void setChangeIdValue(int value) { changeSeq.storeNew(value); } diff --git a/java/com/google/gerrit/server/project/CreateRefControl.java b/java/com/google/gerrit/server/project/CreateRefControl.java index 2e76949942..ab134b50ce 100644 --- a/java/com/google/gerrit/server/project/CreateRefControl.java +++ b/java/com/google/gerrit/server/project/CreateRefControl.java @@ -25,10 +25,13 @@ import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.permissions.PermissionBackend; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.RefPermission; +import com.google.gerrit.server.query.change.ChangeData; +import com.google.gerrit.server.update.RetryHelper; import com.google.inject.Inject; import com.google.inject.Provider; import com.google.inject.Singleton; import java.io.IOException; +import java.util.List; import java.util.Optional; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.PersonIdent; @@ -41,18 +44,24 @@ import org.eclipse.jgit.revwalk.RevWalk; /** Manages access control for creating Git references (aka branches, tags). */ @Singleton public class CreateRefControl { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); private final PermissionBackend permissionBackend; private final ProjectCache projectCache; private final Reachable reachable; + private final RetryHelper retryHelper; @Inject CreateRefControl( - PermissionBackend permissionBackend, ProjectCache projectCache, Reachable reachable) { + PermissionBackend permissionBackend, + ProjectCache projectCache, + Reachable reachable, + RetryHelper retryHelper) { this.permissionBackend = permissionBackend; this.projectCache = projectCache; this.reachable = reachable; + this.retryHelper = retryHelper; } /** @@ -60,30 +69,56 @@ public class CreateRefControl { * * @param user the user performing the operation * @param repo repository on which user want to create - * @param branch the branch the new {@link RevObject} should be created on + * @param destBranch the branch the new {@link RevObject} should be created on * @param object the object the user will start the reference with + * @param sourceBranches the source ref from which the new ref is created from * @throws AuthException if creation is denied; the message explains the denial. * @throws PermissionBackendException on failure of permission checks. * @throws ResourceConflictException if the project state does not permit the operation */ public void checkCreateRef( - Provider<? extends CurrentUser> user, Repository repo, BranchNameKey branch, RevObject object) + Provider<? extends CurrentUser> user, + Repository repo, + BranchNameKey destBranch, + RevObject object, + boolean forPush, + BranchNameKey... sourceBranches) throws AuthException, PermissionBackendException, NoSuchProjectException, IOException, ResourceConflictException { ProjectState ps = - projectCache.get(branch.project()).orElseThrow(noSuchProject(branch.project())); + projectCache.get(destBranch.project()).orElseThrow(noSuchProject(destBranch.project())); ps.checkStatePermitsWrite(); - PermissionBackend.ForRef perm = permissionBackend.user(user.get()).ref(branch); + PermissionBackend.ForRef perm = permissionBackend.user(user.get()).ref(destBranch); if (object instanceof RevCommit) { perm.check(RefPermission.CREATE); - checkCreateCommit(user, repo, (RevCommit) object, ps.getNameKey(), perm); + if (sourceBranches.length == 0) { + checkCreateCommit(user, repo, (RevCommit) object, ps.getNameKey(), perm, forPush); + } else { + for (BranchNameKey src : sourceBranches) { + PermissionBackend.ForRef forRef = permissionBackend.user(user.get()).ref(src); + if (forRef.testOrFalse(RefPermission.READ)) { + return; + } + } + AuthException e = + new AuthException( + String.format( + "must have %s on existing ref to create new ref from it", + RefPermission.READ.describeForException())); + e.setAdvice( + String.format( + "use an existing ref visible to you, or get %s permission on the ref", + RefPermission.READ.describeForException())); + throw e; + } } else if (object instanceof RevTag) { RevTag tag = (RevTag) object; try (RevWalk rw = new RevWalk(repo)) { rw.parseBody(tag); } catch (IOException e) { - logger.atSevere().withCause(e).log("RevWalk(%s) parsing %s:", branch.project(), tag.name()); + logger.atSevere().withCause(e).log( + "RevWalk(%s) parsing %s:", destBranch.project(), tag.name()); throw e; } @@ -97,14 +132,14 @@ public class CreateRefControl { RevObject target = tag.getObject(); if (target instanceof RevCommit) { - checkCreateCommit(user, repo, (RevCommit) target, ps.getNameKey(), perm); + checkCreateCommit(user, repo, (RevCommit) target, ps.getNameKey(), perm, forPush); } else { - checkCreateRef(user, repo, branch, target); + checkCreateRef(user, repo, destBranch, target, forPush); } // If the tag has a PGP signature, allow a lower level of permission // than if it doesn't have a PGP signature. - PermissionBackend.ForRef forRef = permissionBackend.user(user.get()).ref(branch); + PermissionBackend.ForRef forRef = permissionBackend.user(user.get()).ref(destBranch); if (tag.getRawGpgSignature() != null) { forRef.check(RefPermission.CREATE_SIGNED_TAG); } else { @@ -122,13 +157,31 @@ public class CreateRefControl { Repository repo, RevCommit commit, Project.NameKey project, - PermissionBackend.ForRef forRef) + PermissionBackend.ForRef forRef, + boolean forPush) throws AuthException, PermissionBackendException, IOException { try { - // If the user has update (push) permission, they can create the ref regardless - // of whether they are pushing any new objects along with the create. - forRef.check(RefPermission.UPDATE); - return; + // If the user has UPDATE (push) permission, they can set the ref to an arbitrary commit: + // + // * if they don't have access, we don't advertise the data, and a conforming git client + // would send the object along with the push as outcome of the negotation. + // * a malicious client could try to send the update without sending the object. This + // is prevented by JGit's ConnectivityChecker (see receive.checkReferencedObjectsAreReachable + // to switch off this costly check). + // + // Thus, when using the git command-line client, we don't need to do extra checks for users + // with push access. + // + // When using the REST API, there is no negotiation, and the target commit must already be on + // the server, so we must check that the user can see that commit. + if (forPush) { + // We can only shortcut for UPDATE permission. Pushing a tag (CREATE_TAG, CREATE_SIGNED_TAG) + // can also introduce new objects. While there may not be a confidentiality problem + // (the caller supplies the data as documented above), the permission is for creating + // tags to existing commits. + forRef.check(RefPermission.UPDATE); + return; + } } catch (AuthException denied) { // Fall through to check reachability. } @@ -145,6 +198,18 @@ public class CreateRefControl { return; } + // Previous check only catches normal branches. Try PatchSet refs too. If we can create refs, + // we're not a replica, so we can always use the change index. + List<ChangeData> changes = + retryHelper + .changeIndexQuery( + "queryChangesByProjectCommitWithLimit1", + q -> q.enforceVisibility(true).setLimit(1).byProjectCommit(project, commit)) + .call(); + if (!changes.isEmpty()) { + return; + } + AuthException e = new AuthException( String.format( diff --git a/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java b/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java index 2e29bbd4f6..9893d1a778 100644 --- a/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java +++ b/java/com/google/gerrit/server/query/account/AccountQueryProcessor.java @@ -30,6 +30,7 @@ import com.google.gerrit.server.account.AccountState; import com.google.gerrit.server.index.account.AccountIndexCollection; import com.google.gerrit.server.index.account.AccountIndexRewriter; import com.google.gerrit.server.index.account.AccountSchemaDefinitions; +import com.google.gerrit.server.notedb.Sequences; import com.google.inject.Inject; import com.google.inject.Provider; @@ -41,6 +42,8 @@ import com.google.inject.Provider; */ public class AccountQueryProcessor extends QueryProcessor<AccountState> { private final AccountControl.Factory accountControlFactory; + private final Sequences sequences; + private final IndexConfig indexConfig; static { // It is assumed that basic rewrites do not touch visibleto predicates. @@ -57,7 +60,8 @@ public class AccountQueryProcessor extends QueryProcessor<AccountState> { IndexConfig indexConfig, AccountIndexCollection indexes, AccountIndexRewriter rewriter, - AccountControl.Factory accountControlFactory) { + AccountControl.Factory accountControlFactory, + Sequences sequences) { super( metricMaker, AccountSchemaDefinitions.INSTANCE, @@ -67,16 +71,28 @@ public class AccountQueryProcessor extends QueryProcessor<AccountState> { FIELD_LIMIT, () -> limitsFactory.create(userProvider.get()).getQueryLimit()); this.accountControlFactory = accountControlFactory; + this.sequences = sequences; + this.indexConfig = indexConfig; } @Override protected Predicate<AccountState> enforceVisibility(Predicate<AccountState> pred) { return new AndSource<>( - pred, new AccountIsVisibleToPredicate(accountControlFactory.get()), start); + pred, new AccountIsVisibleToPredicate(accountControlFactory.get()), start, indexConfig); } @Override protected String formatForLogging(AccountState accountState) { return accountState.account().id().toString(); } + + @Override + protected int getIndexSize() { + return sequences.lastAccountId(); + } + + @Override + protected int getBatchSize() { + return sequences.accountBatchSize(); + } } diff --git a/java/com/google/gerrit/server/query/change/AndChangeSource.java b/java/com/google/gerrit/server/query/change/AndChangeSource.java index 749204f3f1..98cada3d3a 100644 --- a/java/com/google/gerrit/server/query/change/AndChangeSource.java +++ b/java/com/google/gerrit/server/query/change/AndChangeSource.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.query.change; +import com.google.gerrit.index.IndexConfig; import com.google.gerrit.index.query.AndSource; import com.google.gerrit.index.query.IsVisibleToPredicate; import com.google.gerrit.index.query.Predicate; @@ -22,15 +23,16 @@ import java.util.List; public class AndChangeSource extends AndSource<ChangeData> implements ChangeDataSource { - public AndChangeSource(Collection<Predicate<ChangeData>> that) { - super(that); + public AndChangeSource(Collection<Predicate<ChangeData>> that, IndexConfig indexConfig) { + super(that, indexConfig); } public AndChangeSource( Predicate<ChangeData> that, IsVisibleToPredicate<ChangeData> isVisibleToPredicate, - int start) { - super(that, isVisibleToPredicate, start); + int start, + IndexConfig indexConfig) { + super(that, isVisibleToPredicate, start, indexConfig); } @Override diff --git a/java/com/google/gerrit/server/query/change/ChangeIndexPostFilterPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIndexPostFilterPredicate.java new file mode 100644 index 0000000000..d86d3668e3 --- /dev/null +++ b/java/com/google/gerrit/server/query/change/ChangeIndexPostFilterPredicate.java @@ -0,0 +1,31 @@ +// Copyright (C) 2022 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.server.query.change; + +import com.google.gerrit.index.FieldDef; + +/** + * Predicate that is mapped to a field in the change index, with additional filtering done in the + * {@code match} method. + */ +public abstract class ChangeIndexPostFilterPredicate extends ChangeIndexPredicate { + protected ChangeIndexPostFilterPredicate(FieldDef<ChangeData, ?> def, String value) { + super(def, value); + } + + protected ChangeIndexPostFilterPredicate(FieldDef<ChangeData, ?> def, String name, String value) { + super(def, name, value); + } +} diff --git a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java index 60587da27b..24042adbf1 100644 --- a/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java +++ b/java/com/google/gerrit/server/query/change/ChangeIsVisibleToPredicate.java @@ -41,9 +41,8 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData> } protected final CurrentUser user; - protected final PermissionBackend permissionBackend; protected final ProjectCache projectCache; - private final Provider<AnonymousUser> anonymousUserProvider; + private final PermissionBackend.WithUser withUser; @Inject public ChangeIsVisibleToPredicate( @@ -53,9 +52,14 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData> @Assisted CurrentUser user) { super(ChangeQueryBuilder.FIELD_VISIBLETO, IndexUtils.describe(user)); this.user = user; - this.permissionBackend = permissionBackend; this.projectCache = projectCache; - this.anonymousUserProvider = anonymousUserProvider; + withUser = + user.isIdentifiedUser() + ? permissionBackend.absentUser(user.getAccountId()) + : permissionBackend.user( + Optional.of(user) + .filter(u -> u instanceof GroupBackedUser || u instanceof InternalUser) + .orElseGet(anonymousUserProvider::get)); } @Override @@ -74,17 +78,10 @@ public class ChangeIsVisibleToPredicate extends IsVisibleToPredicate<ChangeData> return false; } if (!projectState.get().statePermitsRead()) { - logger.atFine().log("Filter out change %s of non-reabable project %s", cd, cd.project()); + logger.atFine().log("Filter out change %s of non-readable project %s", cd, cd.project()); return false; } - PermissionBackend.WithUser withUser = - user.isIdentifiedUser() - ? permissionBackend.absentUser(user.getAccountId()) - : permissionBackend.user( - Optional.of(user) - .filter(u -> u instanceof GroupBackedUser || u instanceof InternalUser) - .orElseGet(anonymousUserProvider::get)); try { if (!withUser.change(cd).test(ChangePermission.READ)) { logger.atFine().log("Filter out non-visisble change: %s", cd); diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java index ed1f2f124a..0f0535a67a 100644 --- a/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java +++ b/java/com/google/gerrit/server/query/change/ChangeQueryProcessor.java @@ -39,6 +39,7 @@ import com.google.gerrit.server.index.change.ChangeIndexCollection; import com.google.gerrit.server.index.change.ChangeIndexRewriter; import com.google.gerrit.server.index.change.ChangeSchemaDefinitions; import com.google.gerrit.server.index.change.IndexedChangeQuery; +import com.google.gerrit.server.notedb.Sequences; import com.google.inject.Inject; import com.google.inject.Provider; import java.util.ArrayList; @@ -61,6 +62,8 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData> private final Map<String, DynamicBean> dynamicBeans = new HashMap<>(); private final List<Extension<ChangePluginDefinedInfoFactory>> changePluginDefinedInfoFactoriesByPlugin = new ArrayList<>(); + private final Sequences sequences; + private final IndexConfig indexConfig; static { // It is assumed that basic rewrites do not touch visibleto predicates. @@ -77,6 +80,7 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData> IndexConfig indexConfig, ChangeIndexCollection indexes, ChangeIndexRewriter rewriter, + Sequences sequences, ChangeIsVisibleToPredicate.Factory changeIsVisibleToPredicateFactory, DynamicSet<ChangePluginDefinedInfoFactory> changePluginDefinedInfoFactories) { super( @@ -89,6 +93,8 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData> () -> limitsFactory.create(userProvider.get()).getQueryLimit()); this.userProvider = userProvider; this.changeIsVisibleToPredicateFactory = changeIsVisibleToPredicateFactory; + this.sequences = sequences; + this.indexConfig = indexConfig; changePluginDefinedInfoFactories .entries() @@ -103,8 +109,14 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData> @Override protected QueryOptions createOptions( - IndexConfig indexConfig, int start, int limit, Set<String> requestedFields) { - return IndexedChangeQuery.createOptions(indexConfig, start, limit, requestedFields); + IndexConfig indexConfig, + int start, + int pageSize, + int pageSizeMultiplier, + int limit, + Set<String> requestedFields) { + return IndexedChangeQuery.createOptions( + indexConfig, start, pageSize, pageSizeMultiplier, limit, requestedFields); } @Override @@ -131,11 +143,26 @@ public class ChangeQueryProcessor extends QueryProcessor<ChangeData> @Override protected Predicate<ChangeData> enforceVisibility(Predicate<ChangeData> pred) { return new AndChangeSource( - pred, changeIsVisibleToPredicateFactory.forUser(userProvider.get()), start); + pred, changeIsVisibleToPredicateFactory.forUser(userProvider.get()), start, indexConfig); } @Override protected String formatForLogging(ChangeData changeData) { return changeData.getId().toString(); } + + @Override + protected int getIndexSize() { + return sequences.lastChangeId(); + } + + @Override + protected int getBatchSize() { + return sequences.changeBatchSize(); + } + + @Override + protected int getInitialPageSize(int limit) { + return Math.min(getUserQueryLimit().getAsInt(), limit); + } } diff --git a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java index 12efecbb2d..4a0b649ada 100644 --- a/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java +++ b/java/com/google/gerrit/server/query/change/EqualsLabelPredicate.java @@ -30,7 +30,7 @@ import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; import java.util.Optional; -public class EqualsLabelPredicate extends ChangeIndexPredicate { +public class EqualsLabelPredicate extends ChangeIndexPostFilterPredicate { protected final ProjectCache projectCache; protected final PermissionBackend permissionBackend; protected final IdentifiedUser.GenericFactory userFactory; diff --git a/java/com/google/gerrit/server/query/change/OutputStreamQuery.java b/java/com/google/gerrit/server/query/change/OutputStreamQuery.java index 1b6dc623d8..716cf106d2 100644 --- a/java/com/google/gerrit/server/query/change/OutputStreamQuery.java +++ b/java/com/google/gerrit/server/query/change/OutputStreamQuery.java @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkState; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.Lists; import com.google.common.flogger.FluentLogger; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.LabelTypes; @@ -28,6 +29,8 @@ import com.google.gerrit.extensions.common.PluginDefinedInfo; import com.google.gerrit.index.query.QueryParseException; import com.google.gerrit.index.query.QueryResult; import com.google.gerrit.server.DynamicOptions; +import com.google.gerrit.server.account.AccountAttributeLoader; +import com.google.gerrit.server.cache.PerThreadCache; import com.google.gerrit.server.config.TrackingFooters; import com.google.gerrit.server.data.ChangeAttribute; import com.google.gerrit.server.data.PatchSetAttribute; @@ -86,6 +89,7 @@ public class OutputStreamQuery { private final EventFactory eventFactory; private final TrackingFooters trackingFooters; private final SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory; + private final AccountAttributeLoader.Factory accountAttributeLoaderFactory; private OutputFormat outputFormat = OutputFormat.TEXT; private boolean includePatchSets; @@ -110,13 +114,15 @@ public class OutputStreamQuery { ChangeQueryProcessor queryProcessor, EventFactory eventFactory, TrackingFooters trackingFooters, - SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory) { + SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory, + AccountAttributeLoader.Factory accountAttributeLoaderFactory) { this.repoManager = repoManager; this.queryBuilder = queryBuilder; this.queryProcessor = queryProcessor; this.eventFactory = eventFactory; this.trackingFooters = trackingFooters; this.submitRuleEvaluatorFactory = submitRuleEvaluatorFactory; + this.accountAttributeLoaderFactory = accountAttributeLoaderFactory; } void setLimit(int n) { @@ -205,7 +211,7 @@ public class OutputStreamQuery { return; } - try { + try (PerThreadCache ignored = PerThreadCache.create()) { final QueryStatsAttribute stats = new QueryStatsAttribute(); stats.runTimeMilliseconds = TimeUtil.nowMs(); @@ -214,9 +220,13 @@ public class OutputStreamQuery { QueryResult<ChangeData> results = queryProcessor.query(queryBuilder.parse(queryString)); pluginInfosByChange = queryProcessor.createPluginDefinedInfos(results.entities()); try { + AccountAttributeLoader accountLoader = accountAttributeLoaderFactory.create(); + List<ChangeAttribute> changeAttributes = new ArrayList<>(); for (ChangeData d : results.entities()) { - show(buildChangeAttribute(d, repos, revWalks)); + changeAttributes.add(buildChangeAttribute(d, repos, revWalks, accountLoader)); } + accountLoader.fill(); + changeAttributes.forEach(c -> show(c)); } finally { closeAll(revWalks.values(), repos.values()); } @@ -247,10 +257,14 @@ public class OutputStreamQuery { } private ChangeAttribute buildChangeAttribute( - ChangeData d, Map<Project.NameKey, Repository> repos, Map<Project.NameKey, RevWalk> revWalks) + ChangeData d, + Map<Project.NameKey, Repository> repos, + Map<Project.NameKey, RevWalk> revWalks, + AccountAttributeLoader accountLoader) throws IOException { LabelTypes labelTypes = d.getLabelTypes(); - ChangeAttribute c = eventFactory.asChangeAttribute(d.change(), d.notes()); + ChangeAttribute c = eventFactory.asChangeAttribute(d.change(), accountLoader); + c.hashtags = Lists.newArrayList(d.hashtags()); eventFactory.extend(c, d.change()); if (!trackingFooters.isEmpty()) { @@ -258,13 +272,14 @@ public class OutputStreamQuery { } if (includeAllReviewers) { - eventFactory.addAllReviewers(c, d.notes()); + eventFactory.addAllReviewers(c, d.notes(), accountLoader); } if (includeSubmitRecords) { SubmitRuleOptions options = SubmitRuleOptions.builder().recomputeOnClosedChanges(true).build(); - eventFactory.addSubmitRecords(c, submitRuleEvaluatorFactory.create(options).evaluate(d)); + eventFactory.addSubmitRecords( + c, submitRuleEvaluatorFactory.create(options).evaluate(d), accountLoader); } if (includeCommitMessage) { @@ -292,26 +307,28 @@ public class OutputStreamQuery { includeApprovals ? d.approvals().asMap() : null, includeFiles, d.change(), - labelTypes); + labelTypes, + accountLoader); } if (includeCurrentPatchSet) { PatchSet current = d.currentPatchSet(); if (current != null) { c.currentPatchSet = eventFactory.asPatchSetAttribute(rw, d.change(), current); - eventFactory.addApprovals(c.currentPatchSet, d.currentApprovals(), labelTypes); + eventFactory.addApprovals( + c.currentPatchSet, d.currentApprovals(), labelTypes, accountLoader); if (includeFiles) { eventFactory.addPatchSetFileNames(c.currentPatchSet, d.change(), d.currentPatchSet()); } if (includeComments) { - eventFactory.addPatchSetComments(c.currentPatchSet, d.publishedComments()); + eventFactory.addPatchSetComments(c.currentPatchSet, d.publishedComments(), accountLoader); } } } if (includeComments) { - eventFactory.addComments(c, d.messages()); + eventFactory.addComments(c, d.messages(), accountLoader); if (includePatchSets) { eventFactory.addPatchSets( rw, @@ -320,9 +337,10 @@ public class OutputStreamQuery { includeApprovals ? d.approvals().asMap() : null, includeFiles, d.change(), - labelTypes); + labelTypes, + accountLoader); for (PatchSetAttribute attribute : c.patchSets) { - eventFactory.addPatchSetComments(attribute, d.publishedComments()); + eventFactory.addPatchSetComments(attribute, d.publishedComments(), accountLoader); } } } diff --git a/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java b/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java index 9e56807d94..c6683faa87 100644 --- a/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java +++ b/java/com/google/gerrit/server/query/group/GroupQueryProcessor.java @@ -30,6 +30,7 @@ import com.google.gerrit.server.account.GroupControl; import com.google.gerrit.server.index.group.GroupIndexCollection; import com.google.gerrit.server.index.group.GroupIndexRewriter; import com.google.gerrit.server.index.group.GroupSchemaDefinitions; +import com.google.gerrit.server.notedb.Sequences; import com.google.inject.Inject; import com.google.inject.Provider; @@ -42,6 +43,8 @@ import com.google.inject.Provider; public class GroupQueryProcessor extends QueryProcessor<InternalGroup> { private final Provider<CurrentUser> userProvider; private final GroupControl.GenericFactory groupControlFactory; + private final Sequences sequences; + private final IndexConfig indexConfig; static { // It is assumed that basic rewrites do not touch visibleto predicates. @@ -58,7 +61,8 @@ public class GroupQueryProcessor extends QueryProcessor<InternalGroup> { IndexConfig indexConfig, GroupIndexCollection indexes, GroupIndexRewriter rewriter, - GroupControl.GenericFactory groupControlFactory) { + GroupControl.GenericFactory groupControlFactory, + Sequences sequences) { super( metricMaker, GroupSchemaDefinitions.INSTANCE, @@ -69,16 +73,31 @@ public class GroupQueryProcessor extends QueryProcessor<InternalGroup> { () -> limitsFactory.create(userProvider.get()).getQueryLimit()); this.userProvider = userProvider; this.groupControlFactory = groupControlFactory; + this.sequences = sequences; + this.indexConfig = indexConfig; } @Override protected Predicate<InternalGroup> enforceVisibility(Predicate<InternalGroup> pred) { return new AndSource<>( - pred, new GroupIsVisibleToPredicate(groupControlFactory, userProvider.get()), start); + pred, + new GroupIsVisibleToPredicate(groupControlFactory, userProvider.get()), + start, + indexConfig); } @Override protected String formatForLogging(InternalGroup internalGroup) { return internalGroup.getGroupUUID().get(); } + + @Override + protected int getIndexSize() { + return sequences.lastGroupId(); + } + + @Override + protected int getBatchSize() { + return sequences.groupBatchSize(); + } } diff --git a/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java b/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java index 8e6d8a185a..6dafa92289 100644 --- a/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java +++ b/java/com/google/gerrit/server/query/project/ProjectQueryProcessor.java @@ -30,6 +30,7 @@ import com.google.gerrit.metrics.MetricMaker; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.account.AccountLimits; import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.project.ProjectCache; import com.google.inject.Inject; import com.google.inject.Provider; @@ -44,6 +45,8 @@ import com.google.inject.Provider; public class ProjectQueryProcessor extends QueryProcessor<ProjectData> { private final PermissionBackend permissionBackend; private final Provider<CurrentUser> userProvider; + private final ProjectCache projectCache; + private final IndexConfig indexConfig; static { // It is assumed that basic rewrites do not touch visibleto predicates. @@ -60,7 +63,8 @@ public class ProjectQueryProcessor extends QueryProcessor<ProjectData> { IndexConfig indexConfig, ProjectIndexCollection indexes, ProjectIndexRewriter rewriter, - PermissionBackend permissionBackend) { + PermissionBackend permissionBackend, + ProjectCache projectCache) { super( metricMaker, ProjectSchemaDefinitions.INSTANCE, @@ -71,16 +75,31 @@ public class ProjectQueryProcessor extends QueryProcessor<ProjectData> { () -> limitsFactory.create(userProvider.get()).getQueryLimit()); this.permissionBackend = permissionBackend; this.userProvider = userProvider; + this.projectCache = projectCache; + this.indexConfig = indexConfig; } @Override protected Predicate<ProjectData> enforceVisibility(Predicate<ProjectData> pred) { return new AndSource<>( - pred, new ProjectIsVisibleToPredicate(permissionBackend, userProvider.get()), start); + pred, + new ProjectIsVisibleToPredicate(permissionBackend, userProvider.get()), + start, + indexConfig); } @Override protected String formatForLogging(ProjectData projectData) { return projectData.getProject().getName(); } + + @Override + protected int getIndexSize() { + return projectCache.all().size(); + } + + @Override + protected int getBatchSize() { + return 1; + } } diff --git a/java/com/google/gerrit/server/restapi/project/CreateBranch.java b/java/com/google/gerrit/server/restapi/project/CreateBranch.java index 2fd2d6530f..33549fe669 100644 --- a/java/com/google/gerrit/server/restapi/project/CreateBranch.java +++ b/java/com/google/gerrit/server/restapi/project/CreateBranch.java @@ -47,6 +47,7 @@ import com.google.inject.Singleton; import java.io.IOException; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.Ref; import org.eclipse.jgit.lib.RefUpdate; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.revwalk.RevObject; @@ -129,7 +130,21 @@ public class CreateBranch object = rw.parseCommit(object); } - createRefControl.checkCreateRef(identifiedUser, repo, name, object); + Ref sourceRef = repo.exactRef(input.revision); + if (sourceRef == null) { + createRefControl.checkCreateRef(identifiedUser, repo, name, object, /* forPush= */ false); + } else { + if (sourceRef.isSymbolic()) { + sourceRef = sourceRef.getTarget(); + } + createRefControl.checkCreateRef( + identifiedUser, + repo, + name, + object, + /* forPush= */ false, + BranchNameKey.create(rsrc.getNameKey(), sourceRef.getName())); + } RefUpdate u = repo.updateRef(ref); u.setExpectedOldObjectId(ObjectId.zeroId()); diff --git a/java/com/google/gerrit/server/update/BatchUpdate.java b/java/com/google/gerrit/server/update/BatchUpdate.java index 917e96721a..232aa98f5c 100644 --- a/java/com/google/gerrit/server/update/BatchUpdate.java +++ b/java/com/google/gerrit/server/update/BatchUpdate.java @@ -311,9 +311,14 @@ public class BatchUpdate implements AutoCloseable { @Override public ChangeUpdate getUpdate(PatchSet.Id psId) { + return getUpdate(psId, when); + } + + @Override + public ChangeUpdate getUpdate(PatchSet.Id psId, Timestamp whenOverride) { ChangeUpdate u = defaultUpdates.get(psId); if (u == null) { - u = getNewChangeUpdate(psId); + u = getNewChangeUpdate(psId, whenOverride); defaultUpdates.put(psId, u); } return u; @@ -321,13 +326,18 @@ public class BatchUpdate implements AutoCloseable { @Override public ChangeUpdate getDistinctUpdate(PatchSet.Id psId) { - ChangeUpdate u = getNewChangeUpdate(psId); + return getDistinctUpdate(psId, when); + } + + @Override + public ChangeUpdate getDistinctUpdate(PatchSet.Id psId, Timestamp whenOverride) { + ChangeUpdate u = getNewChangeUpdate(psId, whenOverride); distinctUpdates.put(psId, u); return u; } - private ChangeUpdate getNewChangeUpdate(PatchSet.Id psId) { - ChangeUpdate u = changeUpdateFactory.create(notes, user, when); + private ChangeUpdate getNewChangeUpdate(PatchSet.Id psId, Timestamp whenOverride) { + ChangeUpdate u = changeUpdateFactory.create(notes, user, whenOverride); if (newChanges.containsKey(notes.getChangeId())) { u.setAllowWriteToNewRef(true); } diff --git a/java/com/google/gerrit/server/update/ChangeContext.java b/java/com/google/gerrit/server/update/ChangeContext.java index aeabde4328..feba5414ce 100644 --- a/java/com/google/gerrit/server/update/ChangeContext.java +++ b/java/com/google/gerrit/server/update/ChangeContext.java @@ -20,6 +20,7 @@ import com.google.gerrit.entities.Change; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; +import java.sql.Timestamp; /** * Context for performing the {@link BatchUpdateOp#updateChange} phase. @@ -43,6 +44,15 @@ public interface ChangeContext extends Context { ChangeUpdate getUpdate(PatchSet.Id psId); /** + * Same as {@link ChangeContext#getUpdate}, but allows to override the commit timestamp. + * + * @param psId patch set ID. + * @param whenOverride commit timestamp. + * @return handle for change updates. + */ + ChangeUpdate getUpdate(PatchSet.Id psId, Timestamp whenOverride); + + /** * Gets a new ChangeUpdate for this change at a given patch set. * * <p>To get the current patch set ID, use {@link com.google.gerrit.server.PatchSetUtil#current}. @@ -53,6 +63,15 @@ public interface ChangeContext extends Context { ChangeUpdate getDistinctUpdate(PatchSet.Id psId); /** + * Same as {@link ChangeContext#getDistinctUpdate}, but allows to override the commit timestamp. + * + * @param psId patch set ID. + * @param whenOverride commit timestamp. + * @return handle for change updates. + */ + ChangeUpdate getDistinctUpdate(PatchSet.Id psId, Timestamp whenOverride); + + /** * Get the up-to-date notes for this change. * * <p>The change data is read within the same transaction that {@link diff --git a/java/com/google/gerrit/sshd/CommandModule.java b/java/com/google/gerrit/sshd/CommandModule.java index ac0705679e..4242c71780 100644 --- a/java/com/google/gerrit/sshd/CommandModule.java +++ b/java/com/google/gerrit/sshd/CommandModule.java @@ -15,6 +15,7 @@ package com.google.gerrit.sshd; import com.google.gerrit.lifecycle.LifecycleModule; +import com.google.gerrit.server.account.AccountAttributeLoader; import com.google.inject.binder.LinkedBindingBuilder; import org.apache.sshd.server.command.Command; @@ -29,6 +30,7 @@ public abstract class CommandModule extends LifecycleModule { * @return a binding that must be bound to a non-singleton provider for a {@link Command} object. */ protected LinkedBindingBuilder<Command> command(String name) { + factory(AccountAttributeLoader.Factory.class); return bind(Commands.key(name)); } diff --git a/java/com/google/gerrit/sshd/SshScope.java b/java/com/google/gerrit/sshd/SshScope.java index e9ed7504fe..b85cf6d8b1 100644 --- a/java/com/google/gerrit/sshd/SshScope.java +++ b/java/com/google/gerrit/sshd/SshScope.java @@ -53,6 +53,8 @@ public class SshScope { private volatile long startedMemory; private volatile long finishedMemory; + private IdentifiedUser identifiedUser; + private Context(SshSession s, String c, long at) { session = s; commandLine = c; @@ -125,8 +127,10 @@ public class SshScope { public CurrentUser getUser() { CurrentUser user = session.getUser(); if (user != null && user.isIdentifiedUser()) { - IdentifiedUser identifiedUser = userFactory.create(user.getAccountId()); - identifiedUser.setAccessPath(user.getAccessPath()); + if (identifiedUser == null) { + identifiedUser = userFactory.create(user.getAccountId()); + identifiedUser.setAccessPath(user.getAccessPath()); + } return identifiedUser; } return user; diff --git a/java/com/google/gerrit/sshd/commands/PatchSetParser.java b/java/com/google/gerrit/sshd/commands/PatchSetParser.java index 4ebf15e6a4..65d48dd09c 100644 --- a/java/com/google/gerrit/sshd/commands/PatchSetParser.java +++ b/java/com/google/gerrit/sshd/commands/PatchSetParser.java @@ -18,6 +18,7 @@ import com.google.gerrit.common.Nullable; import com.google.gerrit.entities.Change; import com.google.gerrit.entities.PatchSet; import com.google.gerrit.entities.Project; +import com.google.gerrit.entities.RefNames; import com.google.gerrit.git.ObjectIds; import com.google.gerrit.server.PatchSetUtil; import com.google.gerrit.server.change.ChangeFinder; @@ -59,6 +60,7 @@ public class PatchSetParser { if (token.matches("^([0-9a-fA-F]{4," + ObjectIds.STR_LEN + "})$")) { InternalChangeQuery query = queryProvider.get(); List<ChangeData> cds; + branch = branch != null ? RefNames.fullName(branch) : null; if (projectState != null) { Project.NameKey p = projectState.getNameKey(); if (branch != null) { diff --git a/java/com/google/gerrit/testing/BUILD b/java/com/google/gerrit/testing/BUILD index be32138dd9..a7a1795efd 100644 --- a/java/com/google/gerrit/testing/BUILD +++ b/java/com/google/gerrit/testing/BUILD @@ -34,6 +34,7 @@ java_library( "//java/com/google/gerrit/server/audit", "//java/com/google/gerrit/server/cache/h2", "//java/com/google/gerrit/server/cache/mem", + "//java/com/google/gerrit/server/group/testing", "//java/com/google/gerrit/server/logging", "//java/com/google/gerrit/server/restapi", "//java/com/google/gerrit/server/schema", diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java index 86ceb6056d..b00cadb7a7 100644 --- a/java/com/google/gerrit/testing/InMemoryModule.java +++ b/java/com/google/gerrit/testing/InMemoryModule.java @@ -25,6 +25,7 @@ import com.google.gerrit.acceptance.testsuite.project.ProjectOperationsImpl; import com.google.gerrit.auth.AuthModule; import com.google.gerrit.extensions.client.AuthType; import com.google.gerrit.extensions.config.FactoryModule; +import com.google.gerrit.extensions.registration.DynamicSet; import com.google.gerrit.extensions.systemstatus.ServerInformation; import com.google.gerrit.gpg.GpgModule; import com.google.gerrit.httpd.auth.restapi.OAuthRestModule; @@ -39,6 +40,7 @@ import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.GerritPersonIdentProvider; import com.google.gerrit.server.LibModuleType; import com.google.gerrit.server.PluginUser; +import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.api.GerritApiModule; import com.google.gerrit.server.api.PluginApiModule; import com.google.gerrit.server.audit.AuditModule; @@ -77,6 +79,7 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.PerThreadRequestScope; import com.google.gerrit.server.git.SearchingChangeCacheImpl.SearchingChangeCacheImplModule; import com.google.gerrit.server.git.WorkQueue; +import com.google.gerrit.server.group.testing.TestGroupBackend; import com.google.gerrit.server.index.account.AccountSchemaDefinitions; import com.google.gerrit.server.index.account.AllAccountsIndexer; import com.google.gerrit.server.index.change.AllChangesIndexer; @@ -267,6 +270,8 @@ public class InMemoryModule extends FactoryModule { install(new ConfigExperimentFeaturesModule()); bind(ProjectOperations.class).to(ProjectOperationsImpl.class); + bind(TestGroupBackend.class).in(SINGLETON); + DynamicSet.bind(binder(), GroupBackend.class).to(TestGroupBackend.class); } /** Copy of SchemaModule with a slightly different server ID provider. */ diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeReviewIT.java new file mode 100644 index 0000000000..2b04e56771 --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeReviewIT.java @@ -0,0 +1,49 @@ +// Copyright (C) 2022 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.acceptance.api.change; + +import com.google.gerrit.acceptance.AbstractDaemonTest; +import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.UseSsh; +import org.junit.Test; + +@UseSsh +public class ChangeReviewIT extends AbstractDaemonTest { + + @Test + public void testGerritReviewCommandWithShortNameBranch() throws Exception { + PushOneCommit.Result r = createChange(); + adminSshSession.exec( + "gerrit review --project " + + r.getChange().change().getProject().get() + + " --branch " + + r.getChange().change().getDest().shortName() + + " --code-review 1 " + + r.getCommit().getName()); + adminSshSession.assertSuccess(); + } + + @Test + public void testGerritReviewCommandWithoutProject() throws Exception { + PushOneCommit.Result r = createChange(); + adminSshSession.exec( + "gerrit review" + + " --branch " + + r.getChange().change().getDest().shortName() + + " --code-review 1 " + + r.getCommit().getName()); + adminSshSession.assertSuccess(); + } +} diff --git a/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java index 9d1bdaacbe..a1974b91e9 100644 --- a/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java +++ b/javatests/com/google/gerrit/acceptance/git/PushPermissionsIT.java @@ -47,6 +47,7 @@ import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.Repository; import org.eclipse.jgit.transport.PushResult; import org.eclipse.jgit.transport.RefSpec; +import org.eclipse.jgit.transport.RemoteRefUpdate; import org.eclipse.jgit.transport.RemoteRefUpdate.Status; import org.eclipse.jgit.transport.TrackingRefUpdate; import org.junit.Before; @@ -94,6 +95,32 @@ public class PushPermissionsIT extends AbstractDaemonTest { } @Test + public void pushNewCommitsRequiresPushPermission() throws Exception { + testRepo.branch("HEAD").commit().create(); + projectOperations + .project(project) + .forUpdate() + .add(allow(Permission.CREATE).ref("refs/*").group(REGISTERED_USERS)) + .update(); + + PushResult r = push("HEAD:refs/heads/newbranch"); + + String msg = "update for creating new commit object not permitted"; + RemoteRefUpdate rru = r.getRemoteUpdate("refs/heads/newbranch"); + assertThat(rru.getStatus()).isNotEqualTo(Status.OK); + assertThat(rru.getMessage()).contains(msg); + + projectOperations + .project(project) + .forUpdate() + .add(allow(Permission.PUSH).ref("refs/*").group(REGISTERED_USERS)) + .update(); + RemoteRefUpdate success = + push("HEAD:refs/heads/newbranch").getRemoteUpdate("refs/heads/newbranch"); + assertThat(success.getStatus()).isEqualTo(Status.OK); + } + + @Test public void fastForwardUpdateDenied() throws Exception { testRepo.branch("HEAD").commit().create(); PushResult r = push("HEAD:refs/heads/master"); diff --git a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java index 93ce255f62..33a7dc51cd 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java @@ -26,7 +26,10 @@ import static com.google.gerrit.testing.GerritJUnit.assertThrows; import com.google.common.collect.ImmutableList; import com.google.gerrit.acceptance.AbstractDaemonTest; import com.google.gerrit.acceptance.ExtensionRegistry; +import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.acceptance.RestResponse; +import com.google.gerrit.acceptance.TestAccount; +import com.google.gerrit.acceptance.testsuite.group.GroupOperations; import com.google.gerrit.acceptance.testsuite.project.ProjectOperations; import com.google.gerrit.acceptance.testsuite.request.RequestScopeOperations; import com.google.gerrit.entities.Account; @@ -61,6 +64,7 @@ import org.junit.Test; public class CreateBranchIT extends AbstractDaemonTest { @Inject private ProjectOperations projectOperations; @Inject private RequestScopeOperations requestScopeOperations; + @Inject private GroupOperations groupOperations; @Inject private ExtensionRegistry extensionRegistry; private BranchNameKey testBranch; @@ -410,6 +414,50 @@ public class CreateBranchIT extends AbstractDaemonTest { assertThat(ex).hasMessageThat().isEqualTo("ref must match URL"); } + @Test + public void createBranchRevisionVisibility() throws Exception { + AccountGroup.UUID privilegedGroupUuid = + groupOperations.newGroup().name(name("privilegedGroup")).create(); + TestAccount privilegedUser = + accountCreator.create( + "privilegedUser", "privilegedUser@example.com", "privilegedUser", null); + groupOperations.group(privilegedGroupUuid).forUpdate().addMember(privilegedUser.id()).update(); + projectOperations + .project(project) + .forUpdate() + .add(block(Permission.READ).ref("refs/heads/secret/*").group(REGISTERED_USERS)) + .add(allow(Permission.READ).ref("refs/heads/secret/*").group(privilegedGroupUuid)) + .add(allow(Permission.READ).ref("refs/heads/*").group(REGISTERED_USERS)) + .add(allow(Permission.CREATE).ref("refs/heads/*").group(REGISTERED_USERS)) + .add(allow(Permission.PUSH).ref("refs/heads/*").group(REGISTERED_USERS)) + .update(); + PushOneCommit push = + pushFactory.create(admin.newIdent(), testRepo, "Configure", "file.txt", "contents"); + PushOneCommit.Result result = push.to("refs/heads/secret/main"); + result.assertOkStatus(); + RevCommit secretCommit = result.getCommit(); + requestScopeOperations.setApiUser(privilegedUser.id()); + BranchInfo info = gApi.projects().name(project.get()).branch("refs/heads/secret/main").get(); + assertThat(info.revision).isEqualTo(secretCommit.name()); + TestAccount unprivileged = + accountCreator.create("unprivileged", "unprivileged@example.com", "unprivileged", null); + requestScopeOperations.setApiUser(unprivileged.id()); + assertThrows( + ResourceNotFoundException.class, + () -> gApi.projects().name(project.get()).branch("refs/heads/secret/main").get()); + BranchInput branchInput = new BranchInput(); + branchInput.ref = "public"; + branchInput.revision = secretCommit.name(); + assertThrows( + AuthException.class, + () -> gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput)); + + branchInput.revision = "refs/heads/secret/main"; + assertThrows( + AuthException.class, + () -> gApi.projects().name(project.get()).branch(branchInput.ref).create(branchInput)); + } + private void blockCreateReference() throws Exception { projectOperations .project(project) diff --git a/javatests/com/google/gerrit/index/query/AndSourceTest.java b/javatests/com/google/gerrit/index/query/AndSourceTest.java new file mode 100644 index 0000000000..3ae48fb756 --- /dev/null +++ b/javatests/com/google/gerrit/index/query/AndSourceTest.java @@ -0,0 +1,33 @@ +// Copyright (C) 2022 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.index.query; + +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import com.google.common.collect.Lists; +import org.junit.Test; + +public class AndSourceTest extends PredicateTest { + @Test + public void ensureLowerCostPredicateRunsFirst() { + TestMatchablePredicate p1 = new TestMatchablePredicate("predicate1", "foo", 10); + TestMatchablePredicate p2 = new TestMatchablePredicate("predicate2", "foo", 1); + AndSource<String> andSource = new AndSource<>(Lists.newArrayList(p1, p2), null); + andSource.match("bar"); + assertFalse(p1.ranMatch); + assertTrue(p2.ranMatch); + } +} diff --git a/javatests/com/google/gerrit/index/query/LazyDataSourceTest.java b/javatests/com/google/gerrit/index/query/LazyDataSourceTest.java index 7064f648d4..4105a1d7f5 100644 --- a/javatests/com/google/gerrit/index/query/LazyDataSourceTest.java +++ b/javatests/com/google/gerrit/index/query/LazyDataSourceTest.java @@ -17,6 +17,7 @@ package com.google.gerrit.index.query; import static com.google.gerrit.testing.GerritJUnit.assertThrows; import com.google.common.collect.ImmutableList; +import com.google.gerrit.index.IndexConfig; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.ChangeDataSource; import com.google.gerrit.server.query.change.OrSource; @@ -88,11 +89,17 @@ public class LazyDataSourceTest { public void close() { // No-op } + + @Override + public Object searchAfter() { + return null; + } } @Test public void andSourceIsLazy() { - AndSource<ChangeData> and = new AndSource<>(ImmutableList.of(new LazyPredicate())); + AndSource<ChangeData> and = + new AndSource<>(ImmutableList.of(new LazyPredicate()), IndexConfig.createDefault()); ResultSet<ChangeData> resultSet = and.read(); assertThrows(AssertionError.class, () -> resultSet.toList()); } diff --git a/javatests/com/google/gerrit/index/query/PredicateTest.java b/javatests/com/google/gerrit/index/query/PredicateTest.java index 3ec7f1362d..e10d7749bb 100644 --- a/javatests/com/google/gerrit/index/query/PredicateTest.java +++ b/javatests/com/google/gerrit/index/query/PredicateTest.java @@ -18,7 +18,29 @@ import org.junit.Ignore; @Ignore public abstract class PredicateTest { - protected static final class TestPredicate extends OperatorPredicate<String> { + protected static final class TestMatchablePredicate extends TestPredicate + implements Matchable<String> { + protected int cost; + protected boolean ranMatch = false; + + protected TestMatchablePredicate(String name, String value, int cost) { + super(name, value); + this.cost = cost; + } + + @Override + public boolean match(String object) { + ranMatch = true; + return false; + } + + @Override + public int getCost() { + return cost; + } + } + + protected static class TestPredicate extends OperatorPredicate<String> { protected TestPredicate(String name, String value) { super(name, value); } diff --git a/javatests/com/google/gerrit/integration/git/BUILD b/javatests/com/google/gerrit/integration/git/BUILD index 28755af59e..86b3f36542 100644 --- a/javatests/com/google/gerrit/integration/git/BUILD +++ b/javatests/com/google/gerrit/integration/git/BUILD @@ -1,13 +1,32 @@ load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests") +# TODO(davido): This was only needed as own rule, to provide a dedicated +# tag to skip Git version v2 protocol tests. That was particularly +# needed for RBE, because this test assumes that git client version is +# at least 2.17.1. Once Bazel docker image for Ubuntu 20.04 is available +# and we removed our own RBE docker image, we can merge this rule with +# the other rules in this package. acceptance_tests( srcs = ["GitProtocolV2IT.java"], group = "protocol-v2", labels = ["git-protocol-v2"], ) +# This rule can be also merged with the other tests in this package. acceptance_tests( srcs = ["UploadArchiveIT.java"], group = "upload-archive", labels = ["git-upload-archive"], ) + +acceptance_tests( + srcs = glob( + ["*.java"], + exclude = [ + "GitProtocolV2IT.java", + "UploadArchiveIT.java", + ], + ), + group = "git_tests", + labels = ["git"], +) diff --git a/javatests/com/google/gerrit/integration/git/PushToRefsUsersIT.java b/javatests/com/google/gerrit/integration/git/PushToRefsUsersIT.java new file mode 100644 index 0000000000..c42f00d738 --- /dev/null +++ b/javatests/com/google/gerrit/integration/git/PushToRefsUsersIT.java @@ -0,0 +1,111 @@ +// Copyright (C) 2022 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.integration.git; + +import static com.google.common.truth.Truth.assertThat; +import static java.nio.charset.StandardCharsets.UTF_8; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.gerrit.acceptance.StandaloneSiteTest; +import com.google.gerrit.extensions.api.GerritApi; +import com.google.gerrit.server.config.AllUsersName; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.inject.Inject; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import org.eclipse.jgit.lib.Config; +import org.junit.Test; + +// TODO(davido): In addition to push over HTTP also add a test for push over SSH +public class PushToRefsUsersIT extends StandaloneSiteTest { + private static final String ADMIN_PASSWORD = "secret"; + private final String[] GIT_CLONE = new String[] {"git", "clone"}; + private final String[] GIT_FETCH_USERS_SELF = + new String[] {"git", "fetch", "origin", "refs/users/self"}; + private final String[] GIT_CO_FETCH_HEAD = new String[] {"git", "checkout", "FETCH_HEAD"}; + private final String[] GIT_CONFIG_USER_EMAIL = + new String[] {"git", "config", "user.email", "admin@example.com"}; + private final String[] GIT_CONFIG_USER_NAME = + new String[] {"git", "config", "user.name", "Administrator"}; + private final String[] GIT_COMMIT = new String[] {"git", "commit", "-am", "OOO"}; + private final String[] GIT_PUSH_USERS_SELF = + new String[] {"git", "push", "origin", "HEAD:refs/users/self"}; + + @Inject private GerritApi gApi; + @Inject private @GerritServerConfig Config config; + @Inject private AllUsersName allUsersName; + + @Test + public void testPushToRefsUsersOverHttp() throws Exception { + try (ServerContext ctx = startServer()) { + ctx.getInjector().injectMembers(this); + + // Setup admin password + gApi.accounts().id(admin.username()).setHttpPassword(ADMIN_PASSWORD); + + // Get authenticated Git/HTTP URL + String urlWithCredentials = + config + .getString("gerrit", null, "canonicalweburl") + .replace("http://", "http://" + admin.username() + ":" + ADMIN_PASSWORD + "@"); + + // Clone All-Users repository + execute( + ImmutableList.<String>builder() + .add(GIT_CLONE) + .add(urlWithCredentials + "/a/" + allUsersName) + .add(sitePaths.data_dir.toFile().getAbsolutePath()) + .build(), + sitePaths.site_path); + + // Fetch refs/users/self for admin user + execute(GIT_FETCH_USERS_SELF); + + // Checkout FETCH_HEAD + execute(GIT_CO_FETCH_HEAD); + + // Set admin user status to OOO + Files.write( + sitePaths.data_dir.resolve("account.config"), + " status = OOO".getBytes(UTF_8), + StandardOpenOption.APPEND); + + // Set user email + execute(GIT_CONFIG_USER_EMAIL); + + // Set user name + execute(GIT_CONFIG_USER_NAME); + + // Commit + execute(GIT_COMMIT); + + // Push + assertThat(execute(GIT_PUSH_USERS_SELF)).contains("Processing changes: refs: 1, done"); + + // Verify user status + assertThat(gApi.accounts().id(admin.id().get()).detail().status).isEqualTo("OOO"); + } + } + + private String execute(String... cmds) throws Exception { + return execute(ImmutableList.<String>builder().add(cmds).build(), sitePaths.data_dir); + } + + private String execute(ImmutableList<String> cmd, Path path) throws Exception { + return execute(cmd, path.toFile(), ImmutableMap.of()); + } +} diff --git a/javatests/com/google/gerrit/metrics/dropwizard/BUILD b/javatests/com/google/gerrit/metrics/dropwizard/BUILD index 98d12b2e7b..e236f30ce5 100644 --- a/javatests/com/google/gerrit/metrics/dropwizard/BUILD +++ b/javatests/com/google/gerrit/metrics/dropwizard/BUILD @@ -6,7 +6,10 @@ junit_tests( tags = ["metrics"], visibility = ["//visibility:public"], deps = [ + "//java/com/google/gerrit/metrics", "//java/com/google/gerrit/metrics/dropwizard", + "//lib/mockito", "//lib/truth", + "@dropwizard-core//jar", ], ) diff --git a/javatests/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java b/javatests/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java index 9b21bf6c01..5777779513 100644 --- a/javatests/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java +++ b/javatests/com/google/gerrit/metrics/dropwizard/DropWizardMetricMakerTest.java @@ -15,12 +15,33 @@ package com.google.gerrit.metrics.dropwizard; import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; +import com.codahale.metrics.MetricRegistry; +import com.google.gerrit.metrics.Description; +import com.google.gerrit.metrics.MetricsReservoirConfig; +import com.google.gerrit.metrics.ReservoirType; +import org.junit.Before; import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; +@RunWith(MockitoJUnitRunner.class) public class DropWizardMetricMakerTest { - DropWizardMetricMaker metrics = - new DropWizardMetricMaker(null /* MetricRegistry unused in tests */); + + @Mock MetricsReservoirConfig reservoirConfigMock; + + MetricRegistry registry; + + DropWizardMetricMaker metrics; + + @Before + public void setupMocks() { + registry = new MetricRegistry(); + metrics = new DropWizardMetricMaker(registry, reservoirConfigMock); + } @Test public void shouldSanitizeUnwantedChars() throws Exception { @@ -41,4 +62,15 @@ public class DropWizardMetricMakerTest { assertThat(metrics.sanitizeMetricName("metric//")).isEqualTo("metric"); assertThat(metrics.sanitizeMetricName("metric/submetric/")).isEqualTo("metric/submetric"); } + + @Test + public void shouldRequestForReservoirForNewTimer() throws Exception { + when(reservoirConfigMock.reservoirType()).thenReturn(ReservoirType.ExponentiallyDecaying); + + metrics.newTimer( + "foo", + new Description("foo description").setCumulative().setUnit(Description.Units.MILLISECONDS)); + + verify(reservoirConfigMock).reservoirType(); + } } diff --git a/javatests/com/google/gerrit/metrics/dropwizard/DropWizardReservoirProviderTest.java b/javatests/com/google/gerrit/metrics/dropwizard/DropWizardReservoirProviderTest.java new file mode 100644 index 0000000000..6402b53ad4 --- /dev/null +++ b/javatests/com/google/gerrit/metrics/dropwizard/DropWizardReservoirProviderTest.java @@ -0,0 +1,64 @@ +// Copyright (C) 2022 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 org.mockito.Mockito.when; + +import com.codahale.metrics.ExponentiallyDecayingReservoir; +import com.codahale.metrics.SlidingTimeWindowArrayReservoir; +import com.codahale.metrics.SlidingTimeWindowReservoir; +import com.codahale.metrics.SlidingWindowReservoir; +import com.codahale.metrics.UniformReservoir; +import com.google.gerrit.metrics.MetricsReservoirConfig; +import com.google.gerrit.metrics.ReservoirType; +import java.time.Duration; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnitRunner; + +@RunWith(MockitoJUnitRunner.class) +public class DropWizardReservoirProviderTest { + private static final int SLIDING_WINDOW_INTERVAL = 1; + private static final int SLIDING_WINDOW_SIZE = 256; + + @Mock private MetricsReservoirConfig configMock; + + @Test + public void shouldInstantiateReservoirProviderBasedOnMetricsConfig() { + when(configMock.reservoirType()).thenReturn(ReservoirType.ExponentiallyDecaying); + assertThat(DropWizardReservoirProvider.get(configMock)) + .isInstanceOf(ExponentiallyDecayingReservoir.class); + + when(configMock.reservoirType()).thenReturn(ReservoirType.SlidingTimeWindow); + when(configMock.reservoirWindow()).thenReturn(Duration.ofMinutes(1)); + assertThat(DropWizardReservoirProvider.get(configMock)) + .isInstanceOf(SlidingTimeWindowReservoir.class); + + when(configMock.reservoirType()).thenReturn(ReservoirType.SlidingTimeWindowArray); + when(configMock.reservoirWindow()).thenReturn(Duration.ofMinutes(SLIDING_WINDOW_INTERVAL)); + assertThat(DropWizardReservoirProvider.get(configMock)) + .isInstanceOf(SlidingTimeWindowArrayReservoir.class); + + when(configMock.reservoirType()).thenReturn(ReservoirType.SlidingWindow); + when(configMock.reservoirSize()).thenReturn(SLIDING_WINDOW_SIZE); + assertThat(DropWizardReservoirProvider.get(configMock)) + .isInstanceOf(SlidingWindowReservoir.class); + + when(configMock.reservoirType()).thenReturn(ReservoirType.Uniform); + assertThat(DropWizardReservoirProvider.get(configMock)).isInstanceOf(UniformReservoir.class); + } +} diff --git a/javatests/com/google/gerrit/metrics/proc/ProcMetricModuleTest.java b/javatests/com/google/gerrit/metrics/proc/ProcMetricModuleTest.java index 33919e7ad0..ea89ae9d92 100644 --- a/javatests/com/google/gerrit/metrics/proc/ProcMetricModuleTest.java +++ b/javatests/com/google/gerrit/metrics/proc/ProcMetricModuleTest.java @@ -17,6 +17,7 @@ package com.google.gerrit.metrics.proc; import static com.google.common.truth.Truth.assertThat; import static com.google.common.truth.Truth.assertWithMessage; import static com.google.gerrit.testing.GerritJUnit.assertThrows; +import static org.mockito.Mockito.mock; import com.codahale.metrics.Counter; import com.codahale.metrics.Gauge; @@ -31,7 +32,9 @@ import com.google.gerrit.metrics.Description; import com.google.gerrit.metrics.Description.FieldOrdering; import com.google.gerrit.metrics.Field; import com.google.gerrit.metrics.MetricMaker; +import com.google.gerrit.metrics.MetricsReservoirConfig; import com.google.gerrit.metrics.dropwizard.DropWizardMetricMaker; +import com.google.inject.AbstractModule; import com.google.inject.Guice; import com.google.inject.Inject; import com.google.inject.Injector; @@ -179,7 +182,14 @@ public class ProcMetricModuleTest { @Before public void setup() { - Injector injector = Guice.createInjector(new DropWizardMetricMaker.ApiModule()); + Injector injector = + Guice.createInjector( + new AbstractModule() { + @Override + protected void configure() { + install(new DropWizardMetricMaker.ApiModule(mock(MetricsReservoirConfig.class))); + } + }); LifecycleManager mgr = new LifecycleManager(); mgr.add(injector); diff --git a/javatests/com/google/gerrit/server/events/EventTypesTest.java b/javatests/com/google/gerrit/server/events/EventTypesTest.java index c822d6c68c..7e97f18499 100644 --- a/javatests/com/google/gerrit/server/events/EventTypesTest.java +++ b/javatests/com/google/gerrit/server/events/EventTypesTest.java @@ -48,4 +48,16 @@ public class EventTypesTest { Class<?> clazz = EventTypes.getClass("does-not-exist-event"); assertThat(clazz).isNull(); } + + @Test + public void getRegisteredEventsGetsANewlyRegisteredEvent() { + EventTypes.register(TestEvent.TYPE, TestEvent.class); + assertThat(EventTypes.getRegisteredEvents()).containsEntry(TestEvent.TYPE, TestEvent.class); + } + + @Test + public void getRegisteredEventsGetsTypeGivenAtRegistration() { + EventTypes.register("alternate-type", TestEvent.class); + assertThat(EventTypes.getRegisteredEvents()).containsEntry("alternate-type", TestEvent.class); + } } diff --git a/javatests/com/google/gerrit/server/git/delegate/DelegateRepositoryTest.java b/javatests/com/google/gerrit/server/git/delegate/DelegateRepositoryTest.java new file mode 100644 index 0000000000..74e0facf2b --- /dev/null +++ b/javatests/com/google/gerrit/server/git/delegate/DelegateRepositoryTest.java @@ -0,0 +1,41 @@ +// Copyright (C) 2022 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.server.git.delegate; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.gerrit.entities.Project; +import com.google.gerrit.server.git.DelegateRepository; +import com.google.gerrit.testing.InMemoryRepositoryManager; +import java.io.IOException; +import org.eclipse.jgit.lib.Repository; +import org.junit.Test; + +public class DelegateRepositoryTest { + + @Test + public void shouldDelegateRepositoryFromAnyPackage() throws IOException { + Repository foo = new InMemoryRepositoryManager().createRepository(Project.nameKey("foo")); + try (TestDelegateRepository delegateRepository = new TestDelegateRepository(foo)) { + assertThat(delegateRepository.delegate()).isSameInstanceAs(foo); + } + } + + static class TestDelegateRepository extends DelegateRepository { + protected TestDelegateRepository(Repository delegate) { + super(delegate); + } + } +} diff --git a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java index e5b2ffb548..4894eb3a2b 100644 --- a/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java +++ b/javatests/com/google/gerrit/server/index/change/ChangeIndexRewriterTest.java @@ -97,7 +97,7 @@ public class ChangeIndexRewriterTest { Predicate<ChangeData> in = parse("foo:a file:b"); Predicate<ChangeData> out = rewrite(in); assertThat(AndChangeSource.class).isSameInstanceAs(out.getClass()); - assertThat(out.getChildren()).containsExactly(query(in.getChild(1)), in.getChild(0)).inOrder(); + assertThat(out.getChildren()).containsExactly(query(parse("file:b")), parse("foo:a")).inOrder(); } @Test @@ -126,9 +126,9 @@ public class ChangeIndexRewriterTest { .inOrder(); // Same at the assertions above, that were added for readability - assertThat(out.getChild(0)).isEqualTo(query(in.getChild(0))); + assertThat(out.getChild(0)).isEqualTo(query(parse("-status:abandoned"))); assertThat(indexedSubTree.getChildren()) - .containsExactly(query(in.getChild(1).getChild(1)), in.getChild(1).getChild(0)) + .containsExactly(query(parse("file:b")), parse("foo:a")) .inOrder(); } @@ -137,7 +137,9 @@ public class ChangeIndexRewriterTest { Predicate<ChangeData> in = parse("-foo:a (file:b OR file:c)"); Predicate<ChangeData> out = rewrite(in); assertThat(out.getClass()).isSameInstanceAs(AndChangeSource.class); - assertThat(out.getChildren()).containsExactly(query(in.getChild(1)), in.getChild(0)).inOrder(); + assertThat(out.getChildren()) + .containsExactly(query(parse("file:b OR file:c")), parse("-foo:a")) + .inOrder(); } @Test @@ -146,7 +148,8 @@ public class ChangeIndexRewriterTest { Predicate<ChangeData> out = rewrite(in); assertThat(out.getClass()).isSameInstanceAs(OrSource.class); assertThat(out.getChildren()) - .containsExactly(query(or(in.getChild(0), in.getChild(2))), in.getChild(1), in.getChild(3)) + .containsExactly( + query(or(parse("file:a"), parse("file:c"))), parse("foo:b"), parse("foo:d")) .inOrder(); } @@ -156,7 +159,7 @@ public class ChangeIndexRewriterTest { Predicate<ChangeData> out = rewrite(in); assertThat(AndChangeSource.class).isSameInstanceAs(out.getClass()); assertThat(out.getChildren()) - .containsExactly(query(and(in.getChild(0), in.getChild(2))), in.getChild(1)) + .containsExactly(query(and(parse("status:new"), parse("file:a"))), parse("bar:p")) .inOrder(); } @@ -166,7 +169,7 @@ public class ChangeIndexRewriterTest { Predicate<ChangeData> out = rewrite(in); assertThat(out.getClass()).isEqualTo(AndChangeSource.class); assertThat(out.getChildren()) - .containsExactly(query(and(in.getChild(0), in.getChild(2))), in.getChild(1)) + .containsExactly(query(and(parse("status:new"), parse("file:a"))), parse("bar:p")) .inOrder(); } @@ -176,7 +179,7 @@ public class ChangeIndexRewriterTest { Predicate<ChangeData> out = rewrite(in); assertThat(out.getClass()).isEqualTo(AndChangeSource.class); assertThat(out.getChildren()) - .containsExactly(query(and(in.getChild(0), in.getChild(2))), in.getChild(1)) + .containsExactly(query(and(parse("status:new OR file:a"), parse("file:b"))), parse("bar:p")) .inOrder(); } @@ -186,7 +189,7 @@ public class ChangeIndexRewriterTest { Predicate<ChangeData> out = rewrite(in, options(0, 5)); assertThat(out.getClass()).isEqualTo(AndChangeSource.class); assertThat(out.getChildren()) - .containsExactly(query(in.getChild(1), 5), parse("limit:5"), parse("limit:5")) + .containsExactly(query(parse("file:a"), 5), parse("limit:5"), parse("limit:5")) .inOrder(); } @@ -259,7 +262,7 @@ public class ChangeIndexRewriterTest { @SafeVarargs private static AndChangeSource andSource(Predicate<ChangeData>... preds) { - return new AndChangeSource(Arrays.asList(preds)); + return new AndChangeSource(Arrays.asList(preds), IndexConfig.createDefault()); } private Predicate<ChangeData> rewrite(Predicate<ChangeData> in) throws QueryParseException { diff --git a/javatests/com/google/gerrit/server/notedb/RepoSequenceTest.java b/javatests/com/google/gerrit/server/notedb/RepoSequenceTest.java index a768eaf02d..0c9f731437 100644 --- a/javatests/com/google/gerrit/server/notedb/RepoSequenceTest.java +++ b/javatests/com/google/gerrit/server/notedb/RepoSequenceTest.java @@ -91,27 +91,37 @@ public class RepoSequenceTest { assertThat(s.acquireCount).isEqualTo(0); assertThat(s.next()).isEqualTo(1); + assertThat(s.last()).isEqualTo(1); assertThat(s.acquireCount).isEqualTo(1); assertThat(s.next()).isEqualTo(2); + assertThat(s.last()).isEqualTo(2); assertThat(s.acquireCount).isEqualTo(1); assertThat(s.next()).isEqualTo(3); + assertThat(s.last()).isEqualTo(3); assertThat(s.acquireCount).isEqualTo(1); assertThat(s.next()).isEqualTo(4); + assertThat(s.last()).isEqualTo(4); assertThat(s.acquireCount).isEqualTo(2); assertThat(s.next()).isEqualTo(5); + assertThat(s.last()).isEqualTo(5); assertThat(s.acquireCount).isEqualTo(2); assertThat(s.next()).isEqualTo(6); + assertThat(s.last()).isEqualTo(6); assertThat(s.acquireCount).isEqualTo(2); assertThat(s.next()).isEqualTo(7); + assertThat(s.last()).isEqualTo(7); assertThat(s.acquireCount).isEqualTo(3); assertThat(s.next()).isEqualTo(8); + assertThat(s.last()).isEqualTo(8); assertThat(s.acquireCount).isEqualTo(3); assertThat(s.next()).isEqualTo(9); + assertThat(s.last()).isEqualTo(9); assertThat(s.acquireCount).isEqualTo(3); assertThat(s.next()).isEqualTo(10); + assertThat(s.last()).isEqualTo(10); assertThat(s.acquireCount).isEqualTo(4); } @@ -127,6 +137,8 @@ public class RepoSequenceTest { assertThat(s2.next()).isEqualTo(5); assertThat(s1.next()).isEqualTo(3); assertThat(s2.next()).isEqualTo(6); + assertThat(s1.last()).isEqualTo(3); + assertThat(s2.last()).isEqualTo(6); // s2 acquires 7-9; s1 acquires 10-12. assertThat(s2.next()).isEqualTo(7); @@ -135,6 +147,8 @@ public class RepoSequenceTest { assertThat(s1.next()).isEqualTo(11); assertThat(s2.next()).isEqualTo(9); assertThat(s1.next()).isEqualTo(12); + assertThat(s1.last()).isEqualTo(12); + assertThat(s2.last()).isEqualTo(9); } @Test @@ -284,48 +298,61 @@ public class RepoSequenceTest { } @Test - public void nextWithCountOneCaller() throws Exception { + public void nextWithCountAndLastByOneCaller() throws Exception { RepoSequence s = newSequence("id", 1, 3); assertThat(s.next(2)).containsExactly(1, 2).inOrder(); assertThat(s.acquireCount).isEqualTo(1); + assertThat(s.last()).isEqualTo(2); assertThat(s.next(2)).containsExactly(3, 4).inOrder(); assertThat(s.acquireCount).isEqualTo(2); + assertThat(s.last()).isEqualTo(4); assertThat(s.next(2)).containsExactly(5, 6).inOrder(); assertThat(s.acquireCount).isEqualTo(2); + assertThat(s.last()).isEqualTo(6); assertThat(s.next(3)).containsExactly(7, 8, 9).inOrder(); assertThat(s.acquireCount).isEqualTo(3); + assertThat(s.last()).isEqualTo(9); assertThat(s.next(3)).containsExactly(10, 11, 12).inOrder(); assertThat(s.acquireCount).isEqualTo(4); + assertThat(s.last()).isEqualTo(12); assertThat(s.next(3)).containsExactly(13, 14, 15).inOrder(); assertThat(s.acquireCount).isEqualTo(5); + assertThat(s.last()).isEqualTo(15); assertThat(s.next(7)).containsExactly(16, 17, 18, 19, 20, 21, 22).inOrder(); assertThat(s.acquireCount).isEqualTo(6); + assertThat(s.last()).isEqualTo(22); assertThat(s.next(7)).containsExactly(23, 24, 25, 26, 27, 28, 29).inOrder(); assertThat(s.acquireCount).isEqualTo(7); + assertThat(s.last()).isEqualTo(29); assertThat(s.next(7)).containsExactly(30, 31, 32, 33, 34, 35, 36).inOrder(); assertThat(s.acquireCount).isEqualTo(8); + assertThat(s.last()).isEqualTo(36); } @Test - public void nextWithCountMultipleCallers() throws Exception { + public void nextWithCountAndLastByMultipleCallers() throws Exception { RepoSequence s1 = newSequence("id", 1, 3); RepoSequence s2 = newSequence("id", 1, 4); assertThat(s1.next(2)).containsExactly(1, 2).inOrder(); + assertThat(s1.last()).isEqualTo(2); assertThat(s1.acquireCount).isEqualTo(1); // s1 hasn't exhausted its last batch. assertThat(s2.next(2)).containsExactly(4, 5).inOrder(); + assertThat(s2.last()).isEqualTo(5); assertThat(s2.acquireCount).isEqualTo(1); // s1 acquires again to cover this request, plus a whole new batch. assertThat(s1.next(3)).containsExactly(3, 8, 9); + assertThat(s1.last()).isEqualTo(9); assertThat(s1.acquireCount).isEqualTo(2); // s2 hasn't exhausted its last batch, do so now. assertThat(s2.next(2)).containsExactly(6, 7); + assertThat(s2.last()).isEqualTo(7); assertThat(s2.acquireCount).isEqualTo(1); } diff --git a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java index 16f71990bb..e0a69a0822 100644 --- a/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java +++ b/javatests/com/google/gerrit/server/query/account/AbstractQueryAccountsTest.java @@ -646,7 +646,10 @@ public abstract class AbstractQueryAccountsTest extends GerritServerTests { .getRaw( Account.id(userInfo._accountId), QueryOptions.create( - IndexConfig.createDefault(), 0, 1, schema.getStoredFields().keySet())); + IndexConfig.fromConfig(config).build(), + 0, + 1, + schema.getStoredFields().keySet())); assertThat(rawFields).isPresent(); if (schema.useLegacyNumericFields()) { diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java index 9acce8f605..c3b220b0e4 100644 --- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java @@ -104,6 +104,7 @@ import com.google.gerrit.server.account.AccountManager; import com.google.gerrit.server.account.Accounts; import com.google.gerrit.server.account.AccountsUpdate; import com.google.gerrit.server.account.AuthRequest; +import com.google.gerrit.server.account.ListGroupMembership; import com.google.gerrit.server.account.VersionedAccountQueries; import com.google.gerrit.server.account.externalids.ExternalIdFactory; import com.google.gerrit.server.change.ChangeInserter; @@ -112,6 +113,7 @@ import com.google.gerrit.server.change.NotifyResolver; import com.google.gerrit.server.change.PatchSetInserter; import com.google.gerrit.server.config.AllUsersName; import com.google.gerrit.server.git.meta.MetaDataUpdate; +import com.google.gerrit.server.group.testing.TestGroupBackend; import com.google.gerrit.server.index.change.ChangeField; import com.google.gerrit.server.index.change.ChangeIndexCollection; import com.google.gerrit.server.index.change.ChangeIndexer; @@ -189,14 +191,15 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { @Inject protected SchemaCreator schemaCreator; @Inject protected Sequences seq; @Inject protected ThreadLocalRequestContext requestContext; + @Inject protected TestGroupBackend testGroupBackend; @Inject protected ProjectCache projectCache; @Inject protected MetaDataUpdate.Server metaDataUpdateFactory; @Inject protected IdentifiedUser.GenericFactory identifiedUserFactory; @Inject protected AuthRequest.Factory authRequestFactory; @Inject protected ExternalIdFactory externalIdFactory; + @Inject protected ProjectOperations projectOperations; @Inject private ProjectConfig.Factory projectConfigFactory; - @Inject private ProjectOperations projectOperations; protected Injector injector; protected LifecycleManager lifecycle; @@ -715,7 +718,7 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { assertThat(thrown).hasMessageThat().contains("invalid value"); } - private Change createChange(TestRepository<Repo> repo, PersonIdent person) throws Exception { + protected Change createChange(TestRepository<Repo> repo, PersonIdent person) throws Exception { RevCommit commit = repo.parseBody(repo.commit().message("message").author(person).committer(person).create()); return insert(repo, newChangeForCommit(repo, commit), null); @@ -1279,6 +1282,44 @@ public abstract class AbstractQueryChangesTest extends GerritServerTests { } @Test + public void byLabelExternalGroup() throws Exception { + Account.Id user1 = createAccount("user1"); + Account.Id user2 = createAccount("user2"); + TestRepository<InMemoryRepositoryManager.Repo> repo = createProject("repo"); + + // create group and add users + AccountGroup.UUID external_group1 = AccountGroup.uuid("testbackend:group1"); + AccountGroup.UUID external_group2 = AccountGroup.uuid("testbackend:group2"); + testGroupBackend.create(external_group1); + testGroupBackend.create(external_group2); + testGroupBackend.setMembershipsOf( + user1, new ListGroupMembership(ImmutableList.of(external_group1))); + testGroupBackend.setMembershipsOf( + user2, new ListGroupMembership(ImmutableList.of(external_group2))); + + Change change1 = insert(repo, newChange(repo), user1); + Change change2 = insert(repo, newChange(repo), user1); + + // post a review with user1 and other_user + requestContext.setContext(newRequestContext(user1)); + gApi.changes() + .id(change1.getId().get()) + .current() + .review(new ReviewInput().label("Code-Review", 1)); + requestContext.setContext(newRequestContext(userId)); + gApi.changes() + .id(change2.getId().get()) + .current() + .review(new ReviewInput().label("Code-Review", 1)); + + assertQuery("label:Code-Review=+1," + external_group1.get(), change1); + assertQuery("label:Code-Review=+1,group=" + external_group1.get(), change1); + assertQuery("label:Code-Review=+1,user=user1", change1); + assertQuery("label:Code-Review=+1,user=user2"); + assertQuery("label:Code-Review=+1,group=" + external_group2.get()); + } + + @Test public void limit() throws Exception { TestRepository<Repo> repo = createProject("repo"); Change last = null; diff --git a/javatests/com/google/gerrit/server/query/change/BUILD b/javatests/com/google/gerrit/server/query/change/BUILD index 08456d1d17..802bf5480f 100644 --- a/javatests/com/google/gerrit/server/query/change/BUILD +++ b/javatests/com/google/gerrit/server/query/change/BUILD @@ -21,6 +21,7 @@ java_library( "//java/com/google/gerrit/acceptance/config", "//java/com/google/gerrit/acceptance/testsuite/project", "//java/com/google/gerrit/common:annotations", + "//java/com/google/gerrit/common:server", "//java/com/google/gerrit/entities", "//java/com/google/gerrit/extensions:api", "//java/com/google/gerrit/httpd", @@ -29,6 +30,7 @@ java_library( "//java/com/google/gerrit/index/testing", "//java/com/google/gerrit/lifecycle", "//java/com/google/gerrit/server", + "//java/com/google/gerrit/server/group/testing", "//java/com/google/gerrit/server/project/testing:project-test-util", "//java/com/google/gerrit/server/schema", "//java/com/google/gerrit/server/util/time", diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java index 5496f56901..4dde452dc9 100644 --- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java +++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesLatestIndexVersionTest.java @@ -26,4 +26,11 @@ public class FakeQueryChangesLatestIndexVersionTest extends FakeQueryChangesTest public static Config defaultConfig() { return IndexConfig.createForFake(); } + + @ConfigSuite.Config + public static Config searchAfterPaginationType() { + Config config = defaultConfig(); + config.setString("index", null, "paginationType", "SEARCH_AFTER"); + return config; + } } diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java index 1610ecacb4..95896dc991 100644 --- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java +++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesPreviousIndexVersionTest.java @@ -36,4 +36,11 @@ public class FakeQueryChangesPreviousIndexVersionTest extends FakeQueryChangesTe IndexConfig.createForFake()) .values()); } + + @ConfigSuite.Config + public static Config searchAfterPaginationType() { + Config config = againstPreviousIndexVersion(); + config.setString("index", null, "paginationType", "SEARCH_AFTER"); + return config; + } } diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java index 1e23420b04..01ae9601e5 100644 --- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java @@ -14,10 +14,27 @@ package com.google.gerrit.server.query.change; +import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.block; +import static com.google.gerrit.common.data.GlobalCapability.QUERY_LIMIT; +import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; + +import com.google.gerrit.acceptance.UseClockStep; +import com.google.gerrit.entities.Permission; +import com.google.gerrit.entities.Project; +import com.google.gerrit.index.testing.AbstractFakeIndex; +import com.google.gerrit.server.config.AllProjectsName; +import com.google.gerrit.server.index.change.ChangeIndexCollection; import com.google.gerrit.testing.InMemoryModule; +import com.google.gerrit.testing.InMemoryRepositoryManager; +import com.google.gerrit.testing.InMemoryRepositoryManager.Repo; import com.google.inject.Guice; +import com.google.inject.Inject; import com.google.inject.Injector; +import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; +import org.junit.Test; /** * Test against {@link com.google.gerrit.index.testing.AbstractFakeIndex}. This test might seem @@ -25,6 +42,9 @@ import org.eclipse.jgit.lib.Config; * results as production indices. */ public abstract class FakeQueryChangesTest extends AbstractQueryChangesTest { + @Inject private ChangeIndexCollection changeIndexCollection; + @Inject protected AllProjectsName allProjects; + @Override protected Injector createInjector() { Config fakeConfig = new Config(config); @@ -32,4 +52,57 @@ public abstract class FakeQueryChangesTest extends AbstractQueryChangesTest { fakeConfig.setString("index", null, "type", "fake"); return Guice.createInjector(new InMemoryModule(fakeConfig)); } + + @Test + @UseClockStep + @SuppressWarnings("unchecked") + public void stopQueryIfNoMoreResults() throws Exception { + // create 2 visible changes + TestRepository<InMemoryRepositoryManager.Repo> testRepo = createProject("repo"); + insert(testRepo, newChange(testRepo)); + insert(testRepo, newChange(testRepo)); + + // create 2 invisible changes + TestRepository<Repo> hiddenProject = createProject("hiddenProject"); + insert(hiddenProject, newChange(hiddenProject)); + insert(hiddenProject, newChange(hiddenProject)); + projectOperations + .project(Project.nameKey("hiddenProject")) + .forUpdate() + .add(block(Permission.READ).ref("refs/*").group(REGISTERED_USERS)) + .update(); + + AbstractFakeIndex idx = (AbstractFakeIndex) changeIndexCollection.getSearchIndex(); + newQuery("status:new").withLimit(5).get(); + // Since the limit of the query (i.e. 5) is more than the total number of changes (i.e. 4), + // only 1 index search is expected. + assertThat(idx.getQueryCount()).isEqualTo(1); + } + + @Test + @UseClockStep + @SuppressWarnings("unchecked") + public void noLimitQueryPaginates() throws Exception { + TestRepository<InMemoryRepositoryManager.Repo> testRepo = createProject("repo"); + // create 4 changes + insert(testRepo, newChange(testRepo)); + insert(testRepo, newChange(testRepo)); + insert(testRepo, newChange(testRepo)); + insert(testRepo, newChange(testRepo)); + + // Set queryLimit to 2 + projectOperations + .project(allProjects) + .forUpdate() + .add(allowCapability(QUERY_LIMIT).group(REGISTERED_USERS).range(0, 2)) + .update(); + + AbstractFakeIndex idx = (AbstractFakeIndex) changeIndexCollection.getSearchIndex(); + + // 2 index searches are expected. The first index search will run with size 2 (i.e. + // the configured query-limit), and then we will paginate to get the remaining 2 + // changes with the second index search. + newQuery("status:new").withNoLimit().get(); + assertThat(idx.getQueryCount()).isEqualTo(2); + } } diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java index 52a9170fe1..45879433e6 100644 --- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java +++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesLatestIndexVersionTest.java @@ -23,4 +23,11 @@ public class LuceneQueryChangesLatestIndexVersionTest extends LuceneQueryChanges public static Config defaultConfig() { return IndexConfig.createForLucene(); } + + @ConfigSuite.Config + public static Config searchAfterPaginationType() { + Config config = defaultConfig(); + config.setString("index", null, "paginationType", "SEARCH_AFTER"); + return config; + } } diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java index 62483fa0b7..178269730c 100644 --- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java +++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesPreviousIndexVersionTest.java @@ -33,4 +33,11 @@ public class LuceneQueryChangesPreviousIndexVersionTest extends LuceneQueryChang IndexConfig.createForLucene()) .values()); } + + @ConfigSuite.Config + public static Config searchAfterPaginationType() { + Config config = againstPreviousIndexVersion(); + config.setString("index", null, "paginationType", "SEARCH_AFTER"); + return config; + } } diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java index 6a83fb9d9c..9717bfb919 100644 --- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java +++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java @@ -15,13 +15,18 @@ package com.google.gerrit.server.query.change; import static com.google.common.truth.Truth.assertThat; +import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowCapability; +import static com.google.gerrit.common.data.GlobalCapability.QUERY_LIMIT; +import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS; import static com.google.gerrit.testing.GerritJUnit.assertThrows; import com.google.gerrit.entities.Change; import com.google.gerrit.extensions.restapi.BadRequestException; +import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.testing.InMemoryModule; import com.google.gerrit.testing.InMemoryRepositoryManager.Repo; import com.google.inject.Guice; +import com.google.inject.Inject; import com.google.inject.Injector; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.Config; @@ -29,6 +34,8 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.junit.Test; public abstract class LuceneQueryChangesTest extends AbstractQueryChangesTest { + @Inject protected AllProjectsName allProjects; + @Override protected Injector createInjector() { Config luceneConfig = new Config(config); @@ -66,4 +73,29 @@ public abstract class LuceneQueryChangesTest extends AbstractQueryChangesTest { () -> assertQuery("owner: \"" + nameEmail + "\"\\", change1)); assertThat(thrown).hasMessageThat().contains("Cannot create full-text query with value: \\"); } + + @Test + public void openAndClosedChanges() throws Exception { + TestRepository<Repo> repo = createProject("repo"); + + // create 3 closed changes + Change change1 = insert(repo, newChangeWithStatus(repo, Change.Status.MERGED)); + Change change2 = insert(repo, newChangeWithStatus(repo, Change.Status.MERGED)); + Change change3 = insert(repo, newChangeWithStatus(repo, Change.Status.MERGED)); + + // create 3 new changes + Change change4 = insert(repo, newChangeWithStatus(repo, Change.Status.NEW)); + Change change5 = insert(repo, newChangeWithStatus(repo, Change.Status.NEW)); + Change change6 = insert(repo, newChangeWithStatus(repo, Change.Status.NEW)); + + // Set queryLimit to 1 + projectOperations + .project(allProjects) + .forUpdate() + .add(allowCapability(QUERY_LIMIT).group(REGISTERED_USERS).range(0, 1)) + .update(); + + Change[] expected = new Change[] {change6, change5, change4, change3, change2, change1}; + assertQuery(newQuery("project:repo").withNoLimit(), expected); + } } diff --git a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java index 568b5a017a..6e4fec141f 100644 --- a/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java +++ b/javatests/com/google/gerrit/server/query/group/AbstractQueryGroupsTest.java @@ -374,7 +374,7 @@ public abstract class AbstractQueryGroupsTest extends GerritServerTests { .getRaw( uuid, QueryOptions.create( - IndexConfig.createDefault(), + IndexConfig.fromConfig(config).build(), 0, 10, indexes.getSearchIndex().getSchema().getStoredFields().keySet())); diff --git a/modules/jgit b/modules/jgit -Subproject 78c9b9260a5287d09c87b407e39602159071451 +Subproject d01376106af8800017ac3c08d7c7ac1fd5ccc9e diff --git a/plugins/gitiles b/plugins/gitiles -Subproject 9011b868d1fab8ae35d41cafed922b0b19899dd +Subproject 406c99fc5c5a5b88d09767690e07a5488d7496b diff --git a/polygerrit-ui/app/api/rest-api.ts b/polygerrit-ui/app/api/rest-api.ts index 2315a4eb88..34a5afe06d 100644 --- a/polygerrit-ui/app/api/rest-api.ts +++ b/polygerrit-ui/app/api/rest-api.ts @@ -133,7 +133,7 @@ export enum HttpMethod { export enum InheritedBooleanInfoConfiguredValue { TRUE = 'TRUE', FALSE = 'FALSE', - INHERITED = 'INHERITED', + INHERIT = 'INHERIT', } /** diff --git a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.ts b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.ts index 15f6f4b880..84b7bce9cc 100644 --- a/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.ts +++ b/polygerrit-ui/app/elements/admin/gr-create-change-dialog/gr-create-change-dialog.ts @@ -191,7 +191,7 @@ export class GrCreateChangeDialog extends PolymerElement { return false; } else if ( config && - config.configured_value === InheritedBooleanInfoConfiguredValue.INHERITED + config.configured_value === InheritedBooleanInfoConfiguredValue.INHERIT ) { return !!(config && config.inherited_value); } else { diff --git a/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list.ts b/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list.ts index adcfb64f9c..ff9f66597a 100644 --- a/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list.ts +++ b/polygerrit-ui/app/elements/admin/gr-repo-list/gr-repo-list.ts @@ -143,9 +143,7 @@ export class GrRepoList extends PolymerElement { if (filter !== this._filter || !repos) { return; } - this._repos = repos.filter(repo => - repo.name.toLowerCase().includes(filter.toLowerCase()) - ); + this._repos = repos; this._loading = false; }); } diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts index 236031289c..e11ac9b759 100644 --- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts +++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.ts @@ -39,7 +39,6 @@ import {ChangeListViewState} from '../../../types/types'; import {fireTitleChange} from '../../../utils/event-util'; import {appContext} from '../../../services/app-context'; import {GerritView} from '../../../services/router/router-model'; -import {RELOAD_DASHBOARD_INTERVAL_MS} from '../../../constants/constants'; const LOOKUP_QUERY_PATTERNS: RegExp[] = [ /^\s*i?[0-9a-f]{7,40}\s*$/i, // CHANGE_ID @@ -113,26 +112,11 @@ export class GrChangeListView extends PolymerElement { private reporting = appContext.reportingService; - private lastVisibleTimestampMs = 0; - constructor() { super(); this.addEventListener('next-page', () => this._handleNextPage()); this.addEventListener('previous-page', () => this._handlePreviousPage()); this.addEventListener('reload', () => this.reload()); - // We are not currently verifying if the view is actually visible. We rely - // on gr-app-element to restamp the component if view changes - document.addEventListener('visibilitychange', () => { - if (document.visibilityState === 'visible') { - if ( - Date.now() - this.lastVisibleTimestampMs > - RELOAD_DASHBOARD_INTERVAL_MS - ) - this.reload(); - } else { - this.lastVisibleTimestampMs = Date.now(); - } - }); } override connectedCallback() { diff --git a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts index 5f91dfb2f0..d18911620c 100644 --- a/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts +++ b/polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.ts @@ -114,6 +114,8 @@ import {GrDropdown} from '../../shared/gr-dropdown/gr-dropdown'; const ERR_BRANCH_EMPTY = 'The destination branch can’t be empty.'; const ERR_COMMIT_EMPTY = 'The commit message can’t be empty.'; const ERR_REVISION_ACTIONS = 'Couldn’t load revision actions.'; +const CHERRYPICK_IN_PROGRESS = 'Cherry-pick in progress...'; +const CHERRYPICK_COMPLETED = 'Cherry-pick completed'; enum LabelStatus { /** @@ -1424,6 +1426,7 @@ export class GrChangeActions } this.$.overlay.close(); el.hidden = true; + fireAlert(this, CHERRYPICK_IN_PROGRESS); this._fireAction( '/cherrypick', assertUIActionInfo(this.revisionActions.cherrypick), @@ -1640,6 +1643,7 @@ export class GrChangeActions const cherrypickChangeInfo: ChangeInfo = obj as unknown as ChangeInfo; this._waitForChangeReachable(cherrypickChangeInfo._number).then( () => { + fireAlert(this, CHERRYPICK_COMPLETED); GerritNav.navigateToChange(cherrypickChangeInfo); } ); diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts index 8b46cd8874..24d0e768c1 100644 --- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts +++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata.ts @@ -44,6 +44,7 @@ import {GerritNav} from '../../core/gr-navigation/gr-navigation'; import { ChangeStatus, GpgKeyInfoStatus, + InheritedBooleanInfoConfiguredValue, SubmitType, } from '../../../constants/constants'; import {changeIsOpen} from '../../../utils/change-util'; @@ -55,6 +56,7 @@ import { ChangeInfo, CommitId, CommitInfo, + ConfigInfo, ElementPropertyDeepChange, GpgKeyInfo, Hashtag, @@ -183,7 +185,8 @@ export class GrChangeMetadata extends PolymerElement { @property({ type: Object, - computed: '_computePushCertificateValidation(serverConfig, change)', + computed: + '_computePushCertificateValidation(serverConfig, change, repoConfig)', }) _pushCertificateValidation?: PushCertificateValidationInfo; @@ -220,6 +223,9 @@ export class GrChangeMetadata extends PolymerElement { @property({type: Boolean}) _isSubmitRequirementsUiEnabled = false; + @property({type: Object}) + repoConfig?: ConfigInfo; + restApiService = appContext.restApiService; private readonly reporting = appContext.reportingService; @@ -422,10 +428,14 @@ export class GrChangeMetadata extends PolymerElement { */ _computePushCertificateValidation( serverConfig?: ServerInfo, - change?: ParsedChangeInfo + change?: ParsedChangeInfo, + repoConfig?: ConfigInfo ): PushCertificateValidationInfo | undefined { if (!change || !serverConfig?.receive?.enable_signed_push) return undefined; + if (!this.isEnabledSignedPushOnRepo(repoConfig)) { + return undefined; + } const rev = change.revisions[change.current_revision]; if (!rev.push_certificate?.key) { return { @@ -469,6 +479,20 @@ export class GrChangeMetadata extends PolymerElement { } } + // private but used in test + isEnabledSignedPushOnRepo(repoConfig?: ConfigInfo) { + if (!repoConfig?.enable_signed_push) return false; + + const enableSignedPush = repoConfig.enable_signed_push; + return ( + (enableSignedPush.configured_value === + InheritedBooleanInfoConfiguredValue.INHERIT && + enableSignedPush.inherited_value) || + enableSignedPush.configured_value === + InheritedBooleanInfoConfiguredValue.TRUE + ); + } + _problems(msg: string, key: GpgKeyInfo) { if (!key?.problems || key.problems.length === 0) { return msg; diff --git a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.ts b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.ts index 422c91b343..31908bb2d9 100644 --- a/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.ts +++ b/polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata_test.ts @@ -36,12 +36,14 @@ import { createRevision, createAccountDetailWithId, createChangeConfig, + createConfig, } from '../../../test/test-data-generators'; import { ChangeStatus, SubmitType, RequirementStatus, GpgKeyInfoStatus, + InheritedBooleanInfoConfiguredValue, } from '../../../constants/constants'; import { EmailAddress, @@ -485,6 +487,13 @@ suite('gr-change-metadata tests', () => { labels: {}, mergeable: true, }; + element.repoConfig = { + ...createConfig(), + enable_signed_push: { + configured_value: 'TRUE' as InheritedBooleanInfoConfiguredValue, + value: true, + }, + }; }); test('Push Certificate Validation test BAD', () => { @@ -497,7 +506,8 @@ suite('gr-change-metadata tests', () => { }; const result = element._computePushCertificateValidation( serverConfig, - change + change, + element.repoConfig ); assert.equal( result?.message, @@ -517,7 +527,8 @@ suite('gr-change-metadata tests', () => { }; const result = element._computePushCertificateValidation( serverConfig, - change + change, + element.repoConfig ); assert.equal( result?.message, @@ -531,7 +542,8 @@ suite('gr-change-metadata tests', () => { change!.revisions.rev1! = createRevision(1); const result = element._computePushCertificateValidation( serverConfig, - change + change, + element.repoConfig ); assert.equal( result?.message, @@ -540,6 +552,41 @@ suite('gr-change-metadata tests', () => { assert.equal(result?.icon, 'gr-icons:help'); assert.equal(result?.class, 'help'); }); + + test('_computePushCertificateValidation returns undefined', () => { + delete serverConfig!.receive!.enable_signed_push; + const result = element._computePushCertificateValidation( + serverConfig, + change, + element.repoConfig + ); + assert.isUndefined(result); + }); + + test('isEnabledSignedPushOnRepo', () => { + change!.revisions.rev1!.push_certificate = { + certificate: 'Push certificate', + key: { + status: GpgKeyInfoStatus.TRUSTED, + }, + }; + element.change = change; + element.serverConfig = serverConfig; + element.repoConfig!.enable_signed_push!.configured_value = + InheritedBooleanInfoConfiguredValue.INHERIT; + element.repoConfig!.enable_signed_push!.inherited_value = true; + assert.isTrue(element.isEnabledSignedPushOnRepo(element.repoConfig)); + + element.repoConfig!.enable_signed_push!.inherited_value = false; + assert.isFalse(element.isEnabledSignedPushOnRepo(element.repoConfig)); + + element.repoConfig!.enable_signed_push!.configured_value = + InheritedBooleanInfoConfiguredValue.TRUE; + assert.isTrue(element.isEnabledSignedPushOnRepo(element.repoConfig)); + + element.repoConfig = undefined; + assert.isFalse(element.isEnabledSignedPushOnRepo(element.repoConfig)); + }); }); test('_computeParents', () => { diff --git a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts index 645b835bde..b97d3ab405 100644 --- a/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts +++ b/polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_html.ts @@ -398,6 +398,7 @@ export const htmlTemplate = html` commit-info="[[_commitInfo]]" server-config="[[_serverConfig]]" parent-is-current="[[_parentIsCurrent]]" + repo-config="[[_projectConfig]]" on-show-reply-dialog="_handleShowReplyDialog" > </gr-change-metadata> diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts index 95e43014df..904b74299d 100644 --- a/polygerrit-ui/app/elements/change/gr-message/gr-message.ts +++ b/polygerrit-ui/app/elements/change/gr-message/gr-message.ts @@ -39,6 +39,7 @@ import { PatchSetNum, AccountInfo, BasePatchSetNum, + LabelNameToInfoMap, } from '../../../types/common'; import {CommentThread} from '../../../utils/comment-util'; import {hasOwnProperty} from '../../../utils/common-util'; @@ -182,7 +183,8 @@ export class GrMessage extends PolymerElement { computed: '_computeMessageContentExpanded(_expanded, message.message,' + ' message.accounts_in_message,' + - ' message.tag)', + ' message.tag,' + + ' change.labels)', }) _messageContentExpanded = ''; @@ -192,7 +194,8 @@ export class GrMessage extends PolymerElement { '_computeMessageContentCollapsed(message.message,' + ' message.accounts_in_message,' + ' message.tag,' + - ' commentThreads)', + ' commentThreads,' + + ' change.labels)', }) _messageContentCollapsed = ''; @@ -243,10 +246,17 @@ export class GrMessage extends PolymerElement { expanded: boolean, content?: string, accountsInMessage?: AccountInfo[], - tag?: ReviewInputTag + tag?: ReviewInputTag, + labels?: LabelNameToInfoMap ) { if (!expanded) return ''; - return this._computeMessageContent(true, content, accountsInMessage, tag); + return this._computeMessageContent( + true, + content, + accountsInMessage, + tag, + labels + ); } _patchsetCommentSummary(commentThreads: CommentThread[] = []) { @@ -277,7 +287,8 @@ export class GrMessage extends PolymerElement { content?: string, accountsInMessage?: AccountInfo[], tag?: ReviewInputTag, - commentThreads?: CommentThread[] + commentThreads?: CommentThread[], + labels?: LabelNameToInfoMap ) { // Content is under text-overflow, so it's always shorten const shortenedContent = content?.substring(0, 1000); @@ -285,7 +296,8 @@ export class GrMessage extends PolymerElement { false, shortenedContent, accountsInMessage, - tag + tag, + labels ); if (summary || !commentThreads) return summary; return this._patchsetCommentSummary(commentThreads); @@ -342,7 +354,8 @@ export class GrMessage extends PolymerElement { isExpanded: boolean, content?: string, accountsInMessage?: AccountInfo[], - tag?: ReviewInputTag + tag?: ReviewInputTag, + labels?: LabelNameToInfoMap ) { if (!content) return ''; const isNewPatchSet = this._isNewPatchsetTag(tag); @@ -362,8 +375,24 @@ export class GrMessage extends PolymerElement { if (line.startsWith('(') && line.endsWith(' comments)')) { return false; } - if (!isNewPatchSet && line.match(PATCH_SET_PREFIX_PATTERN)) { - return false; + if (!isNewPatchSet && labels) { + // Legacy change messages may contain the 'Patch Set' prefix + // and a message(not containing label scores) on the same line. + // To handle them correctly, only filter out lines which contain + // the 'Patch Set' prefix and label scores. + const match = line.match(PATCH_SET_PREFIX_PATTERN); + if (match && match[1]) { + const message = match[1].split(' '); + if ( + message + .map(s => s.match(LABEL_TITLE_SCORE_PATTERN)) + .filter( + ms => ms && ms.length === 4 && hasOwnProperty(labels, ms[2]) + ).length === message.length + ) { + return false; + } + } } return true; }); @@ -378,7 +407,7 @@ export class GrMessage extends PolymerElement { // Only make this replacement if the line starts with Patch Set, since if // it starts with "Uploaded patch set" (e.g for votes) we want to keep the // "Uploaded patch set". - if (isNewPatchSet && line.startsWith('Patch Set')) { + if (line.startsWith('Patch Set')) { line = line.replace(PATCH_SET_PREFIX_PATTERN, '$1'); } return line; diff --git a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.ts b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.ts index f87c4c3f1e..9f9b792779 100644 --- a/polygerrit-ui/app/elements/change/gr-message/gr-message_test.ts +++ b/polygerrit-ui/app/elements/change/gr-message/gr-message_test.ts @@ -24,6 +24,7 @@ import { createChangeMessage, createComment, createRevisions, + createLabelInfo, } from '../../../test/test-data-generators'; import { mockPromise, @@ -353,13 +354,18 @@ suite('gr-message tests', () => { }); suite('compute messages', () => { + const labels = { + 'Code-Review': createLabelInfo(1), + 'Code-Style': createLabelInfo(1), + }; test('empty', () => { assert.equal( element._computeMessageContent( true, '', undefined, - '' as ReviewInputTag + '' as ReviewInputTag, + labels ), '' ); @@ -368,7 +374,8 @@ suite('gr-message tests', () => { false, '', undefined, - '' as ReviewInputTag + '' as ReviewInputTag, + labels ), '' ); @@ -380,10 +387,16 @@ suite('gr-message tests', () => { let actual = element._computeMessageContent(true, original, [], tag); assert.equal( actual, - element._computeMessageContentCollapsed(original, [], tag, []) + element._computeMessageContentCollapsed(original, [], tag, [], labels) ); assert.equal(actual, original); - actual = element._computeMessageContent(false, original, [], tag); + actual = element._computeMessageContent( + false, + original, + [], + tag, + labels + ); assert.equal(actual, original); }); @@ -391,13 +404,25 @@ suite('gr-message tests', () => { const original = 'Patch Set 27: Patch Set 26 was rebased'; const tag = 'autogenerated:gerrit:newPatchSet' as ReviewInputTag; const expected = 'Patch Set 26 was rebased'; - let actual = element._computeMessageContent(true, original, [], tag); + let actual = element._computeMessageContent( + true, + original, + [], + tag, + labels + ); assert.equal(actual, expected); assert.equal( actual, element._computeMessageContentCollapsed(original, [], tag, []) ); - actual = element._computeMessageContent(false, original, [], tag); + actual = element._computeMessageContent( + false, + original, + [], + tag, + labels + ); assert.equal(actual, expected); }); @@ -405,31 +430,89 @@ suite('gr-message tests', () => { const original = 'Patch Set 1:\n\nThis change is ready for review.'; const tag = undefined; const expected = 'This change is ready for review.'; - let actual = element._computeMessageContent(true, original, [], tag); + let actual = element._computeMessageContent( + true, + original, + [], + tag, + labels + ); assert.equal(actual, expected); assert.equal( actual, element._computeMessageContentCollapsed(original, [], tag, []) ); - actual = element._computeMessageContent(false, original, [], tag); + actual = element._computeMessageContent( + false, + original, + [], + tag, + labels + ); assert.equal(actual, expected); }); test('new patchset with vote', () => { const original = 'Uploaded patch set 2: Code-Review+1'; const tag = 'autogenerated:gerrit:newPatchSet' as ReviewInputTag; const expected = 'Uploaded patch set 2: Code-Review+1'; - let actual = element._computeMessageContent(true, original, [], tag); + let actual = element._computeMessageContent( + true, + original, + [], + tag, + labels + ); assert.equal(actual, expected); - actual = element._computeMessageContent(false, original, [], tag); + actual = element._computeMessageContent( + false, + original, + [], + tag, + labels + ); assert.equal(actual, expected); }); test('vote', () => { const original = 'Patch Set 1: Code-Style+1'; const tag = undefined; const expected = ''; - let actual = element._computeMessageContent(true, original, [], tag); + let actual = element._computeMessageContent( + true, + original, + [], + tag, + labels + ); + assert.equal(actual, expected); + actual = element._computeMessageContent( + false, + original, + [], + tag, + labels + ); + assert.equal(actual, expected); + }); + + test('legacy change message', () => { + const original = 'Patch Set 1: Legacy Message'; + const tag = undefined; + const expected = 'Legacy Message'; + let actual = element._computeMessageContent( + true, + original, + [], + tag, + labels + ); assert.equal(actual, expected); - actual = element._computeMessageContent(false, original, [], tag); + actual = element._computeMessageContent( + false, + original, + [], + tag, + labels + ); assert.equal(actual, expected); }); @@ -437,9 +520,21 @@ suite('gr-message tests', () => { const original = 'Patch Set 1:\n\n(3 comments)'; const tag = undefined; const expected = ''; - let actual = element._computeMessageContent(true, original, [], tag); + let actual = element._computeMessageContent( + true, + original, + [], + tag, + labels + ); assert.equal(actual, expected); - actual = element._computeMessageContent(false, original, [], tag); + actual = element._computeMessageContent( + false, + original, + [], + tag, + labels + ); assert.equal(actual, expected); }); @@ -457,14 +552,16 @@ suite('gr-message tests', () => { true, original, accountsInMessage, - tag + tag, + labels ); assert.equal(actual, expected); actual = element._computeMessageContent( false, original, accountsInMessage, - tag + tag, + labels ); assert.equal(actual, expected); }); @@ -475,9 +572,21 @@ suite('gr-message tests', () => { const tag = undefined; const expected = 'Removed vote: \n\n * Code-Style+1 by Gerrit Account 1\n * Code-Style-1 by Gerrit Account 2'; - let actual = element._computeMessageContent(true, original, [], tag); + let actual = element._computeMessageContent( + true, + original, + [], + tag, + labels + ); assert.equal(actual, expected); - actual = element._computeMessageContent(false, original, [], tag); + actual = element._computeMessageContent( + false, + original, + [], + tag, + labels + ); assert.equal(actual, expected); }); }); diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts index 08139006a6..87d28f2f39 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.ts @@ -1137,17 +1137,20 @@ export class GrDiffView extends base { } } - @observe('_path', '_prefs', '_reviewedFiles', '_patchRange') + @observe('_loggedIn', '_path', '_prefs', '_reviewedFiles', '_patchRange') _setReviewedObserver( + _loggedIn?: boolean, path?: string, prefs?: DiffPreferencesInfo, reviewedFiles?: Set<string>, patchRange?: PatchRange ) { + if (_loggedIn === undefined) return; if (prefs === undefined) return; if (path === undefined) return; if (reviewedFiles === undefined) return; if (patchRange === undefined) return; + if (!_loggedIn) return; if (prefs.manual_review) { // Checkbox state needs to be set explicitly only when manual_review // is specified. diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts index dd2c8d3219..d468cf9674 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.ts @@ -99,6 +99,7 @@ import { HashtagsInput, ImagesForDiff, IncludedInInfo, + LabelNameToLabelTypeInfoMap, MergeableInfo, NameToProjectInfoMap, NumericChangeId, @@ -132,11 +133,13 @@ import { TagInput, TopMenuEntryInfo, UrlEncodedCommentId, + UrlEncodedRepoName, } from '../../../types/common'; import { DiffInfo, DiffPreferencesInfo, IgnoreWhitespaceType, + WebLinkInfo, } from '../../../types/diff'; import { CancelConditionCallback, @@ -151,6 +154,7 @@ import { createDefaultPreferences, DiffViewMode, HttpMethod, + ProjectState, ReviewerState, } from '../../../constants/constants'; import {firePageError, fireServerError} from '../../../utils/event-util'; @@ -1415,36 +1419,26 @@ export class GrRestApiInterface filter: string | undefined, reposPerPage: number, offset?: number - ) { - const defaultFilter = 'state:active OR state:read-only'; - const namePartDelimiters = /[@.\-\s/_]/g; + ): [boolean, string] { + const defaultFilter = ''; offset = offset || 0; + filter ??= defaultFilter; + const encodedFilter = encodeURIComponent(filter); - if (filter && !filter.includes(':') && filter.match(namePartDelimiters)) { - // The query language specifies hyphens as operators. Split the string - // by hyphens and 'AND' the parts together as 'inname:' queries. + if (filter.includes(':')) { // If the filter includes a semicolon, the user is using a more complex // query so we trust them and don't do any magic under the hood. - const originalFilter = filter; - filter = ''; - originalFilter.split(namePartDelimiters).forEach(part => { - if (part) { - filter += (filter === '' ? 'inname:' : ' AND inname:') + part; - } - }); - } - // Check if filter is now empty which could be either because the user did - // not provide it or because the user provided only a split character. - if (!filter) { - filter = defaultFilter; + return [ + true, + `/projects/?n=${reposPerPage + 1}&S=${offset}` + + `&query=${encodedFilter}`, + ]; } - filter = filter.trim(); - const encodedFilter = encodeURIComponent(filter); - - return ( - `/projects/?n=${reposPerPage + 1}&S=${offset}` + `&query=${encodedFilter}` - ); + return [ + false, + `/projects/?n=${reposPerPage + 1}&S=${offset}` + `&d=&m=${encodedFilter}`, + ]; } invalidateGroupsCache() { @@ -1477,14 +1471,47 @@ export class GrRestApiInterface reposPerPage: number, offset?: number ): Promise<ProjectInfoWithName[] | undefined> { - const url = this._getReposUrl(filter, reposPerPage, offset); + const [isQuery, url] = this._getReposUrl(filter, reposPerPage, offset); // TODO(kaspern): Rename rest api from /projects/ to /repos/ once backend // supports it. - return this._fetchSharedCacheURL({ - url, // The url contains query,so the response is an array, not map - anonymizedUrl: '/projects/?*', - }) as Promise<ProjectInfoWithName[] | undefined>; + // If query then return directly as the result will be expected to be an array + if (isQuery) { + return this._fetchSharedCacheURL({ + url, // The url contains query,so the response is an array, not map + anonymizedUrl: '/projects/?*', + }) as Promise<ProjectInfoWithName[] | undefined>; + } + const result: Promise<NameToProjectInfoMap[] | undefined> = + this._fetchSharedCacheURL({ + url, // The url contains query,so the response is an array, not map + anonymizedUrl: '/projects/?*', + }) as Promise<NameToProjectInfoMap[] | undefined>; + return this._transformToArray(result); + } + + _transformToArray( + res: Promise<NameToProjectInfoMap[] | undefined> + ): Promise<ProjectInfoWithName[] | undefined> { + return res.then(response => { + const reposList: ProjectInfoWithName[] = []; + for (const [name, project] of Object.entries(response ?? {})) { + const projectInfo: ProjectInfoWithName = { + id: project.id as unknown as UrlEncodedRepoName, + name: name as RepoName, + parent: project.parent as unknown as RepoName, + description: project.description as unknown as string, + state: project.state as unknown as ProjectState, + branches: project.branches as unknown as { + [branchName: string]: CommitId; + }, + labels: project.labels as unknown as LabelNameToLabelTypeInfoMap, + web_links: project.web_links as unknown as WebLinkInfo[], + }; + reposList.push(projectInfo); + } + return reposList; + }); } setRepoHead(repo: RepoName, ref: GitRef) { diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js index a60a1efef8..df37502f9c 100644 --- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js +++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface_test.js @@ -676,16 +676,19 @@ suite('gr-rest-api-interface tests', () => { }); test('normal use', () => { - const defaultQuery = 'state%3Aactive%20OR%20state%3Aread-only'; + const defaultQuery = ''; - assert.equal(element._getReposUrl('test', 25), - '/projects/?n=26&S=0&query=test'); + assert.equal(element._getReposUrl('test', 25).toString(), + [false, '/projects/?n=26&S=0&d=&m=test'].toString()); - assert.equal(element._getReposUrl(null, 25), - `/projects/?n=26&S=0&query=${defaultQuery}`); + assert.equal(element._getReposUrl(null, 25).toString(), + [false, `/projects/?n=26&S=0&d=&m=${defaultQuery}`].toString()); - assert.equal(element._getReposUrl('test', 25, 25), - '/projects/?n=26&S=25&query=test'); + assert.equal(element._getReposUrl('test', 25, 25).toString(), + [false, '/projects/?n=26&S=25&d=&m=test'].toString()); + + assert.equal(element._getReposUrl('inname:test', 25, 25).toString(), + [true, '/projects/?n=26&S=25&query=inname%3Atest'].toString()); }); test('invalidateReposCache', () => { @@ -713,67 +716,74 @@ suite('gr-rest-api-interface tests', () => { }); suite('getRepos', () => { - const defaultQuery = 'state%3Aactive%20OR%20state%3Aread-only'; + const defaultQuery = ''; let fetchCacheURLStub; setup(() => { fetchCacheURLStub = - sinon.stub(element._restApiHelper, 'fetchCacheURL'); + sinon.stub(element._restApiHelper, 'fetchCacheURL') + .returns(Promise.resolve([])); }); test('normal use', () => { element.getRepos('test', 25); assert.equal(fetchCacheURLStub.lastCall.args[0].url, - '/projects/?n=26&S=0&query=test'); + '/projects/?n=26&S=0&d=&m=test'); element.getRepos(null, 25); assert.equal(fetchCacheURLStub.lastCall.args[0].url, - `/projects/?n=26&S=0&query=${defaultQuery}`); + `/projects/?n=26&S=0&d=&m=${defaultQuery}`); element.getRepos('test', 25, 25); assert.equal(fetchCacheURLStub.lastCall.args[0].url, - '/projects/?n=26&S=25&query=test'); + '/projects/?n=26&S=25&d=&m=test'); }); test('with blank', () => { element.getRepos('test/test', 25); assert.equal(fetchCacheURLStub.lastCall.args[0].url, - '/projects/?n=26&S=0&query=inname%3Atest%20AND%20inname%3Atest'); + '/projects/?n=26&S=0&d=&m=test%2Ftest'); }); test('with hyphen', () => { element.getRepos('foo-bar', 25); assert.equal(fetchCacheURLStub.lastCall.args[0].url, - '/projects/?n=26&S=0&query=inname%3Afoo%20AND%20inname%3Abar'); + '/projects/?n=26&S=0&d=&m=foo-bar'); }); test('with leading hyphen', () => { element.getRepos('-bar', 25); assert.equal(fetchCacheURLStub.lastCall.args[0].url, - '/projects/?n=26&S=0&query=inname%3Abar'); + '/projects/?n=26&S=0&d=&m=-bar'); }); test('with trailing hyphen', () => { element.getRepos('foo-bar-', 25); assert.equal(fetchCacheURLStub.lastCall.args[0].url, - '/projects/?n=26&S=0&query=inname%3Afoo%20AND%20inname%3Abar'); + '/projects/?n=26&S=0&d=&m=foo-bar-'); }); test('with underscore', () => { element.getRepos('foo_bar', 25); assert.equal(fetchCacheURLStub.lastCall.args[0].url, - '/projects/?n=26&S=0&query=inname%3Afoo%20AND%20inname%3Abar'); + '/projects/?n=26&S=0&d=&m=foo_bar'); }); test('with underscore', () => { element.getRepos('foo_bar', 25); assert.equal(fetchCacheURLStub.lastCall.args[0].url, - '/projects/?n=26&S=0&query=inname%3Afoo%20AND%20inname%3Abar'); + '/projects/?n=26&S=0&d=&m=foo_bar'); }); test('hyphen only', () => { element.getRepos('-', 25); assert.equal(fetchCacheURLStub.lastCall.args[0].url, - `/projects/?n=26&S=0&query=${defaultQuery}`); + `/projects/?n=26&S=0&d=&m=-`); + }); + + test('using query', () =>{ + element.getRepos('description:project', 25); + assert.equal(fetchCacheURLStub.lastCall.args[0].url, + `/projects/?n=26&S=0&query=description%3Aproject`); }); }); diff --git a/polygerrit-ui/app/test/test-data-generators.ts b/polygerrit-ui/app/test/test-data-generators.ts index 351cc13818..76f0cd1679 100644 --- a/polygerrit-ui/app/test/test-data-generators.ts +++ b/polygerrit-ui/app/test/test-data-generators.ts @@ -49,6 +49,7 @@ import { GroupId, GroupInfo, InheritedBooleanInfo, + LabelInfo, MaxObjectSizeLimitInfo, MergeableInfo, NumericChangeId, @@ -211,6 +212,30 @@ export function createGitPerson(name = 'Test name'): GitPersonInfo { }; } +export function createLabelInfo(score = 1): LabelInfo { + return { + all: [ + { + value: score, + permitted_voting_range: { + min: -1, + max: 1, + }, + _account_id: 1000 as AccountId, + name: 'Foo', + email: 'foo@example.com' as EmailAddress, + username: 'foo', + }, + ], + values: { + '-1': 'Fail', + ' 0': 'No score', + '+1': 'Pass', + }, + default_value: 0, + }; +} + export function createCommit(): CommitInfo { return { parents: [], diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml index 789ec611f8..fe7972b9c0 100644 --- a/tools/maven/gerrit-acceptance-framework_pom.xml +++ b/tools/maven/gerrit-acceptance-framework_pom.xml @@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-acceptance-framework</artifactId> - <version>3.5.2</version> + <version>3.5.3</version> <packaging>jar</packaging> <name>Gerrit Code Review - Acceptance Test Framework</name> <description>Framework for Gerrit's acceptance tests</description> diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml index 3391fe730c..c0b09a6f1d 100644 --- a/tools/maven/gerrit-extension-api_pom.xml +++ b/tools/maven/gerrit-extension-api_pom.xml @@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-extension-api</artifactId> - <version>3.5.2</version> + <version>3.5.3</version> <packaging>jar</packaging> <name>Gerrit Code Review - Extension API</name> <description>API for Gerrit Extensions</description> diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml index 8c166d8af9..0d61e69319 100644 --- a/tools/maven/gerrit-plugin-api_pom.xml +++ b/tools/maven/gerrit-plugin-api_pom.xml @@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-plugin-api</artifactId> - <version>3.5.2</version> + <version>3.5.3</version> <packaging>jar</packaging> <name>Gerrit Code Review - Plugin API</name> <description>API for Gerrit Plugins</description> diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml index 1a5176f3b0..6942b0d81a 100644 --- a/tools/maven/gerrit-war_pom.xml +++ b/tools/maven/gerrit-war_pom.xml @@ -2,7 +2,7 @@ <modelVersion>4.0.0</modelVersion> <groupId>com.google.gerrit</groupId> <artifactId>gerrit-war</artifactId> - <version>3.5.2</version> + <version>3.5.3</version> <packaging>war</packaging> <name>Gerrit Code Review - WAR</name> <description>Gerrit WAR</description> diff --git a/tools/nongoogle.bzl b/tools/nongoogle.bzl index 8d98189ca8..413a508b00 100644 --- a/tools/nongoogle.bzl +++ b/tools/nongoogle.bzl @@ -65,18 +65,18 @@ def declare_nongoogle_deps(): sha1 = "cb2f351bf4463751201f43bb99865235d5ba07ca", ) - SSHD_VERS = "2.7.0" + SSHD_VERS = "2.8.0" maven_jar( name = "sshd-osgi", artifact = "org.apache.sshd:sshd-osgi:" + SSHD_VERS, - sha1 = "a101aad0f79ad424498098f7e91c39d3d92177c1", + sha1 = "b2a59b73c045f40d5722b9160d4f909a646d86c9", ) maven_jar( name = "sshd-sftp", artifact = "org.apache.sshd:sshd-sftp:" + SSHD_VERS, - sha1 = "0c9eff7145e20b338c1dd6aca36ba93ed7c0147c", + sha1 = "d3cd9bc8d335b3ed1a86d2965deb4d202de27442", ) maven_jar( @@ -94,7 +94,7 @@ def declare_nongoogle_deps(): maven_jar( name = "sshd-mina", artifact = "org.apache.sshd:sshd-mina:" + SSHD_VERS, - sha1 = "22799941ec7bd5170ea890363cb968e400a69c41", + sha1 = "02f78100cce376198be798a37c84aaf945e8a0f7", ) maven_jar( diff --git a/version.bzl b/version.bzl index 2d885b25e6..5b20d57891 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "3.5.2" +GERRIT_VERSION = "3.5.3" |