Merge "Prevent overwriting of reviewers in ChangeJson"

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java

@@ -71,6 +71,33 @@ public class ChangeReviewersByEmailIT extends AbstractDaemonTest {
     }
   }
 
+  @Test
+  public void addByEmailAndById() throws Exception {
+    assume().that(notesMigration.readChanges()).isTrue();
+    AccountInfo byEmail = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com");
+    AccountInfo byId = new AccountInfo(user.id.get());
+
+    for (ReviewerState state : ImmutableList.of(ReviewerState.CC, ReviewerState.REVIEWER)) {
+      PushOneCommit.Result r = createChange();
+
+      AddReviewerInput inputByEmail = new AddReviewerInput();
+      inputByEmail.reviewer = toRfcAddressString(byEmail);
+      inputByEmail.state = state;
+      gApi.changes().id(r.getChangeId()).addReviewer(inputByEmail);
+
+      AddReviewerInput inputById = new AddReviewerInput();
+      inputById.reviewer = user.email;
+      inputById.state = state;
+      gApi.changes().id(r.getChangeId()).addReviewer(inputById);
+
+      ChangeInfo info =
+          gApi.changes().id(r.getChangeId()).get(EnumSet.of(ListChangesOption.DETAILED_LABELS));
+      assertThat(info.reviewers).isEqualTo(ImmutableMap.of(state, ImmutableList.of(byId, byEmail)));
+      // All reviewers (both by id and by email) should be removable
+      assertThat(info.removableReviewers).isEqualTo(ImmutableList.of(byId, byEmail));
+    }
+  }
+
   @Test
   public void removeByEmail() throws Exception {
     assume().that(notesMigration.readChanges()).isTrue();

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java

@@ -528,14 +528,15 @@ public class ChangeJson {
       }
 
       out.reviewers = new HashMap<>();
-      for (Map.Entry<ReviewerStateInternal, Map<Account.Id, Timestamp>> e :
+      for (ReviewerStateInternal state : ReviewerStateInternal.values()) {
-          cd.reviewers().asTable().rowMap().entrySet()) {
+        if (state == ReviewerStateInternal.REMOVED) {
-        out.reviewers.put(e.getKey().asReviewerState(), toAccountInfo(e.getValue().keySet()));
+          continue;
-      }
+        }
-      for (Map.Entry<ReviewerStateInternal, Map<Address, Timestamp>> e :
+        Collection<AccountInfo> reviewers = toAccountInfo(cd.reviewers().byState(state));
-          cd.reviewersByEmail().asTable().rowMap().entrySet()) {
+        reviewers.addAll(toAccountInfoByEmail(cd.reviewersByEmail().byState(state)));
-        out.reviewers.put(
+        if (!reviewers.isEmpty()) {
-            e.getKey().asReviewerState(), toAccountInfoByEmail(e.getValue().keySet()));
+          out.reviewers.put(state.asReviewerState(), reviewers);
+        }
       }
 
       out.removableReviewers = removableReviewers(ctl, out);