diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java index 779a033ede..fff1eb7c5c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeNotes.java +++ b/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 { - 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 { 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 { 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) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java index 3a271d6865..aca2740239 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotes.java +++ b/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 { 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 queryProvider; private final ProjectCache projectCache; - private final ChangeNoteUtil noteUtil; @VisibleForTesting @Inject - public Factory(GitRepositoryManager repoManager, - NotesMigration migration, - AllUsersName allUsers, + public Factory(Args args, Provider 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 { 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 { * @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 { */ 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 createAsync( @@ -239,8 +223,7 @@ public class ChangeNotes extends AbstractChangeNotes { "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 { public List create(ReviewDb db, Collection changeIds) throws OrmException { List 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 { Collection changeIds, Predicate predicate) throws OrmException { List 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 { public ListMultimap create(ReviewDb db, Predicate predicate) throws IOException, OrmException { ListMultimap 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 changes = scanNoteDb(repo, db, project); for (ChangeNotes cn : changes) { if (predicate.apply(cn)) { @@ -327,7 +310,7 @@ public class ChangeNotes extends AbstractChangeNotes { public List 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 { } } - private final ChangeNoteUtil noteUtil; private final Project.NameKey project; private final Change change; private ImmutableSortedMap patchSets; @@ -396,16 +378,11 @@ public class ChangeNotes extends AbstractChangeNotes { // 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 { 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 { 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) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java index 7155f9bbe7..2dd9d7eaf6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/DraftCommentNotes.java +++ b/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 { - @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 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 { 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 cs = ArrayListMultimap.create(); for (RevisionNote rn : revisionNoteMap.revisionNotes.values()) { @@ -141,7 +116,7 @@ public class DraftCommentNotes extends AbstractChangeNotes { @Override public Project.NameKey getProjectName() { - return draftsProject; + return args.allUsers; } @VisibleForTesting diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java index 6d84b41cfc..6b8cacae88 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/NoteDbModule.java +++ b/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); } } diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java index b387405701..7915053d44 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/server/notedb/AbstractChangeNotesTest.java +++ b/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, diff --git a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java b/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java index e6cb5b8477..7ac6cbe467 100644 --- a/gerrit-server/src/test/java/com/google/gerrit/testutil/TestChanges.java +++ b/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. 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 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);