Merge "Merge branch 'stable-3.9' into master"
diff --git a/java/com/google/gerrit/entities/Change.java b/java/com/google/gerrit/entities/Change.java
index 56fb748..fad3aa8 100644
--- a/java/com/google/gerrit/entities/Change.java
+++ b/java/com/google/gerrit/entities/Change.java
@@ -433,6 +433,9 @@
/** Locally assigned unique identifier of the change */
private Id changeId;
+ /** ServerId of the Gerrit instance that has created the change */
+ private String serverId;
+
/** Globally assigned unique identifier of the change */
private Key changeKey;
@@ -530,6 +533,22 @@
return changeId;
}
+ /**
+ * Set the serverId of the Gerrit instance that created the change. It can be set to null for
+ * testing purposes in the protobuf converter tests.
+ */
+ public void setServerId(@Nullable String serverId) {
+ this.serverId = serverId;
+ }
+
+ /**
+ * ServerId of the Gerrit instance that created the change. It could be null when the change is
+ * not fetched from NoteDb but obtained through protobuf deserialisation.
+ */
+ public @Nullable String getServerId() {
+ return serverId;
+ }
+
/** 32 bit integer identity for a change. */
public int getChangeId() {
return changeId.get();
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java
index 33a2f34..4a3b423 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -102,6 +102,7 @@
public Boolean containsGitConflicts;
public Integer _number;
+ public Integer _virtualIdNumber;
public AccountInfo owner;
diff --git a/java/com/google/gerrit/index/QueryOptions.java b/java/com/google/gerrit/index/QueryOptions.java
index bee8fa1..632e469 100644
--- a/java/com/google/gerrit/index/QueryOptions.java
+++ b/java/com/google/gerrit/index/QueryOptions.java
@@ -204,4 +204,8 @@
allowIncompleteResults(),
filter.apply(this));
}
+
+ public int getLimitBasedOnPaginationType() {
+ return PaginationType.NONE == config().paginationType() ? limit() : pageSize();
+ }
}
diff --git a/java/com/google/gerrit/index/query/FilteredSource.java b/java/com/google/gerrit/index/query/FilteredSource.java
index 9746850..95e6435 100644
--- a/java/com/google/gerrit/index/query/FilteredSource.java
+++ b/java/com/google/gerrit/index/query/FilteredSource.java
@@ -21,6 +21,7 @@
import com.google.common.collect.Iterables;
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.index.IndexConfig;
+import com.google.gerrit.index.QueryOptions;
import java.util.ArrayList;
import java.util.List;
@@ -51,11 +52,36 @@
return new LazyResultSet<>(
() -> {
List<T> r = new ArrayList<>();
+ T last = null;
+ int pageResultSize = 0;
for (T data : buffer(resultSet)) {
if (!isMatchable() || match(data)) {
r.add(data);
}
+ last = data;
+ pageResultSize++;
}
+
+ if (last != null && source instanceof Paginated) {
+ // Restart source and continue if we have not filled the
+ // full limit the caller wants.
+ Paginated<T> p = (Paginated<T>) source;
+ QueryOptions opts = p.getOptions();
+ final int limit = opts.limit();
+ int nextStart = pageResultSize;
+ while (pageResultSize == limit && r.size() < limit) {
+ ResultSet<T> next = p.restart(nextStart);
+ pageResultSize = 0;
+ for (T data : buffer(next)) {
+ if (match(data)) {
+ r.add(data);
+ }
+ pageResultSize++;
+ }
+ nextStart += pageResultSize;
+ }
+ }
+
if (start >= r.size()) {
return ImmutableList.of();
} else if (start > 0) {
diff --git a/java/com/google/gerrit/index/query/IndexedQuery.java b/java/com/google/gerrit/index/query/IndexedQuery.java
index ee25ef9..cb98c06 100644
--- a/java/com/google/gerrit/index/query/IndexedQuery.java
+++ b/java/com/google/gerrit/index/query/IndexedQuery.java
@@ -87,6 +87,12 @@
}
@Override
+ public ResultSet<T> restart(int start) {
+ opts = opts.withStart(start);
+ return search();
+ }
+
+ @Override
public ResultSet<T> restart(int start, int pageSize) {
opts = opts.withStart(start).withPageSize(pageSize);
return search();
diff --git a/java/com/google/gerrit/index/query/Paginated.java b/java/com/google/gerrit/index/query/Paginated.java
index 5521990..1065019 100644
--- a/java/com/google/gerrit/index/query/Paginated.java
+++ b/java/com/google/gerrit/index/query/Paginated.java
@@ -19,6 +19,8 @@
public interface Paginated<T> {
QueryOptions getOptions();
+ ResultSet<T> restart(int start);
+
ResultSet<T> restart(int start, int pageSize);
ResultSet<T> restart(Object searchAfter, int pageSize);
diff --git a/java/com/google/gerrit/index/query/PaginatingSource.java b/java/com/google/gerrit/index/query/PaginatingSource.java
index be789ee..19251ca 100644
--- a/java/com/google/gerrit/index/query/PaginatingSource.java
+++ b/java/com/google/gerrit/index/query/PaginatingSource.java
@@ -54,7 +54,6 @@
if (last != null && source instanceof Paginated) {
// Restart source and continue if we have not filled the
// full limit the caller wants.
- //
@SuppressWarnings("unchecked")
Paginated<T> p = (Paginated<T>) source;
QueryOptions opts = p.getOptions();
diff --git a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
index 65c02d9..570290e 100644
--- a/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
+++ b/java/com/google/gerrit/index/testing/AbstractFakeIndex.java
@@ -150,10 +150,14 @@
.findFirst()
.orElse(-1)
+ 1;
- int toIndex = Math.min(fromIndex + opts.pageSize(), valueList.size());
+ int toIndex = Math.min(fromIndex + opts.getLimitBasedOnPaginationType(), valueList.size());
results = valueList.subList(fromIndex, toIndex);
} else {
- results = valueStream.skip(opts.start()).limit(opts.pageSize()).collect(toImmutableList());
+ results =
+ valueStream
+ .skip(opts.start())
+ .limit(opts.getLimitBasedOnPaginationType())
+ .collect(toImmutableList());
}
queryCount++;
resultsSizes.add(results.size());
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index 9fcf5ae..513abad 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -113,7 +113,7 @@
private static final String CHANGE_FIELD = ChangeField.CHANGE_SPEC.getName();
static Term idTerm(ChangeData cd) {
- return idTerm(cd.getVirtualId());
+ return idTerm(cd.virtualId());
}
static Term idTerm(Change.Id id) {
@@ -405,11 +405,11 @@
IndexSearcher[] searchers = new IndexSearcher[indexes.size()];
Map<ChangeSubIndex, ScoreDoc> searchAfterBySubIndex = new HashMap<>();
try {
- int realPageSize = opts.start() + opts.pageSize();
- if (Integer.MAX_VALUE - opts.pageSize() < opts.start()) {
- realPageSize = Integer.MAX_VALUE;
+ int pageLimit = AbstractLuceneIndex.getLimitBasedOnPaginationType(opts, opts.pageSize());
+ int queryLimit = opts.start() + pageLimit;
+ if (Integer.MAX_VALUE - pageLimit < opts.start()) {
+ queryLimit = Integer.MAX_VALUE;
}
- int queryLimit = AbstractLuceneIndex.getLimitBasedOnPaginationType(opts, realPageSize);
List<TopFieldDocs> hits = new ArrayList<>();
int searchAfterHitsCount = 0;
for (int i = 0; i < indexes.size(); i++) {
@@ -417,7 +417,7 @@
searchers[i] = subIndex.acquire();
if (isSearchAfterPagination) {
ScoreDoc searchAfter = getSearchAfter(subIndex);
- int maxRemainingHits = realPageSize - searchAfterHitsCount;
+ int maxRemainingHits = queryLimit - searchAfterHitsCount;
if (maxRemainingHits > 0) {
TopFieldDocs subIndexHits =
searchers[i].searchAfter(
@@ -571,7 +571,13 @@
IndexableField cb = Iterables.getFirst(doc.get(CHANGE_FIELD), null);
if (cb != null) {
BytesRef proto = cb.binaryValue();
- cd = changeDataFactory.create(parseProtoFrom(proto, ChangeProtoConverter.INSTANCE));
+ // pass the id field value (which is the change virtual id for the imported changes) when
+ // available
+ IndexableField f = Iterables.getFirst(doc.get(idFieldName), null);
+ cd =
+ changeDataFactory.create(
+ parseProtoFrom(proto, ChangeProtoConverter.INSTANCE),
+ f != null ? Change.id(Integer.valueOf(f.stringValue())) : null);
} else {
IndexableField f = Iterables.getFirst(doc.get(idFieldName), null);
diff --git a/java/com/google/gerrit/server/account/AccountResource.java b/java/com/google/gerrit/server/account/AccountResource.java
index 9629809..14b363b 100644
--- a/java/com/google/gerrit/server/account/AccountResource.java
+++ b/java/com/google/gerrit/server/account/AccountResource.java
@@ -99,6 +99,10 @@
public Change getChange() {
return change.getChange();
}
+
+ public Change.Id getVirtualId() {
+ return change.getVirtualId();
+ }
}
public static class Star implements RestResource {
diff --git a/java/com/google/gerrit/server/change/ChangeJson.java b/java/com/google/gerrit/server/change/ChangeJson.java
index f5af631..0a1025e 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -511,6 +511,9 @@
if (has(REVIEWED) && userProvider.get().isIdentifiedUser()) {
ChangeData.ensureReviewedByLoadedForOpenChanges(all);
}
+ if (has(STAR) && userProvider.get().isIdentifiedUser()) {
+ ChangeData.ensureChangeServerId(all);
+ }
ChangeData.ensureCurrentApprovalsLoaded(all);
}
} else {
@@ -800,6 +803,8 @@
}
}
+ out._virtualIdNumber = cd.virtualId().get();
+
return out;
}
@@ -1063,14 +1068,15 @@
// repository only once
try (Repository allUsersRepo = repoManager.openRepository(allUsers)) {
List<Change.Id> changeIds =
- changeInfos.stream().map(c -> Change.id(c._number)).collect(Collectors.toList());
+ changeInfos.stream().map(c -> Change.id(c._virtualIdNumber)).collect(Collectors.toList());
Set<Change.Id> starredChanges =
starredChangesreader.areStarred(
allUsersRepo, changeIds, userProvider.get().asIdentifiedUser().getAccountId());
if (starredChanges.isEmpty()) {
return;
}
- changeInfos.stream().forEach(c -> c.starred = starredChanges.contains(Change.id(c._number)));
+ changeInfos.stream()
+ .forEach(c -> c.starred = starredChanges.contains(Change.id(c._virtualIdNumber)));
} catch (IOException e) {
logger.atWarning().withCause(e).log("Failed to open All-Users repo.");
}
diff --git a/java/com/google/gerrit/server/change/ChangeResource.java b/java/com/google/gerrit/server/change/ChangeResource.java
index dc09bf4..8300541 100644
--- a/java/com/google/gerrit/server/change/ChangeResource.java
+++ b/java/com/google/gerrit/server/change/ChangeResource.java
@@ -161,6 +161,10 @@
return changeData;
}
+ public Change.Id getVirtualId() {
+ return getChangeData().virtualId();
+ }
+
// This includes all information relevant for ETag computation
// unrelated to the UI.
public void prepareETag(Hasher h, CurrentUser user) {
@@ -237,7 +241,7 @@
.build())) {
Hasher h = Hashing.murmur3_128().newHasher();
if (user.isIdentifiedUser()) {
- h.putBoolean(starredChangesReader.isStarred(user.getAccountId(), getId()));
+ h.putBoolean(starredChangesReader.isStarred(user.getAccountId(), getVirtualId()));
}
prepareETag(h, user);
return h.hash().toString();
diff --git a/java/com/google/gerrit/server/change/DeleteChangeOp.java b/java/com/google/gerrit/server/change/DeleteChangeOp.java
index ba7bd90a..bcfa48a 100644
--- a/java/com/google/gerrit/server/change/DeleteChangeOp.java
+++ b/java/com/google/gerrit/server/change/DeleteChangeOp.java
@@ -81,7 +81,8 @@
ensureDeletable(ctx, id, patchSets);
// Cleaning up is only possible as long as the change and its elements are
// still part of the database.
- cleanUpReferences(id);
+ ChangeData cd = changeDataFactory.create(ctx.getChange());
+ cleanUpReferences(cd);
logger.atFine().log(
"Deleting change %s, current patch set %d is commit %s",
@@ -95,7 +96,7 @@
.map(p -> p.commitId().name())
.orElse("n/a")));
ctx.deleteChange();
- changeDeleted.fire(changeDataFactory.create(ctx.getChange()), ctx.getAccount(), ctx.getWhen());
+ changeDeleted.fire(cd, ctx.getAccount(), ctx.getWhen());
return true;
}
@@ -124,11 +125,11 @@
revWalk.parseCommit(patchSet.commitId()), revWalk.parseCommit(destId.get()));
}
- private void cleanUpReferences(Change.Id id) throws IOException {
- accountPatchReviewStore.run(s -> s.clearReviewed(id));
+ private void cleanUpReferences(ChangeData cd) throws IOException {
+ accountPatchReviewStore.run(s -> s.clearReviewed(cd.virtualId()));
// Non-atomic operation on All-Users refs; not much we can do to make it atomic.
- starredChangesWriter.unstarAllForChangeDeletion(id);
+ starredChangesWriter.unstarAllForChangeDeletion(cd.virtualId());
}
@Override
diff --git a/java/com/google/gerrit/server/index/change/ChangeField.java b/java/com/google/gerrit/server/index/change/ChangeField.java
index c5afba4..3ca536c 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -128,7 +128,7 @@
.required()
// The numeric change id is integer in string form
.size(10)
- .build(cd -> String.valueOf(cd.getVirtualId().get()));
+ .build(cd -> String.valueOf(cd.virtualId().get()));
public static final IndexedField<ChangeData, String>.SearchSpec NUMERIC_ID_STR_SPEC =
NUMERIC_ID_STR_FIELD.exact("legacy_id_str");
diff --git a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
index f7ff13c..00642a9 100644
--- a/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
+++ b/java/com/google/gerrit/server/index/change/IndexedChangeQuery.java
@@ -21,7 +21,6 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.gerrit.entities.Change;
import com.google.gerrit.index.IndexConfig;
@@ -52,10 +51,6 @@
*/
public class IndexedChangeQuery extends IndexedQuery<Change.Id, ChangeData>
implements ChangeDataSource, Matchable<ChangeData> {
- public static QueryOptions oneResult() {
- IndexConfig config = IndexConfig.createDefault();
- return createOptions(config, 0, 1, config.pageSizeMultiplier(), 1, ImmutableSet.of());
- }
public static QueryOptions createOptions(
IndexConfig config, int start, int limit, Set<String> fields) {
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index 7f0b068..35add5f 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -553,7 +553,7 @@
return getDraftComments(author, null);
}
- ImmutableList<HumanComment> getDraftComments(Account.Id author, @Nullable Ref ref) {
+ public ImmutableList<HumanComment> getDraftComments(Account.Id author, @Nullable Ref ref) {
loadDraftComments(author, ref);
// Filter out any zombie draft comments. These are drafts that are also in
// the published map, and arise when the update to All-Users to delete them
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotesState.java b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
index e012942..eb6c15a 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotesState.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotesState.java
@@ -364,6 +364,7 @@
change.setOwner(c.owner());
change.setDest(BranchNameKey.create(change.getProject(), c.branch()));
change.setCreatedOn(c.createdOn());
+ change.setServerId(serverId());
copyNonConstructorColumnsTo(change);
}
diff --git a/java/com/google/gerrit/server/notedb/StarredChangesUtilNoteDbImpl.java b/java/com/google/gerrit/server/notedb/StarredChangesUtilNoteDbImpl.java
index c52f082..f13b832 100644
--- a/java/com/google/gerrit/server/notedb/StarredChangesUtilNoteDbImpl.java
+++ b/java/com/google/gerrit/server/notedb/StarredChangesUtilNoteDbImpl.java
@@ -85,31 +85,31 @@
}
@Override
- public boolean isStarred(Account.Id accountId, Change.Id changeId) {
+ public boolean isStarred(Account.Id accountId, Change.Id virtualId) {
try (Repository repo = repoManager.openRepository(allUsers)) {
- return getStarRef(repo, RefNames.refsStarredChanges(changeId, accountId)).isPresent();
+ return getStarRef(repo, RefNames.refsStarredChanges(virtualId, accountId)).isPresent();
} catch (IOException e) {
throw new StorageException(
String.format(
"Reading stars from change %d for account %d failed",
- changeId.get(), accountId.get()),
+ virtualId.get(), accountId.get()),
e);
}
}
@Override
- public void star(Account.Id accountId, Change.Id changeId) {
- updateStar(accountId, changeId, true);
+ public void star(Account.Id accountId, Change.Id virtualId) {
+ updateStar(accountId, virtualId, true);
}
@Override
- public void unstar(Account.Id accountId, Change.Id changeId) {
- updateStar(accountId, changeId, false);
+ public void unstar(Account.Id accountId, Change.Id virtualId) {
+ updateStar(accountId, virtualId, false);
}
- private void updateStar(Account.Id accountId, Change.Id changeId, boolean shouldAdd) {
+ private void updateStar(Account.Id accountId, Change.Id virtualId, boolean shouldAdd) {
try (Repository repo = repoManager.openRepository(allUsers)) {
- String refName = RefNames.refsStarredChanges(changeId, accountId);
+ String refName = RefNames.refsStarredChanges(virtualId, accountId);
if (shouldAdd) {
addRef(repo, refName, null);
} else {
@@ -120,16 +120,16 @@
}
} catch (IOException e) {
throw new StorageException(
- String.format("Star change %d for account %d failed", changeId.get(), accountId.get()),
+ String.format("Star change %d for account %d failed", virtualId.get(), accountId.get()),
e);
}
}
@Override
public Set<Change.Id> areStarred(
- Repository allUsersRepo, List<Change.Id> changeIds, Account.Id caller) {
+ Repository allUsersRepo, List<Change.Id> virtualIds, Account.Id caller) {
List<String> starRefs =
- changeIds.stream()
+ virtualIds.stream()
.map(c -> RefNames.refsStarredChanges(c, caller))
.collect(Collectors.toList());
try {
@@ -140,21 +140,21 @@
} catch (IOException e) {
logger.atWarning().withCause(e).log(
"Failed getting starred changes for account %d within changes: %s",
- caller.get(), Joiner.on(", ").join(changeIds));
+ caller.get(), Joiner.on(", ").join(virtualIds));
return ImmutableSet.of();
}
}
@Override
- public void unstarAllForChangeDeletion(Change.Id changeId) throws IOException {
+ public void unstarAllForChangeDeletion(Change.Id virtualId) throws IOException {
try (Repository repo = repoManager.openRepository(allUsers);
RevWalk rw = new RevWalk(repo)) {
BatchRefUpdate batchUpdate = repo.getRefDatabase().newBatchUpdate();
batchUpdate.setAllowNonFastForwards(true);
batchUpdate.setRefLogIdent(serverIdent.get());
- batchUpdate.setRefLogMessage("Unstar change " + changeId.get(), true);
- for (Account.Id accountId : getStars(repo, changeId)) {
- String refName = RefNames.refsStarredChanges(changeId, accountId);
+ batchUpdate.setRefLogMessage("Unstar change " + virtualId.get(), true);
+ for (Account.Id accountId : getStars(repo, virtualId)) {
+ String refName = RefNames.refsStarredChanges(virtualId, accountId);
Ref ref = repo.getRefDatabase().exactRef(refName);
if (ref != null) {
batchUpdate.addCommand(new ReceiveCommand(ref.getObjectId(), ObjectId.zeroId(), refName));
@@ -166,7 +166,7 @@
String message =
String.format(
"Unstar change %d failed, ref %s could not be deleted: %s",
- changeId.get(), command.getRefName(), command.getResult());
+ virtualId.get(), command.getRefName(), command.getResult());
if (command.getResult() == ReceiveCommand.Result.LOCK_FAILURE) {
throw new LockFailureException(message, batchUpdate);
}
@@ -177,11 +177,11 @@
}
@Override
- public ImmutableList<Account.Id> byChange(Change.Id changeId) {
+ public ImmutableList<Account.Id> byChange(Change.Id virtualId) {
try (Repository repo = repoManager.openRepository(allUsers)) {
ImmutableList.Builder<Account.Id> builder = ImmutableList.builder();
- for (Account.Id accountId : getStars(repo, changeId)) {
- Optional<Ref> starRef = getStarRef(repo, RefNames.refsStarredChanges(changeId, accountId));
+ for (Account.Id accountId : getStars(repo, virtualId)) {
+ Optional<Ref> starRef = getStarRef(repo, RefNames.refsStarredChanges(virtualId, accountId));
if (starRef.isPresent()) {
builder.add(accountId);
}
@@ -189,7 +189,7 @@
return builder.build();
} catch (IOException e) {
throw new StorageException(
- String.format("Get accounts that starred change %d failed", changeId.get()), e);
+ String.format("Get accounts that starred change %d failed", virtualId.get()), e);
}
}
@@ -223,9 +223,9 @@
}
}
- private static Set<Account.Id> getStars(Repository allUsers, Change.Id changeId)
+ private static Set<Account.Id> getStars(Repository allUsers, Change.Id virtualId)
throws IOException {
- String prefix = RefNames.refsStarredChangesPrefix(changeId);
+ String prefix = RefNames.refsStarredChangesPrefix(virtualId);
RefDatabase refDb = allUsers.getRefDatabase();
return refDb.getRefsByPrefix(prefix).stream()
.map(r -> r.getName().substring(prefix.length()))
diff --git a/java/com/google/gerrit/server/plugins/DisablePlugin.java b/java/com/google/gerrit/server/plugins/DisablePlugin.java
index 9e238f8..d845ec17 100644
--- a/java/com/google/gerrit/server/plugins/DisablePlugin.java
+++ b/java/com/google/gerrit/server/plugins/DisablePlugin.java
@@ -53,7 +53,12 @@
if (mandatoryPluginsCollection.contains(name)) {
throw new MethodNotAllowedException("Plugin " + name + " is mandatory");
}
- loader.disablePlugins(ImmutableSet.of(name));
+ try {
+ loader.disablePlugins(ImmutableSet.of(name));
+ } catch (PluginInstallException e) {
+ throw new MethodNotAllowedException("Plugin " + name + " cannot be disabled", e);
+ }
+
return Response.ok(ListPlugins.toPluginInfo(loader.get(name)));
}
}
diff --git a/java/com/google/gerrit/server/plugins/PluginLoader.java b/java/com/google/gerrit/server/plugins/PluginLoader.java
index 9e23c0b..e122d8d 100644
--- a/java/com/google/gerrit/server/plugins/PluginLoader.java
+++ b/java/com/google/gerrit/server/plugins/PluginLoader.java
@@ -221,7 +221,7 @@
toCleanup.add(plugin);
}
- public void disablePlugins(Set<String> names) {
+ public void disablePlugins(Set<String> names) throws PluginInstallException {
if (!isRemoteAdminEnabled()) {
logger.atWarning().log(
"Remote plugin administration is disabled, ignoring disablePlugins(%s)", names);
@@ -235,6 +235,12 @@
continue;
}
+ if (active.getApiModule().isPresent()) {
+ throw new PluginInstallException(
+ String.format(
+ "Plugin %s has registered an ApiModule therefore it cannot be disabled", name));
+ }
+
if (mandatoryPlugins.contains(name)) {
logger.atWarning().log("Mandatory plugin %s cannot be disabled", name);
continue;
@@ -381,11 +387,14 @@
try {
logger.atInfo().log("Reloading plugin %s", name);
Plugin newPlugin = runPlugin(name, active.getSrcFile(), active);
- logger.atInfo().log(
- "Reloaded plugin %s%s, version %s",
- newPlugin.getName(),
- newPlugin.getApiModule().isPresent() ? " (w/ ApiModule)" : "",
- newPlugin.getVersion());
+
+ if (newPlugin != active) {
+ logger.atInfo().log(
+ "Reloaded plugin %s%s, version %s",
+ newPlugin.getName(),
+ newPlugin.getApiModule().isPresent() ? " (w/ ApiModule)" : "",
+ newPlugin.getVersion());
+ }
} catch (PluginInstallException e) {
logger.atWarning().withCause(e.getCause()).log("Cannot reload plugin %s", name);
throw e;
@@ -522,6 +531,13 @@
return oldPlugin;
}
+ if (oldPlugin != null && oldPlugin.getApiModule().isPresent()) {
+ throw new PluginInstallException(
+ String.format(
+ "Plugin %s has registered an ApiModule therefore its restart/reload is not allowed",
+ name));
+ }
+
Plugin newPlugin = loadPlugin(name, plugin, snapshot);
if (newPlugin.getCleanupHandle() != null) {
cleanupHandles.put(newPlugin, newPlugin.getCleanupHandle());
@@ -573,7 +589,13 @@
}
}
for (String name : unload) {
- unloadPlugin(running.get(name));
+ Plugin runningPlugin = running.get(name);
+
+ if (runningPlugin.getApiModule().isPresent()) {
+ logger.atWarning().log("Cannot remove plugin %s as it has registered an ApiModule", name);
+ } else {
+ unloadPlugin(running.get(name));
+ }
}
}
diff --git a/java/com/google/gerrit/server/plugins/ServerPlugin.java b/java/com/google/gerrit/server/plugins/ServerPlugin.java
index c275ff9..036285e 100644
--- a/java/com/google/gerrit/server/plugins/ServerPlugin.java
+++ b/java/com/google/gerrit/server/plugins/ServerPlugin.java
@@ -172,6 +172,11 @@
@Override
protected boolean canReload() {
Attributes main = manifest.getMainAttributes();
+ String apiModule = main.getValue("Gerrit-ApiModule");
+ if (apiModule != null) {
+ return false;
+ }
+
String v = main.getValue("Gerrit-ReloadMode");
if (Strings.isNullOrEmpty(v) || "reload".equalsIgnoreCase(v)) {
return true;
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index b607691..d153bc9 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -77,7 +77,6 @@
import com.google.gerrit.server.change.PureRevert;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.GerritServerConfig;
-import com.google.gerrit.server.config.GerritServerId;
import com.google.gerrit.server.config.SkipCurrentRulesEvaluationOnClosedChanges;
import com.google.gerrit.server.config.TrackingFooters;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -109,6 +108,7 @@
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import java.util.Optional;
import java.util.Set;
import java.util.function.Function;
@@ -247,11 +247,11 @@
}
public ChangeData create(Project.NameKey project, Change.Id id) {
- return assistedFactory.create(project, id, null, null);
+ return assistedFactory.create(project, id, null, null, null);
}
public ChangeData create(Project.NameKey project, Change.Id id, ObjectId metaRevision) {
- ChangeData cd = assistedFactory.create(project, id, null, null);
+ ChangeData cd = assistedFactory.create(project, id, null, null, null);
cd.setMetaRevision(metaRevision);
return cd;
}
@@ -264,19 +264,29 @@
}
public ChangeData create(Change change) {
- return assistedFactory.create(change.getProject(), change.getId(), change, null);
+ return create(change, null);
+ }
+
+ public ChangeData create(Change change, Change.Id virtualId) {
+ return assistedFactory.create(
+ change.getProject(),
+ change.getId(),
+ !Objects.equals(virtualId, change.getId()) ? virtualId : null,
+ change,
+ null);
}
public ChangeData create(ChangeNotes notes) {
return assistedFactory.create(
- notes.getChange().getProject(), notes.getChangeId(), notes.getChange(), notes);
+ notes.getChange().getProject(), notes.getChangeId(), null, notes.getChange(), notes);
}
}
public interface AssistedFactory {
ChangeData create(
Project.NameKey project,
- Change.Id id,
+ @Assisted("changeId") Change.Id id,
+ @Assisted("virtualId") @Nullable Change.Id virtualId,
@Nullable Change change,
@Nullable ChangeNotes notes);
}
@@ -295,7 +305,7 @@
*/
public static ChangeData createForTest(
Project.NameKey project, Change.Id id, int currentPatchSetId, ObjectId commitId) {
- return createForTest(project, id, currentPatchSetId, commitId, null, null, null);
+ return createForTest(project, id, currentPatchSetId, commitId, null, null);
}
/**
@@ -308,7 +318,6 @@
* @param id change ID
* @param currentPatchSetId current patchset number
* @param commitId commit SHA1 of the current patchset
- * @param serverId Gerrit server id
* @param virtualIdAlgo algorithm for virtualising the Change number
* @param changeNotes notes associated with the Change
* @return instance for testing.
@@ -318,7 +327,6 @@
Change.Id id,
int currentPatchSetId,
ObjectId commitId,
- @Nullable String serverId,
@Nullable ChangeNumberVirtualIdAlgorithm virtualIdAlgo,
@Nullable ChangeNotes changeNotes) {
ChangeData cd =
@@ -342,12 +350,12 @@
null,
null,
null,
- serverId,
virtualIdAlgo,
false,
project,
id,
null,
+ null,
changeNotes);
cd.currentPatchSet =
PatchSet.builder()
@@ -451,10 +459,10 @@
private Optional<Instant> mergedOn;
private ImmutableSetMultimap<Project.NameKey, RefState> refStates;
private ImmutableList<byte[]> refStatePatterns;
- private String gerritServerId;
private String changeServerId;
private ChangeNumberVirtualIdAlgorithm virtualIdFunc;
private Boolean failedParsingFromIndex = false;
+ private Change.Id virtualId;
@Inject
private ChangeData(
@@ -477,11 +485,11 @@
SubmitRequirementsEvaluator submitRequirementsEvaluator,
SubmitRequirementsUtil submitRequirementsUtil,
SubmitRuleEvaluator.Factory submitRuleEvaluatorFactory,
- @GerritServerId String gerritServerId,
ChangeNumberVirtualIdAlgorithm virtualIdFunc,
@SkipCurrentRulesEvaluationOnClosedChanges Boolean skipCurrentRulesEvaluationOnClosedChange,
@Assisted Project.NameKey project,
- @Assisted Change.Id id,
+ @Assisted("changeId") Change.Id id,
+ @Assisted("virtualId") @Nullable Change.Id virtualId,
@Assisted @Nullable Change change,
@Assisted @Nullable ChangeNotes notes) {
this.approvalsUtil = approvalsUtil;
@@ -515,8 +523,8 @@
this.notes = notes;
this.changeServerId = notes == null ? null : notes.getServerId();
- this.gerritServerId = gerritServerId;
this.virtualIdFunc = virtualIdFunc;
+ this.virtualId = virtualId;
}
/**
@@ -647,12 +655,33 @@
return legacyId;
}
- public Change.Id getVirtualId() {
- if (virtualIdFunc == null || changeServerId == null || changeServerId.equals(gerritServerId)) {
- return legacyId;
+ public static void ensureChangeServerId(Iterable<ChangeData> changes) {
+ ChangeData first = Iterables.getFirst(changes, null);
+ if (first == null) {
+ return;
}
- return Change.id(virtualIdFunc.apply(changeServerId, legacyId.get()));
+ for (ChangeData cd : changes) {
+ var unused = cd.changeServerId();
+ }
+ }
+
+ @Nullable
+ public String changeServerId() {
+ if (changeServerId == null) {
+ if (!lazyload()) {
+ return null;
+ }
+ changeServerId = notes().getServerId();
+ }
+ return changeServerId;
+ }
+
+ public Change.Id virtualId() {
+ if (virtualId == null) {
+ return virtualIdFunc == null ? legacyId : virtualIdFunc.apply(changeServerId, legacyId);
+ }
+ return virtualId;
}
public Project.NameKey project() {
@@ -1443,7 +1472,7 @@
if (!lazyload()) {
return ImmutableList.of();
}
- starAccounts = requireNonNull(starredChangesReader).byChange(legacyId);
+ starAccounts = requireNonNull(starredChangesReader).byChange(virtualId());
}
return starAccounts;
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeNumberBitmapMaskAlgorithm.java b/java/com/google/gerrit/server/query/change/ChangeNumberBitmapMaskAlgorithm.java
index 726a376..95c287a 100644
--- a/java/com/google/gerrit/server/query/change/ChangeNumberBitmapMaskAlgorithm.java
+++ b/java/com/google/gerrit/server/query/change/ChangeNumberBitmapMaskAlgorithm.java
@@ -16,7 +16,9 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.server.config.GerritImportedServerIds;
+import com.google.gerrit.server.config.GerritServerId;
import com.google.inject.Inject;
import com.google.inject.ProvisionException;
import com.google.inject.Singleton;
@@ -38,9 +40,11 @@
Integer.BYTES * 8 - CHANGE_NUM_BIT_LEN; // Allows up to 64 ServerIds
private final ImmutableMap<String, Integer> serverIdCodes;
+ private final String localServerId;
@Inject
public ChangeNumberBitmapMaskAlgorithm(
+ @GerritServerId String localServerId,
@GerritImportedServerIds ImmutableList<String> importedServerIds) {
if (importedServerIds.size() >= 1 << SERVER_ID_BIT_LEN) {
throw new ProvisionException(
@@ -54,10 +58,17 @@
}
serverIdCodes = serverIdCodesBuilder.build();
+ this.localServerId = localServerId;
}
@Override
- public int apply(String changeServerId, int changeNum) {
+ public Change.Id apply(String changeServerId, Change.Id changeNumId) {
+ if (changeServerId == null || localServerId.equals(changeServerId)) {
+ return changeNumId;
+ }
+
+ int changeNum = changeNumId.get();
+
if ((changeNum & LEGACY_ID_BIT_MASK) != changeNum) {
throw new IllegalArgumentException(
String.format(
@@ -71,6 +82,6 @@
}
int virtualId = (changeNum & LEGACY_ID_BIT_MASK) | (encodedServerId << CHANGE_NUM_BIT_LEN);
- return virtualId;
+ return Change.id(virtualId);
}
}
diff --git a/java/com/google/gerrit/server/query/change/ChangeNumberVirtualIdAlgorithm.java b/java/com/google/gerrit/server/query/change/ChangeNumberVirtualIdAlgorithm.java
index ab21705..6daf16f 100644
--- a/java/com/google/gerrit/server/query/change/ChangeNumberVirtualIdAlgorithm.java
+++ b/java/com/google/gerrit/server/query/change/ChangeNumberVirtualIdAlgorithm.java
@@ -14,6 +14,7 @@
package com.google.gerrit.server.query.change;
+import com.google.gerrit.entities.Change;
import com.google.inject.ImplementedBy;
/**
@@ -31,5 +32,5 @@
* @param legacyChangeNum legacy change number
* @return virtual id which combines serverId and legacyChangeNum together
*/
- int apply(String serverId, int legacyChangeNum);
+ Change.Id apply(String serverId, Change.Id legacyChangeNum);
}
diff --git a/java/com/google/gerrit/server/query/change/PredicateArgs.java b/java/com/google/gerrit/server/query/change/PredicateArgs.java
index ebe4390..02d2ca6 100644
--- a/java/com/google/gerrit/server/query/change/PredicateArgs.java
+++ b/java/com/google/gerrit/server/query/change/PredicateArgs.java
@@ -71,7 +71,7 @@
*
* @param args arguments to be parsed
*/
- PredicateArgs(String args) throws QueryParseException {
+ public PredicateArgs(String args) throws QueryParseException {
positional = new ArrayList<>();
keyValue = new HashMap<>();
diff --git a/java/com/google/gerrit/server/restapi/account/StarredChanges.java b/java/com/google/gerrit/server/restapi/account/StarredChanges.java
index 1565aba..95aa07c 100644
--- a/java/com/google/gerrit/server/restapi/account/StarredChanges.java
+++ b/java/com/google/gerrit/server/restapi/account/StarredChanges.java
@@ -69,7 +69,7 @@
throws RestApiException, PermissionBackendException, IOException {
IdentifiedUser user = parent.getUser();
ChangeResource change = changes.parse(TopLevelResource.INSTANCE, id);
- if (starredChangesReader.isStarred(user.getAccountId(), change.getId())) {
+ if (starredChangesReader.isStarred(user.getAccountId(), change.getVirtualId())) {
return new AccountResource.StarredChange(user, change);
}
throw new ResourceNotFoundException(id);
@@ -125,7 +125,7 @@
}
try {
- starredChangesWriter.star(self.get().getAccountId(), change.getId());
+ starredChangesWriter.star(self.get().getAccountId(), change.getVirtualId());
} catch (DuplicateKeyException e) {
return Response.none();
}
@@ -168,7 +168,7 @@
if (!self.get().hasSameAccountId(rsrc.getUser())) {
throw new AuthException("not allowed remove starred change");
}
- starredChangesWriter.unstar(self.get().getAccountId(), rsrc.getChange().getId());
+ starredChangesWriter.unstar(self.get().getAccountId(), rsrc.getVirtualId());
return Response.none();
}
}
diff --git a/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java b/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java
index 0119349b..d478a9d 100644
--- a/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java
+++ b/java/com/google/gerrit/sshd/commands/PluginRemoveCommand.java
@@ -17,6 +17,7 @@
import static com.google.gerrit.sshd.CommandMetaData.Mode.MASTER_OR_SLAVE;
import com.google.common.collect.Sets;
+import com.google.gerrit.server.plugins.PluginInstallException;
import com.google.gerrit.sshd.CommandMetaData;
import java.util.List;
import org.kohsuke.args4j.Argument;
@@ -29,7 +30,11 @@
@Override
protected void doRun() throws UnloggedFailure {
if (names != null && !names.isEmpty()) {
- loader.disablePlugins(Sets.newHashSet(names));
+ try {
+ loader.disablePlugins(Sets.newHashSet(names));
+ } catch (PluginInstallException e) {
+ throw die(e);
+ }
}
}
}
diff --git a/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java b/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
index bbf10bd..812a0df 100644
--- a/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
+++ b/javatests/com/google/gerrit/entities/converter/ChangeProtoConverterTest.java
@@ -32,6 +32,7 @@
public class ChangeProtoConverterTest {
private final ChangeProtoConverter changeProtoConverter = ChangeProtoConverter.INSTANCE;
+ private static final String TEST_SERVER_ID = "test-server-id";
@Test
public void allValuesConvertedToProto() {
@@ -42,6 +43,7 @@
Account.id(35),
BranchNameKey.create(Project.nameKey("project 67"), "branch 74"),
Instant.ofEpochMilli(987654L));
+ change.setServerId(TEST_SERVER_ID);
change.setLastUpdatedOn(Instant.ofEpochMilli(1234567L));
change.setStatus(Change.Status.MERGED);
change.setCurrentPatchSet(
@@ -89,6 +91,7 @@
Account.id(35),
BranchNameKey.create(Project.nameKey("project 67"), "branch-74"),
Instant.ofEpochMilli(987654L));
+ change.setServerId(TEST_SERVER_ID);
Entities.Change proto = changeProtoConverter.toProto(change);
@@ -124,6 +127,7 @@
Account.id(35),
BranchNameKey.create(Project.nameKey("project 67"), "branch-74"),
Instant.ofEpochMilli(987654L));
+ change.setServerId(TEST_SERVER_ID);
// O as ID actually means that no current patch set is present.
change.setCurrentPatchSet(PatchSet.id(Change.id(14), 0), null, null);
@@ -161,6 +165,7 @@
Account.id(35),
BranchNameKey.create(Project.nameKey("project 67"), "branch-74"),
Instant.ofEpochMilli(987654L));
+ change.setServerId(TEST_SERVER_ID);
change.setCurrentPatchSet(PatchSet.id(Change.id(14), 23), "subject ABC", null);
Entities.Change proto = changeProtoConverter.toProto(change);
@@ -189,7 +194,7 @@
}
@Test
- public void allValuesConvertedToProtoAndBackAgain() {
+ public void allValuesConvertedToProtoAndBackAgainExceptServerId() {
Change change =
new Change(
Change.key("change 1"),
@@ -197,6 +202,7 @@
Account.id(35),
BranchNameKey.create(Project.nameKey("project 67"), "branch-74"),
Instant.ofEpochMilli(987654L));
+ change.setServerId(TEST_SERVER_ID);
change.setLastUpdatedOn(Instant.ofEpochMilli(1234567L));
change.setStatus(Change.Status.MERGED);
change.setCurrentPatchSet(
@@ -209,6 +215,11 @@
change.setRevertOf(Change.id(180));
Change convertedChange = changeProtoConverter.fromProto(changeProtoConverter.toProto(change));
+
+ // Change serverId is not one of the protobuf definitions, hence is not supposed to be converted
+ // from proto
+ assertThat(convertedChange.getServerId()).isNull();
+ change.setServerId(null);
assertEqualChange(convertedChange, change);
}
@@ -275,6 +286,7 @@
.hasFields(
ImmutableMap.<String, Type>builder()
.put("changeId", Change.Id.class)
+ .put("serverId", String.class)
.put("changeKey", Change.Key.class)
.put("createdOn", Instant.class)
.put("lastUpdatedOn", Instant.class)
@@ -298,6 +310,7 @@
// an AutoValue.
private static void assertEqualChange(Change change, Change expectedChange) {
assertThat(change.getChangeId()).isEqualTo(expectedChange.getChangeId());
+ assertThat(change.getServerId()).isEqualTo(expectedChange.getServerId());
assertThat(change.getKey()).isEqualTo(expectedChange.getKey());
assertThat(change.getCreatedOn()).isEqualTo(expectedChange.getCreatedOn());
assertThat(change.getLastUpdatedOn()).isEqualTo(expectedChange.getLastUpdatedOn());
diff --git a/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java b/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
index 8cb4974..53431d1 100644
--- a/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
+++ b/javatests/com/google/gerrit/server/index/change/ChangeFieldTest.java
@@ -158,7 +158,7 @@
public void tolerateNullValuesForInsertion() {
Project.NameKey project = Project.nameKey("project");
ChangeData cd =
- ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId(), null, null, null);
+ ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId(), null, null);
assertThat(ChangeField.ADDED_LINES_SPEC.setIfPossible(cd, new FakeStoredValue(null))).isTrue();
}
@@ -166,7 +166,7 @@
public void tolerateNullValuesForDeletion() {
Project.NameKey project = Project.nameKey("project");
ChangeData cd =
- ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId(), null, null, null);
+ ChangeData.createForTest(project, Change.id(1), 1, ObjectId.zeroId(), null, null);
assertThat(ChangeField.DELETED_LINES_SPEC.setIfPossible(cd, new FakeStoredValue(null)))
.isTrue();
}
diff --git a/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java b/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
index 0ce00eb..f954a57 100644
--- a/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
+++ b/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
@@ -34,8 +34,6 @@
@RunWith(MockitoJUnitRunner.class)
public class ChangeDataTest {
- private static final String GERRIT_SERVER_ID = UUID.randomUUID().toString();
-
@Mock private ChangeNotes changeNotesMock;
@Test
@@ -55,7 +53,7 @@
@Test
public void getChangeVirtualIdUsingAlgorithm() throws Exception {
Project.NameKey project = Project.nameKey("project");
- final int encodedChangeNum = 12345678;
+ final Change.Id encodedChangeNum = Change.id(12345678);
when(changeNotesMock.getServerId()).thenReturn(UUID.randomUUID().toString());
@@ -65,11 +63,10 @@
Change.id(1),
1,
ObjectId.zeroId(),
- GERRIT_SERVER_ID,
(s, c) -> encodedChangeNum,
changeNotesMock);
- assertThat(cd.getVirtualId().get()).isEqualTo(encodedChangeNum);
+ assertThat(cd.virtualId().get()).isEqualTo(encodedChangeNum.get());
}
private static PatchSet newPatchSet(Change.Id changeId, int num) {
diff --git a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
index c7f4f64..d00cc45 100644
--- a/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/FakeQueryChangesTest.java
@@ -26,6 +26,8 @@
import com.google.common.collect.ImmutableList;
import com.google.gerrit.acceptance.UseClockStep;
+import com.google.gerrit.entities.Account;
+import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Permission;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.common.ChangeInfo;
@@ -90,11 +92,49 @@
@SuppressWarnings("unused")
var unused = newQuery("status:new").withLimit(5).get();
+ // Since the limit of the query (i.e. 5) is more than the total number of changes (i.e. 4),
+ // only 1 index search is expected.
assertThatSearchQueryWasNotPaginated(idx.getQueryCount());
}
@Test
@UseClockStep
+ public void queryRightNumberOfTimes() throws Exception {
+ Project.NameKey project = Project.nameKey("repo");
+ TestRepository<Repository> repo = createAndOpenProject(project);
+ Account.Id user2 =
+ accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId();
+
+ // create 1 visible change
+ Change visibleChange1 = insert(project, newChangeWithStatus(repo, Change.Status.NEW));
+
+ // create 4 private changes
+ Change invisibleChange2 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ Change invisibleChange3 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ Change invisibleChange4 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ Change invisibleChange5 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ gApi.changes().id(invisibleChange2.getKey().get()).setPrivate(true, null);
+ gApi.changes().id(invisibleChange3.getKey().get()).setPrivate(true, null);
+ gApi.changes().id(invisibleChange4.getKey().get()).setPrivate(true, null);
+ gApi.changes().id(invisibleChange5.getKey().get()).setPrivate(true, null);
+
+ AbstractFakeIndex<?, ?, ?> idx =
+ (AbstractFakeIndex<?, ?, ?>) changeIndexCollection.getSearchIndex();
+ idx.resetQueryCount();
+ List<ChangeInfo> queryResult = newQuery("status:new").withLimit(2).get();
+ assertThat(queryResult).hasSize(1);
+ assertThat(queryResult.get(0).changeId).isEqualTo(visibleChange1.getKey().get());
+
+ // Since the limit of the query (i.e. 2), 2 index searches are expected in fact:
+ // 1: The first query will return invisibleChange5, invisibleChange4 and invisibleChange3,
+ // 2: Another query is needed to back-fill the limit requested by the user.
+ // even if one result in the second query is skipped because it is not visible,
+ // there are no more results to query.
+ assertThatSearchQueryWasPaginated(idx.getQueryCount(), 2);
+ }
+
+ @Test
+ @UseClockStep
public void noLimitQueryPaginates() throws Exception {
assumeFalse(PaginationType.NONE == getCurrentPaginationType());
@@ -139,39 +179,7 @@
@Test
@UseClockStep
- public void invisibleChangesNotPaginatedWithNonePaginationType() throws Exception {
- assumeTrue(PaginationType.NONE == getCurrentPaginationType());
- AbstractFakeIndex<?, ?, ?> idx = setupRepoWithFourChanges();
- final int LIMIT = 3;
-
- projectOperations
- .project(allProjectsName)
- .forUpdate()
- .removeAllAccessSections()
- .add(allow(Permission.READ).ref("refs/*").group(SystemGroupBackend.REGISTERED_USERS))
- .update();
-
- // Set queryLimit to 3
- projectOperations
- .project(allProjects)
- .forUpdate()
- .add(allowCapability(QUERY_LIMIT).group(ANONYMOUS_USERS).range(0, LIMIT))
- .update();
-
- @SuppressWarnings("unused")
- var unused = requestContext.setContext(anonymousUserProvider::get);
-
- List<ChangeInfo> result = newQuery("status:new").withLimit(LIMIT).get();
- assertThat(result.size()).isEqualTo(0);
- assertThatSearchQueryWasNotPaginated(idx.getQueryCount());
- assertThat(idx.getResultsSizes().get(0)).isEqualTo(LIMIT + 1);
- }
-
- @Test
- @UseClockStep
public void invisibleChangesPaginatedWithPagination() throws Exception {
- assumeFalse(PaginationType.NONE == getCurrentPaginationType());
-
AbstractFakeIndex<?, ?, ?> idx = setupRepoWithFourChanges();
final int LIMIT = 3;
diff --git a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
index 987a87e..82c9065 100644
--- a/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
+++ b/javatests/com/google/gerrit/server/query/change/LuceneQueryChangesTest.java
@@ -20,6 +20,7 @@
import static com.google.gerrit.server.group.SystemGroupBackend.REGISTERED_USERS;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
+import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Project;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -109,4 +110,28 @@
Change[] expected = new Change[] {change6, change5, change4, change3, change2, change1};
assertQuery(newQuery("project:repo").withNoLimit(), expected);
}
+
+ @Test
+ public void skipChangesNotVisible() throws Exception {
+ // create 1 new change on a repo
+ Project.NameKey project = Project.nameKey("repo");
+ repo = createAndOpenProject(project);
+ Change visibleChange = insert(project, newChangeWithStatus(repo, Change.Status.NEW));
+ Change[] expected = new Change[] {visibleChange};
+
+ // pagination does not need to restart the datasource, the request is fulfilled
+ assertQuery(newQuery("status:new").withLimit(1), expected);
+
+ // create 2 new private changes
+ Account.Id user2 =
+ accountManager.authenticate(authRequestFactory.createForUser("anotheruser")).getAccountId();
+
+ Change invisibleChange1 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ Change invisibleChange2 = insert(project, newChangeWithStatus(repo, Change.Status.NEW), user2);
+ gApi.changes().id(invisibleChange1.getKey().get()).setPrivate(true, null);
+ gApi.changes().id(invisibleChange2.getKey().get()).setPrivate(true, null);
+
+ // pagination should back-fill when the results skipped because of the visibility
+ assertQuery(newQuery("status:new").withLimit(1), expected);
+ }
}
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 0bf49db..1a18e9c 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,7 +98,10 @@
} from '../../../utils/attention-set-util';
import {RestApiService} from '../../../services/gr-rest-api/gr-rest-api';
import {resolve} from '../../../models/dependency';
-import {changeModelToken} from '../../../models/change/change-model';
+import {
+ changeModelToken,
+ updateRevisionsWithCommitShas,
+} 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';
@@ -1459,8 +1462,10 @@
return this.saveReview(reviewInput, errFn)
.then(result => {
this.getChangeModel().updateStateChange(
- GrReviewerUpdatesParser.parse(
- result?.change_info as ChangeViewChangeInfo
+ updateRevisionsWithCommitShas(
+ GrReviewerUpdatesParser.parse(
+ result?.change_info as ChangeViewChangeInfo
+ )
)
);
