LabelNormalizer: record what happens to each approval

Unchanged, updated, and deleted approvals may need to be handled
separately by the underlying storage layer, e.g. in Submit. Record
this information in the result of LabelNormalizer.normalize().

For callers that don't care whether approvals were updated or not,
provide a convenience method to concatenate them.

Change-Id: Ifea7db3f7333d3ddb5e4d647a1d7e8eeb8cbff11

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

@@ -81,8 +81,8 @@ public class ApprovalCopier {
     db.patchSetApprovals().insert(getForPatchSet(db, ctl, ps));
   }
 
-  private List<PatchSetApproval> getForPatchSet(ReviewDb db, ChangeControl ctl,
+  private Iterable<PatchSetApproval> getForPatchSet(ReviewDb db,
-      PatchSet ps) throws OrmException {
+      ChangeControl ctl, PatchSet ps) throws OrmException {
     ChangeData cd = changeDataFactory.create(db, ctl);
     try {
       ProjectState project =
@@ -123,7 +123,7 @@ public class ApprovalCopier {
             }
           }
         }
-        return labelNormalizer.normalize(ctl, byUser.values());
+        return labelNormalizer.normalize(ctl, byUser.values()).getNormalized();
       } finally {
         repo.close();
       }

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

@@ -512,13 +512,14 @@ public class ChangeJson {
       allUsers.add(psa.getAccountId());
     }
 
-    List<PatchSetApproval> currentList = labelNormalizer.normalize(
+    List<PatchSetApproval> currentList =
-        baseCtrl, allApprovals.get(baseCtrl.getChange().currentPatchSetId()));
+        allApprovals.get(baseCtrl.getChange().currentPatchSetId());
     // Most recent, normalized vote on each label for the current patch set by
     // each user (may be 0).
     Table<Account.Id, String, PatchSetApproval> current = HashBasedTable.create(
         allUsers.size(), baseCtrl.getLabelTypes().getLabelTypes().size());
