Do not invoke submit rules twice when updating a change
Currently the submit rules are executed twice whenever a change gets
updated, once for indexing the change, and once for sending out the
change-updated event (both of these code paths create a ChangeData
instance and the submit rules get triggered when certain fields in
ChangeData are accessed for the first time).
Executing the submit rules twice is bad as the execution time for the
submit rules adds up to the latency of the update request, and running
some submit rules (e.g. the code owners submit rule) is rather
expensive.
Change I415d745de caches the ChangeData instance that is created for
indexing the change and makes it available in the postUpdate phase. We
now reuse this ChangeData instance to send out the change-updated event.
Since this ChangeData instance has the results of the submit rules
already cached, the submit rules are not triggered again.
By re-using the ChangeData instance we potentially also avoid doing
other expensive lazy loads twice (EventUtil already avoids this for some
expensive options by skipping them when formatting the change JSON for
the change events, with this change doing this may become unnecessary).
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Iad5f9f2294983087918beceab327dc28eb24deef
diff --git a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
index c8b1715..34679bd 100644
--- a/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
+++ b/javatests/com/google/gerrit/acceptance/api/change/PostReviewIT.java
@@ -40,6 +40,7 @@
import com.google.gerrit.entities.Change;
import com.google.gerrit.entities.LabelId;
import com.google.gerrit.entities.PatchSet;
+import com.google.gerrit.entities.SubmitRecord;
import com.google.gerrit.extensions.annotations.Exports;
import com.google.gerrit.extensions.api.changes.DraftInput;
import com.google.gerrit.extensions.api.changes.ReviewInput;
@@ -60,8 +61,10 @@
import com.google.gerrit.extensions.validators.CommentValidator;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.notedb.ChangeNotes;
+import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.restapi.change.OnPostReview;
import com.google.gerrit.server.restapi.change.PostReview;
+import com.google.gerrit.server.rules.SubmitRule;
import com.google.gerrit.server.update.CommentsRejectedException;
import com.google.gerrit.testing.TestCommentHelper;
import com.google.inject.Inject;
@@ -677,6 +680,19 @@
}
}
+ @Test
+ public void submitRulesAreInvokedOnlyOnce() throws Exception {
+ PushOneCommit.Result r = createChange();
+
+ TestSubmitRule testSubmitRule = new TestSubmitRule();
+ try (Registration registration = extensionRegistry.newRegistration().add(testSubmitRule)) {
+ ReviewInput input = new ReviewInput().label(LabelId.CODE_REVIEW, 1);
+ gApi.changes().id(r.getChangeId()).current().review(input);
+ }
+
+ assertThat(testSubmitRule.count).isEqualTo(1);
+ }
+
private static class TestListener implements CommentAddedListener {
public CommentAddedListener.Event lastCommentAddedEvent;
@@ -753,4 +769,14 @@
assertThat(approvals).containsExactly(labelName, (short) expectedNewValue);
}
}
+
+ private static class TestSubmitRule implements SubmitRule {
+ int count;
+
+ @Override
+ public Optional<SubmitRecord> evaluate(ChangeData changeData) {
+ count++;
+ return Optional.empty();
+ }
+ }
}
