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 2705991362..8f1acc0c5e 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 @@ public class ChangeDetailFactory extends Handler { 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 394b43fd2d..59755e3528 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 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements final Map psas = new HashMap(); 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 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements final Map psas = new HashMap(); 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 a050cfd47b..4c386e0c6e 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.AddReviewer; 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 @@ public class GerritRequestModule extends FactoryModule { 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 88676bd555..2c756d490b 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.MergeFailSender; 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 @@ public class MergeOp { 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 @@ public class MergeOp { @CanonicalWebUrl @Nullable final Provider 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 @@ public class MergeOp { this.approvalTypes = approvalTypes; patchSetInfoFactory = psif; identifiedUserFactory = iuf; + this.changeControlFactory = changeControlFactory; this.mergeQueue = mergeQueue; this.hooks = hooks; this.accountCache = accountCache; @@ -1240,7 +1245,11 @@ public class MergeOp { c.setStatus(Change.Status.MERGED); final List 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 @@ public class MergeOp { 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 244648a349..8efb4ae3a9 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 @@ public class PublishComments implements Callable { 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 @@ public class PublishComments implements Callable { db.patchComments().update(drafts); } - private void publishApprovals() throws OrmException { + private void publishApprovals(ChangeControl ctl) throws OrmException { ChangeUtil.updated(change); final Set dirty = new HashSet(); @@ -169,7 +169,7 @@ public class PublishComments implements Callable { // 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 0000000000..500ac06309 --- /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 controls; + + @Inject + PerRequestProjectControlCache(ProjectCache projectCache, + CurrentUser userProvider) { + this.projectCache = projectCache; + this.user = userProvider; + this.controls = new HashMap(); + } + + 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 b5a1f27f8b..30ad76c168 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 class ProjectControl { } public static class Factory { - private final ProjectCache projectCache; - private final Provider user; + private final Provider userCache; @Inject - Factory(final ProjectCache pc, final Provider cu) { - projectCache = pc; - user = cu; + Factory(Provider 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 ffed95a1de..7b8b4b9590 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 @@ public abstract class CategoryFunction { * 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 2cb3e8142d..2a871f759d 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.reviewdb.ApprovalCategory.Id; 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 @@ import java.util.Map; /** 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 all); } @@ -55,20 +52,19 @@ public class FunctionState { new HashMap>(); private final Map valid = new HashMap(); + 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 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 @@ public class FunctionState { 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 applyTypeFloor, applyRightFloor. */ 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 a08981703a..08d0705dea 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 class NoBlock extends CategoryFunction { 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 6d2c26cd10..8c76cc52fa 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 @@ public class NoOpFunction extends CategoryFunction { @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 0f63577025..467488a639 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 @@ public class ReviewCommand extends BaseCommand { new PatchSetApproval(new PatchSetApproval.Key(patchSetId, currentUser .getAccountId(), ao.getCategoryId()), v); final FunctionState fs = - functionStateFactory.create(changeControl.getChange(), patchSetId, + functionStateFactory.create(changeControl, patchSetId, Collections. emptyList()); psa.setValue(v); fs.normalize(approvalTypes.byId(psa.getCategoryId()), psa);