From 1898caeeaba925b37d32eb1f611fcdb81a2efbaa Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Fri, 28 Oct 2016 16:25:53 +0200 Subject: [PATCH] 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 --- .../acceptance/api/change/ChangeIT.java | 29 +++++++++++++- .../gerrit/server/change/ChangeJson.java | 38 +++++++++++-------- 2 files changed, 51 insertions(+), 16 deletions(-) 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 c09a6a2f65..479e34b4b2 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 @@ public class ChangeIT extends AbstractDaemonTest { 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 c51b81cc6e..1684e76d72 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 @@ public class ChangeJson { } } - // We can only approximately reconstruct what the submit rule evaluator - // would have done. These should really come from a stored submit record. Set labelNames = new HashSet<>(); Multimap current = HashMultimap.create(); for (PatchSetApproval a : cd.currentApprovals()) { @@ -787,25 +785,35 @@ public class ChangeJson { } Map 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()));