diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java index 5def6a0d99..14aa2e3fe4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java +++ b/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 getForPatchSet(ReviewDb db, ChangeControl ctl, - PatchSet ps) throws OrmException { + private Iterable getForPatchSet(ReviewDb db, + 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(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index 12369853ae..6d2dc581a3 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -512,13 +512,14 @@ public class ChangeJson { allUsers.add(psa.getAccountId()); } - List currentList = labelNormalizer.normalize( - baseCtrl, allApprovals.get(baseCtrl.getChange().currentPatchSetId())); + List currentList = + allApprovals.get(baseCtrl.getChange().currentPatchSetId()); // Most recent, normalized vote on each label for the current patch set by // each user (may be 0). Table 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); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java index 5f2f17e51c..5008d02908 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java +++ b/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 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(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()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index d9d51f71f8..b2265a7700 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -255,12 +255,14 @@ public class Submit implements RestModifyView, // 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) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java index 499b8d783a..19b5ec92f7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/LabelNormalizer.java +++ b/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 unchanged; + private final ImmutableList updated; + private final ImmutableList deleted; + + @VisibleForTesting + Result( + List unchanged, + List updated, + List deleted) { + this.unchanged = ImmutableList.copyOf(unchanged); + this.updated = ImmutableList.copyOf(updated); + this.deleted = ImmutableList.copyOf(deleted); + } + + public ImmutableList getUnchanged() { + return unchanged; + } + + public ImmutableList getUpdated() { + return updated; + } + + public ImmutableList getDeleted() { + return deleted; + } + + public Iterable 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 normalize(Change change, + public Result normalize(Change change, Collection 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 normalize(ChangeControl ctl, + public Result normalize(ChangeControl ctl, Collection approvals) { - List result = + List unchanged = + Lists.newArrayListWithCapacity(approvals.size()); + List updated = + Lists.newArrayListWithCapacity(approvals.size()); + List 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) { - psa = copy(psa); - applyTypeFloor(label, psa); - if (applyRightFloor(ctl, label, psa)) { - result.add(psa); - } + if (label == null) { + deleted.add(psa); + continue; + } + 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 result; + return new Result(unchanged, updated, deleted); } /** diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java index cae5329a79..5412c671d0 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/git/LabelNormalizerTest.java +++ b/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), - norm.normalize(change, ImmutableList.of(cr, v))); + assertEquals(new Result( + 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)), - norm.normalize(change, ImmutableList.of(cr, v))); + assertEquals(new Result( + 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(), - norm.normalize(change, ImmutableList.of(cr, v))); + assertEquals(new Result( + 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), - norm.normalize(change, ImmutableList.of(cr, v))); + assertEquals(new Result( + 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 list(PatchSetApproval... psas) { + return ImmutableList. copyOf(psas); + } } diff --git a/plugins/reviewnotes b/plugins/reviewnotes index 6df20f370c..b544447649 160000 --- a/plugins/reviewnotes +++ b/plugins/reviewnotes @@ -1 +1 @@ -Subproject commit 6df20f370c328a87946246dca08f5f166e69ac6b +Subproject commit b544447649d9ee3b3f78a6a1a7f839cb6a361292