Factor out an arguments class for ChangeNotes

We do a semi-manual injection thing with ChangeNotes.Factory that
makes adding new arguments painful. Limit the pain by encapsulating
into an injectable Args class.

This has more side benefits, like making DraftCommentNotes able to use
straight assisted injection, and simplifying TestChanges.

Change-Id: I747aefb8d6e16ad6488b2a686bd23304a0fe24f6

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

@@ -14,10 +14,14 @@
 
 package com.google.gerrit.server.notedb;
 
+import com.google.common.annotations.VisibleForTesting;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.Project;
+import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gwtorm.server.OrmException;
+import com.google.inject.Inject;
+import com.google.inject.Singleton;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.ObjectId;
@@ -29,17 +33,35 @@ import java.io.IOException;
 
 /** View of contents at a single ref related to some change. **/
 public abstract class AbstractChangeNotes<T> {
-  protected final GitRepositoryManager repoManager;
-  protected final NotesMigration migration;
+  @VisibleForTesting
+  @Singleton
+  public static class Args {
+    final GitRepositoryManager repoManager;
+    final NotesMigration migration;
+    final AllUsersName allUsers;
+    final ChangeNoteUtil noteUtil;
+
+    @Inject
+    Args(
+        GitRepositoryManager repoManager,
+        NotesMigration migration,
+        AllUsersName allUsers,
+        ChangeNoteUtil noteUtil) {
+      this.repoManager = repoManager;
+      this.migration = migration;
+      this.allUsers = allUsers;
+      this.noteUtil = noteUtil;
+    }
+  }
+
+  protected final Args args;
   private final Change.Id changeId;
 
   private ObjectId revision;
   private boolean loaded;
 
-  AbstractChangeNotes(GitRepositoryManager repoManager,
-      NotesMigration migration, Change.Id changeId) {
-    this.repoManager = repoManager;
-    this.migration = migration;
+  AbstractChangeNotes(Args args, Change.Id changeId) {
+    this.args = args;
     this.changeId = changeId;
   }
 
@@ -56,11 +78,12 @@ public abstract class AbstractChangeNotes<T> {
     if (loaded) {
       return self();
     }
-    if (!migration.enabled() || changeId == null) {
+    if (!args.migration.enabled() || changeId == null) {
       loadDefaults();
       return self();
     }
-    try (Repository repo = repoManager.openMetadataRepository(getProjectName());
+    try (Repository repo =
+            args.repoManager.openMetadataRepository(getProjectName());
         RevWalk walk = new RevWalk(repo)) {
       Ref ref = repo.getRefDatabase().exactRef(getRefName());
       ObjectId id = ref != null ? ref.getObjectId() : null;
@@ -81,10 +104,11 @@ public abstract class AbstractChangeNotes<T> {
   public ObjectId loadRevision() throws OrmException {
     if (loaded) {
       return getRevision();
-    } else if (!migration.enabled()) {
+    } else if (!args.migration.enabled()) {
       return null;
     }
-    try (Repository repo = repoManager.openMetadataRepository(getProjectName())) {
+    try (Repository repo =
+        args.repoManager.openMetadataRepository(getProjectName())) {
       Ref ref = repo.getRefDatabase().exactRef(getRefName());
       return ref != null ? ref.getObjectId() : null;
     } catch (IOException e) {

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

@@ -53,8 +53,6 @@ import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.reviewdb.server.ReviewDb;
 import com.google.gerrit.reviewdb.server.ReviewDbUtil;
-import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.git.GitRepositoryManager;
 import com.google.gerrit.server.project.NoSuchChangeException;
 import com.google.gerrit.server.project.ProjectCache;
 import com.google.gerrit.server.query.change.ChangeData;
@@ -113,27 +111,18 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   public static class Factory {
     private static final Logger log = LoggerFactory.getLogger(Factory.class);
 
-    private final GitRepositoryManager repoManager;
-    private final NotesMigration migration;
-    private final AllUsersName allUsers;
+    private final Args args;
     private final Provider<InternalChangeQuery> queryProvider;
     private final ProjectCache projectCache;
-    private final ChangeNoteUtil noteUtil;
 
     @VisibleForTesting
     @Inject
-    public Factory(GitRepositoryManager repoManager,
-        NotesMigration migration,
-        AllUsersName allUsers,
+    public Factory(Args args,
         Provider<InternalChangeQuery> queryProvider,
-        ProjectCache projectCache,
-        ChangeNoteUtil noteUtil) {
-      this.repoManager = repoManager;
-      this.migration = migration;
-      this.allUsers = allUsers;
+        ProjectCache projectCache) {
+      this.args = args;
       this.queryProvider = queryProvider;
       this.projectCache = projectCache;
-      this.noteUtil = noteUtil;
     }
 
     public ChangeNotes createChecked(ReviewDb db, Change c)
@@ -178,8 +167,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
           project, changeId, change.getProject());
       // TODO: Throw NoSuchChangeException when the change is not found in the
       // database
-      return new ChangeNotes(repoManager, migration, allUsers, noteUtil,
-          project, change).load();
+      return new ChangeNotes(args, project, change).load();
     }
 
     /**
@@ -191,23 +179,20 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
      * @return change notes
      */
     public ChangeNotes createFromIndexedChange(Change change) {
-      return new ChangeNotes(repoManager, migration, allUsers, noteUtil,
-          change.getProject(), change);
+      return new ChangeNotes(args, change.getProject(), change);
     }
 
     public ChangeNotes createForNew(Change change) throws OrmException {
-      return new ChangeNotes(repoManager, migration, allUsers, noteUtil,
-          change.getProject(), change).load();
+      return new ChangeNotes(args, change.getProject(), change).load();
     }
 
     // TODO(dborowitz): Remove when deleting index schemas <27.
     public ChangeNotes createFromIdOnlyWhenNoteDbDisabled(
         ReviewDb db, Change.Id changeId) throws OrmException {
-    checkState(!migration.readChanges(), "do not call"
+    checkState(!args.migration.readChanges(), "do not call"
         + " createFromIdOnlyWhenNoteDbDisabled when NoteDb is enabled");
       Change change = unwrap(db).changes().get(changeId);
-      return new ChangeNotes(repoManager, migration, allUsers, noteUtil,
-          change.getProject(), change).load();
+      return new ChangeNotes(args, change.getProject(), change).load();
     }
 
     // TODO(ekempin): Remove when database backend is deleted
@@ -217,10 +202,9 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
      */
     private ChangeNotes createFromChangeOnlyWhenNoteDbDisabled(Change change)
         throws OrmException {
-      checkState(!migration.readChanges(), "do not call"
+      checkState(!args.migration.readChanges(), "do not call"
           + " createFromChangeWhenNoteDbDisabled when NoteDb is enabled");
-      return new ChangeNotes(repoManager, migration, allUsers, noteUtil,
-          change.getProject(), change).load();
+      return new ChangeNotes(args, change.getProject(), change).load();
     }
 
     public CheckedFuture<ChangeNotes, OrmException> createAsync(
@@ -239,8 +223,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
                           "passed project %s when creating ChangeNotes for %s,"
                               + " but actual project is %s",
                           project, changeId, change.getProject());
-                      return new ChangeNotes(repoManager, migration, allUsers,
-                          noteUtil, project, change).load();
+                      return new ChangeNotes(args, project, change).load();
                     }
                   });
                 }
@@ -258,7 +241,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     public List<ChangeNotes> create(ReviewDb db,
         Collection<Change.Id> changeIds) throws OrmException {
       List<ChangeNotes> notes = new ArrayList<>();
-      if (migration.enabled()) {
+      if (args.migration.enabled()) {
         for (Change.Id changeId : changeIds) {
           try {
             notes.add(createChecked(changeId));
@@ -279,7 +262,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
         Collection<Change.Id> changeIds, Predicate<ChangeNotes> predicate)
             throws OrmException {
       List<ChangeNotes> notes = new ArrayList<>();
-      if (migration.enabled()) {
+      if (args.migration.enabled()) {
         for (Change.Id cid : changeIds) {
           ChangeNotes cn = create(db, project, cid);
           if (cn.getChange() != null && predicate.apply(cn)) {
@@ -303,9 +286,9 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     public ListMultimap<Project.NameKey, ChangeNotes> create(ReviewDb db,
         Predicate<ChangeNotes> predicate) throws IOException, OrmException {
       ListMultimap<Project.NameKey, ChangeNotes> m = ArrayListMultimap.create();
-      if (migration.readChanges()) {
+      if (args.migration.readChanges()) {
         for (Project.NameKey project : projectCache.all()) {
-          try (Repository repo = repoManager.openRepository(project)) {
+          try (Repository repo = args.repoManager.openRepository(project)) {
             List<ChangeNotes> changes = scanNoteDb(repo, db, project);
             for (ChangeNotes cn : changes) {
               if (predicate.apply(cn)) {
@@ -327,7 +310,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
 
     public List<ChangeNotes> scan(Repository repo, ReviewDb db,
         Project.NameKey project) throws OrmException, IOException {
-      if (!migration.readChanges()) {
+      if (!args.migration.readChanges()) {
         return scanDb(repo, db);
       }
 
@@ -379,7 +362,6 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
     }
   }
 
-  private final ChangeNoteUtil noteUtil;
   private final Project.NameKey project;
   private final Change change;
   private ImmutableSortedMap<PatchSet.Id, PatchSet> patchSets;
@@ -396,16 +378,11 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
   // notes easier.
   RevisionNoteMap revisionNoteMap;
 
-  private final AllUsersName allUsers;
   private DraftCommentNotes draftCommentNotes;
 
   @VisibleForTesting
-  public ChangeNotes(GitRepositoryManager repoManager, NotesMigration migration,
-      AllUsersName allUsers, ChangeNoteUtil noteUtil,
-      Project.NameKey project, Change change) {
-    super(repoManager, migration, change != null ? change.getId() : null);
-    this.allUsers = allUsers;
-    this.noteUtil = noteUtil;
+  public ChangeNotes(Args args, Project.NameKey project, Change change) {
+    super(args, change != null ? change.getId() : null);
     this.project = project;
     this.change = change != null ? new Change(change) : null;
   }
@@ -502,8 +479,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       throws OrmException {
     if (draftCommentNotes == null ||
         !author.equals(draftCommentNotes.getAuthor())) {
-      draftCommentNotes = new DraftCommentNotes(repoManager, migration,
-          allUsers, noteUtil, getChangeId(), author);
+      draftCommentNotes = new DraftCommentNotes(args, getChangeId(), author);
       draftCommentNotes.load();
     }
   }
@@ -550,7 +526,7 @@ public class ChangeNotes extends AbstractChangeNotes<ChangeNotes> {
       return;
     }
     try (ChangeNotesParser parser = new ChangeNotesParser(
-         project, change.getId(), rev, walk, repoManager, noteUtil)) {
+         project, change.getId(), rev, walk, args.repoManager, args.noteUtil)) {
       parser.parseAll();
 
       if (parser.status != null) {

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

@@ -24,10 +24,8 @@ import com.google.gerrit.reviewdb.client.PatchLineComment;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RefNames;
 import com.google.gerrit.reviewdb.client.RevId;
-import com.google.gerrit.server.config.AllUsersName;
-import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.inject.Inject;
-import com.google.inject.Singleton;
+import com.google.inject.assistedinject.Assisted;
+import com.google.inject.assistedinject.AssistedInject;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.ObjectId;
@@ -43,45 +41,22 @@ import java.io.IOException;
  * its drafts branch.
  */
 public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
-  @Singleton
-  public static class Factory {
-    private final GitRepositoryManager repoManager;
-    private final NotesMigration migration;
-    private final AllUsersName draftsProject;
-    private final ChangeNoteUtil noteUtil;
-
-    @VisibleForTesting
-    @Inject
-    public Factory(GitRepositoryManager repoManager,
-        NotesMigration migration,
-        AllUsersName allUsers,
-        ChangeNoteUtil noteUtil) {
-      this.repoManager = repoManager;
-      this.migration = migration;
-      this.draftsProject = allUsers;
-      this.noteUtil = noteUtil;
-    }
-
-    public DraftCommentNotes create(Change.Id changeId, Account.Id accountId) {
-      return new DraftCommentNotes(repoManager, migration, draftsProject,
-          noteUtil, changeId, accountId);
-    }
+  public interface Factory {
+    DraftCommentNotes create(Change.Id changeId, Account.Id accountId);
   }
 
-  private final AllUsersName draftsProject;
-  private final ChangeNoteUtil noteUtil;
   private final Account.Id author;
 
   private ImmutableListMultimap<RevId, PatchLineComment> comments;
   private RevisionNoteMap revisionNoteMap;
 
-  DraftCommentNotes(GitRepositoryManager repoManager, NotesMigration migration,
-      AllUsersName draftsProject, ChangeNoteUtil noteUtil, Change.Id changeId,
-      Account.Id author) {
-    super(repoManager, migration, changeId);
-    this.draftsProject = draftsProject;
+  @AssistedInject
+  DraftCommentNotes(
+      Args args,
+      @Assisted Change.Id changeId,
+      @Assisted Account.Id author) {
+    super(args, changeId);
     this.author = author;
-    this.noteUtil = noteUtil;
   }
 
   RevisionNoteMap getRevisionNoteMap() {
@@ -123,7 +98,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
     RevCommit tipCommit = walk.parseCommit(rev);
     ObjectReader reader = walk.getObjectReader();
     revisionNoteMap = RevisionNoteMap.parse(
-        noteUtil, getChangeId(), reader, NoteMap.read(reader, tipCommit),
+        args.noteUtil, getChangeId(), reader, NoteMap.read(reader, tipCommit),
         true);
     Multimap<RevId, PatchLineComment> cs = ArrayListMultimap.create();
     for (RevisionNote rn : revisionNoteMap.revisionNotes.values()) {
@@ -141,7 +116,7 @@ public class DraftCommentNotes extends AbstractChangeNotes<DraftCommentNotes> {
 
   @Override
   public Project.NameKey getProjectName() {
-    return draftsProject;
+    return args.allUsers;
   }
 
   @VisibleForTesting

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

@@ -21,6 +21,7 @@ public class NoteDbModule extends FactoryModule {
   public void configure() {
     factory(ChangeUpdate.Factory.class);
     factory(ChangeDraftUpdate.Factory.class);
+    factory(DraftCommentNotes.Factory.class);
     factory(NoteDbUpdateManager.Factory.class);
   }
 }

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

@@ -110,6 +110,9 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
   @Inject
   protected ChangeNoteUtil noteUtil;
 
+  @Inject
+  protected AbstractChangeNotes.Args args;
+
   private Injector injector;
   private String systemTimeZone;
 
@@ -139,7 +142,7 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
       @Override
       public void configure() {
         install(new GitModule());
-        factory(NoteDbUpdateManager.Factory.class);
+        install(new NoteDbModule());
         bind(AllUsersName.class).toProvider(AllUsersNameProvider.class);
         bind(String.class).annotatedWith(GerritServerId.class)
             .toInstance("gerrit");
@@ -198,14 +201,11 @@ public abstract class AbstractChangeNotesTest extends GerritBaseTests {
 
   protected ChangeUpdate newUpdate(Change c, CurrentUser user)
       throws Exception {
-    ChangeUpdate update = TestChanges.newUpdate(
-        injector, repoManager, MIGRATION, c, allUsers, user);
-    return update;
+    return TestChanges.newUpdate(injector, c, user);
   }
 
   protected ChangeNotes newNotes(Change c) throws OrmException {
-    return new ChangeNotes(repoManager, MIGRATION, allUsers, noteUtil,
-        c.getProject(), c).load();
+    return new ChangeNotes(args, c.getProject(), c).load();
   }
 
   protected static SubmitRecord submitRecord(String status,

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

@@ -28,10 +28,8 @@ import com.google.gerrit.reviewdb.client.PatchSetInfo;
 import com.google.gerrit.reviewdb.client.Project;
 import com.google.gerrit.reviewdb.client.RevId;
 import com.google.gerrit.server.CurrentUser;
-import com.google.gerrit.server.config.AllUsersName;
 import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.notedb.ChangeDraftUpdate;
-import com.google.gerrit.server.notedb.ChangeNoteUtil;
+import com.google.gerrit.server.notedb.AbstractChangeNotes;
 import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.notedb.ChangeUpdate;
 import com.google.gerrit.server.notedb.NotesMigration;
@@ -88,34 +86,33 @@ public class TestChanges {
   }
 
   public static ChangeUpdate newUpdate(Injector injector,
-      GitRepositoryManager repoManager, NotesMigration migration, Change c,
-      final AllUsersName allUsers, final CurrentUser user)
-      throws Exception  {
+      Change c, final CurrentUser user) throws Exception  {
     injector = injector.createChildInjector(new FactoryModule() {
       @Override
       public void configure() {
-        factory(ChangeUpdate.Factory.class);
-        factory(ChangeDraftUpdate.Factory.class);
         bind(CurrentUser.class).toInstance(user);
       }
     });
     ChangeUpdate update = injector.getInstance(ChangeUpdate.Factory.class)
         .create(
             stubChangeControl(
-                repoManager, migration, c, allUsers,
-                injector.getInstance(ChangeNoteUtil.class),
+                injector.getInstance(AbstractChangeNotes.Args.class),
+                c,
                 user),
             TimeUtil.nowTs(), Ordering.<String> natural());
 
     ChangeNotes notes = update.getChangeNotes();
     boolean hasPatchSets = notes.getPatchSets() != null
         && !notes.getPatchSets().isEmpty();
+    NotesMigration migration = injector.getInstance(NotesMigration.class);
     if (hasPatchSets || !migration.readChanges()) {
       return update;
     }
 
     // Change doesn't exist yet. NoteDb requires that there be a commit for the
     // first patch set, so create one.
+    GitRepositoryManager repoManager =
+        injector.getInstance(GitRepositoryManager.class);
     try (Repository repo = repoManager.openRepository(c.getProject())) {
       TestRepository<Repository> tr = new TestRepository<>(repo);
       PersonIdent ident = user.asIdentifiedUser()
@@ -136,16 +133,13 @@ public class TestChanges {
   }
 
   private static ChangeControl stubChangeControl(
-      GitRepositoryManager repoManager, NotesMigration migration,
-      Change c, AllUsersName allUsers, ChangeNoteUtil noteUtil,
-      CurrentUser user) throws OrmException {
+      AbstractChangeNotes.Args args,
+      Change c, CurrentUser user) throws OrmException {
     ChangeControl ctl = EasyMock.createMock(ChangeControl.class);
     expect(ctl.getChange()).andStubReturn(c);
     expect(ctl.getProject()).andStubReturn(new Project(c.getProject()));
     expect(ctl.getUser()).andStubReturn(user);
-    ChangeNotes notes =
-        new ChangeNotes(repoManager, migration, allUsers, noteUtil,
-            c.getProject(), c).load();
+    ChangeNotes notes = new ChangeNotes(args, c.getProject(), c).load();
     expect(ctl.getNotes()).andStubReturn(notes);
     expect(ctl.getId()).andStubReturn(c.getId());
     EasyMock.replay(ctl);