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
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 14aa2e3..e68b29c 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 @@
db.patchSetApprovals().insert(getForPatchSet(db, ctl, ps));
}
+ Iterable<PatchSetApproval> getForPatchSet(ReviewDb db,
+ ChangeControl ctl, PatchSet.Id psId) throws OrmException {
+ return getForPatchSet(db, ctl, db.patchSets().get(psId));
+ }
+
private Iterable<PatchSetApproval> 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 b648548..64b5169 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.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 @@
}
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 @@
return notes.load().getApprovals();
}
- public List<PatchSetApproval> byPatchSet(ReviewDb db, ChangeNotes notes,
+ public Iterable<PatchSetApproval> 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<PatchSetApproval> byPatchSetUser(ReviewDb db,
- ChangeNotes notes, PatchSet.Id psId, Account.Id accountId)
+ public Iterable<PatchSetApproval> 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 @@
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 4a7cbdb..7ac90fd 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.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 @@
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 25c63b5..0a2ccef 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 @@
Map<String, PatchSetApproval> 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 7fde39c..6fd9002 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 @@
indexer.indexAsync(rsrc.getChange().getId());
result.reviewers = Lists.newArrayListWithCapacity(added.size());
for (PatchSetApproval psa : added) {
+ // New reviewers have value 0, don't bother normalizing.
result.reviewers.add(json.format(
new ReviewerInfo(psa.getAccountId()),
reviewers.get(psa.getAccountId()),
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 5008d02..22ddd2d 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
@@ -29,7 +29,6 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.account.AccountInfo;
-import com.google.gerrit.server.git.LabelNormalizer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.query.change.ChangeData;
@@ -46,19 +45,16 @@
private final Provider<ReviewDb> db;
private final ChangeData.Factory changeDataFactory;
private final ApprovalsUtil approvalsUtil;
- private final LabelNormalizer labelNormalizer;
private final AccountInfo.Loader.Factory accountLoaderFactory;
@Inject
ReviewerJson(Provider<ReviewDb> 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 @@
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<PatchSetApproval> approvals) throws OrmException {
+ Iterable<PatchSetApproval> approvals) throws OrmException {
LabelTypes labelTypes = ctl.getLabelTypes();
// Don't use Maps.newTreeMap(Comparator) due to OpenJDK bug 100167.
out.approvals = new TreeMap<String,String>(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 f585f16..d7f12ca 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 @@
ChangeUpdate update, IdentifiedUser caller, Timestamp timestamp)
throws OrmException {
PatchSet.Id psId = rsrc.getPatchSet().getId();
- List<PatchSetApproval> approvals =
- approvalsUtil.byPatchSet(dbProvider.get(), rsrc.getNotes(), psId);
-
- Map<PatchSetApproval.Key, PatchSetApproval> byKey =
- Maps.newHashMapWithExpectedSize(approvals.size());
- for (PatchSetApproval psa : approvals) {
+ Map<PatchSetApproval.Key, PatchSetApproval> 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 b9624fe..9b74ee5 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 @@
return "Verified".equalsIgnoreCase(id.get());
}
- private List<PatchSetApproval> safeGetApprovals(CodeReviewCommit n) {
+ private Iterable<PatchSetApproval> 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 18df4c1..6d15ed3 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 @@
final List<PatchSetApproval> 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 a97b3fd..5fabb06 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 @@
List<PatchSetApproval> 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 07a5f9a..5cb1ba1 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 @@
Table<Account.Id, String, PatchSetApproval> pos = HashBasedTable.create();
Table<Account.Id, String, PatchSetApproval> 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 333c343..0e976c8 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 @@
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 @@
} 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 e81c061..ba33b97 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.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 @@
.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 b544447..6170241 160000
--- a/plugins/reviewnotes
+++ b/plugins/reviewnotes
@@ -1 +1 @@
-Subproject commit b544447649d9ee3b3f78a6a1a7f839cb6a361292
+Subproject commit 61702414c046dd6b811c9137b765f9db422f83db
