Fix StoreSubmitRequirementsOp for the Rebase submit type
StoreSubmitRequirementsOp is a BatchUpdateOp that is executed when a
change is about to be merged. For projects requiring a rebase upon
submission (e.g. if default submit type = RebaseAlways), the batch
update contains another RebaseSubmitStrategy#RebaseOneOp that creates an
additional patchset (the rebase patchset) which is then submitted. This
patchset is also inserted in the ChangeContext and becomes visible to
the StoreSubmitRequirementsOp.
Current implementation of StoreSubmitRequirementsOp used to create
ChangeData using the ChangeContext#getChange() parameter (which was just
updated by the rebase op). Trying to lookup ChangeData#currentPatchset
fails, because it loads patchsets from NoteDb but tries to find the
patchset with the ID of the one just inserted.
In this change, we update StoreSubmitRequirementsOp to create ChangeData
using the project/change IDs. Now requesting ChangeData#currentPatchset
succeeds but it returns the pre last patchset commit ID. This means that
in this situation, we will store submit requirement results in the
changes notes of the pre last patchset commit. This should be fine
though, since submit requirements will evaluate to the exact same
results for both commits. Additionally, the pre-last commit is the one
for which we displayed the submit requirement results of the last
patchset to the user before it was merged.
Alternatives considered:
* Make SR storage non-atomic, i.e. with a separate update after the
change is merged.
* Implement an ordering of batch update ops to enforce SRs storage op be
executed first. This will still imply storing SR results in the pre last
commit and should not be implemented for just this corner case: Our
batch update ops do/should not rely on ordering.
Bug: Google b/196271654
Change-Id: Ia3cd55a3c8693a1b96e8b70dc084af4650999e76
diff --git a/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java b/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
index 4e20642..57a3cd7 100644
--- a/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
+++ b/java/com/google/gerrit/server/notedb/StoreSubmitRequirementsOp.java
@@ -14,7 +14,6 @@
package com.google.gerrit.server.notedb;
-import com.google.gerrit.entities.Change;
import com.google.gerrit.server.project.SubmitRequirementsEvaluator;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.update.BatchUpdateOp;
@@ -39,9 +38,18 @@
@Override
public boolean updateChange(ChangeContext ctx) throws Exception {
- Change change = ctx.getChange();
- ChangeData changeData = changeDataFactory.create(change);
- ChangeUpdate update = ctx.getUpdate(change.currentPatchSetId());
+ // Create ChangeData using the project/change IDs instead of ctx.getChange(). We do that because
+ // for changes requiring a rebase before submission (e.g. if submit type = RebaseAlways), the
+ // RebaseOp inserts a new patchset that is visible here (via Change#getCurrentPatchset). If we
+ // then try to get ChangeData#currentPatchset it will return null, since it loads patchsets from
+ // NoteDb but tries to find the patchset with the ID of the one just inserted by the rebase op.
+ // Note that this implementation means that, in this case, submit requirement results will be
+ // stored in change notes of the pre last patchset commit. This is fine since submit requirement
+ // results should evaluate to the exact same results for both commits. Additionally, the
+ // pre-last commit is the one for which we displayed the submit requirement results of the last
+ // patchset to the user before it was merged.
+ ChangeData changeData = changeDataFactory.create(ctx.getProject(), ctx.getChange().getId());
+ ChangeUpdate update = ctx.getUpdate(ctx.getChange().currentPatchSetId());
update.putSubmitRequirementResults(evaluator.evaluateAllRequirements(changeData).values());
return !changeData.submitRequirements().isEmpty();
}
diff --git a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
index 7bc393f..4ec1589 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/ChangeIT.java
@@ -4322,37 +4322,43 @@
@Test
public void submitRequirement_storedForClosedChanges() throws Exception {
- configSubmitRequirement(
- project,
- SubmitRequirement.builder()
- .setName("code-review")
- .setSubmittabilityExpression(SubmitRequirementExpression.create("label:code-review=+2"))
- .setAllowOverrideInChildProjects(false)
- .build());
+ for (SubmitType submitType : SubmitType.values()) {
+ Project.NameKey project = createProjectForPush(submitType);
+ TestRepository<InMemoryRepository> repo = cloneProject(project);
+ configSubmitRequirement(
+ project,
+ SubmitRequirement.builder()
+ .setName("code-review")
+ .setSubmittabilityExpression(
+ SubmitRequirementExpression.create("label:code-review=+2"))
+ .setAllowOverrideInChildProjects(false)
+ .build());
- PushOneCommit.Result r = createChange("Add a file", "foo", "content");
- String changeId = r.getChangeId();
+ PushOneCommit.Result r =
+ createChange(repo, "master", "Add a file", "foo", "content", "topic");
+ String changeId = r.getChangeId();
- voteLabel(changeId, "code-review", 2);
+ voteLabel(changeId, "code-review", 2);
- ChangeInfo change = gApi.changes().id(changeId).get();
- assertThat(change.submitRequirements).hasSize(1);
- assertSubmitRequirementStatus(
- change.submitRequirements, "code-review", Status.SATISFIED, /* isLegacy= */ false);
+ ChangeInfo change = gApi.changes().id(changeId).get();
+ assertThat(change.submitRequirements).hasSize(1);
+ assertSubmitRequirementStatus(
+ change.submitRequirements, "code-review", Status.SATISFIED, /* isLegacy= */ false);
- RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
- revision.review(ReviewInput.approve());
- revision.submit();
+ RevisionApi revision = gApi.changes().id(r.getChangeId()).current();
+ revision.review(ReviewInput.approve());
+ revision.submit();
- ChangeNotes notes = notesFactory.create(project, r.getChange().getId());
+ ChangeNotes notes = notesFactory.create(project, r.getChange().getId());
- SubmitRequirementResult result =
- notes.getSubmitRequirementsResult().stream().collect(MoreCollectors.onlyElement());
- assertThat(result.status()).isEqualTo(SubmitRequirementResult.Status.SATISFIED);
- assertThat(result.submittabilityExpressionResult().status())
- .isEqualTo(SubmitRequirementExpressionResult.Status.PASS);
- assertThat(result.submittabilityExpressionResult().expression().expressionString())
- .isEqualTo("label:code-review=+2");
+ SubmitRequirementResult result =
+ notes.getSubmitRequirementsResult().stream().collect(MoreCollectors.onlyElement());
+ assertThat(result.status()).isEqualTo(SubmitRequirementResult.Status.SATISFIED);
+ assertThat(result.submittabilityExpressionResult().status())
+ .isEqualTo(SubmitRequirementExpressionResult.Status.PASS);
+ assertThat(result.submittabilityExpressionResult().expression().expressionString())
+ .isEqualTo("label:code-review=+2");
+ }
}
@Test
@@ -5126,4 +5132,15 @@
.map(r -> String.format("%s=%s", r.name, r.status))
.collect(toImmutableList())));
}
+
+ private Project.NameKey createProjectForPush(SubmitType submitType) throws Exception {
+ Project.NameKey project = projectOperations.newProject().submitType(submitType).create();
+ projectOperations
+ .project(project)
+ .forUpdate()
+ .add(allow(Permission.PUSH).ref("refs/heads/*").group(adminGroupUuid()))
+ .add(allow(Permission.SUBMIT).ref("refs/for/refs/heads/*").group(adminGroupUuid()))
+ .update();
+ return project;
+ }
}
