From a91dd93c40020ade94e4d5806fff0b49bb3b140c Mon Sep 17 00:00:00 2001 From: Patrick Hiesel Date: Thu, 14 Sep 2017 15:50:39 +0200 Subject: [PATCH] Remove ChangeControl from ChangeUpdate and migrate callers Change-Id: Id189e2ed5049a96adc15302aa475cf9e0289e3e5 --- .../server/notedb/NoteDbPrimaryIT.java | 5 ++- .../server/notedb/AbstractChangeUpdate.java | 12 +++---- .../gerrit/server/notedb/ChangeUpdate.java | 32 ++++++++++--------- .../server/notedb/PrimaryStorageMigrator.java | 13 ++++---- .../server/update/NoteDbBatchUpdate.java | 19 ++++------- .../server/update/ReviewDbBatchUpdate.java | 20 ++++-------- .../google/gerrit/testutil/TestChanges.java | 20 ++---------- 7 files changed, 47 insertions(+), 74 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java index 37bdeee098..d81bf6b87c 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/server/notedb/NoteDbPrimaryIT.java @@ -63,7 +63,6 @@ import com.google.gerrit.server.notedb.NoteDbChangeState; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.PrimaryStorageMigrator; import com.google.gerrit.server.notedb.TestChangeRebuilderWrapper; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.update.RetryHelper; import com.google.gerrit.testutil.ConfigSuite; import com.google.gerrit.testutil.NoteDbMode; @@ -101,7 +100,7 @@ public class NoteDbPrimaryIT extends AbstractDaemonTest { @Inject private ChangeBundleReader bundleReader; @Inject private CommentsUtil commentsUtil; @Inject private TestChangeRebuilderWrapper rebuilderWrapper; - @Inject private ChangeControl.GenericFactory changeControlFactory; + @Inject private ChangeNotes.Factory changeNotesFactory; @Inject private ChangeUpdate.Factory updateFactory; @Inject private InternalUser.Factory internalUserFactory; @Inject private RetryHelper retryHelper; @@ -125,7 +124,7 @@ public class NoteDbPrimaryIT extends AbstractDaemonTest { allUsers, rebuilderWrapper, ensureRebuiltRetryer, - changeControlFactory, + changeNotesFactory, queryProvider, updateFactory, internalUserFactory, diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java index 6dfe7a375d..f3f3a13c3f 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/AbstractChangeUpdate.java @@ -26,7 +26,6 @@ import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.InternalUser; -import com.google.gerrit.server.project.ChangeControl; import com.google.gwtorm.server.OrmException; import java.io.IOException; import java.sql.Timestamp; @@ -62,7 +61,8 @@ public abstract class AbstractChangeUpdate { protected AbstractChangeUpdate( Config cfg, NotesMigration migration, - ChangeControl ctl, + ChangeNotes notes, + CurrentUser user, PersonIdent serverIdent, String anonymousCowardName, ChangeNoteUtil noteUtil, @@ -71,12 +71,12 @@ public abstract class AbstractChangeUpdate { this.noteUtil = noteUtil; this.serverIdent = new PersonIdent(serverIdent, when); this.anonymousCowardName = anonymousCowardName; - this.notes = ctl.getNotes(); + this.notes = notes; this.change = notes.getChange(); - this.accountId = accountId(ctl.getUser()); - Account.Id realAccountId = accountId(ctl.getUser().getRealUser()); + this.accountId = accountId(user); + Account.Id realAccountId = accountId(user.getRealUser()); this.realAccountId = realAccountId != null ? realAccountId : accountId; - this.authorIdent = ident(noteUtil, serverIdent, anonymousCowardName, ctl.getUser(), when); + this.authorIdent = ident(noteUtil, serverIdent, anonymousCowardName, user, when); this.when = when; this.readOnlySkewMs = NoteDbChangeState.getReadOnlySkew(cfg); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java index d692dff2c6..7e0daa61c4 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java @@ -59,12 +59,12 @@ import com.google.gerrit.reviewdb.client.PatchLineComment; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.reviewdb.client.RobotComment; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GerritPersonIdent; import com.google.gerrit.server.account.AccountCache; import com.google.gerrit.server.config.AnonymousCowardName; import com.google.gerrit.server.config.GerritServerConfig; import com.google.gerrit.server.mail.Address; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.util.LabelVote; import com.google.gerrit.server.util.RequestId; @@ -108,9 +108,9 @@ import org.eclipse.jgit.revwalk.RevWalk; */ public class ChangeUpdate extends AbstractChangeUpdate { public interface Factory { - ChangeUpdate create(ChangeControl ctl); + ChangeUpdate create(ChangeNotes notes, CurrentUser user); - ChangeUpdate create(ChangeControl ctl, Date when); + ChangeUpdate create(ChangeNotes notes, CurrentUser user, Date when); ChangeUpdate create( Change change, @@ -121,7 +121,8 @@ public class ChangeUpdate extends AbstractChangeUpdate { Comparator labelNameComparator); @VisibleForTesting - ChangeUpdate create(ChangeControl ctl, Date when, Comparator labelNameComparator); + ChangeUpdate create( + ChangeNotes notes, CurrentUser user, Date when, Comparator labelNameComparator); } private final AccountCache accountCache; @@ -175,7 +176,8 @@ public class ChangeUpdate extends AbstractChangeUpdate { RobotCommentUpdate.Factory robotCommentUpdateFactory, DeleteCommentRewriter.Factory deleteCommentRewriterFactory, ProjectCache projectCache, - @Assisted ChangeControl ctl, + @Assisted ChangeNotes notes, + @Assisted CurrentUser user, ChangeNoteUtil noteUtil) { this( cfg, @@ -188,7 +190,8 @@ public class ChangeUpdate extends AbstractChangeUpdate { robotCommentUpdateFactory, deleteCommentRewriterFactory, projectCache, - ctl, + notes, + user, serverIdent.getWhen(), noteUtil); } @@ -205,7 +208,8 @@ public class ChangeUpdate extends AbstractChangeUpdate { RobotCommentUpdate.Factory robotCommentUpdateFactory, DeleteCommentRewriter.Factory deleteCommentRewriterFactory, ProjectCache projectCache, - @Assisted ChangeControl ctl, + @Assisted ChangeNotes notes, + @Assisted CurrentUser user, @Assisted Date when, ChangeNoteUtil noteUtil) { this( @@ -218,16 +222,13 @@ public class ChangeUpdate extends AbstractChangeUpdate { draftUpdateFactory, robotCommentUpdateFactory, deleteCommentRewriterFactory, - ctl, + notes, + user, when, - projectCache.get(getProjectName(ctl)).getLabelTypes().nameComparator(), + projectCache.get(notes.getProjectName()).getLabelTypes().nameComparator(), noteUtil); } - private static Project.NameKey getProjectName(ChangeControl ctl) { - return ctl.getProject().getNameKey(); - } - private static Table> approvals( Comparator nameComparator) { return TreeBasedTable.create(nameComparator, comparing(IntKey::get)); @@ -244,11 +245,12 @@ public class ChangeUpdate extends AbstractChangeUpdate { ChangeDraftUpdate.Factory draftUpdateFactory, RobotCommentUpdate.Factory robotCommentUpdateFactory, DeleteCommentRewriter.Factory deleteCommentRewriterFactory, - @Assisted ChangeControl ctl, + @Assisted ChangeNotes notes, + @Assisted CurrentUser user, @Assisted Date when, @Assisted Comparator labelNameComparator, ChangeNoteUtil noteUtil) { - super(cfg, migration, ctl, serverIdent, anonymousCowardName, noteUtil, when); + super(cfg, migration, notes, user, serverIdent, anonymousCowardName, noteUtil, when); this.accountCache = accountCache; this.updateManagerFactory = updateManagerFactory; this.draftUpdateFactory = draftUpdateFactory; diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java index 8503f0aa19..aaa1c2f72c 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/notedb/PrimaryStorageMigrator.java @@ -46,7 +46,6 @@ import com.google.gerrit.server.index.change.ChangeField; import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.NoteDbChangeState.RefState; import com.google.gerrit.server.notedb.rebuild.ChangeRebuilder; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; import com.google.gerrit.server.update.BatchUpdate; @@ -83,7 +82,7 @@ public class PrimaryStorageMigrator { private static final Logger log = LoggerFactory.getLogger(PrimaryStorageMigrator.class); private final AllUsersName allUsers; - private final ChangeControl.GenericFactory changeControlFactory; + private final ChangeNotes.Factory changeNotesFactory; private final ChangeRebuilder rebuilder; private final ChangeUpdate.Factory updateFactory; private final GitRepositoryManager repoManager; @@ -103,7 +102,7 @@ public class PrimaryStorageMigrator { GitRepositoryManager repoManager, AllUsersName allUsers, ChangeRebuilder rebuilder, - ChangeControl.GenericFactory changeControlFactory, + ChangeNotes.Factory changeNotesFactory, Provider queryProvider, ChangeUpdate.Factory updateFactory, InternalUser.Factory internalUserFactory, @@ -115,7 +114,7 @@ public class PrimaryStorageMigrator { allUsers, rebuilder, null, - changeControlFactory, + changeNotesFactory, queryProvider, updateFactory, internalUserFactory, @@ -130,7 +129,7 @@ public class PrimaryStorageMigrator { AllUsersName allUsers, ChangeRebuilder rebuilder, @Nullable Retryer testEnsureRebuiltRetryer, - ChangeControl.GenericFactory changeControlFactory, + ChangeNotes.Factory changeNotesFactory, Provider queryProvider, ChangeUpdate.Factory updateFactory, InternalUser.Factory internalUserFactory, @@ -140,7 +139,7 @@ public class PrimaryStorageMigrator { this.allUsers = allUsers; this.rebuilder = rebuilder; this.testEnsureRebuiltRetryer = testEnsureRebuiltRetryer; - this.changeControlFactory = changeControlFactory; + this.changeNotesFactory = changeNotesFactory; this.queryProvider = queryProvider; this.updateFactory = updateFactory; this.internalUserFactory = internalUserFactory; @@ -410,7 +409,7 @@ public class PrimaryStorageMigrator { Timestamp until = new Timestamp(now.getTime() + timeoutMs); ChangeUpdate update = updateFactory.create( - changeControlFactory.controlFor(db.get(), project, id, internalUserFactory.create())); + changeNotesFactory.createChecked(db.get(), project, id), internalUserFactory.create()); update.setReadOnlyUntil(until); return update.commit(); } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java index f7ed3d11f2..ceef352dc6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/NoteDbBatchUpdate.java @@ -35,7 +35,6 @@ import com.google.gerrit.server.index.change.ChangeIndexer; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.ChangeUpdate; import com.google.gerrit.server.notedb.NoteDbUpdateManager; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.util.RequestId; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -215,13 +214,13 @@ class NoteDbBatchUpdate extends BatchUpdate { } private class ChangeContextImpl extends ContextImpl implements ChangeContext { - private final ChangeControl ctl; + private final ChangeNotes notes; private final Map updates; private boolean deleted; - protected ChangeContextImpl(ChangeControl ctl) { - this.ctl = checkNotNull(ctl); + protected ChangeContextImpl(ChangeNotes notes) { + this.notes = checkNotNull(notes); updates = new TreeMap<>(comparing(PatchSet.Id::get)); } @@ -229,8 +228,8 @@ class NoteDbBatchUpdate extends BatchUpdate { public ChangeUpdate getUpdate(PatchSet.Id psId) { ChangeUpdate u = updates.get(psId); if (u == null) { - u = changeUpdateFactory.create(ctl, when); - if (newChanges.containsKey(ctl.getId())) { + u = changeUpdateFactory.create(notes, user, when); + if (newChanges.containsKey(notes.getChangeId())) { u.setAllowWriteToNewRef(true); } u.setPatchSetId(psId); @@ -241,7 +240,7 @@ class NoteDbBatchUpdate extends BatchUpdate { @Override public ChangeNotes getNotes() { - return ctl.getNotes(); + return notes; } @Override @@ -264,7 +263,6 @@ class NoteDbBatchUpdate extends BatchUpdate { } private final ChangeNotes.Factory changeNotesFactory; - private final ChangeControl.GenericFactory changeControlFactory; private final ChangeUpdate.Factory changeUpdateFactory; private final NoteDbUpdateManager.Factory updateManagerFactory; private final ChangeIndexer indexer; @@ -276,7 +274,6 @@ class NoteDbBatchUpdate extends BatchUpdate { GitRepositoryManager repoManager, @GerritPersonIdent PersonIdent serverIdent, ChangeNotes.Factory changeNotesFactory, - ChangeControl.GenericFactory changeControlFactory, ChangeUpdate.Factory changeUpdateFactory, NoteDbUpdateManager.Factory updateManagerFactory, ChangeIndexer indexer, @@ -287,7 +284,6 @@ class NoteDbBatchUpdate extends BatchUpdate { @Assisted Timestamp when) { super(repoManager, serverIdent, project, user, when); this.changeNotesFactory = changeNotesFactory; - this.changeControlFactory = changeControlFactory; this.changeUpdateFactory = changeUpdateFactory; this.updateManagerFactory = updateManagerFactory; this.indexer = indexer; @@ -445,8 +441,7 @@ class NoteDbBatchUpdate extends BatchUpdate { logDebug("Change {} is new", id); } ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c, !isNew); - ChangeControl ctl = changeControlFactory.controlFor(notes, user); - return new ChangeContextImpl(ctl); + return new ChangeContextImpl(notes); } private void executePostOps() throws Exception { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java index 3dbf0d255c..6835cb4017 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/update/ReviewDbBatchUpdate.java @@ -56,7 +56,6 @@ import com.google.gerrit.server.notedb.NoteDbChangeState.PrimaryStorage; import com.google.gerrit.server.notedb.NoteDbUpdateManager; import com.google.gerrit.server.notedb.NoteDbUpdateManager.MismatchedStateException; import com.google.gerrit.server.notedb.NotesMigration; -import com.google.gerrit.server.project.ChangeControl; import com.google.gerrit.server.util.RequestId; import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.SchemaFactory; @@ -165,7 +164,7 @@ class ReviewDbBatchUpdate extends BatchUpdate { } private class ChangeContextImpl extends ContextImpl implements ChangeContext { - private final ChangeControl ctl; + private final ChangeNotes notes; private final Map updates; private final ReviewDbWrapper dbWrapper; private final Repository threadLocalRepo; @@ -175,8 +174,8 @@ class ReviewDbBatchUpdate extends BatchUpdate { private boolean bumpLastUpdatedOn = true; protected ChangeContextImpl( - ChangeControl ctl, ReviewDbWrapper dbWrapper, Repository repo, RevWalk rw) { - this.ctl = ctl; + ChangeNotes notes, ReviewDbWrapper dbWrapper, Repository repo, RevWalk rw) { + this.notes = checkNotNull(notes); this.dbWrapper = dbWrapper; this.threadLocalRepo = repo; this.threadLocalRevWalk = rw; @@ -198,8 +197,8 @@ class ReviewDbBatchUpdate extends BatchUpdate { public ChangeUpdate getUpdate(PatchSet.Id psId) { ChangeUpdate u = updates.get(psId); if (u == null) { - u = changeUpdateFactory.create(ctl, when); - if (newChanges.containsKey(ctl.getId())) { + u = changeUpdateFactory.create(notes, user, when); + if (newChanges.containsKey(notes.getChangeId())) { u.setAllowWriteToNewRef(true); } u.setPatchSetId(psId); @@ -210,8 +209,7 @@ class ReviewDbBatchUpdate extends BatchUpdate { @Override public ChangeNotes getNotes() { - checkNotNull(ctl); - return ctl.getNotes(); + return notes; } @Override @@ -308,7 +306,6 @@ class ReviewDbBatchUpdate extends BatchUpdate { } private final AllUsersName allUsers; - private final ChangeControl.GenericFactory changeControlFactory; private final ChangeIndexer indexer; private final ChangeNotes.Factory changeNotesFactory; private final ChangeUpdate.Factory changeUpdateFactory; @@ -329,7 +326,6 @@ class ReviewDbBatchUpdate extends BatchUpdate { ReviewDbBatchUpdate( @GerritServerConfig Config cfg, AllUsersName allUsers, - ChangeControl.GenericFactory changeControlFactory, ChangeIndexer indexer, ChangeNotes.Factory changeNotesFactory, @ChangeUpdateExecutor ListeningExecutorService changeUpdateExector, @@ -347,7 +343,6 @@ class ReviewDbBatchUpdate extends BatchUpdate { @Assisted Timestamp when) { super(repoManager, serverIdent, project, user, when); this.allUsers = allUsers; - this.changeControlFactory = changeControlFactory; this.changeNotesFactory = changeNotesFactory; this.changeUpdateExector = changeUpdateExector; this.changeUpdateFactory = changeUpdateFactory; @@ -786,8 +781,7 @@ class ReviewDbBatchUpdate extends BatchUpdate { NoteDbChangeState.checkNotReadOnly(c, skewMs); } ChangeNotes notes = changeNotesFactory.createForBatchUpdate(c, !isNew); - ChangeControl ctl = changeControlFactory.controlFor(notes, user); - return new ChangeContextImpl(ctl, new BatchUpdateReviewDb(db), repo, rw); + return new ChangeContextImpl(notes, new BatchUpdateReviewDb(db), repo, rw); } private NoteDbUpdateManager stageNoteDbUpdate(ChangeContextImpl ctx, boolean deleted) 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 eadbaae3b9..a47a0e5374 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 @@ -15,7 +15,6 @@ package com.google.gerrit.testutil; import static com.google.common.base.MoreObjects.firstNonNull; -import static org.easymock.EasyMock.expect; import com.google.common.collect.Ordering; import com.google.gerrit.common.TimeUtil; @@ -33,12 +32,9 @@ 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; -import com.google.gerrit.server.project.ChangeControl; -import com.google.gwtorm.server.OrmException; import com.google.inject.Injector; import java.util.TimeZone; import java.util.concurrent.atomic.AtomicInteger; -import org.easymock.EasyMock; import org.eclipse.jgit.junit.TestRepository; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.PersonIdent; @@ -95,7 +91,8 @@ public class TestChanges { injector .getInstance(ChangeUpdate.Factory.class) .create( - stubChangeControl(injector.getInstance(AbstractChangeNotes.Args.class), c, user), + new ChangeNotes(injector.getInstance(AbstractChangeNotes.Args.class), c).load(), + user, TimeUtil.nowTs(), Ordering.natural()); @@ -129,19 +126,6 @@ public class TestChanges { } } - private static ChangeControl stubChangeControl( - 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(args, c).load(); - expect(ctl.getNotes()).andStubReturn(notes); - expect(ctl.getId()).andStubReturn(c.getId()); - EasyMock.replay(ctl); - return ctl; - } - public static void incrementPatchSet(Change change) { PatchSet.Id curr = change.currentPatchSetId(); PatchSetInfo ps =