Merge "feat: Allow listing tags in specific order"
diff --git a/Documentation/rest-api-config.txt b/Documentation/rest-api-config.txt
index 4cd5bb2..37121350 100644
--- a/Documentation/rest-api-config.txt
+++ b/Documentation/rest-api-config.txt
@@ -1728,7 +1728,7 @@
This endpoint allows to trigger background reindexing of an index version. It is
also supported to specify whether to reuse existing up-to-date (non-stale) index
-documents.
+documents and whether to notifyListeners or not.
.Request
----
@@ -1736,7 +1736,8 @@
Content-Type: application/json; charset=UTF-8
{
- "reuse": "true"
+ "reuse": "true",
+ "notifyListeners": "false"
}
----
diff --git a/java/com/google/gerrit/entities/Account.java b/java/com/google/gerrit/entities/Account.java
index 767e68e..efbac97 100644
--- a/java/com/google/gerrit/entities/Account.java
+++ b/java/com/google/gerrit/entities/Account.java
@@ -162,6 +162,17 @@
public abstract String metaId();
/**
+ * A unique tag which identifies the current version of the account.
+ *
+ * <p>It can be any non-empty string. For open-source gerrit it is the same as metaId; internally
+ * in google a different value is assigned.
+ *
+ * <p>The value can be null only during account updating/creation.
+ */
+ @Nullable
+ public abstract String uniqueTag();
+
+ /**
* Create a new account.
*
* @param newId unique id, see Sequences#nextAccountId().
@@ -278,6 +289,11 @@
public abstract Builder setMetaId(@Nullable String metaId);
+ @Nullable
+ public abstract String uniqueTag();
+
+ public abstract Builder setUniqueTag(@Nullable String uniqueTag);
+
public abstract Account build();
}
@@ -296,6 +312,7 @@
.add("inactive", inactive())
.add("status", status())
.add("metaId", metaId())
+ .add("uniqueTag", uniqueTag())
.toString();
}
}
diff --git a/java/com/google/gerrit/index/SiteIndexer.java b/java/com/google/gerrit/index/SiteIndexer.java
index 32b4b21..bfb4407 100644
--- a/java/com/google/gerrit/index/SiteIndexer.java
+++ b/java/com/google/gerrit/index/SiteIndexer.java
@@ -76,6 +76,16 @@
/** Indexes all entities for the provided index. */
public abstract Result indexAll(I index);
+ /**
+ * Indexes all entities for the provided index.
+ *
+ * <p>NOTE: This method does not implement the 'notifyListeners' logic which is effectively
+ * ignored and all listeners are always notified.
+ */
+ public Result indexAll(I index, @SuppressWarnings("unused") boolean notifyListeners) {
+ return indexAll(index);
+ }
+
protected final void addErrorListener(
ListenableFuture<?> future, String desc, ProgressMonitor progress, AtomicBoolean ok) {
future.addListener(
diff --git a/java/com/google/gerrit/pgm/init/AccountsOnInitNoteDbImpl.java b/java/com/google/gerrit/pgm/init/AccountsOnInitNoteDbImpl.java
index e3e485f..5584eba 100644
--- a/java/com/google/gerrit/pgm/init/AccountsOnInitNoteDbImpl.java
+++ b/java/com/google/gerrit/pgm/init/AccountsOnInitNoteDbImpl.java
@@ -110,6 +110,7 @@
throw new IOException(String.format("Failed to update ref %s: %s", refName, result.name()));
}
account.setMetaId(id.name());
+ account.setUniqueTag(id.name());
}
return account.build();
}
diff --git a/java/com/google/gerrit/server/account/AccountProperties.java b/java/com/google/gerrit/server/account/AccountProperties.java
index 5f56aa3..9cc20d4 100644
--- a/java/com/google/gerrit/server/account/AccountProperties.java
+++ b/java/com/google/gerrit/server/account/AccountProperties.java
@@ -97,6 +97,7 @@
accountBuilder.setStatus(get(accountConfig, KEY_STATUS));
accountBuilder.setMetaId(metaId != null ? metaId.name() : null);
+ accountBuilder.setUniqueTag(accountBuilder.metaId());
account = accountBuilder.build();
}
diff --git a/java/com/google/gerrit/server/account/CachedAccountDetails.java b/java/com/google/gerrit/server/account/CachedAccountDetails.java
index e167a23..e6e2735 100644
--- a/java/com/google/gerrit/server/account/CachedAccountDetails.java
+++ b/java/com/google/gerrit/server/account/CachedAccountDetails.java
@@ -114,7 +114,8 @@
.setDisplayName(Strings.nullToEmpty(account.displayName()))
.setPreferredEmail(Strings.nullToEmpty(account.preferredEmail()))
.setStatus(Strings.nullToEmpty(account.status()))
- .setMetaId(Strings.nullToEmpty(account.metaId()));
+ .setMetaId(Strings.nullToEmpty(account.metaId()))
+ .setUniqueTag(Strings.nullToEmpty(account.uniqueTag()));
serialized.setAccount(accountProto);
for (Map.Entry<ProjectWatches.ProjectWatchKey, ImmutableSet<NotifyConfig.NotifyType>> watch :
@@ -145,7 +146,7 @@
public CachedAccountDetails deserialize(byte[] in) {
Cache.AccountDetailsProto proto =
Protos.parseUnchecked(Cache.AccountDetailsProto.parser(), in);
- Account account =
+ Account.Builder builder =
Account.builder(
Account.id(proto.getAccount().getId()),
Instant.ofEpochMilli(proto.getAccount().getRegisteredOn()))
@@ -155,7 +156,11 @@
.setInactive(proto.getAccount().getInactive())
.setStatus(Strings.emptyToNull(proto.getAccount().getStatus()))
.setMetaId(Strings.emptyToNull(proto.getAccount().getMetaId()))
- .build();
+ .setUniqueTag(Strings.emptyToNull(proto.getAccount().getUniqueTag()));
+ if (Strings.isNullOrEmpty(builder.uniqueTag())) {
+ builder.setUniqueTag(builder.metaId());
+ }
+ Account account = builder.build();
ImmutableMap.Builder<ProjectWatches.ProjectWatchKey, ImmutableSet<NotifyConfig.NotifyType>>
projectWatches = ImmutableMap.builder();
diff --git a/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java b/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
index ad46483..81f1e71 100644
--- a/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
+++ b/java/com/google/gerrit/server/cache/h2/H2CacheImpl.java
@@ -419,7 +419,7 @@
@Nullable
private BloomFilter<K> buildBloomFilter() {
SqlHandle c = null;
- try {
+ try (TraceTimer ignored = TraceContext.newTimer("Build bloom filter", Metadata.empty())) {
c = acquire();
if (estimatedSize <= 0) {
try (PreparedStatement ps =
@@ -761,6 +761,8 @@
"ALTER TABLE data ADD COLUMN IF NOT EXISTS "
+ "space BIGINT AS OCTET_LENGTH(k) + OCTET_LENGTH(v)");
stmt.addBatch("ALTER TABLE data ADD COLUMN IF NOT EXISTS version INT DEFAULT 0 NOT NULL");
+ stmt.addBatch("CREATE INDEX IF NOT EXISTS version_key ON data(version, k)");
+ stmt.addBatch("CREATE INDEX IF NOT EXISTS accessed ON data(accessed)");
stmt.executeBatch();
}
}
diff --git a/java/com/google/gerrit/server/change/ChangeResource.java b/java/com/google/gerrit/server/change/ChangeResource.java
index 481c17b..dc09bf4 100644
--- a/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/java/com/google/gerrit/server/change/ChangeResource.java
@@ -251,7 +251,15 @@
private void hashAccount(Hasher h, AccountState accountState, byte[] buf) {
h.putInt(accountState.account().id().get());
- h.putString(MoreObjects.firstNonNull(accountState.account().metaId(), ZERO_ID_STRING), UTF_8);
+ // Based on the code, it seems the metaId should never be null in this place and so the
+ // uniqueTag.
+ // However, the null-check for metaId has been existed here for some time already - for safety
+ // the same check is applied to uniqueTag.
+ h.putString(
+ MoreObjects.firstNonNull(
+ accountState.account().uniqueTag(),
+ MoreObjects.firstNonNull(accountState.account().metaId(), ZERO_ID_STRING)),
+ UTF_8);
accountState.externalIds().stream().forEach(e -> hashObjectId(h, e.blobId(), buf));
}
}
diff --git a/java/com/google/gerrit/server/index/IndexVersionReindexer.java b/java/com/google/gerrit/server/index/IndexVersionReindexer.java
index 5d136e8..84be97e 100644
--- a/java/com/google/gerrit/server/index/IndexVersionReindexer.java
+++ b/java/com/google/gerrit/server/index/IndexVersionReindexer.java
@@ -35,14 +35,14 @@
}
public <K, V, I extends Index<K, V>> Future<SiteIndexer.Result> reindex(
- IndexDefinition<K, V, I> def, int version, boolean reuse) {
+ IndexDefinition<K, V, I> def, int version, boolean reuse, boolean notifyListeners) {
I index = def.getIndexCollection().getWriteIndex(version);
SiteIndexer<K, V, I> siteIndexer = def.getSiteIndexer(reuse);
return executor.submit(
() -> {
String name = def.getName();
logger.atInfo().log("Starting reindex of %s version %d", name, version);
- SiteIndexer.Result result = siteIndexer.indexAll(index);
+ SiteIndexer.Result result = siteIndexer.indexAll(index, notifyListeners);
if (result.success()) {
logger.atInfo().log("Reindex %s version %s complete", name, version);
} else {
diff --git a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
index 925f68d..19a0223 100644
--- a/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
+++ b/java/com/google/gerrit/server/index/change/AllChangesIndexer.java
@@ -177,6 +177,11 @@
@Override
public Result indexAll(ChangeIndex index) {
+ return indexAll(index, true);
+ }
+
+ @Override
+ public Result indexAll(ChangeIndex index, boolean notifyListeners) {
// The simplest approach to distribute indexing would be to let each thread grab a project
// and index it fully. But if a site has one big project and 100s of small projects, then
// in the beginning all CPUs would be busy reindexing projects. But soon enough all small
@@ -199,7 +204,7 @@
failedTask = mpm.beginSubTask("failed", MultiProgressMonitor.UNKNOWN);
List<ListenableFuture<?>> futures;
try {
- futures = new SliceScheduler(index, ok).schedule();
+ futures = new SliceScheduler(index, ok, notifyListeners).schedule();
} catch (ProjectsCollectionFailure e) {
logger.atSevere().log("%s", e.getMessage());
return Result.create(sw, false, 0, 0);
@@ -359,6 +364,7 @@
private class SliceScheduler {
final ChangeIndex index;
final AtomicBoolean ok;
+ final boolean notifyListeners;
final AtomicInteger changeCount = new AtomicInteger(0);
final AtomicInteger projectsFailed = new AtomicInteger(0);
final List<ListenableFuture<?>> sliceIndexerFutures = new ArrayList<>();
@@ -366,9 +372,10 @@
VolatileTask projTask = mpm.beginVolatileSubTask("project-slices");
Task slicingProjects;
- public SliceScheduler(ChangeIndex index, AtomicBoolean ok) {
+ public SliceScheduler(ChangeIndex index, AtomicBoolean ok, boolean notifyListeners) {
this.index = index;
this.ok = ok;
+ this.notifyListeners = notifyListeners;
}
private List<ListenableFuture<?>> schedule() throws ProjectsCollectionFailure {
@@ -376,7 +383,7 @@
int projectCount = projects.size();
slicingProjects = mpm.beginSubTask("Slicing projects", projectCount);
for (Project.NameKey name : projects) {
- sliceCreationFutures.add(executor.submit(new ProjectSliceCreator(name)));
+ sliceCreationFutures.add(executor.submit(new ProjectSliceCreator(name, notifyListeners)));
}
try {
@@ -407,9 +414,11 @@
private class ProjectSliceCreator implements Callable<Void> {
final Project.NameKey name;
+ final boolean notifyListeners;
- public ProjectSliceCreator(Project.NameKey name) {
+ public ProjectSliceCreator(Project.NameKey name, boolean notifyListeners) {
this.name = name;
+ this.notifyListeners = notifyListeners;
}
@Override
@@ -434,9 +443,10 @@
ChangeIndexer indexer;
if (reuseExistingDocuments) {
indexer =
- indexerFactory.create(executor, index, stalenessCheckerFactory.create(index));
+ indexerFactory.create(
+ executor, index, stalenessCheckerFactory.create(index), notifyListeners);
} else {
- indexer = indexerFactory.create(executor, index);
+ indexer = indexerFactory.create(executor, index, notifyListeners);
}
ListenableFuture<?> future =
executor.submit(reindexProjectSlice(indexer, projectSlice, doneTask, failedTask));
diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/java/com/google/gerrit/server/index/change/ChangeIndexer.java
index 4ec390d..fc666ad 100644
--- a/java/com/google/gerrit/server/index/change/ChangeIndexer.java
+++ b/java/com/google/gerrit/server/index/change/ChangeIndexer.java
@@ -71,9 +71,18 @@
ChangeIndexer create(ListeningExecutorService executor, ChangeIndex index);
ChangeIndexer create(
- ListeningExecutorService executor, ChangeIndex index, StalenessChecker stalenessChecker);
+ ListeningExecutorService executor, ChangeIndex index, boolean notifyListeners);
+
+ ChangeIndexer create(
+ ListeningExecutorService executor,
+ ChangeIndex index,
+ StalenessChecker stalenessChecker,
+ boolean notifyListeners);
ChangeIndexer create(ListeningExecutorService executor, ChangeIndexCollection indexes);
+
+ ChangeIndexer create(
+ ListeningExecutorService executor, ChangeIndexCollection indexes, boolean notifyListeners);
}
@Nullable private final ChangeIndexCollection indexes;
@@ -87,6 +96,7 @@
private final StalenessChecker stalenessChecker;
private final boolean autoReindexIfStale;
private final IsFirstInsertForEntry isFirstInsertForEntry;
+ private final boolean notifyListeners;
private final Map<Change.Id, IndexTask> queuedIndexTasks = new ConcurrentHashMap<>();
private final Set<ReindexIfStaleTask> queuedReindexIfStaleTasks =
@@ -104,6 +114,33 @@
@Assisted ListeningExecutorService executor,
@Assisted ChangeIndex index,
IsFirstInsertForEntry isFirstInsertForEntry) {
+ this(
+ cfg,
+ changeDataFactory,
+ notesFactory,
+ context,
+ indexedListeners,
+ stalenessChecker,
+ batchExecutor,
+ executor,
+ index,
+ true,
+ isFirstInsertForEntry);
+ }
+
+ @AssistedInject
+ ChangeIndexer(
+ @GerritServerConfig Config cfg,
+ ChangeData.Factory changeDataFactory,
+ ChangeNotes.Factory notesFactory,
+ ThreadLocalRequestContext context,
+ PluginSetContext<ChangeIndexedListener> indexedListeners,
+ StalenessChecker stalenessChecker,
+ @IndexExecutor(BATCH) ListeningExecutorService batchExecutor,
+ @Assisted ListeningExecutorService executor,
+ @Assisted ChangeIndex index,
+ @Assisted boolean notifyListeners,
+ IsFirstInsertForEntry isFirstInsertForEntry) {
this.executor = executor;
this.changeDataFactory = changeDataFactory;
this.notesFactory = notesFactory;
@@ -115,6 +152,7 @@
this.index = index;
this.indexes = null;
this.isFirstInsertForEntry = isFirstInsertForEntry;
+ this.notifyListeners = notifyListeners;
}
@AssistedInject
@@ -128,7 +166,8 @@
IsFirstInsertForEntry isFirstInsertForEntry,
@Assisted ListeningExecutorService executor,
@Assisted ChangeIndex index,
- @Assisted StalenessChecker stalenessChecker) {
+ @Assisted StalenessChecker stalenessChecker,
+ @Assisted boolean notifyListeners) {
this.executor = executor;
this.changeDataFactory = changeDataFactory;
this.notesFactory = notesFactory;
@@ -140,6 +179,7 @@
this.index = index;
this.indexes = null;
this.stalenessChecker = stalenessChecker;
+ this.notifyListeners = notifyListeners;
}
@AssistedInject
@@ -154,6 +194,33 @@
@Assisted ListeningExecutorService executor,
@Assisted ChangeIndexCollection indexes,
IsFirstInsertForEntry isFirstInsertForEntry) {
+ this(
+ cfg,
+ changeDataFactory,
+ notesFactory,
+ context,
+ indexedListeners,
+ stalenessChecker,
+ batchExecutor,
+ executor,
+ indexes,
+ true,
+ isFirstInsertForEntry);
+ }
+
+ @AssistedInject
+ ChangeIndexer(
+ @GerritServerConfig Config cfg,
+ ChangeData.Factory changeDataFactory,
+ ChangeNotes.Factory notesFactory,
+ ThreadLocalRequestContext context,
+ PluginSetContext<ChangeIndexedListener> indexedListeners,
+ StalenessChecker stalenessChecker,
+ @IndexExecutor(BATCH) ListeningExecutorService batchExecutor,
+ @Assisted ListeningExecutorService executor,
+ @Assisted ChangeIndexCollection indexes,
+ @Assisted boolean notifyListeners,
+ IsFirstInsertForEntry isFirstInsertForEntry) {
this.executor = executor;
this.changeDataFactory = changeDataFactory;
this.notesFactory = notesFactory;
@@ -164,6 +231,7 @@
this.autoReindexIfStale = autoReindexIfStale(cfg);
this.index = null;
this.indexes = indexes;
+ this.notifyListeners = notifyListeners;
this.isFirstInsertForEntry = isFirstInsertForEntry;
}
@@ -290,19 +358,27 @@
}
private void fireChangeScheduledForIndexingEvent(String projectName, int id) {
- indexedListeners.runEach(l -> l.onChangeScheduledForIndexing(projectName, id));
+ if (notifyListeners) {
+ indexedListeners.runEach(l -> l.onChangeScheduledForIndexing(projectName, id));
+ }
}
private void fireChangeIndexedEvent(String projectName, int id) {
- indexedListeners.runEach(l -> l.onChangeIndexed(projectName, id));
+ if (notifyListeners) {
+ indexedListeners.runEach(l -> l.onChangeIndexed(projectName, id));
+ }
}
private void fireChangeScheduledForDeletionFromIndexEvent(int id) {
- indexedListeners.runEach(l -> l.onChangeScheduledForDeletionFromIndex(id));
+ if (notifyListeners) {
+ indexedListeners.runEach(l -> l.onChangeScheduledForDeletionFromIndex(id));
+ }
}
private void fireChangeDeletedFromIndexEvent(int id) {
- indexedListeners.runEach(l -> l.onChangeDeleted(id));
+ if (notifyListeners) {
+ indexedListeners.runEach(l -> l.onChangeDeleted(id));
+ }
}
/**
diff --git a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
index 5b66277..aa2c297 100644
--- a/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
+++ b/java/com/google/gerrit/server/project/SubmitRuleEvaluator.java
@@ -15,9 +15,9 @@
package com.google.gerrit.server.project;
import static com.google.common.collect.ImmutableList.toImmutableList;
+import static com.google.gerrit.server.project.ProjectCache.illegalState;
import com.google.common.collect.Streams;
-import com.google.gerrit.entities.Project;
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.entities.SubmitTypeRecord;
import com.google.gerrit.exceptions.StorageException;
@@ -156,7 +156,7 @@
// Skip evaluating the default submit rule if the project has prolog rules.
// Note that in this case, the prolog submit rule will handle labels for us
.filter(
- projectState.hasPrologRules()
+ projectState.hasPrologRules() && prologSubmitRuleUtil.isProjectRulesEnabled()
? rule -> !(rule.get() instanceof DefaultSubmitRule)
: rule -> true)
.map(
@@ -185,14 +185,10 @@
*/
public SubmitTypeRecord getSubmitType(ChangeData cd) {
try (Timer0.Context ignored = metrics.submitTypeEvaluationLatency.start()) {
- try {
- Project.NameKey name = cd.project();
- Optional<ProjectState> project = projectCache.get(name);
- if (!project.isPresent()) {
- throw new NoSuchProjectException(name);
- }
- } catch (NoSuchProjectException e) {
- throw new IllegalStateException("Unable to find project while evaluating submit rule", e);
+ ProjectState projectState =
+ projectCache.get(cd.project()).orElseThrow(illegalState(cd.project()));
+ if (!prologSubmitRuleUtil.isProjectRulesEnabled()) {
+ return SubmitTypeRecord.OK(projectState.getSubmitType());
}
return prologSubmitRuleUtil.getSubmitType(cd);
diff --git a/java/com/google/gerrit/server/restapi/config/ReindexIndexVersion.java b/java/com/google/gerrit/server/restapi/config/ReindexIndexVersion.java
index d923155..21cd1c1 100644
--- a/java/com/google/gerrit/server/restapi/config/ReindexIndexVersion.java
+++ b/java/com/google/gerrit/server/restapi/config/ReindexIndexVersion.java
@@ -25,7 +25,8 @@
public class ReindexIndexVersion implements RestModifyView<IndexVersionResource, Input> {
public static class Input {
- boolean reuse;
+ public boolean reuse;
+ public boolean notifyListeners;
}
private final IndexVersionReindexer indexVersionReindexer;
@@ -41,7 +42,7 @@
IndexDefinition<?, ?, ?> def = rsrc.getIndexDefinition();
int version = rsrc.getIndex().getSchema().getVersion();
@SuppressWarnings("unused")
- var unused = indexVersionReindexer.reindex(def, version, input.reuse);
+ var unused = indexVersionReindexer.reindex(def, version, input.reuse, input.notifyListeners);
return Response.accepted(
String.format("Index %s version %d submitted for reindexing", def.getName(), version));
}
diff --git a/java/com/google/gerrit/server/rules/PrologSubmitRuleUtil.java b/java/com/google/gerrit/server/rules/PrologSubmitRuleUtil.java
index a94fb6e..a568371 100644
--- a/java/com/google/gerrit/server/rules/PrologSubmitRuleUtil.java
+++ b/java/com/google/gerrit/server/rules/PrologSubmitRuleUtil.java
@@ -20,22 +20,29 @@
/** Provides prolog-related operations to different callers. */
public interface PrologSubmitRuleUtil {
+ /** Returns true if prolog rules are enabled for the project. */
+ boolean isProjectRulesEnabled();
/**
* Returns the submit-type of a change depending on the change data and the definition of the
* prolog rules file.
+ *
+ * <p>Must only be called when Prolog rules are enabled on the Gerrit server.
*/
SubmitTypeRecord getSubmitType(ChangeData cd);
/**
* Returns the submit-type of a change depending on the change data and the definition of the
* prolog rules file.
+ *
+ * <p>Must only be called when Prolog rules are enabled on the Gerrit server.
*/
SubmitTypeRecord getSubmitType(ChangeData cd, String ruleToTest, boolean skipFilters);
- /** Evaluates a submit rule. */
+ /**
+ * Evaluates a submit rule.
+ *
+ * <p>Must only be called when Prolog rules are enabled on the Gerrit server.
+ */
SubmitRecord evaluate(ChangeData cd, String ruleToTest, boolean skipFilters);
-
- /** Returns true if prolog rules are enabled for the project. */
- boolean isProjectRulesEnabled();
}
diff --git a/java/com/google/gerrit/server/rules/prolog/PrologRule.java b/java/com/google/gerrit/server/rules/prolog/PrologRule.java
index 13814bb..c560fd2 100644
--- a/java/com/google/gerrit/server/rules/prolog/PrologRule.java
+++ b/java/com/google/gerrit/server/rules/prolog/PrologRule.java
@@ -30,15 +30,22 @@
class PrologRule implements SubmitRule {
private final PrologRuleEvaluator.Factory factory;
private final ProjectCache projectCache;
+ private final boolean isProjectRulesEnabled;
@Inject
- private PrologRule(PrologRuleEvaluator.Factory factory, ProjectCache projectCache) {
+ private PrologRule(
+ PrologRuleEvaluator.Factory factory, ProjectCache projectCache, RulesCache rulesCache) {
this.factory = factory;
this.projectCache = projectCache;
+ this.isProjectRulesEnabled = rulesCache.isProjectRulesEnabled();
}
@Override
public Optional<SubmitRecord> evaluate(ChangeData cd) {
+ if (!isProjectRulesEnabled) {
+ return Optional.empty();
+ }
+
ProjectState projectState =
projectCache.get(cd.project()).orElseThrow(illegalState(cd.project()));
// We only want to run the Prolog engine if we have at least one rules.pl file to use.
@@ -49,15 +56,11 @@
return Optional.of(evaluate(cd, PrologOptions.defaultOptions()));
}
- public SubmitRecord evaluate(ChangeData cd, PrologOptions opts) {
+ SubmitRecord evaluate(ChangeData cd, PrologOptions opts) {
return getEvaluator(cd, opts).evaluate();
}
- public SubmitTypeRecord getSubmitType(ChangeData cd) {
- return getSubmitType(cd, PrologOptions.defaultOptions());
- }
-
- public SubmitTypeRecord getSubmitType(ChangeData cd, PrologOptions opts) {
+ SubmitTypeRecord getSubmitType(ChangeData cd, PrologOptions opts) {
return getEvaluator(cd, opts).getSubmitType();
}
diff --git a/java/com/google/gerrit/server/rules/prolog/PrologSubmitRuleUtilImpl.java b/java/com/google/gerrit/server/rules/prolog/PrologSubmitRuleUtilImpl.java
index 3d017e2..6be71f8 100644
--- a/java/com/google/gerrit/server/rules/prolog/PrologSubmitRuleUtilImpl.java
+++ b/java/com/google/gerrit/server/rules/prolog/PrologSubmitRuleUtilImpl.java
@@ -14,6 +14,8 @@
package com.google.gerrit.server.rules.prolog;
+import static com.google.common.base.Preconditions.checkState;
+
import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.entities.SubmitTypeRecord;
import com.google.gerrit.server.query.change.ChangeData;
@@ -25,7 +27,6 @@
@Singleton
public class PrologSubmitRuleUtilImpl implements PrologSubmitRuleUtil {
private final PrologRule prologRule;
-
private final RulesCache rulesCache;
@Inject
@@ -35,22 +36,25 @@
}
@Override
+ public boolean isProjectRulesEnabled() {
+ return rulesCache.isProjectRulesEnabled();
+ }
+
+ @Override
public SubmitTypeRecord getSubmitType(ChangeData cd) {
- return prologRule.getSubmitType(cd);
+ checkState(isProjectRulesEnabled(), "prolog rules disabled");
+ return prologRule.getSubmitType(cd, PrologOptions.defaultOptions());
}
@Override
public SubmitTypeRecord getSubmitType(ChangeData cd, String ruleToTest, boolean skipFilters) {
+ checkState(isProjectRulesEnabled(), "prolog rules disabled");
return prologRule.getSubmitType(cd, PrologOptions.dryRunOptions(ruleToTest, skipFilters));
}
@Override
public SubmitRecord evaluate(ChangeData cd, String ruleToTest, boolean skipFilters) {
+ checkState(isProjectRulesEnabled(), "prolog rules disabled");
return prologRule.evaluate(cd, PrologOptions.dryRunOptions(ruleToTest, skipFilters));
}
-
- @Override
- public boolean isProjectRulesEnabled() {
- return rulesCache.isProjectRulesEnabled();
- }
}
diff --git a/java/com/google/gerrit/testing/FakeAccountCache.java b/java/com/google/gerrit/testing/FakeAccountCache.java
index 2c01548..330b602 100644
--- a/java/com/google/gerrit/testing/FakeAccountCache.java
+++ b/java/com/google/gerrit/testing/FakeAccountCache.java
@@ -43,6 +43,7 @@
return newState(
Account.builder(accountId, TimeUtil.now())
.setMetaId("1234567812345678123456781234567812345678")
+ .setUniqueTag("1234567812345678123456781234567812345678")
.build());
}
diff --git a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
index 6f3f8d7..fdf7457 100644
--- a/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/accounts/AccountIT.java
@@ -2089,23 +2089,30 @@
}
@Test
- public void checkMetaId() throws Exception {
- // metaId is set when account is loaded
+ public void checkMetaIdAndUniqueTag() throws Exception {
+ // In open-source Gerrit, the uniqueTag and metaId are always the same. Check them together
+ // in this test.
+ // metaId and uniqueTag are set when account is loaded
assertThat(accounts.get(admin.id()).get().account().metaId()).isEqualTo(getMetaId(admin.id()));
+ assertThat(accounts.get(admin.id()).get().account().uniqueTag())
+ .isEqualTo(getMetaId(admin.id()));
- // metaId is set when account is created
+ // metaId and uniqueTag are set when account is created
AccountsUpdate au = accountsUpdateProvider.get();
Account.Id accountId = Account.id(seq.nextAccountId());
AccountState accountState = au.insert("Create Test Account", accountId, u -> {});
assertThat(accountState.account().metaId()).isEqualTo(getMetaId(accountId));
+ assertThat(accountState.account().uniqueTag()).isEqualTo(getMetaId(accountId));
- // metaId is set when account is updated
+ // metaId and uniqueTag are set when account is updated
Optional<AccountState> updatedAccountState =
au.update("Set Full Name", accountId, u -> u.setFullName("foo"));
assertThat(updatedAccountState).isPresent();
Account updatedAccount = updatedAccountState.get().account();
assertThat(accountState.account().metaId()).isNotEqualTo(updatedAccount.metaId());
+ assertThat(accountState.account().uniqueTag()).isNotEqualTo(updatedAccount.uniqueTag());
assertThat(updatedAccount.metaId()).isEqualTo(getMetaId(accountId));
+ assertThat(updatedAccount.uniqueTag()).isEqualTo(getMetaId(accountId));
}
private EmailInput newEmailInput(String email, boolean noConfirmation) {
diff --git a/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java b/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java
index d1d3620..83de9824 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/SubmitTypeRuleIT.java
@@ -28,8 +28,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
-import com.google.gerrit.acceptance.PushOneCommit.Result;
-import com.google.gerrit.entities.Change;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.entities.RefNames;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.api.projects.BranchInput;
@@ -37,14 +36,12 @@
import com.google.gerrit.extensions.common.TestSubmitRuleInfo;
import com.google.gerrit.extensions.common.TestSubmitRuleInput;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.server.git.meta.MetaDataUpdate;
import com.google.gerrit.server.git.meta.VersionedMetaData;
import com.google.gerrit.testing.ConfigSuite;
import java.io.IOException;
import java.util.concurrent.atomic.AtomicInteger;
import org.eclipse.jgit.api.Git;
-import org.eclipse.jgit.errors.ConfigInvalidException;
import org.eclipse.jgit.lib.CommitBuilder;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectId;
@@ -60,7 +57,7 @@
return submitWholeTopicEnabledConfig();
}
- private class RulesPl extends VersionedMetaData {
+ private static class RulesPl extends VersionedMetaData {
private String rule;
@Override
@@ -69,21 +66,12 @@
}
@Override
- protected void onLoad() throws IOException, ConfigInvalidException {
+ protected void onLoad() throws IOException {
rule = readUTF8(RULES_PL_FILE);
}
@Override
- protected boolean onSave(CommitBuilder commit) throws IOException, ConfigInvalidException {
- TestSubmitRuleInput in = new TestSubmitRuleInput();
- in.rule = rule;
- try {
- @SuppressWarnings("unused")
- var unused = gApi.changes().id(testChangeId.get()).current().testSubmitType(in);
- } catch (RestApiException e) {
- throw new ConfigInvalidException("Invalid submit type rule", e);
- }
-
+ protected boolean onSave(CommitBuilder commit) throws IOException {
saveUTF8(RULES_PL_FILE, rule);
return true;
}
@@ -91,18 +79,10 @@
private AtomicInteger fileCounter;
- // The change is used only to verify that the rule is valid. It is never submitted in the test.
- private Change.Id testChangeId;
-
@Before
public void setUp() throws Exception {
fileCounter = new AtomicInteger();
gApi.projects().name(project.get()).branch("test").create(new BranchInput());
- Result testChange = createChange("test", "test change");
- testChangeId = testChange.getChange().getId();
- // Reset repo back to the original state - otherwise all changes in tests have testChange as a
- // parent.
- testRepo.reset(testChange.getCommit().getParent(0));
}
private void setRulesPl(String rule) throws Exception {
@@ -193,6 +173,21 @@
}
@Test
+ @GerritConfig(name = "rules.enable", value = "false")
+ public void submitType_rulesTakeNoEffectWhenDisabled() throws Exception {
+ PushOneCommit.Result r1 = createChange("master", "Default 1");
+ PushOneCommit.Result r2 = createChange("master", "FAST_FORWARD_ONLY 2");
+ PushOneCommit.Result r3 = createChange("master", "MERGE_IF_NECESSARY 3");
+
+ setRulesPl(SUBMIT_TYPE_FROM_SUBJECT);
+
+ // Rules take no effect
+ assertSubmitType(MERGE_IF_NECESSARY, r1.getChangeId());
+ assertSubmitType(MERGE_IF_NECESSARY, r2.getChangeId());
+ assertSubmitType(MERGE_IF_NECESSARY, r3.getChangeId());
+ }
+
+ @Test
public void submitTypeIsUsedForSubmit() throws Exception {
setRulesPl(SUBMIT_TYPE_FROM_SUBJECT);
diff --git a/javatests/com/google/gerrit/acceptance/server/index/change/BUILD b/javatests/com/google/gerrit/acceptance/server/index/BUILD
similarity index 81%
rename from javatests/com/google/gerrit/acceptance/server/index/change/BUILD
rename to javatests/com/google/gerrit/acceptance/server/index/BUILD
index 3be5249..1d4ef02 100644
--- a/javatests/com/google/gerrit/acceptance/server/index/change/BUILD
+++ b/javatests/com/google/gerrit/acceptance/server/index/BUILD
@@ -1,7 +1,7 @@
load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests")
acceptance_tests(
- srcs = glob(["*IT.java"]),
+ srcs = glob(["**/*IT.java"]),
group = "server_index",
labels = ["server"],
)
diff --git a/javatests/com/google/gerrit/acceptance/server/index/ReindexIndexVersionIT.java b/javatests/com/google/gerrit/acceptance/server/index/ReindexIndexVersionIT.java
new file mode 100644
index 0000000..d433ca7
--- /dev/null
+++ b/javatests/com/google/gerrit/acceptance/server/index/ReindexIndexVersionIT.java
@@ -0,0 +1,87 @@
+// 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.acceptance.server.index;
+
+import static com.google.common.truth.Truth.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+
+import com.google.gerrit.acceptance.AbstractDaemonTest;
+import com.google.gerrit.acceptance.ExtensionRegistry;
+import com.google.gerrit.acceptance.ExtensionRegistry.Registration;
+import com.google.gerrit.entities.Change;
+import com.google.gerrit.extensions.events.ChangeIndexedListener;
+import com.google.gerrit.extensions.restapi.ResourceNotFoundException;
+import com.google.gerrit.extensions.restapi.Response;
+import com.google.gerrit.index.Index;
+import com.google.gerrit.index.IndexDefinition;
+import com.google.gerrit.server.config.IndexVersionResource;
+import com.google.gerrit.server.restapi.config.ReindexIndexVersion;
+import com.google.inject.Inject;
+import java.util.Collection;
+import javax.servlet.http.HttpServletResponse;
+import org.junit.Before;
+import org.junit.Test;
+
+public class ReindexIndexVersionIT extends AbstractDaemonTest {
+
+ @Inject private ReindexIndexVersion reindexIndexVersion;
+ @Inject private Collection<IndexDefinition<?, ?, ?>> indexDefs;
+ @Inject private ExtensionRegistry extensionRegistry;
+
+ private IndexDefinition<?, ?, ?> def;
+ private Index<?, ?> changeIndex;
+ private Change.Id C1;
+ private Change.Id C2;
+
+ private ChangeIndexedListener changeIndexedListener;
+ private ReindexIndexVersion.Input input = new ReindexIndexVersion.Input();
+
+ @Before
+ public void setUp() throws Exception {
+ def = indexDefs.stream().filter(i -> i.getName().equals("changes")).findFirst().get();
+ changeIndex = def.getIndexCollection().getSearchIndex();
+ C1 = createChange().getChange().getId();
+ C2 = createChange().getChange().getId();
+ changeIndexedListener = mock(ChangeIndexedListener.class);
+ input = new ReindexIndexVersion.Input();
+ }
+
+ @Test
+ public void reindexWithListenerNotification() throws Exception {
+ input.notifyListeners = true;
+ reindex();
+ verify(changeIndexedListener, times(1)).onChangeIndexed(project.get(), C1.get());
+ verify(changeIndexedListener, times(1)).onChangeIndexed(project.get(), C2.get());
+ }
+
+ @Test
+ public void reindexWithoutListenerNotification() throws Exception {
+ input.notifyListeners = false;
+ reindex();
+ verifyNoInteractions(changeIndexedListener);
+ }
+
+ private void reindex() throws ResourceNotFoundException {
+ try (Registration registration =
+ extensionRegistry.newRegistration().add(changeIndexedListener)) {
+ Response<?> rsp =
+ reindexIndexVersion.apply(new IndexVersionResource(def, changeIndex), input);
+ assertThat(rsp.statusCode()).isEqualTo(HttpServletResponse.SC_ACCEPTED);
+ }
+ }
+}
diff --git a/javatests/com/google/gerrit/acceptance/server/rules/prolog/RulesIT.java b/javatests/com/google/gerrit/acceptance/server/rules/prolog/RulesIT.java
index 1f47958..772812f 100644
--- a/javatests/com/google/gerrit/acceptance/server/rules/prolog/RulesIT.java
+++ b/javatests/com/google/gerrit/acceptance/server/rules/prolog/RulesIT.java
@@ -22,6 +22,7 @@
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
+import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.change.IndexOperations;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
import com.google.gerrit.entities.RefNames;
@@ -170,6 +171,29 @@
assertThat(statusForRuleAddFile("foo")).isEqualTo(SubmitRecord.Status.RULE_ERROR);
}
+ @Test
+ @GerritConfig(name = "rules.enable", value = "false")
+ public void prologRule_noEffectWhenRulesDisabled() throws Exception {
+ modifySubmitRules("gerrit:commit_message_matches('foo.*')");
+ String changeId = createChange().getChangeId();
+ // Default rules don't allow submission
+ assertThat(gApi.changes().id(changeId).get().submittable).isFalse();
+ // Satisfy default rules
+ approve(changeId);
+
+ assertThat(gApi.changes().id(changeId).get().submittable).isTrue();
+ }
+
+ @Test
+ @GerritConfig(name = "rules.enable", value = "true")
+ public void prologRule_takesEffectWhenRulesEnabled() throws Exception {
+ modifySubmitRules("gerrit:commit_message_matches('foo.*')");
+ String changeId = createChange().getChangeId();
+ approve(changeId);
+
+ assertThat(gApi.changes().id(changeId).get().submittable).isFalse();
+ }
+
private SubmitRecord.Status statusForRule() throws Exception {
String oldHead = projectOperations.project(project).getHead("master").name();
PushOneCommit.Result result =
diff --git a/javatests/com/google/gerrit/server/IdentifiedUserTest.java b/javatests/com/google/gerrit/server/IdentifiedUserTest.java
index 46027a2..f726be3 100644
--- a/javatests/com/google/gerrit/server/IdentifiedUserTest.java
+++ b/javatests/com/google/gerrit/server/IdentifiedUserTest.java
@@ -103,6 +103,7 @@
Account account =
Account.builder(Account.id(1), TimeUtil.now())
.setMetaId("1234567812345678123456781234567812345678")
+ .setUniqueTag("1234567812345678123456781234567812345678")
.build();
Account.Id ownerId = account.id();
diff --git a/javatests/com/google/gerrit/server/account/AccountCacheTest.java b/javatests/com/google/gerrit/server/account/AccountCacheTest.java
index 6628362..45eff9a 100644
--- a/javatests/com/google/gerrit/server/account/AccountCacheTest.java
+++ b/javatests/com/google/gerrit/server/account/AccountCacheTest.java
@@ -41,12 +41,15 @@
@Test
public void account_roundTrip() throws Exception {
+ // The uniqueTag and metaId can be different (in google internal implementation).
+ // This tests ensures that they are serialized/deserialized separately.
Account account =
Account.builder(Account.id(1), Instant.EPOCH)
.setFullName("foo bar")
.setDisplayName("foo")
.setActive(false)
.setMetaId("dead..beef")
+ .setUniqueTag("dead..beef..tag")
.setStatus("OOO")
.setPreferredEmail("foo@bar.tld")
.build();
@@ -63,6 +66,7 @@
.setDisplayName("foo")
.setInactive(true)
.setMetaId("dead..beef")
+ .setUniqueTag("dead..beef..tag")
.setStatus("OOO")
.setPreferredEmail("foo@bar.tld"))
.build();
@@ -71,6 +75,39 @@
}
@Test
+ public void account_deserializeOldRecordWithoutUniqueTag() throws Exception {
+ Account.Builder builder =
+ Account.builder(Account.id(1), Instant.EPOCH)
+ .setFullName("foo bar")
+ .setDisplayName("foo")
+ .setActive(false)
+ .setMetaId("dead..beef")
+ .setStatus("OOO")
+ .setPreferredEmail("foo@bar.tld");
+ CachedAccountDetails original =
+ CachedAccountDetails.create(builder.build(), ImmutableMap.of(), CachedPreferences.EMPTY);
+ CachedAccountDetails expected =
+ CachedAccountDetails.create(
+ builder.setUniqueTag("dead..beef").build(), ImmutableMap.of(), CachedPreferences.EMPTY);
+ byte[] serialized = SERIALIZER.serialize(original);
+ Cache.AccountDetailsProto expectedProto =
+ Cache.AccountDetailsProto.newBuilder()
+ .setAccount(
+ Cache.AccountProto.newBuilder()
+ .setId(1)
+ .setRegisteredOn(0)
+ .setFullName("foo bar")
+ .setDisplayName("foo")
+ .setInactive(true)
+ .setMetaId("dead..beef")
+ .setStatus("OOO")
+ .setPreferredEmail("foo@bar.tld"))
+ .build();
+ ProtoTruth.assertThat(Cache.AccountDetailsProto.parseFrom(serialized)).isEqualTo(expectedProto);
+ Truth.assertThat(SERIALIZER.deserialize(serialized)).isEqualTo(expected);
+ }
+
+ @Test
public void account_roundTripNullFields() throws Exception {
CachedAccountDetails original =
CachedAccountDetails.create(ACCOUNT, ImmutableMap.of(), CachedPreferences.EMPTY);
diff --git a/javatests/com/google/gerrit/server/account/AccountResolverTest.java b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
index 34f746a..a53abfa 100644
--- a/javatests/com/google/gerrit/server/account/AccountResolverTest.java
+++ b/javatests/com/google/gerrit/server/account/AccountResolverTest.java
@@ -351,6 +351,7 @@
return AccountState.forAccount(
Account.builder(Account.id(id), TimeUtil.now())
.setMetaId("1234567812345678123456781234567812345678")
+ .setUniqueTag("1234567812345678123456781234567812345678")
.build());
}
diff --git a/plugins/replication b/plugins/replication
index 2f6c7ce..012f042 160000
--- a/plugins/replication
+++ b/plugins/replication
@@ -1 +1 @@
-Subproject commit 2f6c7ceeb0cc50bc73d018cd9f990392d58804ab
+Subproject commit 012f04240eafe6dfa21fd94e012e97498881c621
diff --git a/polygerrit-ui/app/constants/reporting.ts b/polygerrit-ui/app/constants/reporting.ts
index dded0c6..37a17ba 100644
--- a/polygerrit-ui/app/constants/reporting.ts
+++ b/polygerrit-ui/app/constants/reporting.ts
@@ -146,6 +146,7 @@
GENERATE_SUGGESTION_ENABLED = 'generate_suggestion_enabled',
// User disabled generating suggestions
GENERATE_SUGGESTION_DISABLED = 'generate_suggestion_disabled',
+ GENERATE_SUGGESTION_EDITED = 'generate_suggestion_edited',
START_REVIEW = 'start-review',
CODE_REVIEW_APPROVAL = 'code-review-approval',
FILE_LIST_DIFF_COLLAPSED = 'file-list-diff-collapsed',
diff --git a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts
index df04f68..68fbe8b 100644
--- a/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts
+++ b/polygerrit-ui/app/elements/change/gr-messages-list/gr-messages-list_test.ts
@@ -33,7 +33,12 @@
import {GrButton} from '../../shared/gr-button/gr-button';
import {PaperToggleButtonElement} from '@polymer/paper-toggle-button';
import {testResolver} from '../../../test/common-test-setup';
-import {commentsModelToken} from '../../../models/comments/comments-model';
+import {TEST_PROJECT_NAME} from '../../../test/test-data-generators';
+import {
+ ChangeChildView,
+ changeViewModelToken,
+} from '../../../models/views/change';
+import {GerritView} from '../../../services/router/router-model';
const author = {
_account_id: 42 as AccountId,
@@ -138,9 +143,12 @@
element = await fixture<GrMessagesList>(
html`<gr-messages-list></gr-messages-list>`
);
- await testResolver(commentsModelToken).reloadComments(
- 0 as NumericChangeId
- );
+ testResolver(changeViewModelToken).setState({
+ view: GerritView.CHANGE,
+ childView: ChangeChildView.OVERVIEW,
+ changeNum: 123 as NumericChangeId,
+ repo: TEST_PROJECT_NAME,
+ });
element.messages = messages;
await element.updateComplete;
});
diff --git a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
index 1a18e9c..0bf49db 100644
--- a/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
+++ b/polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog.ts
@@ -98,10 +98,7 @@
} from '../../../utils/attention-set-util';
import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
import {resolve} from '../../../models/dependency';
-import {
- changeModelToken,
- updateRevisionsWithCommitShas,
-} from '../../../models/change/change-model';
+import {changeModelToken} from '../../../models/change/change-model';
import {LabelNameToValuesMap, PatchSetNumber} from '../../../api/rest-api';
import {css, html, PropertyValues, LitElement, nothing} from 'lit';
import {sharedStyles} from '../../../styles/shared-styles';
@@ -1462,10 +1459,8 @@
return this.saveReview(reviewInput, errFn)
.then(result => {
this.getChangeModel().updateStateChange(
- updateRevisionsWithCommitShas(
- GrReviewerUpdatesParser.parse(
- result?.change_info as ChangeViewChangeInfo
- )
+ GrReviewerUpdatesParser.parse(
+ result?.change_info as ChangeViewChangeInfo
)
);
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 16a783c..48d7b1b 100644
--- a/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
+++ b/polygerrit-ui/app/elements/shared/gr-comment/gr-comment.ts
@@ -232,6 +232,9 @@
generatedSuggestionId?: string;
@state()
+ addedGeneratedSuggestion?: string;
+
+ @state()
suggestionsProvider?: SuggestionsProvider;
@state()
@@ -1149,9 +1152,10 @@
private handleAddGeneratedSuggestion(code: string) {
const addNewLine = this.messageText.length !== 0;
- this.messageText += `${
+ this.addedGeneratedSuggestion = `${
addNewLine ? '\n' : ''
}${USER_SUGGESTION_START_PATTERN}${code}${'\n```'}`;
+ this.messageText += this.addedGeneratedSuggestion;
}
private generateSuggestEdit() {
@@ -1665,6 +1669,7 @@
} else {
// No need to make a backend call when nothing has changed.
while (this.somethingToSave()) {
+ this.trackGeneratedSuggestionEdit();
this.comment = await this.rawSave({showToast: true});
if (isError(this.comment)) return;
}
@@ -1746,6 +1751,19 @@
);
this.closeDeleteCommentModal();
}
+
+ private trackGeneratedSuggestionEdit() {
+ const wasGeneratedSuggestionEdited =
+ this.addedGeneratedSuggestion &&
+ !this.messageText.includes(this.addedGeneratedSuggestion);
+ if (wasGeneratedSuggestionEdited) {
+ this.reporting.reportInteraction(Interaction.GENERATE_SUGGESTION_EDITED, {
+ uuid: this.generatedSuggestionId,
+ commentId: this.comment?.id ?? '',
+ });
+ this.addedGeneratedSuggestion = undefined;
+ }
+ }
}
declare global {
diff --git a/polygerrit-ui/app/models/change/change-model.ts b/polygerrit-ui/app/models/change/change-model.ts
index 2414107..bba7e47 100644
--- a/polygerrit-ui/app/models/change/change-model.ts
+++ b/polygerrit-ui/app/models/change/change-model.ts
@@ -19,7 +19,13 @@
} from '../../types/common';
import {ChangeStatus, DefaultBase} from '../../constants/constants';
import {combineLatest, from, Observable, forkJoin, of} from 'rxjs';
-import {map, filter, withLatestFrom, switchMap} from 'rxjs/operators';
+import {
+ map,
+ filter,
+ withLatestFrom,
+ switchMap,
+ catchError,
+} from 'rxjs/operators';
import {
computeAllPatchSets,
computeLatestPatchNum,
@@ -55,7 +61,7 @@
export interface ChangeState {
/**
* If `change` is undefined, this must be either NOT_LOADED or LOADING.
- * If `change` is defined, this must be either LOADED or RELOADING.
+ * If `change` is defined, this must be either LOADED.
*/
loadingStatus: LoadingStatus;
change?: ParsedChangeInfo;
@@ -177,7 +183,13 @@
};
export const changeModelToken = define<ChangeModel>('change-model');
-
+/**
+ * Change model maintains information about the current change.
+ *
+ * The "current" change is defined by ChangeViewModel. This model tracks part of
+ * the current view. As such it's a singleton global state. It's NOT meant to
+ * keep the state of an arbitrary change.
+ */
export class ChangeModel extends Model<ChangeState> {
private change?: ParsedChangeInfo;
@@ -199,8 +211,7 @@
public readonly loading$ = select(
this.changeLoadingStatus$,
- status =>
- status === LoadingStatus.LOADING || status === LoadingStatus.RELOADING
+ status => status === LoadingStatus.LOADING
);
public readonly reviewedFiles$ = select(
@@ -388,10 +399,7 @@
private reportChangeReload() {
return this.changeLoadingStatus$.subscribe(loadingStatus => {
- if (
- loadingStatus === LoadingStatus.LOADING ||
- loadingStatus === LoadingStatus.RELOADING
- ) {
+ if (loadingStatus === LoadingStatus.LOADING) {
this.reporting.time(Timing.CHANGE_RELOAD);
}
if (
@@ -557,16 +565,21 @@
return this.viewModel.changeNum$
.pipe(
switchMap(changeNum => {
- if (changeNum !== undefined) this.updateStateLoading(changeNum);
- const change = from(this.restApiService.getChangeDetail(changeNum));
- const edit = from(this.restApiService.getChangeEdit(changeNum));
+ this.updateStateLoading(changeNum);
+ // if changeNum is undefined restApi calls return undefined.
+ const change = this.restApiService.getChangeDetail(changeNum);
+ const edit = this.restApiService.getChangeEdit(changeNum);
return forkJoin([change, edit]);
}),
withLatestFrom(this.viewModel.patchNum$),
map(([[change, edit], patchNum]) =>
updateChangeWithEdit(change, edit, patchNum)
),
- map(updateRevisionsWithCommitShas)
+ catchError(err => {
+ // Reset loading state and re-throw.
+ this.updateState({loadingStatus: LoadingStatus.NOT_LOADED});
+ throw err;
+ })
)
.subscribe(change => {
// The change service is currently a singleton, so we have to be
@@ -752,25 +765,33 @@
/**
* Called when change detail loading is initiated.
*
- * If the change number matches the current change in the state, then
- * this is a reload. If not, then we not just want to set the state to
- * LOADING instead of RELOADING, but we also want to set the change to
+ * We want to set the state to LOADING, but we also want to set the change to
* undefined right away. Otherwise components could see inconsistent state:
* a new change number, but an old change.
*/
- private updateStateLoading(changeNum: NumericChangeId) {
- const current = this.getState();
- const reloading = current.change?._number === changeNum;
+ private updateStateLoading(changeNum?: NumericChangeId) {
this.updateState({
- change: reloading ? current.change : undefined,
- loadingStatus: reloading
- ? LoadingStatus.RELOADING
- : LoadingStatus.LOADING,
+ change: undefined,
+ loadingStatus: changeNum
+ ? LoadingStatus.LOADING
+ : LoadingStatus.NOT_LOADED,
});
}
// Private but used in tests.
+ /**
+ * Update the change information in the state.
+ *
+ * Since the ChangeModel must maintain consistency with ChangeViewModel
+ * The update is only allowed, if the new change has the same number as the
+ * current change or if the current change is not set (it was reset to
+ * undefined when ChangeViewModel.changeNum updated).
+ */
updateStateChange(change?: ParsedChangeInfo) {
+ if (this.change && change?._number !== this.change?._number) {
+ return;
+ }
+ change = updateRevisionsWithCommitShas(change);
this.updateState({
change,
loadingStatus:
diff --git a/polygerrit-ui/app/models/change/change-model_test.ts b/polygerrit-ui/app/models/change/change-model_test.ts
index f074ac3..ebe6066 100644
--- a/polygerrit-ui/app/models/change/change-model_test.ts
+++ b/polygerrit-ui/app/models/change/change-model_test.ts
@@ -306,11 +306,11 @@
// Reloading same change
document.dispatchEvent(new CustomEvent('reload'));
- state = await waitForLoadingStatus(LoadingStatus.RELOADING);
+ state = await waitForLoadingStatus(LoadingStatus.LOADING);
assert.equal(stub.callCount, 3);
assert.equal(stub.getCall(1).firstArg, undefined);
assert.equal(stub.getCall(2).firstArg, TEST_NUMERIC_CHANGE_ID);
- assert.deepEqual(state?.change, updateRevisionsWithCommitShas(knownChange));
+ assert.deepEqual(state?.change, undefined);
promise.resolve(knownChange);
state = await waitForLoadingStatus(LoadingStatus.LOADED);
diff --git a/polygerrit-ui/app/models/checks/checks-model_test.ts b/polygerrit-ui/app/models/checks/checks-model_test.ts
index 767e604..a8eda0f 100644
--- a/polygerrit-ui/app/models/checks/checks-model_test.ts
+++ b/polygerrit-ui/app/models/checks/checks-model_test.ts
@@ -30,12 +30,16 @@
} from '../../test/test-data-generators';
import {waitUntil, waitUntilCalled} from '../../test/test-utils';
import {ParsedChangeInfo} from '../../types/types';
-import {changeModelToken} from '../change/change-model';
+import {
+ changeModelToken,
+ updateRevisionsWithCommitShas,
+} from '../change/change-model';
import {assert} from '@open-wc/testing';
import {testResolver} from '../../test/common-test-setup';
import {changeViewModelToken} from '../views/change';
import {NumericChangeId, PatchSetNumber} from '../../api/rest-api';
import {pluginLoaderToken} from '../../elements/shared/gr-js-api-interface/gr-plugin-loader';
+import {deepEqual} from '../../utils/deep-util';
const PLUGIN_NAME = 'test-plugin';
@@ -106,17 +110,17 @@
});
await waitUntil(() => change === undefined);
- const testChange = createParsedChange();
+ const testChange = updateRevisionsWithCommitShas(createParsedChange());
testResolver(changeModelToken).updateStateChange(testChange);
- await waitUntil(() => change === testChange);
+ await waitUntil(() => deepEqual(change, testChange));
await waitUntilCalled(fetchSpy, 'fetch');
assert.equal(
model.latestPatchNum,
- testChange.revisions[testChange.current_revision]
+ testChange!.revisions[testChange!.current_revision]
._number as PatchSetNumber
);
- assert.equal(model.changeNum, testChange._number);
+ assert.equal(model.changeNum, testChange!._number);
});
test('fetch throttle', async () => {
@@ -133,9 +137,9 @@
});
await waitUntil(() => change === undefined);
- const testChange = createParsedChange();
+ const testChange = updateRevisionsWithCommitShas(createParsedChange());
testResolver(changeModelToken).updateStateChange(testChange);
- await waitUntil(() => change === testChange);
+ await waitUntil(() => deepEqual(change, testChange));
model.reload('test-plugin');
model.reload('test-plugin');
@@ -339,9 +343,9 @@
});
await waitUntil(() => change === undefined);
clock.tick(1);
- const testChange = createParsedChange();
+ const testChange = updateRevisionsWithCommitShas(createParsedChange());
testResolver(changeModelToken).updateStateChange(testChange);
- await waitUntil(() => change === testChange);
+ await waitUntil(() => deepEqual(change, testChange));
clock.tick(600); // need to wait for 500ms throttle
await waitUntilCalled(fetchSpy, 'fetch');
const pollCount = fetchSpy.callCount;
@@ -366,9 +370,9 @@
});
await waitUntil(() => change === undefined);
clock.tick(1);
- const testChange = createParsedChange();
+ const testChange = updateRevisionsWithCommitShas(createParsedChange());
testResolver(changeModelToken).updateStateChange(testChange);
- await waitUntil(() => change === testChange);
+ await waitUntil(() => deepEqual(change, testChange));
clock.tick(600); // need to wait for 500ms throttle
await waitUntilCalled(fetchSpy, 'fetch');
clock.tick(1);
diff --git a/polygerrit-ui/app/models/comments/comments-model.ts b/polygerrit-ui/app/models/comments/comments-model.ts
index f26a565..41b47ef 100644
--- a/polygerrit-ui/app/models/comments/comments-model.ts
+++ b/polygerrit-ui/app/models/comments/comments-model.ts
@@ -7,7 +7,6 @@
import {
CommentInfo,
NumericChangeId,
- PatchSetNum,
RevisionId,
UrlEncodedCommentId,
RobotCommentInfo,
@@ -34,7 +33,14 @@
import {deepEqual} from '../../utils/deep-util';
import {select} from '../../utils/observable-util';
import {define} from '../dependency';
-import {combineLatest, forkJoin, from, Observable, of} from 'rxjs';
+import {
+ BehaviorSubject,
+ combineLatest,
+ forkJoin,
+ from,
+ Observable,
+ of,
+} from 'rxjs';
import {fire, fireAlert} from '../../utils/event-util';
import {CURRENT} from '../../utils/patch-set-util';
import {RestApiService} from '../../services/gr-rest-api/gr-rest-api';
@@ -69,9 +75,20 @@
drafts?: {[path: string]: DraftInfo[]};
// Ported comments only affect `CommentThread` properties, not individual
// comments.
- /** undefined means 'still loading' */
+ /**
+ * Comments ported from earlier patchsets.
+ *
+ * This only considers current patchset (right side), not the base patchset
+ * (left-side).
+ *
+ * undefined means 'still loading'
+ */
portedComments?: {[path: string]: CommentInfo[]};
- /** undefined means 'still loading' */
+ /**
+ * Drafts ported from earlier patchsets.
+ *
+ * undefined means 'still loading'
+ */
portedDrafts?: {[path: string]: DraftInfo[]};
/**
* If a draft is discarded by the user, then we temporarily keep it in this
@@ -216,7 +233,7 @@
return nextState;
}
-/** Adds or updates a draft. */
+/** Adds or updates a draft in the state. */
export function setDraft(state: CommentState, draft: DraftInfo): CommentState {
const nextState = {...state};
assert(!!draft.path, 'draft without path');
@@ -235,6 +252,12 @@
return nextState;
}
+/** Removes a draft from the state.
+ *
+ * Removed draft is stored in discardedDrafts for potential undo operation.
+ * discardedDrafts however is only a client-side cache and such drafts are not
+ * retained in the server.
+ */
export function deleteDraft(
state: CommentState,
draft: DraftInfo
@@ -254,6 +277,10 @@
}
export const commentsModelToken = define<CommentsModel>('comments-model');
+/**
+ * Model that maintains the state of all comments and drafts for the current
+ * change in the context of change-view.
+ */
export class CommentsModel extends Model<CommentState> {
public readonly commentsLoading$ = select(
this.state$,
@@ -415,6 +442,8 @@
}
);
+ public readonly reloadAllComments$ = new BehaviorSubject(undefined);
+
public thread$(id: UrlEncodedCommentId) {
return select(this.threads$, threads => threads.find(t => t.rootId === id));
}
@@ -423,8 +452,6 @@
private changeNum?: NumericChangeId;
- private patchNum?: PatchSetNum;
-
private drafts: {[path: string]: DraftInfo[]} = {};
private draftToastTask?: DelayedTask;
@@ -464,9 +491,8 @@
this.subscriptions.push(
this.drafts$.subscribe(x => (this.drafts = x ?? {}))
);
- this.subscriptions.push(
- this.changeModel.patchNum$.subscribe(x => (this.patchNum = x))
- );
+ // Patchset-level draft should always exist when opening reply dialog.
+ // If there are none, create an empty one.
this.subscriptions.push(
combineLatest([
this.draftsLoading$,
@@ -478,41 +504,55 @@
})
);
this.subscriptions.push(
- this.changeViewModel.changeNum$.subscribe(changeNum => {
- this.changeNum = changeNum;
- this.setState({...initialState});
- this.reloadAllComments();
- })
+ combineLatest([this.changeViewModel.changeNum$, this.reloadAllComments$])
+ .pipe(
+ switchMap(([changeNum, _]) => {
+ this.changeNum = changeNum;
+ this.setState({...initialState});
+ if (!changeNum) return of([undefined, undefined, undefined]);
+ return forkJoin([
+ this.restApiService.getDiffComments(changeNum),
+ this.restApiService.getDiffRobotComments(changeNum),
+ this.restApiService.getDiffDrafts(changeNum),
+ ]);
+ })
+ )
+ .subscribe(([comments, robotComments, drafts]) => {
+ this.reportRobotCommentStats(robotComments);
+ this.modifyState(s => {
+ s = setComments(s, comments);
+ s = setRobotComments(s, robotComments);
+ return setDrafts(s, drafts);
+ });
+ })
);
+ // When the patchset selection changes update information about comments
+ // ported from earlier patchsets.
this.subscriptions.push(
- combineLatest([
- this.changeModel.changeNum$,
- this.changeModel.patchNum$,
- ]).subscribe(([changeNum, patchNum]) => {
- this.changeNum = changeNum;
- this.patchNum = patchNum;
- this.reloadAllPortedComments();
- })
+ combineLatest([this.changeModel.changeNum$, this.changeModel.patchNum$])
+ .pipe(
+ switchMap(([changeNum, patchNum]) => {
+ this.changeNum = changeNum;
+ if (!changeNum) return of([undefined, undefined]);
+ const revision = patchNum ?? (CURRENT as RevisionId);
+ return forkJoin([
+ this.restApiService.getPortedComments(changeNum, revision),
+ this.restApiService.getPortedDrafts(changeNum, revision),
+ ]);
+ })
+ )
+ .subscribe(([portedComments, portedDrafts]) =>
+ this.modifyState(s => {
+ s = setPortedComments(s, portedComments);
+ return setPortedDrafts(s, portedDrafts);
+ })
+ )
);
}
// Note that this does *not* reload ported comments.
- async reloadAllComments() {
- if (!this.changeNum) return;
- await Promise.all([
- this.reloadComments(this.changeNum),
- this.reloadRobotComments(this.changeNum),
- this.reloadDrafts(this.changeNum),
- ]);
- }
-
- async reloadAllPortedComments() {
- if (!this.changeNum) return;
- if (!this.patchNum) return;
- await Promise.all([
- this.reloadPortedComments(this.changeNum, this.patchNum),
- this.reloadPortedDrafts(this.changeNum, this.patchNum),
- ]);
+ reloadAllComments() {
+ this.reloadAllComments$.next(undefined);
}
// visible for testing
@@ -520,19 +560,6 @@
this.setState(reducer({...this.getState()}));
}
- async reloadComments(changeNum: NumericChangeId): Promise<void> {
- const comments = await this.restApiService.getDiffComments(changeNum);
- this.modifyState(s => setComments(s, comments));
- }
-
- async reloadRobotComments(changeNum: NumericChangeId): Promise<void> {
- const robotComments = await this.restApiService.getDiffRobotComments(
- changeNum
- );
- this.reportRobotCommentStats(robotComments);
- this.modifyState(s => setRobotComments(s, robotComments));
- }
-
private reportRobotCommentStats(obj?: PathToRobotCommentsInfoMap) {
if (!obj) return;
const comments = Object.values(obj).flat();
@@ -561,33 +588,6 @@
);
}
- async reloadDrafts(changeNum: NumericChangeId): Promise<void> {
- const drafts = await this.restApiService.getDiffDrafts(changeNum);
- this.modifyState(s => setDrafts(s, drafts));
- }
-
- async reloadPortedComments(
- changeNum: NumericChangeId,
- patchNum = CURRENT as RevisionId
- ): Promise<void> {
- const portedComments = await this.restApiService.getPortedComments(
- changeNum,
- patchNum
- );
- this.modifyState(s => setPortedComments(s, portedComments));
- }
-
- async reloadPortedDrafts(
- changeNum: NumericChangeId,
- patchNum = CURRENT as RevisionId
- ): Promise<void> {
- const portedDrafts = await this.restApiService.getPortedDrafts(
- changeNum,
- patchNum
- );
- this.modifyState(s => setPortedDrafts(s, portedDrafts));
- }
-
async restoreDraft(draftId: UrlEncodedCommentId) {
const found = this.discardedDrafts?.find(d => id(d) === draftId);
if (!found) throw new Error('discarded draft not found');
diff --git a/polygerrit-ui/app/rules.bzl b/polygerrit-ui/app/rules.bzl
index 9ab0f64..bdfb870 100644
--- a/polygerrit-ui/app/rules.bzl
+++ b/polygerrit-ui/app/rules.bzl
@@ -1,5 +1,5 @@
-load("//tools/bzl:genrule2.bzl", "genrule2")
load("@npm//@bazel/rollup:index.bzl", "rollup_bundle")
+load("//tools/bzl:genrule2.bzl", "genrule2")
def polygerrit_bundle(name, srcs, outs, entry_point, app_name):
"""Build .zip bundle from source code
diff --git a/polygerrit-ui/app/types/types.ts b/polygerrit-ui/app/types/types.ts
index 43e06a4..f48588b 100644
--- a/polygerrit-ui/app/types/types.ts
+++ b/polygerrit-ui/app/types/types.ts
@@ -41,7 +41,6 @@
export enum LoadingStatus {
NOT_LOADED = 'NOT_LOADED',
LOADING = 'LOADING',
- RELOADING = 'RELOADING',
LOADED = 'LOADED',
}
diff --git a/proto/cache.proto b/proto/cache.proto
index e4273bf..09c99df 100644
--- a/proto/cache.proto
+++ b/proto/cache.proto
@@ -355,6 +355,7 @@
bool inactive = 6;
string status = 7;
string meta_id = 8;
+ string unique_tag = 9;
}
// Serialized form of com.google.gerrit.server.account.CachedAccountDetails.Key.
diff --git a/tools/bzl/plugin.bzl b/tools/bzl/plugin.bzl
index 9e515e5..5bacec8 100644
--- a/tools/bzl/plugin.bzl
+++ b/tools/bzl/plugin.bzl
@@ -1,6 +1,10 @@
+"""
+Build rules for plugins.
+"""
+
+load("//:version.bzl", "GERRIT_VERSION")
load("@rules_java//java:defs.bzl", "java_binary", "java_library")
load("//tools/bzl:genrule2.bzl", "genrule2")
-load("//:version.bzl", "GERRIT_VERSION")
IN_TREE_BUILD_MODE = True
diff --git a/tools/util.py b/tools/util.py
index 947e2c0..aed9e91 100644
--- a/tools/util.py
+++ b/tools/util.py
@@ -16,8 +16,10 @@
REPO_ROOTS = {
'ECLIPSE': 'https://repo.eclipse.org/content/groups/releases',
+ 'ECLIPSE_EGIT': 'https://repo.eclipse.org/content/repositories/egit-releases',
'GERRIT': 'https://gerrit-maven.storage.googleapis.com',
'GERRIT_API': 'https://gerrit-api.commondatastorage.googleapis.com/release',
+ 'JENKINS': 'https://repo.jenkins-ci.org/artifactory/public',
'MAVEN_CENTRAL': 'https://repo1.maven.org/maven2',
'MAVEN_LOCAL': 'file://' + path.expanduser('~/.m2/repository'),
'MAVEN_SNAPSHOT': 'https://oss.sonatype.org/content/repositories/snapshots',
