Only call openMetadataRepository when notedb is enabled

This requires some slight ugliness of passing the NotesMigration down
into every AbstractChangeNotes so we can short-circuit during load().

Change-Id: I96a75fc3a7aa1005006e1b70bfb5506c2a0397f4

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

@@ -27,12 +27,16 @@ import java.io.IOException;
 
 /** View of contents at a single ref related to some change. **/
 public abstract class AbstractChangeNotes<T> extends VersionedMetaData {
-  private boolean loaded;
   protected final GitRepositoryManager repoManager;
+  protected final NotesMigration migration;
   private final Change.Id changeId;
 
-  AbstractChangeNotes(GitRepositoryManager repoManager, Change.Id changeId) {
+  private boolean loaded;
+
+  AbstractChangeNotes(GitRepositoryManager repoManager,
+      NotesMigration migration, Change.Id changeId) {
     this.repoManager = repoManager;
+    this.migration = migration;
     this.changeId = changeId;
   }
 
@@ -41,25 +45,33 @@ public abstract class AbstractChangeNotes<T> extends VersionedMetaData {
   }
 
   public T load() throws OrmException {
-    if (!loaded) {
-      Repository repo;
-      try {
-        repo = repoManager.openMetadataRepository(getProjectName());
-      } catch (IOException e) {
-        throw new OrmException(e);
-      }
-      try {
-        load(repo);
-        loaded = true;
-      } catch (ConfigInvalidException | IOException e) {
-        throw new OrmException(e);
-      } finally {
-        repo.close();
-      }
+    if (loaded) {
+      return self();
+    }
+    if (!migration.enabled()) {
+      loadDefaults();
+      return self();
+    }
+    Repository repo;
+    try {
+      repo = repoManager.openMetadataRepository(getProjectName());
+    } catch (IOException e) {
+      throw new OrmException(e);
+    }
+    try {
+      load(repo);
+      loaded = true;
+    } catch (ConfigInvalidException | IOException e) {
+      throw new OrmException(e);
+    } finally {
+      repo.close();
     }
     return self();
   }
 
+  /** Load default values for any instance variables when notedb is disabled. */
+  protected abstract void loadDefaults();
+
   /**
    * @return the NameKey for the project where the notes should be stored,
    *    which is not necessarily the same as the change's project.

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

@@ -112,17 +112,21 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   @Singleton
   public static class Factory {
     private final GitRepositoryManager repoManager;
+    private final NotesMigration migration;
     private final AllUsersNameProvider allUsersProvider;
 
     @VisibleForTesting
     @Inject
-    public Factory(GitRepositoryManager repoManager, AllUsersNameProvider allUsersProvider) {
+    public Factory(GitRepositoryManager repoManager,
+        NotesMigration migration,
+        AllUsersNameProvider allUsersProvider) {
       this.repoManager = repoManager;
+      this.migration = migration;
       this.allUsersProvider = allUsersProvider;
     }
 
     public ChangeNotes create(Change change) {
-      return new ChangeNotes(repoManager, allUsersProvider, change);
+      return new ChangeNotes(repoManager, migration, allUsersProvider, change);
     }
   }
 
@@ -140,9 +144,9 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   private DraftCommentNotes draftCommentNotes;
 
   @VisibleForTesting
-  public ChangeNotes(GitRepositoryManager repoManager,
+  public ChangeNotes(GitRepositoryManager repoManager, NotesMigration migration,
       AllUsersNameProvider allUsersProvider, Change change) {
-    super(repoManager, change.getId());
+    super(repoManager, migration, change.getId());
     this.allUsers = allUsersProvider.get();
     this.change = new Change(change);
   }
@@ -213,8 +217,8 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       throws OrmException {
     if (draftCommentNotes == null ||
         !author.equals(draftCommentNotes.getAuthor())) {
-      draftCommentNotes = new DraftCommentNotes(repoManager, allUsers,
-          getChangeId(), author);
+      draftCommentNotes = new DraftCommentNotes(repoManager, migration,
+          allUsers, getChangeId(), author);
       draftCommentNotes.load();
     }
   }
@@ -289,7 +293,8 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     }
   }
 
-  private void loadDefaults() {
+  @Override
+  protected void loadDefaults() {
     approvals = ImmutableListMultimap.of();
     reviewers = ImmutableSetMultimap.of();
     submitRecords = ImmutableList.of();

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

@@ -48,19 +48,22 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
   @Singleton
   public static class Factory {
     private final GitRepositoryManager repoManager;
+    private final NotesMigration migration;
     private final AllUsersName draftsProject;
 
     @VisibleForTesting
     @Inject
     public Factory(GitRepositoryManager repoManager,
+        NotesMigration migration,
         AllUsersNameProvider allUsers) {
       this.repoManager = repoManager;
+      this.migration = migration;
       this.draftsProject = allUsers.get();
     }
 
     public DraftCommentNotes create(Change.Id changeId, Account.Id accountId) {
-      return new DraftCommentNotes(repoManager, draftsProject, changeId,
-          accountId);
+      return new DraftCommentNotes(repoManager, migration, draftsProject,
+          changeId, accountId);
     }
   }
 
@@ -71,9 +74,9 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
   private final Table<PatchSet.Id, String, PatchLineComment> draftPsComments;
   private NoteMap noteMap;
 
-  DraftCommentNotes(GitRepositoryManager repoManager,
+  DraftCommentNotes(GitRepositoryManager repoManager, NotesMigration migration,
       AllUsersName draftsProject, Change.Id changeId, Account.Id author) {
-    super(repoManager, changeId);
+    super(repoManager, migration, changeId);
     this.draftsProject = draftsProject;
     this.author = author;
 
@@ -149,6 +152,11 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
         getClass().getSimpleName() + " is read-only");
   }
 
+  @Override
+  protected void loadDefaults() {
+    // Do nothing; tables are final and initialized in constructor.
+  }
+
   @Override
   protected Project.NameKey getProjectName() {
     return draftsProject;

gerrit-server/src/test/java/com/google/gerrit/server/change/CommentsTest.java

@@ -126,6 +126,7 @@ public class CommentsTest  {
   @Inject private GetComment getComment;
   @Inject private IdentifiedUser.GenericFactory userFactory;
   @Inject private InMemoryRepositoryManager repoManager;
+  @Inject private NotesMigration migration;
   @Inject private PatchLineCommentsUtil plcUtil;
 
   @Before
@@ -294,7 +295,8 @@ public class CommentsTest  {
   }
 
   private ChangeControl stubChangeControl(Change c) throws OrmException {
-    return TestChanges.stubChangeControl(repoManager, c, allUsers, changeOwner);
+    return TestChanges.stubChangeControl(
+        repoManager, migration, c, allUsers, changeOwner);
   }
 
   private Change newChange() {
@@ -302,7 +304,8 @@ public class CommentsTest  {
   }
 
   private ChangeUpdate newUpdate(Change c, final IdentifiedUser user) throws Exception {
-    return TestChanges.newUpdate(injector, repoManager, c, allUsers, user);
+    return TestChanges.newUpdate(
+        injector, repoManager, migration, c, allUsers, user);
   }
 
   @Test

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

@@ -75,6 +75,8 @@ public class AbstractChangeNotesTest {
   private static final TimeZone TZ =
       TimeZone.getTimeZone("America/Los_Angeles");
 
+  private static final NotesMigration MIGRATION = NotesMigration.allEnabled();
+
   protected Account.Id otherUserId;
   protected FakeAccountCache accountCache;
   protected IdentifiedUser changeOwner;
@@ -116,7 +118,7 @@ public class AbstractChangeNotesTest {
       @Override
       public void configure() {
         install(new GitModule());
-        bind(NotesMigration.class).toInstance(NotesMigration.allEnabled());
+        bind(NotesMigration.class).toInstance(MIGRATION);
         bind(GitRepositoryManager.class).toInstance(repoManager);
         bind(ProjectCache.class).toProvider(Providers.<ProjectCache> of(null));
         bind(CapabilityControl.Factory.class)
@@ -170,11 +172,12 @@ public class AbstractChangeNotesTest {
 
   protected ChangeUpdate newUpdate(Change c, IdentifiedUser user)
       throws OrmException {
-    return TestChanges.newUpdate(injector, repoManager, c, allUsers, user);
+    return TestChanges.newUpdate(
+        injector, repoManager, MIGRATION, c, allUsers, user);
   }
 
   protected ChangeNotes newNotes(Change c) throws OrmException {
-    return new ChangeNotes(repoManager, allUsers, c).load();
+    return new ChangeNotes(repoManager, MIGRATION, allUsers, c).load();
   }
 
   protected static SubmitRecord submitRecord(String status,

gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java

@@ -30,6 +30,7 @@ import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.notedb.ChangeDraftUpdate;
 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.project.ChangeControl;
 import com.google.gerrit.server.util.TimeUtil;
 import com.google.gwtorm.server.OrmException;
@@ -64,7 +65,7 @@ public class TestChanges {
   }
 
   public static ChangeUpdate newUpdate(Injector injector,
-      GitRepositoryManager repoManager, Change c,
+      GitRepositoryManager repoManager, NotesMigration migration, Change c,
       final AllUsersNameProvider allUsers, final IdentifiedUser user)
       throws OrmException {
     return injector.createChildInjector(new FactoryModule() {
@@ -76,18 +77,19 @@ public class TestChanges {
         bind(AllUsersName.class).toProvider(allUsers);
       }
     }).getInstance(ChangeUpdate.Factory.class).create(
-        stubChangeControl(repoManager, c, allUsers, user), TimeUtil.nowTs(),
-        Ordering.<String> natural());
+        stubChangeControl(repoManager, migration, c, allUsers, user),
+        TimeUtil.nowTs(), Ordering.<String> natural());
   }
 
   public static ChangeControl stubChangeControl(
-      GitRepositoryManager repoManager, Change c, AllUsersNameProvider allUsers,
+      GitRepositoryManager repoManager, NotesMigration migration,
+      Change c, AllUsersNameProvider allUsers,
       IdentifiedUser user) throws OrmException {
     ChangeControl ctl = EasyMock.createNiceMock(ChangeControl.class);
     expect(ctl.getChange()).andStubReturn(c);
     expect(ctl.getCurrentUser()).andStubReturn(user);
-    ChangeNotes notes = new ChangeNotes(repoManager, allUsers, c);
-    notes = notes.load();
+    ChangeNotes notes = new ChangeNotes(repoManager, migration, allUsers, c)
+        .load();
     expect(ctl.getNotes()).andStubReturn(notes);
     EasyMock.replay(ctl);
     return ctl;