ChangeJson: Ensure for merged changes that labels for approvals are included
There may be approvals on labels that are not contained in the submit
records. E.g. this is the case if there is an approval on a label that
is ignored by a Prolog submit rule. In this case ChangeJson was
failing with:
com.google.gwtorm.server.OrmException: java.lang.NullPointerException
at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:303)
at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:285)
at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:263)
at com.google.gerrit.server.change.GetChange.apply(GetChange.java:50)
at com.google.gerrit.server.change.GetDetail.apply(GetDetail.java:51)
at com.google.gerrit.server.change.GetDetail.apply(GetDetail.java:26)
at com.google.gerrit.httpd.restapi.RestApiServlet.service(RestApiServlet.java:367)
...
Caused by: java.lang.NullPointerException
at com.google.gerrit.server.change.ChangeJson.setLabelScores(ChangeJson.java:670)
at com.google.gerrit.server.change.ChangeJson.labelsForClosedChange(ChangeJson.java:845)
at com.google.gerrit.server.change.ChangeJson.labelsFor(ChangeJson.java:598)
at com.google.gerrit.server.change.ChangeJson.toChangeInfo(ChangeJson.java:499)
at com.google.gerrit.server.change.ChangeJson.format(ChangeJson.java:294)
...
Change-Id: I46073f4077d83963d4a45a50189957cd8c9a1bd0
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
index c09a6a2..479e34b 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -2229,15 +2229,42 @@
assertThat(change.permittedLabels.keySet())
.containsExactly("Code-Review", "Verified");
- // add an approval on the new label
+ // ignore the new label by Prolog submit rule and assert that the label is
+ // no longer returned
+ GitUtil.fetch(testRepo, RefNames.REFS_CONFIG + ":config");
+ testRepo.reset("config");
+ PushOneCommit push2 = pushFactory.create(db, admin.getIdent(), testRepo,
+ "Ignore Verified",
+ "rules.pl",
+ "submit_rule(submit(CR)) :-\n"
+ + " gerrit:max_with_block(-2, 2, 'Code-Review', CR).");
+ push2.to(RefNames.REFS_CONFIG);
+
+ change = gApi.changes()
+ .id(r.getChangeId())
+ .get();
+ assertThat(change.labels.keySet()).containsExactly("Code-Review");
+ assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
+
+ // add an approval on the new label and assert that the label is now
+ // returned although it is ignored by the Prolog submit rule and hence not
+ // included in the submit records
gApi.changes()
.id(r.getChangeId())
.revision(r.getCommit().name())
.review(new ReviewInput().label(
verified.getName(), verified.getMax().getValue()));
+ change = gApi.changes()
+ .id(r.getChangeId())
+ .get();
+ assertThat(change.labels.keySet())
+ .containsExactly("Code-Review", "Verified");
+ assertThat(change.permittedLabels.keySet()).containsExactly("Code-Review");
+
// remove label and assert that it's no longer returned for existing
// changes, even if there is an approval for it
+ cfg = projectCache.checkedGet(project).getConfig();
cfg.getLabelSections().remove(verified.getName());
Util.remove(cfg, Permission.forLabel(verified.getName()), registeredUsers,
heads);
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 c51b81c..1684e76 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
@@ -771,8 +771,6 @@
}
}
- // We can only approximately reconstruct what the submit rule evaluator
- // would have done. These should really come from a stored submit record.
Set<String> labelNames = new HashSet<>();
Multimap<Account.Id, PatchSetApproval> current = HashMultimap.create();
for (PatchSetApproval a : cd.currentApprovals()) {
@@ -787,25 +785,35 @@
}
Map<String, LabelWithStatus> labels;
- if (cd.change().getStatus() == Change.Status.ABANDONED) {
- // For abandoned changes return only labels for which an approval exists.
+ if (cd.change().getStatus() == Change.Status.MERGED) {
+ // Since voting on merged changes is allowed all labels which apply to
+ // the change must be returned. All applying labels can be retrieved from
+ // the submit records, which is what initLabels does.
+ // It's not possible to only compute the labels based on the approvals
+ // since merged changes may not have approvals for all labels (e.g. if not
+ // all labels are required for submit or if the change was auto-closed due
+ // to direct push or if new labels were defined after the change was
+ // merged).
+ labels = initLabels(cd, labelTypes, standard);
+
+ // Also include all labels for which approvals exists. E.g. there can be
+ // approvals for labels that are ignored by a Prolog submit rule and hence
+ // it wouldn't be included in the submit records.
+ for (String name : labelNames) {
+ if (!labels.containsKey(name)) {
+ labels.put(name, LabelWithStatus.create(new LabelInfo(), null));
+ }
+ }
+ } else {
+ // For abandoned changes return only labels for which approvals exist.
// Other labels are not needed since voting on abandoned changes is not
// allowed.
labels = new TreeMap<>(labelTypes.nameComparator());
for (String name : labelNames) {
- labels.put(labelTypes.byLabel(name).getName(),
- LabelWithStatus.create(new LabelInfo(), null));
+ labels.put(name, LabelWithStatus.create(new LabelInfo(), null));
}
- } else {
- // Since voting on merged changes is allowed all labels which apply to
- // the change must be returned. All applying labels can be retrieved from
- // the submit records, which is what initLabels does.
- // It's not possible to compute the labels based on the approvals since
- // merged changes may not have approvals for all labels (e.g. if not all
- // labels are required for submit or if the change was auto-closed due to
- // direct push or if new labels were defined after the change was merged).
- labels = initLabels(cd, labelTypes, standard);
}
+
if (detailed) {
labels.entrySet().stream().forEach(
e -> setLabelValues(labelTypes.byLabel(e.getKey()), e.getValue()));
