From 6cae753a4c58dab5dac3ae7e573a797426f6f32d Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 14 Feb 2013 17:11:25 -0800 Subject: [PATCH] Access LabelTypes through ProjectState rather than globally We want to allow projects to define their own labels, so we can't assume the label list is global. In typical cases, we can access it through a ProjectControl, ChangeControl, or related factory. This does result in a few more places where we propagate NoSuchProject/ChangeExceptions where there were none before. This is intended: operations that query/modify labels on, say, a bare Change.Id now do need to verify that the project/change exists. For now, leave code in LabelTypesProvider, but try not to inject LabelTypes where at all possible. Change-Id: I4936ccafdb41848aaac3e335adf4648369d6abbc --- .../rpc/changedetail/ChangeDetailFactory.java | 6 ++- .../EditCommitMessageHandler.java | 3 +- .../PatchSetPublishDetailFactory.java | 6 +-- .../httpd/rpc/changedetail/PublishAction.java | 4 +- .../rpc/changedetail/RebaseChangeHandler.java | 3 +- .../rpc/patch/PatchDetailServiceImpl.java | 12 ++--- .../gerrit/common/ChangeHookRunner.java | 16 +++--- .../google/gerrit/server/ApprovalsUtil.java | 13 ++--- .../com/google/gerrit/server/ChangeUtil.java | 8 ++- .../google/gerrit/server/change/Abandon.java | 2 +- .../gerrit/server/change/ChangeJson.java | 23 ++++----- .../gerrit/server/change/PostReview.java | 11 +++-- .../gerrit/server/change/PostReviewers.java | 21 +++----- .../google/gerrit/server/change/Restore.java | 2 +- .../gerrit/server/change/ReviewerJson.java | 4 +- .../server/changedetail/PublishDraft.java | 13 +++-- .../server/changedetail/RebaseChange.java | 6 ++- .../server/config/LabelTypesProvider.java | 2 +- .../gerrit/server/events/EventFactory.java | 35 ++++++------- .../com/google/gerrit/server/git/MergeOp.java | 25 +++++----- .../gerrit/server/git/ReceiveCommits.java | 12 +++-- .../server/git/SubmitStrategyFactory.java | 14 ++++-- .../gerrit/server/mail/MergedSender.java | 10 ++-- .../gerrit/server/project/ChangeControl.java | 6 +++ .../gerrit/server/project/ProjectControl.java | 5 ++ .../gerrit/server/project/ProjectState.java | 8 +++ .../query/change/ChangeQueryBuilder.java | 9 ++-- .../query/change/ChangeQueryRewriter.java | 2 +- .../server/query/change/LabelPredicate.java | 49 +++++++++++++------ .../server/query/change/QueryProcessor.java | 24 +++++++-- .../gerrit/server/workflow/FunctionState.java | 10 ++-- .../gerrit/server/project/RefControlTest.java | 4 +- 32 files changed, 212 insertions(+), 156 deletions(-) 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 4ace9f48d9..120b9af847 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 @@ -39,6 +39,7 @@ import com.google.gerrit.server.git.MergeOp; 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.NoSuchProjectException; import com.google.gerrit.server.query.change.ChangeData; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.ResultSet; @@ -110,7 +111,7 @@ public class ChangeDetailFactory extends Handler { @Override public ChangeDetail call() throws OrmException, NoSuchEntityException, PatchSetInfoNotAvailableException, NoSuchChangeException, - RepositoryNotFoundException, IOException { + RepositoryNotFoundException, IOException, NoSuchProjectException { control = changeControlFactory.validateFor(changeId); final Change change = control.getChange(); final PatchSet patch = db.patchSets().get(change.currentPatchSetId()); @@ -206,7 +207,8 @@ public class ChangeDetailFactory extends Handler { } } - private void load() throws OrmException, NoSuchChangeException { + private void load() throws OrmException, NoSuchChangeException, + NoSuchProjectException { final Change.Status status = detail.getChange().getStatus(); if ((status.equals(Change.Status.NEW) || status.equals(Change.Status.DRAFT)) && testMerge) { diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java index 5e58a35656..5b064c8576 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java @@ -36,6 +36,7 @@ import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; +import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.ssh.NoSshInfo; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -114,7 +115,7 @@ class EditCommitMessageHandler extends Handler { public ChangeDetail call() throws NoSuchChangeException, OrmException, EmailException, NoSuchEntityException, PatchSetInfoNotAvailableException, MissingObjectException, IncorrectObjectTypeException, IOException, - InvalidChangeOperationException { + InvalidChangeOperationException, NoSuchProjectException { final Change.Id changeId = patchSetId.getParentKey(); final ChangeControl control = changeControlFactory.validateFor(changeId); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java index 4755c6f8b3..06c6964f38 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PatchSetPublishDetailFactory.java @@ -15,7 +15,6 @@ package com.google.gerrit.httpd.rpc.changedetail; import com.google.gerrit.common.data.LabelType; -import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.PatchSetPublishDetail; import com.google.gerrit.common.data.PermissionRange; import com.google.gerrit.common.data.SubmitRecord; @@ -47,7 +46,6 @@ final class PatchSetPublishDetailFactory extends Handler private final PatchSetInfoFactory infoFactory; private final ReviewDb db; private final ChangeControl.Factory changeControlFactory; - private final LabelTypes labelTypes; private final AccountInfoCacheFactory aic; private final IdentifiedUser user; @@ -62,12 +60,10 @@ final class PatchSetPublishDetailFactory extends Handler final ReviewDb db, final AccountInfoCacheFactory.Factory accountInfoCacheFactory, final ChangeControl.Factory changeControlFactory, - final LabelTypes labelTypes, final IdentifiedUser user, @Assisted final PatchSet.Id patchSetId) { this.infoFactory = infoFactory; this.db = db; this.changeControlFactory = changeControlFactory; - this.labelTypes = labelTypes; this.aic = accountInfoCacheFactory.create(); this.user = user; @@ -124,7 +120,7 @@ final class PatchSetPublishDetailFactory extends Handler boolean canMakeOk = false; PermissionRange range = rangeByName.get(lbl.label); if (range != null) { - LabelType lt = labelTypes.byLabel(lbl.label); + LabelType lt = control.getLabelTypes().byLabel(lbl.label); if (lt == null || lt.getMax().getValue() == range.getMax()) { canMakeOk = true; } diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PublishAction.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PublishAction.java index f57b29c449..38f8fc62bf 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PublishAction.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/PublishAction.java @@ -22,6 +22,7 @@ import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.server.changedetail.PublishDraft; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.NoSuchChangeException; +import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -53,7 +54,8 @@ class PublishAction extends Handler { @Override public ChangeDetail call() throws OrmException, NoSuchEntityException, IllegalStateException, PatchSetInfoNotAvailableException, - NoSuchChangeException, RepositoryNotFoundException, IOException { + NoSuchChangeException, RepositoryNotFoundException, IOException, + NoSuchProjectException { final ReviewResult result = publishFactory.create(patchSetId).call(); if (result.getErrors().size() > 0) { throw new IllegalStateException("Cannot publish patchset"); diff --git a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RebaseChangeHandler.java b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RebaseChangeHandler.java index b47c8f2c3f..b9acfa9c6c 100644 --- a/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RebaseChangeHandler.java +++ b/gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/RebaseChangeHandler.java @@ -24,6 +24,7 @@ import com.google.gerrit.server.changedetail.RebaseChange; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; +import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -60,7 +61,7 @@ class RebaseChangeHandler extends Handler { public ChangeDetail call() throws NoSuchChangeException, OrmException, EmailException, NoSuchEntityException, PatchSetInfoNotAvailableException, MissingObjectException, IncorrectObjectTypeException, IOException, - InvalidChangeOperationException { + InvalidChangeOperationException, NoSuchProjectException { rebaseChange.rebase(patchSetId, currentUser.getAccountId()); return changeDetailFactory.create(patchSetId.getParentKey()).call(); } 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 0177ac19c2..b019b58581 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 @@ -16,7 +16,6 @@ package com.google.gerrit.httpd.rpc.patch; import com.google.gerrit.common.data.ApprovalSummary; import com.google.gerrit.common.data.ApprovalSummarySet; -import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.ChangeDetail; import com.google.gerrit.common.data.PatchDetailService; import com.google.gerrit.common.data.PatchScript; @@ -39,6 +38,7 @@ import com.google.gerrit.server.changedetail.DeleteDraftPatchSet; 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.NoSuchProjectException; import com.google.gerrit.server.workflow.FunctionState; import com.google.gwtjsonrpc.common.AsyncCallback; import com.google.gwtjsonrpc.common.VoidResult; @@ -56,8 +56,6 @@ import java.util.Set; class PatchDetailServiceImpl extends BaseServiceImplementation implements PatchDetailService { - private final LabelTypes labelTypes; - private final AccountInfoCacheFactory.Factory accountInfoCacheFactory; private final ChangeControl.Factory changeControlFactory; private final DeleteDraftPatchSet.Factory deleteDraftPatchSetFactory; @@ -69,7 +67,6 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements @Inject PatchDetailServiceImpl(final Provider schema, final Provider currentUser, - final LabelTypes labelTypes, final AccountInfoCacheFactory.Factory accountInfoCacheFactory, final ChangeControl.Factory changeControlFactory, final DeleteDraftPatchSet.Factory deleteDraftPatchSetFactory, @@ -78,7 +75,6 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements final SaveDraft.Factory saveDraftFactory, final ChangeDetailFactory.Factory changeDetailFactory) { super(schema, currentUser); - this.labelTypes = labelTypes; this.accountInfoCacheFactory = accountInfoCacheFactory; this.changeControlFactory = changeControlFactory; @@ -149,6 +145,8 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements return changeDetailFactory.create(result.getChangeId()).call(); } catch (NoSuchChangeException e) { throw new Failure(new NoSuchChangeException(result.getChangeId())); + } catch (NoSuchProjectException e) { + throw new Failure(e); } catch (NoSuchEntityException e) { throw new Failure(e); } catch (PatchSetInfoNotAvailableException e) { @@ -189,7 +187,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements continue; } if (change.getStatus().isOpen()) { - fs.normalize(labelTypes.byId(category.get()), ca); + fs.normalize(cc.getLabelTypes().byId(category.get()), ca); } if (ca.getValue() == 0) { continue; @@ -235,7 +233,7 @@ class PatchDetailServiceImpl extends BaseServiceImplementation implements continue; } if (change.getStatus().isOpen()) { - fs.normalize(labelTypes.byId(category.get()), ca); + fs.normalize(cc.getLabelTypes().byId(category.get()), ca); } if (ca.getValue() == 0) { continue; diff --git a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java index 79a444509a..6e8639452f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java +++ b/gerrit-server/src/main/java/com/google/gerrit/common/ChangeHookRunner.java @@ -206,8 +206,6 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener { private final AccountCache accountCache; - private final LabelTypes labelTypes; - private final EventFactory eventFactory; private final SitePaths sitePaths; @@ -232,16 +230,17 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener { final GitRepositoryManager repoManager, final @GerritServerConfig Config config, final @AnonymousCowardName String anonymousCowardName, - final SitePaths sitePath, final ProjectCache projectCache, - final AccountCache accountCache, final LabelTypes labelTypes, - final EventFactory eventFactory, final SitePaths sitePaths, + final SitePaths sitePath, + final ProjectCache projectCache, + final AccountCache accountCache, + final EventFactory eventFactory, + final SitePaths sitePaths, final DynamicSet unrestrictedListeners) { this.anonymousCowardName = anonymousCowardName; this.repoManager = repoManager; this.hookQueue = queue.createQueue(1, "hook"); this.projectCache = projectCache; this.accountCache = accountCache; - this.labelTypes = labelTypes; this.eventFactory = eventFactory; this.sitePaths = sitePath; this.unrestrictedListeners = unrestrictedListeners; @@ -395,11 +394,12 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener { event.patchSet = eventFactory.asPatchSetAttribute(patchSet); event.comment = comment; + LabelTypes labelTypes = projectCache.get(change.getProject()).getLabelTypes(); if (approvals.size() > 0) { event.approvals = new ApprovalAttribute[approvals.size()]; int i = 0; for (Map.Entry approval : approvals.entrySet()) { - event.approvals[i++] = getApprovalAttribute(approval); + event.approvals[i++] = getApprovalAttribute(labelTypes, approval); } } @@ -612,7 +612,7 @@ public class ChangeHookRunner implements ChangeHooks, LifecycleListener { * @param approval * @return object suitable for serialization to JSON */ - private ApprovalAttribute getApprovalAttribute( + private ApprovalAttribute getApprovalAttribute(LabelTypes labelTypes, Entry approval) { ApprovalAttribute a = new ApprovalAttribute(); a.type = approval.getKey().get(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java index 078c2262d4..5fe9a871af 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java @@ -46,20 +46,17 @@ import java.util.Set; */ public class ApprovalsUtil { private final ReviewDb db; - private final LabelTypes labelTypes; @Inject - public ApprovalsUtil(ReviewDb db, LabelTypes labelTypes) { + public ApprovalsUtil(ReviewDb db) { this.db = db; - this.labelTypes = labelTypes; } /** * Resync the changeOpen status which is cached in the approvals table for * performance reasons */ - public void syncChangeStatus(final Change change) - throws OrmException { + public void syncChangeStatus(final Change change) throws OrmException { final List approvals = db.patchSetApprovals().byChange(change.getId()).toList(); for (PatchSetApproval a : approvals) { @@ -78,7 +75,7 @@ public class ApprovalsUtil { * @return List The previous approvals */ public List copyVetosToPatchSet(ReviewDb db, - PatchSet.Id dest) throws OrmException { + LabelTypes labelTypes, PatchSet.Id dest) throws OrmException { PatchSet.Id source; if (dest.get() > 1) { source = new PatchSet.Id(dest.getParentKey(), dest.get() - 1); @@ -103,8 +100,8 @@ public class ApprovalsUtil { return patchSetApprovals; } - public void addReviewers(ReviewDb db, Change change, PatchSet ps, - PatchSetInfo info, Set wantReviewers, + public void addReviewers(ReviewDb db, LabelTypes labelTypes, Change change, + PatchSet ps, PatchSetInfo info, Set wantReviewers, Set existingReviewers) throws OrmException { List allTypes = labelTypes.getLabelTypes(); if (allTypes.isEmpty()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java index 7127dce22f..480cd66025 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java @@ -40,6 +40,7 @@ import com.google.gerrit.server.patch.PatchSetInfoFactory; import com.google.gerrit.server.patch.PatchSetInfoNotAvailableException; import com.google.gerrit.server.project.InvalidChangeOperationException; import com.google.gerrit.server.project.NoSuchChangeException; +import com.google.gerrit.server.project.NoSuchProjectException; import com.google.gerrit.server.project.RefControl; import com.google.gerrit.server.util.IdGenerator; import com.google.gwtorm.server.AtomicUpdate; @@ -176,7 +177,8 @@ public class ChangeUtil { db.trackingIds().delete(toDelete); } - public static void testMerge(MergeOp.Factory opFactory, Change change) { + public static void testMerge(MergeOp.Factory opFactory, Change change) + throws NoSuchProjectException { opFactory.create(change.getDest()).verifyMergeability(change); } @@ -443,7 +445,9 @@ public class ChangeUtil { "Change %s was modified", change.getId())); } - approvalsUtil.copyVetosToPatchSet(db, change.currentPatchSetId()); + approvalsUtil.copyVetosToPatchSet(db, + refControl.getProjectControl().getLabelTypes(), + change.currentPatchSetId()); final List footerLines = newCommit.getFooterLines(); updateTrackingIds(db, change, trackingFooters, footerLines); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java index 79083912a7..f766297a6f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Abandon.java @@ -101,7 +101,7 @@ public class Abandon implements RestModifyView { } message = newMessage(input, caller, change); db.changeMessages().insert(Collections.singleton(message)); - new ApprovalsUtil(db, null).syncChangeStatus(change); + new ApprovalsUtil(db).syncChangeStatus(change); db.commit(); } finally { db.rollback(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java index bc8cc264e4..01c337cb87 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeJson.java @@ -114,7 +114,6 @@ public class ChangeJson { } private final Provider db; - private final LabelTypes labelTypes; private final CurrentUser user; private final AnonymousUser anonymous; private final IdentifiedUser.GenericFactory userFactory; @@ -133,7 +132,6 @@ public class ChangeJson { @Inject ChangeJson( Provider db, - LabelTypes at, CurrentUser u, AnonymousUser au, IdentifiedUser.GenericFactory uf, @@ -144,7 +142,6 @@ public class ChangeJson { @CanonicalWebUrl Provider curl, Urls urls) { this.db = db; - this.labelTypes = at; this.user = u; this.anonymous = au; this.userFactory = uf; @@ -321,16 +318,18 @@ public class ChangeJson { return null; } + LabelTypes labelTypes = ctl.getLabelTypes(); if (cd.getChange().getStatus().isOpen()) { - return labelsForOpenChange(cd, standard, detailed); + return labelsForOpenChange(cd, labelTypes, standard, detailed); } else { - return labelsForClosedChange(cd, standard, detailed); + return labelsForClosedChange(cd, labelTypes, standard, detailed); } } private Map labelsForOpenChange(ChangeData cd, - boolean standard, boolean detailed) throws OrmException { - Map labels = initLabels(cd, standard); + LabelTypes labelTypes, boolean standard, boolean detailed) + throws OrmException { + Map labels = initLabels(cd, labelTypes, standard); if (detailed) { setAllApprovals(cd, labels); } @@ -349,8 +348,8 @@ public class ChangeJson { return labels; } - private Map initLabels(ChangeData cd, boolean standard) - throws OrmException { + private Map initLabels(ChangeData cd, + LabelTypes labelTypes, boolean standard) throws OrmException { // Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167. Map labels = new TreeMap(LabelOrdering.create(labelTypes)); @@ -448,7 +447,7 @@ public class ChangeJson { } for (PatchSetApproval psa : current.get(accountId)) { // TODO Support arbitrary labels placed by a reviewer. - LabelType lt = labelTypes.byId(psa.getCategoryId().get()); + LabelType lt = ctl.getLabelTypes().byId(psa.getCategoryId().get()); if (lt == null) { continue; } @@ -465,7 +464,8 @@ public class ChangeJson { } private Map labelsForClosedChange(ChangeData cd, - boolean standard, boolean detailed) throws OrmException { + LabelTypes labelTypes, boolean standard, boolean detailed) + throws OrmException { Set allUsers = Sets.newHashSet(); for (PatchSetApproval psa : cd.allApprovals(db)) { allUsers.add(psa.getAccountId()); @@ -570,6 +570,7 @@ public class ChangeJson { return null; } + LabelTypes labelTypes = ctl.getLabelTypes(); ListMultimap permitted = LinkedListMultimap.create(); for (SubmitRecord rec : submitRecords(cd)) { if (rec.labels == null) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java index bb38b9e382..647853afb7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java @@ -102,7 +102,6 @@ public class PostReview implements RestModifyView { } private final ReviewDb db; - private final LabelTypes labelTypes; private final EmailReviewComments.Factory email; @Deprecated private final ChangeHooks hooks; @@ -116,11 +115,9 @@ public class PostReview implements RestModifyView { @Inject PostReview(ReviewDb db, - LabelTypes labelTypes, EmailReviewComments.Factory email, ChangeHooks hooks) { this.db = db; - this.labelTypes = labelTypes; this.email = email; this.hooks = hooks; } @@ -180,7 +177,8 @@ public class PostReview implements RestModifyView { Map.Entry ent = itr.next(); // TODO Support more generic label assignments. - LabelType lt = labelTypes.byLabel(ent.getKey()); + LabelType lt = revision.getControl().getLabelTypes() + .byLabel(ent.getKey()); if (lt == null) { if (strict) { throw new BadRequestException(String.format( @@ -343,6 +341,7 @@ public class PostReview implements RestModifyView { List upd = Lists.newArrayList(); Map current = scanLabels(rsrc, del); + LabelTypes labelTypes = rsrc.getControl().getLabelTypes(); for (Map.Entry ent : labels.entrySet()) { // TODO Support arbitrary label names. LabelType lt = labelTypes.byLabel(ent.getKey()); @@ -406,7 +405,8 @@ public class PostReview implements RestModifyView { PatchSetApproval c = new PatchSetApproval(new PatchSetApproval.Key( rsrc.getPatchSet().getId(), rsrc.getAccountId(), - labelTypes.getLabelTypes().get(0).getApprovalCategoryId()), + rsrc.getControl().getLabelTypes().getLabelTypes().get(0) + .getApprovalCategoryId()), (short) 0); c.setGranted(timestamp); c.cache(change); @@ -426,6 +426,7 @@ public class PostReview implements RestModifyView { private Map scanLabels(RevisionResource rsrc, List del) throws OrmException { + LabelTypes labelTypes = rsrc.getControl().getLabelTypes(); Map current = Maps.newHashMap(); for (PatchSetApproval a : db.patchSetApprovals().byPatchSetUser( rsrc.getPatchSet().getId(), rsrc.getAccountId())) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java index c8f31a314c..c95f4edd0a 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java @@ -22,7 +22,6 @@ import com.google.common.collect.Lists; import com.google.common.collect.Sets; import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.data.GroupDescription; -import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.common.errors.NoSuchGroupException; import com.google.gerrit.extensions.restapi.AuthException; @@ -84,7 +83,6 @@ public class PostReviewers implements RestModifyView { private final Provider db; private final IdentifiedUser currentUser; private final IdentifiedUser.GenericFactory identifiedUserFactory; - private final ApprovalCategory.Id addReviewerCategoryId; private final Config cfg; private final ChangeHooks hooks; private final AccountCache accountCache; @@ -100,7 +98,6 @@ public class PostReviewers implements RestModifyView { Provider db, IdentifiedUser currentUser, IdentifiedUser.GenericFactory identifiedUserFactory, - LabelTypes labelTypes, @GerritServerConfig Config cfg, ChangeHooks hooks, AccountCache accountCache, @@ -118,9 +115,6 @@ public class PostReviewers implements RestModifyView { this.hooks = hooks; this.accountCache = accountCache; this.json = json; - - this.addReviewerCategoryId = Iterables.getLast(labelTypes.getLabelTypes()) - .getApprovalCategoryId(); } @Override @@ -234,7 +228,7 @@ public class PostReviewers implements RestModifyView { continue; } ChangeControl control = rsrc.getControl().forUser(user); - PatchSetApproval psa = dummyApproval(control.getChange(), psid, id); + PatchSetApproval psa = dummyApproval(control, psid, id); result.reviewers.add(json.format( new ReviewerInfo(id), control, ImmutableList.of(psa))); toInsert.add(psa); @@ -283,12 +277,13 @@ public class PostReviewers implements RestModifyView { || AccountGroup.REGISTERED_USERS.equals(groupUUID)); } - private PatchSetApproval dummyApproval(Change change, PatchSet.Id patchSetId, - Account.Id reviewerId) { - PatchSetApproval dummyApproval = - new PatchSetApproval(new PatchSetApproval.Key(patchSetId, reviewerId, - addReviewerCategoryId), (short) 0); - dummyApproval.cache(change); + private PatchSetApproval dummyApproval(ChangeControl ctl, + PatchSet.Id patchSetId, Account.Id reviewerId) { + ApprovalCategory.Id id = new ApprovalCategory.Id( + Iterables.getLast(ctl.getLabelTypes().getLabelTypes()).getId()); + PatchSetApproval dummyApproval = new PatchSetApproval( + new PatchSetApproval.Key(patchSetId, reviewerId, id), (short) 0); + dummyApproval.cache(ctl.getChange()); return dummyApproval; } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java index 7bae29379d..afb58f9f99 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Restore.java @@ -100,7 +100,7 @@ public class Restore implements RestModifyView { } message = newMessage(input, caller, change); db.changeMessages().insert(Collections.singleton(message)); - new ApprovalsUtil(db, null).syncChangeStatus(change); + new ApprovalsUtil(db).syncChangeStatus(change); db.commit(); } finally { db.rollback(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java index c3c951d535..2893cc3d6b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java @@ -43,17 +43,14 @@ import java.util.TreeMap; public class ReviewerJson { private final Provider db; - private final LabelTypes labelTypes; private final FunctionState.Factory functionState; private final AccountInfo.Loader.Factory accountLoaderFactory; @Inject ReviewerJson(Provider db, - LabelTypes labelTypes, FunctionState.Factory functionState, AccountInfo.Loader.Factory accountLoaderFactory) { this.db = db; - this.labelTypes = labelTypes; this.functionState = functionState; this.accountLoaderFactory = accountLoaderFactory; } @@ -84,6 +81,7 @@ public class ReviewerJson { .byPatchSetUser(psId, out._id)); } + LabelTypes labelTypes = ctl.getLabelTypes(); FunctionState fs = functionState.create(ctl, psId, approvals); for (LabelType at : labelTypes.getLabelTypes()) { CategoryFunction.forType(at).run(at, fs); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/PublishDraft.java b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/PublishDraft.java index 1cf0bd24c0..22eae2d9fa 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/PublishDraft.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/PublishDraft.java @@ -19,6 +19,7 @@ import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromApprovals; import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters; import com.google.gerrit.common.ChangeHooks; +import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.ReviewResult; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Change; @@ -108,6 +109,7 @@ public class PublishDraft implements Callable { final Change.Id changeId = patchSetId.getParentKey(); result.setChangeId(changeId); final ChangeControl control = changeControlFactory.validateFor(changeId); + final LabelTypes labelTypes = control.getLabelTypes(); final PatchSet patch = db.patchSets().get(patchSetId); if (patch == null) { throw new NoSuchChangeException(changeId); @@ -147,7 +149,8 @@ public class PublishDraft implements Callable { hooks.doDraftPublishedHook(updatedChange, updatedPatchSet, db); sendNotifications(control.getChange().getStatus() == Change.Status.DRAFT, - (IdentifiedUser) control.getCurrentUser(), updatedChange, updatedPatchSet); + (IdentifiedUser) control.getCurrentUser(), updatedChange, updatedPatchSet, + labelTypes); } } @@ -156,8 +159,8 @@ public class PublishDraft implements Callable { private void sendNotifications(final boolean newChange, final IdentifiedUser currentUser, final Change updatedChange, - final PatchSet updatedPatchSet) throws OrmException, IOException, - PatchSetInfoNotAvailableException { + final PatchSet updatedPatchSet, final LabelTypes labelTypes) + throws OrmException, IOException, PatchSetInfoNotAvailableException { final Repository git = repoManager.openRepository(updatedChange.getProject()); try { final RevWalk revWalk = new RevWalk(git); @@ -175,7 +178,7 @@ public class PublishDraft implements Callable { recipients.remove(me); if (newChange) { - approvalsUtil.addReviewers(db, updatedChange, updatedPatchSet, info, + approvalsUtil.addReviewers(db, labelTypes, updatedChange, updatedPatchSet, info, recipients.getReviewers(), Collections. emptySet()); try { CreateChangeSender cm = createChangeSenderFactory.create(updatedChange); @@ -192,7 +195,7 @@ public class PublishDraft implements Callable { db.patchSetApprovals().byChange(updatedChange.getId()).toList(); final MailRecipients oldRecipients = getRecipientsFromApprovals(patchSetApprovals); - approvalsUtil.addReviewers(db, updatedChange, updatedPatchSet, info, + approvalsUtil.addReviewers(db, labelTypes, updatedChange, updatedPatchSet, info, recipients.getReviewers(), oldRecipients.getAll()); final ChangeMessage msg = new ChangeMessage(new ChangeMessage.Key(updatedChange.getId(), diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java index 98c0133f8b..5c4fdcaf2d 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/changedetail/RebaseChange.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.changedetail; import com.google.common.collect.Sets; import com.google.gerrit.common.ChangeHookRunner; +import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; @@ -373,7 +374,10 @@ public class RebaseChange { "Change %s was modified", change.getId())); } - approvalsUtil.copyVetosToPatchSet(db, change.currentPatchSetId()); + final LabelTypes labelTypes = changeControlFactory.controlFor(change) + .getLabelTypes(); + approvalsUtil.copyVetosToPatchSet(db, labelTypes, + change.currentPatchSetId()); final ChangeMessage cmsg = new ChangeMessage(new ChangeMessage.Key(change.getId(), diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/LabelTypesProvider.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/LabelTypesProvider.java index 08ca057888..0aa396b1a9 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/config/LabelTypesProvider.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/LabelTypesProvider.java @@ -53,7 +53,7 @@ public class LabelTypesProvider implements Provider { db.close(); } } catch (OrmException e) { - throw new ProvisionException("Cannot query approval categories", e); + throw new ProvisionException("Cannot query label categories", e); } return new LabelTypes(Collections.unmodifiableList(types)); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java index 7637e880aa..acdbcc616c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/events/EventFactory.java @@ -62,7 +62,6 @@ public class EventFactory { private static final Logger log = LoggerFactory.getLogger(EventFactory.class); private final AccountCache accountCache; private final Provider urlProvider; - private final LabelTypes labelTypes; private final PatchListCache patchListCache; private final SchemaFactory schema; private final PatchSetInfoFactory psInfoFactory; @@ -71,13 +70,11 @@ public class EventFactory { @Inject EventFactory(AccountCache accountCache, @CanonicalWebUrl @Nullable Provider urlProvider, - LabelTypes labelTypes, - final PatchSetInfoFactory psif, + PatchSetInfoFactory psif, PatchListCache patchListCache, SchemaFactory schema, @GerritPersonIdent PersonIdent myIdent) { this.accountCache = accountCache; this.urlProvider = urlProvider; - this.labelTypes = labelTypes; this.patchListCache = patchListCache; this.schema = schema; this.psInfoFactory = psif; @@ -244,25 +241,26 @@ public class EventFactory { a.commitMessage = commitMessage; } - public void addPatchSets(ChangeAttribute a, Collection ps) { - addPatchSets(a, ps, null, false, null); + public void addPatchSets(ChangeAttribute a, Collection ps, + LabelTypes labelTypes) { + addPatchSets(a, ps, null, false, null, labelTypes); } public void addPatchSets(ChangeAttribute ca, Collection ps, - Map> approvals) { - addPatchSets(ca, ps, approvals, false, null); + Map> approvals, + LabelTypes labelTypes) { + addPatchSets(ca, ps, approvals, false, null, labelTypes); } public void addPatchSets(ChangeAttribute ca, Collection ps, - Map> approvals, - boolean includeFiles, Change change) { - + Map> approvals, + boolean includeFiles, Change change, LabelTypes labelTypes) { if (!ps.isEmpty()) { ca.patchSets = new ArrayList(ps.size()); for (PatchSet p : ps) { PatchSetAttribute psa = asPatchSetAttribute(p); if (approvals != null) { - addApprovals(psa, p.getId(), approvals); + addApprovals(psa, p.getId(), approvals, labelTypes); } ca.patchSets.add(psa); if (includeFiles && change != null) { @@ -381,20 +379,21 @@ public class EventFactory { } public void addApprovals(PatchSetAttribute p, PatchSet.Id id, - Map> all) { + Map> all, + LabelTypes labelTypes) { Collection list = all.get(id); if (list != null) { - addApprovals(p, list); + addApprovals(p, list, labelTypes); } } public void addApprovals(PatchSetAttribute p, - Collection list) { + Collection list, LabelTypes labelTypes) { if (!list.isEmpty()) { p.approvals = new ArrayList(list.size()); for (PatchSetApproval a : list) { if (a.getValue() != 0) { - p.approvals.add(asApprovalAttribute(a)); + p.approvals.add(asApprovalAttribute(a, labelTypes)); } } if (p.approvals.isEmpty()) { @@ -451,9 +450,11 @@ public class EventFactory { * serialization to JSON. * * @param approval + * @param labelTypes label types for the containing project * @return object suitable for serialization to JSON */ - public ApprovalAttribute asApprovalAttribute(PatchSetApproval approval) { + public ApprovalAttribute asApprovalAttribute(PatchSetApproval approval, + LabelTypes labelTypes) { ApprovalAttribute a = new ApprovalAttribute(); a.type = approval.getCategoryId().get(); a.value = Short.toString(approval.getValue()); 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 4a16399e70..965d4e15e5 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 @@ -26,9 +26,9 @@ import com.google.common.collect.ArrayListMultimap; import com.google.common.collect.Iterables; import com.google.common.collect.ListMultimap; import com.google.gerrit.common.ChangeHooks; +import com.google.gerrit.common.data.Capable; import com.google.gerrit.common.data.LabelType; import com.google.gerrit.common.data.LabelTypes; -import com.google.gerrit.common.data.Capable; import com.google.gerrit.common.data.SubmitTypeRecord; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.ApprovalCategory; @@ -52,6 +52,7 @@ 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.NoSuchProjectException; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.util.RequestScopePropagator; @@ -130,7 +131,6 @@ public class MergeOp { private final GitReferenceUpdated gitRefUpdated; private final MergedSender.Factory mergedSenderFactory; private final MergeFailSender.Factory mergeFailSenderFactory; - private final LabelTypes labelTypes; private final PatchSetInfoFactory patchSetInfoFactory; private final IdentifiedUser.GenericFactory identifiedUserFactory; private final ChangeControl.GenericFactory changeControlFactory; @@ -182,7 +182,6 @@ public class MergeOp { gitRefUpdated = gru; mergedSenderFactory = msf; mergeFailSenderFactory = mfsf; - this.labelTypes = labelTypes; patchSetInfoFactory = psif; identifiedUserFactory = iuf; this.changeControlFactory = changeControlFactory; @@ -201,7 +200,7 @@ public class MergeOp { commits = new HashMap(); } - public void verifyMergeability(Change change) { + public void verifyMergeability(Change change) throws NoSuchProjectException { try { setDestProject(); openRepository(); @@ -269,7 +268,7 @@ public class MergeOp { } } - public void merge() throws MergeException { + public void merge() throws MergeException, NoSuchProjectException { setDestProject(); try { openSchema(); @@ -389,7 +388,8 @@ public class MergeOp { commits.putAll(strategy.getNewCommits()); } - private SubmitStrategy createStrategy(final SubmitType submitType) throws MergeException { + private SubmitStrategy createStrategy(final SubmitType submitType) + throws MergeException, NoSuchProjectException { return submitStrategyFactory.create(submitType, db, repo, rw, inserter, canMergeFlag, getAlreadyAccepted(branchTip), destBranch, destProject.isUseContentMerge()); @@ -931,12 +931,11 @@ public class MergeOp { c.setStatus(Change.Status.MERGED); final List approvals = db.patchSetApprovals().byChange(changeId).toList(); + final ChangeControl control = changeControlFactory.controlFor(c, + identifiedUserFactory.create(c.getOwner())); final FunctionState fs = functionState.create( - changeControlFactory.controlFor( - c, - identifiedUserFactory.create(c.getOwner())), - merged, approvals); - for (LabelType lt : labelTypes.getLabelTypes()) { + control, merged, approvals); + for (LabelType lt : control.getLabelTypes().getLabelTypes()) { CategoryFunction.forType(lt).run(lt, fs); } for (PatchSetApproval a : approvals) { @@ -987,7 +986,9 @@ public class MergeOp { } try { - final MergedSender cm = mergedSenderFactory.create(c); + final ChangeControl control = changeControlFactory.controlFor(c, + identifiedUserFactory.create(c.getOwner())); + final MergedSender cm = mergedSenderFactory.create(control); if (from != null) { cm.setFrom(from.getAccountId()); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index cf541bb506..820e196f63 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java @@ -18,6 +18,7 @@ import static com.google.gerrit.reviewdb.client.Change.INITIAL_PATCH_SET_ID; import static com.google.gerrit.server.git.MultiProgressMonitor.UNKNOWN; import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromApprovals; import static com.google.gerrit.server.mail.MailUtil.getRecipientsFromFooters; + import static org.eclipse.jgit.lib.Constants.R_HEADS; import static org.eclipse.jgit.transport.ReceiveCommand.Result.NOT_ATTEMPTED; import static org.eclipse.jgit.transport.ReceiveCommand.Result.OK; @@ -42,6 +43,7 @@ import com.google.common.util.concurrent.ListeningExecutorService; import com.google.gerrit.common.ChangeHookRunner.HookResult; import com.google.gerrit.common.ChangeHooks; import com.google.gerrit.common.data.Capable; +import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.Branch; @@ -263,6 +265,7 @@ public class ReceiveCommits { private final ReceivePack rp; private final NoteMap rejectCommits; private MagicBranchInput magicBranch; + private LabelTypes labelTypes; private List newChanges = Collections.emptyList(); private final Map replaceByChange = @@ -1116,6 +1119,7 @@ public class ReceiveCommits { reject(cmd, "cannot upload review"); return; } + labelTypes = projectControl.getLabelTypes(); // Validate that the new commits are connected with the target // branch. If they aren't, we want to abort. We do this check by @@ -1463,7 +1467,7 @@ public class ReceiveCommits { db.patchSets().insert(Collections.singleton(ps)); db.changes().insert(Collections.singleton(change)); ChangeUtil.updateTrackingIds(db, change, trackingFooters, footerLines); - approvalsUtil.addReviewers(db, change, ps, info, + approvalsUtil.addReviewers(db, labelTypes, change, ps, info, recipients.getReviewers(), Collections. emptySet()); db.commit(); } finally { @@ -1769,10 +1773,10 @@ public class ReceiveCommits { } List patchSetApprovals = - approvalsUtil.copyVetosToPatchSet(db, newPatchSet.getId()); + approvalsUtil.copyVetosToPatchSet(db, labelTypes, newPatchSet.getId()); final MailRecipients oldRecipients = getRecipientsFromApprovals(patchSetApprovals); - approvalsUtil.addReviewers(db, change, newPatchSet, info, + approvalsUtil.addReviewers(db, labelTypes, change, newPatchSet, info, recipients.getReviewers(), oldRecipients.getAll()); recipients.add(oldRecipients); @@ -2161,7 +2165,7 @@ public class ReceiveCommits { @Override public void run() { try { - final MergedSender cm = mergedSenderFactory.create(result.change); + final MergedSender cm = mergedSenderFactory.create(result.changeCtl); cm.setFrom(currentUser.getAccountId()); cm.setPatchSet(result.newPatchSet, result.info); cm.send(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmitStrategyFactory.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmitStrategyFactory.java index 093a7583d2..f4cf6c5308 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmitStrategyFactory.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/SubmitStrategyFactory.java @@ -24,6 +24,8 @@ import com.google.gerrit.server.changedetail.RebaseChange; import com.google.gerrit.server.config.CanonicalWebUrl; import com.google.gerrit.server.extensions.events.GitReferenceUpdated; import com.google.gerrit.server.patch.PatchSetInfoFactory; +import com.google.gerrit.server.project.NoSuchProjectException; +import com.google.gerrit.server.project.ProjectControl; import com.google.inject.Inject; import com.google.inject.Provider; @@ -49,9 +51,9 @@ public class SubmitStrategyFactory { private final PersonIdent myIdent; private final PatchSetInfoFactory patchSetInfoFactory; private final Provider urlProvider; - private final LabelTypes labelTypes; private final GitReferenceUpdated gitRefUpdated; private final RebaseChange rebaseChange; + private final ProjectControl.Factory projectFactory; @Inject SubmitStrategyFactory( @@ -59,28 +61,30 @@ public class SubmitStrategyFactory { @GerritPersonIdent final PersonIdent myIdent, final PatchSetInfoFactory patchSetInfoFactory, @CanonicalWebUrl @Nullable final Provider urlProvider, - final LabelTypes labelTypes, final GitReferenceUpdated gitRefUpdated, - final RebaseChange rebaseChange) { + final GitReferenceUpdated gitRefUpdated, final RebaseChange rebaseChange, + final ProjectControl.Factory projectFactory) { this.identifiedUserFactory = identifiedUserFactory; this.myIdent = myIdent; this.patchSetInfoFactory = patchSetInfoFactory; this.urlProvider = urlProvider; - this.labelTypes = labelTypes; this.gitRefUpdated = gitRefUpdated; this.rebaseChange = rebaseChange; + this.projectFactory = projectFactory; } public SubmitStrategy create(final SubmitType submitType, final ReviewDb db, final Repository repo, final RevWalk rw, final ObjectInserter inserter, final RevFlag canMergeFlag, final Set alreadyAccepted, final Branch.NameKey destBranch, final boolean useContentMerge) - throws MergeException { + throws MergeException, NoSuchProjectException { final SubmitStrategy.Arguments args = new SubmitStrategy.Arguments(identifiedUserFactory, myIdent, db, repo, rw, inserter, canMergeFlag, alreadyAccepted, destBranch, useContentMerge); switch (submitType) { case CHERRY_PICK: + LabelTypes labelTypes = + projectFactory.controlFor(destBranch.getParentKey()).getLabelTypes(); return new CherryPick(args, patchSetInfoFactory, urlProvider, labelTypes, gitRefUpdated); case FAST_FORWARD_ONLY: diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java index 42f1dd33f2..2255584226 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java @@ -22,8 +22,8 @@ import com.google.gerrit.common.data.LabelValue; import com.google.gerrit.common.errors.EmailException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountProjectWatch.NotifyType; -import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSetApproval; +import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; import com.google.inject.assistedinject.Assisted; @@ -31,15 +31,15 @@ import com.google.inject.assistedinject.Assisted; /** Send notice about a change successfully merged. */ public class MergedSender extends ReplyToChangeSender { public static interface Factory { - public MergedSender create(Change change); + public MergedSender create(ChangeControl change); } private final LabelTypes labelTypes; @Inject - public MergedSender(EmailArguments ea, LabelTypes lt, @Assisted Change c) { - super(ea, c, "merged"); - labelTypes = lt; + public MergedSender(EmailArguments ea, @Assisted ChangeControl c) { + super(ea, c.getChange(), "merged"); + labelTypes = c.getLabelTypes(); } @Override diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java index 6064560947..d0185dd8c0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java @@ -14,6 +14,7 @@ package com.google.gerrit.server.project; +import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.PermissionRange; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.common.data.SubmitTypeRecord; @@ -211,6 +212,11 @@ public class ChangeControl { && getRefControl().canUpload(); // as long as you can upload too } + /** All available label types for this project. */ + public LabelTypes getLabelTypes() { + return getProjectControl().getLabelTypes(); + } + /** All value ranges of any allowed label permission. */ public List getLabelRanges() { return getRefControl().getLabelRanges(); 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 8f8d3152c6..9cfdddd439 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 @@ -20,6 +20,7 @@ import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.Capable; import com.google.gerrit.common.data.ContributorAgreement; import com.google.gerrit.common.data.GroupReference; +import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.common.data.PermissionRule.Action; @@ -181,6 +182,10 @@ public class ProjectControl { return state.getProject(); } + public LabelTypes getLabelTypes() { + return state.getLabelTypes(); + } + private boolean isHidden() { return getProject().getState().equals(Project.State.HIDDEN); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java index e42be92fc4..42c69f2e7b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectState.java @@ -20,6 +20,7 @@ import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.gerrit.common.data.AccessSection; import com.google.gerrit.common.data.GroupReference; +import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRule; import com.google.gerrit.reviewdb.client.AccountGroup; @@ -65,6 +66,7 @@ public class ProjectState { private final PrologEnvironment.Factory envFactory; private final GitRepositoryManager gitMgr; private final RulesCache rulesCache; + private final LabelTypes labelTypes; private final ProjectConfig config; private final Set localOwners; @@ -89,6 +91,7 @@ public class ProjectState { final PrologEnvironment.Factory envFactory, final GitRepositoryManager gitMgr, final RulesCache rulesCache, + final LabelTypes labelTypes, @Assisted final ProjectConfig config) { this.projectCache = projectCache; this.isAllProjects = config.getProject().getNameKey().equals(allProjectsName); @@ -101,6 +104,7 @@ public class ProjectState { this.capabilities = isAllProjects ? new CapabilityCollection(config.getAccessSection(AccessSection.GLOBAL_CAPABILITIES)) : null; + this.labelTypes = labelTypes; if (isAllProjects && !Permission.canBeOnAllProjects(AccessSection.ALL, Permission.OWNER)) { localOwners = Collections.emptySet(); @@ -337,6 +341,10 @@ public class ProjectState { }); } + public LabelTypes getLabelTypes() { + return labelTypes; + } + private boolean getInheritableBoolean(Function func) { for (ProjectState s : tree()) { switch (func.apply(s.getProject())) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java index e80fa331e8..7bbb073a6c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java @@ -15,7 +15,6 @@ package com.google.gerrit.server.query.change; import com.google.common.collect.Lists; -import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.GroupReference; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.AccountGroup; @@ -110,7 +109,6 @@ public class ChangeQueryBuilder extends QueryBuilder { final ChangeControl.GenericFactory changeControlGenericFactory; final AccountResolver accountResolver; final GroupBackend groupBackend; - final LabelTypes labelTypes; final AllProjectsName allProjectsName; final PatchListCache patchListCache; final GitRepositoryManager repoManager; @@ -124,7 +122,6 @@ public class ChangeQueryBuilder extends QueryBuilder { ChangeControl.GenericFactory changeControlGenericFactory, AccountResolver accountResolver, GroupBackend groupBackend, - LabelTypes labelTypes, AllProjectsName allProjectsName, PatchListCache patchListCache, GitRepositoryManager repoManager, @@ -136,7 +133,6 @@ public class ChangeQueryBuilder extends QueryBuilder { this.changeControlGenericFactory = changeControlGenericFactory; this.accountResolver = accountResolver; this.groupBackend = groupBackend; - this.labelTypes = labelTypes; this.allProjectsName = allProjectsName; this.patchListCache = patchListCache; this.repoManager = repoManager; @@ -301,8 +297,9 @@ public class ChangeQueryBuilder extends QueryBuilder { @Operator public Predicate label(String name) { - return new LabelPredicate(args.changeControlGenericFactory, - args.userFactory, args.dbProvider, args.labelTypes, name); + return new LabelPredicate(args.projectCache, + args.changeControlGenericFactory, args.userFactory, args.dbProvider, + name); } @Operator diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java index 761aba231a..e6251bc1a6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeQueryRewriter.java @@ -39,7 +39,7 @@ public class ChangeQueryRewriter extends QueryRewriter { new ChangeQueryBuilder.Arguments( // new InvalidProvider(), // new InvalidProvider(), // - null, null, null, null, null, null, // + null, null, null, null, null, // null, null, null, null), null)); private final Provider dbProvider; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java index 4fc48a5d6f..e1bcb559b8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/LabelPredicate.java @@ -24,6 +24,8 @@ import com.google.gerrit.reviewdb.server.ReviewDb; import com.google.gerrit.server.IdentifiedUser; 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.query.OperatorPredicate; import com.google.gwtorm.server.OrmException; import com.google.inject.Provider; @@ -103,52 +105,66 @@ class LabelPredicate extends OperatorPredicate { return Integer.parseInt(value); } + private final ProjectCache projectCache; private final ChangeControl.GenericFactory ccFactory; private final IdentifiedUser.GenericFactory userFactory; private final Provider dbProvider; private final Test test; - private final LabelType type; - private final String permissionName; + private final String type; private final int expVal; - LabelPredicate(ChangeControl.GenericFactory ccFactory, - IdentifiedUser.GenericFactory userFactory, Provider dbProvider, - LabelTypes types, String value) { + LabelPredicate(ProjectCache projectCache, + ChangeControl.GenericFactory ccFactory, + IdentifiedUser.GenericFactory userFactory, + Provider dbProvider, + String value) { super(ChangeQueryBuilder.FIELD_LABEL, value); this.ccFactory = ccFactory; + this.projectCache = projectCache; this.userFactory = userFactory; this.dbProvider = dbProvider; Matcher m1 = Pattern.compile("(=|>=|<=)([+-]?\\d+)$").matcher(value); Matcher m2 = Pattern.compile("([+-]\\d+)$").matcher(value); if (m1.find()) { - type = type(types, value.substring(0, m1.start())); + type = value.substring(0, m1.start()); test = op(m1.group(1)); expVal = value(m1.group(2)); } else if (m2.find()) { - type = type(types, value.substring(0, m2.start())); + type = value.substring(0, m2.start()); test = Test.EQ; expVal = value(m2.group(1)); } else { - type = type(types, value); + type = value; test = Test.EQ; expVal = 1; } - - this.permissionName = Permission.forLabel(type.getName()); } @Override public boolean match(final ChangeData object) throws OrmException { + final Change c = object.change(dbProvider); + if (c == null) { + // The change has disappeared. + // + return false; + } + final ProjectState project = projectCache.get(c.getDest().getParentKey()); + if (project == null) { + // The project has disappeared. + // + return false; + } + final LabelType labelType = type(project.getLabelTypes(), type); final Set allApprovers = new HashSet(); final Set approversThatVotedInCategory = new HashSet(); for (PatchSetApproval p : object.currentApprovals(dbProvider)) { allApprovers.add(p.getAccountId()); - if (p.getCategoryId().get().equals(type.getId())) { + if (p.getCategoryId().get().equals(labelType.getId())) { approversThatVotedInCategory.add(p.getAccountId()); - if (match(object.change(dbProvider), p.getValue(), p.getAccountId())) { + if (match(c, p.getValue(), p.getAccountId(), labelType)) { return true; } } @@ -156,8 +172,8 @@ class LabelPredicate extends OperatorPredicate { final Set approversThatDidNotVoteInCategory = new HashSet(allApprovers); approversThatDidNotVoteInCategory.removeAll(approversThatVotedInCategory); - for (final Account.Id a : approversThatDidNotVoteInCategory) { - if (match(object.change(dbProvider), 0, a)) { + for (Account.Id a : approversThatDidNotVoteInCategory) { + if (match(c, 0, a, labelType)) { return true; } } @@ -166,7 +182,8 @@ class LabelPredicate extends OperatorPredicate { } private boolean match(final Change change, final int value, - final Account.Id approver) throws OrmException { + final Account.Id approver, final LabelType type) + throws OrmException { int psVal = value; if (test.match(psVal, expVal)) { // Double check the value is still permitted for the user. @@ -179,7 +196,7 @@ class LabelPredicate extends OperatorPredicate { // return false; } - psVal = cc.getRange(permissionName).squash(psVal); + psVal = cc.getRange(Permission.forLabel(type.getName())).squash(psVal); } catch (NoSuchChangeException e) { // The project has disappeared. // diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java index 8ca0548b14..c781d97913 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/QueryProcessor.java @@ -15,6 +15,7 @@ package com.google.gerrit.server.query.change; import com.google.gerrit.common.data.GlobalCapability; +import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.SubmitRecord; import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.PatchSet; @@ -25,6 +26,8 @@ import com.google.gerrit.server.events.EventFactory; import com.google.gerrit.server.events.PatchSetAttribute; import com.google.gerrit.server.events.QueryStats; import com.google.gerrit.server.git.GitRepositoryManager; +import com.google.gerrit.server.project.ChangeControl; +import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.query.Predicate; import com.google.gerrit.server.query.QueryParseException; import com.google.gson.Gson; @@ -93,6 +96,7 @@ public class QueryProcessor { private final ChangeQueryRewriter queryRewriter; private final Provider db; private final GitRepositoryManager repoManager; + private final ChangeControl.Factory changeControlFactory; private final int maxLimit; private OutputFormat outputFormat = OutputFormat.TEXT; @@ -116,12 +120,14 @@ public class QueryProcessor { QueryProcessor(EventFactory eventFactory, ChangeQueryBuilder.Factory queryBuilder, CurrentUser currentUser, ChangeQueryRewriter queryRewriter, Provider db, - GitRepositoryManager repoManager) { + GitRepositoryManager repoManager, + ChangeControl.Factory changeControlFactory) { this.eventFactory = eventFactory; this.queryBuilder = queryBuilder.create(currentUser); this.queryRewriter = queryRewriter; this.db = db; this.repoManager = repoManager; + this.changeControlFactory = changeControlFactory; this.maxLimit = currentUser.getCapabilities() .getRange(GlobalCapability.QUERY_LIMIT) .getMax(); @@ -267,6 +273,8 @@ public class QueryProcessor { List results = queryChanges(queryString); ChangeAttribute c = null; for (ChangeData d : results) { + LabelTypes labelTypes = changeControlFactory.controlFor(d.getChange()) + .getLabelTypes(); c = eventFactory.asChangeAttribute(d.getChange()); eventFactory.extend(c, d.getChange()); eventFactory.addTrackingIds(c, d.trackingIds(db)); @@ -287,10 +295,11 @@ public class QueryProcessor { if (includeFiles) { eventFactory.addPatchSets(c, d.patches(db), includeApprovals ? d.approvalsMap(db).asMap() : null, - includeFiles, d.change(db)); + includeFiles, d.change(db), labelTypes); } else { eventFactory.addPatchSets(c, d.patches(db), - includeApprovals ? d.approvalsMap(db).asMap() : null); + includeApprovals ? d.approvalsMap(db).asMap() : null, + labelTypes); } } @@ -298,8 +307,8 @@ public class QueryProcessor { PatchSet current = d.currentPatchSet(db); if (current != null) { c.currentPatchSet = eventFactory.asPatchSetAttribute(current); - eventFactory.addApprovals(c.currentPatchSet, // - d.currentApprovals(db)); + eventFactory.addApprovals(c.currentPatchSet, + d.currentApprovals(db), labelTypes); if (includeFiles) { eventFactory.addPatchSetFileNames(c.currentPatchSet, @@ -342,6 +351,11 @@ public class QueryProcessor { ErrorMessage m = new ErrorMessage(); m.message = e.getMessage(); show(m); + } catch (NoSuchChangeException e) { + log.error("Missing change: " + e.getMessage(), e); + ErrorMessage m = new ErrorMessage(); + m.message = "missing change " + e.getMessage(); + show(m); } } finally { try { 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 a7a542230f..6b9cb93099 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 @@ -15,7 +15,6 @@ package com.google.gerrit.server.workflow; import com.google.gerrit.common.data.LabelType; -import com.google.gerrit.common.data.LabelTypes; import com.google.gerrit.common.data.LabelValue; import com.google.gerrit.common.data.Permission; import com.google.gerrit.common.data.PermissionRange; @@ -42,7 +41,6 @@ public class FunctionState { Collection all); } - private final LabelTypes labelTypes; private final IdentifiedUser.GenericFactory userFactory; private final Map> approvals = @@ -52,11 +50,9 @@ public class FunctionState { private final Change change; @Inject - FunctionState(final LabelTypes labelTypes, - final IdentifiedUser.GenericFactory userFactory, + FunctionState(final IdentifiedUser.GenericFactory userFactory, @Assisted final ChangeControl c, @Assisted final PatchSet.Id psId, @Assisted final Collection all) { - this.labelTypes = labelTypes; this.userFactory = userFactory; callerChangeControl = c; @@ -68,7 +64,7 @@ public class FunctionState { approvals.get(ca.getCategoryId().get()); if (l == null) { l = new ArrayList(); - LabelType lt = labelTypes.byId(ca.getCategoryId().get()); + LabelType lt = c.getLabelTypes().byId(ca.getCategoryId().get()); if (lt != null) { // TODO: Support arbitrary labels approvals.put(lt.getName(), l); @@ -80,7 +76,7 @@ public class FunctionState { } List getLabelTypes() { - return labelTypes.getLabelTypes(); + return callerChangeControl.getLabelTypes().getLabelTypes(); } Change getChange() { diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java index 51ea5a2725..f818746c15 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java @@ -541,10 +541,10 @@ public class RefControlTest extends TestCase { RulesCache rulesCache = null; all.put(local.getProject().getNameKey(), new ProjectState( projectCache, allProjectsName, projectControlFactory, - envFactory, mgr, rulesCache, local)); + envFactory, mgr, rulesCache, null, local)); all.put(parent.getProject().getNameKey(), new ProjectState( projectCache, allProjectsName, projectControlFactory, - envFactory, mgr, rulesCache, parent)); + envFactory, mgr, rulesCache, null, parent)); return all.get(local.getProject().getNameKey()); }