Inject ChangeNotes into ChangeControl

Almost all of the places that will need a ChangeNotes (when we start
reading from that class) should already have a ChangeControl, so this
is a convenient way to ship it around.

To deal with the migration, do not eagerly load ChangeNotes from the
constructor. We could lazily load on e.g. getApprovals(), but that
would require catching OrmException in more places.

Change-Id: I545b555e8369f7f0f775b5188079ea3e93ed9cef

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java

@@ -78,21 +78,8 @@ public class ChangeNotes extends VersionedMetaData {
       this.repoManager = repoManager;
     }
 
-    // TODO(dborowitz): Wrap fewer exceptions if/when we kill gwtorm.
-    public ChangeNotes load(Change change) throws OrmException {
-      Repository repo;
-      try {
-        repo = repoManager.openRepository(change.getProject());
-      } catch (IOException e) {
-        throw new OrmException(e);
-      }
-      try {
-        return new ChangeNotes(repo, change);
-      } catch (ConfigInvalidException | IOException e) {
-        throw new OrmException(e);
-      } finally {
-        repo.close();
-      }
+    public ChangeNotes create(Change change) {
+      return new ChangeNotes(repoManager, change);
     }
   }
 
@@ -241,15 +228,41 @@ public class ChangeNotes extends VersionedMetaData {
     }
   }
 
+  private final GitRepositoryManager repoManager;
   private final Change change;
+  private boolean loaded;
   private ImmutableListMultimap<PatchSet.Id, PatchSetApproval> approvals;
   private ImmutableSetMultimap<ReviewerState, Account.Id> reviewers;
 
   @VisibleForTesting
