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

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<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);

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<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,
+  public Iterable<PatchSetApproval> byPatchSetUser(ReviewDb db,
-      ChangeNotes notes, PatchSet.Id psId, Account.Id accountId)
+      ChangeControl ctl, PatchSet.Id psId, Account.Id accountId)
       throws OrmException {
     if (!migration.readPatchSetApprovals()) {
       return sortApprovals(
           db.patchSetApprovals().byPatchSetUser(psId, accountId));
     }
-    return ImmutableList.copyOf(
+    return filterApprovals(byPatchSet(db, ctl, psId), accountId);
-        filterApprovals(byPatchSet(db, notes, 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;
     }

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;

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java

@@ -469,7 +469,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
     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;

gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java

@@ -239,6 +239,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
         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()),

gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerJson.java

@@ -29,7 +29,6 @@ import com.google.gerrit.reviewdb.client.PatchSetApproval;
 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 @@ public class ReviewerJson {
   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 @@ 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<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 :
+    for (PatchSetApproval ca : approvals) {
-        labelNormalizer.normalize(ctl, approvals).getNormalized()) {
       for (PermissionRange pr : ctl.getLabelRanges()) {
         if (!pr.isEmpty()) {
           LabelType at = labelTypes.byLabel(ca.getLabelId());

gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java

@@ -257,12 +257,9 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
       ChangeUpdate update, IdentifiedUser caller, Timestamp timestamp)
       throws OrmException {
     PatchSet.Id psId = rsrc.getPatchSet().getId();
-    List<PatchSetApproval> approvals =
+    Map<PatchSetApproval.Key, PatchSetApproval> byKey = Maps.newHashMap();
-        approvalsUtil.byPatchSet(dbProvider.get(), rsrc.getNotes(), psId);
+    for (PatchSetApproval psa :
-
+        approvalsUtil.byPatchSet(dbProvider.get(), rsrc.getControl(), psId)) {
-    Map<PatchSetApproval.Key, PatchSetApproval> byKey =
-        Maps.newHashMapWithExpectedSize(approvals.size());
-    for (PatchSetApproval psa : approvals) {
       if (!byKey.containsKey(psa.getKey())) {
         byKey.put(psa.getKey(), psa);
       }

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<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();

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/CherryPick.java

@@ -179,7 +179,7 @@ public class CherryPick extends SubmitStrategy {
 
       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);

gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/RebaseIfNecessary.java

@@ -96,7 +96,7 @@ public class RebaseIfNecessary extends SubmitStrategy {
 
             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,

gerrit-server/src/main/java/com/google/gerrit/server/mail/MergedSender.java

@@ -62,7 +62,7 @@ public class MergedSender extends ReplyToChangeSender {
       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;

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,
+  public ChangeControl changeControl() throws OrmException {
-      OrmException {
     if (changeControl == null) {
       Change c = change();
-      changeControl =
+      try {
-          changeControlFactory.controlFor(c, userFactory.create(c.getOwner()));
+        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(
+        currentApprovals = ImmutableList.copyOf(approvalsUtil.byPatchSet(
-            db, notes(), c.currentPatchSetId());
+            db, changeControl(), c.currentPatchSetId()));
       }
     }
     return currentApprovals;

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);
       }
     });
 

plugins/reviewnotes

@@ -1 +1 @@
-Subproject commit b544447649d9ee3b3f78a6a1a7f839cb6a361292
+Subproject commit 61702414c046dd6b811c9137b765f9db422f83db