opendev/gerrit
Code Issues Proposed changes

Support voting on merged changes

Browse Source 
Historically, voting in Gerrit has always happened before a change is
submitted, and is an integral piece of the submit rule based workflow
for submitting changes. However, there are some circumstances where
reviewing a change after it has been merged is advantageous:

* when a change is merged into the branch by a direct push
* when a commit is merged bypassing Gerrit entirely, and a new change
  is created after the fact using the %merged push option (after
  I4de55806).

When this happens, it can be useful to not only write comments but
also vote on the merged changes.

Support this in Gerrit, and add a postSubmit bit to PatchSetApproval.
In NoteDb, this bit is populated automatically, based on whether an
approval happened after the change status was set to merged.

One major caveat is that PostReview enforces that votes can only be
increased, not decreased, for post-submit approvals. This limitation
exists for several reasons.

First, for normal changes that underwent pre-submit code review, we
assume that high review scores were necessary for submission. Allowing
scores to be reduced after the fact could make a change look like it
never should have been submitted in the first place. It should usually
be possible to figure out what happened by reading the message log
closely, but it's less confusing if we try to avoid this situation
entirely.

Second, since a submitted change is already a part of history, no
amount of negative voting on that change will cause the change to stop
being a part of history. (It could be removed via force push, but
that's outside of the code review.) Disallowing scores from being
reduced helps avoid the false impression that a negative score might
actually have some effect.

Finally, we want to enable developers to write tools that audit the
full commit history of a project and ensure that all commits have been
properly approved. These tools can make a major optimization by
assuming that once a change has been approved (e.g. has received
Code-Review+2), it can't be un-approved.

Change-Id: Ie337b5fa5261c873f3038e37300e8e626f9d6d3e
This commit is contained in:
Dave Borowitz
2016-09-09 13:57:14 -04:00
committed by Edwin Kempin
parent c3eaaeb81f
commit e47fe47537
18 changed files with 404 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
Documentation/rest-api-changes.txt
View File

@@ -4644,6 +4644,8 @@ Value of the `tag` field from link:#review-input[ReviewInput] set
while posting the review.
NOTE: To apply different tags on on different votes/comments multiple
invocations of the REST call are required.
|`post_submit` |not set if `false`|
If true, this vote was made after the change was submitted.
|===========================
[[assignee-input]]

9
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/change/ChangeIT.java
View File

@@ -405,15 +405,6 @@ public class ChangeIT extends AbstractDaemonTest {
assertThat(query(r.getChangeId())).isEmpty();
}
@Test
public void voteOnClosedChange() throws Exception {
PushOneCommit.Result r = createChange();
merge(r);
exception.expect(ResourceConflictException.class);
exception.expectMessage("change is closed");
revision(r).review(ReviewInput.reject());
}
@Test
public void rebaseUpToDateChange() throws Exception {
PushOneCommit.Result r = createChange();

133
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/api/revision/RevisionIT.java
View File

@@ -29,9 +29,11 @@ import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.Iterables;
import com.google.common.collect.Iterators;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.PushOneCommit;
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.acceptance.TestProjectInput;
import com.google.gerrit.extensions.api.changes.ChangeApi;
import com.google.gerrit.extensions.api.changes.CherryPickInput;
import com.google.gerrit.extensions.api.changes.DraftApi;
@@ -41,12 +43,15 @@ import com.google.gerrit.extensions.api.changes.ReviewInput.CommentInput;
import com.google.gerrit.extensions.api.changes.RevisionApi;
import com.google.gerrit.extensions.api.projects.BranchInput;
import com.google.gerrit.extensions.client.ChangeStatus;
import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ApprovalInfo;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.ChangeMessageInfo;
import com.google.gerrit.extensions.common.CommentInfo;
import com.google.gerrit.extensions.common.DiffInfo;
import com.google.gerrit.extensions.common.FileInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.common.MergeableInfo;
import com.google.gerrit.extensions.common.RevisionInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
@@ -54,14 +59,18 @@ import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.ETagView;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
import com.google.gerrit.reviewdb.client.Branch;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.server.change.GetRevisionActions;
import com.google.gerrit.server.change.RevisionResource;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.PersonIdent;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.transport.RefSpec;
import org.junit.Test;
import java.io.ByteArrayOutputStream;
@@ -70,6 +79,7 @@ import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
@@ -130,6 +140,115 @@ public class RevisionIT extends AbstractDaemonTest {
.isEqualTo(ChangeStatus.MERGED);
}
@Test
public void postSubmitApproval() throws Exception {
PushOneCommit.Result r = createChange();
String changeId = project.get() + "~master~" + r.getChangeId();
gApi.changes()
.id(changeId)
.current()
.review(ReviewInput.recommend());
String label = "Code-Review";
ApprovalInfo approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(1);
assertThat(approval.postSubmit).isNull();
// Submit by direct push.
git().push()
.setRefSpecs(new RefSpec(r.getCommit().name() + ":refs/heads/master"))
.call();
assertThat(gApi.changes().id(changeId).get().status)
.isEqualTo(ChangeStatus.MERGED);
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(1);
assertThat(approval.postSubmit).isNull();
// Repeating the current label is allowed. Does not flip the postSubmit bit
// due to deduplication codepath.
gApi.changes()
.id(changeId)
.current()
.review(ReviewInput.recommend());
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(1);
assertThat(approval.postSubmit).isNull();
// Reducing vote is not allowed.
try {
gApi.changes()
.id(changeId)
.current()
.review(ReviewInput.dislike());
fail("expected ResourceConflictException");
} catch (ResourceConflictException e) {
assertThat(e).hasMessage(
"Cannot reduce vote on labels for closed change: Code-Review");
}
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(1);
assertThat(approval.postSubmit).isNull();
// Increasing vote is allowed.
gApi.changes()
.id(changeId)
.current()
.review(ReviewInput.approve());
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(2);
assertThat(approval.postSubmit).isTrue();
// Decreasing to previous post-submit vote is still not allowed.
try {
gApi.changes()
.id(changeId)
.current()
.review(ReviewInput.dislike());
fail("expected ResourceConflictException");
} catch (ResourceConflictException e) {
assertThat(e).hasMessage(
"Cannot reduce vote on labels for closed change: Code-Review");
}
approval = getApproval(changeId, label);
assertThat(approval.value).isEqualTo(2);
assertThat(approval.postSubmit).isTrue();
}
@TestProjectInput(submitType = SubmitType.CHERRY_PICK)
@Test
public void approvalCopiedDuringSubmitIsNotPostSubmit() throws Exception {
PushOneCommit.Result r = createChange();
Change.Id id = r.getChange().getId();
gApi.changes()
.id(id.get())
.current()
.review(ReviewInput.approve());
gApi.changes()
.id(id.get())
.current()
.submit();
ChangeData cd = r.getChange();
assertThat(cd.patchSets()).hasSize(2);
PatchSetApproval psa = Iterators.getOnlyElement(
cd.currentApprovals().stream()
.filter(a -> !a.isLegacySubmit()).iterator());
assertThat(psa.getPatchSetId().get()).isEqualTo(2);
assertThat(psa.getLabel()).isEqualTo("Code-Review");
assertThat(psa.getValue()).isEqualTo(2);
assertThat(psa.isPostSubmit()).isFalse();
}
@Test
public void voteOnAbandonedChange() throws Exception {
PushOneCommit.Result r = createChange();
gApi.changes().id(r.getChangeId()).abandon();
exception.expect(ResourceConflictException.class);
exception.expectMessage("change is closed");
gApi.changes().id(r.getChangeId()).current().review(ReviewInput.reject());
}
@Test
public void deleteDraft() throws Exception {
PushOneCommit.Result r = createDraft();
@@ -946,4 +1065,18 @@ public class RevisionIT extends AbstractDaemonTest {
mergeChangeResult.assertOkStatus();
return mergeChangeResult;
}
private ApprovalInfo getApproval(String changeId, String label)
throws Exception {
ChangeInfo info = gApi.changes()
.id(changeId)
.get(EnumSet.of(ListChangesOption.DETAILED_LABELS));
LabelInfo li = info.labels.get(label);
assertThat(li).isNotNull();
int accountId = atrScope.get().getUser().getAccountId().get();
return li.all.stream()
.filter(a -> a._accountId == accountId)
.findFirst()
.get();
}
}

1
gerrit-extension-api/src/main/java/com/google/gerrit/extensions/common/ApprovalInfo.java
View File

@@ -20,6 +20,7 @@ public class ApprovalInfo extends AccountInfo {
public String tag;
public Integer value;
public Timestamp date;
public Boolean postSubmit;
public ApprovalInfo(Integer id) {
super(id);

29
gerrit-reviewdb/src/main/java/com/google/gerrit/reviewdb/client/PatchSetApproval.java
View File

@@ -100,6 +100,9 @@ public final class PatchSetApproval {
@Column(id = 7, notNull = false)
protected Account.Id realAccountId;
@Column(id = 8)
protected boolean postSubmit;
// DELETED: id = 4 (changeOpen)
// DELETED: id = 5 (changeSortKey)
@@ -119,6 +122,7 @@ public final class PatchSetApproval {
granted = src.granted;
realAccountId = src.realAccountId;
tag = src.tag;
postSubmit = src.postSubmit;
}
public PatchSetApproval(PatchSetApproval src) {
@@ -186,14 +190,24 @@ public final class PatchSetApproval {
return tag;
}
public void setPostSubmit(boolean postSubmit) {
this.postSubmit = postSubmit;
}
public boolean isPostSubmit() {
return postSubmit;
}
@Override
public String toString() {
return "["
+ key + ": "
+ value
+ ",tag:" + tag
+ ",realAccountId:" + realAccountId
+ ']';
StringBuilder sb = new StringBuilder("[")
.append(key).append(": ").append(value)
.append(",tag:").append(tag)
.append(",realAccountId:").append(realAccountId);
if (postSubmit) {
sb.append(",postSubmit");
}
return sb.append(']').toString();
}
@Override
@@ -204,7 +218,8 @@ public final class PatchSetApproval {
&& Objects.equals(value, p.value)
&& Objects.equals(granted, p.granted)
&& Objects.equals(tag, p.tag)
&& Objects.equals(realAccountId, p.realAccountId);
&& Objects.equals(realAccountId, p.realAccountId)
&& postSubmit == p.postSubmit;
}
return false;
}

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

@@ -14,6 +14,7 @@
package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.extensions.client.ListChangesOption.ALL_COMMITS;
import static com.google.gerrit.extensions.client.ListChangesOption.ALL_FILES;
import static com.google.gerrit.extensions.client.ListChangesOption.ALL_REVISIONS;
@@ -695,6 +696,10 @@ public class ChangeJson {
private void setAllApprovals(ChangeControl baseCtrl, ChangeData cd,
Map<String, LabelWithStatus> labels) throws OrmException {
Change.Status status = cd.change().getStatus();
checkState(status.isOpen(),
"should not call setAllApprovals on %s change", status);
// Include a user in the output for this label if either:
// - They are an explicit reviewer.
// - They ever voted on this change.
@@ -734,6 +739,9 @@ public class ChangeJson {
}
tag = psa.getTag();
date = psa.getGranted();
if (psa.isPostSubmit()) {
log.warn("unexpected post-submit approval on open change: {}", psa);
}
} else {
// Either the user cannot vote on this label, or they were added as a
// reviewer but have not responded yet. Explicitly check whether the
@@ -816,6 +824,9 @@ public class ChangeJson {
info.value = Integer.valueOf(val);
info.date = psa.getGranted();
info.tag = psa.getTag();
if (psa.isPostSubmit()) {
info.postSubmit = true;
}
}
if (!standard) {
continue;

51
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@@ -15,9 +15,11 @@
package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.server.CommentsUtil.setCommentRevId;
import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
import static java.nio.charset.StandardCharsets.UTF_8;
import static java.util.stream.Collectors.joining;
import static java.util.stream.Collectors.toSet;
import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
@@ -55,6 +57,7 @@ import com.google.gerrit.extensions.restapi.RestApiException;
import com.google.gerrit.extensions.restapi.RestModifyView;
import com.google.gerrit.extensions.restapi.UnprocessableEntityException;
import com.google.gerrit.extensions.restapi.Url;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
import com.google.gerrit.reviewdb.client.Comment;
import com.google.gerrit.reviewdb.client.LabelId;
@@ -799,10 +802,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
}
}
if ((!del.isEmpty() || !ups.isEmpty())
&& ctx.getChange().getStatus().isClosed()) {
throw new ResourceConflictException("change is closed");
}
validatePostSubmitLabels(ctx, labelTypes, previous, ups, del);
// Return early if user is not a reviewer and not posting any labels.
// This allows us to preserve their CC status.
@@ -817,6 +817,49 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
return !del.isEmpty() || !ups.isEmpty();
}
private void validatePostSubmitLabels(ChangeContext ctx,
LabelTypes labelTypes, Map<String, Short> previous,
List<PatchSetApproval> ups, List<PatchSetApproval> del)
throws ResourceConflictException {
if (ctx.getChange().getStatus().isOpen()) {
return; // Not closed, nothing to validate.
} else if (del.isEmpty() && ups.isEmpty()) {
return; // No new votes.
} else if (ctx.getChange().getStatus() != Change.Status.MERGED) {
throw new ResourceConflictException("change is closed");
}
// Disallow reducing votes on any labels post-submit. This assumes the
// high values were broadly necessary to submit, so reducing them would
// make it possible to take a merged change and make it no longer
// submittable.
List<PatchSetApproval> reduced = new ArrayList<>(ups.size() + del.size());
reduced.addAll(del);
for (PatchSetApproval psa : ups) {
LabelType lt = checkNotNull(labelTypes.byLabel(psa.getLabel()));
String normName = lt.getName();
Short prev = previous.get(normName);
if (prev == null) {
continue;
}
checkState(prev != psa.getValue()); // Should be filtered out above.
if (prev > psa.getValue()) {
reduced.add(psa);
} else {
// Set postSubmit bit in ReviewDb; not required for NoteDb, which sets
// it automatically.
psa.setPostSubmit(true);
}
}
if (!reduced.isEmpty()) {
throw new ResourceConflictException(
"Cannot reduce vote on labels for closed change: "
+ reduced.stream().map(p -> p.getLabel()).distinct().sorted()
.collect(joining(", ")));
}
}
private void forceCallerAsReviewer(ChangeContext ctx,
Map<String, PatchSetApproval> current, List<PatchSetApproval> ups,
List<PatchSetApproval> del) {

13
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeBundle.java
View File

@@ -230,7 +230,7 @@ public class ChangeBundle {
checkColumns(PatchSet.Id.class, 1, 2);
checkColumns(PatchSet.class, 1, 2, 3, 4, 5, 6, 8);
checkColumns(PatchSetApproval.Key.class, 1, 2, 3);
checkColumns(PatchSetApproval.class, 1, 2, 3, 6, 7);
checkColumns(PatchSetApproval.class, 1, 2, 3, 6, 7, 8);
checkColumns(PatchLineComment.Key.class, 1, 2);
checkColumns(PatchLineComment.class, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11);
}
@@ -692,6 +692,11 @@ public class ChangeBundle {
// ReviewDb allows timestamps before patch set was created, but NoteDb
// truncates this to the patch set creation timestamp.
//
// ChangeRebuilder ensures all post-submit approvals happen after the
// actual submit, so the timestamps may not line up. This shouldn't really
// happen, because postSubmit shouldn't be set in ReviewDb until after the
// change is submitted in ReviewDb, but you never know.
Timestamp ta = a.getGranted();
Timestamp tb = b.getGranted();
PatchSet psa = checkNotNull(bundleA.patchSets.get(a.getPatchSetId()));
@@ -700,10 +705,12 @@ public class ChangeBundle {
List<String> exclude = new ArrayList<>(1);
if (bundleA.source == REVIEW_DB && bundleB.source == NOTE_DB) {
excludeGranted =
ta.before(psa.getCreatedOn()) && tb.equals(psb.getCreatedOn());
(ta.before(psa.getCreatedOn()) && tb.equals(psb.getCreatedOn()))
|| ta.compareTo(tb) < 0;
} else if (bundleA.source == NOTE_DB && bundleB.source == REVIEW_DB) {
excludeGranted =
tb.before(psb.getCreatedOn()) && ta.equals(psa.getCreatedOn());
tb.before(psb.getCreatedOn()) && ta.equals(psa.getCreatedOn())
|| tb.compareTo(ta) < 0;
}
if (excludeGranted) {
exclude.add("granted");

39
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java
View File

@@ -131,6 +131,7 @@ class ChangeNotesParser {
private final Set<PatchSet.Id> deletedPatchSets;
private final Map<PatchSet.Id, PatchSetState> patchSetStates;
private final Map<ApprovalKey, PatchSetApproval> approvals;
private final List<PatchSetApproval> bufferedApprovals;
private final List<ChangeMessage> allChangeMessages;
private final Multimap<PatchSet.Id, ChangeMessage> changeMessagesByPatchSet;
@@ -160,6 +161,7 @@ class ChangeNotesParser {
this.noteUtil = noteUtil;
this.metrics = metrics;
approvals = new LinkedHashMap<>();
bufferedApprovals = new ArrayList<>();
reviewers = HashBasedTable.create();
allPastReviewers = new ArrayList<>();
reviewerUpdates = new ArrayList<>();
@@ -281,9 +283,6 @@ class ChangeNotesParser {
if (branch == null) {
branch = parseBranch(commit);
}
if (status == null) {
status = parseStatus(commit);
}
PatchSet.Id psId = parsePatchSetId(commit);
if (currentPatchSetId == null || psId.get() > currentPatchSetId.get()) {
@@ -343,6 +342,12 @@ class ChangeNotesParser {
parseSubmitRecords(commit.getFooterLineValues(FOOTER_SUBMITTED_WITH));
}
if (status == null) {
status = parseStatus(commit);
}
// Parse approvals after status to treat approvals in the same commit as
// "Status: merged" as non-post-submit.
for (String line : commit.getFooterLineValues(FOOTER_LABEL)) {
parseApproval(psId, accountId, realAccountId, ts, line);
}
@@ -543,6 +548,15 @@ class ChangeNotesParser {
if (status == null) {
throw invalidFooter(FOOTER_STATUS, statusLines.get(0));
}
// All approvals after MERGED and before the next status change get the
// postSubmit bit. (Currently the state can't change from MERGED to
// something else, but just in case.)
if (status == Change.Status.MERGED) {
for (PatchSetApproval psa : bufferedApprovals) {
psa.setPostSubmit(true);
}
}
bufferedApprovals.clear();
return status;
}
@@ -666,15 +680,18 @@ class ChangeNotesParser {
throw parseException(
"patch set %s requires an identified user as uploader", psId.get());
}
PatchSetApproval psa;
if (line.startsWith("-")) {
parseRemoveApproval(psId, accountId, realAccountId, ts, line);
psa = parseRemoveApproval(psId, accountId, realAccountId, ts, line);
} else {
parseAddApproval(psId, accountId, realAccountId, ts, line);
psa = parseAddApproval(psId, accountId, realAccountId, ts, line);
}
bufferedApprovals.add(psa);
}
private void parseAddApproval(PatchSet.Id psId, Account.Id committerId,
Account.Id realAccountId, Timestamp ts, String line)
private PatchSetApproval parseAddApproval(PatchSet.Id psId,
Account.Id committerId, Account.Id realAccountId, Timestamp ts,
String line)
throws ConfigInvalidException {
// There are potentially 3 accounts involved here:
// 1. The account from the commit, which is the effective IdentifiedUser
@@ -727,11 +744,12 @@ class ChangeNotesParser {
if (!approvals.containsKey(k)) {
approvals.put(k, psa);
}
return psa;
}
private void parseRemoveApproval(PatchSet.Id psId, Account.Id committerId,
Account.Id realAccountId, Timestamp ts, String line)
throws ConfigInvalidException {
private PatchSetApproval parseRemoveApproval(PatchSet.Id psId,
Account.Id committerId, Account.Id realAccountId, Timestamp ts,
String line) throws ConfigInvalidException {
// See comments in parseAddApproval about the various users involved.
Account.Id effectiveAccountId;
String label;
@@ -775,6 +793,7 @@ class ChangeNotesParser {
if (!approvals.containsKey(k)) {
approvals.put(k, remove);
}
return remove;
}
private void parseSubmitRecords(List<String> lines)

5
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ApprovalEvent.java
View File

@@ -38,4 +38,9 @@ class ApprovalEvent extends Event {
checkUpdate(update);
update.putApproval(psa.getLabel(), psa.getValue());
}
@Override
protected boolean isPostSubmitApproval() {
return psa.isPostSubmit();
}
}

16
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/ChangeRebuilderImpl.java
View File

@@ -25,6 +25,7 @@ import static java.util.stream.Collectors.toList;
import com.google.common.base.Splitter;
import com.google.common.collect.ArrayListMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.collect.Multimap;
import com.google.common.collect.Ordering;
@@ -387,7 +388,9 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
private void sortAndFillEvents(Change change, Change noteDbChange,
List<Event> events, Integer minPsNum) {
events.add(new FinalUpdatesEvent(change, noteDbChange));
Event finalUpdates = new FinalUpdatesEvent(change, noteDbChange);
events.add(finalUpdates);
setPostSubmitDeps(events);
new EventSorter(events).sort();
// Ensure the first event in the list creates the change, setting the author
@@ -436,6 +439,17 @@ public class ChangeRebuilderImpl extends ChangeRebuilder {
}
}
private void setPostSubmitDeps(List<Event> events) {
Optional<Event> submitEvent = Lists.reverse(events).stream()
.filter(Event::isSubmit)
.findFirst();
if (submitEvent.isPresent()) {
events.stream()
.filter(Event::isPostSubmitApproval)
.forEach(e -> e.addDep(submitEvent.get()));
}
}
private void flushEventsToUpdate(NoteDbUpdateManager manager,
EventList<Event> events, Change change) throws OrmException, IOException {
if (events.isEmpty()) {

8
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/Event.java
View File

@@ -86,6 +86,14 @@ abstract class Event implements Comparable<Event> {
abstract void apply(ChangeUpdate update) throws OrmException, IOException;
protected boolean isPostSubmitApproval() {
return false;
}
protected boolean isSubmit() {
return false;
}
@Override
public String toString() {
return MoreObjects.toStringHelper(this)

9
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/EventList.java
View File

@@ -28,6 +28,7 @@ import java.util.Objects;
class EventList<E extends Event> implements Iterable<E> {
private final ArrayList<E> list = new ArrayList<>();
private boolean isSubmit;
@Override
public Iterator<E> iterator() {
@@ -36,10 +37,14 @@ class EventList<E extends Event> implements Iterable<E> {
void add(E e) {
list.add(e);
if (e.isSubmit()) {
isSubmit = true;
}
}
void clear() {
list.clear();
isSubmit = false;
}
boolean isEmpty() {
@@ -61,6 +66,10 @@ class EventList<E extends Event> implements Iterable<E> {
|| !Objects.equals(e.tag, last.tag)) {
return false; // Different patch set, author, or tag.
}
if (e.isPostSubmitApproval() && isSubmit) {
// Post-submit approvals must come after the update that submits.
return false;
}
long t = e.when.getTime();
long tFirst = getFirstTime();

5
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/FinalUpdatesEvent.java
View File

@@ -58,4 +58,9 @@ class FinalUpdatesEvent extends Event {
update.setSubjectForCommit("Final NoteDb migration updates");
}
}
@Override
protected boolean isSubmit() {
return change.getStatus() == Change.Status.MERGED;
}
}

7
gerrit-server/src/main/java/com/google/gerrit/server/notedb/rebuild/StatusChangeEvent.java
View File

@@ -51,9 +51,9 @@ class StatusChangeEvent extends Event {
return Optional.empty();
}
private final Change.Status status;
private final Change change;
private final Change noteDbChange;
private final Change.Status status;
private StatusChangeEvent(ChangeMessage message, Change change,
Change noteDbChange, Change.Status status) {
@@ -87,4 +87,9 @@ class StatusChangeEvent extends Event {
noteDbChange.setSubmissionId(change.getSubmissionId());
}
}
@Override
protected boolean isSubmit() {
return status == Change.Status.MERGED;
}
}

2
gerrit-server/src/main/java/com/google/gerrit/server/schema/SchemaVersion.java
View File

@@ -33,7 +33,7 @@ import java.util.List;
/** A version of the database schema. */
public abstract class SchemaVersion {
/** The current schema version. */
public static final Class<Schema_133> C = Schema_133.class;
public static final Class<Schema_134> C = Schema_134.class;
public static int getBinaryVersion() {
return guessVersion(C);

25
gerrit-server/src/main/java/com/google/gerrit/server/schema/Schema_134.java Normal file
View File

@@ -0,0 +1,25 @@
// Copyright (C) 2016 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.schema;
import com.google.inject.Inject;
import com.google.inject.Provider;
public class Schema_134 extends SchemaVersion {
@Inject
Schema_134(Provider<Schema_133> prior) {
super(prior);
}
}

75
gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java
View File

@@ -398,6 +398,81 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
assertThat(approvals.get(1).getValue()).isEqualTo((short) -1);
}
@Test
public void approvalsPostSubmit() throws Exception {
Change c = newChange();
RequestId submissionId = RequestId.forChange(c);
ChangeUpdate update = newUpdate(c, changeOwner);
update.putApproval("Code-Review", (short) 1);
update.putApproval("Verified", (short) 1);
update.commit();
update = newUpdate(c, changeOwner);
update.merge(submissionId, ImmutableList.of(
submitRecord("NOT_READY", null,
submitLabel("Verified", "OK", changeOwner.getAccountId()),
submitLabel("Code-Review", "NEED", null))));
update.commit();
update = newUpdate(c, changeOwner);
update.putApproval("Code-Review", (short) 2);
update.commit();
ChangeNotes notes = newNotes(c);
List<PatchSetApproval> approvals =
Lists.newArrayList(notes.getApprovals().values());
assertThat(approvals).hasSize(2);
assertThat(approvals.get(0).getLabel()).isEqualTo("Verified");
assertThat(approvals.get(0).getValue()).isEqualTo((short) 1);
assertThat(approvals.get(0).isPostSubmit()).isFalse();
assertThat(approvals.get(1).getLabel()).isEqualTo("Code-Review");
assertThat(approvals.get(1).getValue()).isEqualTo((short) 2);
assertThat(approvals.get(1).isPostSubmit()).isTrue();
}
@Test
public void approvalsDuringSubmit() throws Exception {
Change c = newChange();
RequestId submissionId = RequestId.forChange(c);
ChangeUpdate update = newUpdate(c, changeOwner);
update.putApproval("Code-Review", (short) 1);
update.putApproval("Verified", (short) 1);
update.commit();
Account.Id ownerId = changeOwner.getAccountId();
Account.Id otherId = otherUser.getAccountId();
update = newUpdate(c, otherUser);
update.merge(submissionId, ImmutableList.of(
submitRecord("NOT_READY", null,
submitLabel("Verified", "OK", ownerId),
submitLabel("Code-Review", "NEED", null))));
update.putApproval("Other-Label", (short) 1);
update.putApprovalFor(ownerId, "Code-Review", (short) 2);
update.commit();
update = newUpdate(c, otherUser);
update.putApproval("Other-Label", (short) 2);
update.commit();
ChangeNotes notes = newNotes(c);
List<PatchSetApproval> approvals =
Lists.newArrayList(notes.getApprovals().values());
assertThat(approvals).hasSize(3);
assertThat(approvals.get(0).getAccountId()).isEqualTo(ownerId);
assertThat(approvals.get(0).getLabel()).isEqualTo("Verified");
assertThat(approvals.get(0).getValue()).isEqualTo(1);
assertThat(approvals.get(0).isPostSubmit()).isFalse();
assertThat(approvals.get(1).getAccountId()).isEqualTo(ownerId);
assertThat(approvals.get(1).getLabel()).isEqualTo("Code-Review");
assertThat(approvals.get(1).getValue()).isEqualTo(2);
assertThat(approvals.get(1).isPostSubmit()).isFalse(); // During submit.
assertThat(approvals.get(2).getAccountId()).isEqualTo(otherId);
assertThat(approvals.get(2).getLabel()).isEqualTo("Other-Label");
assertThat(approvals.get(2).getValue()).isEqualTo(2);
assertThat(approvals.get(2).isPostSubmit()).isTrue();
}
@Test
public void multipleReviewers() throws Exception {
Change c = newChange();