opendev
/
gerrit
Code Issues Proposed changes

Evaluate SubmitRules at most once when formatting ChangeInfo 

This commit fixes a bug where we evaluated SubmitRules twice when
requesting change details and once when searching for changes. We
now do 1 and 0 evaluations respectively.

Change-Id: I6863f764d511ec8d7af5c82abf3982a2df448cc4
(cherry picked from commit 561f98207c6266a2c09446812e0b270a8958d720)
This commit is contained in:
Patrick Hiesel 2021-04-15 10:38:33 +02:00 committed by David Ostrovsky
parent 1b3c845e41
commit fb98f642c0
6 changed files with 57 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
java/com/google/gerrit/server/change/ActionJson.java
View File

@ -31,7 +31,7 @@ import com.google.gerrit.extensions.webui.PrivateInternals_UiActionDescription;
import com.google.gerrit.extensions.webui.UiAction;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.extensions.webui.UiActions;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.google.inject.Singleton;
@ -89,9 +89,9 @@ public class ActionJson {
return Lists.newArrayList(visitorSet);
}
void addChangeActions(ChangeInfo to, ChangeNotes notes) {
void addChangeActions(ChangeInfo to, ChangeData changeData) {
List<ActionVisitor> visitors = visitors();
to.actions = toActionMap(notes, visitors, copy(visitors, to));
to.actions = toActionMap(changeData, visitors, copy(visitors, to));
}
void addRevisionActions(@Nullable ChangeInfo changeInfo, RevisionInfo to, RevisionResource rsrc) {
@ -167,7 +167,7 @@ public class ActionJson {
}
private Map<String, ActionInfo> toActionMap(
ChangeNotes notes, List<ActionVisitor> visitors, ChangeInfo changeInfo) {
ChangeData changeData, List<ActionVisitor> visitors, ChangeInfo changeInfo) {
CurrentUser user = userProvider.get();
Map<String, ActionInfo> out = new LinkedHashMap<>();
if (!user.isIdentifiedUser()) {
@ -175,12 +175,12 @@ public class ActionJson {
}
Iterable<UiAction.Description> descs =
uiActions.from(changeViews, changeResourceFactory.create(notes, user));
uiActions.from(changeViews, changeResourceFactory.create(changeData, user));
// The followup action is a client-side only operation that does not
// have a server side handler. It must be manually registered into the
// resulting action map.
if (!notes.getChange().isAbandoned()) {
if (!changeData.change().isAbandoned()) {
UiAction.Description descr = new UiAction.Description();
PrivateInternals_UiActionDescription.setId(descr, "followup");
PrivateInternals_UiActionDescription.setMethod(descr, "POST");

2
java/com/google/gerrit/server/change/ChangeJson.java
View File

@ -678,7 +678,7 @@ public class ChangeJson {
}
if (has(CURRENT_ACTIONS) || has(CHANGE_ACTIONS)) {
actionJson.addChangeActions(out, cd.notes());
actionJson.addChangeActions(out, cd);
}
if (has(TRACKING_IDS)) {

58
java/com/google/gerrit/server/change/ChangeResource.java
View File

@ -44,9 +44,10 @@ import com.google.gerrit.server.permissions.PermissionBackend;
import com.google.gerrit.server.plugincontext.PluginSetContext;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.inject.Inject;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.inject.TypeLiteral;
import com.google.inject.assistedinject.Assisted;
import com.google.inject.assistedinject.AssistedInject;
import java.util.HashSet;
import java.util.Optional;
import java.util.Set;
@ -66,6 +67,8 @@ public class ChangeResource implements RestResource, HasETag {
public interface Factory {
ChangeResource create(ChangeNotes notes, CurrentUser user);
ChangeResource create(ChangeData changeData, CurrentUser user);
}
private static final String ZERO_ID_STRING = ObjectId.zeroId().name();
@ -77,10 +80,10 @@ public class ChangeResource implements RestResource, HasETag {
private final StarredChangesUtil starredChangesUtil;
private final ProjectCache projectCache;
private final PluginSetContext<ChangeETagComputation> changeETagComputation;
private final ChangeNotes notes;
private final ChangeData changeData;
private final CurrentUser user;
@Inject
@AssistedInject
ChangeResource(
AccountCache accountCache,
ApprovalsUtil approvalUtil,
@ -89,6 +92,7 @@ public class ChangeResource implements RestResource, HasETag {
StarredChangesUtil starredChangesUtil,
ProjectCache projectCache,
PluginSetContext<ChangeETagComputation> changeETagComputation,
ChangeData.Factory changeDataFactory,
@Assisted ChangeNotes notes,
@Assisted CurrentUser user) {
this.accountCache = accountCache;
@ -98,12 +102,34 @@ public class ChangeResource implements RestResource, HasETag {
this.starredChangesUtil = starredChangesUtil;
this.projectCache = projectCache;
this.changeETagComputation = changeETagComputation;
this.notes = notes;
this.changeData = changeDataFactory.create(notes);
this.user = user;
}
@AssistedInject
ChangeResource(
AccountCache accountCache,
ApprovalsUtil approvalUtil,
PatchSetUtil patchSetUtil,
PermissionBackend permissionBackend,
StarredChangesUtil starredChangesUtil,
ProjectCache projectCache,
PluginSetContext<ChangeETagComputation> changeETagComputation,
@Assisted ChangeData changeData,
@Assisted CurrentUser user) {
this.accountCache = accountCache;
this.approvalUtil = approvalUtil;
this.patchSetUtil = patchSetUtil;
this.permissionBackend = permissionBackend;
this.starredChangesUtil = starredChangesUtil;
this.projectCache = projectCache;
this.changeETagComputation = changeETagComputation;
this.changeData = changeData;
this.user = user;
}
public PermissionBackend.ForChange permissions() {
return permissionBackend.user(user).change(notes);
return permissionBackend.user(user).change(getNotes());
}
public CurrentUser getUser() {
@ -111,7 +137,7 @@ public class ChangeResource implements RestResource, HasETag {
}
public Change.Id getId() {
return notes.getChangeId();
return changeData.getId();
}
/** @return true if {@link #getUser()} is the change's owner. */
@ -121,7 +147,7 @@ public class ChangeResource implements RestResource, HasETag {
}
public Change getChange() {
return notes.getChange();
return changeData.change();
}
public Project.NameKey getProject() {
@ -129,7 +155,11 @@ public class ChangeResource implements RestResource, HasETag {
}
public ChangeNotes getNotes() {
return notes;
return changeData.notes();
}
public ChangeData getChangeData() {
return changeData;
}
// This includes all information relevant for ETag computation
@ -153,7 +183,7 @@ public class ChangeResource implements RestResource, HasETag {
accounts.add(getChange().getAssignee());
}
try {
patchSetUtil.byChange(notes).stream().map(PatchSet::uploader).forEach(accounts::add);
patchSetUtil.byChange(getNotes()).stream().map(PatchSet::uploader).forEach(accounts::add);
// It's intentional to include the states for *all* reviewers into the ETag computation.
// We need the states of all current reviewers and CCs because they are part of ChangeInfo.
@ -162,7 +192,7 @@ public class ChangeResource implements RestResource, HasETag {
// set of accounts that posted a message is too expensive. However everyone who posts a
// message is automatically added as reviewer. Hence if we include removed reviewers we can
// be sure that we have all accounts that posted messages on the change.
accounts.addAll(approvalUtil.getReviewers(notes).all());
accounts.addAll(approvalUtil.getReviewers(getNotes()).all());
} catch (StorageException e) {
// This ETag will be invalidated if it loads next time.
}
@ -178,7 +208,7 @@ public class ChangeResource implements RestResource, HasETag {
ObjectId noteId;
try {
noteId = notes.loadRevision();
noteId = getNotes().loadRevision();
} catch (StorageException e) {
noteId = null; // This ETag will be invalidated if it loads next time.
}
@ -194,7 +224,7 @@ public class ChangeResource implements RestResource, HasETag {
changeETagComputation.runEach(
c -> {
String pluginETag = c.getETag(notes.getProjectName(), notes.getChangeId());
String pluginETag = c.getETag(changeData.project(), changeData.getId());
if (pluginETag != null) {
h.putString(pluginETag, UTF_8);
}
@ -207,8 +237,8 @@ public class ChangeResource implements RestResource, HasETag {
TraceContext.newTimer(
"Compute change ETag",
Metadata.builder()
.changeId(notes.getChangeId().get())
.projectName(notes.getProjectName().get())
.changeId(changeData.getId().get())
.projectName(changeData.project().get())
.build())) {
Hasher h = Hashing.murmur3_128().newHasher();
if (user.isIdentifiedUser()) {

2
java/com/google/gerrit/server/change/RevisionJson.java
View File

@ -327,7 +327,7 @@ public class RevisionJson {
actionJson.addRevisionActions(
changeInfo,
out,
new RevisionResource(changeResourceFactory.create(cd.notes(), userProvider.get()), in));
new RevisionResource(changeResourceFactory.create(cd, userProvider.get()), in));
}
if (gpgApi.isEnabled() && has(PUSH_CERTIFICATES)) {

2
java/com/google/gerrit/server/restapi/change/Submit.java
View File

@ -311,7 +311,7 @@ public class Submit
throw new StorageException("Could not determine problems for the change", e);
}
ChangeData cd = changeDataFactory.create(resource.getNotes());
ChangeData cd = resource.getChangeResource().getChangeData();
try {
MergeOp.checkSubmitRule(cd, false);
} catch (ResourceConflictException e) {

11
javatests/com/google/gerrit/acceptance/api/change/ChangeSubmitRequirementIT.java
View File

@ -182,10 +182,8 @@ public class ChangeSubmitRequirementIT extends AbstractDaemonTest {
.id(changeId)
.get(ListChangesOption.ALL_REVISIONS, ListChangesOption.CURRENT_ACTIONS);
// Submit rules are invoked twice, once for populating submit requirements in the change JSON
// and once for checking if the submit button should be visible.
// TODO(hiesel): Try to avoid calling submit rules twice.
assertThat(rule.numberOfEvaluations.get()).isEqualTo(2);
// Submit rules are computed freshly, but only once.
assertThat(rule.numberOfEvaluations.get()).isEqualTo(1);
}
@Test
@ -199,9 +197,8 @@ public class ChangeSubmitRequirementIT extends AbstractDaemonTest {
.withOptions(ListChangesOption.ALL_REVISIONS, ListChangesOption.CURRENT_ACTIONS)
.get();
// Submit rules are invoked to check if the submit button should be visible.
// TODO(hiesel): Change queries must not trigger submit rules.
assertThat(rule.numberOfEvaluations.get()).isEqualTo(1);
// Submit rule evaluation results from the change index are reused
assertThat(rule.numberOfEvaluations.get()).isEqualTo(0);
}
private List<ChangeInfo> queryIsSubmittable() throws Exception {