Notedb: Support applying labels for other users directly

We already supported removing labels for other users; generalize this
to support modifying arbitrary labels as well.

However, this still won't be used by PostReview in the near future,
since that also needs to update inline comments and drafts, which
complicates things considerably.

Change-Id: I1b47b691904bb9dec4d35fccd41907f1422b0691

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

@@ -27,7 +27,6 @@ import com.google.common.base.Optional;
 import com.google.common.base.Splitter;
 import com.google.common.base.Supplier;
 import com.google.common.collect.ArrayListMultimap;
-import com.google.common.collect.HashMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableSet;
@@ -87,9 +86,8 @@ class ChangeNotesParser implements AutoCloseable {
   private final ObjectId tip;
   private final RevWalk walk;
   private final Repository repo;
-  private final Map<PatchSet.Id, Table<Account.Id, String, PatchSetApproval>>
+  private final Map<PatchSet.Id,
-      approvals;
+      Table<Account.Id, String, Optional<PatchSetApproval>>> approvals;
-  private final Map<PatchSet.Id, Multimap<Account.Id, String>> removedApprovals;
   private final List<ChangeMessage> allChangeMessages;
   private final Multimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet;
 
@@ -102,7 +100,6 @@ class ChangeNotesParser implements AutoCloseable {
     this.repo =
         repoManager.openMetadataRepository(ChangeNotes.getProjectName(change));
     approvals = Maps.newHashMap();
-    removedApprovals = Maps.newHashMap();
     reviewers = Maps.newLinkedHashMap();
     allPastReviewers = Lists.newArrayList();
     submitRecords = Lists.newArrayListWithExpectedSize(1);
@@ -130,9 +127,11 @@ class ChangeNotesParser implements AutoCloseable {
       buildApprovals() {
     Multimap<PatchSet.Id, PatchSetApproval> result =
         ArrayListMultimap.create(approvals.keySet().size(), 3);
-    for (Table<?, ?, PatchSetApproval> curr : approvals.values()) {
+    for (Table<?, ?, Optional<PatchSetApproval>> curr : approvals.values()) {
-      for (PatchSetApproval psa : curr.values()) {
+      for (Optional<PatchSetApproval> psa : curr.values()) {
-        result.put(psa.getPatchSetId(), psa);
+        if (psa.isPresent()) {
+          result.put(psa.get().getPatchSetId(), psa.get());
+        }
       }
     }
     for (Collection<PatchSetApproval> v : result.asMap().values()) {
@@ -315,92 +314,98 @@ class ChangeNotesParser implements AutoCloseable {
     }
   }
 
-  private void parseAddApproval(PatchSet.Id psId, Account.Id accountId,
+  private void parseAddApproval(PatchSet.Id psId, Account.Id committerId,
       RevCommit commit, String line) throws ConfigInvalidException {
+    Account.Id accountId;
+    String labelVoteStr;
+    int s = line.indexOf(' ');
+    if (s > 0) {
+      labelVoteStr = line.substring(0, s);
+      PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1));
+      checkFooter(ident != null, FOOTER_LABEL, line);
+      accountId = parseIdent(ident);
+    } else {
+      labelVoteStr = line;
+      accountId = committerId;
+    }
+
     LabelVote l;
     try {
-      l = LabelVote.parseWithEquals(line);
+      l = LabelVote.parseWithEquals(labelVoteStr);
     } catch (IllegalArgumentException e) {
       ConfigInvalidException pe =
           parseException("invalid %s: %s", FOOTER_LABEL, line);
       pe.initCause(e);
       throw pe;
     }
-    if (isApprovalRemoved(psId, accountId, l.label())) {
-      return;
-    }
 
-    Table<Account.Id, String, PatchSetApproval> curr = approvals.get(psId);
+    Table<Account.Id, String, Optional<PatchSetApproval>> curr =
+        getApprovalsTableIfNoVotePresent(psId, accountId, l.label());
     if (curr != null) {
-      if (curr.contains(accountId, l.label())) {
+      curr.put(accountId, l.label(), Optional.of(new PatchSetApproval(
-        return;
+          new PatchSetApproval.Key(
-      }
+              psId,
-    } else {
+              accountId,
-      curr = newApprovalsTable();
+              new LabelId(l.label())),
-      approvals.put(psId, curr);
+          l.value(),
+          new Timestamp(commit.getCommitterIdent().getWhen().getTime()))));
     }
-    curr.put(accountId, l.label(), new PatchSetApproval(
-        new PatchSetApproval.Key(
-            psId,
-            accountId,
-            new LabelId(l.label())),
-        l.value(),
-        new Timestamp(commit.getCommitterIdent().getWhen().getTime())));
   }
 
-  private static Table<Account.Id, String, PatchSetApproval>
+  private void parseRemoveApproval(PatchSet.Id psId, Account.Id committerId,
-      newApprovalsTable() {
+      String line) throws ConfigInvalidException {
-    return Tables.newCustomTable(
+    Account.Id accountId;
-          Maps.<Account.Id, Map<String, PatchSetApproval>>
+    String label;
+    int s = line.indexOf(' ');
+    if (s > 0) {
+      label = line.substring(1, s);
+      PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1));
+      checkFooter(ident != null, FOOTER_LABEL, line);
+      accountId = parseIdent(ident);
+    } else {
+      label = line.substring(1);
+      accountId = committerId;
+    }
+
+    try {
+      LabelType.checkNameInternal(label);
+    } catch (IllegalArgumentException e) {
+      ConfigInvalidException pe =
+          parseException("invalid %s: %s", FOOTER_LABEL, line);
+      pe.initCause(e);
+      throw pe;
+    }
+
+    Table<Account.Id, String, Optional<PatchSetApproval>> curr =
+        getApprovalsTableIfNoVotePresent(psId, accountId, label);
+    if (curr != null) {
+      curr.put(accountId, label, Optional.<PatchSetApproval> absent());
+    }
+  }
+
+  private Table<Account.Id, String, Optional<PatchSetApproval>>
+      getApprovalsTableIfNoVotePresent(PatchSet.Id psId, Account.Id accountId,
+        String label) {
+
+    Table<Account.Id, String, Optional<PatchSetApproval>> curr =
+        approvals.get(psId);
+    if (curr != null) {
+      if (curr.contains(accountId, label)) {
+        return null;
+      }
+    } else {
+      curr = Tables.newCustomTable(
+          Maps.<Account.Id, Map<String, Optional<PatchSetApproval>>>
               newHashMapWithExpectedSize(2),
-          new Supplier<Map<String, PatchSetApproval>>() {
+          new Supplier<Map<String, Optional<PatchSetApproval>>>() {
             @Override
-            public Map<String, PatchSetApproval> get() {
+            public Map<String, Optional<PatchSetApproval>> get() {
               return Maps.newLinkedHashMap();
             }
           });
-  }
+      approvals.put(psId, curr);
-
-  private void parseRemoveApproval(PatchSet.Id psId, Account.Id accountId,
-      String line) throws ConfigInvalidException {
-    Multimap<Account.Id, String> curr = removedApprovals.get(psId);
-    if (curr == null) {
-      curr = HashMultimap.create(1, 1);
-      removedApprovals.put(psId, curr);
     }
-    String label;
+    return curr;
-    Account.Id removedAccountId;
-    line = line.substring(1);
-    int s = line.indexOf(' ');
-    if (s > 0) {
-      label = line.substring(0, s);
-      PersonIdent ident = RawParseUtils.parsePersonIdent(line.substring(s + 1));
-      checkFooter(ident != null, FOOTER_LABEL, line);
-      removedAccountId = parseIdent(ident);
-    } else {
-      label = line;
-      removedAccountId = accountId;
-    }
-
-    Table<Account.Id, String, PatchSetApproval> added = approvals.get(psId);
-    if (added != null && added.contains(accountId, label)) {
-      return;
-    }
-
-    try {
-      curr.put(removedAccountId, LabelType.checkNameInternal(label));
-    } catch (IllegalArgumentException e) {
-      ConfigInvalidException pe =
-          parseException("invalid %s: %s", FOOTER_LABEL, line);
-      pe.initCause(e);
-      throw pe;
-    }
-  }
-
-  private boolean isApprovalRemoved(PatchSet.Id psId, Account.Id accountId,
-      String label) {
-    Multimap<Account.Id, String> curr = removedApprovals.get(psId);
-    return curr != null && curr.containsEntry(accountId, label);
   }
 
   private void parseSubmitRecords(List<String> lines)

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

@@ -24,13 +24,16 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_TOPIC;
 import static com.google.gerrit.server.notedb.CommentsInNotesUtil.addCommentToMap;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Function;
 import com.google.common.base.Joiner;
+import com.google.common.base.Optional;
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
-import com.google.common.collect.Multimap;
+import com.google.common.collect.Ordering;
-import com.google.common.collect.MultimapBuilder;
+import com.google.common.collect.Table;
+import com.google.common.collect.TreeBasedTable;
 import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -88,9 +91,8 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   }
 
   private final AccountCache accountCache;
-  private final Map<String, Short> approvals;
+  private final Table<String, Account.Id, Optional<Short>> approvals;
   private final Map<Account.Id, ReviewerStateInternal> reviewers;
-  private final Multimap<Account.Id, String> removedApprovals;
   private Change.Status status;
   private String subject;
   private List<SubmitRecord> submitRecords;
@@ -163,10 +165,15 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     this.draftUpdateFactory = draftUpdateFactory;
     this.accountCache = accountCache;
     this.commentsUtil = commentsUtil;
-    this.approvals = Maps.newTreeMap(labelNameComparator);
+    this.approvals = TreeBasedTable.create(
+        labelNameComparator,
+        Ordering.natural().onResultOf(new Function<Account.Id, Integer>() {
+          @Override
+          public Integer apply(Account.Id in) {
+            return in.get();
+          }
+        }));
     this.reviewers = Maps.newLinkedHashMap();
-    this.removedApprovals =
-        MultimapBuilder.linkedHashKeys(1).arrayListValues(1).build();
     this.comments = Lists.newArrayList();
   }
 
@@ -181,7 +188,11 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   }
 
   public void putApproval(String label, short value) {
-    approvals.put(label, value);
+    putApprovalFor(getUser().getAccountId(), label, value);
+  }
+
+  public void putApprovalFor(Account.Id reviewer, String label, short value) {
+    approvals.put(label, reviewer, Optional.of(value));
   }
 
   public void removeApproval(String label) {
@@ -189,7 +200,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   }
 
   public void removeApprovalFor(Account.Id reviewer, String label) {
-    removedApprovals.put(reviewer, label);
+    approvals.put(label, reviewer, Optional.<Short> absent());
   }
 
   public void merge(Iterable<SubmitRecord> submitRecords) {
@@ -446,14 +457,16 @@ public class ChangeUpdate extends AbstractChangeUpdate {
       addIdent(msg, e.getKey()).append('\n');
     }
 
-    for (Map.Entry<String, Short> e : approvals.entrySet()) {
+    for (Table.Cell<String, Account.Id, Optional<Short>> c
-      addFooter(msg, FOOTER_LABEL, LabelVote.create(
+        : approvals.cellSet()) {
-          e.getKey(), e.getValue()).formatWithEquals());
+      addFooter(msg, FOOTER_LABEL);
-    }
+      if (!c.getValue().isPresent()) {
-
+        msg.append('-').append(c.getRowKey());
-    for (Map.Entry<Account.Id, String> e : removedApprovals.entries()) {
+      } else {
-      addFooter(msg, FOOTER_LABEL).append('-').append(e.getValue());
+        msg.append(LabelVote.create(
-      Account.Id id = e.getKey();
+            c.getRowKey(), c.getValue().get()).formatWithEquals());
+      }
+      Account.Id id = c.getColumnKey();
       if (!id.equals(ctl.getUser().getAccountId())) {
         addIdent(msg.append(' '), id);
       }
@@ -496,7 +509,6 @@ public class ChangeUpdate extends AbstractChangeUpdate {
 
   private boolean isEmpty() {
     return approvals.isEmpty()
-        && removedApprovals.isEmpty()
         && changeMessage == null
         && comments.isEmpty()
         && reviewers.isEmpty()

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

@@ -20,6 +20,7 @@ import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 import static com.google.gerrit.testutil.TestChanges.incrementPatchSet;
 import static java.nio.charset.StandardCharsets.UTF_8;
 
+import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableMultimap;
@@ -27,6 +28,7 @@ import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Lists;
+import com.google.common.collect.Ordering;
 import com.google.gerrit.common.TimeUtil;
 import com.google.gerrit.common.data.SubmitRecord;
 import com.google.gerrit.reviewdb.client.Account;
@@ -228,7 +230,35 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(psa.getAccountId()).isEqualTo(otherUserId);
     assertThat(psa.getLabel()).isEqualTo("Not-For-Long");
     assertThat(psa.getValue()).isEqualTo((short) 2);
+  }
 
+  @Test
+  public void putOtherUsersApprovals() throws Exception {
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.putApproval("Code-Review", (short) 1);
+    update.putApprovalFor(otherUser.getAccountId(), "Code-Review", (short) -1);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    List<PatchSetApproval> approvals = Ordering.natural().onResultOf(
+        new Function<PatchSetApproval, Integer>() {
+          @Override
+          public Integer apply(PatchSetApproval in) {
+            return in.getAccountId().get();
+          }
+        }).sortedCopy(notes.getApprovals().get(c.currentPatchSetId()));
+    assertThat(approvals).hasSize(2);
+
+    assertThat(approvals.get(0).getAccountId())
+        .isEqualTo(changeOwner.getAccountId());
+    assertThat(approvals.get(0).getLabel()).isEqualTo("Code-Review");
+    assertThat(approvals.get(0).getValue()).isEqualTo((short) 1);
+
+    assertThat(approvals.get(1).getAccountId())
+        .isEqualTo(otherUser.getAccountId());
+    assertThat(approvals.get(1).getLabel()).isEqualTo("Code-Review");
+    assertThat(approvals.get(1).getValue()).isEqualTo((short) -1);
   }
 
   @Test