opendev/gerrit
Code Issues Proposed changes

ChangeJson: Ensure limitToPsId revision is present in output

Browse Source 
Due to a logic bug, we weren't including the limitToPsId revision in the
revisions map in the output ChangeInfo (unless it would have been
included for another reason, such as ALL_REVISIONS being requested).

In addition to a more direct test in RevisionIT, add a test to ActionsIT
ensuring that actions load on a non-current revision, which is the
codepath in the wild that led to discovering this bug.

Fixing the immediate bug unfortunately makes this code even uglier; it
cries out for a more thorough refactoring, but we need to fix this
critical bug preventing ActionVisitors from working. On the plus side,
the additional tests in this change will make that refactoring safer.

Change-Id: Iea61de55f2efc54d3955ff40f993a40e2936beb0
This commit is contained in:
Dave Borowitz
2017-08-29 11:29:04 -04:00
parent 8ecd222ed5
commit 04d82e10e6
3 changed files with 104 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

65
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/ActionsIT.java
View File

@@ -33,6 +33,7 @@ import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.server.change.ChangeJson;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.testutil.ConfigSuite;
@@ -225,6 +226,7 @@ public class ActionsIT extends AbstractDaemonTest {
String changeId2 =
createChangeWithTopic(testRepo, "foo2", "touching b", "b.txt", "real content")
.getChangeId();
int changeNum2 = gApi.changes().id(changeId2).info()._number;
approve(changeId2);
// collide with the other change in the same topic
@@ -243,7 +245,7 @@ public class ActionsIT extends AbstractDaemonTest {
assertThat(info.enabled).isNull();
assertThat(info.label).isEqualTo("Submit whole topic");
assertThat(info.method).isEqualTo("POST");
assertThat(info.title).isEqualTo("Problems with change(s): 2");
assertThat(info.title).isEqualTo("Problems with change(s): " + changeNum2);
} else {
noSubmitWholeTopicAssertions(actions, 1);
}
@@ -357,9 +359,11 @@ public class ActionsIT extends AbstractDaemonTest {
}
@Test
public void revisionActionVisitor() throws Exception {
public void currentRevisionActionVisitor() throws Exception {
String id = createChange().getChangeId();
amendChange(id);
ChangeInfo origChange = gApi.changes().id(id).get(EnumSet.of(ListChangesOption.CHANGE_ACTIONS));
Change.Id changeId = new Change.Id(origChange._number);
class Visitor implements ActionVisitor {
@Override
@@ -373,7 +377,7 @@ public class ActionsIT extends AbstractDaemonTest {
assertThat(changeInfo).isNotNull();
assertThat(changeInfo._number).isEqualTo(origChange._number);
assertThat(revisionInfo).isNotNull();
assertThat(revisionInfo._number).isEqualTo(1);
assertThat(revisionInfo._number).isEqualTo(2);
if (name.equals("cherrypick")) {
return false;
}
@@ -393,24 +397,23 @@ public class ActionsIT extends AbstractDaemonTest {
// Test different codepaths within ActionJson...
// ...via revision API.
visitedRevisionActionsAssertions(origActions, gApi.changes().id(id).current().actions());
visitedCurrentRevisionActionsAssertions(origActions, gApi.changes().id(id).current().actions());
// ...via change API with option.
EnumSet<ListChangesOption> opts = EnumSet.of(CURRENT_ACTIONS, CURRENT_REVISION);
ChangeInfo changeInfo = gApi.changes().id(id).get(opts);
RevisionInfo revisionInfo = Iterables.getOnlyElement(changeInfo.revisions.values());
visitedRevisionActionsAssertions(origActions, revisionInfo.actions);
visitedCurrentRevisionActionsAssertions(origActions, revisionInfo.actions);
// ...via ChangeJson directly.
ChangeData cd = changeDataFactory.create(db, project, new Change.Id(origChange._number));
ChangeData cd = changeDataFactory.create(db, project, changeId);
revisionInfo =
changeJsonFactory
.create(opts)
.getRevisionInfo(cd.changeControl(), Iterables.getOnlyElement(cd.patchSets()));
visitedRevisionActionsAssertions(origActions, revisionInfo.actions);
.getRevisionInfo(cd.changeControl(), cd.patchSet(new PatchSet.Id(changeId, 1)));
}
private void visitedRevisionActionsAssertions(
private void visitedCurrentRevisionActionsAssertions(
Map<String, ActionInfo> origActions, Map<String, ActionInfo> newActions) {
assertThat(newActions).isNotNull();
Set<String> expectedNames = new TreeSet<>(origActions.keySet());
@@ -422,6 +425,50 @@ public class ActionsIT extends AbstractDaemonTest {
assertThat(rebase.label).isEqualTo("All Your Base");
}
@Test
public void oldRevisionActionVisitor() throws Exception {
String id = createChange().getChangeId();
amendChange(id);
ChangeInfo origChange = gApi.changes().id(id).get(EnumSet.of(ListChangesOption.CHANGE_ACTIONS));
class Visitor implements ActionVisitor {
@Override
public boolean visit(String name, ActionInfo actionInfo, ChangeInfo changeInfo) {
return true; // Do nothing; implicitly called for CURRENT_ACTIONS.
}
@Override
public boolean visit(
String name, ActionInfo actionInfo, ChangeInfo changeInfo, RevisionInfo revisionInfo) {
assertThat(changeInfo).isNotNull();
assertThat(changeInfo._number).isEqualTo(origChange._number);
assertThat(revisionInfo).isNotNull();
assertThat(revisionInfo._number).isEqualTo(1);
if (name.equals("description")) {
actionInfo.label = "Describify";
}
return true;
}
}
Map<String, ActionInfo> origActions = gApi.changes().id(id).revision(1).actions();
assertThat(origActions.keySet()).containsExactly("description");
assertThat(origActions.get("description").label).isEqualTo("Edit Description");
Visitor v = new Visitor();
visitorHandle = actionVisitors.add(v);
// Unlike for the current revision, actions for old revisions are only available via the
// revision API.
Map<String, ActionInfo> newActions = gApi.changes().id(id).revision(1).actions();
assertThat(newActions).isNotNull();
assertThat(newActions.keySet()).isEqualTo(origActions.keySet());
ActionInfo description = newActions.get("description");
assertThat(description).isNotNull();
assertThat(description.label).isEqualTo("Describify");
}
private void commonActionsAssertions(Map<String, ActionInfo> actions) {
assertThat(actions).hasSize(4);
assertThat(actions).containsKey("cherrypick");

32
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/revision/RevisionIT.java
View File

@@ -23,6 +23,8 @@ import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.extensions.api.changes.ReviewInput;
import com.google.gerrit.extensions.common.ChangeInfo;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.util.Base64;
import org.junit.Test;
@@ -75,4 +77,34 @@ public class RevisionIT extends AbstractDaemonTest {
response.assertBadRequest();
assertThat(response.getEntityContent()).isEqualTo("invalid parent");
}
@Test
public void getReview() throws Exception {
PushOneCommit.Result r = createChange();
ObjectId ps1Commit = r.getCommit();
r = amendChange(r.getChangeId());
ObjectId ps2Commit = r.getCommit();
ChangeInfo info1 = checkRevisionReview(r, 1, ps1Commit);
assertThat(info1.currentRevision).isNull();
ChangeInfo info2 = checkRevisionReview(r, 2, ps2Commit);
assertThat(info2.currentRevision).isEqualTo(ps2Commit.name());
}
private ChangeInfo checkRevisionReview(
PushOneCommit.Result r, int psNum, ObjectId expectedRevision) throws Exception {
gApi.changes().id(r.getChangeId()).current().review(ReviewInput.approve());
RestResponse response =
adminRestSession.get("/changes/" + r.getChangeId() + "/revisions/" + psNum + "/review");
response.assertOK();
ChangeInfo info = newGson().fromJson(response.getReader(), ChangeInfo.class);
// Check for DETAILED_ACCOUNTS, DETAILED_LABELS, and specified revision.
assertThat(info.owner.name).isNotNull();
assertThat(info.labels.get("Code-Review").all).hasSize(1);
assertThat(info.revisions.keySet()).containsExactly(expectedRevision.name());
return info;
}
}

20
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java
View File

@@ -586,7 +586,7 @@ public class ChangeJson {
// This block must come after the ChangeInfo is mostly populated, since
// it will be passed to ActionVisitors as-is.
if (needRevisions) {
out.revisions = revisions(ctl, cd, src, out);
out.revisions = revisions(ctl, cd, src, limitToPsId, out);
if (out.revisions != null) {
for (Map.Entry<String, RevisionInfo> entry : out.revisions.entrySet()) {
if (entry.getValue().isCurrent) {
@@ -1202,14 +1202,26 @@ public class ChangeJson {
}
private Map<String, RevisionInfo> revisions(
ChangeControl ctl, ChangeData cd, Map<PatchSet.Id, PatchSet> map, ChangeInfo changeInfo)
ChangeControl ctl,
ChangeData cd,
Map<PatchSet.Id, PatchSet> map,
Optional<PatchSet.Id> limitToPsId,
ChangeInfo changeInfo)
throws PatchListNotAvailableException, GpgException, OrmException, IOException {
Map<String, RevisionInfo> res = new LinkedHashMap<>();
try (Repository repo = openRepoIfNecessary(ctl);
RevWalk rw = newRevWalk(repo)) {
for (PatchSet in : map.values()) {
if ((has(ALL_REVISIONS) || in.getId().equals(ctl.getChange().currentPatchSetId()))
&& ctl.isPatchVisible(in, db.get())) {
PatchSet.Id id = in.getId();
boolean want = false;
if (has(ALL_REVISIONS)) {
want = true;
} else if (limitToPsId.isPresent()) {
want = id.equals(limitToPsId.get());
} else {
want = id.equals(ctl.getChange().currentPatchSetId());
}
if (want && ctl.isPatchVisible(in, db.get())) {
res.put(in.getRevision().get(), toRevisionInfo(ctl, cd, in, repo, rw, false, changeInfo));
}
}