Fix errors when adding reviewer multiple times Now that multiple reviewers can be added at once, we need to be more tolerant of duplicate entries in the request. Not only can an account be referenced directly multiple times in a single request, but it's also possible for groups to be referenced that might overlap. The fix is to take pending reviewer changes in the ChangeUpdate into account when filtering the set of reviewers to add. Change-Id: Ibd70cc5ba6e8c5e256abdfbf4904c3c8a0603eb8
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java index 1de0996..8f4f3cb 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersIT.java
@@ -453,6 +453,85 @@ admin.getId().get()); } + @Test + public void addDuplicateReviewers() throws Exception { + PushOneCommit.Result r = createChange(); + ReviewInput input = ReviewInput.approve() + .reviewer(user.email) + .reviewer(user.email); + ReviewResult result = review(r.getChangeId(), r.getCommit().name(), input); + assertThat(result.reviewers).isNotNull(); + assertThat(result.reviewers).hasSize(1); + AddReviewerResult reviewerResult = result.reviewers.get(user.email); + assertThat(reviewerResult).isNotNull(); + assertThat(reviewerResult.confirm).isNull(); + assertThat(reviewerResult.error).isNull(); + } + + @Test + public void addOverlappingGroups() throws Exception { + String emailPrefix = "addOverlappingGroups-"; + TestAccount user1 = accounts.create(name("user1"), + emailPrefix + "user1@example.com", "User1"); + TestAccount user2 = accounts.create(name("user2"), + emailPrefix + "user2@example.com", "User2"); + TestAccount user3 = accounts.create(name("user3"), + emailPrefix + "user3@example.com", "User3"); + String group1 = createGroup("group1"); + String group2 = createGroup("group2"); + gApi.groups().id(group1).addMembers(user1.username, user2.username); + gApi.groups().id(group2).addMembers(user2.username, user3.username); + + PushOneCommit.Result r = createChange(); + ReviewInput input = ReviewInput.approve() + .reviewer(group1) + .reviewer(group2); + ReviewResult result = review(r.getChangeId(), r.getCommit().name(), input); + assertThat(result.reviewers).isNotNull(); + assertThat(result.reviewers).hasSize(2); + AddReviewerResult reviewerResult = result.reviewers.get(group1); + assertThat(reviewerResult.error).isNull(); + assertThat(reviewerResult.reviewers).hasSize(2); + reviewerResult = result.reviewers.get(group2); + assertThat(reviewerResult.error).isNull(); + assertThat(reviewerResult.reviewers).hasSize(1); + + // Repeat the above for CCs + if (!notesMigration.readChanges()) { + return; + } + r = createChange(); + input = ReviewInput.approve() + .reviewer(group1, CC, false) + .reviewer(group2, CC, false); + result = review(r.getChangeId(), r.getCommit().name(), input); + assertThat(result.reviewers).isNotNull(); + assertThat(result.reviewers).hasSize(2); + reviewerResult = result.reviewers.get(group1); + assertThat(reviewerResult.error).isNull(); + assertThat(reviewerResult.ccs).hasSize(2); + reviewerResult = result.reviewers.get(group2); + assertThat(reviewerResult.error).isNull(); + assertThat(reviewerResult.ccs).hasSize(1); + + // Repeat again with one group REVIEWER, the other CC. The overlapping + // member should end up as a REVIEWER. + r = createChange(); + input = ReviewInput.approve() + .reviewer(group1, REVIEWER, false) + .reviewer(group2, CC, false); + result = review(r.getChangeId(), r.getCommit().name(), input); + assertThat(result.reviewers).isNotNull(); + assertThat(result.reviewers).hasSize(2); + reviewerResult = result.reviewers.get(group1); + assertThat(reviewerResult.error).isNull(); + assertThat(reviewerResult.reviewers).hasSize(2); + reviewerResult = result.reviewers.get(group2); + assertThat(reviewerResult.error).isNull(); + assertThat(reviewerResult.reviewers).isNull(); + assertThat(reviewerResult.ccs).hasSize(1); + } + private AddReviewerResult addReviewer(String changeId, String reviewer) throws Exception { return addReviewer(changeId, reviewer, SC_OK);
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 219e5fc..e0526e4 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
@@ -41,6 +41,7 @@ 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; @@ -179,6 +180,15 @@ // Prior to NoteDB, we gather all reviewers regardless of state. existingReviewers = getReviewers(db, notes).all(); } + // Existing reviewers should include pending additions in the REVIEWER + // state, taken from ChangeUpdate. + existingReviewers = Lists.newArrayList(existingReviewers); + for (Map.Entry<Account.Id, ReviewerStateInternal> entry : + update.getReviewers().entrySet()) { + if (entry.getValue() == REVIEWER) { + existingReviewers.add(entry.getKey()); + } + } return addReviewers(db, update, labelTypes, change, psId, false, null, null, wantReviewers, existingReviewers); } @@ -237,6 +247,7 @@ Collection<Account.Id> wantCCs, ReviewerSet existingReviewers) { Set<Account.Id> need = new LinkedHashSet<>(wantCCs); need.removeAll(existingReviewers.all()); + need.removeAll(update.getReviewers().keySet()); for (Account.Id account : need) { update.putReviewer(account, CC); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index 36bf1bc..357b358 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
@@ -377,6 +377,10 @@ this.hashtags = hashtags; } + public Map<Account.Id, ReviewerStateInternal> getReviewers() { + return reviewers; + } + public void putReviewer(Account.Id reviewer, ReviewerStateInternal type) { checkArgument(type != ReviewerStateInternal.REMOVED, "invalid ReviewerType"); reviewers.put(reviewer, type);