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
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 5def6a0..14aa2e3 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 @@
db.patchSetApprovals().insert(getForPatchSet(db, ctl, ps));
}
- private List<PatchSetApproval> getForPatchSet(ReviewDb db, ChangeControl ctl,
- PatchSet ps) throws OrmException {
+ private Iterable<PatchSetApproval> getForPatchSet(ReviewDb db,
+ ChangeControl ctl, PatchSet ps) throws OrmException {
ChangeData cd = changeDataFactory.create(db, ctl);
try {
ProjectState project =
@@ -123,7 +123,7 @@
}
}
}
- 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 1236985..6d2dc58 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 @@
allUsers.add(psa.getAccountId());
}
- List<PatchSetApproval> currentList = labelNormalizer.normalize(
- baseCtrl, allApprovals.get(baseCtrl.getChange().currentPatchSetId()));
+ List<PatchSetApproval> 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<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);
}
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 5f2f17e5..5008d02 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 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());
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 d9d51f7..b2265a7 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 @@
// 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 499b8d7..19b5ec9 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 @@
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 @@
* 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 @@
* 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 @@
* 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 @@
"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 cae5329..5412c67 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.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.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 @@
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 @@
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 @@
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 @@
result.setValue((short) newValue);
return result;
}
+
+ private static List<PatchSetApproval> list(PatchSetApproval... psas) {
+ return ImmutableList.<PatchSetApproval> copyOf(psas);
+ }
}
diff --git a/plugins/reviewnotes b/plugins/reviewnotes
index 6df20f3..b544447 160000
--- a/plugins/reviewnotes
+++ b/plugins/reviewnotes
@@ -1 +1 @@
-Subproject commit 6df20f370c328a87946246dca08f5f166e69ac6b
+Subproject commit b544447649d9ee3b3f78a6a1a7f839cb6a361292
