Add reviewers by email to NoteDb

This change adds code for NoteDb to store reviewers and CCs that do not
currently have a Gerrit account.

Once completed, this feature will send out emails to these reviewers and
provide an additional way of notifying users who do not have a
Gerrit account.

The feature will be supported exclusively on NoteDb.

In a follow-up change, we will add migration logic for reviewers by
email to regular reviewers should the user decide to create a Gerrit
account. This migration will happen on a per-change basis when the user
interacts with the change.

Bug: Issue 4134
Change-Id: I8b9f53f9ba0928f721e608c402257542775289f9

gerrit-server/src/main/java/com/google/gerrit/server/ReviewerByEmailSet.java

@@ -0,0 +1,79 @@
+// Copyright (C) 2017 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 com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableTable;
+import com.google.common.collect.Table;
+import com.google.gerrit.server.mail.Address;
+import com.google.gerrit.server.notedb.ReviewerStateInternal;
+import java.sql.Timestamp;
+
+/**
+ * Set of reviewers on a change that do not have a Gerrit account and were added by email instead.
+ *
+ * <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 ReviewerByEmailSet {
+  private static final ReviewerByEmailSet EMPTY = new ReviewerByEmailSet(ImmutableTable.of());
+
+  public static ReviewerByEmailSet fromTable(
+      Table<ReviewerStateInternal, Address, Timestamp> table) {
+    return new ReviewerByEmailSet(table);
+  }
+
+  public static ReviewerByEmailSet empty() {
+    return EMPTY;
+  }
+
+  private final ImmutableTable<ReviewerStateInternal, Address, Timestamp> table;
+  private ImmutableSet<Address> users;
+
+  private ReviewerByEmailSet(Table<ReviewerStateInternal, Address, Timestamp> table) {
+    this.table = ImmutableTable.copyOf(table);
+  }
+
+  public ImmutableSet<Address> all() {
+    if (users == null) {
+      // Idempotent and immutable, don't bother locking.
+      users = ImmutableSet.copyOf(table.columnKeySet());
+    }
+    return users;
+  }
+
+  public ImmutableSet<Address> byState(ReviewerStateInternal state) {
+    return table.row(state).keySet();
+  }
+
+  public ImmutableTable<ReviewerStateInternal, Address, Timestamp> asTable() {
+    return table;
+  }
+
+  @Override
+  public boolean equals(Object o) {
+    return (o instanceof ReviewerByEmailSet) && table.equals(((ReviewerByEmailSet) o).table);
+  }
+
+  @Override
+  public int hashCode() {
+    return table.hashCode();
+  }
+
+  @Override
+  public String toString() {
+    return getClass().getSimpleName() + table;
+  }
+}

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -48,6 +48,7 @@ import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.client.RobotComment;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.reviewdb.server.ReviewDbUtil;
+import com.google.gerrit.server.ReviewerByEmailSet;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.ReviewerStatusUpdate;
 import com.google.gerrit.server.git.RefCache;
@@ -428,6 +429,11 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     return state.reviewers();
   }
 
+  /** @return reviewers that do not currently have a Gerrit account and were added by email. */
+  public ReviewerByEmailSet getReviewersByEmail() {
+    return state.reviewersByEmail();
+  }
+
   public ImmutableList<ReviewerStatusUpdate> getReviewerUpdates() {
     return state.reviewerUpdates();
   }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -63,8 +63,10 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDbUtil;
+import com.google.gerrit.server.ReviewerByEmailSet;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.ReviewerStatusUpdate;
+import com.google.gerrit.server.mail.Address;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
 import com.google.gerrit.server.util.LabelVote;
 import java.io.IOException;
