Allow removing approvals from a label in the notedb

This allows the server to indicate a previously-saved approval is for
a label that no longer exists or the user no longer has permissions to
vote on.

Reuse the "Label" footer, with "-Label-Name" indicating a tombstone.

Change-Id: I8c390a28c639d1642aa434eb8c816d6c89f0ebdd

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

@@ -20,13 +20,16 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Function;
+import com.google.common.base.Optional;
+import com.google.common.base.Supplier;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableListMultimap;
 import com.google.common.collect.ImmutableSetMultimap;
-import com.google.common.collect.ListMultimap;
 import com.google.common.collect.Maps;
+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.collect.Tables;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -56,7 +59,6 @@ import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
 
 /** View of a single {@link Change} based on the log of its notes branch. */
 public class ChangeNotes extends VersionedMetaData {
@@ -88,14 +90,15 @@ public class ChangeNotes extends VersionedMetaData {
     private final Change.Id changeId;
     private final ObjectId tip;
     private final RevWalk walk;
-    private final ListMultimap<PatchSet.Id, PatchSetApproval> approvals;
+    private final Map<PatchSet.Id,
+        Table<Account.Id, String, Optional<PatchSetApproval>>> approvals;
     private final Map<Account.Id, ReviewerState> reviewers;
 
     private Parser(Change.Id changeId, ObjectId tip, RevWalk walk) {
       this.changeId = changeId;
       this.tip = tip;
       this.walk = walk;
-      approvals = ArrayListMultimap.create();
+      approvals = Maps.newHashMap();
       reviewers = Maps.newLinkedHashMap();
     }
 
@@ -105,30 +108,30 @@ public class ChangeNotes extends VersionedMetaData {
         parse(commit);
       }
       pruneReviewers();
-      for (Collection<PatchSetApproval> v : approvals.asMap().values()) {
+    }
+
+    private ImmutableListMultimap<PatchSet.Id, PatchSetApproval>
+        buildApprovals() {
+      Multimap<PatchSet.Id, PatchSetApproval> result =
+          ArrayListMultimap.create(approvals.keySet().size(), 3);
+      for (Table<?, ?, Optional<PatchSetApproval>> curr
+          : approvals.values()) {
+        for (PatchSetApproval psa : Optional.presentInstances(curr.values())) {
+          result.put(psa.getPatchSetId(), psa);
+        }
+      }
+      for (Collection<PatchSetApproval> v : result.asMap().values()) {
         Collections.sort((List<PatchSetApproval>) v, PSA_BY_TIME);
       }
+      return ImmutableListMultimap.copyOf(result);
     }
 
     private void parse(RevCommit commit) throws ConfigInvalidException {
       PatchSet.Id psId = parsePatchSetId(commit);
       Account.Id accountId = parseIdent(commit);
-      List<PatchSetApproval> psas = approvals.get(psId);
-
-      Map<String, PatchSetApproval> curr =
-          Maps.newHashMapWithExpectedSize(psas.size());
-      for (PatchSetApproval psa : psas) {
-        if (psa.getAccountId().equals(accountId)) {
-          curr.put(psa.getLabel(), psa);
-        }
-      }
 
       for (String line : commit.getFooterLines(FOOTER_LABEL)) {
-        PatchSetApproval psa = parseApproval(psId, accountId, commit, line);
+        parseApproval(psId, accountId, commit, line);
-        if (!curr.containsKey(psa.getLabel())) {
-          curr.put(psa.getLabel(), psa);
-          psas.add(psa);
-        }
       }
       for (ReviewerState state : ReviewerState.values()) {
         for (String line : commit.getFooterLines(state.getFooterKey())) {
@@ -152,20 +155,47 @@ public class ChangeNotes extends VersionedMetaData {
       return new PatchSet.Id(changeId, psId);
     }
 
-    private PatchSetApproval parseApproval(PatchSet.Id psId, Account.Id accountId,
+    private void parseApproval(PatchSet.Id psId, Account.Id accountId,
         RevCommit commit, String line) throws ConfigInvalidException {
-      try {
+      Table<Account.Id, String, Optional<PatchSetApproval>> curr =
-        LabelVote l = LabelVote.parseWithEquals(line);
+          approvals.get(psId);
-        return new PatchSetApproval(
+      if (curr == null) {
-            new PatchSetApproval.Key(
+        curr = Tables.newCustomTable(
-              psId, parseIdent(commit), new LabelId(l.getLabel())),
+            Maps.<Account.Id, Map<String, Optional<PatchSetApproval>>>
-            l.getValue(),
+                newHashMapWithExpectedSize(2),
-            new Timestamp(commit.getCommitterIdent().getWhen().getTime()));
+            new Supplier<Map<String, Optional<PatchSetApproval>>>() {
-      } catch (IllegalArgumentException e) {
+              @Override
-        ConfigInvalidException pe =
+              public Map<String, Optional<PatchSetApproval>> get() {
-            parseException("invalid %s: %s", FOOTER_LABEL, line);
+                return Maps.newLinkedHashMap();
-        pe.initCause(e);
+              }
-        throw pe;
+            });
+        approvals.put(psId, curr);
+      }
+
+      if (line.startsWith("-")) {
+        String label = line.substring(1);
+        if (!curr.contains(accountId, label)) {
+          curr.put(accountId, label, Optional.<PatchSetApproval> absent());
+        }
+      } else {
+        LabelVote l;
+        try {
+          l = LabelVote.parseWithEquals(line);
+        } catch (IllegalArgumentException e) {
+          ConfigInvalidException pe =
+              parseException("invalid %s: %s", FOOTER_LABEL, line);
+          pe.initCause(e);
+          throw pe;
+        }
+        if (!curr.contains(accountId, l.getLabel())) {
+          curr.put(accountId, l.getLabel(), Optional.of(new PatchSetApproval(
+              new PatchSetApproval.Key(
+                  psId,
+                  accountId,
+                  new LabelId(l.getLabel())),
+              l.getValue(),
+              new Timestamp(commit.getCommitterIdent().getWhen().getTime()))));
+        }
       }
     }
 
@@ -203,22 +233,15 @@ public class ChangeNotes extends VersionedMetaData {
     }
 
     private void pruneReviewers() {
-      Set<Account.Id> removed = Sets.newHashSetWithExpectedSize(reviewers.size());
       Iterator<Map.Entry<Account.Id, ReviewerState>> rit =
           reviewers.entrySet().iterator();
       while (rit.hasNext()) {
         Map.Entry<Account.Id, ReviewerState> e = rit.next();
         if (e.getValue() == ReviewerState.REMOVED) {
-          removed.add(e.getKey());
           rit.remove();
-        }
+          for (Table<Account.Id, ?, ?> curr : approvals.values()) {
-      }
+            curr.rowKeySet().remove(e.getKey());
-
+          }
-      Iterator<Map.Entry<PatchSet.Id, PatchSetApproval>> ait =
-          approvals.entries().iterator();
-      while (ait.hasNext()) {
-        if (removed.contains(ait.next().getValue().getAccountId())) {
-          ait.remove();
         }
       }
     }
@@ -293,7 +316,7 @@ public class ChangeNotes extends VersionedMetaData {
     try {
       Parser parser = new Parser(change.getId(), rev, walk);
       parser.parseAll();
-      approvals = ImmutableListMultimap.copyOf(parser.approvals);
+      approvals = parser.buildApprovals();
       ImmutableSetMultimap.Builder<ReviewerState, Account.Id> reviewers =
           ImmutableSetMultimap.builder();
       for (Map.Entry<Account.Id, ReviewerState> e

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

@@ -20,6 +20,7 @@ import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_PATCH_SET;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.GERRIT_PLACEHOLDER_HOST;
 
 import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Optional;
 import com.google.common.collect.Maps;
 import com.google.gerrit.reviewdb.client.Account;
 import com.google.gerrit.reviewdb.client.Change;
@@ -76,7 +77,7 @@ public class ChangeUpdate extends VersionedMetaData {
   private final ChangeControl ctl;
   private final PersonIdent serverIdent;
   private final Date when;
-  private final Map<String, Short> approvals;
+  private final Map<String, Optional<Short>> approvals;
   private final Map<Account.Id, ReviewerState> reviewers;
   private String subject;
   private PatchSet.Id psId;
@@ -147,7 +148,11 @@ public class ChangeUpdate extends VersionedMetaData {
   }
 
   public void putApproval(String label, short value) {
-    approvals.put(label, value);
+    approvals.put(label, Optional.of(value));
+  }
+
+  public void removeApproval(String label) {
+    approvals.put(label, Optional.<Short> absent());
   }
 
   public void setSubject(String subject) {
@@ -279,9 +284,14 @@ public class ChangeUpdate extends VersionedMetaData {
           .append(ident.getName())
           .append(" <").append(ident.getEmailAddress()).append(">\n");
     }
-    for (Map.Entry<String, Short> e : approvals.entrySet()) {
+
-      addFooter(msg, FOOTER_LABEL,
+    for (Map.Entry<String, Optional<Short>> e : approvals.entrySet()) {
-          new LabelVote(e.getKey(), e.getValue()).formatWithEquals());
+      if (!e.getValue().isPresent()) {
+        addFooter(msg, FOOTER_LABEL, '-', e.getKey());
+      } else {
+        addFooter(msg, FOOTER_LABEL,
+            new LabelVote(e.getKey(), e.getValue().get()).formatWithEquals());
+      }
     }
     commit.setMessage(msg.toString());
     return true;
@@ -292,8 +302,12 @@ public class ChangeUpdate extends VersionedMetaData {
   }
 
   private static void addFooter(StringBuilder sb, FooterKey footer,
-      Object value) {
+      Object... values) {
-    addFooter(sb, footer).append(value).append('\n');
+    addFooter(sb, footer);
+    for (Object value : values) {
+      sb.append(value);
+    }
+    sb.append('\n');
   }
 
   @Override

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

@@ -21,6 +21,7 @@ import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.SECONDS;
 import static org.easymock.EasyMock.expect;
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
 
 import com.google.common.collect.ImmutableSetMultimap;
 import com.google.common.collect.Iterables;
@@ -55,7 +56,9 @@ import com.google.gerrit.server.util.TimeUtil;
 import com.google.gerrit.testutil.FakeAccountCache;
 import com.google.gerrit.testutil.FakeRealm;
 import com.google.gerrit.testutil.InMemoryRepositoryManager;
+import com.google.gwtorm.client.KeyUtil;
 import com.google.gwtorm.server.OrmException;
+import com.google.gwtorm.server.StandardKeyEncoder;
 import com.google.inject.Guice;
 import com.google.inject.Injector;
 import com.google.inject.util.Providers;
@@ -98,6 +101,7 @@ public class ChangeNotesTest {
   @Before
   public void setUp() throws Exception {
     setTimeForTesting();
+    KeyUtil.setEncoderImpl(new StandardKeyEncoder());
 
     serverIdent = new PersonIdent(
         "Gerrit Server", "noreply@gerrit.com", TimeUtil.nowTs(), TZ);
@@ -206,6 +210,27 @@ public class ChangeNotesTest {
     }
   }
 
+  @Test
+  public void approvalTombstoneCommitFormat() throws Exception {
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.removeApproval("Code-Review");
+    update.commit();
+
+    RevWalk walk = new RevWalk(repo);
+    try {
+      RevCommit commit = walk.parseCommit(update.getRevision());
+      walk.parseBody(commit);
+      assertEquals("Update patch set 1\n"
+          + "\n"
+          + "Patch-set: 1\n"
+          + "Label: -Code-Review\n",
+          commit.getFullMessage());
+    } finally {
+      walk.release();
+    }
+  }
+
   @Test
   public void approvalsOnePatchSet() throws Exception {
     Change c = newChange();
@@ -320,6 +345,28 @@ public class ChangeNotesTest {
     assertEquals(truncate(after(c, 2000)), psas.get(1).getGranted());
   }
 
+  @Test
+  public void approvalsTombstone() throws Exception {
+    Change c = newChange();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.putApproval("Not-For-Long", (short) 1);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    PatchSetApproval psa = Iterables.getOnlyElement(
+        notes.getApprovals().get(c.currentPatchSetId()));
+    assertEquals(1, psa.getAccountId().get());
+    assertEquals("Not-For-Long", psa.getLabel());
+    assertEquals((short) 1, psa.getValue());
+
+    update = newUpdate(c, changeOwner);
+    update.removeApproval("Not-For-Long");
+    update.commit();
+
+    notes = newNotes(c);
+    assertTrue(notes.getApprovals().isEmpty());
+  }
+
   @Test
   public void multipleReviewers() throws Exception {
     Change c = newChange();