Fix starred changes clash after repo import from another site
Introduce `virtualId` to change data and initiate it with the
`legacy_id_str` from the index so that changes list screen (which is
populated from the index only) could be properly updated with star bit.
Ensure that `changeServerId` is loaded from notes when notes are
available in the `ChangeData`.
Finally use the `virtualId` in all calls to get/set starred changes so
that the refs clash is avoided. As a result the starred changes ref
gets modified to the following form:
refs/starred-changes/NN/<virtual-id>/<account-id>
where `virtual-id` is `server-id` encoded `change-id` for imported
changes and `change-id` for existing/newly created ones.
Issue 331016670: to be addressed as the follow-up - toggle star from
changes list screen doesn't work for imported changes - the problem is
that UI uses change.id to obtain the project first which clashes with
existing changes. It works fine on the single change screen.
Bug: Issue 325309573
Release-Notes: Starred changes clash when importing changes from another instance
Forward-Compatible: checked
Change-Id: I85c946efcab5d20dd25682cb758bb9601c93b1d2
diff --git a/java/com/google/gerrit/extensions/common/ChangeInfo.java b/java/com/google/gerrit/extensions/common/ChangeInfo.java
index 40ae2ec..d381b68 100644
--- a/java/com/google/gerrit/extensions/common/ChangeInfo.java
+++ b/java/com/google/gerrit/extensions/common/ChangeInfo.java
@@ -99,6 +99,7 @@
public Boolean containsGitConflicts;
public Integer _number;
+ public Integer _virtualIdNumber;
public AccountInfo owner;
diff --git a/java/com/google/gerrit/lucene/LuceneChangeIndex.java b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
index eaae40f..a47b87e 100644
--- a/java/com/google/gerrit/lucene/LuceneChangeIndex.java
+++ b/java/com/google/gerrit/lucene/LuceneChangeIndex.java
@@ -112,7 +112,7 @@
private static final String CHANGE_FIELD = ChangeField.CHANGE.getName();
static Term idTerm(ChangeData cd) {
- return idTerm(cd.getVirtualId());
+ return idTerm(cd.virtualId());
}
static Term idTerm(Change.Id id) {
@@ -541,7 +541,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/StarredChangesUtil.java b/java/com/google/gerrit/server/StarredChangesUtil.java
index c845415..0f4e8f3 100644
--- a/java/com/google/gerrit/server/StarredChangesUtil.java
+++ b/java/com/google/gerrit/server/StarredChangesUtil.java
@@ -183,22 +183,22 @@
this.queryProvider = queryProvider;
}
- public NavigableSet<String> getLabels(Account.Id accountId, Change.Id changeId) {
+ public NavigableSet<String> getLabels(Account.Id accountId, Change.Id virtualId) {
try (Repository repo = repoManager.openRepository(allUsers)) {
- return readLabels(repo, RefNames.refsStarredChanges(changeId, accountId)).labels();
+ return readLabels(repo, RefNames.refsStarredChanges(virtualId, accountId)).labels();
} 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);
}
}
- public void star(Account.Id accountId, Project.NameKey project, Change.Id changeId, Operation op)
+ public void star(Account.Id accountId, Project.NameKey project, Change.Id virtualId, Operation op)
throws IllegalLabelException {
try (Repository repo = repoManager.openRepository(allUsers)) {
- String refName = RefNames.refsStarredChanges(changeId, accountId);
+ String refName = RefNames.refsStarredChanges(virtualId, accountId);
StarRef old = readLabels(repo, refName);
NavigableSet<String> labels = new TreeSet<>(old.labels());
@@ -218,19 +218,19 @@
}
} 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);
}
}
/**
- * Returns a subset of change IDs among the input {@code changeIds} list that are starred by the
+ * Returns a subset of change IDs among the input {@code virtualIds} list that are starred by the
* {@code caller} user.
*/
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 {
@@ -241,7 +241,7 @@
} 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();
}
}
@@ -252,18 +252,18 @@
* <p>Intended for use only when we're about to delete a change. For that reason, the change is
* not reindexed.
*
- * @param changeId change ID.
+ * @param virtualId change ID.
* @throws IOException if an error occurred.
*/
- 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 : byChangeFromIndex(changeId).keySet()) {
- String refName = RefNames.refsStarredChanges(changeId, accountId);
+ batchUpdate.setRefLogMessage("Unstar change " + virtualId.get(), true);
+ for (Account.Id accountId : byChangeFromIndex(virtualId).keySet()) {
+ String refName = RefNames.refsStarredChanges(virtualId, accountId);
Ref ref = repo.getRefDatabase().exactRef(refName);
if (ref != null) {
batchUpdate.addCommand(new ReceiveCommand(ref.getObjectId(), ObjectId.zeroId(), refName));
@@ -275,7 +275,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);
}
@@ -285,21 +285,21 @@
}
}
- public ImmutableMap<Account.Id, StarRef> byChange(Change.Id changeId) {
+ public ImmutableMap<Account.Id, StarRef> byChange(Change.Id virtualId) {
try (Repository repo = repoManager.openRepository(allUsers)) {
ImmutableMap.Builder<Account.Id, StarRef> builder = ImmutableMap.builder();
- for (String refPart : getRefNames(repo, RefNames.refsStarredChangesPrefix(changeId))) {
+ for (String refPart : getRefNames(repo, RefNames.refsStarredChangesPrefix(virtualId))) {
Integer id = Ints.tryParse(refPart);
if (id == null) {
continue;
}
Account.Id accountId = Account.id(id);
- builder.put(accountId, readLabels(repo, RefNames.refsStarredChanges(changeId, accountId)));
+ builder.put(accountId, readLabels(repo, RefNames.refsStarredChanges(virtualId, accountId)));
}
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);
}
}
@@ -332,14 +332,14 @@
}
}
- public ImmutableListMultimap<Account.Id, String> byChangeFromIndex(Change.Id changeId) {
+ public ImmutableListMultimap<Account.Id, String> byChangeFromIndex(Change.Id virtualId) {
List<ChangeData> changeData =
queryProvider
.get()
.setRequestedFields(ChangeField.ID, ChangeField.STAR)
- .byLegacyChangeId(changeId);
+ .byLegacyChangeId(virtualId);
if (changeData.size() != 1) {
- throw new NoSuchChangeException(changeId);
+ throw new NoSuchChangeException(virtualId);
}
return changeData.get(0).stars();
}
@@ -351,14 +351,14 @@
.collect(toSet());
}
- public ObjectId getObjectId(Account.Id accountId, Change.Id changeId) {
+ public ObjectId getObjectId(Account.Id accountId, Change.Id virtualId) {
try (Repository repo = repoManager.openRepository(allUsers)) {
- Ref ref = repo.exactRef(RefNames.refsStarredChanges(changeId, accountId));
+ Ref ref = repo.exactRef(RefNames.refsStarredChanges(virtualId, accountId));
return ref != null ? ref.getObjectId() : ObjectId.zeroId();
} catch (IOException e) {
logger.atSevere().withCause(e).log(
"Getting star object ID for account %d on change %d failed",
- accountId.get(), changeId.get());
+ accountId.get(), virtualId.get());
return ObjectId.zeroId();
}
}
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 c7e0799..928c8f8 100644
--- a/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/java/com/google/gerrit/server/change/ChangeJson.java
@@ -486,6 +486,9 @@
if (has(REVIEWED) && userProvider.get().isIdentifiedUser()) {
ChangeData.ensureReviewedByLoadedForOpenChanges(all);
}
+ if (has(STAR) && userProvider.get().isIdentifiedUser()) {
+ ChangeData.ensureChangeServerId(all);
+ }
ChangeData.ensureCurrentApprovalsLoaded(all);
} else {
for (ChangeData cd : all) {
@@ -770,6 +773,8 @@
.collect(toList());
}
+ out._virtualIdNumber = cd.virtualId().get();
+
return out;
}
@@ -960,14 +965,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 =
starredChangesUtil.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 919586e..f074100 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) {
@@ -240,7 +244,8 @@
.build())) {
Hasher h = Hashing.murmur3_128().newHasher();
if (user.isIdentifiedUser()) {
- h.putString(starredChangesUtil.getObjectId(user.getAccountId(), getId()).name(), UTF_8);
+ h.putString(
+ starredChangesUtil.getObjectId(user.getAccountId(), getVirtualId()).name(), UTF_8);
}
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 c7ddf19..ac75165 100644
--- a/java/com/google/gerrit/server/change/DeleteChangeOp.java
+++ b/java/com/google/gerrit/server/change/DeleteChangeOp.java
@@ -80,7 +80,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",
@@ -94,7 +95,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;
}
@@ -123,11 +124,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.getId()));
// Non-atomic operation on All-Users refs; not much we can do to make it atomic.
- starredChangesUtil.unstarAllForChangeDeletion(id);
+ starredChangesUtil.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 01a19df..2658522 100644
--- a/java/com/google/gerrit/server/index/change/ChangeField.java
+++ b/java/com/google/gerrit/server/index/change/ChangeField.java
@@ -127,7 +127,7 @@
// TODO: Rename LEGACY_ID to NUMERIC_ID
/** Legacy change ID. */
public static final FieldDef<ChangeData, String> LEGACY_ID_STR =
- exact("legacy_id_str").stored().build(cd -> String.valueOf(cd.getVirtualId().get()));
+ exact("legacy_id_str").stored().build(cd -> String.valueOf(cd.virtualId().get()));
/** Newer style Change-Id key. */
public static final FieldDef<ChangeData, String> ID =
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 26e660a..a69d837 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -106,6 +106,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;
@@ -237,11 +238,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;
}
@@ -254,19 +255,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);
}
@@ -333,6 +344,7 @@
project,
id,
null,
+ null,
changeNotes);
cd.currentPatchSet =
PatchSet.builder()
@@ -432,6 +444,7 @@
private ImmutableList<byte[]> refStatePatterns;
private String changeServerId;
private ChangeNumberVirtualIdAlgorithm virtualIdFunc;
+ private Change.Id virtualId;
@Inject
private ChangeData(
@@ -455,7 +468,8 @@
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;
@@ -485,6 +499,7 @@
this.changeServerId = notes == null ? null : notes.getServerId();
this.virtualIdFunc = virtualIdFunc;
+ this.virtualId = virtualId;
}
/**
@@ -605,8 +620,32 @@
return legacyId;
}
- public Change.Id getVirtualId() {
- return virtualIdFunc == null ? legacyId : virtualIdFunc.apply(changeServerId, legacyId);
+ public static void ensureChangeServerId(Iterable<ChangeData> changes) {
+ ChangeData first = Iterables.getFirst(changes, null);
+ if (first == null) {
+ return;
+ }
+
+ for (ChangeData cd : changes) {
+ cd.changeServerId();
+ }
+ }
+
+ 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() {
@@ -1345,7 +1384,7 @@
if (!lazyload()) {
return ImmutableMap.of();
}
- starRefs = requireNonNull(starredChangesUtil).byChange(legacyId);
+ starRefs = requireNonNull(starredChangesUtil).byChange(virtualId());
}
return starRefs;
}
@@ -1363,7 +1402,7 @@
if (!lazyload()) {
return ImmutableSet.of();
}
- starsOf = StarsOf.create(accountId, starredChangesUtil.getLabels(accountId, legacyId));
+ starsOf = StarsOf.create(accountId, starredChangesUtil.getLabels(accountId, virtualId()));
}
}
return starsOf.stars();
@@ -1524,7 +1563,7 @@
// this is suboptimal, but is ok for the purposes of
// draftsByUser(), and easier than trying to rebuild the change at
// this point.
- && !notes().getDraftComments(account, getVirtualId(), ref).isEmpty()) {
+ && !notes().getDraftComments(account, virtualId(), ref).isEmpty()) {
draftsByUser.put(account, ref.getObjectId());
}
}
diff --git a/java/com/google/gerrit/server/restapi/account/StarredChanges.java b/java/com/google/gerrit/server/restapi/account/StarredChanges.java
index 12abf3d..3880bce 100644
--- a/java/com/google/gerrit/server/restapi/account/StarredChanges.java
+++ b/java/com/google/gerrit/server/restapi/account/StarredChanges.java
@@ -73,7 +73,7 @@
IdentifiedUser user = parent.getUser();
ChangeResource change = changes.parse(TopLevelResource.INSTANCE, id);
if (starredChangesUtil
- .getLabels(user.getAccountId(), change.getId())
+ .getLabels(user.getAccountId(), change.getVirtualId())
.contains(StarredChangesUtil.DEFAULT_LABEL)) {
return new AccountResource.StarredChange(user, change);
}
@@ -133,7 +133,7 @@
starredChangesUtil.star(
self.get().getAccountId(),
change.getProject(),
- change.getId(),
+ change.getVirtualId(),
StarredChangesUtil.Operation.ADD);
} catch (MutuallyExclusiveLabelsException e) {
throw new ResourceConflictException(e.getMessage());
@@ -184,7 +184,7 @@
starredChangesUtil.star(
self.get().getAccountId(),
rsrc.getChange().getProject(),
- rsrc.getChange().getId(),
+ rsrc.getVirtualId(),
StarredChangesUtil.Operation.REMOVE);
return Response.none();
}
diff --git a/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java b/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
index c8e709a..beec379 100644
--- a/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
+++ b/javatests/com/google/gerrit/server/query/change/ChangeDataTest.java
@@ -66,7 +66,7 @@
(s, c) -> encodedChangeNum,
changeNotesMock);
- assertThat(cd.getVirtualId().get()).isEqualTo(encodedChangeNum.get());
+ assertThat(cd.virtualId().get()).isEqualTo(encodedChangeNum.get());
}
private static PatchSet newPatchSet(Change.Id changeId, int num) {
