From a4a21a69c1cb76c55bb89dc6895b96e4033492f8 Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Thu, 29 Nov 2018 10:09:47 +0100 Subject: [PATCH] Fix IAE when listing reviewers of change that has reviewers by email Reviewers by email don't have an account ID but formatting reviewers as JSON tried to get account IDs of all reviewers and trying to get an account ID from a reviewer by email fails with an IllegalArgumentException. Reviewers by email only have an email and optionally a name. Since they don't have an account we must not invoke the account loader for them. In addition reviewers by email cannot have approvals so we can skip the approval format step. Listing reviewers of a change is not available through the extension API. This is why the test needs to make a REST call to test this functionality. Change-Id: I97226ddce397f2ae9cc3c3526fbc1c4861e4ee58 Signed-off-by: Edwin Kempin (cherry picked from commit b7a24723c121fc159962780b2f1ce0e176fe8e90) --- .../gerrit/server/change/ReviewerJson.java | 30 ++++++++++------- .../rest/change/ChangeReviewersByEmailIT.java | 33 +++++++++++++++++-- 2 files changed, 49 insertions(+), 14 deletions(-) diff --git a/java/com/google/gerrit/server/change/ReviewerJson.java b/java/com/google/gerrit/server/change/ReviewerJson.java index 650256915b..ef2c926393 100644 --- a/java/com/google/gerrit/server/change/ReviewerJson.java +++ b/java/com/google/gerrit/server/change/ReviewerJson.java @@ -23,6 +23,7 @@ import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.extensions.api.changes.ReviewerInfo; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.mail.Address; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.PatchSetApproval; @@ -77,12 +78,15 @@ public class ReviewerJson { if (cd == null || !cd.getId().equals(rsrc.getChangeId())) { cd = changeDataFactory.create(db.get(), rsrc.getChangeResource().getNotes()); } - ReviewerInfo info = - format( - new ReviewerInfo(rsrc.getReviewerUser().getAccountId().get()), - rsrc.getReviewerUser().getAccountId(), - cd); - loader.put(info); + ReviewerInfo info; + if (rsrc.isByEmail()) { + Address address = rsrc.getReviewerByEmail(); + info = ReviewerInfo.byEmail(address.getName(), address.getEmail()); + } else { + Account.Id reviewerAccountId = rsrc.getReviewerUser().getAccountId(); + info = format(new ReviewerInfo(reviewerAccountId.get()), reviewerAccountId, cd); + loader.put(info); + } infos.add(info); } loader.fill(); @@ -94,19 +98,21 @@ public class ReviewerJson { return format(ImmutableList.of(rsrc)); } - public ReviewerInfo format(ReviewerInfo out, Account.Id reviewer, ChangeData cd) + public ReviewerInfo format(ReviewerInfo out, Account.Id reviewerAccountId, ChangeData cd) throws OrmException, PermissionBackendException { PatchSet.Id psId = cd.change().currentPatchSetId(); return format( out, - reviewer, + reviewerAccountId, cd, - approvalsUtil.byPatchSetUser( - db.get(), cd.notes(), psId, new Account.Id(out._accountId), null, null)); + approvalsUtil.byPatchSetUser(db.get(), cd.notes(), psId, reviewerAccountId, null, null)); } public ReviewerInfo format( - ReviewerInfo out, Account.Id reviewer, ChangeData cd, Iterable approvals) + ReviewerInfo out, + Account.Id reviewerAccountId, + ChangeData cd, + Iterable approvals) throws OrmException, PermissionBackendException { LabelTypes labelTypes = cd.getLabelTypes(); @@ -123,7 +129,7 @@ public class ReviewerJson { PatchSet ps = cd.currentPatchSet(); if (ps != null) { PermissionBackend.ForChange perm = - permissionBackend.absentUser(reviewer).database(db).change(cd); + permissionBackend.absentUser(reviewerAccountId).database(db).change(cd); for (SubmitRecord rec : submitRuleEvaluator.evaluate(cd)) { if (rec.labels == null) { diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java index 257c88b4a0..dc71c1fffe 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java @@ -22,8 +22,8 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; import com.google.gerrit.acceptance.AbstractDaemonTest; -import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; +import com.google.gerrit.acceptance.RestResponse; import com.google.gerrit.extensions.api.changes.AddReviewerInput; import com.google.gerrit.extensions.api.changes.AddReviewerResult; import com.google.gerrit.extensions.api.changes.ReviewInput; @@ -35,11 +35,12 @@ import com.google.gerrit.extensions.common.AccountInfo; import com.google.gerrit.extensions.common.ChangeInfo; import com.google.gerrit.mail.Address; import com.google.gerrit.testing.FakeEmailSender.Message; +import com.google.gson.reflect.TypeToken; +import java.lang.reflect.Type; import java.util.List; import org.junit.Before; import org.junit.Test; -@NoHttpd public class ChangeReviewersByEmailIT extends AbstractDaemonTest { @Before @@ -95,6 +96,34 @@ public class ChangeReviewersByEmailIT extends AbstractDaemonTest { } } + @Test + public void listReviewersByEmail() throws Exception { + assume().that(notesMigration.readChanges()).isTrue(); + AccountInfo acc = new AccountInfo("Foo Bar", "foo.bar@gerritcodereview.com"); + + for (ReviewerState state : ImmutableList.of(ReviewerState.CC, ReviewerState.REVIEWER)) { + PushOneCommit.Result r = createChange(); + + AddReviewerInput input = new AddReviewerInput(); + input.reviewer = toRfcAddressString(acc); + input.state = state; + gApi.changes().id(r.getChangeId()).addReviewer(input); + + RestResponse restResponse = + adminRestSession.get("/changes/" + r.getChangeId() + "/reviewers/"); + restResponse.assertOK(); + Type type = new TypeToken>() {}.getType(); + List reviewers = newGson().fromJson(restResponse.getReader(), type); + restResponse.consume(); + + assertThat(reviewers).hasSize(1); + ReviewerInfo reviewerInfo = Iterables.getOnlyElement(reviewers); + assertThat(reviewerInfo._accountId).isNull(); + assertThat(reviewerInfo.name).isEqualTo(acc.name); + assertThat(reviewerInfo.email).isEqualTo(acc.email); + } + } + @Test public void removeByEmail() throws Exception { assume().that(notesMigration.readChanges()).isTrue();