opendev/gerrit
Code Issues Proposed changes

Convert PatchSetInserter to use a builder pattern

Browse Source 
Instead of multiplying methods based on various parameters, store the
parameters as instance variables in the PatchSetInserter object, and
use a single insert() method with no arguments to do the insertion.
Writing setter methods for each of the "optional arguments" increases
boilerplate slightly, but allows us to do per-argument validation in a
cleanly separated way.

Change-Id: I694c8a1f381d6ba01a31acf904802ea99e8f50e8
This commit is contained in:
Dave Borowitz
2013-05-15 08:19:39 -07:00
parent e539c87f09
commit d838b4355d
5 changed files with 87 additions and 45 deletions
Download Patch File Download Diff File Expand all files Collapse all files

12
gerrit-httpd/src/main/java/com/google/gerrit/httpd/rpc/changedetail/EditCommitMessageHandler.java
View File

@@ -25,7 +25,6 @@ import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.PatchSetInserter;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.mail.CommitMessageEditedSender;
@@ -65,7 +64,7 @@ class EditCommitMessageHandler extends Handler<ChangeDetail> {
private final CommitValidators.Factory commitValidatorsFactory;
private final GitRepositoryManager gitManager;
private final PersonIdent myIdent;
private final PatchSetInserter patchSetInserter;
private final PatchSetInserter.Factory patchSetInserterFactory;
@Inject
EditCommitMessageHandler(final ChangeControl.Factory changeControlFactory,
@@ -77,7 +76,7 @@ class EditCommitMessageHandler extends Handler<ChangeDetail> {
final CommitValidators.Factory commitValidatorsFactory,
final GitRepositoryManager gitManager,
@GerritPersonIdent final PersonIdent myIdent,
final PatchSetInserter patchSetInserter) {
final PatchSetInserter.Factory patchSetInserterFactory) {
this.changeControlFactory = changeControlFactory;
this.db = db;
this.currentUser = currentUser;
@@ -88,7 +87,7 @@ class EditCommitMessageHandler extends Handler<ChangeDetail> {
this.commitValidatorsFactory = commitValidatorsFactory;
this.gitManager = gitManager;
this.myIdent = myIdent;
this.patchSetInserter = patchSetInserter;
this.patchSetInserterFactory = patchSetInserterFactory;
}
@Override
@@ -114,8 +113,9 @@ class EditCommitMessageHandler extends Handler<ChangeDetail> {
CommitValidators commitValidators =
commitValidatorsFactory.create(control.getRefControl(), new NoSshInfo(), git);
ChangeUtil.editCommitMessage(patchSetId, control.getRefControl(), commitValidators, currentUser, message, db,
commitMessageEditedSenderFactory, git, myIdent, patchSetInserter);
ChangeUtil.editCommitMessage(patchSetId, control.getRefControl(),
commitValidators, currentUser, message, db,
commitMessageEditedSenderFactory, git, myIdent, patchSetInserterFactory);
return changeDetailFactory.create(changeId).call();
} finally {

11
gerrit-server/src/main/java/com/google/gerrit/server/ChangeUtil.java
View File

@@ -314,7 +314,8 @@ public class ChangeUtil {
final RefControl refControl, CommitValidators commitValidators,
final IdentifiedUser user, final String message, final ReviewDb db,
final CommitMessageEditedSender.Factory commitMessageEditedSenderFactory,
Repository git, PersonIdent myIdent, PatchSetInserter patchSetInserter)
Repository git, PersonIdent myIdent,
PatchSetInserter.Factory patchSetInserterFactory)
throws NoSuchChangeException, EmailException, OrmException,
MissingObjectException, IncorrectObjectTypeException, IOException,
InvalidChangeOperationException, PatchSetInfoNotAvailableException {
@@ -384,8 +385,12 @@ public class ChangeUtil {
"Patch Set " + newPatchSet.getPatchSetId()
+ ": Commit message was updated";
change = patchSetInserter.insertPatchSet(git, revWalk, change,
newPatchSet, newCommit, refControl, msg, true);
change = patchSetInserterFactory
.create(git, revWalk, refControl, change, newCommit)
.setPatchSet(newPatchSet)
.setMessage(msg)
.setCopyLabels(true)
.insert();
return change.getId();
} finally {

14
gerrit-server/src/main/java/com/google/gerrit/server/change/CherryPickChange.java
View File

@@ -74,7 +74,7 @@ public class CherryPickChange {
private final IdentifiedUser currentUser;
private final CommitValidators.Factory commitValidatorsFactory;
private final ChangeInserter changeInserter;
private final PatchSetInserter patchSetInserter;
private final PatchSetInserter.Factory patchSetInserterFactory;
final MergeUtil.Factory mergeUtilFactory;
@Inject
@@ -83,7 +83,7 @@ public class CherryPickChange {
final GitRepositoryManager gitManager, final IdentifiedUser currentUser,
final CommitValidators.Factory commitValidatorsFactory,
final ChangeInserter changeInserter,
final PatchSetInserter patchSetInserter,
final PatchSetInserter.Factory patchSetInserterFactory,
final MergeUtil.Factory mergeUtilFactory) {
this.patchSetInfoFactory = patchSetInfoFactory;
this.db = db;
@@ -92,7 +92,7 @@ public class CherryPickChange {
this.currentUser = currentUser;
this.commitValidatorsFactory = commitValidatorsFactory;
this.changeInserter = changeInserter;
this.patchSetInserter = patchSetInserter;
this.patchSetInserterFactory = patchSetInserterFactory;
this.mergeUtilFactory = mergeUtilFactory;
}
@@ -223,9 +223,11 @@ public class CherryPickChange {
} catch (CommitValidationException e) {
throw new InvalidChangeOperationException(e.getMessage());
}
patchSetInserter.insertPatchSet(git, revWalk, change, newPatchSet,
cherryPickCommit, refControl, buildChangeMessage(patchSetId, change),
false);
patchSetInserterFactory
.create(git, revWalk, refControl, change, cherryPickCommit)
.setPatchSet(newPatchSet)
.setMessage(buildChangeMessage(patchSetId, change))
.insert();
return change.getId();
}

1
gerrit-server/src/main/java/com/google/gerrit/server/change/Module.java
View File

@@ -93,6 +93,7 @@ public class Module extends RestApiModule {
factory(ReviewerResource.Factory.class);
factory(AccountInfo.Loader.Factory.class);
factory(EmailReviewComments.Factory.class);
factory(PatchSetInserter.Factory.class);
}
});
}

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

@@ -14,6 +14,9 @@
package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkState;
import com.google.gerrit.common.ChangeHooks;
import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.ChangeMessage;
@@ -26,11 +29,11 @@ import com.google.gerrit.server.config.TrackingFooters;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.RefControl;
import com.google.gwtorm.server.AtomicUpdate;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.RefUpdate;
@@ -44,6 +47,11 @@ import java.util.Collections;
import java.util.List;
public class PatchSetInserter {
public static interface Factory {
PatchSetInserter create(Repository git, RevWalk revWalk, RefControl refControl,
Change change, RevCommit commit);
}
private final ChangeHooks hooks;
private final TrackingFooters trackingFooters;
private final PatchSetInfoFactory patchSetInfoFactory;
@@ -51,40 +59,75 @@ public class PatchSetInserter {
private final IdentifiedUser user;
private final GitReferenceUpdated gitRefUpdated;
private final Repository git;
private final RevWalk revWalk;
private final RevCommit commit;
private final Change change;
private final RefControl refControl;
private PatchSet patchSet;
private ChangeMessage changeMessage;
private boolean copyLabels;
@Inject
public PatchSetInserter(ChangeHooks hooks,
TrackingFooters trackingFooters,
ReviewDb db,
PatchSetInfoFactory patchSetInfoFactory,
IdentifiedUser user,
GitReferenceUpdated gitRefUpdated) {
GitReferenceUpdated gitRefUpdated,
@Assisted Repository git,
@Assisted RevWalk revWalk,
@Assisted RefControl refControl,
@Assisted Change change,
@Assisted RevCommit commit) {
this.hooks = hooks;
this.trackingFooters = trackingFooters;
this.db = db;
this.patchSetInfoFactory = patchSetInfoFactory;
this.user = user;
this.gitRefUpdated = gitRefUpdated;
this.git = git;
this.revWalk = revWalk;
this.refControl = refControl;
this.change = change;
this.commit = commit;
}
public Change insertPatchSet(Repository git, RevWalk revWalk,
Change change, PatchSet patchSet, RevCommit commit, RefControl refControl,
String message, boolean copyLabels) throws OrmException,
InvalidChangeOperationException, NoSuchChangeException, IOException {
final ChangeMessage cmsg =
new ChangeMessage(new ChangeMessage.Key(change.getId(),
ChangeUtil.messageUUID(db)), user.getAccountId(), patchSet.getId());
cmsg.setMessage(message);
return insertPatchSet(git, revWalk, change, patchSet, commit, refControl,
cmsg, copyLabels);
public PatchSetInserter setPatchSet(PatchSet patchSet) {
PatchSet.Id psid = patchSet.getId();
checkArgument(psid.getParentKey().equals(change.getId()),
"patch set %s not for change %s", psid, change.getId());
checkArgument(psid.get() > change.currentPatchSetId().get(),
"new patch set ID %s is not greater than current patch set ID %s",
psid.get(), change.currentPatchSetId().get());
this.patchSet = patchSet;
return this;
}
public Change insertPatchSet(Repository git, RevWalk revWalk,
Change change, final PatchSet patchSet, final RevCommit commit,
RefControl refControl, ChangeMessage changeMessage, boolean copyLabels)
throws OrmException, InvalidChangeOperationException,
NoSuchChangeException, IOException {
public PatchSetInserter setMessage(String message) throws OrmException {
changeMessage = new ChangeMessage(new ChangeMessage.Key(change.getId(),
ChangeUtil.messageUUID(db)), user.getAccountId(), patchSet.getId());
changeMessage.setMessage(message);
return this;
}
public PatchSetInserter setMessage(ChangeMessage changeMessage) throws OrmException {
this.changeMessage = changeMessage;
return this;
}
public PatchSetInserter setCopyLabels(boolean copyLabels) {
this.copyLabels = copyLabels;
return this;
}
public Change insert() throws InvalidChangeOperationException, OrmException,
IOException {
checkState(patchSet != null, "patch set not set");
Change updatedChange;
RefUpdate ru = git.updateRef(patchSet.getRefName());
ru.setExpectedOldObjectId(ObjectId.zeroId());
ru.setNewObjectId(commit);
@@ -98,13 +141,6 @@ public class PatchSetInserter {
final PatchSet.Id currentPatchSetId = change.currentPatchSetId();
if (patchSet.getId().get() <= currentPatchSetId.get()) {
throw new InvalidChangeOperationException("New Patch Set ID ["
+ patchSet.getId().get()
+ "] is not greater than the current Patch Set ID ["
+ currentPatchSetId.get() + "]");
}
db.changes().beginTransaction(change.getId());
try {
if (!db.changes().get(change.getId()).getStatus().isOpen()) {
@@ -115,7 +151,7 @@ public class PatchSetInserter {
ChangeUtil.insertAncestors(db, patchSet.getId(), commit);
db.patchSets().insert(Collections.singleton(patchSet));
Change updatedChange =
updatedChange =
db.changes().atomicUpdate(change.getId(), new AtomicUpdate<Change>() {
@Override
public Change update(Change change) {
@@ -135,9 +171,7 @@ public class PatchSetInserter {
return change;
}
});
if (updatedChange != null) {
change = updatedChange;
} else {
if (updatedChange == null) {
throw new ChangeModifiedException(String.format(
"Change %s was modified", change.getId()));
}
@@ -159,7 +193,7 @@ public class PatchSetInserter {
} finally {
db.rollback();
}
return change;
return updatedChange;
}
public class ChangeModifiedException extends InvalidChangeOperationException {