@@ -128,6 +130,7 @@ class ChangeNotesParser {
   // Private final but mutable members initialized in the constructor and filled
   // in during the parsing process.
   private final Table<Account.Id, ReviewerStateInternal, Timestamp> reviewers;
+  private final Table<Address, ReviewerStateInternal, Timestamp> reviewersByEmail;
   private final List<Account.Id> allPastReviewers;
   private final List<ReviewerStatusUpdate> reviewerUpdates;
   private final List<SubmitRecord> submitRecords;
@@ -174,6 +177,7 @@ class ChangeNotesParser {
     approvals = new LinkedHashMap<>();
     bufferedApprovals = new ArrayList<>();
     reviewers = HashBasedTable.create();
+    reviewersByEmail = HashBasedTable.create();
     allPastReviewers = new ArrayList<>();
     reviewerUpdates = new ArrayList<>();
     submitRecords = Lists.newArrayListWithExpectedSize(1);
@@ -201,6 +205,7 @@ class ChangeNotesParser {
       parseNotes();
       allPastReviewers.addAll(reviewers.rowKeySet());
       pruneReviewers();
+      pruneReviewersByEmail();
 
       updatePatchSetStates();
       checkMandatoryFooters();
@@ -234,6 +239,7 @@ class ChangeNotesParser {
         patchSets,
         buildApprovals(),
         ReviewerSet.fromTable(Tables.transpose(reviewers)),
+        ReviewerByEmailSet.fromTable(Tables.transpose(reviewersByEmail)),
         allPastReviewers,
         buildReviewerUpdates(),
         submitRecords,
@@ -374,6 +380,9 @@ class ChangeNotesParser {
       for (String line : commit.getFooterLineValues(state.getFooterKey())) {
         parseReviewer(ts, state, line);
       }
+      for (String line : commit.getFooterLineValues(state.getByEmailFooterKey())) {
+        parseReviewerByEmail(ts, state, line);
+      }
       // Don't update timestamp when a reviewer was added, matching RevewDb
       // behavior.
     }
@@ -917,6 +926,19 @@ class ChangeNotesParser {
     }
   }
 
+  private void parseReviewerByEmail(Timestamp ts, ReviewerStateInternal state, String line)
+      throws ConfigInvalidException {
+    Address adr;
+    try {
+      adr = Address.parse(line);
+    } catch (IllegalArgumentException e) {
+      throw invalidFooter(state.getByEmailFooterKey(), line);
+    }
+    if (!reviewersByEmail.containsRow(adr)) {
+      reviewersByEmail.put(adr, state, ts);
+    }
+  }
+
   private void parseReadOnlyUntil(ChangeNotesCommit commit) throws ConfigInvalidException {
     String raw = parseOneFooter(commit, FOOTER_READ_ONLY_UNTIL);
     if (raw == null) {
@@ -956,6 +978,17 @@ class ChangeNotesParser {
     }
   }
 
+  private void pruneReviewersByEmail() {
+    Iterator<Table.Cell<Address, ReviewerStateInternal, Timestamp>> rit =
+        reviewersByEmail.cellSet().iterator();
+    while (rit.hasNext()) {
+      Table.Cell<Address, ReviewerStateInternal, Timestamp> e = rit.next();
+      if (e.getColumnKey() == ReviewerStateInternal.REMOVED) {
+        rit.remove();
+      }
+    }
+  }
+
   private void updatePatchSetStates() {
     Set<PatchSet.Id> missing = new TreeSet<>(ReviewDbUtil.intKeyOrdering());
     for (Iterator<PatchSet> it = patchSets.values().iterator(); it.hasNext(); ) {

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesState.java

@@ -34,6 +34,7 @@ import com.google.gerrit.reviewdb.client.PatchSet;
 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.ReviewerByEmailSet;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.ReviewerStatusUpdate;
 import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage;
@@ -65,6 +66,7 @@ public abstract class ChangeNotesState {
         ImmutableList.of(),
         ImmutableList.of(),
         ReviewerSet.empty(),
+        ReviewerByEmailSet.empty(),
         ImmutableList.of(),
         ImmutableList.of(),
         ImmutableList.of(),
@@ -95,6 +97,7 @@ public abstract class ChangeNotesState {
       Map<PatchSet.Id, PatchSet> patchSets,
       ListMultimap<PatchSet.Id, PatchSetApproval> approvals,
       ReviewerSet reviewers,
+      ReviewerByEmailSet reviewersByEmail,
       List<Account.Id> allPastReviewers,
       List<ReviewerStatusUpdate> reviewerUpdates,
       List<SubmitRecord> submitRecords,
@@ -128,6 +131,7 @@ public abstract class ChangeNotesState {
         ImmutableList.copyOf(patchSets.entrySet()),
         ImmutableList.copyOf(approvals.entries()),
         reviewers,
+        reviewersByEmail,
         ImmutableList.copyOf(allPastReviewers),
         ImmutableList.copyOf(reviewerUpdates),
         ImmutableList.copyOf(submitRecords),
@@ -204,6 +208,8 @@ public abstract class ChangeNotesState {
 
   abstract ReviewerSet reviewers();
 
+  abstract ReviewerByEmailSet reviewersByEmail();
+
   abstract ImmutableList<Account.Id> allPastReviewers();
 
   abstract ImmutableList<ReviewerStatusUpdate> reviewerUpdates();

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java

@@ -61,6 +61,7 @@ import com.google.gerrit.server.GerritPersonIdent;
 import com.google.gerrit.server.account.AccountCache;
 import com.google.gerrit.server.config.AnonymousCowardName;
 import com.google.gerrit.server.config.GerritServerConfig;
+import com.google.gerrit.server.mail.Address;
 import com.google.gerrit.server.project.ChangeControl;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.util.LabelVote;
@@ -128,6 +129,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
 
   private final Table<String, Account.Id, Optional<Short>> approvals;
   private final Map<Account.Id, ReviewerStateInternal> reviewers = new LinkedHashMap<>();
+  private final Map<Address, ReviewerStateInternal> reviewersByEmail = new LinkedHashMap<>();
   private final List<Comment> comments = new ArrayList<>();
 
   private String commitSubject;
@@ -471,6 +473,15 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     reviewers.put(reviewer, ReviewerStateInternal.REMOVED);
   }
 
+  public void putReviewerByEmail(Address reviewer, ReviewerStateInternal type) {
+    checkArgument(type != ReviewerStateInternal.REMOVED, "invalid ReviewerType");
+    reviewersByEmail.put(reviewer, type);
+  }
+
+  public void removeReviewerByEmail(Address reviewer) {
+    reviewersByEmail.put(reviewer, ReviewerStateInternal.REMOVED);
+  }
+
   public void setPatchSetState(PatchSetState psState) {
     this.psState = psState;
   }
@@ -660,6 +671,10 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       addIdent(msg, e.getKey()).append('\n');
     }
 
+    for (Map.Entry<Address, ReviewerStateInternal> e : reviewersByEmail.entrySet()) {
+      addFooter(msg, e.getValue().getByEmailFooterKey(), e.getKey().toString());
+    }
+
     for (Table.Cell<String, Account.Id, Optional<Short>> c : approvals.cellSet()) {
       addFooter(msg, FOOTER_LABEL);
       // Label names/values are safe to append without sanitizing.
@@ -749,6 +764,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
         && changeMessage == null
         && comments.isEmpty()
         && reviewers.isEmpty()
+        && reviewersByEmail.isEmpty()
         && changeId == null
         && branch == null
         && status == null

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ReviewerStateInternal.java

@@ -58,6 +58,10 @@ public enum ReviewerStateInternal {
     return footerKey;
   }
 
+  FooterKey getByEmailFooterKey() {
+    return new FooterKey(footerKey.getName() + "-email");
+  }
+
   public ReviewerState asReviewerState() {
     return state;
   }

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -49,6 +49,7 @@ import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.ReviewerSet;
 import com.google.gerrit.server.config.GerritServerId;
+import com.google.gerrit.server.mail.Address;
 import com.google.gerrit.server.notedb.ChangeNotesCommit.ChangeNotesRevWalk;
 import com.google.gerrit.server.util.RequestId;
 import com.google.gerrit.testutil.TestChanges;
@@ -3298,6 +3299,105 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(notes.isPrivate()).isFalse();
   }
 
+  @Test
+  public void defaultReviewersByEmailIsEmpty() throws Exception {
+    Change c = newChange();
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getReviewersByEmail().all()).isEmpty();
+  }
+
+  @Test
+  public void putReviewerByEmail() throws Exception {
+    Address adr = new Address("Foo Bar", "foo.bar@gerritcodereview.com");
+
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.putReviewerByEmail(adr, ReviewerStateInternal.REVIEWER);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getReviewersByEmail().all()).containsExactly(adr);
+  }
+
+  @Test
+  public void putAndRemoveReviewerByEmail() throws Exception {
+    Address adr = new Address("Foo Bar", "foo.bar@gerritcodereview.com");
+
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.putReviewerByEmail(adr, ReviewerStateInternal.REVIEWER);
+    update.commit();
+
+    update = newUpdate(c, changeOwner);
+    update.removeReviewerByEmail(adr);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getReviewersByEmail().all()).isEmpty();
+  }
+
+  @Test
+  public void putRemoveAndAddBackReviewerByEmail() throws Exception {
+    Address adr = new Address("Foo Bar", "foo.bar@gerritcodereview.com");
+
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.putReviewerByEmail(adr, ReviewerStateInternal.REVIEWER);
+    update.commit();
+
+    update = newUpdate(c, changeOwner);
+    update.removeReviewerByEmail(adr);
+    update.commit();
+
+    update = newUpdate(c, changeOwner);
+    update.putReviewerByEmail(adr, ReviewerStateInternal.CC);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getReviewersByEmail().all()).containsExactly(adr);
+  }
+
+  @Test
+  public void putReviewerByEmailAndCcByEmail() throws Exception {
+    Address adrReviewer = new Address("Foo Bar", "foo.bar@gerritcodereview.com");
+    Address adrCc = new Address("Foo Bor", "foo.bar.2@gerritcodereview.com");
+
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.putReviewerByEmail(adrReviewer, ReviewerStateInternal.REVIEWER);
+    update.commit();
+
+    update = newUpdate(c, changeOwner);
+    update.putReviewerByEmail(adrCc, ReviewerStateInternal.CC);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getReviewersByEmail().byState(ReviewerStateInternal.REVIEWER))
+        .containsExactly(adrReviewer);
+    assertThat(notes.getReviewersByEmail().byState(ReviewerStateInternal.CC))
+        .containsExactly(adrCc);
+    assertThat(notes.getReviewersByEmail().all()).containsExactly(adrReviewer, adrCc);
+  }
+
+  @Test
+  public void putReviewerByEmailAndChangeToCc() throws Exception {
+    Address adr = new Address("Foo Bar", "foo.bar@gerritcodereview.com");
+
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.putReviewerByEmail(adr, ReviewerStateInternal.REVIEWER);
+    update.commit();
+
+    update = newUpdate(c, changeOwner);
+    update.putReviewerByEmail(adr, ReviewerStateInternal.CC);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(notes.getReviewersByEmail().byState(ReviewerStateInternal.REVIEWER)).isEmpty();
+    assertThat(notes.getReviewersByEmail().byState(ReviewerStateInternal.CC)).containsExactly(adr);
+    assertThat(notes.getReviewersByEmail().all()).containsExactly(adr);
+  }
+
   private boolean testJson() {
     return noteUtil.getWriteJson();
   }

gerrit-server/src/test/java/com/google/gerrit/server/notedb/CommitMessageOutputTest.java

@@ -23,6 +23,7 @@ import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.server.CurrentUser;
+import com.google.gerrit.server.mail.Address;
 import com.google.gerrit.server.util.RequestId;
 import com.google.gerrit.testutil.ConfigSuite;
 import com.google.gerrit.testutil.TestChanges;
@@ -382,6 +383,32 @@ public class CommitMessageOutputTest extends AbstractChangeNotesTest {
     assertBodyEquals("Update patch set 1\n\nPatch-set: 1\nCurrent: true\n", update.getResult());
   }
 
+  @Test
+  public void reviewerByEmail() throws Exception {
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.putReviewerByEmail(
+        new Address("John Doe", "j.doe@gerritcodereview.com"), ReviewerStateInternal.REVIEWER);
+    update.commit();
+
+    assertBodyEquals(
+        "Update patch set 1\n\nPatch-set: 1\n"
+            + "Reviewer-email: John Doe <j.doe@gerritcodereview.com>\n",
+        update.getResult());
+  }
+
+  @Test
+  public void ccByEmail() throws Exception {
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.putReviewerByEmail(new Address("j.doe@gerritcodereview.com"), ReviewerStateInternal.CC);
+    update.commit();
+
+    assertBodyEquals(
+        "Update patch set 1\n\nPatch-set: 1\nCC-email: j.doe@gerritcodereview.com\n",
+        update.getResult());
+  }
+
   private RevCommit parseCommit(ObjectId id) throws Exception {
     if (id instanceof RevCommit) {
       return (RevCommit) id;