Validate new commits in ChangeInserter rather than callers

We need to expose an explicit validation step because this needs to
happen before the ref updates, which are still managed by callers (but
won't be for long).

The one exception is ReceiveCommits, but it can just explicitly set
the validation policy to NONE.

Change-Id: I9cf6c5d21aacb6e4fc4cba28bdc4f4316c336961
This commit is contained in:
Dave Borowitz
2015-10-08 12:46:49 -04:00
parent 4cebfa4b58
commit bb0b7f833c
8 changed files with 117 additions and 101 deletions

View File

@@ -39,7 +39,9 @@ import com.google.gerrit.server.change.PostReviewers;
import com.google.gerrit.server.config.AllProjectsNameProvider;
import com.google.gerrit.server.git.MetaDataUpdate;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.group.SystemGroupBackend;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.SetParent;
@@ -50,6 +52,7 @@ import com.google.inject.assistedinject.Assisted;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import java.io.IOException;
import java.util.List;
@@ -120,9 +123,14 @@ public class ReviewProjectAccess extends ProjectAccessHandler<Change.Id> {
config.getProject().getNameKey(),
RefNames.REFS_CONFIG),
TimeUtil.nowTs());
ChangeInserter ins =
changeInserterFactory.create(ctl, change, commit);
ins.insert();
try (RevWalk rw = new RevWalk(md.getRepository())) {
ChangeInserter ins = changeInserterFactory.create(
md.getRepository(), rw, ctl, change, commit)
.setValidatePolicy(CommitValidators.Policy.NONE);
ins.insert();
} catch (InvalidChangeOperationException e) {
throw new IOException(e);
}
ChangeResource rsrc;
try {

View File

@@ -31,10 +31,8 @@ import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.change.ChangeInserter;
import com.google.gerrit.server.change.ChangeMessages;
import com.google.gerrit.server.change.ChangeTriplet;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.mail.RevertedSender;
@@ -43,9 +41,7 @@ import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.ssh.SshInfo;
import com.google.gerrit.server.util.IdGenerator;
import com.google.gerrit.server.util.MagicBranch;
import com.google.gwtorm.server.OrmConcurrencyException;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
@@ -193,7 +189,6 @@ public class ChangeUtil {
}
private final Provider<CurrentUser> userProvider;
private final CommitValidators.Factory commitValidatorsFactory;
private final Provider<ReviewDb> db;
private final Provider<InternalChangeQuery> queryProvider;
private final RevertedSender.Factory revertedSenderFactory;
@@ -204,7 +199,6 @@ public class ChangeUtil {
@Inject
ChangeUtil(Provider<CurrentUser> userProvider,
CommitValidators.Factory commitValidatorsFactory,
Provider<ReviewDb> db,
Provider<InternalChangeQuery> queryProvider,
RevertedSender.Factory revertedSenderFactory,
@@ -213,7 +207,6 @@ public class ChangeUtil {
GitReferenceUpdated gitRefUpdated,
ChangeIndexer indexer) {
this.userProvider = userProvider;
this.commitValidatorsFactory = commitValidatorsFactory;
this.db = db;
this.queryProvider = queryProvider;
this.revertedSenderFactory = revertedSenderFactory;
@@ -224,7 +217,7 @@ public class ChangeUtil {
}
public Change.Id revert(ChangeControl ctl, PatchSet.Id patchSetId,
String message, PersonIdent myIdent, SshInfo sshInfo)
String message, PersonIdent myIdent)
throws NoSuchChangeException, OrmException,
MissingObjectException, IncorrectObjectTypeException, IOException,
InvalidChangeOperationException {
@@ -280,26 +273,12 @@ public class ChangeUtil {
changeToRevert.getDest(),
TimeUtil.nowTs());
change.setTopic(changeToRevert.getTopic());
ChangeInserter ins =
changeInserterFactory.create(refControl.getProjectControl(),
change, revertCommit);
ChangeInserter ins = changeInserterFactory.create(
git, revWalk, refControl.getProjectControl(), change, revertCommit)
.setValidatePolicy(CommitValidators.Policy.GERRIT);
ins.validate();
PatchSet ps = ins.getPatchSet();
String ref = refControl.getRefName();
String cmdRef = MagicBranch.NEW_PUBLISH_CHANGE
+ ref.substring(ref.lastIndexOf('/') + 1);
CommitReceivedEvent commitReceivedEvent = new CommitReceivedEvent(
new ReceiveCommand(ObjectId.zeroId(), revertCommit.getId(), cmdRef),
refControl.getProjectControl().getProject(),
refControl.getRefName(), revertCommit, user());
try {
commitValidatorsFactory.create(refControl, sshInfo, git)
.validateForGerritCommits(commitReceivedEvent);
} catch (CommitValidationException e) {
throw new InvalidChangeOperationException(e.getMessage());
}
RefUpdate ru = git.updateRef(ps.getRefName());
ru.setExpectedOldObjectId(ObjectId.zeroId());
ru.setNewObjectId(revertCommit);

View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.change;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.gerrit.reviewdb.client.Change.INITIAL_PATCH_SET_ID;
@@ -34,15 +35,22 @@ import com.google.gerrit.server.ChangeMessagesUtil;
import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.account.AccountCache;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.BanCommit;
import com.google.gerrit.server.git.GroupCollector;
import com.google.gerrit.server.git.WorkQueue;
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.index.ChangeIndexer;
import com.google.gerrit.server.mail.CreateChangeSender;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectControl;
import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.ssh.NoSshInfo;
import com.google.gerrit.server.util.RequestScopePropagator;
import com.google.gerrit.server.validators.ValidationException;
import com.google.gwtorm.server.OrmException;
@@ -51,7 +59,11 @@ import com.google.inject.Provider;
import com.google.inject.assistedinject.Assisted;
import org.eclipse.jgit.lib.ObjectId;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.notes.NoteMap;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
@@ -62,7 +74,8 @@ import java.util.Set;
public class ChangeInserter {
public static interface Factory {
ChangeInserter create(ProjectControl ctl, Change c, RevCommit rc);
ChangeInserter create(Repository git, RevWalk revWalk, ProjectControl ctl,
Change c, RevCommit rc);
}
private static final Logger log =
@@ -79,8 +92,11 @@ public class ChangeInserter {
private final HashtagsUtil hashtagsUtil;
private final AccountCache accountCache;
private final WorkQueue workQueue;
private final CommitValidators.Factory commitValidatorsFactory;
private final ProjectControl projectControl;
private final Repository git;
private final RevWalk revWalk;
private final RefControl refControl;
private final IdentifiedUser user;
private final Change change;
private final PatchSet patchSet;
@@ -89,6 +105,8 @@ public class ChangeInserter {
// Fields exposed as setters.
private String message;
private CommitValidators.Policy validatePolicy =
CommitValidators.Policy.GERRIT;
private Set<Account.Id> reviewers;
private Set<Account.Id> extraCC;
private Map<String, Short> approvals;
@@ -99,6 +117,7 @@ public class ChangeInserter {
// Fields set during the insertion process.
private ChangeMessage changeMessage;
private boolean validated; // TODO(dborowitz): Remove; see validate().
@Inject
ChangeInserter(Provider<ReviewDb> dbProvider,
@@ -113,6 +132,9 @@ public class ChangeInserter {
HashtagsUtil hashtagsUtil,
AccountCache accountCache,
WorkQueue workQueue,
CommitValidators.Factory commitValidatorsFactory,
@Assisted Repository git,
@Assisted RevWalk revWalk,
@Assisted ProjectControl projectControl,
@Assisted Change change,
@Assisted RevCommit commit) {
@@ -127,7 +149,11 @@ public class ChangeInserter {
this.hashtagsUtil = hashtagsUtil;
this.accountCache = accountCache;
this.workQueue = workQueue;
this.projectControl = projectControl;
this.commitValidatorsFactory = commitValidatorsFactory;
this.git = git;
this.revWalk = revWalk;
this.refControl = projectControl.controlForRef(change.getDest());
this.change = change;
this.commit = commit;
this.reviewers = Collections.emptySet();
@@ -162,6 +188,11 @@ public class ChangeInserter {
return this;
}
public ChangeInserter setValidatePolicy(CommitValidators.Policy validate) {
this.validatePolicy = checkNotNull(validate);
return this;
}
public ChangeInserter setReviewers(Set<Account.Id> reviewers) {
this.reviewers = reviewers;
return this;
@@ -225,8 +256,12 @@ public class ChangeInserter {
return changeMessage;
}
public Change insert() throws OrmException, IOException {
public Change insert()
throws OrmException, IOException, InvalidChangeOperationException {
validate();
ReviewDb db = dbProvider.get();
ProjectControl projectControl = refControl.getProjectControl();
ChangeControl ctl = projectControl.controlFor(change);
ChangeUpdate update = updateFactory.create(
ctl,
@@ -314,4 +349,43 @@ public class ChangeInserter {
return change;
}
// TODO(dborowitz): This is only public because callers expect validation to
// happen before updating any refs, and they are still updating refs manually.
// Make private once we have migrated ref updates into this class.
public void validate() throws IOException, InvalidChangeOperationException {
if (validated || validatePolicy == CommitValidators.Policy.NONE) {
return;
}
CommitValidators cv =
commitValidatorsFactory.create(refControl, new NoSshInfo(), git);
String refName = patchSet.getId().toRefName();
CommitReceivedEvent event = new CommitReceivedEvent(
new ReceiveCommand(
ObjectId.zeroId(),
commit.getId(),
refName),
refControl.getProjectControl().getProject(),
refControl.getRefName(),
commit,
user);
try {
switch (validatePolicy) {
case RECEIVE_COMMITS:
NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(git, revWalk);
cv.validateForReceiveCommits(event, rejectCommits);
break;
case GERRIT:
cv.validateForGerritCommits(event);
break;
case NONE:
break;
}
} catch (CommitValidationException e) {
throw new InvalidChangeOperationException(e.getMessage());
}
validated = true;
}
}

View File

@@ -30,7 +30,6 @@ import com.google.gerrit.server.ChangeUtil;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.CodeReviewCommit;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
@@ -40,7 +39,6 @@ import com.google.gerrit.server.git.MergeException;
import com.google.gerrit.server.git.MergeIdenticalTreeException;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.project.ChangeControl;
@@ -50,7 +48,6 @@ import com.google.gerrit.server.project.ProjectState;
import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.query.change.ChangeData;
import com.google.gerrit.server.query.change.InternalChangeQuery;
import com.google.gerrit.server.ssh.NoSshInfo;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -66,7 +63,6 @@ import org.eclipse.jgit.lib.Ref;
import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
import org.eclipse.jgit.util.ChangeIdUtil;
import java.io.IOException;
@@ -81,7 +77,6 @@ public class CherryPickChange {
private final GitRepositoryManager gitManager;
private final TimeZone serverTimeZone;
private final Provider<CurrentUser> currentUser;
private final CommitValidators.Factory commitValidatorsFactory;
private final ChangeInserter.Factory changeInserterFactory;
private final PatchSetInserter.Factory patchSetInserterFactory;
private final MergeUtil.Factory mergeUtilFactory;
@@ -95,7 +90,6 @@ public class CherryPickChange {
@GerritPersonIdent PersonIdent myIdent,
GitRepositoryManager gitManager,
Provider<CurrentUser> currentUser,
CommitValidators.Factory commitValidatorsFactory,
ChangeInserter.Factory changeInserterFactory,
PatchSetInserter.Factory patchSetInserterFactory,
MergeUtil.Factory mergeUtilFactory,
@@ -107,7 +101,6 @@ public class CherryPickChange {
this.gitManager = gitManager;
this.serverTimeZone = myIdent.getTimeZone();
this.currentUser = currentUser;
this.commitValidatorsFactory = commitValidatorsFactory;
this.changeInserterFactory = changeInserterFactory;
this.patchSetInserterFactory = patchSetInserterFactory;
this.mergeUtilFactory = mergeUtilFactory;
@@ -247,25 +240,13 @@ public class CherryPickChange {
identifiedUser.getAccountId(), new Branch.NameKey(project,
destRef.getName()), TimeUtil.nowTs());
change.setTopic(topic);
ChangeInserter ins =
changeInserterFactory.create(refControl.getProjectControl(), change,
cherryPickCommit);
ChangeInserter ins = changeInserterFactory.create(
git, revWalk, refControl.getProjectControl(), change,
cherryPickCommit)
.setValidatePolicy(CommitValidators.Policy.GERRIT);
ins.validate();
PatchSet newPatchSet = ins.getPatchSet();
CommitValidators commitValidators =
commitValidatorsFactory.create(refControl, new NoSshInfo(), git);
CommitReceivedEvent commitReceivedEvent =
new CommitReceivedEvent(new ReceiveCommand(ObjectId.zeroId(),
cherryPickCommit.getId(), newPatchSet.getRefName()), refControl
.getProjectControl().getProject(), refControl.getRefName(),
cherryPickCommit, identifiedUser);
try {
commitValidators.validateForGerritCommits(commitReceivedEvent);
} catch (CommitValidationException e) {
throw new InvalidChangeOperationException(e.getMessage());
}
final RefUpdate ru = git.updateRef(newPatchSet.getRefName());
ru.setExpectedOldObjectId(ObjectId.zeroId());
ru.setNewObjectId(cherryPickCommit);

View File

@@ -41,15 +41,12 @@ import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.events.CommitReceivedEvent;
import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.validators.CommitValidationException;
import com.google.gerrit.server.git.validators.CommitValidators;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.ProjectResource;
import com.google.gerrit.server.project.ProjectsCollection;
import com.google.gerrit.server.project.RefControl;
import com.google.gerrit.server.ssh.NoSshInfo;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Provider;
@@ -65,7 +62,6 @@ import org.eclipse.jgit.lib.RefUpdate;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevCommit;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
import org.eclipse.jgit.util.ChangeIdUtil;
import java.io.IOException;
@@ -83,7 +79,6 @@ public class CreateChange implements
private final TimeZone serverTimeZone;
private final Provider<CurrentUser> userProvider;
private final ProjectsCollection projectsCollection;
private final CommitValidators.Factory commitValidatorsFactory;
private final ChangeInserter.Factory changeInserterFactory;
private final ChangeJson.Factory jsonFactory;
private final ChangeUtil changeUtil;
@@ -95,7 +90,6 @@ public class CreateChange implements
@GerritPersonIdent PersonIdent myIdent,
Provider<CurrentUser> userProvider,
ProjectsCollection projectsCollection,
CommitValidators.Factory commitValidatorsFactory,
ChangeInserter.Factory changeInserterFactory,
ChangeJson.Factory json,
ChangeUtil changeUtil,
@@ -105,7 +99,6 @@ public class CreateChange implements
this.serverTimeZone = myIdent.getTimeZone();
this.userProvider = userProvider;
this.projectsCollection = projectsCollection;
this.commitValidatorsFactory = commitValidatorsFactory;
this.changeInserterFactory = changeInserterFactory;
this.jsonFactory = json;
this.changeUtil = changeUtil;
@@ -203,12 +196,13 @@ public class CreateChange implements
new Branch.NameKey(project, refName),
now);
ChangeInserter ins = changeInserterFactory
.create(refControl.getProjectControl(), change, c);
ChangeInserter ins = changeInserterFactory.create(
git, rw, refControl.getProjectControl(), change, c)
.setValidatePolicy(CommitValidators.Policy.GERRIT);
ins.setMessage(String.format("Uploaded patch set %s.",
ins.getPatchSet().getPatchSetId()));
ins.validate();
validateCommit(git, refControl, c, me, ins);
updateRef(git, rw, c, change, ins.getPatchSet());
String topic = input.topic;
@@ -225,29 +219,6 @@ public class CreateChange implements
}
}
private void validateCommit(Repository git, RefControl refControl,
RevCommit c, IdentifiedUser me, ChangeInserter ins)
throws ResourceConflictException {
PatchSet newPatchSet = ins.getPatchSet();
CommitValidators commitValidators =
commitValidatorsFactory.create(refControl, new NoSshInfo(), git);
CommitReceivedEvent commitReceivedEvent =
new CommitReceivedEvent(new ReceiveCommand(
ObjectId.zeroId(),
c.getId(),
newPatchSet.getRefName()),
refControl.getProjectControl().getProject(),
refControl.getRefName(),
c,
me);
try {
commitValidators.validateForGerritCommits(commitReceivedEvent);
} catch (CommitValidationException e) {
throw new ResourceConflictException(e.getMessage());
}
}
private static void updateRef(Repository git, RevWalk rw, RevCommit c,
Change change, PatchSet newPatchSet) throws IOException {
RefUpdate ru = git.updateRef(newPatchSet.getRefName());

View File

@@ -32,7 +32,6 @@ import com.google.gerrit.server.GerritPersonIdent;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.ssh.NoSshInfo;
import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject;
import com.google.inject.Singleton;
@@ -74,8 +73,7 @@ public class Revert implements RestModifyView<ChangeResource, RevertInput>,
revertedChangeId = changeUtil.revert(control,
change.currentPatchSetId(),
Strings.emptyToNull(input.message),
new PersonIdent(myIdent, TimeUtil.nowTs()),
new NoSshInfo());
new PersonIdent(myIdent, TimeUtil.nowTs()));
} catch (InvalidChangeOperationException e) {
throw new BadRequestException(e.getMessage());
} catch (NoSuchChangeException e) {

View File

@@ -111,6 +111,7 @@ import com.google.gerrit.server.notedb.ChangeUpdate;
import com.google.gerrit.server.notedb.NotesMigration;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.InvalidChangeOperationException;
import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectControl;
@@ -1717,8 +1718,11 @@ public class ReceiveCommits {
magicBranch.dest,
TimeUtil.nowTs());
change.setTopic(magicBranch.topic);
ins = changeInserterFactory.create(ctl.getProjectControl(), change, c)
.setDraft(magicBranch.draft);
ins = changeInserterFactory.create(
repo, rp.getRevWalk(), ctl.getProjectControl(), change, c)
.setDraft(magicBranch.draft)
// Changes already validated in validateNewCommits.
.setValidatePolicy(CommitValidators.Policy.NONE);
cmd = new ReceiveCommand(ObjectId.zeroId(), c,
ins.getPatchSet().getRefName());
}
@@ -1730,7 +1734,7 @@ public class ReceiveCommits {
requestScopePropagator.wrap(new Callable<Void>() {
@Override
public Void call() throws OrmException, IOException,
ResourceConflictException {
ResourceConflictException, InvalidChangeOperationException {
insertChangeImpl();
synchronized (newProgress) {
newProgress.update(1);
@@ -1742,7 +1746,7 @@ public class ReceiveCommits {
}
private void insertChangeImpl() throws OrmException, IOException,
ResourceConflictException {
ResourceConflictException, InvalidChangeOperationException {
final PatchSet ps = ins.setGroups(groups).getPatchSet();
final Account.Id me = currentUser.getAccountId();
final List<FooterLine> footerLines = commit.getFooterLines();

View File

@@ -1312,9 +1312,10 @@ public abstract class AbstractQueryChangesTest {
new Branch.NameKey(project, branch), TimeUtil.nowTs());
IdentifiedUser user = userFactory.create(Providers.of(db), ownerId);
return changeFactory.create(
projectControlFactory.controlFor(project, user),
change,
commit);
repo.getRepository(), repo.getRevWalk(),
projectControlFactory.controlFor(project, user),
change, commit)
.setValidatePolicy(CommitValidators.Policy.NONE);
}
protected Change newPatchSet(TestRepository<Repo> repo, Change c)