Reuse cached RefControl data in FunctionState
Currently the FunctionState code is run for every change in a
change listing in the web UI, or the user dashboard. Looking
up the relevant permissions for each reviewer is costly, but
can typically be cached and reused off the request's cache,
as most changes are not on username specific references.
Change-Id: Idff421e1eecf651db11549d6591484578cd9543c
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
index 2705991..8f1acc0 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/ChangeDetailFactory.java
@@ -154,8 +154,7 @@
db.patchSetApprovals().byChange(changeId).toList();
if (detail.getChange().getStatus().isOpen()) {
- final FunctionState fs =
- functionState.create(detail.getChange(), psId, allApprovals);
+ final FunctionState fs = functionState.create(control, psId, allApprovals);
for (final ApprovalType at : approvalTypes.getApprovalTypes()) {
CategoryFunction.forCategory(at.getCategory()).run(at, fs);
diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java
index 394b43f..59755e3 100644
--- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java
+++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/patch/PatchDetailServiceImpl.java
@@ -182,7 +182,7 @@
final Map<ApprovalCategory.Id, PatchSetApproval> psas =
new HashMap<ApprovalCategory.Id, PatchSetApproval>();
final FunctionState fs =
- functionStateFactory.create(change, ps_id, psas.values());
+ functionStateFactory.create(cc, ps_id, psas.values());
for (final PatchSetApproval ca : db.patchSetApprovals()
.byPatchSetUser(ps_id, aid)) {
@@ -229,7 +229,7 @@
final Map<ApprovalCategory.Id, PatchSetApproval> psas =
new HashMap<ApprovalCategory.Id, PatchSetApproval>();
final FunctionState fs =
- functionStateFactory.create(change, ps_id, psas.values());
+ functionStateFactory.create(cc, ps_id, psas.values());
for (PatchSetApproval ca : db.patchSetApprovals().byPatchSet(ps_id)) {
final ApprovalCategory.Id category = ca.getCategoryId();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java
index a050cfd..4c386e0 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritRequestModule.java
@@ -40,6 +40,7 @@
import com.google.gerrit.server.patch.PublishComments;
import com.google.gerrit.server.patch.RemoveReviewer;
import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.PerRequestProjectControlCache;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.query.change.ChangeQueryBuilder;
import com.google.gerrit.server.query.change.ChangeQueryRewriter;
@@ -58,6 +59,7 @@
bind(ChangeQueryRewriter.class);
bind(AnonymousUser.class).in(RequestScoped.class);
+ bind(PerRequestProjectControlCache.class).in(RequestScoped.class);
bind(ChangeControl.Factory.class).in(SINGLETON);
bind(GroupControl.Factory.class).in(SINGLETON);
bind(ProjectControl.Factory.class).in(SINGLETON);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
index 88676bd..2c756d4 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOp.java
@@ -40,6 +40,8 @@
import com.google.gerrit.server.mail.MergedSender;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException;
+import com.google.gerrit.server.project.ChangeControl;
+import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.workflow.CategoryFunction;
@@ -137,6 +139,7 @@
private final ApprovalTypes approvalTypes;
private final PatchSetInfoFactory patchSetInfoFactory;
private final IdentifiedUser.GenericFactory identifiedUserFactory;
+ private final ChangeControl.GenericFactory changeControlFactory;
private final MergeQueue mergeQueue;
private final PersonIdent myIdent;
@@ -167,6 +170,7 @@
@CanonicalWebUrl @Nullable final Provider<String> cwu,
final ApprovalTypes approvalTypes, final PatchSetInfoFactory psif,
final IdentifiedUser.GenericFactory iuf,
+ final ChangeControl.GenericFactory changeControlFactory,
@GerritPersonIdent final PersonIdent myIdent,
final MergeQueue mergeQueue, @Assisted final Branch.NameKey branch,
final ChangeHookRunner hooks, final AccountCache accountCache,
@@ -183,6 +187,7 @@
this.approvalTypes = approvalTypes;
patchSetInfoFactory = psif;
identifiedUserFactory = iuf;
+ this.changeControlFactory = changeControlFactory;
this.mergeQueue = mergeQueue;
this.hooks = hooks;
this.accountCache = accountCache;
@@ -1240,7 +1245,11 @@
c.setStatus(Change.Status.MERGED);
final List<PatchSetApproval> approvals =
schema.patchSetApprovals().byChange(changeId).toList();
- final FunctionState fs = functionState.create(c, merged, approvals);
+ final FunctionState fs = functionState.create(
+ changeControlFactory.controlFor(
+ c,
+ identifiedUserFactory.create(c.getOwner())),
+ merged, approvals);
for (ApprovalType at : approvalTypes.getApprovalTypes()) {
CategoryFunction.forCategory(at.getCategory()).run(at, fs);
}
@@ -1256,6 +1265,8 @@
a.cache(c);
}
schema.patchSetApprovals().update(approvals);
+ } catch (NoSuchChangeException err) {
+ log.warn("Cannot normalize approvals for change " + changeId, err);
} catch (OrmException err) {
log.warn("Cannot normalize approvals for change " + changeId, err);
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java
index 244648a..8efb4ae 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PublishComments.java
@@ -120,7 +120,7 @@
final boolean isCurrent = patchSetId.equals(change.currentPatchSetId());
if (isCurrent && change.getStatus().isOpen()) {
- publishApprovals();
+ publishApprovals(ctl);
} else if (! approvals.isEmpty()) {
throw new InvalidChangeOperationException("Change is closed");
} else {
@@ -141,7 +141,7 @@
db.patchComments().update(drafts);
}
- private void publishApprovals() throws OrmException {
+ private void publishApprovals(ChangeControl ctl) throws OrmException {
ChangeUtil.updated(change);
final Set<ApprovalCategory.Id> dirty = new HashSet<ApprovalCategory.Id>();
@@ -169,7 +169,7 @@
// Normalize all of the items the user is changing.
//
final FunctionState functionState =
- functionStateFactory.create(change, patchSetId, all);
+ functionStateFactory.create(ctl, patchSetId, all);
for (final ApprovalCategoryValue.Id want : approvals) {
final PatchSetApproval a = mine.get(want.getParentKey());
final short o = a.getValue();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/PerRequestProjectControlCache.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/PerRequestProjectControlCache.java
new file mode 100644
index 0000000..500ac06
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/PerRequestProjectControlCache.java
@@ -0,0 +1,52 @@
+// Copyright (C) 2011 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.project;
+
+import com.google.gerrit.reviewdb.Project;
+import com.google.gerrit.server.CurrentUser;
+import com.google.inject.Inject;
+import com.google.inject.servlet.RequestScoped;
+
+import java.util.HashMap;
+import java.util.Map;
+
+/** Caches {@link ProjectControl} objects for the current user of the request. */
+@RequestScoped
+public class PerRequestProjectControlCache {
+ private final ProjectCache projectCache;
+ private final CurrentUser user;
+ private final Map<Project.NameKey, ProjectControl> controls;
+
+ @Inject
+ PerRequestProjectControlCache(ProjectCache projectCache,
+ CurrentUser userProvider) {
+ this.projectCache = projectCache;
+ this.user = userProvider;
+ this.controls = new HashMap<Project.NameKey, ProjectControl>();
+ }
+
+ ProjectControl get(Project.NameKey nameKey) throws NoSuchProjectException {
+ ProjectControl ctl = controls.get(nameKey);
+ if (ctl == null) {
+ ProjectState p = projectCache.get(nameKey);
+ if (p == null) {
+ throw new NoSuchProjectException(nameKey);
+ }
+ ctl = p.controlFor(user);
+ controls.put(nameKey, ctl);
+ }
+ return ctl;
+ }
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index b5a1f27..30ad76c 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -80,22 +80,16 @@
}
public static class Factory {
- private final ProjectCache projectCache;
- private final Provider<CurrentUser> user;
+ private final Provider<PerRequestProjectControlCache> userCache;
@Inject
- Factory(final ProjectCache pc, final Provider<CurrentUser> cu) {
- projectCache = pc;
- user = cu;
+ Factory(Provider<PerRequestProjectControlCache> uc) {
+ userCache = uc;
}
public ProjectControl controlFor(final Project.NameKey nameKey)
throws NoSuchProjectException {
- final ProjectState p = projectCache.get(nameKey);
- if (p == null) {
- throw new NoSuchProjectException(nameKey);
- }
- return p.controlFor(user.get());
+ return userCache.get().get(nameKey);
}
public ProjectControl validateFor(final Project.NameKey nameKey)
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/CategoryFunction.java b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/CategoryFunction.java
index ffed95a..7b8b4b9 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/CategoryFunction.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/CategoryFunction.java
@@ -76,11 +76,4 @@
* the valid status into.
*/
public abstract void run(ApprovalType at, FunctionState state);
-
- public boolean isValid(final CurrentUser user, final ApprovalType at,
- final FunctionState state) {
- return !state.controlFor(user) //
- .getRange(Permission.forLabel(at.getCategory().getLabelName())) //
- .isEmpty();
- }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/FunctionState.java b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/FunctionState.java
index 2cb3e81..2a871f7 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/FunctionState.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/FunctionState.java
@@ -27,10 +27,7 @@
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.GroupCache;
-import com.google.gerrit.server.project.ProjectCache;
-import com.google.gerrit.server.project.ProjectControl;
-import com.google.gerrit.server.project.ProjectState;
-import com.google.gerrit.server.project.RefControl;
+import com.google.gerrit.server.project.ChangeControl;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
@@ -44,7 +41,7 @@
/** State passed through to a {@link CategoryFunction}. */
public class FunctionState {
public interface Factory {
- FunctionState create(Change c, PatchSet.Id psId,
+ FunctionState create(ChangeControl c, PatchSet.Id psId,
Collection<PatchSetApproval> all);
}
@@ -55,20 +52,19 @@
new HashMap<ApprovalCategory.Id, Collection<PatchSetApproval>>();
private final Map<ApprovalCategory.Id, Boolean> valid =
new HashMap<ApprovalCategory.Id, Boolean>();
+ private final ChangeControl callerChangeControl;
private final Change change;
- private final ProjectState project;
@Inject
FunctionState(final ApprovalTypes approvalTypes,
- final ProjectCache projectCache,
final IdentifiedUser.GenericFactory userFactory, final GroupCache egc,
- @Assisted final Change c, @Assisted final PatchSet.Id psId,
+ @Assisted final ChangeControl c, @Assisted final PatchSet.Id psId,
@Assisted final Collection<PatchSetApproval> all) {
this.approvalTypes = approvalTypes;
this.userFactory = userFactory;
- change = c;
- project = projectCache.get(change.getProject());
+ callerChangeControl = c;
+ change = c.getChange();
for (final PatchSetApproval ca : all) {
if (psId.equals(ca.getPatchSetId())) {
@@ -147,10 +143,8 @@
a.setValue((short) range.squash(a.getValue()));
}
- RefControl controlFor(final CurrentUser user) {
- ProjectControl pc = project.controlFor(user);
- RefControl rc = pc.controlForRef(change.getDest().get());
- return rc;
+ private ChangeControl controlFor(CurrentUser user) {
+ return callerChangeControl.forUser(user);
}
/** Run <code>applyTypeFloor</code>, <code>applyRightFloor</code>. */
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/NoBlock.java b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/NoBlock.java
index a089817..08d0705 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/NoBlock.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/NoBlock.java
@@ -15,7 +15,6 @@
package com.google.gerrit.server.workflow;
import com.google.gerrit.common.data.ApprovalType;
-import com.google.gerrit.server.CurrentUser;
/** A function that does nothing. */
public class NoBlock extends CategoryFunction {
@@ -25,10 +24,4 @@
public void run(final ApprovalType at, final FunctionState state) {
state.valid(at, true);
}
-
- @Override
- public boolean isValid(final CurrentUser user, final ApprovalType at,
- final FunctionState state) {
- return true;
- }
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/NoOpFunction.java b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/NoOpFunction.java
index 6d2c26c..8c76cc5 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/workflow/NoOpFunction.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/workflow/NoOpFunction.java
@@ -15,7 +15,6 @@
package com.google.gerrit.server.workflow;
import com.google.gerrit.common.data.ApprovalType;
-import com.google.gerrit.server.CurrentUser;
/** A function that does nothing. */
public class NoOpFunction extends CategoryFunction {
@@ -24,10 +23,4 @@
@Override
public void run(final ApprovalType at, final FunctionState state) {
}
-
- @Override
- public boolean isValid(final CurrentUser user, final ApprovalType at,
- final FunctionState state) {
- return false;
- }
}
diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
index 0f63577..467488a 100644
--- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
+++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/commands/ReviewCommand.java
@@ -380,7 +380,7 @@
new PatchSetApproval(new PatchSetApproval.Key(patchSetId, currentUser
.getAccountId(), ao.getCategoryId()), v);
final FunctionState fs =
- functionStateFactory.create(changeControl.getChange(), patchSetId,
+ functionStateFactory.create(changeControl, patchSetId,
Collections.<PatchSetApproval> emptyList());
psa.setValue(v);
fs.normalize(approvalTypes.byId(psa.getCategoryId()), psa);
