From 8e36bbdcd729077caccf3ef35e714c7987a85f5a Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Thu, 10 Apr 2014 13:01:30 -0400 Subject: [PATCH] Copy labels dynamically in ApprovalsUtil.byPatchSet The intent is to use this method from ChangeJson to dynamically copy labels every time the change detail is loaded, thus not relying on one-time copying of labels in the notedb read path. To do so requires a bit of refactoring, since the ChangeControl now needs to be passed into this method. Change-Id: I8bc25d32a91218e074517dcacf4bf3a77d4ae957 --- .../google/gerrit/server/ApprovalCopier.java | 5 +++++ .../google/gerrit/server/ApprovalsUtil.java | 20 +++++++++++-------- .../gerrit/server/change/ChangeJson.java | 3 +-- .../gerrit/server/change/PostReview.java | 2 +- .../gerrit/server/change/PostReviewers.java | 1 + .../gerrit/server/change/ReviewerJson.java | 11 +++------- .../google/gerrit/server/change/Submit.java | 9 +++------ .../google/gerrit/server/git/MergeUtil.java | 4 ++-- .../server/git/strategy/CherryPick.java | 2 +- .../git/strategy/RebaseIfNecessary.java | 2 +- .../gerrit/server/mail/MergedSender.java | 2 +- .../server/query/change/ChangeData.java | 15 ++++++++------ .../google/gerrit/server/project/Util.java | 3 +++ plugins/reviewnotes | 2 +- 14 files changed, 44 insertions(+), 37 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java index 14aa2e3fe4..e68b29c470 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalCopier.java @@ -81,6 +81,11 @@ public class ApprovalCopier { db.patchSetApprovals().insert(getForPatchSet(db, ctl, ps)); } + Iterable getForPatchSet(ReviewDb db, + ChangeControl ctl, PatchSet.Id psId) throws OrmException { + return getForPatchSet(db, ctl, db.patchSets().get(psId)); + } + private Iterable getForPatchSet(ReviewDb db, ChangeControl ctl, PatchSet ps) throws OrmException { ChangeData cd = changeDataFactory.create(db, ctl); 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 b6485489b3..64b51690e7 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 @@ -43,6 +43,7 @@ import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.NotesMigration; import com.google.gerrit.server.notedb.ReviewerState; +import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.util.TimeUtil; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -90,11 +91,14 @@ public class ApprovalsUtil { } private final NotesMigration migration; + private final ApprovalCopier copier; @VisibleForTesting @Inject - public ApprovalsUtil(NotesMigration migration) { + public ApprovalsUtil(NotesMigration migration, + ApprovalCopier copier) { this.migration = migration; + this.copier = copier; } /** @@ -225,23 +229,22 @@ public class ApprovalsUtil { return notes.load().getApprovals(); } - public List byPatchSet(ReviewDb db, ChangeNotes notes, + public Iterable byPatchSet(ReviewDb db, ChangeControl ctl, PatchSet.Id psId) throws OrmException { if (!migration.readPatchSetApprovals()) { return sortApprovals(db.patchSetApprovals().byPatchSet(psId)); } - return notes.load().getApprovals().get(psId); + return copier.getForPatchSet(db, ctl, psId); } - public List byPatchSetUser(ReviewDb db, - ChangeNotes notes, PatchSet.Id psId, Account.Id accountId) + public Iterable byPatchSetUser(ReviewDb db, + ChangeControl ctl, PatchSet.Id psId, Account.Id accountId) throws OrmException { if (!migration.readPatchSetApprovals()) { return sortApprovals( db.patchSetApprovals().byPatchSetUser(psId, accountId)); } - return ImmutableList.copyOf( - filterApprovals(byPatchSet(db, notes, psId), accountId)); + return filterApprovals(byPatchSet(db, ctl, psId), accountId); } public PatchSetApproval getSubmitter(ReviewDb db, ChangeNotes notes, @@ -250,7 +253,8 @@ public class ApprovalsUtil { return null; } try { - return getSubmitter(c, byPatchSet(db, notes, c)); + // Submit approval is never copied, so bypass expensive byPatchSet call. + return getSubmitter(c, byChange(db, notes).get(c)); } catch (OrmException e) { return null; } 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 4a7cbdbbdc..7ac90fd0d2 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 @@ -88,7 +88,6 @@ import com.google.gerrit.server.patch.PatchListNotAvailableException; 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.ProjectControl; import com.google.gerrit.server.query.change.ChangeData; @@ -370,7 +369,7 @@ public class ChangeJson { ctrl = projectControls.get(cd.change().getProject()) .controlFor(cd.change()); } - } catch (NoSuchChangeException | ExecutionException e) { + } catch (ExecutionException e) { throw new OrmException(e); } lastControl = ctrl; 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 25c63b5d11..0a2ccef1bc 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 @@ -469,7 +469,7 @@ public class PostReview implements RestModifyView Map current = Maps.newHashMap(); for (PatchSetApproval a : approvalsUtil.byPatchSetUser( - db.get(), rsrc.getNotes(), rsrc.getPatchSet().getId(), + db.get(), rsrc.getControl(), rsrc.getPatchSet().getId(), rsrc.getAccountId())) { if (a.isSubmit()) { continue; 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 7fde39c9eb..6fd90025ab 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 @@ -239,6 +239,7 @@ public class PostReviewers implements RestModifyView db; private final ChangeData.Factory changeDataFactory; private final ApprovalsUtil approvalsUtil; - private final LabelNormalizer labelNormalizer; private final AccountInfo.Loader.Factory accountLoaderFactory; @Inject ReviewerJson(Provider db, ChangeData.Factory changeDataFactory, ApprovalsUtil approvalsUtil, - LabelNormalizer labelNormalizer, AccountInfo.Loader.Factory accountLoaderFactory) { this.db = db; this.changeDataFactory = changeDataFactory; this.approvalsUtil = approvalsUtil; - this.labelNormalizer = labelNormalizer; this.accountLoaderFactory = accountLoaderFactory; } @@ -86,17 +82,16 @@ public class ReviewerJson { ChangeNotes changeNotes) throws OrmException { PatchSet.Id psId = ctl.getChange().currentPatchSetId(); return format(out, ctl, - approvalsUtil.byPatchSetUser(db.get(), changeNotes, psId, out._id)); + approvalsUtil.byPatchSetUser(db.get(), ctl, psId, out._id)); } public ReviewerInfo format(ReviewerInfo out, ChangeControl ctl, - List approvals) throws OrmException { + Iterable approvals) throws OrmException { LabelTypes labelTypes = ctl.getLabelTypes(); // Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167. out.approvals = new TreeMap(labelTypes.nameComparator()); - for (PatchSetApproval ca : - labelNormalizer.normalize(ctl, approvals).getNormalized()) { + for (PatchSetApproval ca : approvals) { for (PermissionRange pr : ctl.getLabelRanges()) { if (!pr.isEmpty()) { LabelType at = labelTypes.byLabel(ca.getLabelId()); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java index f585f16d9b..d7f12ca0bf 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java @@ -257,12 +257,9 @@ public class Submit implements RestModifyView, ChangeUpdate update, IdentifiedUser caller, Timestamp timestamp) throws OrmException { PatchSet.Id psId = rsrc.getPatchSet().getId(); - List approvals = - approvalsUtil.byPatchSet(dbProvider.get(), rsrc.getNotes(), psId); - - Map byKey = - Maps.newHashMapWithExpectedSize(approvals.size()); - for (PatchSetApproval psa : approvals) { + Map byKey = Maps.newHashMap(); + for (PatchSetApproval psa : + approvalsUtil.byPatchSet(dbProvider.get(), rsrc.getControl(), psId)) { if (!byKey.containsKey(psa.getKey())) { byKey.put(psa.getKey(), psa); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java index b9624fea26..9b74ee58a8 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeUtil.java @@ -311,9 +311,9 @@ public class MergeUtil { return "Verified".equalsIgnoreCase(id.get()); } - private List safeGetApprovals(CodeReviewCommit n) { + private Iterable safeGetApprovals(CodeReviewCommit n) { try { - return approvalsUtil.byPatchSet(db.get(), n.notes(), n.getPatchsetId()); + return approvalsUtil.byPatchSet(db.get(), n.getControl(), n.getPatchsetId()); } catch (OrmException e) { log.error("Can't read approval records for " + n.getPatchsetId(), e); return Collections.emptyList(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java index 18df4c140e..6d15ed3c2b 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java @@ -179,7 +179,7 @@ public class CherryPick extends SubmitStrategy { final List approvals = Lists.newArrayList(); for (PatchSetApproval a - : args.approvalsUtil.byPatchSet(args.db, n.notes(), n.getPatchsetId())) { + : args.approvalsUtil.byPatchSet(args.db, n.getControl(), n.getPatchsetId())) { approvals.add(new PatchSetApproval(ps.getId(), a)); } args.db.patchSetApprovals().insert(approvals); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java index a97b3fde2e..5fabb06a20 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java @@ -96,7 +96,7 @@ public class RebaseIfNecessary extends SubmitStrategy { List approvals = Lists.newArrayList(); for (PatchSetApproval a : args.approvalsUtil.byPatchSet( - args.db, n.notes(), n.getPatchsetId())) { + args.db, n.getControl(), n.getPatchsetId())) { approvals.add(new PatchSetApproval(newPatchSet.getId(), a)); } // rebaseChange.rebase() may already have copied some approvals, 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 07a5f9ae64..5cb1ba1d9e 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 @@ -62,7 +62,7 @@ public class MergedSender extends ReplyToChangeSender { Table pos = HashBasedTable.create(); Table neg = HashBasedTable.create(); for (PatchSetApproval ca : args.approvalsUtil.byPatchSet( - args.db.get(), changeData.notes(), patchSet.getId())) { + args.db.get(), changeData.changeControl(), patchSet.getId())) { LabelType lt = labelTypes.byLabel(ca.getLabelId()); if (lt == null) { continue; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java index 333c343c60..0e976c8659 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/query/change/ChangeData.java @@ -343,12 +343,15 @@ public class ChangeData { return changeControl != null; } - public ChangeControl changeControl() throws NoSuchChangeException, - OrmException { + public ChangeControl changeControl() throws OrmException { if (changeControl == null) { Change c = change(); - changeControl = - changeControlFactory.controlFor(c, userFactory.create(c.getOwner())); + try { + changeControl = + changeControlFactory.controlFor(c, userFactory.create(c.getOwner())); + } catch (NoSuchChangeException e) { + throw new OrmException(e); + } } return changeControl; } @@ -397,8 +400,8 @@ public class ChangeData { } else if (allApprovals != null) { return allApprovals.get(c.currentPatchSetId()); } else { - currentApprovals = approvalsUtil.byPatchSet( - db, notes(), c.currentPatchSetId()); + currentApprovals = ImmutableList.copyOf(approvalsUtil.byPatchSet( + db, changeControl(), c.currentPatchSetId())); } } return currentApprovals; diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java index e81c061723..ba33b97f0a 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java +++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java @@ -38,6 +38,8 @@ import com.google.gerrit.server.account.CapabilityControl; import com.google.gerrit.server.account.GroupBackend; import com.google.gerrit.server.account.GroupMembership; import com.google.gerrit.server.account.ListGroupMembership; +import com.google.gerrit.server.change.ChangeKindCache; +import com.google.gerrit.server.change.ChangeKindCacheImpl; import com.google.gerrit.server.config.AllProjectsName; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.config.AnonymousCowardNameProvider; @@ -222,6 +224,7 @@ public class Util { .toProvider(CanonicalWebUrlProvider.class); bind(String.class).annotatedWith(AnonymousCowardName.class) .toProvider(AnonymousCowardNameProvider.class); + bind(ChangeKindCache.class).to(ChangeKindCacheImpl.NoCache.class); } }); diff --git a/plugins/reviewnotes b/plugins/reviewnotes index b544447649..61702414c0 160000 --- a/plugins/reviewnotes +++ b/plugins/reviewnotes @@ -1 +1 @@ -Subproject commit b544447649d9ee3b3f78a6a1a7f839cb6a361292 +Subproject commit 61702414c046dd6b811c9137b765f9db422f83db