opendev/gerrit
Code Issues Proposed changes

Create ChangeUpdates with ChangeControls

Browse Source 
This simplifies the factory methods, since ChangeControl ties a user
to a change. It also means all the factory methods can be used without
depending on an injected IdentifiedUser.

The only thing it complicates slightly is tests. ChangeControl is much
more than just a pair of (Change, User), so producing a fully-working
ChangeControl is infeasible. Instead, use EasyMock to create a stub
only exposing the getters we need. (This is slightly fragile but
probably no more so than tying ourselves to one set of injected
dependencies.)

Change-Id: I7c88d0e7f0e7ea7777adef201325e27810dc0ca7
This commit is contained in:
Dave Borowitz
2014-01-10 11:36:15 -08:00
parent 4242dfda15
commit 88dd894720
10 changed files with 52 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
View File

@@ -28,7 +28,6 @@ import com.google.gerrit.reviewdb.client.RevId;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.mail.CreateChangeSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
@@ -159,8 +158,9 @@ public class ChangeInserter {
public Change insert() throws OrmException, IOException {
ReviewDb db = dbProvider.get();
ChangeUpdate update = updateFactory.create(change, change.getCreatedOn(),
(IdentifiedUser) refControl.getCurrentUser());
ChangeUpdate update = updateFactory.create(
refControl.getProjectControl().controlFor(change),
change.getCreatedOn());
db.changes().beginTransaction(change.getId());
try {
ChangeUtil.insertAncestors(db, patchSet.getId(), commit);

2
gerrit-server/src/main/java/com/google/gerrit/server/change/DeleteReviewer.java
View File

@@ -72,7 +72,7 @@ public class DeleteReviewer implements RestModifyView<ReviewerResource, Input> {
ChangeControl control = rsrc.getControl();
Change.Id changeId = rsrc.getChange().getId();
ReviewDb db = dbProvider.get();
ChangeUpdate update = updateFactory.create(rsrc.getChange());
ChangeUpdate update = updateFactory.create(rsrc.getControl());
StringBuilder msg = new StringBuilder();
db.changes().beginTransaction(changeId);

3
gerrit-server/src/main/java/com/google/gerrit/server/change/PatchSetInserter.java
View File

@@ -225,8 +225,7 @@ public class PatchSetInserter {
final PatchSet.Id currentPatchSetId = c.currentPatchSetId();
ChangeUpdate update = updateFactory.create(c, patchSet.getCreatedOn(),
(IdentifiedUser) ctl.getCurrentUser());
ChangeUpdate update = updateFactory.create(ctl, patchSet.getCreatedOn());
db.changes().beginTransaction(c.getId());
try {

2
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReview.java
View File

@@ -135,7 +135,7 @@ public class PostReview implements RestModifyView<RevisionResource, ReviewInput>
ChangeUtil.updated(change);
timestamp = change.getLastUpdatedOn();
update = updateFactory.create(change, timestamp, revision.getUser());
update = updateFactory.create(revision.getControl(), timestamp);
dirty |= insertComments(revision, input.comments, input.drafts);
dirty |= updateLabels(revision, update, input.labels);
dirty |= insertMessage(revision, input.message);

2
gerrit-server/src/main/java/com/google/gerrit/server/change/PostReviewers.java
View File

@@ -221,7 +221,7 @@ public class PostReviewers implements RestModifyView<ChangeResource, AddReviewer
Map<Account.Id, ChangeControl> reviewers)
throws OrmException, EmailException, IOException {
ReviewDb db = dbProvider.get();
ChangeUpdate update = updateFactory.create(rsrc.getChange());
ChangeUpdate update = updateFactory.create(rsrc.getControl());
List<PatchSetApproval> added;
db.changes().beginTransaction(rsrc.getChange().getId());
try {

2
gerrit-server/src/main/java/com/google/gerrit/server/change/Publish.java
View File

@@ -76,7 +76,7 @@ public class Publish implements RestModifyView<RevisionResource, Input>,
PatchSet updatedPatchSet = updateDraftPatchSet(rsrc);
Change updatedChange = updateDraftChange(rsrc);
ChangeUpdate update = updateFactory.create(rsrc.getChange(),
ChangeUpdate update = updateFactory.create(rsrc.getControl(),
updatedChange.getLastUpdatedOn());
try {

2
gerrit-server/src/main/java/com/google/gerrit/server/change/Submit.java
View File

@@ -191,7 +191,7 @@ public class Submit implements RestModifyView<RevisionResource, SubmitInput>,
throws OrmException, IOException {
final Timestamp timestamp = TimeUtil.nowTs();
Change change = rsrc.getChange();
ChangeUpdate update = updateFactory.create(change, timestamp, caller);
ChangeUpdate update = updateFactory.create(rsrc.getControl(), timestamp);
ReviewDb db = dbProvider.get();
db.changes().beginTransaction(change.getId());
try {

2
gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
View File

@@ -1842,7 +1842,7 @@ public class ReceiveCommits {
recipients.add(getRecipientsFromFooters(accountResolver, newPatchSet, footerLines));
recipients.remove(me);
ChangeUpdate update = updateFactory.create(change, newPatchSet.getCreatedOn());
ChangeUpdate update = updateFactory.create(changeCtl, newPatchSet.getCreatedOn());
db.changes().beginTransaction(change.getId());
try {
change = db.changes().get(change.getId());

63
gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java
View File

@@ -27,12 +27,15 @@ import com.google.gerrit.common.data.LabelTypes;
import com.google.gerrit.reviewdb.client.Account;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.VersionedMetaData;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.util.LabelVote;
import com.google.inject.assistedinject.Assisted;
@@ -63,9 +66,8 @@ import java.util.TimeZone;
*/
public class ChangeUpdate extends VersionedMetaData {
public interface Factory {
ChangeUpdate create(Change change);
ChangeUpdate create(Change change, Date when);
ChangeUpdate create(Change change, Date when, IdentifiedUser user);
ChangeUpdate create(ChangeControl ctl);
ChangeUpdate create(ChangeControl ctl, Date when);
}
private final NotesMigration migration;
@@ -73,8 +75,7 @@ public class ChangeUpdate extends VersionedMetaData {
private final AccountCache accountCache;
private final MetaDataUpdate.User updateFactory;
private final LabelTypes labelTypes;
private final Change change;
private final IdentifiedUser user;
private final ChangeControl ctl;
private final Date when;
private final TimeZone tz;
private final Map<String, Short> approvals;
@@ -90,10 +91,9 @@ public class ChangeUpdate extends VersionedMetaData {
AccountCache accountCache,
MetaDataUpdate.User updateFactory,
ProjectCache projectCache,
IdentifiedUser user,
@Assisted Change change) {
@Assisted ChangeControl ctl) {
this(serverIdent, repoManager, migration, accountCache, updateFactory,
projectCache, user, change, serverIdent.getWhen());
projectCache, ctl, serverIdent.getWhen());
}
@AssistedInject
@@ -104,27 +104,15 @@ public class ChangeUpdate extends VersionedMetaData {
AccountCache accountCache,
MetaDataUpdate.User updateFactory,
ProjectCache projectCache,
IdentifiedUser user,
@Assisted Change change,
@Assisted ChangeControl ctl,
@Assisted Date when) {
this(serverIdent, repoManager, migration, accountCache, updateFactory,
projectCache, change, when, user);
projectCache.get(getProjectName(ctl)).getLabelTypes(),
ctl, when);
}
@AssistedInject
ChangeUpdate(
@GerritPersonIdent PersonIdent serverIdent,
GitRepositoryManager repoManager,
NotesMigration migration,
AccountCache accountCache,
MetaDataUpdate.User updateFactory,
ProjectCache projectCache,
@Assisted Change change,
@Assisted Date when,
@Assisted IdentifiedUser user) {
this(serverIdent, repoManager, migration, accountCache, updateFactory,
projectCache.get(change.getDest().getParentKey()).getLabelTypes(),
change, when, user);
private static Project.NameKey getProjectName(ChangeControl ctl) {
return ctl.getChange().getDest().getParentKey();
}
@VisibleForTesting
@@ -135,16 +123,14 @@ public class ChangeUpdate extends VersionedMetaData {
AccountCache accountCache,
MetaDataUpdate.User updateFactory,
LabelTypes labelTypes,
Change change,
Date when,
IdentifiedUser user) {
ChangeControl ctl,
Date when) {
this.repoManager = repoManager;
this.migration = migration;
this.accountCache = accountCache;
this.updateFactory = updateFactory;
this.labelTypes = labelTypes;
this.change = change;
this.user = user;
this.ctl = ctl;
this.when = when;
this.tz = serverIdent.getTimeZone();
this.approvals = Maps.newTreeMap(labelTypes.nameComparator());
@@ -152,11 +138,11 @@ public class ChangeUpdate extends VersionedMetaData {
}
public Change getChange() {
return change;
return ctl.getChange();
}
public IdentifiedUser getUser() {
return user;
return (IdentifiedUser) ctl.getCurrentUser();
}
public Date getWhen() {
@@ -172,7 +158,8 @@ public class ChangeUpdate extends VersionedMetaData {
}
public void setPatchSetId(PatchSet.Id psId) {
checkArgument(psId == null || psId.getParentKey().equals(change.getKey()));
checkArgument(psId == null
|| psId.getParentKey().equals(getChange().getKey()));
this.psId = psId;
}
@@ -187,7 +174,7 @@ public class ChangeUpdate extends VersionedMetaData {
public RevCommit commit() throws IOException {
return commit(checkNotNull(updateFactory, "MetaDataUpdate.Factory")
.create(change.getProject(), user));
.create(getChange().getProject(), getUser()));
}
@Override
@@ -195,7 +182,7 @@ public class ChangeUpdate extends VersionedMetaData {
if (!migration.write()) {
return null;
}
Repository repo = repoManager.openRepository(change.getProject());
Repository repo = repoManager.openRepository(getChange().getProject());
try {
load(repo);
} catch (ConfigInvalidException e) {
@@ -218,7 +205,7 @@ public class ChangeUpdate extends VersionedMetaData {
}
public PersonIdent newCommitter() {
return newIdent(user.getAccount());
return newIdent(getUser().getAccount());
}
@Override
@@ -261,7 +248,7 @@ public class ChangeUpdate extends VersionedMetaData {
@Override
protected String getRefName() {
return ChangeNoteUtil.changeRefName(change.getId());
return ChangeNoteUtil.changeRefName(getChange().getId());
}
@Override
@@ -269,7 +256,7 @@ public class ChangeUpdate extends VersionedMetaData {
if (approvals.isEmpty() && reviewers.isEmpty()) {
return false;
}
int ps = psId != null ? psId.get() : change.currentPatchSetId().get();
int ps = psId != null ? psId.get() : getChange().currentPatchSetId().get();
StringBuilder msg = new StringBuilder();
if (subject != null) {
msg.append(subject);

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

@@ -19,10 +19,13 @@ import static com.google.gerrit.server.notedb.ReviewerState.REVIEWER;
import static com.google.gerrit.server.project.Util.category;
import static com.google.gerrit.server.project.Util.value;
import static com.google.inject.Scopes.SINGLETON;
import static java.util.concurrent.TimeUnit.DAYS;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.MINUTES;
import static java.util.concurrent.TimeUnit.SECONDS;
import static org.easymock.EasyMock.expect;
import static org.junit.Assert.assertEquals;
import com.google.common.collect.ImmutableList;
@@ -50,6 +53,7 @@ import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.util.TimeUtil;
import com.google.gerrit.testutil.FakeAccountCache;
import com.google.gerrit.testutil.FakeRealm;
@@ -60,7 +64,7 @@ import com.google.inject.Guice;
import com.google.inject.Injector;
import com.google.inject.util.Providers;
import org.eclipse.jgit.errors.ConfigInvalidException;
import org.easymock.EasyMock;
import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.PersonIdent;
@@ -414,10 +418,11 @@ public class ChangeNotesTest {
}
private ChangeUpdate newUpdate(Change c, IdentifiedUser user)
throws ConfigInvalidException, IOException {
throws Exception {
return new ChangeUpdate(SERVER_IDENT, repoManager,
NotesMigration.allEnabled(), accountCache, null, LABEL_TYPES, c,
TimeUtil.nowTs(), user);
NotesMigration.allEnabled(), accountCache, null, LABEL_TYPES,
stubChangeControl(c, user),
TimeUtil.nowTs());
}
private ChangeNotes newNotes(Change c) throws OrmException {
@@ -447,4 +452,12 @@ public class ChangeNotesTest {
update.getUser().newCommitterIdent(update.getWhen(), TZ));
return update.commit(md);
}
private ChangeControl stubChangeControl(Change c, IdentifiedUser user) {
ChangeControl ctl = EasyMock.createNiceMock(ChangeControl.class);
expect(ctl.getChange()).andStubReturn(c);
expect(ctl.getCurrentUser()).andStubReturn(user);
EasyMock.replay(ctl);
return ctl;
}
}