Merge changes from topic 'notedb-reviewers'
* changes:
Include reviewers when rebuilding NoteDb
Extract a type for sets of reviewers, and store timestamp
diff --git a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
index 84e7557..f2c701b 100644
--- a/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
+++ b/gerrit-acceptance-framework/src/test/java/com/google/gerrit/acceptance/PushOneCommit.java
@@ -298,7 +298,7 @@
throws OrmException, NoSuchChangeException {
Iterable<Account.Id> actualIds = approvalsUtil
.getReviewers(db, notesFactory.createChecked(db, c))
- .values();
+ .all();
assertThat(actualIds).containsExactlyElementsIn(
Sets.newHashSet(TestAccount.ids(expectedReviewers)));
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
index dc3a6b1..847d559 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
@@ -14,8 +14,6 @@
package com.google.gerrit.server;
-import static com.google.common.base.Preconditions.checkArgument;
-import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import com.google.common.annotations.VisibleForTesting;
@@ -23,13 +21,10 @@
import com.google.common.base.Predicate;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
-import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.Iterables;
-import com.google.common.collect.LinkedHashMultimap;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
import com.google.common.collect.Ordering;
-import com.google.common.collect.SetMultimap;
import com.google.common.collect.Sets;
import com.google.gerrit.common.data.LabelType;
import com.google.gerrit.common.data.LabelTypes;
@@ -45,7 +40,6 @@
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
-import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.util.LabelVote;
import com.google.gwtorm.server.OrmException;
@@ -115,15 +109,14 @@
*
* @param db review database.
* @param notes change notes.
- * @return multimap of reviewers keyed by state, where each account appears
- * exactly once in {@link SetMultimap#values()}, and
- * {@link ReviewerStateInternal#REMOVED} is not present.
+ * @return reviewers for the change.
* @throws OrmException if reviewers for the change could not be read.
*/
- public ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers(
- ReviewDb db, ChangeNotes notes) throws OrmException {
+ public ReviewerSet getReviewers(ReviewDb db, ChangeNotes notes)
+ throws OrmException {
if (!migration.readChanges()) {
- return getReviewers(db.patchSetApprovals().byChange(notes.getChangeId()));
+ return ReviewerSet.fromApprovals(
+ db.patchSetApprovals().byChange(notes.getChangeId()));
}
return notes.load().getReviewers();
}
@@ -133,44 +126,18 @@
*
* @param allApprovals all approvals to consider; must all belong to the same
* change.
- * @return multimap of reviewers keyed by state, where each account appears
- * exactly once in {@link SetMultimap#values()}, and
- * {@link ReviewerStateInternal#REMOVED} is not present.
+ * @return reviewers for the change.
+ * @throws OrmException if reviewers for the change could not be read.
*/
- public ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers(
- ChangeNotes notes, Iterable<PatchSetApproval> allApprovals)
+ public ReviewerSet getReviewers(ChangeNotes notes,
+ Iterable<PatchSetApproval> allApprovals)
throws OrmException {
if (!migration.readChanges()) {
- return getReviewers(allApprovals);
+ return ReviewerSet.fromApprovals(allApprovals);
}
return notes.load().getReviewers();
}
- private static ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers(
- Iterable<PatchSetApproval> allApprovals) {
- PatchSetApproval first = null;
- SetMultimap<ReviewerStateInternal, Account.Id> reviewers =
- LinkedHashMultimap.create();
- for (PatchSetApproval psa : allApprovals) {
- if (first == null) {
- first = psa;
- } else {
- checkArgument(
- first.getKey().getParentKey().getParentKey().equals(
- psa.getKey().getParentKey().getParentKey()),
- "multiple change IDs: %s, %s", first.getKey(), psa.getKey());
- }
- Account.Id id = psa.getAccountId();
- if (psa.getValue() != 0) {
- reviewers.put(REVIEWER, id);
- reviewers.remove(CC, id);
- } else if (!reviewers.containsEntry(REVIEWER, id)) {
- reviewers.put(CC, id);
- }
- }
- return ImmutableSetMultimap.copyOf(reviewers);
- }
-
public List<PatchSetApproval> addReviewers(ReviewDb db,
ChangeUpdate update, LabelTypes labelTypes, Change change, PatchSet ps,
PatchSetInfo info, Iterable<Account.Id> wantReviewers,
@@ -185,7 +152,7 @@
Iterable<Account.Id> wantReviewers) throws OrmException {
PatchSet.Id psId = change.currentPatchSetId();
return addReviewers(db, update, labelTypes, change, psId, false, null, null,
- wantReviewers, getReviewers(db, notes).values());
+ wantReviewers, getReviewers(db, notes).all());
}
private List<PatchSetApproval> addReviewers(ReviewDb db, ChangeUpdate update,
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerSet.java
new file mode 100644
index 0000000..515cef7
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerSet.java
@@ -0,0 +1,115 @@
+// Copyright (C) 2016 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.server;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
+import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
+
+import com.google.common.collect.HashBasedTable;
+import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableTable;
+import com.google.common.collect.Table;
+import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.reviewdb.client.PatchSetApproval;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
+
+import java.sql.Timestamp;
+
+/**
+ * Set of reviewers on a change.
+ * <p>
+ * A given account may appear in multiple states and at different timestamps. No
+ * reviewers with state {@link ReviewerStateInternal#REMOVED} are ever exposed
+ * by this interface.
+ */
+public class ReviewerSet {
+ private static final ReviewerSet EMPTY = new ReviewerSet(
+ ImmutableTable.<ReviewerStateInternal, Account.Id, Timestamp>of());
+
+ public static ReviewerSet fromApprovals(
+ Iterable<PatchSetApproval> approvals) {
+ PatchSetApproval first = null;
+ Table<ReviewerStateInternal, Account.Id, Timestamp> reviewers =
+ HashBasedTable.create();
+ for (PatchSetApproval psa : approvals) {
+ if (first == null) {
+ first = psa;
+ } else {
+ checkArgument(
+ first.getKey().getParentKey().getParentKey().equals(
+ psa.getKey().getParentKey().getParentKey()),
+ "multiple change IDs: %s, %s", first.getKey(), psa.getKey());
+ }
+ Account.Id id = psa.getAccountId();
+ if (psa.getValue() != 0) {
+ reviewers.put(REVIEWER, id, psa.getGranted());
+ reviewers.remove(CC, id);
+ } else if (!reviewers.contains(REVIEWER, id)) {
+ reviewers.put(CC, id, psa.getGranted());
+ }
+ }
+ return new ReviewerSet(reviewers);
+ }
+
+ public static ReviewerSet fromTable(
+ Table<ReviewerStateInternal, Account.Id, Timestamp> table) {
+ return new ReviewerSet(table);
+ }
+
+ public static ReviewerSet empty() {
+ return EMPTY;
+ }
+
+ private final ImmutableTable<ReviewerStateInternal, Account.Id, Timestamp>
+ table;
+ private ImmutableSet<Account.Id> accounts;
+
+ private ReviewerSet(Table<ReviewerStateInternal, Account.Id, Timestamp> table) {
+ this.table = ImmutableTable.copyOf(table);
+ }
+
+ public ImmutableSet<Account.Id> all() {
+ if (accounts == null) {
+ // Idempotent and immutable, don't bother locking.
+ accounts = ImmutableSet.copyOf(table.columnKeySet());
+ }
+ return accounts;
+ }
+
+ public ImmutableSet<Account.Id> byState(ReviewerStateInternal state) {
+ return table.row(state).keySet();
+ }
+
+ public ImmutableTable<ReviewerStateInternal, Account.Id, Timestamp>
+ asTable() {
+ return table;
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ return (o instanceof ReviewerSet) && table.equals(((ReviewerSet) o).table);
+ }
+
+ @Override
+ public int hashCode() {
+ return table.hashCode();
+ }
+
+ @Override
+ public String toString() {
+ return getClass().getSimpleName() + table;
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
index 568f816..f93bb72 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
@@ -442,10 +442,10 @@
out.removableReviewers = removableReviewers(ctl, out.labels.values());
out.reviewers = new HashMap<>();
- for (Map.Entry<ReviewerStateInternal, Collection<Account.Id>> e
- : cd.reviewers().asMap().entrySet()) {
+ for (Map.Entry<ReviewerStateInternal, Map<Account.Id, Timestamp>> e
+ : cd.reviewers().asTable().rowMap().entrySet()) {
out.reviewers.put(e.getKey().asReviewerState(),
- toAccountInfo(e.getValue()));
+ toAccountInfo(e.getValue().keySet()));
}
}
@@ -617,7 +617,7 @@
// - They are an explicit reviewer.
// - They ever voted on this change.
Set<Account.Id> allUsers = new HashSet<>();
- allUsers.addAll(cd.reviewers().values());
+ allUsers.addAll(cd.reviewers().all());
for (PatchSetApproval psa : cd.approvals().values()) {
allUsers.add(psa.getAccountId());
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListReviewers.java
index 1e3480f..9a0c691 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ListReviewers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ListReviewers.java
@@ -51,7 +51,7 @@
Map<Account.Id, ReviewerResource> reviewers = new LinkedHashMap<>();
ReviewDb db = dbProvider.get();
for (Account.Id accountId
- : approvalsUtil.getReviewers(db, rsrc.getNotes()).values()) {
+ : approvalsUtil.getReviewers(db, rsrc.getNotes()).all()) {
if (!reviewers.containsKey(accountId)) {
reviewers.put(accountId, resourceFactory.create(rsrc, accountId));
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
index 3defdd7..ffbfc36 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
@@ -19,10 +19,8 @@
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
-import com.google.common.collect.SetMultimap;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
-import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.PatchSet;
@@ -33,6 +31,7 @@
import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.BanCommit;
import com.google.gerrit.server.git.BatchUpdate;
@@ -43,7 +42,6 @@
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.mail.ReplacePatchSetSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
-import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.RefControl;
@@ -106,7 +104,7 @@
private PatchSet patchSet;
private PatchSetInfo patchSetInfo;
private ChangeMessage changeMessage;
- private SetMultimap<ReviewerStateInternal, Account.Id> oldReviewers;
+ private ReviewerSet oldReviewers;
@AssistedInject
public PatchSetInserter(ChangeHooks hooks,
@@ -264,8 +262,8 @@
cm.setFrom(ctx.getUser().getAccountId());
cm.setPatchSet(patchSet, patchSetInfo);
cm.setChangeMessage(changeMessage);
- cm.addReviewers(oldReviewers.get(REVIEWER));
- cm.addExtraCC(oldReviewers.get(CC));
+ cm.addReviewers(oldReviewers.byState(REVIEWER));
+ cm.addExtraCC(oldReviewers.byState(CC));
cm.send();
} catch (Exception err) {
log.error("Cannot send email for new patch set on change "
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java
index ff5185e..4304669 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PublishDraftPatchSet.java
@@ -207,7 +207,7 @@
throws OrmException, IOException {
LabelTypes labelTypes = ctx.getControl().getLabelTypes();
Collection<Account.Id> oldReviewers = approvalsUtil.getReviewers(
- ctx.getDb(), ctx.getNotes()).values();
+ ctx.getDb(), ctx.getNotes()).all();
RevCommit commit = ctx.getRevWalk().parseCommit(
ObjectId.fromString(patchSet.getRevision().get()));
patchSetInfo = patchSetInfoFactory.get(ctx.getRevWalk(), commit, psId);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java
index 74a8866..d45d260 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Reviewers.java
@@ -83,6 +83,6 @@
private Collection<Account.Id> fetchAccountIds(ChangeResource rsrc)
throws OrmException {
return approvalsUtil.getReviewers(
- dbProvider.get(), rsrc.getNotes()).values();
+ dbProvider.get(), rsrc.getNotes()).all();
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
index 5807c26..56daccc 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java
@@ -203,7 +203,7 @@
public void addAllReviewers(ReviewDb db, ChangeAttribute a, ChangeNotes notes)
throws OrmException {
Collection<Account.Id> reviewers =
- approvalsUtil.getReviewers(db, notes).values();
+ approvalsUtil.getReviewers(db, notes).all();
if (!reviewers.isEmpty()) {
a.allReviewers = Lists.newArrayListWithCapacity(reviewers.size());
for (Account.Id id : reviewers) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java
index 367159d..ad543ea 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/ChangeEmail.java
@@ -111,7 +111,7 @@
appendText(velocifyFile("ChangeFooter.vm"));
try {
TreeSet<String> names = new TreeSet<>();
- for (Account.Id who : changeData.reviewers().values()) {
+ for (Account.Id who : changeData.reviewers().all()) {
names.add(getNameEmailFor(who));
}
for (String name : names) {
@@ -337,7 +337,7 @@
}
try {
- for (Account.Id id : changeData.reviewers().values()) {
+ for (Account.Id id : changeData.reviewers().all()) {
add(RecipientType.CC, id);
}
} catch (OrmException err) {
@@ -353,7 +353,7 @@
}
try {
- for (Account.Id id : changeData.reviewers().get(REVIEWER)) {
+ for (Account.Id id : changeData.reviewers().byState(REVIEWER)) {
add(RecipientType.CC, id);
}
} catch (OrmException err) {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java
index af8a3f69..a1274c3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MailUtil.java
@@ -17,12 +17,11 @@
import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
-import com.google.common.collect.Multimap;
import com.google.gerrit.common.FooterConstants;
import com.google.gerrit.common.errors.NoSuchAccountException;
import com.google.gerrit.reviewdb.client.Account;
+import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.account.AccountResolver;
-import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gwtorm.server.OrmException;
import org.eclipse.jgit.revwalk.FooterKey;
@@ -58,10 +57,10 @@
}
public static MailRecipients getRecipientsFromReviewers(
- Multimap<ReviewerStateInternal, Account.Id> reviewers) {
+ ReviewerSet reviewers) {
MailRecipients recipients = new MailRecipients();
- recipients.reviewers.addAll(reviewers.get(REVIEWER));
- recipients.cc.addAll(reviewers.get(CC));
+ recipients.reviewers.addAll(reviewers.byState(REVIEWER));
+ recipients.cc.addAll(reviewers.byState(CC));
return recipients;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
index 0dfd8c9..1f40498 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
@@ -40,6 +40,7 @@
import com.google.common.collect.Multimap;
import com.google.common.collect.Ordering;
import com.google.common.collect.Sets;
+import com.google.common.collect.Table;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
@@ -50,6 +51,7 @@
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.PatchLineCommentsUtil;
+import com.google.gerrit.server.ReviewerSet;
import com.google.gwtorm.client.Column;
import com.google.gwtorm.server.OrmException;
@@ -59,6 +61,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Comparator;
+import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedList;
import java.util.List;
@@ -84,12 +87,15 @@
throws OrmException {
db.changes().beginTransaction(id);
try {
+ List<PatchSetApproval> approvals =
+ db.patchSetApprovals().byChange(id).toList();
return new ChangeBundle(
db.changes().get(id),
db.changeMessages().byChange(id),
db.patchSets().byChange(id),
- db.patchSetApprovals().byChange(id),
+ approvals,
db.patchComments().byChange(id),
+ ReviewerSet.fromApprovals(approvals),
Source.REVIEW_DB);
} finally {
db.rollback();
@@ -106,6 +112,7 @@
Iterables.concat(
plcUtil.draftByChange(null, notes),
plcUtil.publishedByChange(null, notes)),
+ notes.getReviewers(),
Source.NOTE_DB);
}
@@ -156,7 +163,6 @@
return CHANGE_MESSAGE_ORDER.immutableSortedCopy(in);
}
-
private static TreeMap<PatchSet.Id, PatchSet> patchSetMap(Iterable<PatchSet> in) {
TreeMap<PatchSet.Id, PatchSet> out = new TreeMap<>(
new Comparator<PatchSet.Id>() {
@@ -256,6 +262,7 @@
patchSetApprovals;
private final ImmutableMap<PatchLineComment.Key, PatchLineComment>
patchLineComments;
+ private final ReviewerSet reviewers;
private final Source source;
public ChangeBundle(
@@ -264,6 +271,7 @@
Iterable<PatchSet> patchSets,
Iterable<PatchSetApproval> patchSetApprovals,
Iterable<PatchLineComment> patchLineComments,
+ ReviewerSet reviewers,
Source source) {
this.change = checkNotNull(change);
this.changeMessages = changeMessageList(changeMessages);
@@ -272,6 +280,7 @@
ImmutableMap.copyOf(patchSetApprovalMap(patchSetApprovals));
this.patchLineComments =
ImmutableMap.copyOf(patchLineCommentMap(patchLineComments));
+ this.reviewers = checkNotNull(reviewers);
this.source = checkNotNull(source);
for (ChangeMessage m : this.changeMessages) {
@@ -309,6 +318,10 @@
return patchLineComments.values();
}
+ public ReviewerSet getReviewers() {
+ return reviewers;
+ }
+
public Source getSource() {
return source;
}
@@ -319,6 +332,7 @@
diffChangeMessages(diffs, this, o);
diffPatchSets(diffs, this, o);
diffPatchSetApprovals(diffs, this, o);
+ diffReviewers(diffs, this, o);
diffPatchLineComments(diffs, this, o);
return ImmutableList.copyOf(diffs);
}
@@ -661,6 +675,39 @@
}
}
+ @AutoValue
+ static abstract class ReviewerKey {
+ private static Map<ReviewerKey, Timestamp> toMap(ReviewerSet reviewers) {
+ Map<ReviewerKey, Timestamp> result = new HashMap<>();
+ for (Table.Cell<ReviewerStateInternal, Account.Id, Timestamp> c :
+ reviewers.asTable().cellSet()) {
+ result.put(new AutoValue_ChangeBundle_ReviewerKey(
+ c.getRowKey(), c.getColumnKey()), c.getValue());
+ }
+ return result;
+ }
+
+ abstract ReviewerStateInternal state();
+ abstract Account.Id account();
+
+ @Override
+ public String toString() {
+ return state() + "," + account();
+ }
+ }
+
+ private static void diffReviewers(List<String> diffs,
+ ChangeBundle bundleA, ChangeBundle bundleB) {
+ Map<ReviewerKey, Timestamp> as = ReviewerKey.toMap(bundleA.reviewers);
+ Map<ReviewerKey, Timestamp> bs = ReviewerKey.toMap(bundleB.reviewers);
+ for (ReviewerKey k : diffKeySets(diffs, as, bs)) {
+ Timestamp a = as.get(k);
+ Timestamp b = bs.get(k);
+ String desc = describe(k);
+ diffTimestamps(diffs, desc, bundleA, a, bundleB, b, "timestamp");
+ }
+ }
+
private static void diffPatchLineComments(List<String> diffs,
ChangeBundle bundleA, ChangeBundle bundleB) {
Map<PatchLineComment.Key, PatchLineComment> as =
@@ -825,9 +872,14 @@
private static String keyClass(Object obj) {
Class<?> clazz = obj.getClass();
String name = clazz.getSimpleName();
- checkArgument(name.equals("Key") || name.equals("Id"),
+ checkArgument(name.endsWith("Key") || name.endsWith("Id"),
"not an Id/Key class: %s", name);
- return clazz.getEnclosingClass().getSimpleName() + "." + name;
+ if (name.equals("Key") || name.equals("Id")) {
+ return clazz.getEnclosingClass().getSimpleName() + "." + name;
+ } else if (name.startsWith("AutoValue_")) {
+ return name.substring(name.lastIndexOf('_') + 1);
+ }
+ return name;
}
@Override
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
index a6cd8fa..926194b 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java
@@ -28,7 +28,6 @@
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSetMultimap;
import com.google.common.collect.ImmutableSortedMap;
import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Iterables;
@@ -36,6 +35,7 @@
import com.google.common.collect.Multimap;
import com.google.common.collect.Multimaps;
import com.google.common.collect.Ordering;
+import com.google.common.collect.Tables;
import com.google.common.util.concurrent.AsyncFunction;
import com.google.common.util.concurrent.CheckedFuture;
import com.google.common.util.concurrent.Futures;
@@ -55,6 +55,7 @@
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.reviewdb.server.ReviewDbUtil;
+import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.git.RefCache;
import com.google.gerrit.server.git.RepoRefCache;
import com.google.gerrit.server.project.NoSuchChangeException;
@@ -381,7 +382,7 @@
private ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets;
private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
- private ImmutableSetMultimap<ReviewerStateInternal, Account.Id> reviewers;
+ private ReviewerSet reviewers;
private ImmutableList<Account.Id> allPastReviewers;
private ImmutableList<SubmitRecord> submitRecords;
private ImmutableList<ChangeMessage> allChangeMessages;
@@ -420,7 +421,7 @@
return approvals;
}
- public ImmutableSetMultimap<ReviewerStateInternal, Account.Id> getReviewers() {
+ public ReviewerSet getReviewers() {
return reviewers;
}
@@ -583,13 +584,7 @@
} else {
hashtags = ImmutableSet.of();
}
- ImmutableSetMultimap.Builder<ReviewerStateInternal, Account.Id> reviewers =
- ImmutableSetMultimap.builder();
- for (Map.Entry<Account.Id, ReviewerStateInternal> e
- : parser.reviewers.entrySet()) {
- reviewers.put(e.getValue(), e.getKey());
- }
- this.reviewers = reviewers.build();
+ this.reviewers = ReviewerSet.fromTable(Tables.transpose(parser.reviewers));
this.allPastReviewers = ImmutableList.copyOf(parser.allPastReviewers);
submitRecords = ImmutableList.copyOf(parser.submitRecords);
@@ -598,7 +593,7 @@
@Override
protected void loadDefaults() {
approvals = ImmutableListMultimap.of();
- reviewers = ImmutableSetMultimap.of();
+ reviewers = ReviewerSet.empty();
submitRecords = ImmutableList.of();
allChangeMessages = ImmutableList.of();
changeMessagesByPatchSet = ImmutableListMultimap.of();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
index 9d9e180..02dd441 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
@@ -36,6 +36,7 @@
import com.google.common.base.Splitter;
import com.google.common.base.Supplier;
import com.google.common.collect.ArrayListMultimap;
+import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableSet;
@@ -94,7 +95,7 @@
private static final RevId PARTIAL_PATCH_SET =
new RevId("INVALID PARTIAL PATCH SET");
- final Map<Account.Id, ReviewerStateInternal> reviewers;
+ final Table<Account.Id, ReviewerStateInternal, Timestamp> reviewers;
final List<Account.Id> allPastReviewers;
final List<SubmitRecord> submitRecords;
final Multimap<RevId, PatchLineComment> comments;
@@ -134,7 +135,7 @@
this.noteUtil = noteUtil;
this.metrics = metrics;
approvals = new HashMap<>();
- reviewers = new LinkedHashMap<>();
+ reviewers = HashBasedTable.create();
allPastReviewers = new ArrayList<>();
submitRecords = Lists.newArrayListWithExpectedSize(1);
allChangeMessages = new ArrayList<>();
@@ -156,7 +157,7 @@
parse(commit);
}
parseNotes();
- allPastReviewers.addAll(reviewers.keySet());
+ allPastReviewers.addAll(reviewers.rowKeySet());
pruneReviewers();
updatePatchSetStates();
checkMandatoryFooters();
@@ -262,7 +263,7 @@
for (ReviewerStateInternal state : ReviewerStateInternal.values()) {
for (String line : commit.getFooterLineValues(state.getFooterKey())) {
- parseReviewer(state, line);
+ parseReviewer(ts, state, line);
}
// Don't update timestamp when a reviewer was added, matching RevewDb
// behavior.
@@ -698,27 +699,27 @@
return noteUtil.parseIdent(commit.getAuthorIdent(), id);
}
- private void parseReviewer(ReviewerStateInternal state, String line)
- throws ConfigInvalidException {
+ private void parseReviewer(Timestamp ts, ReviewerStateInternal state,
+ String line) throws ConfigInvalidException {
PersonIdent ident = RawParseUtils.parsePersonIdent(line);
if (ident == null) {
throw invalidFooter(state.getFooterKey(), line);
}
Account.Id accountId = noteUtil.parseIdent(ident, id);
- if (!reviewers.containsKey(accountId)) {
- reviewers.put(accountId, state);
+ if (!reviewers.containsRow(accountId)) {
+ reviewers.put(accountId, state, ts);
}
}
private void pruneReviewers() {
- Iterator<Map.Entry<Account.Id, ReviewerStateInternal>> rit =
- reviewers.entrySet().iterator();
+ Iterator<Table.Cell<Account.Id, ReviewerStateInternal, Timestamp>> rit =
+ reviewers.cellSet().iterator();
while (rit.hasNext()) {
- Map.Entry<Account.Id, ReviewerStateInternal> e = rit.next();
- if (e.getValue() == ReviewerStateInternal.REMOVED) {
+ Table.Cell<Account.Id, ReviewerStateInternal, Timestamp> e = rit.next();
+ if (e.getColumnKey() == ReviewerStateInternal.REMOVED) {
rit.remove();
for (Table<Account.Id, ?, ?> curr : approvals.values()) {
- curr.rowKeySet().remove(e.getKey());
+ curr.rowKeySet().remove(e.getRowKey());
}
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
index 9aa69bf..5f16eb4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeRebuilderImpl.java
@@ -36,6 +36,7 @@
import com.google.common.collect.Multimap;
import com.google.common.collect.Ordering;
import com.google.common.collect.Sets;
+import com.google.common.collect.Table;
import com.google.common.primitives.Ints;
import com.google.gerrit.common.FormatUtil;
import com.google.gerrit.common.Nullable;
@@ -277,6 +278,11 @@
}
}
+ for (Table.Cell<ReviewerStateInternal, Account.Id, Timestamp> r :
+ bundle.getReviewers().asTable().cellSet()) {
+ events.add(new ReviewerEvent(r, change.getCreatedOn()));
+ }
+
Change noteDbChange = new Change(null, null, null, null, null);
for (ChangeMessage msg : bundle.getChangeMessages()) {
if (msg.getPatchSetId() == null || psIds.contains(msg.getPatchSetId())) {
@@ -714,6 +720,33 @@
}
}
+ private static class ReviewerEvent extends Event {
+ private Table.Cell<ReviewerStateInternal, Account.Id, Timestamp> reviewer;
+
+ ReviewerEvent(
+ Table.Cell<ReviewerStateInternal, Account.Id, Timestamp> reviewer,
+ Timestamp changeCreatedOn) {
+ super(
+ // Reviewers aren't generally associated with a particular patch set
+ // (although as an implementation detail they were in ReviewDb). Just
+ // use the latest patch set at the time of the event.
+ null,
+ reviewer.getColumnKey(), reviewer.getValue(), changeCreatedOn, null);
+ this.reviewer = reviewer;
+ }
+
+ @Override
+ boolean uniquePerUpdate() {
+ return false;
+ }
+
+ @Override
+ void apply(ChangeUpdate update) throws IOException, OrmException {
+ checkUpdate(update);
+ update.putReviewer(reviewer.getColumnKey(), reviewer.getRowKey());
+ }
+ }
+
private static class PatchSetEvent extends Event {
private final Change change;
private final PatchSet ps;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
index 4bda245..62c6d5e 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java
@@ -340,7 +340,7 @@
public boolean isReviewer(ReviewDb db, @Nullable ChangeData cd)
throws OrmException {
if (getUser().isIdentifiedUser()) {
- Collection<Account.Id> results = changeData(db, cd).reviewers().values();
+ Collection<Account.Id> results = changeData(db, cd).reviewers().all();
return results.contains(getUser().getAccountId());
}
return false;
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
index ed7b134..93a16ad 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java
@@ -30,7 +30,6 @@
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
-import com.google.common.collect.SetMultimap;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.common.data.SubmitRecord;
import com.google.gerrit.common.data.SubmitTypeRecord;
@@ -50,13 +49,13 @@
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.PatchLineCommentsUtil;
import com.google.gerrit.server.PatchSetUtil;
+import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.StarredChangesUtil;
import com.google.gerrit.server.change.MergeabilityCache;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.NotesMigration;
-import com.google.gerrit.server.notedb.ReviewerStateInternal;
import com.google.gerrit.server.patch.PatchList;
import com.google.gerrit.server.patch.PatchListCache;
import com.google.gerrit.server.patch.PatchListEntry;
@@ -905,8 +904,7 @@
return Optional.absent();
}
- public SetMultimap<ReviewerStateInternal, Account.Id> reviewers()
- throws OrmException {
+ public ReviewerSet reviewers() throws OrmException {
return approvalsUtil.getReviewers(notes(), approvals().values());
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java
index eb07250..2da8c54 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerPredicate.java
@@ -40,7 +40,7 @@
object.change().getStatus() == Change.Status.DRAFT) {
return false;
}
- for (Account.Id accountId : object.reviewers().values()) {
+ for (Account.Id accountId : object.reviewers().all()) {
if (id.equals(accountId)) {
return true;
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerinPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerinPredicate.java
index eb93451..76a02432 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerinPredicate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ReviewerinPredicate.java
@@ -37,7 +37,7 @@
@Override
public boolean match(final ChangeData object) throws OrmException {
- for (Account.Id accountId : object.reviewers().values()) {
+ for (Account.Id accountId : object.reviewers().all()) {
IdentifiedUser reviewer = userFactory.create(accountId);
if (reviewer.getEffectiveGroups().contains(uuid)) {
return true;
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
index ea1120f..4306d74 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeBundleTest.java
@@ -14,14 +14,18 @@
package com.google.gerrit.server.notedb;
+import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.truth.Truth.assertThat;
import static com.google.gerrit.common.TimeUtil.roundToSecond;
import static com.google.gerrit.server.notedb.ChangeBundle.Source.NOTE_DB;
import static com.google.gerrit.server.notedb.ChangeBundle.Source.REVIEW_DB;
+import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.SECONDS;
+import com.google.common.collect.HashBasedTable;
import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Table;
import com.google.gerrit.common.TimeUtil;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
@@ -33,6 +37,7 @@
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RevId;
+import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.testutil.TestChanges;
import com.google.gerrit.testutil.TestTimeUtil;
import com.google.gwtorm.client.KeyUtil;
@@ -113,9 +118,9 @@
Change c2 = TestChanges.newChange(project, accountId);
int id2 = c2.getId().get();
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"changeId differs for Changes: {" + id1 + "} != {" + id2 + "}",
@@ -131,9 +136,9 @@
new Project.NameKey("project"), new Account.Id(100));
Change c2 = clone(c1);
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
@@ -153,9 +158,9 @@
// Both are ReviewDb, exact timestamp match is required.
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"createdOn differs for Change.Id " + c1.getId() + ":"
+ " {2009-09-30 17:00:01.0} != {2009-09-30 17:00:02.0}",
@@ -164,9 +169,9 @@
// One NoteDb, slop is allowed.
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
assertNoDiffs(b2, b1);
@@ -175,9 +180,9 @@
Change c3 = clone(c1);
c3.setLastUpdatedOn(TimeUtil.nowTs());
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
ChangeBundle b3 = new ChangeBundle(c3, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
String msg = "effective last updated time differs for Change.Id "
+ c1.getId() + " in NoteDb vs. ReviewDb:"
+ " {2009-09-30 17:00:01.0} != {2009-09-30 17:00:10.0}";
@@ -197,27 +202,27 @@
// Both ReviewDb, exact match required.
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"originalSubject differs for Change.Id " + c1.getId() + ":"
+ " {Original A} != {Original B}");
// Both NoteDb, exact match required.
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
assertDiffs(b1, b2,
"originalSubject differs for Change.Id " + c1.getId() + ":"
+ " {Original A} != {Original B}");
// One ReviewDb, one NoteDb, original subject is ignored.
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
assertNoDiffs(b1, b2);
assertNoDiffs(b2, b1);
}
@@ -233,25 +238,25 @@
// Both ReviewDb, exact match required.
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"topic differs for Change.Id " + c1.getId() + ":"
+ " {} != {null}");
// Topic ignored if ReviewDb is empty and NoteDb is null.
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
assertNoDiffs(b1, b2);
// Exact match still required if NoteDb has empty value (not realistic).
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"topic differs for Change.Id " + c1.getId() + ":"
+ " {} != {null}");
@@ -260,9 +265,9 @@
Change c3 = clone(c1);
c3.setTopic("topic");
b1 = new ChangeBundle(c3, messages(), patchSets(), approvals(), comments(),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
assertDiffs(b1, b2,
"topic differs for Change.Id " + c1.getId() + ":"
+ " {topic} != {null}");
@@ -284,16 +289,16 @@
// Both ReviewDb, exact match required.
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(),
- approvals(a), comments(), REVIEW_DB);
+ approvals(a), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(),
- approvals(a), comments(), REVIEW_DB);
+ approvals(a), comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"effective last updated time differs for Change.Id " + c1.getId() + ":"
+ " {2009-09-30 17:00:00.0} != {2009-09-30 17:00:06.0}");
// NoteDb allows latest timestamp from all entities in bundle.
b2 = new ChangeBundle(c2, messages(), patchSets(),
- approvals(a), comments(), NOTE_DB);
+ approvals(a), comments(), reviewers(), NOTE_DB);
assertNoDiffs(b1, b2);
}
@@ -314,18 +319,18 @@
// ReviewDb has later lastUpdatedOn timestamp than NoteDb, allowed since
// NoteDb matches the latest timestamp of a non-Change entity.
ChangeBundle b1 = new ChangeBundle(c2, messages(), patchSets(),
- approvals(a), comments(), REVIEW_DB);
+ approvals(a), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c1, messages(), patchSets(),
- approvals(a), comments(), NOTE_DB);
+ approvals(a), comments(), reviewers(), NOTE_DB);
assertThat(b1.getChange().getLastUpdatedOn())
.isGreaterThan(b2.getChange().getLastUpdatedOn());
assertNoDiffs(b1, b2);
// Timestamps must actually match if Change is the only entity.
b1 = new ChangeBundle(c2, messages(), patchSets(), approvals(), comments(),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
b2 = new ChangeBundle(c1, messages(), patchSets(), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
assertDiffs(b1, b2,
"effective last updated time differs for Change.Id " + c1.getId()
+ " in NoteDb vs. ReviewDb:"
@@ -344,25 +349,25 @@
// Both ReviewDb, exact match required.
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"subject differs for Change.Id " + c1.getId() + ":"
+ " {Change subject} != {Change sub}");
// ReviewDb has shorter subject, allowed.
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
// NoteDb has shorter subject, not allowed.
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
assertDiffs(b1, b2,
"subject differs for Change.Id " + c1.getId() + ":"
+ " {Change subject} != {Change sub}");
@@ -379,18 +384,18 @@
// Both ReviewDb, exact match required.
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"subject differs for Change.Id " + c1.getId() + ":"
+ " {Change subject} != { Change subject}");
// ReviewDb is missing leading spaces, allowed.
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
assertNoDiffs(b2, b1);
}
@@ -406,18 +411,18 @@
// Both ReviewDb.
ChangeBundle b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"subject differs for Change.Id " + c1.getId() + ":"
+ " {Change subject} != {\tChange subject}");
// One NoteDb.
b1 = new ChangeBundle(c1, messages(), patchSets(), approvals(),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
b2 = new ChangeBundle(c2, messages(), patchSets(), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"subject differs for Change.Id " + c1.getId() + ":"
+ " {Change subject} != {\tChange subject}");
@@ -437,9 +442,9 @@
new ChangeMessage.Key(c.getId(), "uuid2"),
accountId, TimeUtil.nowTs(), c.currentPatchSetId());
ChangeBundle b1 = new ChangeBundle(c, messages(cm1), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(cm2), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"ChangeMessage.Key sets differ:"
@@ -455,9 +460,9 @@
cm1.setMessage("message 1");
ChangeMessage cm2 = clone(cm1);
ChangeBundle b1 = new ChangeBundle(c, messages(cm1), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(cm2), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
@@ -479,9 +484,9 @@
cm2.getKey().set("uuid2");
ChangeBundle b1 = new ChangeBundle(c, messages(cm1), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(cm2), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
// Both are ReviewDb, exact UUID match is required.
assertDiffs(b1, b2,
"ChangeMessage.Key sets differ:"
@@ -489,9 +494,9 @@
// One NoteDb, UUIDs are ignored.
b1 = new ChangeBundle(c, messages(cm1), latest(c), approvals(), comments(),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
b2 = new ChangeBundle(c, messages(cm2), latest(c), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
assertNoDiffs(b1, b2);
}
@@ -510,18 +515,18 @@
// Both ReviewDb: Uses same keySet diff as other types.
ChangeBundle b1 = new ChangeBundle(c, messages(cm1, cm2), latest(c),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(cm1), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"ChangeMessage.Key sets differ: [" + id
+ ",uuid2] only in A; [] only in B");
// One NoteDb: UUIDs in keys can't be used for comparison, just diff counts.
b1 = new ChangeBundle(c, messages(cm1, cm2), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
b2 = new ChangeBundle(c, messages(cm1), latest(c), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
assertDiffs(b1, b2,
"ChangeMessages differ for Change.Id " + id + "\n"
+ "Only in A:\n " + cm2);
@@ -544,9 +549,9 @@
cm3.getKey().set("uuid2"); // Differs only in UUID.
ChangeBundle b1 = new ChangeBundle(c, messages(cm1, cm3), latest(c),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(cm2, cm3), latest(c),
- approvals(), comments(), NOTE_DB);
+ approvals(), comments(), reviewers(), NOTE_DB);
// Implementation happens to pair up cm1 in b1 with cm3 in b2 because it
// depends on iteration order and doesn't care about UUIDs. The important
// thing is that there's some diff.
@@ -572,18 +577,18 @@
// Both are ReviewDb, exact timestamp match is required.
ChangeBundle b1 = new ChangeBundle(c, messages(cm1), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(cm2), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"writtenOn differs for ChangeMessage.Key " + c.getId() + ",uuid1:"
+ " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:03.0}");
// One NoteDb, slop is allowed.
b1 = new ChangeBundle(c, messages(cm1), latest(c), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
b2 = new ChangeBundle(c, messages(cm2), latest(c), approvals(), comments(),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
assertNoDiffs(b2, b1);
@@ -592,9 +597,9 @@
ChangeMessage cm3 = clone(cm1);
cm3.setWrittenOn(TimeUtil.nowTs());
b1 = new ChangeBundle(c, messages(cm1), latest(c), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
ChangeBundle b3 = new ChangeBundle(c, messages(cm3), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
int id = c.getId().get();
assertDiffs(b1, b3,
"ChangeMessages differ for Change.Id " + id + "\n"
@@ -619,9 +624,9 @@
cm2.setPatchSetId(null);
ChangeBundle b1 = new ChangeBundle(c, messages(cm1), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(cm2), latest(c), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
// Both are ReviewDb, exact patch set ID match is required.
assertDiffs(b1, b2,
@@ -630,16 +635,16 @@
// Null patch set ID on ReviewDb is ignored.
b1 = new ChangeBundle(c, messages(cm1), latest(c), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
b2 = new ChangeBundle(c, messages(cm2), latest(c), approvals(), comments(),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
// Null patch set ID on NoteDb is not ignored (but is not realistic).
b1 = new ChangeBundle(c, messages(cm1), latest(c), approvals(), comments(),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
b2 = new ChangeBundle(c, messages(cm2), latest(c), approvals(), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
assertDiffs(b1, b2,
"ChangeMessages differ for Change.Id " + id + "\n"
+ "Only in A:\n " + cm1 + "\n"
@@ -665,9 +670,9 @@
ps2.setCreatedOn(TimeUtil.nowTs());
ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(ps2),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(ps1, ps2),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"PatchSet.Id sets differ:"
@@ -683,9 +688,9 @@
ps1.setCreatedOn(TimeUtil.nowTs());
PatchSet ps2 = clone(ps1);
ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(ps1),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(ps2),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
@@ -709,18 +714,18 @@
// Both are ReviewDb, exact timestamp match is required.
ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(ps1),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(ps2),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"createdOn differs for PatchSet.Id " + c.getId() + ",1:"
+ " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:03.0}");
// One NoteDb, slop is allowed.
b1 = new ChangeBundle(c, messages(), patchSets(ps1), approvals(),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
b2 = new ChangeBundle(c, messages(), patchSets(ps2), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
// But not too much slop.
@@ -728,9 +733,9 @@
PatchSet ps3 = clone(ps1);
ps3.setCreatedOn(TimeUtil.nowTs());
b1 = new ChangeBundle(c, messages(), patchSets(ps1), approvals(),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
ChangeBundle b3 = new ChangeBundle(c, messages(), patchSets(ps3),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
String msg = "createdOn differs for PatchSet.Id " + c.getId()
+ ",1 in NoteDb vs. ReviewDb:"
+ " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:10.0}";
@@ -752,16 +757,16 @@
ps2.setPushCertificate(ps2.getPushCertificate() + "\n\n");
ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(ps1),
- approvals(), comments(), NOTE_DB);
+ approvals(), comments(), reviewers(), NOTE_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(ps2),
- approvals(), comments(), REVIEW_DB);
+ approvals(), comments(), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
assertNoDiffs(b2, b1);
b1 = new ChangeBundle(c, messages(), patchSets(ps1), approvals(),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
b2 = new ChangeBundle(c, messages(), patchSets(ps2), approvals(),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
assertNoDiffs(b1, b2);
assertNoDiffs(b2, b1);
}
@@ -793,24 +798,24 @@
// Both ReviewDb.
ChangeBundle b1 = new ChangeBundle(c, messages(), patchSets(ps1),
- approvals(a1), comments(), REVIEW_DB);
+ approvals(a1), comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), patchSets(ps1, ps2),
- approvals(a1, a2), comments(), REVIEW_DB);
+ approvals(a1, a2), comments(), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
// One NoteDb.
b1 = new ChangeBundle(c, messages(), patchSets(ps1), approvals(a1),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
b2 = new ChangeBundle(c, messages(), patchSets(ps1, ps2), approvals(a1, a2),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
assertNoDiffs(b2, b1);
// Both NoteDb.
b1 = new ChangeBundle(c, messages(), patchSets(ps1), approvals(a1),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
b2 = new ChangeBundle(c, messages(), patchSets(ps1, ps2), approvals(a1, a2),
- comments(), NOTE_DB);
+ comments(), reviewers(), NOTE_DB);
assertNoDiffs(b1, b2);
}
@@ -830,9 +835,9 @@
TimeUtil.nowTs());
ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(a1),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(a2),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"PatchSetApproval.Key sets differ:"
@@ -850,9 +855,9 @@
TimeUtil.nowTs());
PatchSetApproval a2 = clone(a1);
ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(a1),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(a2),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
@@ -877,9 +882,9 @@
// Both are ReviewDb, exact timestamp match is required.
ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(a1),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(a2),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"granted differs for PatchSetApproval.Key "
+ c.getId() + "%2C1,100,Code-Review:"
@@ -887,9 +892,9 @@
// One NoteDb, slop is allowed.
b1 = new ChangeBundle(c, messages(), latest(c), approvals(a1), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
b2 = new ChangeBundle(c, messages(), latest(c), approvals(a2), comments(),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
// But not too much slop.
@@ -897,9 +902,9 @@
PatchSetApproval a3 = clone(a1);
a3.setGranted(TimeUtil.nowTs());
b1 = new ChangeBundle(c, messages(), latest(c), approvals(a1), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
ChangeBundle b3 = new ChangeBundle(c, messages(), latest(c), approvals(a3),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
String msg = "granted differs for PatchSetApproval.Key "
+ c.getId() + "%2C1,100,Code-Review in NoteDb vs. ReviewDb:"
+ " {2009-09-30 17:00:07.0} != {2009-09-30 17:00:15.0}";
@@ -923,9 +928,9 @@
// Both are ReviewDb, exact match is required.
ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(a1),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(a2),
- comments(), REVIEW_DB);
+ comments(), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"granted differs for PatchSetApproval.Key "
+ c.getId() + "%2C1,100,Code-Review:"
@@ -933,14 +938,91 @@
// Truncating NoteDb timestamp is allowed.
b1 = new ChangeBundle(c, messages(), latest(c), approvals(a1), comments(),
- NOTE_DB);
+ reviewers(), NOTE_DB);
b2 = new ChangeBundle(c, messages(), latest(c), approvals(a2), comments(),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
assertNoDiffs(b2, b1);
}
@Test
+ public void diffReviewerKeySets() throws Exception {
+ Change c = TestChanges.newChange(project, accountId);
+ Timestamp now = TimeUtil.nowTs();
+ ReviewerSet r1 = reviewers(REVIEWER, new Account.Id(1), now);
+ ReviewerSet r2 = reviewers(REVIEWER, new Account.Id(2), now);
+
+ ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(),
+ comments(), r1, REVIEW_DB);
+ ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(),
+ comments(), r2, REVIEW_DB);
+ assertNoDiffs(b1, b1);
+ assertNoDiffs(b2, b2);
+ assertDiffs(b1, b2,
+ "ReviewerKey sets differ:"
+ + " [REVIEWER,1] only in A;"
+ + " [REVIEWER,2] only in B");
+ }
+
+ @Test
+ public void diffReviewerTimestamps() throws Exception {
+ Change c = TestChanges.newChange(project, accountId);
+ ReviewerSet r1 = reviewers(REVIEWER, new Account.Id(1), TimeUtil.nowTs());
+ ReviewerSet r2 = reviewers(REVIEWER, new Account.Id(1), TimeUtil.nowTs());
+
+ ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(),
+ comments(), r1, REVIEW_DB);
+ ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(),
+ comments(), r2, REVIEW_DB);
+ assertDiffs(b1, b2,
+ "timestamp differs for ReviewerKey REVIEWER,1:"
+ + " {2009-09-30 17:00:06.0} != {2009-09-30 17:00:12.0}");
+
+ b1 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r1,
+ REVIEW_DB);
+ b2 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r2,
+ NOTE_DB);
+ assertDiffs(b1, b2,
+ "timestamp differs for ReviewerKey REVIEWER,1 in NoteDb vs. ReviewDb:"
+ + " {2009-09-30 17:00:12.0} != {2009-09-30 17:00:06.0}");
+ }
+
+ @Test
+ public void diffReviewerTimestampsAllowsSlop() throws Exception {
+ subWindowResolution();
+ Change c = TestChanges.newChange(project, accountId);
+ ReviewerSet r1 = reviewers(REVIEWER, new Account.Id(1), TimeUtil.nowTs());
+ ReviewerSet r2 = reviewers(REVIEWER, new Account.Id(1), TimeUtil.nowTs());
+
+ // Both are ReviewDb, exact timestamp match is required.
+ ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(),
+ comments(), r1, REVIEW_DB);
+ ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(),
+ comments(), r2, REVIEW_DB);
+ assertDiffs(b1, b2,
+ "timestamp differs for ReviewerKey REVIEWER,1:"
+ + " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:03.0}");
+
+ // One NoteDb, slop is allowed
+ b1 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r1,
+ NOTE_DB);
+ b2 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r2,
+ REVIEW_DB);
+ assertNoDiffs(b1, b2);
+
+ // But not too much slop.
+ superWindowResolution();
+ ReviewerSet r3 = reviewers(REVIEWER, new Account.Id(1), TimeUtil.nowTs());
+ b1 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(), r1,
+ NOTE_DB);
+ ChangeBundle b3 = new ChangeBundle(c, messages(), latest(c), approvals(),
+ comments(), r3, REVIEW_DB);
+ assertDiffs(b1, b3,
+ "timestamp differs for ReviewerKey REVIEWER,1 in NoteDb vs. ReviewDb:"
+ + " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:10.0}");
+ }
+
+ @Test
public void diffPatchLineCommentKeySets() throws Exception {
Change c = TestChanges.newChange(project, accountId);
int id = c.getId().get();
@@ -954,9 +1036,9 @@
5, accountId, null, TimeUtil.nowTs());
ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(),
- comments(c1), REVIEW_DB);
+ comments(c1), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(),
- comments(c2), REVIEW_DB);
+ comments(c2), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"PatchLineComment.Key sets differ:"
@@ -973,9 +1055,9 @@
5, accountId, null, TimeUtil.nowTs());
PatchLineComment c2 = clone(c1);
ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(),
- comments(c1), REVIEW_DB);
+ comments(c1), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(),
- comments(c2), REVIEW_DB);
+ comments(c2), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
@@ -999,9 +1081,9 @@
// Both are ReviewDb, exact timestamp match is required.
ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(),
- comments(c1), REVIEW_DB);
+ comments(c1), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(),
- comments(c2), REVIEW_DB);
+ comments(c2), reviewers(), REVIEW_DB);
assertDiffs(b1, b2,
"writtenOn differs for PatchLineComment.Key "
+ c.getId() + ",1,filename,uuid:"
@@ -1009,9 +1091,9 @@
// One NoteDb, slop is allowed.
b1 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(c1),
- NOTE_DB);
+ reviewers(), NOTE_DB);
b2 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(c2),
- REVIEW_DB);
+ reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
// But not too much slop.
@@ -1019,9 +1101,9 @@
PatchLineComment c3 = clone(c1);
c3.setWrittenOn(TimeUtil.nowTs());
b1 = new ChangeBundle(c, messages(), latest(c), approvals(), comments(c1),
- NOTE_DB);
+ reviewers(), NOTE_DB);
ChangeBundle b3 = new ChangeBundle(c, messages(), latest(c), approvals(),
- comments(c3), REVIEW_DB);
+ comments(c3), reviewers(), REVIEW_DB);
String msg = "writtenOn differs for PatchLineComment.Key " + c.getId()
+ ",1,filename,uuid in NoteDb vs. ReviewDb:"
+ " {2009-09-30 17:00:02.0} != {2009-09-30 17:00:10.0}";
@@ -1043,9 +1125,9 @@
5, accountId, null, TimeUtil.nowTs());
ChangeBundle b1 = new ChangeBundle(c, messages(), latest(c), approvals(),
- comments(c1, c2), REVIEW_DB);
+ comments(c1, c2), reviewers(), REVIEW_DB);
ChangeBundle b2 = new ChangeBundle(c, messages(), latest(c), approvals(),
- comments(c1), REVIEW_DB);
+ comments(c1), reviewers(), REVIEW_DB);
assertNoDiffs(b1, b2);
}
@@ -1085,6 +1167,17 @@
return Arrays.asList(ents);
}
+ private static ReviewerSet reviewers(Object... ents) {
+ checkArgument(ents.length % 3 == 0);
+ Table<ReviewerStateInternal, Account.Id, Timestamp> t =
+ HashBasedTable.create();
+ for (int i = 0; i < ents.length; i += 3) {
+ t.put((ReviewerStateInternal) ents[i], (Account.Id) ents[i + 1],
+ (Timestamp) ents[i + 2]);
+ }
+ return ReviewerSet.fromTable(t);
+ }
+
private static List<PatchLineComment> comments(PatchLineComment... ents) {
return Arrays.asList(ents);
}
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
index 3f77498..1f54f55 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
@@ -28,7 +28,7 @@
import com.google.common.collect.ImmutableListMultimap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.ImmutableSetMultimap;
+import com.google.common.collect.ImmutableTable;
import com.google.common.collect.Iterables;
import com.google.common.collect.ListMultimap;
import com.google.common.collect.Lists;
@@ -46,6 +46,7 @@
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.ReviewerSet;
import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
import com.google.gerrit.testutil.TestChanges;
import com.google.gwtorm.server.OrmException;
@@ -392,10 +393,12 @@
update.commit();
ChangeNotes notes = newNotes(c);
- assertThat(notes.getReviewers()).isEqualTo(
- ImmutableSetMultimap.of(
- REVIEWER, new Account.Id(1),
- REVIEWER, new Account.Id(2)));
+ Timestamp ts = new Timestamp(update.getWhen().getTime());
+ assertThat(notes.getReviewers()).isEqualTo(ReviewerSet.fromTable(
+ ImmutableTable.<ReviewerStateInternal, Account.Id, Timestamp>builder()
+ .put(REVIEWER, new Account.Id(1), ts)
+ .put(REVIEWER, new Account.Id(2), ts)
+ .build()));
}
@Test
@@ -407,10 +410,12 @@
update.commit();
ChangeNotes notes = newNotes(c);
- assertThat(notes.getReviewers()).isEqualTo(
- ImmutableSetMultimap.of(
- REVIEWER, new Account.Id(1),
- CC, new Account.Id(2)));
+ Timestamp ts = new Timestamp(update.getWhen().getTime());
+ assertThat(notes.getReviewers()).isEqualTo(ReviewerSet.fromTable(
+ ImmutableTable.<ReviewerStateInternal, Account.Id, Timestamp>builder()
+ .put(REVIEWER, new Account.Id(1), ts)
+ .put(CC, new Account.Id(2), ts)
+ .build()));
}
@Test
@@ -421,16 +426,18 @@
update.commit();
ChangeNotes notes = newNotes(c);
- assertThat(notes.getReviewers()).isEqualTo(
- ImmutableSetMultimap.of(REVIEWER, new Account.Id(2)));
+ Timestamp ts = new Timestamp(update.getWhen().getTime());
+ assertThat(notes.getReviewers()).isEqualTo(ReviewerSet.fromTable(
+ ImmutableTable.of(REVIEWER, new Account.Id(2), ts)));
update = newUpdate(c, otherUser);
update.putReviewer(otherUser.getAccount().getId(), CC);
update.commit();
notes = newNotes(c);
- assertThat(notes.getReviewers()).isEqualTo(
- ImmutableSetMultimap.of(CC, new Account.Id(2)));
+ ts = new Timestamp(update.getWhen().getTime());
+ assertThat(notes.getReviewers()).isEqualTo(ReviewerSet.fromTable(
+ ImmutableTable.of(CC, new Account.Id(2), ts)));
}
@Test
