Resolve draft comment reference ambiguity for imported changes
This commit addresses ambiguity in draft comment references when dealing
with imported changes. Previously, the system encountered issues when
two changes shared the same change number, with one being imported. The
solution introduces a new pattern for creating draft comment references:
refs/draft-comments/NN/<virtual-id>/<account-id>.
This approach ensures distinct draft comment references for each
imported change, eliminating the side effects caused by sharing the
same reference with non-imported changes.
Bug: Issue 3253095744
Forward-Compatible: checked
Release-Notes: Resolve draft comment reference ambiguity for imported changes
Change-Id: I108671395e047f688744e1c4a7ed3bee8d359735
diff --git a/java/com/google/gerrit/server/CommentsUtil.java b/java/com/google/gerrit/server/CommentsUtil.java
index 8198ce4..c2bd792 100644
--- a/java/com/google/gerrit/server/CommentsUtil.java
+++ b/java/com/google/gerrit/server/CommentsUtil.java
@@ -46,6 +46,7 @@
import com.google.gerrit.server.patch.DiffOperations;
import com.google.gerrit.server.patch.DiffOptions;
import com.google.gerrit.server.patch.filediff.FileDiffOutput;
+import com.google.gerrit.server.query.change.ChangeNumberVirtualIdAlgorithm;
import com.google.gerrit.server.update.ChangeContext;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -117,17 +118,20 @@
private final GitRepositoryManager repoManager;
private final AllUsersName allUsers;
private final String serverId;
+ private final ChangeNumberVirtualIdAlgorithm virtualIdAlgorithm;
@Inject
CommentsUtil(
DiffOperations diffOperations,
GitRepositoryManager repoManager,
AllUsersName allUsers,
- @GerritServerId String serverId) {
+ @GerritServerId String serverId,
+ @Nullable ChangeNumberVirtualIdAlgorithm virtualIdAlgorithm) {
this.diffOperations = diffOperations;
this.repoManager = repoManager;
this.allUsers = allUsers;
this.serverId = serverId;
+ this.virtualIdAlgorithm = virtualIdAlgorithm;
}
public HumanComment newHumanComment(
@@ -224,7 +228,7 @@
public List<HumanComment> draftByChange(ChangeNotes notes) {
List<HumanComment> comments = new ArrayList<>();
- for (Ref ref : getDraftRefs(notes.getChangeId())) {
+ for (Ref ref : getDraftRefs(getVirtualId(notes))) {
Account.Id account = Account.Id.fromRefSuffix(ref.getName());
if (account != null) {
comments.addAll(draftByChangeAuthor(notes, account));
@@ -323,17 +327,19 @@
public List<HumanComment> draftByPatchSetAuthor(
PatchSet.Id psId, Account.Id author, ChangeNotes notes) {
- return commentsOnPatchSet(notes.load().getDraftComments(author).values(), psId);
+ return commentsOnPatchSet(
+ notes.load().getDraftComments(author, getVirtualId(notes)).values(), psId);
}
public List<HumanComment> draftByChangeFileAuthor(
ChangeNotes notes, String file, Account.Id author) {
- return commentsOnFile(notes.load().getDraftComments(author).values(), file);
+ return commentsOnFile(
+ notes.load().getDraftComments(author, getVirtualId(notes)).values(), file);
}
public List<HumanComment> draftByChangeAuthor(ChangeNotes notes, Account.Id author) {
List<HumanComment> comments = new ArrayList<>();
- comments.addAll(notes.getDraftComments(author).values());
+ comments.addAll(notes.getDraftComments(author, getVirtualId(notes)).values());
return sort(comments);
}
@@ -477,8 +483,8 @@
}
}
- private Collection<Ref> getDraftRefs(Repository repo, Change.Id changeId) throws IOException {
- return repo.getRefDatabase().getRefsByPrefix(RefNames.refsDraftCommentsPrefix(changeId));
+ private Collection<Ref> getDraftRefs(Repository repo, Change.Id virtualId) throws IOException {
+ return repo.getRefDatabase().getRefsByPrefix(RefNames.refsDraftCommentsPrefix(virtualId));
}
private Collection<Change.Id> getChangesWithDrafts(Repository repo, Account.Id accountId)
@@ -501,4 +507,10 @@
comments.sort(COMMENT_ORDER);
return comments;
}
+
+ private Change.Id getVirtualId(ChangeNotes notes) {
+ return virtualIdAlgorithm == null
+ ? notes.getChangeId()
+ : virtualIdAlgorithm.apply(notes.getServerId(), notes.getChangeId());
+ }
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java
index 73161d7..165f3e4 100644
--- a/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java
+++ b/java/com/google/gerrit/server/notedb/ChangeDraftUpdate.java
@@ -19,6 +19,7 @@
import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
import com.google.auto.value.AutoValue;
+import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.Account;
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.Comment;
@@ -28,6 +29,7 @@
import com.google.gerrit.exceptions.StorageException;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.config.AllUsersName;
+import com.google.gerrit.server.query.change.ChangeNumberVirtualIdAlgorithm;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.io.IOException;
@@ -54,6 +56,8 @@
* <p>This class is not thread safe.
*/
public class ChangeDraftUpdate extends AbstractChangeUpdate {
+ private final ChangeNumberVirtualIdAlgorithm virtualIdFunc;
+
public interface Factory {
ChangeDraftUpdate create(
ChangeNotes notes,
@@ -98,6 +102,7 @@
@GerritPersonIdent PersonIdent serverIdent,
AllUsersName allUsers,
ChangeNoteUtil noteUtil,
+ @Nullable ChangeNumberVirtualIdAlgorithm virtualIdFunc,
@Assisted ChangeNotes notes,
@Assisted("effective") Account.Id accountId,
@Assisted("real") Account.Id realAccountId,
@@ -105,6 +110,7 @@
@Assisted Instant when) {
super(noteUtil, serverIdent, notes, null, accountId, realAccountId, authorIdent, when);
this.draftsProject = allUsers;
+ this.virtualIdFunc = virtualIdFunc;
}
@AssistedInject
@@ -112,6 +118,7 @@
@GerritPersonIdent PersonIdent serverIdent,
AllUsersName allUsers,
ChangeNoteUtil noteUtil,
+ @Nullable ChangeNumberVirtualIdAlgorithm virtualIdFunc,
@Assisted Change change,
@Assisted("effective") Account.Id accountId,
@Assisted("real") Account.Id realAccountId,
@@ -119,6 +126,7 @@
@Assisted Instant when) {
super(noteUtil, serverIdent, null, change, accountId, realAccountId, authorIdent, when);
this.draftsProject = allUsers;
+ this.virtualIdFunc = virtualIdFunc;
}
public void putComment(HumanComment c) {
@@ -178,6 +186,7 @@
authorIdent,
draftsProject,
noteUtil,
+ virtualIdFunc,
new Change(getChange()),
accountId,
realAccountId,
@@ -283,7 +292,7 @@
@Override
protected String getRefName() {
- return RefNames.refsDraftComments(getId(), accountId);
+ return RefNames.refsDraftComments(getVirtualId(), accountId);
}
@Override
@@ -295,4 +304,11 @@
public boolean isEmpty() {
return delete.isEmpty() && put.isEmpty();
}
+
+ private Change.Id getVirtualId() {
+ Change change = getChange();
+ return virtualIdFunc == null
+ ? change.getId()
+ : virtualIdFunc.apply(change.getServerId(), change.getId());
+ }
}
diff --git a/java/com/google/gerrit/server/notedb/ChangeNotes.java b/java/com/google/gerrit/server/notedb/ChangeNotes.java
index b44da6b..80f9953 100644
--- a/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -563,12 +563,17 @@
}
public ImmutableListMultimap<ObjectId, HumanComment> getDraftComments(Account.Id author) {
- return getDraftComments(author, null);
+ return getDraftComments(author, null, null);
}
public ImmutableListMultimap<ObjectId, HumanComment> getDraftComments(
- Account.Id author, @Nullable Ref ref) {
- loadDraftComments(author, ref);
+ Account.Id author, @Nullable Change.Id virtualId) {
+ return getDraftComments(author, virtualId, null);
+ }
+
+ public ImmutableListMultimap<ObjectId, HumanComment> getDraftComments(
+ Account.Id author, @Nullable Change.Id virtualId, @Nullable Ref ref) {
+ loadDraftComments(author, virtualId, 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
// during the publish operation failed.
@@ -587,9 +592,10 @@
* However, this method will load the comments if no draft comments have been loaded or if the
* caller would like the drafts for another author.
*/
- private void loadDraftComments(Account.Id author, @Nullable Ref ref) {
+ private void loadDraftComments(
+ Account.Id author, @Nullable Change.Id virtualId, @Nullable Ref ref) {
if (draftCommentNotes == null || !author.equals(draftCommentNotes.getAuthor()) || ref != null) {
- draftCommentNotes = new DraftCommentNotes(args, getChangeId(), author, ref);
+ draftCommentNotes = new DraftCommentNotes(args, getChangeId(), virtualId, author, ref);
draftCommentNotes.load();
}
}
diff --git a/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
index 5d8f57f..d36acb0 100644
--- a/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
+++ b/java/com/google/gerrit/server/notedb/DraftCommentNotes.java
@@ -41,8 +41,15 @@
public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
private static final FluentLogger logger = FluentLogger.forEnclosingClass();
+ private final Change.Id virtualId;
+
public interface Factory {
DraftCommentNotes create(Change.Id changeId, Account.Id accountId);
+
+ DraftCommentNotes create(
+ @Assisted("changeId") Change.Id changeId,
+ @Assisted("virtualId") Change.Id virtualId,
+ Account.Id accountId);
}
private final Account.Id author;
@@ -53,11 +60,26 @@
@AssistedInject
DraftCommentNotes(Args args, @Assisted Change.Id changeId, @Assisted Account.Id author) {
- this(args, changeId, author, null);
+ this(args, changeId, null, author, null);
}
- DraftCommentNotes(Args args, Change.Id changeId, Account.Id author, @Nullable Ref ref) {
+ @AssistedInject
+ DraftCommentNotes(
+ Args args,
+ @Assisted("changeId") Change.Id changeId,
+ @Assisted("virtualId") Change.Id virtualId,
+ @Assisted Account.Id author) {
+ this(args, changeId, virtualId, author, null);
+ }
+
+ DraftCommentNotes(
+ Args args,
+ Change.Id changeId,
+ @Nullable Change.Id virtualId,
+ Account.Id author,
+ @Nullable Ref ref) {
super(args, changeId, null);
+ this.virtualId = virtualId;
this.author = requireNonNull(author);
this.ref = ref;
if (ref != null) {
@@ -93,7 +115,7 @@
@Override
protected String getRefName() {
- return refsDraftComments(getChangeId(), author);
+ return refsDraftComments(virtualId != null ? virtualId : getChangeId(), author);
}
@Override
diff --git a/java/com/google/gerrit/server/query/change/ChangeData.java b/java/com/google/gerrit/server/query/change/ChangeData.java
index 9fcce26..26e660a 100644
--- a/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -1524,7 +1524,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, ref).isEmpty()) {
+ && !notes().getDraftComments(account, getVirtualId(), ref).isEmpty()) {
draftsByUser.put(account, ref.getObjectId());
}
}
