Merge "Update attention set behaviour when commenting on a thread." into stable-3.11
diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt
index 9af1df8..8e9e1a93 100644
--- a/Documentation/config-gerrit.txt
+++ b/Documentation/config-gerrit.txt
@@ -3684,9 +3684,67 @@
+
Defaults to `false`.
+[[scheduledIndexer]]
+=== Section scheduledIndexer
+
+This section configures periodic indexing. Periodic indexing can be run
+on both primaries and replicas.
+
+
+[[scheduledIndexer.groups]]
+==== Subsection scheduledIndexer.groups
+
+Periodic groups reindexing will be scheduled by default on replicas if there is
+no explicit `scheduledIndexer.groups` configuration.
+
+Replication to replicas happens on Git level so that Gerrit is not aware
+of incoming replication events. But replicas need an updated group index
+to resolve memberships of users for ACL validation. To keep the group
+index in replicas up-to-date the Gerrit replica periodically scans the
+group refs in the All-Users repository to reindex groups if they are
+stale.
+
+[[scheduledIndexer.groups.runOnStartup]]scheduledIndexer.groups.runOnStartup::
++
+Whether the scheduled indexer should run once immediately on startup.
+If set to `true` the server startup is blocked until all stale groups
+were reindexed. Enabling this allows to prevent that servers that were
+offline for a longer period of time run with outdated group information
+until the first scheduled indexing is done.
++
+Defaults to `true` for replicas, `false` for primaries.
+
+[[scheduledIndexer.groups.enabled]]scheduledIndexer.groups.enabled::
++
+Whether the scheduled indexer is enabled. If the scheduled indexer is disabled
+you may need to implement other means to keep the groups index on replicas
+up-to-date.
++
+Defaults to `true` for replicas, `false` for primaries.
+
+[[scheduledIndexer.groups.startTime]]groups.scheduledIndexer.startTime::
++
+The link:#schedule-configuration-startTime[start time] for running
+the scheduled indexer.
++
+Defaults to `00:00`.
+
+[[scheduledIndexer.groups.interval]]scheduledIndexer.groups.interval::
++
+The link:#schedule-configuration-interval[interval] for running
+the scheduled indexer.
++
+Defaults to `5m`.
+
+link:#schedule-configuration-examples[Schedule examples] can be found
+in the link:#schedule-configuration[Schedule Configuration] section.
+
+
[[index.scheduledIndexer]]
==== Subsection index.scheduledIndexer
+*(DEPRECATED)* Use the link:#scheduledIndexer[scheduledIndexer section] instead.
+
This section configures periodic indexing. Periodic indexing is
intended to run only on replicas and only updates the group index.
Replication to replicas happens on Git level so that Gerrit is not aware
diff --git a/Documentation/config-submit-requirements.txt b/Documentation/config-submit-requirements.txt
index 5ab1add..560c77f 100644
--- a/Documentation/config-submit-requirements.txt
+++ b/Documentation/config-submit-requirements.txt
@@ -473,6 +473,38 @@
+
`distinctvoters:[Code-Review,Trust,API-Review],count>2`
+[[operator_label_with_users_arg]]
+label:'<label><operator><value>,users=human_reviewers'::
++
+Extension of the link:user-search.html#labels[label] predicate that
+allows matching changes that have a matching vote from all human
+reviewers. Votes from service users (members of the
+link:access-control.html#service_users[Service Users] group) and the
+change owner are ignored.
++
+If link:config-project-config.html#reviewer.enableByEmail[reviewers by
+email] are present then "user=all_reviewers" doesn't match if the
+expected value is other than 0. Reviewers by email are reviewers that
+don't have a Gerrit account. Without Gerrit account they cannot vote
+on the change, which means changes that have any such reviewers never
+match when a vote from all reviewers is expected.
++
+If a change has no human reviewers, this operator doesn't match
+(because a human review is required but no human reviewer is present).
++
+Examples:
+`label:Code-Review=MAX,users=human_reviewers`
++
+`label:Code-Review>=1,users=human_reviewers`
++
+The 'users' arg cannot be combined with other arguments ('count',
+'user', 'group').
++
+'label:Code-Review=MAX,users=human_reviewers' can be used to
+implement "Want-Code-Review-From-All" functionaly, see
+link#require-code-review-approvals-from-all-human-reviewers-example[examples
+below].
+
[[operator_is_true]]
is:true::
+
@@ -557,7 +589,7 @@
== Examples
[[code-review-example]]
-=== Code-Review Example
+=== Require Code-Review approval from a non-uploader
To define a submit requirement for code-review that requires a maximum vote for
the “Code-Review” label from a non-uploader without a maximum negative vote:
@@ -571,7 +603,7 @@
----
[[exempt-branch-example]]
-=== Exempt a branch Example
+=== Exempt a branch
We could exempt a submit requirement from certain branches. For example,
project owners might want to skip the 'Code-Style' requirement from the
@@ -602,7 +634,7 @@
----
[[require-footer-example]]
-=== Require a footer Example
+=== Require a footer
It's possible to use a submit requirement to require a footer to be present in
the commit message.
@@ -614,6 +646,59 @@
submittableIf = hasfooter:\"Bug\"
----
+[[require-code-review-approvals-from-all-human-reviewers-example]]
+=== Require Code-Review approvals from all human reviewers
+
+The following submit requirement requires a 'Code-Review' approval
+('Code-Review+1' or 'Code-Review+2') from all human reviewers of the
+change. Votes from service users (members of the
+link:access-control.html#service_users[Service Users] group) and the
+change owner are ignored.
+
+The 'applicableIf' condition makes this submit requirement show up in
+the UI only if it is not satisfied (to keep the submit requirement
+showing when it is satisfied omit the 'applicableIf' condition).
+
+If a change has no human reviewers, this submit requirement is
+unsatisfied (because a human review is required but no human reviewer
+is present).
+
+----
+[submit-requirement "Want-Code-Review-From-All"]
+ description = A 'Code-Review' vote is required from all human \
+ reviewers (service users that are reviewers are \
+ ignored).
+ applicableIf = -label:Code-Review>=1,users=human_reviewers
+ submittableIf = label:Code-Review>=1,users=human_reviewers
+----
+
+It is possible to configure the 'Want-Code-Review-From-All' submit
+requirement so that it only applies when a 'Want-Code-Review: all'
+footer is present in the commit message. This way users can enable
+this submit requirement on demand by including this footer into their
+commit messages.
+
+The 'applicableIf' condition checks for the 'Want-Code-Review: all'
+footer and makes this submit requirement show up in the UI only if it
+is not satisfied (to keep the submit requirement showing when it is
+satisfied omit the '-label:Code-Review>=1,users=human_reviewers'
+predicate from the 'applicableIf' condition).
+
+Note, the footer key cannot contain underscores (e.g. using
+'Want_Code_Review: all' as the footer does not work).
+
+----
+[submit-requirement "Want-Code-Review-From-All"]
+ description = A 'Code-Review' vote is required from all human \
+ reviewers (service users that are reviewers are \
+ ignored).
+ applicableIf = footer:\"Want-Code-Review: all\" -label:Code-Review>=1,users=human_reviewers
+ submittableIf = label:Code-Review>=1,users=human_reviewers
+----
+
+For more information about the "users=human_reviewers" arg see
+link:#operator_label_with_users_arg[above].
+
GERRIT
------
Part of link:index.html[Gerrit Code Review]
diff --git a/Documentation/rest-api-changes.txt b/Documentation/rest-api-changes.txt
index 0ed6b27..c199d82 100644
--- a/Documentation/rest-api-changes.txt
+++ b/Documentation/rest-api-changes.txt
@@ -2751,7 +2751,7 @@
}
----
-[[set-work-in-pogress]]
+[[set-work-in-progress]]
=== Set Work-In-Progress
--
'POST /changes/link:#change-id[\{change-id\}]/wip'
diff --git a/java/com/google/gerrit/acceptance/GerritServer.java b/java/com/google/gerrit/acceptance/GerritServer.java
index 9d8674f..cf4cf2c 100644
--- a/java/com/google/gerrit/acceptance/GerritServer.java
+++ b/java/com/google/gerrit/acceptance/GerritServer.java
@@ -58,6 +58,7 @@
import com.google.gerrit.lucene.LuceneIndexModule;
import com.google.gerrit.pgm.Daemon;
import com.google.gerrit.pgm.Init;
+import com.google.gerrit.server.config.GerritOptions;
import com.google.gerrit.server.config.GerritRuntime;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.SitePath;
@@ -525,6 +526,7 @@
new AbstractModule() {
@Override
protected void configure() {
+ bind(GerritOptions.class).toInstance(GerritOptions.DEFAULT);
bind(GerritRuntime.class).toInstance(GerritRuntime.DAEMON);
}
},
diff --git a/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java b/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java
index 9a652e3..c313a06 100644
--- a/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java
+++ b/java/com/google/gerrit/acceptance/InMemoryTestingDatabaseModule.java
@@ -25,6 +25,7 @@
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.GerritIsReplica;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.config.GlobalPluginConfigProvider;
import com.google.gerrit.server.config.MetricsReservoirConfigImpl;
@@ -83,6 +84,8 @@
install(new SchemaModule());
install(new SshdModule());
+
+ bind(Boolean.class).annotatedWith(GerritIsReplica.class).toInstance(false);
}
static class CreateSchema implements LifecycleListener {
diff --git a/java/com/google/gerrit/httpd/init/WebAppInitializer.java b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
index 0e37684..c8dab81 100644
--- a/java/com/google/gerrit/httpd/init/WebAppInitializer.java
+++ b/java/com/google/gerrit/httpd/init/WebAppInitializer.java
@@ -362,7 +362,7 @@
new AbstractModule() {
@Override
protected void configure() {
- bind(GerritOptions.class).toInstance(new GerritOptions(false, false));
+ bind(GerritOptions.class).toInstance(GerritOptions.DEFAULT);
bind(GerritRuntime.class).toInstance(GerritRuntime.DAEMON);
}
});
diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java
index 574f68d..4a47e5ad 100644
--- a/java/com/google/gerrit/pgm/Daemon.java
+++ b/java/com/google/gerrit/pgm/Daemon.java
@@ -63,6 +63,7 @@
import com.google.gerrit.server.account.AccountCacheImpl;
import com.google.gerrit.server.account.AccountDeactivator.AccountDeactivatorModule;
import com.google.gerrit.server.account.InternalAccountDirectory.InternalAccountDirectoryModule;
+import com.google.gerrit.server.account.externalids.storage.notedb.ExternalIdCacheImpl;
import com.google.gerrit.server.account.externalids.storage.notedb.ExternalIdCaseSensitivityMigrator;
import com.google.gerrit.server.account.storage.notedb.AccountNoteDbReadStorageModule;
import com.google.gerrit.server.account.storage.notedb.AccountNoteDbWriteStorageModule;
@@ -92,12 +93,12 @@
import com.google.gerrit.server.git.ChangesByProjectCache;
import com.google.gerrit.server.git.GarbageCollectionModule;
import com.google.gerrit.server.git.WorkQueue.WorkQueueModule;
-import com.google.gerrit.server.group.PeriodicGroupIndexer.PeriodicGroupIndexerModule;
import com.google.gerrit.server.index.AbstractIndexModule;
import com.google.gerrit.server.index.IndexModule;
import com.google.gerrit.server.index.OnlineUpgrader.OnlineUpgraderModule;
import com.google.gerrit.server.index.VersionManager;
import com.google.gerrit.server.index.options.AutoFlush;
+import com.google.gerrit.server.index.scheduler.PeriodicIndexScheduler;
import com.google.gerrit.server.mail.EmailModule;
import com.google.gerrit.server.mail.SignedTokenEmailTokenVerifier.SignedTokenEmailTokenVerifierModule;
import com.google.gerrit.server.mail.receive.MailReceiver.MailReceiverModule;
@@ -384,11 +385,11 @@
@VisibleForTesting
public void start() throws IOException {
if (dbInjector == null) {
- dbInjector = createDbInjector(true /* enableMetrics */);
+ dbInjector =
+ createDbInjector(true /* enableMetrics */, new GerritOptions(headless, replica, devCdn));
}
cfgInjector = createCfgInjector();
config = cfgInjector.getInstance(Key.get(Config.class, GerritServerConfig.class));
- config.setBoolean("container", null, "replica", replica);
indexType = IndexModule.getIndexType(cfgInjector);
sysInjector = createSysInjector();
sysInjector.getInstance(PluginGuiceEnvironment.class).setDbCfgInjector(dbInjector, cfgInjector);
@@ -476,6 +477,8 @@
modules.add(new AccountNoteDbWriteStorageModule());
modules.add(new AccountNoteDbReadStorageModule());
+ modules.add(new ExternalIdCacheImpl.ExternalIdCacheModule());
+ modules.add(new ExternalIdCacheImpl.ExternalIdCacheBindingModule());
modules.add(new RepoSequenceModule());
modules.add(new FromAddressGeneratorProvider.UserAddressGenModule());
modules.add(new NoteDbDraftCommentsModule());
@@ -545,7 +548,6 @@
new AbstractModule() {
@Override
protected void configure() {
- bind(GerritOptions.class).toInstance(new GerritOptions(headless, replica, devCdn));
if (inMemoryTest) {
bind(String.class)
.annotatedWith(SecureStoreClassName.class)
@@ -555,9 +557,8 @@
}
});
modules.add(new GarbageCollectionModule());
- if (replica) {
- modules.add(new PeriodicGroupIndexerModule());
- } else {
+ modules.add(new PeriodicIndexScheduler.Module());
+ if (!replica) {
modules.add(new AccountDeactivatorModule());
modules.add(new AttentionSetOwnerAdderModule());
modules.add(new ChangeCleanupRunnerModule());
diff --git a/java/com/google/gerrit/pgm/Init.java b/java/com/google/gerrit/pgm/Init.java
index d3e9988..fc5a2c7 100644
--- a/java/com/google/gerrit/pgm/Init.java
+++ b/java/com/google/gerrit/pgm/Init.java
@@ -30,6 +30,7 @@
import com.google.gerrit.pgm.init.InitPlugins;
import com.google.gerrit.pgm.init.api.ConsoleUI;
import com.google.gerrit.pgm.util.ErrorLogFile;
+import com.google.gerrit.server.config.GerritOptions;
import com.google.gerrit.server.config.GerritServerConfigModule;
import com.google.gerrit.server.config.SitePath;
import com.google.gerrit.server.index.GerritIndexStatus;
@@ -159,6 +160,7 @@
bind(String.class)
.annotatedWith(SecureStoreClassName.class)
.toProvider(Providers.of(getConfiguredSecureStoreClass()));
+ bind(GerritOptions.class).toInstance(GerritOptions.DEFAULT);
}
});
modules.add(new GerritServerConfigModule());
diff --git a/java/com/google/gerrit/pgm/Reindex.java b/java/com/google/gerrit/pgm/Reindex.java
index d393a89..7424407 100644
--- a/java/com/google/gerrit/pgm/Reindex.java
+++ b/java/com/google/gerrit/pgm/Reindex.java
@@ -33,6 +33,7 @@
import com.google.gerrit.pgm.util.SiteProgram;
import com.google.gerrit.server.LibModuleLoader;
import com.google.gerrit.server.ModuleOverloader;
+import com.google.gerrit.server.account.externalids.storage.notedb.ExternalIdCacheImpl;
import com.google.gerrit.server.account.storage.notedb.AccountNoteDbReadStorageModule;
import com.google.gerrit.server.account.storage.notedb.AccountNoteDbWriteStorageModule;
import com.google.gerrit.server.cache.CacheDisplay;
@@ -243,6 +244,8 @@
});
modules.add(new AccountNoteDbWriteStorageModule());
modules.add(new AccountNoteDbReadStorageModule());
+ modules.add(new ExternalIdCacheImpl.ExternalIdCacheModule());
+ modules.add(new ExternalIdCacheImpl.ExternalIdCacheBindingModule());
modules.add(new RepoSequenceModule());
modules.add(new NoteDbDraftCommentsModule());
modules.add(new NoteDbStarredChangesModule());
diff --git a/java/com/google/gerrit/pgm/init/BaseInit.java b/java/com/google/gerrit/pgm/init/BaseInit.java
index 1f56512..e0eb773 100644
--- a/java/com/google/gerrit/pgm/init/BaseInit.java
+++ b/java/com/google/gerrit/pgm/init/BaseInit.java
@@ -36,6 +36,7 @@
import com.google.gerrit.pgm.init.index.IndexModuleOnInit;
import com.google.gerrit.pgm.init.index.lucene.LuceneIndexModuleOnInit;
import com.google.gerrit.pgm.util.SiteProgram;
+import com.google.gerrit.server.config.GerritOptions;
import com.google.gerrit.server.config.GerritServerConfigModule;
import com.google.gerrit.server.config.SitePath;
import com.google.gerrit.server.config.SitePaths;
@@ -275,6 +276,7 @@
bind(Boolean.class).annotatedWith(LibraryDownload.class).toInstance(skipAllDownloads());
bind(MetricMaker.class).to(DisabledMetricMaker.class);
+ bind(GerritOptions.class).toInstance(GerritOptions.DEFAULT);
}
});
diff --git a/java/com/google/gerrit/pgm/util/BatchProgramModule.java b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
index f45f1be..f30efd4 100644
--- a/java/com/google/gerrit/pgm/util/BatchProgramModule.java
+++ b/java/com/google/gerrit/pgm/util/BatchProgramModule.java
@@ -42,7 +42,6 @@
import com.google.gerrit.server.account.GroupIncludeCacheImpl;
import com.google.gerrit.server.account.Realm;
import com.google.gerrit.server.account.ServiceUserClassifierImpl;
-import com.google.gerrit.server.account.externalids.storage.notedb.ExternalIdCacheModule;
import com.google.gerrit.server.cache.CacheRemovalListener;
import com.google.gerrit.server.cache.h2.H2CacheModule;
import com.google.gerrit.server.cache.mem.DefaultMemoryCacheModule;
@@ -99,6 +98,7 @@
import com.google.gerrit.server.submitrequirement.predicate.DistinctVotersPredicate;
import com.google.gerrit.server.submitrequirement.predicate.FileEditsPredicate;
import com.google.gerrit.server.submitrequirement.predicate.HasSubmoduleUpdatePredicate;
+import com.google.gerrit.server.submitrequirement.predicate.SubmitRequirementLabelExtensionPredicate;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.inject.Injector;
import com.google.inject.Key;
@@ -181,7 +181,6 @@
modules.add(new DefaultPermissionBackendModule());
modules.add(new DefaultMemoryCacheModule());
modules.add(new H2CacheModule());
- modules.add(new ExternalIdCacheModule());
modules.add(new GroupModule());
modules.add(new NoteDbModule());
modules.add(AccountCacheImpl.module());
@@ -203,6 +202,7 @@
factory(ChangeData.AssistedFactory.class);
factory(ChangeIsVisibleToPredicate.Factory.class);
factory(DistinctVotersPredicate.Factory.class);
+ factory(SubmitRequirementLabelExtensionPredicate.Factory.class);
factory(HasSubmoduleUpdatePredicate.Factory.class);
factory(ProjectState.Factory.class);
diff --git a/java/com/google/gerrit/pgm/util/SiteProgram.java b/java/com/google/gerrit/pgm/util/SiteProgram.java
index aeaa1d6..ddf62fe 100644
--- a/java/com/google/gerrit/pgm/util/SiteProgram.java
+++ b/java/com/google/gerrit/pgm/util/SiteProgram.java
@@ -25,6 +25,7 @@
import com.google.gerrit.server.LibModuleLoader;
import com.google.gerrit.server.LibModuleType;
import com.google.gerrit.server.ModuleOverloader;
+import com.google.gerrit.server.config.GerritOptions;
import com.google.gerrit.server.config.GerritRuntime;
import com.google.gerrit.server.config.GerritServerConfigModule;
import com.google.gerrit.server.config.SitePath;
@@ -77,11 +78,11 @@
/** Provides database connectivity and site path. */
protected Injector createDbInjector() {
- return createDbInjector(false);
+ return createDbInjector(false, GerritOptions.DEFAULT);
}
/** Provides database connectivity and site path. */
- protected Injector createDbInjector(boolean enableMetrics) {
+ protected Injector createDbInjector(boolean enableMetrics, GerritOptions options) {
List<Module> modules = new ArrayList<>();
Module sitePathModule =
@@ -124,7 +125,15 @@
bind(GerritRuntime.class).toInstance(getGerritRuntime());
}
});
- Injector cfgInjector = Guice.createInjector(sitePathModule, configModule);
+ Module gerritOptionsModule =
+ new AbstractModule() {
+ @Override
+ protected void configure() {
+ bind(GerritOptions.class).toInstance(options);
+ }
+ };
+ modules.add(gerritOptionsModule);
+ Injector cfgInjector = Guice.createInjector(sitePathModule, configModule, gerritOptionsModule);
modules.add(new SchemaModule());
modules.add(cfgInjector.getInstance(GitRepositoryManagerModule.class));
diff --git a/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java b/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
index a23e7bc..886fe70 100644
--- a/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
+++ b/java/com/google/gerrit/server/account/externalids/ExternalIdCache.java
@@ -28,6 +28,12 @@
* cache is up to date.
*
* <p>All returned collections are unmodifiable.
+ *
+ * <p>NOTE: Modules which bind {@link ExternalIdCache} by using modules other than {@link
+ * com.google.gerrit.server.account.externalids.storage.notedb.ExternalIdCacheImpl.ExternalIdCacheBindingModule},
+ * should also provide an {@code Optional<}{@link
+ * com.google.gerrit.server.account.externalids.storage.notedb.ExternalIdCacheImpl}{@code >}
+ * binding.
*/
public interface ExternalIdCache {
Optional<ExternalId> byKey(ExternalId.Key key) throws IOException;
diff --git a/java/com/google/gerrit/server/account/externalids/storage/notedb/DisabledExternalIdCache.java b/java/com/google/gerrit/server/account/externalids/storage/notedb/DisabledExternalIdCache.java
index 8e53277..fd19fcc 100644
--- a/java/com/google/gerrit/server/account/externalids/storage/notedb/DisabledExternalIdCache.java
+++ b/java/com/google/gerrit/server/account/externalids/storage/notedb/DisabledExternalIdCache.java
@@ -21,6 +21,8 @@
import com.google.gerrit.server.account.externalids.ExternalIdCache;
import com.google.inject.AbstractModule;
import com.google.inject.Module;
+import com.google.inject.Provides;
+import com.google.inject.Singleton;
import java.io.IOException;
import java.util.Optional;
@@ -32,6 +34,12 @@
protected void configure() {
bind(ExternalIdCache.class).to(DisabledExternalIdCache.class);
}
+
+ @Provides
+ @Singleton
+ Optional<ExternalIdCacheImpl> provideNoteDbExternalIdCacheImpl() {
+ return Optional.empty();
+ }
};
}
diff --git a/java/com/google/gerrit/server/account/externalids/storage/notedb/ExternalIdCacheImpl.java b/java/com/google/gerrit/server/account/externalids/storage/notedb/ExternalIdCacheImpl.java
index dbfe205..20c94eb 100644
--- a/java/com/google/gerrit/server/account/externalids/storage/notedb/ExternalIdCacheImpl.java
+++ b/java/com/google/gerrit/server/account/externalids/storage/notedb/ExternalIdCacheImpl.java
@@ -14,27 +14,86 @@
package com.google.gerrit.server.account.externalids.storage.notedb;
+import static com.google.inject.Scopes.SINGLETON;
+
import com.google.common.cache.Cache;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSetMultimap;
import com.google.gerrit.entities.Account;
import com.google.gerrit.server.account.externalids.ExternalId;
import com.google.gerrit.server.account.externalids.ExternalIdCache;
+import com.google.gerrit.server.cache.CacheModule;
+import com.google.gerrit.server.cache.serialize.ObjectIdCacheSerializer;
+import com.google.inject.AbstractModule;
import com.google.inject.Inject;
+import com.google.inject.Provides;
import com.google.inject.Singleton;
+import com.google.inject.TypeLiteral;
import com.google.inject.name.Named;
import java.io.IOException;
+import java.time.Duration;
import java.util.Optional;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.ObjectId;
-/** Caches external IDs of all accounts. The external IDs are always loaded from NoteDb. */
-@Singleton
-class ExternalIdCacheImpl implements ExternalIdCache {
+/**
+ * Caches external IDs of all accounts. The external IDs are always loaded from NoteDb. *
+ *
+ * <p>This class should be bounded as a Singleton. However, due to internal limitations in Google,
+ * it cannot be marked as a singleton. The common installation pattern should therefore be:
+ *
+ * <pre>{@code
+ * * install(new ExternalIdCacheModule());
+ * * install(new ExternalIdCacheBindingModule());
+ * *
+ * }</pre>
+ */
+public class ExternalIdCacheImpl implements ExternalIdCache {
public static final String CACHE_NAME = "external_ids_map";
+ public static class ExternalIdCacheModule extends CacheModule {
+ @Override
+ protected void configure() {
+ persist(CACHE_NAME, ObjectId.class, new TypeLiteral<AllExternalIds>() {})
+ // The cached data is potentially pretty large and we are always only interested
+ // in the latest value. However, due to a race condition, it is possible for different
+ // threads to observe different values of the meta ref, and hence request different keys
+ // from the cache. Extend the cache size by 1 to cover this case, but expire the extra
+ // object after a short period of time, since it may be a potentially large amount of
+ // memory.
+ // When loading a new value because the primary data advanced, we want to leverage the old
+ // cache state to recompute only what changed. This doesn't affect cache size though as
+ // Guava calls the loader first and evicts later on.
+ .maximumWeight(2)
+ .expireFromMemoryAfterAccess(Duration.ofMinutes(1))
+ .diskLimit(-1)
+ .version(1)
+ .keySerializer(ObjectIdCacheSerializer.INSTANCE)
+ .valueSerializer(AllExternalIds.Serializer.INSTANCE);
+ }
+ }
+
+ public static class ExternalIdCacheBindingModule extends AbstractModule {
+ @Override
+ protected void configure() {
+ bind(ExternalIdCache.class).to(ExternalIdCacheImpl.class).in(SINGLETON);
+ }
+
+ /**
+ * Used by {@link ExternalIdsNoteDbImpl}. Modules which bind {@link ExternalIdCache} by using
+ * modules other than {@link ExternalIdCacheBindingModule}, should also provide an {@code
+ * Optional<ExternalIdCacheImpl>} binding.
+ */
+ @Provides
+ @Singleton
+ Optional<ExternalIdCacheImpl> provideNoteDbExternalIdCacheImpl(
+ ExternalIdCacheImpl externalIdCache) {
+ return Optional.of(externalIdCache);
+ }
+ }
+
private final Cache<ObjectId, AllExternalIds> extIdsByAccount;
private final ExternalIdReader externalIdReader;
private final ExternalIdCacheLoader externalIdCacheLoader;
diff --git a/java/com/google/gerrit/server/account/externalids/storage/notedb/ExternalIdCacheModule.java b/java/com/google/gerrit/server/account/externalids/storage/notedb/ExternalIdCacheModule.java
deleted file mode 100644
index aca0e1a..0000000
--- a/java/com/google/gerrit/server/account/externalids/storage/notedb/ExternalIdCacheModule.java
+++ /dev/null
@@ -1,47 +0,0 @@
-// Copyright (C) 2017 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.externalids.storage.notedb;
-
-import com.google.gerrit.server.account.externalids.ExternalIdCache;
-import com.google.gerrit.server.cache.CacheModule;
-import com.google.gerrit.server.cache.serialize.ObjectIdCacheSerializer;
-import com.google.inject.TypeLiteral;
-import java.time.Duration;
-import org.eclipse.jgit.lib.ObjectId;
-
-public class ExternalIdCacheModule extends CacheModule {
- @Override
- protected void configure() {
- persist(ExternalIdCacheImpl.CACHE_NAME, ObjectId.class, new TypeLiteral<AllExternalIds>() {})
- // The cached data is potentially pretty large and we are always only interested
- // in the latest value. However, due to a race condition, it is possible for different
- // threads to observe different values of the meta ref, and hence request different keys
- // from the cache. Extend the cache size by 1 to cover this case, but expire the extra
- // object after a short period of time, since it may be a potentially large amount of
- // memory.
- // When loading a new value because the primary data advanced, we want to leverage the old
- // cache state to recompute only what changed. This doesn't affect cache size though as
- // Guava calls the loader first and evicts later on.
- .maximumWeight(2)
- .expireFromMemoryAfterAccess(Duration.ofMinutes(1))
- .diskLimit(-1)
- .version(1)
- .keySerializer(ObjectIdCacheSerializer.INSTANCE)
- .valueSerializer(AllExternalIds.Serializer.INSTANCE);
-
- bind(ExternalIdCacheImpl.class);
- bind(ExternalIdCache.class).to(ExternalIdCacheImpl.class);
- }
-}
diff --git a/java/com/google/gerrit/server/account/externalids/storage/notedb/ExternalIdsNoteDbImpl.java b/java/com/google/gerrit/server/account/externalids/storage/notedb/ExternalIdsNoteDbImpl.java
index 7a2945c..4c26442 100644
--- a/java/com/google/gerrit/server/account/externalids/storage/notedb/ExternalIdsNoteDbImpl.java
+++ b/java/com/google/gerrit/server/account/externalids/storage/notedb/ExternalIdsNoteDbImpl.java
@@ -21,7 +21,6 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.server.account.externalids.ExternalId;
-import com.google.gerrit.server.account.externalids.ExternalIdCache;
import com.google.gerrit.server.account.externalids.ExternalIdKeyFactory;
import com.google.gerrit.server.account.externalids.ExternalIds;
import com.google.gerrit.server.config.AuthConfig;
@@ -47,21 +46,15 @@
@Inject
ExternalIdsNoteDbImpl(
ExternalIdReader externalIdReader,
- ExternalIdCache externalIdCache,
+ Optional<ExternalIdCacheImpl> externalIdCacheImpl,
ExternalIdKeyFactory externalIdKeyFactory,
AuthConfig authConfig) {
this.externalIdReader = externalIdReader;
- if (externalIdCache instanceof ExternalIdCacheImpl) {
- this.externalIdCache = (ExternalIdCacheImpl) externalIdCache;
- } else if (externalIdCache instanceof DisabledExternalIdCache) {
- // Supported case for testing only. Non of the disabled cache methods should be called, so
- // it's safe to not assign the var.
- this.externalIdCache = null;
- } else {
- throw new IllegalStateException(
- "The cache provided in ExternalIdsNoteDbImpl should be either ExternalIdCacheImpl or"
- + " DisabledExternalIdCache");
- }
+ this.externalIdCache =
+ externalIdCacheImpl.orElse(
+ // Supported case for tests or Google implementation. None of the disabled cache methods
+ // should be called from these flows, so it's safe to not assign the var.
+ null);
this.externalIdKeyFactory = externalIdKeyFactory;
this.authConfig = authConfig;
}
diff --git a/java/com/google/gerrit/server/config/GerritGlobalModule.java b/java/com/google/gerrit/server/config/GerritGlobalModule.java
index 2dbe45e..1f0bd6e 100644
--- a/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -109,7 +109,6 @@
import com.google.gerrit.server.account.ServiceUserClassifierImpl;
import com.google.gerrit.server.account.VersionedAuthorizedKeys;
import com.google.gerrit.server.account.externalids.ExternalIdModule;
-import com.google.gerrit.server.account.externalids.storage.notedb.ExternalIdCacheModule;
import com.google.gerrit.server.approval.ApprovalsUtil;
import com.google.gerrit.server.auth.AuthBackend;
import com.google.gerrit.server.auth.UniversalAuthBackend;
@@ -219,6 +218,7 @@
import com.google.gerrit.server.submitrequirement.predicate.DistinctVotersPredicate;
import com.google.gerrit.server.submitrequirement.predicate.FileEditsPredicate;
import com.google.gerrit.server.submitrequirement.predicate.HasSubmoduleUpdatePredicate;
+import com.google.gerrit.server.submitrequirement.predicate.SubmitRequirementLabelExtensionPredicate;
import com.google.gerrit.server.tools.ToolsCatalog;
import com.google.gerrit.server.update.BatchUpdate;
import com.google.gerrit.server.util.IdGenerator;
@@ -279,7 +279,6 @@
install(new AccessControlModule());
install(new AccountModule());
install(new CmdLineParserModule());
- install(new ExternalIdCacheModule());
install(new ExternalIdModule());
install(new GitModule());
install(new GroupDbModule());
@@ -303,6 +302,7 @@
factory(ChangeJson.AssistedFactory.class);
factory(ChangeIsVisibleToPredicate.Factory.class);
factory(DistinctVotersPredicate.Factory.class);
+ factory(SubmitRequirementLabelExtensionPredicate.Factory.class);
factory(HasSubmoduleUpdatePredicate.Factory.class);
factory(DeadlineChecker.Factory.class);
factory(EmailNewPatchSet.Factory.class);
diff --git a/java/com/google/gerrit/server/config/GerritIsReplicaProvider.java b/java/com/google/gerrit/server/config/GerritIsReplicaProvider.java
index f242a50..f53d718 100644
--- a/java/com/google/gerrit/server/config/GerritIsReplicaProvider.java
+++ b/java/com/google/gerrit/server/config/GerritIsReplicaProvider.java
@@ -26,14 +26,16 @@
*/
public final class GerritIsReplicaProvider implements Provider<Boolean> {
private final Config config;
+ private final boolean replicaOption;
@Inject
- public GerritIsReplicaProvider(@GerritServerConfig Config config) {
+ public GerritIsReplicaProvider(@GerritServerConfig Config config, GerritOptions opts) {
this.config = config;
+ this.replicaOption = opts.replica();
}
@Override
public Boolean get() {
- return ReplicaUtil.isReplica(config);
+ return replicaOption || ReplicaUtil.isReplica(config);
}
}
diff --git a/java/com/google/gerrit/server/config/GerritOptions.java b/java/com/google/gerrit/server/config/GerritOptions.java
index 0390620..30633a5 100644
--- a/java/com/google/gerrit/server/config/GerritOptions.java
+++ b/java/com/google/gerrit/server/config/GerritOptions.java
@@ -20,16 +20,18 @@
public class GerritOptions {
private final boolean headless;
- private final boolean slave;
+ private final boolean replica;
private final Optional<String> devCdn;
- public GerritOptions(boolean headless, boolean slave) {
- this(headless, slave, null);
+ public static GerritOptions DEFAULT = new GerritOptions(false, false);
+
+ public GerritOptions(boolean headless, boolean replica) {
+ this(headless, replica, null);
}
- public GerritOptions(boolean headless, boolean slave, @Nullable String devCdn) {
+ public GerritOptions(boolean headless, boolean replica, @Nullable String devCdn) {
this.headless = headless;
- this.slave = slave;
+ this.replica = replica;
this.devCdn = headless ? Optional.empty() : Optional.ofNullable(Strings.emptyToNull(devCdn));
}
@@ -37,8 +39,12 @@
return headless;
}
+ public boolean replica() {
+ return replica;
+ }
+
public boolean enableMasterFeatures() {
- return !slave;
+ return !replica;
}
public Optional<String> devCdn() {
diff --git a/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java b/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java
index 72e15ee..d466041 100644
--- a/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java
+++ b/java/com/google/gerrit/server/group/PeriodicGroupIndexer.java
@@ -21,20 +21,12 @@
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.GroupReference;
-import com.google.gerrit.extensions.events.LifecycleListener;
-import com.google.gerrit.lifecycle.LifecycleModule;
import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.config.ScheduleConfig;
-import com.google.gerrit.server.config.ScheduleConfig.Schedule;
import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.git.WorkQueue;
import com.google.gerrit.server.group.db.GroupNameNotes;
import com.google.gerrit.server.index.group.GroupIndexer;
import com.google.inject.Inject;
import com.google.inject.Provider;
-import java.util.concurrent.TimeUnit;
-import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.Repository;
/**
@@ -59,52 +51,6 @@
public class PeriodicGroupIndexer implements Runnable {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
- public static class PeriodicGroupIndexerModule extends LifecycleModule {
- @Override
- protected void configure() {
- listener().to(Lifecycle.class);
- }
- }
-
- private static class Lifecycle implements LifecycleListener {
- private final Config cfg;
- private final WorkQueue queue;
- private final PeriodicGroupIndexer runner;
-
- @Inject
- Lifecycle(@GerritServerConfig Config cfg, WorkQueue queue, PeriodicGroupIndexer runner) {
- this.cfg = cfg;
- this.queue = queue;
- this.runner = runner;
- }
-
- @Override
- public void start() {
- boolean runOnStartup = cfg.getBoolean("index", "scheduledIndexer", "runOnStartup", true);
- if (runOnStartup) {
- runner.run();
- }
-
- boolean isEnabled = cfg.getBoolean("index", "scheduledIndexer", "enabled", true);
- if (!isEnabled) {
- logger.atWarning().log("index.scheduledIndexer is disabled");
- return;
- }
-
- Schedule schedule =
- ScheduleConfig.builder(cfg, "index")
- .setSubsection("scheduledIndexer")
- .buildSchedule()
- .orElseGet(() -> Schedule.createOrFail(TimeUnit.MINUTES.toMillis(5), "00:00"));
- queue.scheduleAtFixedRate(runner, schedule);
- }
-
- @Override
- public void stop() {
- // handled by WorkQueue.stop() already
- }
- }
-
private final AllUsersName allUsersName;
private final GitRepositoryManager repoManager;
private final Provider<GroupIndexer> groupIndexerProvider;
diff --git a/java/com/google/gerrit/server/index/scheduler/PeriodicIndexScheduler.java b/java/com/google/gerrit/server/index/scheduler/PeriodicIndexScheduler.java
new file mode 100644
index 0000000..ab4b8fc
--- /dev/null
+++ b/java/com/google/gerrit/server/index/scheduler/PeriodicIndexScheduler.java
@@ -0,0 +1,102 @@
+// Copyright (C) 2024 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.index.scheduler;
+
+import com.google.common.flogger.FluentLogger;
+import com.google.gerrit.extensions.events.LifecycleListener;
+import com.google.gerrit.lifecycle.LifecycleModule;
+import com.google.gerrit.server.config.GerritIsReplica;
+import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.config.ScheduleConfig;
+import com.google.gerrit.server.config.ScheduleConfig.Schedule;
+import com.google.gerrit.server.git.WorkQueue;
+import com.google.gerrit.server.group.PeriodicGroupIndexer;
+import com.google.inject.Inject;
+import java.util.Set;
+import java.util.concurrent.TimeUnit;
+import org.eclipse.jgit.lib.Config;
+
+public class PeriodicIndexScheduler implements LifecycleListener {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ public static class Module extends LifecycleModule {
+ @Override
+ protected void configure() {
+ listener().to(PeriodicIndexScheduler.class);
+ }
+ }
+
+ private final Config cfg;
+ private final WorkQueue queue;
+ private final PeriodicGroupIndexer groupIndexer;
+ private final boolean isReplica;
+
+ @Inject
+ PeriodicIndexScheduler(
+ @GerritServerConfig Config cfg,
+ WorkQueue queue,
+ PeriodicGroupIndexer groupIndexer,
+ @GerritIsReplica boolean isReplica) {
+ this.cfg = cfg;
+ this.queue = queue;
+ this.groupIndexer = groupIndexer;
+ this.isReplica = isReplica;
+ }
+
+ @Override
+ public void start() {
+ Subsection s = determineConfigSubsection();
+ boolean runOnStartup = cfg.getBoolean(s.section, s.subsection, "runOnStartup", isReplica);
+ if (runOnStartup) {
+ groupIndexer.run();
+ }
+
+ boolean isEnabled = cfg.getBoolean(s.section, s.subsection, "enabled", isReplica);
+ if (!isEnabled) {
+ logger.atWarning().log("index.scheduledIndexer is disabled");
+ return;
+ }
+
+ Schedule schedule =
+ ScheduleConfig.builder(cfg, s.section)
+ .setSubsection(s.subsection)
+ .buildSchedule()
+ .orElseGet(() -> Schedule.createOrFail(TimeUnit.MINUTES.toMillis(5), "00:00"));
+ queue.scheduleAtFixedRate(groupIndexer, schedule);
+ }
+
+ private Subsection determineConfigSubsection() {
+ Set<String> scheduledIndexerConfig = cfg.getSubsections("scheduledIndexer");
+ if (scheduledIndexerConfig.contains("groups")) {
+ return new Subsection("scheduledIndexer", "groups");
+ }
+ return new Subsection("index", "scheduledIndexer");
+ }
+
+ private static class Subsection {
+ final String section;
+ final String subsection;
+
+ Subsection(String section, String subsection) {
+ this.section = section;
+ this.subsection = subsection;
+ }
+ }
+
+ @Override
+ public void stop() {
+ // handled by WorkQueue.stop() already
+ }
+}
diff --git a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
index d598739..9b85582 100644
--- a/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
@@ -236,6 +236,7 @@
public static final String ARG_ID_NON_UPLOADER = "non_uploader";
public static final String ARG_ID_NON_CONTRIBUTOR = "non_contributor";
public static final String ARG_COUNT = "count";
+ public static final String ARG_USERS = "users";
public static final Account.Id OWNER_ACCOUNT_ID = Account.id(0);
public static final Account.Id NON_UPLOADER_ACCOUNT_ID = Account.id(-1);
public static final Account.Id NON_CONTRIBUTOR_ACCOUNT_ID = Account.id(-2);
@@ -1108,6 +1109,9 @@
"count=%d is not allowed. Maximum allowed value for count is %d.",
count, LabelPredicate.MAX_COUNT));
}
+ } else if (key.equalsIgnoreCase(ARG_USERS)) {
+ throw new QueryParseException(
+ String.format("Cannot use the '%s' argument in search", ARG_USERS));
} else {
throw new QueryParseException("Invalid argument identifier '" + pair.getKey() + "'");
}
diff --git a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
index cb92ddd..55d3505 100644
--- a/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
+++ b/java/com/google/gerrit/server/query/change/SubmitRequirementChangeQueryBuilder.java
@@ -28,13 +28,16 @@
import com.google.gerrit.server.submitrequirement.predicate.RegexAuthorEmailPredicate;
import com.google.gerrit.server.submitrequirement.predicate.RegexCommitterEmailPredicate;
import com.google.gerrit.server.submitrequirement.predicate.RegexUploaderEmailPredicateFactory;
+import com.google.gerrit.server.submitrequirement.predicate.SubmitRequirementLabelExtensionPredicate;
import com.google.inject.Inject;
+import java.io.IOException;
import java.util.List;
import java.util.Locale;
import java.util.Set;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
+import org.eclipse.jgit.errors.ConfigInvalidException;
/**
* A query builder for submit requirement expressions that includes all {@link ChangeQueryBuilder}
@@ -48,6 +51,8 @@
new QueryBuilder.Definition<>(SubmitRequirementChangeQueryBuilder.class);
private final DistinctVotersPredicate.Factory distinctVotersPredicateFactory;
+ private final SubmitRequirementLabelExtensionPredicate.Factory
+ submitRequirementLabelExtensionPredicateFactory;
private final HasSubmoduleUpdatePredicate.Factory hasSubmoduleUpdateFactory;
/**
@@ -70,11 +75,15 @@
SubmitRequirementChangeQueryBuilder(
Arguments args,
DistinctVotersPredicate.Factory distinctVotersPredicateFactory,
+ SubmitRequirementLabelExtensionPredicate.Factory
+ submitRequirementLabelExtensionPredicateFactory,
FileEditsPredicate.Factory fileEditsPredicateFactory,
HasSubmoduleUpdatePredicate.Factory hasSubmoduleUpdateFactory,
RegexUploaderEmailPredicateFactory regexUploaderEmailPredicateFactory) {
super(def, args);
this.distinctVotersPredicateFactory = distinctVotersPredicateFactory;
+ this.submitRequirementLabelExtensionPredicateFactory =
+ submitRequirementLabelExtensionPredicateFactory;
this.fileEditsPredicateFactory = fileEditsPredicateFactory;
this.hasSubmoduleUpdateFactory = hasSubmoduleUpdateFactory;
this.regexUploaderEmailPredicateFactory = regexUploaderEmailPredicateFactory;
@@ -150,6 +159,16 @@
return distinctVotersPredicateFactory.create(value);
}
+ @Override
+ public Predicate<ChangeData> label(String value)
+ throws QueryParseException, IOException, ConfigInvalidException {
+ if (SubmitRequirementLabelExtensionPredicate.matches(value)) {
+ return submitRequirementLabelExtensionPredicateFactory.create(value);
+ }
+ SubmitRequirementLabelExtensionPredicate.validateIfNoMatch(value);
+ return super.label(value);
+ }
+
/**
* A SR operator that can match with file path and content pattern. The value should be of the
* form:
diff --git a/java/com/google/gerrit/server/submitrequirement/predicate/SubmitRequirementLabelExtensionPredicate.java b/java/com/google/gerrit/server/submitrequirement/predicate/SubmitRequirementLabelExtensionPredicate.java
new file mode 100644
index 0000000..389c7f4
--- /dev/null
+++ b/java/com/google/gerrit/server/submitrequirement/predicate/SubmitRequirementLabelExtensionPredicate.java
@@ -0,0 +1,213 @@
+// Copyright (C) 2024 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.submitrequirement.predicate;
+
+import static com.google.common.collect.ImmutableSet.toImmutableSet;
+
+import com.google.common.base.Enums;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.flogger.FluentLogger;
+import com.google.common.primitives.Ints;
+import com.google.errorprone.annotations.CanIgnoreReturnValue;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.SubmitRecord;
+import com.google.gerrit.index.query.QueryParseException;
+import com.google.gerrit.server.account.ServiceUserClassifier;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
+import com.google.gerrit.server.query.change.ChangeData;
+import com.google.gerrit.server.query.change.ChangeQueryBuilder.Arguments;
+import com.google.gerrit.server.query.change.LabelPredicate;
+import com.google.gerrit.server.query.change.SubmitRequirementPredicate;
+import com.google.inject.Inject;
+import com.google.inject.assistedinject.Assisted;
+import java.util.Locale;
+import java.util.Optional;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Extensions of the {@link LabelPredicate} that are only available for submit requirement
+ * expressions, but not for search.
+ *
+ * <p>Supported extensions:
+ *
+ * <ul>
+ * <li>"users=human_reviewers" arg, e.g. "label:Code-Review=MAX,users=human_reviewers" matches
+ * changes where all human reviewers have approved the change with Code-Review=MAX
+ * </ul>
+ */
+public class SubmitRequirementLabelExtensionPredicate extends SubmitRequirementPredicate {
+ private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+
+ public interface Factory {
+ SubmitRequirementLabelExtensionPredicate create(String value) throws QueryParseException;
+ }
+
+ private static final Pattern PATTERN = Pattern.compile("(?<label>[^,]*),users=human_reviewers$");
+ private static final Pattern PATTERN_LABEL =
+ Pattern.compile("(?<label>[^,<>=]*)(?<op>=|<=|>=|<|>)(?<value>[^,]*)");
+
+ public static boolean matches(String value) {
+ return PATTERN.matcher(value).matches();
+ }
+
+ public static void validateIfNoMatch(String value) throws QueryParseException {
+ if (value.contains(",users=")) {
+ throw new QueryParseException(
+ "Cannot use the 'users' argument in conjunction with other arguments ('count', 'user',"
+ + " group')");
+ }
+ }
+
+ private final Arguments args;
+ private final ServiceUserClassifier serviceUserClassifier;
+ private final String label;
+
+ @Inject
+ SubmitRequirementLabelExtensionPredicate(
+ Arguments args, ServiceUserClassifier serviceUserClassifier, @Assisted String value)
+ throws QueryParseException {
+ super("label", value);
+ this.args = args;
+ this.serviceUserClassifier = serviceUserClassifier;
+
+ Matcher m = PATTERN.matcher(value);
+ if (!m.matches()) {
+ throw new QueryParseException(
+ String.format("invalid value for '%s': %s", getOperator(), value));
+ }
+ this.label = validateLabel(m.group("label"));
+ }
+
+ @CanIgnoreReturnValue
+ private String validateLabel(String label) throws QueryParseException {
+ int eq = label.indexOf('=');
+
+ if (eq <= 0) {
+ return label;
+ }
+
+ String statusName = label.substring(eq + 1).toUpperCase(Locale.US);
+ SubmitRecord.Label.Status status =
+ Enums.getIfPresent(SubmitRecord.Label.Status.class, statusName).orNull();
+ if (status != null) {
+ // We would need to use SubmitRecordPredicate but can't because it doesn't implement
+ // Matchable.
+ throw new QueryParseException(
+ "Cannot use the 'users=human_reviewers' argument in conjunction with a submit record"
+ + " label status");
+ }
+ return label;
+ }
+
+ @Override
+ public boolean match(ChangeData cd) {
+ if (!cd.reviewersByEmail().byState(ReviewerStateInternal.REVIEWER).isEmpty()
+ && !matchZeroVotes(label)) {
+ // Reviewers by email are reviewers that don't have a Gerrit account. Without Gerrit
+ // account they cannot vote on the change, which means changes that have any such
+ // reviewers never match when we expect a vote != 0 from all reviewers.
+ logger.atFine().log(
+ "change %s doesn't match since there are reviewers by email"
+ + " (that don't have a matching approval): %s",
+ cd.change().getChangeId(), cd.reviewersByEmail().byState(ReviewerStateInternal.REVIEWER));
+ return false;
+ }
+
+ ImmutableSet<Account.Id> humanReviewers =
+ cd.reviewers().byState(ReviewerStateInternal.REVIEWER).stream()
+ // Ignore the change owner (if the change owner voted on their own change they are
+ // technically a reviewer).
+ .filter(accountId -> !accountId.equals(cd.change().getOwner()))
+ // Ignore reviewers that are service users.
+ .filter(accountId -> !serviceUserClassifier.isServiceUser(accountId))
+ .collect(toImmutableSet());
+
+ if (humanReviewers.isEmpty()) {
+ // a review from human reviewers is required, but no human reviewers are present
+ return false;
+ }
+
+ for (Account.Id reviewer : humanReviewers) {
+ if (!new LabelPredicate(
+ args,
+ label,
+ ImmutableSet.of(reviewer),
+ /* group= */ null,
+ /* count= */ null,
+ /* countOp= */ null)
+ .match(cd)) {
+ logger.atFine().log(
+ "change %s doesn't match because it misses matching approvals from: %s",
+ cd.change().getChangeId(), reviewer);
+ return false;
+ }
+ }
+
+ logger.atFine().log(
+ "change %s matches because it has matching approvals from all human reviewers: %s",
+ cd.change().getChangeId(), humanReviewers);
+ return true;
+ }
+
+ private boolean matchZeroVotes(String label) {
+ Matcher m = PATTERN_LABEL.matcher(label);
+ if (!m.matches()) {
+ return false;
+ }
+
+ String op = m.group("op");
+ String value = m.group("value");
+
+ Optional<Integer> intValue = Optional.ofNullable(Ints.tryParse(value));
+
+ if (op.equals("=") && (intValue.isPresent() && intValue.get() == 0)) {
+ return true;
+ } else if (op.equals("<=")) {
+ if (intValue.isPresent() && intValue.get() >= 0) {
+ return true;
+ } else if (value.equals("MAX")) {
+ return true;
+ }
+ return false;
+ } else if (op.equals("<")) {
+ if (intValue.isPresent() && intValue.get() > 0) {
+ return true;
+ } else if (value.equals("MAX")) {
+ return true;
+ }
+ } else if (op.equals(">=")) {
+ if (intValue.isPresent() && intValue.get() <= 0) {
+ return true;
+ } else if (value.equals("MIN")) {
+ return true;
+ }
+ return false;
+ } else if (op.equals(">")) {
+ if (intValue.isPresent() && intValue.get() < 0) {
+ return true;
+ } else if (value.equals("MIN")) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ @Override
+ public int getCost() {
+ return 1;
+ }
+}
diff --git a/java/com/google/gerrit/testing/InMemoryModule.java b/java/com/google/gerrit/testing/InMemoryModule.java
index c1192f6..789655f 100644
--- a/java/com/google/gerrit/testing/InMemoryModule.java
+++ b/java/com/google/gerrit/testing/InMemoryModule.java
@@ -49,6 +49,7 @@
import com.google.gerrit.server.Sequence.LightweightGroups;
import com.google.gerrit.server.account.AccountCacheImpl;
import com.google.gerrit.server.account.GroupBackend;
+import com.google.gerrit.server.account.externalids.storage.notedb.ExternalIdCacheImpl;
import com.google.gerrit.server.account.storage.notedb.AccountNoteDbReadStorageModule;
import com.google.gerrit.server.account.storage.notedb.AccountNoteDbWriteStorageModule;
import com.google.gerrit.server.api.GerritApiModule;
@@ -208,6 +209,8 @@
install(cfgInjector.getInstance(GerritGlobalModule.class));
install(new AccountNoteDbWriteStorageModule());
install(new AccountNoteDbReadStorageModule());
+ install(new ExternalIdCacheImpl.ExternalIdCacheModule());
+ install(new ExternalIdCacheImpl.ExternalIdCacheBindingModule());
install(new RepoSequenceModule());
install(new FromAddressGeneratorProvider.UserAddressGenModule());
install(new NoteDbDraftCommentsModule());
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
index a5d86ce..d7d2f26 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementIT.java
@@ -637,6 +637,252 @@
}
@Test
+ public void submitRequirement_wantCodeReviewFromHumanReviewers() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/*").group(REGISTERED_USERS).range(-2, 2))
+ .update();
+
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Code-Review")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Want-Code-Review-From-All")
+ .setApplicabilityExpression(
+ Optional.of(
+ SubmitRequirementExpression.create(
+ "-label:Code-Review>=1,users=human_reviewers")))
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review>=1,users=human_reviewers"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ // Code-Review is unsatisfied because there is no Code-Review+2 approval.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Code-Review",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ // Want-Code-Review-From-All is unsatisfied (since a review from reviewers is required but no
+ // reviewer is present on the change).
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ // Add some reviewers
+ TestAccount reviewer1 = accountCreator.create("reviewer1");
+ gApi.changes().id(changeId).addReviewer("reviewer1");
+ TestAccount reviewer2 = accountCreator.create("reviewer2");
+ gApi.changes().id(changeId).addReviewer("reviewer2");
+
+ // Code-Review is unsatisfied because there is no Code-Review+2 approval.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Code-Review",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ // Want-Code-Review-From-All is unsatisfied since there are reviewers on the change that
+ // didn't approve it yet.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ requestScopeOperations.setApiUser(reviewer1.id());
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Code-Review is satisfied because there is Code-Review+2 approval from reviewer1.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Code-Review",
+ Status.SATISFIED,
+ /* isLegacy= */ false);
+
+ // Want-Code-Review-From-All is unsatisfied since there is no approval from reviewer2.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ requestScopeOperations.setApiUser(reviewer2.id());
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Code-Review is satisfied because there are Code-Review+2 approvals from reviewer1 and
+ // reviewer2.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Code-Review",
+ Status.SATISFIED,
+ /* isLegacy= */ false);
+
+ // Want-Code-Review-From-All is not applicable since there are approval from all reviewers
+ // (reviewer1 and reviewer2) which makes "label:Code-Review=MAX,users=human_reviewers"
+ // satisfied.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.NOT_APPLICABLE,
+ /* isLegacy= */ false);
+
+ // Add another reviewer
+ TestAccount reviewer3 = accountCreator.create("reviewer3");
+ gApi.changes().id(changeId).addReviewer("reviewer3");
+
+ // Want-Code-Review-From-All is unsatisfied because reviewer3 didn't vote yet.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ // Vote with Code-Review+1 by reviewer3.
+ requestScopeOperations.setApiUser(reviewer3.id());
+ voteLabel(changeId, "Code-Review", 1);
+
+ // Want-Code-Review-From-All is not applicable because all reviewers voted with Code-Review >=
+ // 1.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.NOT_APPLICABLE,
+ /* isLegacy= */ false);
+ }
+
+ @Test
+ public void submitRequirement_wantCodeReviewFromHumanReviewers_enabledByFooter()
+ throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/*").group(REGISTERED_USERS).range(-2, 2))
+ .update();
+
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Code-Review")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review=MAX"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("Want-Code-Review-From-All")
+ .setApplicabilityExpression(
+ Optional.of(
+ SubmitRequirementExpression.create(
+ "footer:\"Want-Code-Review: all\" -label:Code-Review>=1,users=human_reviewers")))
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:Code-Review>=1,users=human_reviewers"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
+
+ PushOneCommit.Result r = createChange();
+ String changeId = r.getChangeId();
+
+ // Add some reviewers
+ TestAccount reviewer1 = accountCreator.create("reviewer1");
+ gApi.changes().id(changeId).addReviewer("reviewer1");
+ TestAccount reviewer2 = accountCreator.create("reviewer2");
+ gApi.changes().id(changeId).addReviewer("reviewer2");
+
+ // Approve by one reviewer
+ requestScopeOperations.setApiUser(reviewer1.id());
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Code-Review is satisfied because there is Code-Review+2 approval from reviewer1.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Code-Review",
+ Status.SATISFIED,
+ /* isLegacy= */ false);
+
+ // Want-Code-Review-From-All is not applicable since the commit message doesn't contain a
+ // "Want-Code-Review: all" footer.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.NOT_APPLICABLE,
+ /* isLegacy= */ false);
+
+ // Amend the change to add a "Want-Code-Review: all" footer.
+ amendChange(
+ changeId,
+ PushOneCommit.SUBJECT
+ + "\n\nSome Description\n\nChange-Id: "
+ + changeId
+ + "\nWant-Code-Review: all\n",
+ PushOneCommit.FILE_NAME,
+ "content");
+
+ // Re-Approve by reviewer1.
+ requestScopeOperations.setApiUser(reviewer1.id());
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Want-Code-Review-From-All is applicable since there is a "Want-Code-Review: all" footer and
+ // it is unsatisfied since there is no approval from reviewer2.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ // Approve by reviewer2.
+ requestScopeOperations.setApiUser(reviewer2.id());
+ voteLabel(changeId, "Code-Review", 2);
+
+ // Want-Code-Review-From-All is not applicable since there are approval from all reviewers
+ // (reviewer1 and reviewer2) which makes "label:Code-Review=MAX,users=human_reviewers"
+ // satisfied.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.NOT_APPLICABLE,
+ /* isLegacy= */ false);
+
+ // Add another reviewer
+ TestAccount reviewer3 = accountCreator.create("reviewer3");
+ gApi.changes().id(changeId).addReviewer("reviewer3");
+
+ // Want-Code-Review-From-All is unsatisfied because reviewer3 didn't vote yet.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.UNSATISFIED,
+ /* isLegacy= */ false);
+
+ // Vote with Code-Review+1 by reviewer3.
+ requestScopeOperations.setApiUser(reviewer3.id());
+ voteLabel(changeId, "Code-Review", 1);
+
+ // Want-Code-Review-From-All is not applicable because all reviewers voted with Code-Review >=
+ // 1.
+ assertSubmitRequirementStatus(
+ gApi.changes().id(changeId).get().submitRequirements,
+ "Want-Code-Review-From-All",
+ Status.NOT_APPLICABLE,
+ /* isLegacy= */ false);
+ }
+
+ @Test
public void submitRequirementIsSatisfied_whenSubmittabilityExpressionIsFulfilled()
throws Exception {
configSubmitRequirement(
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
index 8643489..61a06a3 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitRequirementPredicateIT.java
@@ -15,6 +15,7 @@
package com.google.gerrit.acceptance.api.change;
import static com.google.common.truth.Truth.assertThat;
+import static com.google.common.truth.Truth.assertWithMessage;
import static com.google.gerrit.acceptance.testsuite.project.TestProjectUpdate.allowLabel;
import static com.google.gerrit.server.group.SystemGroupBackend.ANONYMOUS_USERS;
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
@@ -39,13 +40,19 @@
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.AccountGroup;
+import com.google.gerrit.entities.BooleanProjectConfig;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelType;
+import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.entities.SubmitRequirementExpression;
import com.google.gerrit.entities.SubmitRequirementExpressionResult;
import com.google.gerrit.extensions.api.changes.ReviewInput;
+import com.google.gerrit.extensions.client.InheritableBoolean;
import com.google.gerrit.extensions.common.ChangeInfo;
+import com.google.gerrit.server.account.ServiceUserClassifier;
+import com.google.gerrit.server.git.meta.MetaDataUpdate;
+import com.google.gerrit.server.project.ProjectConfig;
import com.google.gerrit.server.project.SubmitRequirementsEvaluatorImpl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
@@ -467,6 +474,373 @@
assertMatching("label:Code-Review=+2,user=non_contributor", r1.getChange().getId());
}
+ @Test
+ public void label_requireVoteFromHumanReviewers() throws Exception {
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allowLabel("Code-Review").ref("refs/*").group(REGISTERED_USERS).range(-2, 2))
+ .update();
+
+ Account.Id owner = accountCreator.create("owner").id();
+ Account.Id reviewer1 = accountCreator.create("reviewer1").id();
+ Account.Id reviewer2 = accountCreator.create("reviewer2").id();
+ Account.Id reviewer3 = accountCreator.create("reviewer3").id();
+
+ Account.Id serviceUser = accountCreator.create("serviceUser").id();
+ gApi.groups().id(ServiceUserClassifier.SERVICE_USERS).addMembers(serviceUser.toString());
+
+ Change.Id changeApprovedByAllReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(project, changeApprovedByAllReviewers, reviewer1, reviewer2, reviewer3);
+ addReviews(
+ project,
+ changeApprovedByAllReviewers,
+ ReviewInput.approve(),
+ reviewer1,
+ reviewer2,
+ reviewer3);
+
+ Change.Id changeApprovedBySomeReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(project, changeApprovedBySomeReviewers, reviewer1, reviewer2, reviewer3);
+ addReviews(project, changeApprovedBySomeReviewers, ReviewInput.approve(), reviewer1, reviewer2);
+
+ Change.Id changeRecommendedByAllReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(project, changeRecommendedByAllReviewers, reviewer1, reviewer2, reviewer3);
+ addReviews(
+ project,
+ changeRecommendedByAllReviewers,
+ ReviewInput.recommend(),
+ reviewer1,
+ reviewer2,
+ reviewer3);
+
+ Change.Id changeRecommendedBySomeReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(project, changeRecommendedBySomeReviewers, reviewer1, reviewer2, reviewer3);
+ addReviews(
+ project, changeRecommendedBySomeReviewers, ReviewInput.recommend(), reviewer1, reviewer2);
+
+ Change.Id changeNoVotesByReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(project, changeNoVotesByReviewers, reviewer1, reviewer2, reviewer3);
+
+ Change.Id changeWithoutReviewers =
+ changeOperations.newChange().project(project).owner(owner).create();
+
+ requestScopeOperations.setApiUser(user.id());
+
+ // change without reviewers doesn't match
+ assertNotMatching("label:Code-Review=MAX,users=human_reviewers", changeWithoutReviewers);
+
+ // match changes where all reviewers have the same vote
+ assertRequirement(
+ "label:Code-Review=MAX,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+ assertRequirement(
+ "label:Code-Review=2,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+ assertRequirement(
+ "label:Code-Review=1,users=human_reviewers",
+ ImmutableList.of(changeRecommendedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeApprovedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // match changes where no reviewer voted (same as "label:Code-Review=0")
+ assertRequirement(
+ "label:Code-Review=0,users=human_reviewers",
+ ImmutableList.of(changeNoVotesByReviewers),
+ ImmutableList.of(
+ changeApprovedByAllReviewers,
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers));
+
+ // match changes where all reviewers have a vote <=, >=, < or >
+ assertRequirement(
+ "label:Code-Review<=2,users=human_reviewers",
+ ImmutableList.of(
+ changeApprovedByAllReviewers,
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers),
+ ImmutableList.of());
+ assertRequirement(
+ "label:Code-Review<=1,users=human_reviewers",
+ ImmutableList.of(
+ changeRecommendedByAllReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers),
+ ImmutableList.of(changeApprovedByAllReviewers));
+ assertRequirement(
+ "label:Code-Review>=1,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers, changeRecommendedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+ assertRequirement(
+ "label:Code-Review<1,users=human_reviewers",
+ ImmutableList.of(changeNoVotesByReviewers),
+ ImmutableList.of(
+ changeApprovedByAllReviewers,
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers));
+ assertRequirement(
+ "label:Code-Review>1,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // match changes where all reviewers have any (non-zero) vote
+ assertRequirement(
+ "label:Code-Review=ANY,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers, changeRecommendedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // votes of the change owners are ignored (as the change owner is not considered as a reviewer)
+ addReviews(project, changeApprovedByAllReviewers, ReviewInput.dislike(), owner);
+ assertRequirement(
+ "label:Code-Review=MAX,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // missing votes from service users are fine
+ addReviewers(project, changeApprovedByAllReviewers, serviceUser);
+ assertRequirement(
+ "label:Code-Review=MAX,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // votes from service users are ignored
+ addReviews(project, changeApprovedByAllReviewers, ReviewInput.dislike(), serviceUser);
+ assertRequirement(
+ "label:Code-Review=MAX,users=human_reviewers",
+ ImmutableList.of(changeApprovedByAllReviewers),
+ ImmutableList.of(
+ changeApprovedBySomeReviewers,
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+
+ // when reviewers by email are present changes do not match, unless the expected value is 0
+ try (MetaDataUpdate md = metaDataUpdateFactory.create(project)) {
+ ProjectConfig cfg = projectConfigFactory.create(project);
+ cfg.load(md);
+ cfg.updateProject(
+ update ->
+ update.setBooleanConfig(
+ BooleanProjectConfig.ENABLE_REVIEWER_BY_EMAIL, InheritableBoolean.TRUE));
+ cfg.commit(md);
+ }
+ projectCache.evictAndReindex(project);
+ Change.Id changeRecommendedByAllReviewersWithReviewersByEmail =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(
+ project,
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ reviewer1,
+ reviewer2,
+ reviewer3);
+ addReviews(
+ project,
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ ReviewInput.recommend(),
+ reviewer1,
+ reviewer2,
+ reviewer3);
+ addReviewer(
+ project,
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ "email-without-account@example.com");
+ Change.Id changeNoVotesByReviewersWithReviewersByEmail =
+ changeOperations.newChange().project(project).owner(owner).create();
+ addReviewers(
+ project, changeNoVotesByReviewersWithReviewersByEmail, reviewer1, reviewer2, reviewer3);
+ addReviewer(
+ project, changeNoVotesByReviewersWithReviewersByEmail, "email-without-account@example.com");
+ assertRequirement(
+ "label:Code-Review=MAX,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review=2,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review=ANY,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review=0,users=human_reviewers",
+ ImmutableList.of(changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of(changeRecommendedByAllReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review<=2,users=human_reviewers",
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of());
+ assertRequirement(
+ "label:Code-Review<=0,users=human_reviewers",
+ ImmutableList.of(changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of(changeRecommendedByAllReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review<=-1,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review<2,users=human_reviewers",
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of());
+ assertRequirement(
+ "label:Code-Review<1,users=human_reviewers",
+ ImmutableList.of(changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of(changeRecommendedByAllReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review<0,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review>=0,users=human_reviewers",
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of());
+ assertRequirement(
+ "label:Code-Review>=1,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+ assertRequirement(
+ "label:Code-Review>-1,users=human_reviewers",
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail),
+ ImmutableList.of());
+ assertRequirement(
+ "label:Code-Review>0,users=human_reviewers",
+ ImmutableList.of(),
+ ImmutableList.of(
+ changeRecommendedByAllReviewersWithReviewersByEmail,
+ changeNoVotesByReviewersWithReviewersByEmail));
+
+ // cannot combine users=human_reviewers" with submit record status
+ assertError(
+ "label:Code-Review=ok,users=human_reviewers",
+ changeApprovedByAllReviewers,
+ "Cannot use the 'users=human_reviewers' argument in conjunction with a submit record label"
+ + " status");
+
+ // cannot combine "users" arg with a "user" arg
+ assertError(
+ "label:Code-Review=MAX,users=human_reviewers,user=reviewer1",
+ changeApprovedByAllReviewers,
+ "Cannot use the 'users' argument in conjunction with other arguments ('count', 'user',"
+ + " group')");
+
+ // cannot combine "users" arg with a "group" arg
+ assertError(
+ "label:Code-Review=MAX,users=human_reviewers,group=foo",
+ changeApprovedByAllReviewers,
+ "Cannot use the 'users' argument in conjunction with other arguments ('count', 'user',"
+ + " group')");
+
+ // cannot combine "users" arg with a positional arg
+ assertError(
+ "label:Code-Review=MAX,users=human_reviewers,reviewer1",
+ changeApprovedByAllReviewers,
+ "Cannot use the 'users' argument in conjunction with other arguments ('count', 'user',"
+ + " group')");
+ assertError(
+ "label:Code-Review=MAX,reviewer1,users=human_reviewers",
+ changeApprovedByAllReviewers,
+ "Cannot use the 'users' argument in conjunction with other arguments ('count', 'user',"
+ + " group')");
+
+ // label without "users=human_reviewers" still works
+ assertRequirement(
+ "label:Code-Review=MAX,user=reviewer1",
+ ImmutableList.of(changeApprovedByAllReviewers, changeApprovedBySomeReviewers),
+ ImmutableList.of(
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+ assertRequirement(
+ "label:Code-Review=MAX,reviewer1",
+ ImmutableList.of(changeApprovedByAllReviewers, changeApprovedBySomeReviewers),
+ ImmutableList.of(
+ changeRecommendedByAllReviewers,
+ changeRecommendedBySomeReviewers,
+ changeNoVotesByReviewers));
+ }
+
+ private void addReviewers(Project.NameKey project, Change.Id changeId, Account.Id... reviewers)
+ throws Exception {
+ for (Account.Id reviewer : reviewers) {
+ addReviewer(project, changeId, reviewer.toString());
+ }
+ }
+
+ private void addReviewer(Project.NameKey project, Change.Id changeId, String reviewer)
+ throws Exception {
+ gApi.changes().id(project.get(), changeId.get()).addReviewer(reviewer);
+ }
+
+ private void addReviews(
+ Project.NameKey project, Change.Id changeId, ReviewInput reviewInput, Account.Id... reviewers)
+ throws Exception {
+ for (Account.Id reviewer : reviewers) {
+ requestScopeOperations.setApiUser(reviewer);
+ gApi.changes().id(project.get(), changeId.get()).current().review(reviewInput);
+ }
+ }
+
private void approveAsUser(String changeId, Account.Id userId) throws Exception {
requestScopeOperations.setApiUser(userId);
approve(changeId);
@@ -540,13 +914,28 @@
return threeWayMerger.getResultTreeId();
}
+ private void assertRequirement(
+ String requirement,
+ ImmutableList<Change.Id> matchingChanges,
+ ImmutableList<Change.Id> nonMatchingChanges) {
+ for (Change.Id matchingChange : matchingChanges) {
+ assertMatching(requirement, matchingChange);
+ }
+
+ for (Change.Id nonMatchingChange : nonMatchingChanges) {
+ assertNotMatching(requirement, nonMatchingChange);
+ }
+ }
+
private void assertMatching(String requirement, Change.Id change) {
- assertThat(evaluate(requirement, change).status())
+ assertWithMessage("requirement \"%s\" doesn't match change %s", requirement, change)
+ .that(evaluate(requirement, change).status())
.isEqualTo(SubmitRequirementExpressionResult.Status.PASS);
}
private void assertNotMatching(String requirement, Change.Id change) {
- assertThat(evaluate(requirement, change).status())
+ assertWithMessage("requirement \"%s\" matches change %s", requirement, change)
+ .that(evaluate(requirement, change).status())
.isEqualTo(SubmitRequirementExpressionResult.Status.FAIL);
}
diff --git a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
index a0aad96..1a546fa 100644
--- a/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/AbstractQueryChangesTest.java
@@ -1552,6 +1552,12 @@
}
@Test
+ public void cannotUseUsersArgWithLabel() throws Exception {
+ assertFailingQuery(
+ "label:Code-Review=MAX,users=human_reviewers", "Cannot use the 'users' argument in search");
+ }
+
+ @Test
public void byLabelMulti() throws Exception {
Project.NameKey project = Project.nameKey("repo");
repo = createAndOpenProject(project);
diff --git a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
index 7a3b7ec..ea1c20d 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -1177,6 +1177,7 @@
if (this.generatedFixSuggestion) {
return html`<gr-suggestion-diff-preview
id="suggestionDiffPreview"
+ .uuid=${this.generatedSuggestionId}
.fixSuggestionInfo=${this.generatedFixSuggestion}
></gr-suggestion-diff-preview>`;
} else {
diff --git a/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts b/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
index d03bd54..8ff0292 100644
--- a/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
+++ b/polygerrit-ui/app/elements/shared/gr-fix-suggestions/gr-fix-suggestions.ts
@@ -280,7 +280,7 @@
if (!this.comment?.fix_suggestions) return;
this.applyingFix = true;
try {
- await this.suggestionDiffPreview?.applyFixSuggestion();
+ await this.suggestionDiffPreview?.applyFix();
} finally {
this.applyingFix = false;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts
index 6fb7d533..5dbf9b7 100644
--- a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts
+++ b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview.ts
@@ -30,7 +30,6 @@
import {subscribe} from '../../lit/subscription-controller';
import {DiffPreview} from '../../diff/gr-apply-fix-dialog/gr-apply-fix-dialog';
import {userModelToken} from '../../../models/user/user-model';
-import {createUserFixSuggestion} from '../../../utils/comment-util';
import {commentModelToken} from '../gr-comment-model/gr-comment-model';
import {navigationToken} from '../../core/gr-navigation/gr-navigation';
import {fire} from '../../../utils/event-util';
@@ -48,15 +47,18 @@
*/
@customElement('gr-suggestion-diff-preview')
export class GrSuggestionDiffPreview extends LitElement {
+ // Optional. Used as backup when preview is not loaded.
@property({type: String})
- suggestion?: string;
+ codeText?: string;
+ // Required.
@property({type: Object})
fixSuggestionInfo?: FixSuggestionInfo;
@property({type: Boolean, attribute: 'previewed', reflect: true})
previewed = false;
+ // Optional. Used in logging.
@property({type: String})
uuid?: string;
@@ -64,13 +66,16 @@
comment?: Comment;
@state()
- commentedText?: string;
-
- @state()
layers: DiffLayer[] = [];
+ /**
+ * The fix suggestion info that the preview is loaded for.
+ *
+ * This is used to determine if the preview has been loaded for the same
+ * fix suggestion info currently in gr-comment.
+ */
@state()
- previewLoadedFor?: string | FixSuggestionInfo;
+ public previewLoadedFor?: string | FixSuggestionInfo;
@state() repo?: RepoName;
@@ -147,11 +152,6 @@
);
subscribe(
this,
- () => this.getCommentModel().commentedText$,
- commentedText => (this.commentedText = commentedText)
- );
- subscribe(
- this,
() => this.getChangeModel().repo$,
x => (this.repo = x)
);
@@ -192,27 +192,24 @@
}
override updated(changed: PropertyValues) {
- if (changed.has('commentedText') || changed.has('comment')) {
- if (this.previewLoadedFor !== this.suggestion) {
- this.fetchFixPreview();
- }
- }
-
- if (changed.has('changeNum') || changed.has('comment')) {
+ if (
+ changed.has('fixSuggestionInfo') ||
+ changed.has('changeNum') ||
+ changed.has('comment')
+ ) {
if (this.previewLoadedFor !== this.fixSuggestionInfo) {
- this.fetchfixSuggestionInfoPreview();
+ this.fetchFixPreview();
}
}
}
override render() {
- if (!this.suggestion && !this.fixSuggestionInfo) return nothing;
- const code = this.suggestion;
+ if (!this.fixSuggestionInfo) return nothing;
return html`
${when(
this.previewLoadedFor,
() => this.renderDiff(),
- () => html`<code>${code}</code>`
+ () => html`<code>${this.codeText}</code>`
)}
`;
}
@@ -236,58 +233,15 @@
}
private async fetchFixPreview() {
- if (
- !this.changeNum ||
- !this.comment?.patch_set ||
- !this.suggestion ||
- !this.commentedText
- )
- return;
- const fixSuggestions = createUserFixSuggestion(
- this.comment,
- this.commentedText,
- this.suggestion
- );
- this.reporting.time(Timing.PREVIEW_FIX_LOAD);
- const res = await this.restApiService.getFixPreview(
- this.changeNum,
- this.comment?.patch_set,
- fixSuggestions[0].replacements
- );
- if (!res) return;
- const currentPreviews = Object.keys(res).map(key => {
- return {filepath: key, preview: res[key]};
- });
- this.reporting.timeEnd(Timing.PREVIEW_FIX_LOAD, {
- uuid: this.uuid,
- commentId: this.comment?.id ?? '',
- });
- if (currentPreviews.length > 0) {
- this.preview = currentPreviews[0];
- this.previewLoadedFor = this.suggestion;
- this.previewed = true;
- }
-
- return res;
- }
-
- private async fetchfixSuggestionInfoPreview() {
- if (
- this.suggestion ||
- !this.changeNum ||
- !this.comment?.patch_set ||
- !this.fixSuggestionInfo
- )
+ if (!this.changeNum || !this.comment?.patch_set || !this.fixSuggestionInfo)
return;
- this.previewed = false;
this.reporting.time(Timing.PREVIEW_FIX_LOAD);
const res = await this.restApiService.getFixPreview(
this.changeNum,
this.comment?.patch_set,
this.fixSuggestionInfo.replacements
);
-
if (!res) return;
const currentPreviews = Object.keys(res).map(key => {
return {filepath: key, preview: res[key]};
@@ -298,26 +252,12 @@
});
if (currentPreviews.length > 0) {
this.preview = currentPreviews[0];
- this.previewed = true;
this.previewLoadedFor = this.fixSuggestionInfo;
+ this.previewed = true;
}
return res;
}
-
- /**
- * Applies a fix (fix_suggestion in comment) previewed in
- * `suggestion-diff-preview`, navigating to the new change URL with the EDIT
- * patchset.
- *
- * Similar code flow is in gr-apply-fix-dialog.handleApplyFix
- * Used in gr-fix-suggestions
- */
- public applyFixSuggestion() {
- if (this.suggestion || !this.fixSuggestionInfo) return;
- return this.applyFix(this.fixSuggestionInfo);
- }
-
/**
* Applies a fix (codeblock in comment message) previewed in
* `suggestion-diff-preview`, navigating to the new change URL with the EDIT
@@ -326,20 +266,11 @@
* Similar code flow is in gr-apply-fix-dialog.handleApplyFix
* Used in gr-user-suggestion-fix
*/
- public applyUserSuggestedFix() {
- if (!this.comment || !this.suggestion || !this.commentedText) return;
- const fixSuggestions = createUserFixSuggestion(
- this.comment,
- this.commentedText,
- this.suggestion
- );
- this.applyFix(fixSuggestions[0]);
- }
-
- private async applyFix(fixSuggestion: FixSuggestionInfo) {
+ public async applyFix() {
const changeNum = this.changeNum;
const basePatchNum = this.comment?.patch_set as BasePatchSetNum;
+ const fixSuggestion = this.fixSuggestionInfo;
if (!changeNum || !basePatchNum || !fixSuggestion) return;
this.reporting.time(Timing.APPLY_FIX_LOAD);
diff --git a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts
index 2630aad..39d937d 100644
--- a/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-suggestion-diff-preview/gr-suggestion-diff-preview_test.ts
@@ -11,7 +11,10 @@
commentModelToken,
} from '../gr-comment-model/gr-comment-model';
import {wrapInProvider} from '../../../models/di-provider-element';
-import {createComment} from '../../../test/test-data-generators';
+import {
+ createComment,
+ createFixSuggestionInfo,
+} from '../../../test/test-data-generators';
import {getAppContext} from '../../../services/app-context';
import {GrSuggestionDiffPreview} from './gr-suggestion-diff-preview';
import {stubFlags} from '../../../test/test-utils';
@@ -29,7 +32,8 @@
wrapInProvider(
html`
<gr-suggestion-diff-preview
- .suggestion=${'Hello World'}
+ .codeText=${'Hello World'}
+ .fixSuggestionInfo=${createFixSuggestionInfo()}
></gr-suggestion-diff-preview>
`,
commentModelToken,
@@ -48,7 +52,7 @@
test('render diff', async () => {
stubFlags('isEnabled').returns(true);
- element.suggestion =
+ element.codeText =
' private handleClick(e: MouseEvent) {\ne.stopPropagation();\ne.preventDefault();';
element.previewLoadedFor =
' private handleClick(e: MouseEvent) {\ne.stopPropagation();\ne.preventDefault();';
diff --git a/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix.ts b/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix.ts
index 14e7134..fef2252 100644
--- a/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix.ts
+++ b/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix.ts
@@ -19,6 +19,7 @@
import {Comment, PatchSetNumber} from '../../../types/common';
import {commentModelToken} from '../gr-comment-model/gr-comment-model';
import {waitUntil} from '../../../utils/async-util';
+import {createUserFixSuggestion} from '../../../utils/comment-util';
declare global {
interface HTMLElementEventMap {
@@ -47,6 +48,8 @@
@state() private previewLoaded = false;
+ @state() commentedText?: string;
+
private readonly getConfigModel = resolve(this, configModelToken);
private readonly getChangeModel = resolve(this, changeModelToken);
@@ -70,6 +73,11 @@
() => this.getCommentModel().comment$,
comment => (this.comment = comment)
);
+ subscribe(
+ this,
+ () => this.getCommentModel().commentedText$,
+ commentedText => (this.commentedText = commentedText)
+ );
}
static override get styles() {
@@ -95,8 +103,14 @@
}
override render() {
- if (!this.textContent) return nothing;
+ if (!this.textContent || !this.comment || !this.commentedText)
+ return nothing;
const code = this.textContent;
+ const fixSuggestions = createUserFixSuggestion(
+ this.comment,
+ this.commentedText,
+ code
+ );
return html`<div class="header">
<div class="title">
<span>Suggested edit</span>
@@ -139,7 +153,8 @@
</div>
</div>
<gr-suggestion-diff-preview
- .suggestion=${this.textContent}
+ .fixSuggestions=${fixSuggestions[0]}
+ .codeText=${code}
></gr-suggestion-diff-preview>`;
}
@@ -151,7 +166,7 @@
async handleApplyFix() {
if (!this.textContent) return;
this.applyingFix = true;
- await this.suggestionDiffPreview?.applyUserSuggestedFix();
+ await this.suggestionDiffPreview?.applyFix();
this.applyingFix = false;
}
diff --git a/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix_test.ts b/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix_test.ts
index 8baee43..56d826e 100644
--- a/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix_test.ts
+++ b/polygerrit-ui/app/elements/shared/gr-user-suggestion-fix/gr-user-suggestion-fix_test.ts
@@ -22,6 +22,7 @@
const commentModel = new CommentModel(getAppContext().restApiService);
commentModel.updateState({
comment: createComment(),
+ commentedText: 'Hello World',
});
element = (
await fixture<GrUserSuggestionsFix>(
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
index d138414..89bb49e 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor.ts
@@ -181,18 +181,29 @@
return chunkIndex;
}
+ /**
+ * Check if a chunk is collapsible.
+ *
+ * A chunk is collapsible if it is either common or skippable, and it is not
+ * a key location, or it is outside of the focus range.
+ *
+ * @param chunk The chunk to check.
+ * @param offsetLeft The offset of the left side of the chunk.
+ * @param offsetRight The offset of the right side of the chunk.
+ * @return True if the chunk is collapsible, false otherwise.
+ */
private isCollapsibleChunk(
chunk: DiffContent,
offsetLeft: number,
offsetRight: number
) {
- return (
- (chunk.ab ||
- chunk.common ||
- chunk.skip ||
- this.isChunkOutsideOfFocusRange(chunk, offsetLeft, offsetRight)) &&
- !chunk.keyLocation
+ const isCommonOrSkip = chunk.ab || chunk.common || chunk.skip;
+ const isOutsideOfFocusRange = this.isChunkOutsideOfFocusRange(
+ chunk,
+ offsetLeft,
+ offsetRight
);
+ return (isCommonOrSkip && !chunk.keyLocation) || isOutsideOfFocusRange;
}
private isChunkOutsideOfFocusRange(
diff --git a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
index 3f01096..19a687e 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff-processor/gr-diff-processor_test.ts
@@ -1054,6 +1054,32 @@
assert.equal(result.groups[0].lines.length, 5);
assert.equal(result.groups[1].type, GrDiffGroupType.DELTA);
});
+
+ test('collapse chunks with key locations if out of focus range', () => {
+ const keyLocationLineText = 'key location behind a context group';
+ state = {
+ lineNums: {left: 7, right: 6},
+ chunkIndex: 4,
+ };
+ const result = processor.processNext(state, [
+ ...chunks,
+ {
+ ab: Array.from<string>({length: 2}).fill(
+ 'all work and no play make jill a dull boy'
+ ),
+ keyLocation: false,
+ },
+ {
+ ab: Array.from<string>({length: 5}).fill(keyLocationLineText),
+ keyLocation: true,
+ },
+ ]);
+ assert.equal(result.groups.length, 3);
+ assert.equal(
+ result.groups[2].contextGroups[0].lines[0].text,
+ keyLocationLineText
+ );
+ });
});
});
diff --git a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-styles.ts b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-styles.ts
index 98a2093..2913fc8 100644
--- a/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-styles.ts
+++ b/polygerrit-ui/app/embed/diff/gr-diff/gr-diff-styles.ts
@@ -369,6 +369,9 @@
background-color: var(--dark-add-highlight-color);
&:has(.is-out-of-focus-range) {
background-color: transparent;
+ .intraline {
+ background-color: transparent;
+ }
}
}
gr-diff-row td.content.add div.contentText,
@@ -376,6 +379,9 @@
background-color: var(--light-add-highlight-color);
&:has(.is-out-of-focus-range) {
background-color: transparent;
+ .intraline {
+ background-color: transparent;
+ }
}
}
/* If there are no intraline info, consider everything changed */
