opendev/gerrit
Code Issues Proposed changes

Fix IAE when listing reviewers of change that has reviewers by email

Browse Source 
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 <ekempin@google.com>
(cherry picked from commit b7a24723c1)
This commit is contained in:
Edwin Kempin
2018-11-29 10:09:47 +01:00
committed by David Pursehouse
parent da8a8ef4be
commit a4a21a69c1
2 changed files with 49 additions and 14 deletions
Download Patch File Download Diff File Expand all files Collapse all files

30
java/com/google/gerrit/server/change/ReviewerJson.java
View File

@@ -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.<ReviewerResource>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<PatchSetApproval> approvals)
ReviewerInfo out,
Account.Id reviewerAccountId,
ChangeData cd,
Iterable<PatchSetApproval> 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) {

33
javatests/com/google/gerrit/acceptance/rest/change/ChangeReviewersByEmailIT.java
View File

@@ -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<List<ReviewerInfo>>() {}.getType();
List<ReviewerInfo> 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();