-  ChangeNotes(Repository repo, Change change)
-      throws ConfigInvalidException, IOException {
+  ChangeNotes(GitRepositoryManager repoManager, Change change) {
+    this.repoManager = repoManager;
     this.change = change;
-    load(repo);
+  }
+
+  // TODO(dborowitz): Wrap fewer exceptions if/when we kill gwtorm.
+  public ChangeNotes load() throws OrmException {
+    if (!loaded) {
+      Repository repo;
+      try {
+        repo = repoManager.openRepository(change.getProject());
+      } catch (IOException e) {
+        throw new OrmException(e);
+      }
+      try {
+        load(repo);
+        loaded = true;
+      } catch (ConfigInvalidException | IOException e) {
+        throw new OrmException(e);
+      } finally {
+        repo.close();
+      }
+    }
+    return this;
+  }
+
+  public Change getChange() {
+    return change;
   }
 
   public ImmutableListMultimap<PatchSet.Id, PatchSetApproval> getApprovals() {

gerrit-server/src/main/java/com/google/gerrit/server/project/ChangeControl.java

@@ -31,6 +31,7 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.server.ApprovalsUtil;
 import com.google.gerrit.server.CurrentUser;
 import com.google.gerrit.server.IdentifiedUser;
+import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.query.change.ChangeData;
 import com.google.gwtorm.server.OrmException;
 import com.google.inject.Inject;
@@ -171,6 +172,7 @@ public class ChangeControl {
 
   interface AssistedFactory {
     ChangeControl create(RefControl refControl, Change change);
+    ChangeControl create(RefControl refControl, ChangeNotes notes);
   }
 
   /**
@@ -189,23 +191,34 @@ public class ChangeControl {
   private final ApprovalsUtil approvalsUtil;
   private final ChangeData.Factory changeDataFactory;
   private final RefControl refControl;
-  private final Change change;
+  private final ChangeNotes notes;
+
+  @AssistedInject
+  ChangeControl(
+      ApprovalsUtil approvalsUtil,
+      ChangeData.Factory changeDataFactory,
+      ChangeNotes.Factory notesFactory,
+      @Assisted RefControl refControl,
+      @Assisted Change change) {
+    this(approvalsUtil, changeDataFactory, refControl,
+        notesFactory.create(change));
+  }
 
   @AssistedInject
   ChangeControl(
       ApprovalsUtil approvalsUtil,
       ChangeData.Factory changeDataFactory,
       @Assisted RefControl refControl,
-      @Assisted Change change) {
+      @Assisted ChangeNotes notes) {
     this.approvalsUtil = approvalsUtil;
     this.changeDataFactory = changeDataFactory;
     this.refControl = refControl;
-    this.change = change;
+    this.notes = notes;
   }
 
   public ChangeControl forUser(final CurrentUser who) {
     return new ChangeControl(approvalsUtil, changeDataFactory,
-        getRefControl().forUser(who), change);
+        getRefControl().forUser(who), notes);
   }
 
   public RefControl getRefControl() {
@@ -225,12 +238,17 @@ public class ChangeControl {
   }
 
   public Change getChange() {
-    return change;
+    return notes.getChange();
+  }
+
+  public ChangeNotes getNotes() {
+    return notes;
   }
 
   /** Can this user see this change? */
   public boolean isVisible(ReviewDb db) throws OrmException {
-    if (change.getStatus() == Change.Status.DRAFT && !isDraftVisible(db, null)) {
+    if (getChange().getStatus() == Change.Status.DRAFT
+        && !isDraftVisible(db, null)) {
       return false;
     }
     return isRefVisible();
@@ -326,7 +344,7 @@ public class ChangeControl {
   public boolean isOwner() {
     if (getCurrentUser().isIdentifiedUser()) {
       final IdentifiedUser i = (IdentifiedUser) getCurrentUser();
-      return i.getAccountId().equals(change.getOwner());
+      return i.getAccountId().equals(getChange().getOwner());
     }
     return false;
   }
@@ -384,7 +402,7 @@ public class ChangeControl {
 
   /** Can this user edit the topic name? */
   public boolean canEditTopicName() {
-    if (change.getStatus().isOpen()) {
+    if (getChange().getStatus().isOpen()) {
       return isOwner() // owner (aka creator) of the change can edit topic
           || getRefControl().isOwner() // branch owner can edit topic
           || getProjectControl().isOwner() // project owner can edit topic
@@ -411,18 +429,18 @@ public class ChangeControl {
   public List<SubmitRecord> canSubmit(ReviewDb db, PatchSet patchSet,
       @Nullable ChangeData cd, boolean fastEvalLabels, boolean allowClosed,
       boolean allowDraft) {
-    if (!allowClosed && change.getStatus().isClosed()) {
+    if (!allowClosed && getChange().getStatus().isClosed()) {
       SubmitRecord rec = new SubmitRecord();
       rec.status = SubmitRecord.Status.CLOSED;
       return Collections.singletonList(rec);
     }
 
-    if (!patchSet.getId().equals(change.currentPatchSetId())) {
+    if (!patchSet.getId().equals(getChange().currentPatchSetId())) {
       return ruleError("Patch set " + patchSet.getPatchSetId() + " is not current");
     }
 
     cd = changeData(db, cd);
-    if ((change.getStatus() == Change.Status.DRAFT || patchSet.isDraft())
+    if ((getChange().getStatus() == Change.Status.DRAFT || patchSet.isDraft())
         && !allowDraft) {
       return cannotSubmitDraft(db, patchSet, cd);
     }
@@ -432,7 +450,7 @@ public class ChangeControl {
     try {
       evaluator = new SubmitRuleEvaluator(db, patchSet,
           getProjectControl(),
-          this, change, cd,
+          this, getChange(), cd,
           fastEvalLabels,
           "locate_submit_rule", "can_submit",
           "locate_submit_filter", "filter_submit_results");
@@ -447,7 +465,7 @@ public class ChangeControl {
       // required for this change to be submittable. Each label will indicate
       // whether or not that is actually possible given the permissions.
       log.error("Submit rule '" + evaluator.getSubmitRule() + "' for change "
-          + change.getId() + " of " + getProject().getName()
+          + getChange().getId() + " of " + getProject().getName()
           + " has no solution.");
       return ruleError("Project submit rule has no solution");
     }
@@ -569,7 +587,8 @@ public class ChangeControl {
       @Nullable ChangeData cd) {
     cd = changeData(db, cd);
     try {
-      if (change.getStatus() == Change.Status.DRAFT && !isDraftVisible(db, cd)) {
+      if (getChange().getStatus() == Change.Status.DRAFT
+          && !isDraftVisible(db, cd)) {
         return typeRuleError("Patch set " + patchSet.getPatchSetId()
             + " not found");
       }
@@ -586,7 +605,7 @@ public class ChangeControl {
     SubmitRuleEvaluator evaluator;
     try {
       evaluator = new SubmitRuleEvaluator(db, patchSet,
-          getProjectControl(), this, change, cd,
+          getProjectControl(), this, getChange(), cd,
           false,
           "locate_submit_type", "get_submit_type",
           "locate_submit_type_filter", "filter_submit_type_results");
@@ -598,7 +617,7 @@ public class ChangeControl {
     if (results.isEmpty()) {
       // Should never occur for a well written rule
       log.error("Submit rule '" + evaluator.getSubmitRule() + "' for change "
-          + change.getId() + " of " + getProject().getName()
+          + getChange().getId() + " of " + getProject().getName()
           + " has no solution.");
       return typeRuleError("Project submit rule has no solution");
     }
@@ -606,7 +625,7 @@ public class ChangeControl {
     Term typeTerm = results.get(0);
     if (!typeTerm.isSymbol()) {
       log.error("Submit rule '" + evaluator.getSubmitRule() + "' for change "
-          + change.getId() + " of " + getProject().getName()
+          + getChange().getId() + " of " + getProject().getName()
           + " did not return a symbol.");
       return typeRuleError("Project submit rule has invalid solution");
     }
@@ -621,7 +640,7 @@ public class ChangeControl {
   }
 
   private List<SubmitRecord> logInvalidResult(Term rule, Term record, String reason) {
-    return logRuleError("Submit rule " + rule + " for change " + change.getId()
+    return logRuleError("Submit rule " + rule + " for change " + getChange().getId()
         + " of " + getProject().getName() + " output invalid result: " + record
         + (reason == null ? "" : ". Reason: " + reason));
   }
@@ -649,7 +668,7 @@ public class ChangeControl {
 
   private SubmitTypeRecord logInvalidType(Term rule, String record) {
     return logTypeRuleError("Submit type rule " + rule + " for change "
-        + change.getId() + " of " + getProject().getName()
+        + getChange().getId() + " of " + getProject().getName()
         + " output invalid result: " + record);
   }
 
@@ -671,7 +690,7 @@ public class ChangeControl {
   }
 
   private ChangeData changeData(ReviewDb db, @Nullable ChangeData cd) {
-    return cd != null ? cd : changeDataFactory.create(db, change);
+    return cd != null ? cd : changeDataFactory.create(db, getChange());
   }
 
   private void appliedBy(SubmitRecord.Label label, Term status)

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -40,6 +40,7 @@ import com.google.gerrit.server.git.MetaDataUpdate;
 import com.google.gerrit.server.util.TimeUtil;
 import com.google.gerrit.testutil.FakeAccountCache;
 import com.google.gerrit.testutil.InMemoryRepositoryManager;
+import com.google.gwtorm.server.OrmException;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
@@ -377,9 +378,8 @@ public class ChangeNotesTest {
         TimeUtil.nowTs(), TZ);
   }
 
-  private ChangeNotes newNotes(Change c)
-      throws ConfigInvalidException, IOException {
-    return new ChangeNotes(repo, c);
+  private ChangeNotes newNotes(Change c) throws OrmException {
+    return new ChangeNotes(repoManager, c).load();
   }
 
   private static void incrementPatchSet(Change change) {

gerrit-server/src/test/java/com/google/gerrit/server/project/Util.java

@@ -244,7 +244,7 @@ public class Util {
 
     return new ProjectControl(Collections.<AccountGroup.UUID> emptySet(),
         Collections.<AccountGroup.UUID> emptySet(), projectCache,
-        sectionSorter, null, changeControlFactory, canonicalWebUrl,
+        sectionSorter, repoManager, changeControlFactory, canonicalWebUrl,
         new MockUser(name, memberOf), newProjectState(local));
   }