-    for (PatchSetApproval psa : currentList) {
+    for (PatchSetApproval psa :
+        labelNormalizer.normalize(baseCtrl, currentList).getNormalized()) {
       current.put(psa.getAccountId(), psa.getLabel(), psa);
     }
 

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

@@ -91,12 +91,12 @@ public class ReviewerJson {
 
   public ReviewerInfo format(ReviewerInfo out, ChangeControl ctl,
       List<PatchSetApproval> approvals) throws OrmException {
-    approvals = labelNormalizer.normalize(ctl, approvals);
     LabelTypes labelTypes = ctl.getLabelTypes();
 
     // Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
     out.approvals = new TreeMap<String,String>(labelTypes.nameComparator());
-    for (PatchSetApproval ca : approvals) {
+    for (PatchSetApproval ca :
+        labelNormalizer.normalize(ctl, approvals).getNormalized()) {
       for (PermissionRange pr : ctl.getLabelRanges()) {
         if (!pr.isEmpty()) {
           LabelType at = labelTypes.byLabel(ca.getLabelId());

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

@@ -255,12 +255,14 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
     // presentation view time, except for zero votes used to indicate a reviewer
     // was added. So we need to make sure votes are accurate now. This way if
     // permissions get modified in the future, historical records stay accurate.
-    approvals = labelNormalizer.normalize(rsrc.getControl(), byKey.values());
+    LabelNormalizer.Result normalized =
+        labelNormalizer.normalize(rsrc.getControl(), byKey.values());
 
     // TODO(dborowitz): Don't use a label in notedb; just check when status
     // change happened.
     update.putApproval(submit.getLabel(), submit.getValue());
-    dbProvider.get().patchSetApprovals().upsert(approvals);
+    dbProvider.get().patchSetApprovals().upsert(normalized.getNormalized());
+    dbProvider.get().patchSetApprovals().delete(normalized.getDeleted());
   }
 
   private void checkSubmitRule(RevisionResource rsrc)

gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java

@@ -16,6 +16,10 @@ package com.google.gerrit.server.git;
 
 import static com.google.common.base.Preconditions.checkArgument;
 
+import com.google.common.annotations.VisibleForTesting;
+import com.google.common.base.Objects;
+import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Lists;
 import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.LabelTypes;
@@ -43,6 +47,63 @@ import java.util.List;
  * This class normalizes old votes against current project configuration.
  */
 public class LabelNormalizer {
+  public static class Result {
+    private final ImmutableList<PatchSetApproval> unchanged;
+    private final ImmutableList<PatchSetApproval> updated;
+    private final ImmutableList<PatchSetApproval> deleted;
+
+    @VisibleForTesting
+    Result(
+        List<PatchSetApproval> unchanged,
+        List<PatchSetApproval> updated,
+        List<PatchSetApproval> deleted) {
+      this.unchanged = ImmutableList.copyOf(unchanged);
+      this.updated = ImmutableList.copyOf(updated);
+      this.deleted = ImmutableList.copyOf(deleted);
+    }
+
+    public ImmutableList<PatchSetApproval> getUnchanged() {
+      return unchanged;
+    }
+
+    public ImmutableList<PatchSetApproval> getUpdated() {
+      return updated;
+    }
+
+    public ImmutableList<PatchSetApproval> getDeleted() {
+      return deleted;
+    }
+
+    public Iterable<PatchSetApproval> getNormalized() {
+      return Iterables.concat(unchanged, updated);
+    }
+
+    @Override
+    public boolean equals(Object o) {
+      if (o instanceof Result) {
+        Result r = (Result) o;
+        return Objects.equal(unchanged, r.unchanged)
+            && Objects.equal(updated, r.updated)
+            && Objects.equal(deleted, r.deleted);
+      }
+      return false;
+    }
+
+    @Override
+    public int hashCode() {
+      return Objects.hashCode(unchanged, updated, deleted);
+    }
+
+    @Override
+    public String toString() {
+      return Objects.toStringHelper(this)
+          .add("unchanged", unchanged)
+          .add("updated", updated)
+          .add("deleted", deleted)
+          .toString();
+    }
+  }
+
   private final ChangeControl.GenericFactory changeFactory;
   private final IdentifiedUser.GenericFactory userFactory;
 
@@ -62,7 +123,7 @@ public class LabelNormalizer {
    *     permissions for that label.
    * @throws NoSuchChangeException
    */
-  public List<PatchSetApproval> normalize(Change change,
+  public Result normalize(Change change,
       Collection<PatchSetApproval> approvals) throws NoSuchChangeException {
     return normalize(
         changeFactory.controlFor(change, userFactory.create(change.getOwner())),
@@ -77,9 +138,13 @@ public class LabelNormalizer {
    *     included in the output, nor are approvals where the user has no
    *     permissions for that label.
    */
-  public List<PatchSetApproval> normalize(ChangeControl ctl,
+  public Result normalize(ChangeControl ctl,
       Collection<PatchSetApproval> approvals) {
-    List<PatchSetApproval> result =
+    List<PatchSetApproval> unchanged =
+        Lists.newArrayListWithCapacity(approvals.size());
+    List<PatchSetApproval> updated =
+        Lists.newArrayListWithCapacity(approvals.size());
+    List<PatchSetApproval> deleted =
         Lists.newArrayListWithCapacity(approvals.size());
     LabelTypes labelTypes = ctl.getLabelTypes();
     for (PatchSetApproval psa : approvals) {
@@ -88,19 +153,25 @@ public class LabelNormalizer {
           "Approval %s does not match change %s",
           psa.getKey(), ctl.getChange().getKey());
       if (psa.isSubmit()) {
-        result.add(copy(psa));
+        unchanged.add(psa);
         continue;
       }
       LabelType label = labelTypes.byLabel(psa.getLabelId());
-      if (label != null) {
+      if (label == null) {
-        psa = copy(psa);
+        deleted.add(psa);
-        applyTypeFloor(label, psa);
+        continue;
-        if (applyRightFloor(ctl, label, psa)) {
+      }
-          result.add(psa);
+      PatchSetApproval copy = copy(psa);
+      applyTypeFloor(label, copy);
+      if (!applyRightFloor(ctl, label, copy)) {
+        deleted.add(psa);
+      } else if (copy.getValue() != psa.getValue()) {
+        updated.add(copy);
+      } else {
+        unchanged.add(psa);
       }
     }
-    }
+    return new Result(unchanged, updated, deleted);
-    return result;
   }
 
   /**

gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java

@@ -38,6 +38,7 @@ import com.google.gerrit.server.IdentifiedUser;
 import com.google.gerrit.server.account.AccountManager;
 import com.google.gerrit.server.account.AuthRequest;
 import com.google.gerrit.server.config.AllProjectsName;
+import com.google.gerrit.server.git.LabelNormalizer.Result;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.schema.SchemaCreator;
 import com.google.gerrit.server.util.TimeUtil;
@@ -52,6 +53,8 @@ import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
 
+import java.util.List;
+
 /** Unit tests for {@link LabelNormalizer}. */
 public class LabelNormalizerTest {
   @Inject private AccountManager accountManager;
@@ -136,8 +139,11 @@ public class LabelNormalizerTest {
 
     PatchSetApproval cr = psa(userId, "Code-Review", 2);
     PatchSetApproval v = psa(userId, "Verified", 1);
-    assertEquals(ImmutableList.of(copy(cr, 1), v),
+    assertEquals(new Result(
-        norm.normalize(change, ImmutableList.of(cr, v)));
+          list(v),
+          list(copy(cr, 1)),
+          list()),
+        norm.normalize(change, list(cr, v)));
   }
 
   @Test
@@ -149,16 +155,22 @@ public class LabelNormalizerTest {
 
     PatchSetApproval cr = psa(userId, "Code-Review", 5);
     PatchSetApproval v = psa(userId, "Verified", 5);
-    assertEquals(ImmutableList.of(copy(cr, 2), copy(v, 1)),
+    assertEquals(new Result(
-        norm.normalize(change, ImmutableList.of(cr, v)));
+          list(),
+          list(copy(cr, 2), copy(v, 1)),
+          list()),
+        norm.normalize(change, list(cr, v)));
   }
 
   @Test
   public void emptyPermissionRangeOmitsResult() throws Exception {
     PatchSetApproval cr = psa(userId, "Code-Review", 1);
     PatchSetApproval v = psa(userId, "Verified", 1);
-    assertEquals(ImmutableList.of(),
+    assertEquals(new Result(
-        norm.normalize(change, ImmutableList.of(cr, v)));
+          list(),
+          list(),
+          list(cr, v)),
+        norm.normalize(change, list(cr, v)));
   }
 
   @Test
@@ -169,8 +181,11 @@ public class LabelNormalizerTest {
 
     PatchSetApproval cr = psa(userId, "Code-Review", 0);
     PatchSetApproval v = psa(userId, "Verified", 0);
-    assertEquals(ImmutableList.of(cr),
+    assertEquals(new Result(
-        norm.normalize(change, ImmutableList.of(cr, v)));
+          list(cr),
+          list(),
+          list(v)),
+        norm.normalize(change, list(cr, v)));
   }
 
   private ProjectConfig loadAllProjects() throws Exception {
@@ -204,4 +219,8 @@ public class LabelNormalizerTest {
     result.setValue((short) newValue);
     return result;
   }
+
+  private static List<PatchSetApproval> list(PatchSetApproval... psas) {
+    return ImmutableList.<PatchSetApproval> copyOf(psas);
+  }
 }

plugins/reviewnotes

@@ -1 +1 @@
-Subproject commit 6df20f370c328a87946246dca08f5f166e69ac6b
+Subproject commit b544447649d9ee3b3f78a6a1a7f839cb6a361292