opendev/gerrit
Code Issues Proposed changes

New extension to validate branch updates by submit strategies.

Browse Source 
Adds new extension point to validate a branch tip update when a change is being
submitted by submit strategy. As submit strategies may generate new commits
(e.g. Cherry Pick), this listener allows validation of resulting new commit
before branch is updated, potentially aborting the submission.

Before, only MergeValidationListener.onPreMerge was available to
validate changes being submitted. However, onPreMerge was called
before submit strategies were run, thus commits generated by submit
strategies were previously impossible to validate.

Now, OnSubmitValidationListener.preBranchUpdate can be used
to abort submission before any refs are updated, even if there are
several changes submitted together.

Change-Id: I2ab75c7c38eb8ed0c010507b49f40e1b5cebad3b
This commit is contained in:
Andrii Shyshkalov 2017-01-19 13:44:16 -08:00
parent 48d5c9b9aa
commit da2034f1a3
8 changed files with 302 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
Documentation/config-validation.txt
View File

@ -45,6 +45,18 @@ are merged to the git repository.
If the commit fails the validation, the plugin can throw an exception
which will cause the merge to fail.
[[on-submit-validation]]
== On submit validation
Plugins implementing the `OnSubmitValidationListener` interface can
perform additional validation checks against ref operations resuling
from execution of submit operation before they are applied to any git
repositories (there could be more than one in case of topic submits).
Plugin can throw an exception which will cause submit operation to be
aborted.
[[pre-upload-validation]]
== Pre-upload validation

114
gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
View File

@ -29,6 +29,7 @@ import static org.junit.Assert.fail;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.Iterables;
import com.google.common.collect.Lists;
import com.google.common.collect.Sets;
import com.google.gerrit.acceptance.AbstractDaemonTest;
import com.google.gerrit.acceptance.NoHttpd;
import com.google.gerrit.acceptance.PushOneCommit;
@ -45,6 +46,8 @@ import com.google.gerrit.extensions.client.ListChangesOption;
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.ChangeInfo;
import com.google.gerrit.extensions.common.LabelInfo;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.extensions.registration.RegistrationHandle;
import com.google.gerrit.extensions.restapi.AuthException;
import com.google.gerrit.extensions.restapi.BinaryResult;
import com.google.gerrit.extensions.restapi.ResourceConflictException;
@ -56,6 +59,7 @@ import com.google.gerrit.reviewdb.client.Change;
import com.google.gerrit.reviewdb.client.PatchSet;
import com.google.gerrit.reviewdb.client.PatchSetApproval;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.RefNames;
import com.google.gerrit.server.ApprovalsUtil;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.change.RevisionResource;
@ -63,8 +67,10 @@ import com.google.gerrit.server.change.Submit;
import com.google.gerrit.server.git.BatchUpdate;
import com.google.gerrit.server.git.BatchUpdate.ChangeContext;
import com.google.gerrit.server.git.ProjectConfig;
import com.google.gerrit.server.git.validators.OnSubmitValidationListener;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.project.Util;
import com.google.gerrit.server.validators.ValidationException;
import com.google.gerrit.testutil.ConfigSuite;
import com.google.gerrit.testutil.TestTimeUtil;
import com.google.gwtorm.server.OrmException;
@ -86,9 +92,13 @@ import org.junit.Before;
import org.junit.Test;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.stream.Collectors;
@NoHttpd
@ -110,6 +120,10 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
@Inject
private BatchUpdate.Factory updateFactory;
@Inject
private DynamicSet<OnSubmitValidationListener> onSubmitValidationListeners;
private RegistrationHandle onSubmitValidatorHandle;
private String systemTimeZone;
@Before
@ -129,6 +143,13 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
db.close();
}
@After
public void removeOnSubmitValidator(){
if (onSubmitValidatorHandle != null){
onSubmitValidatorHandle.remove();
}
}
protected abstract SubmitType getSubmitType();
@Test
@ -647,6 +668,94 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
assertThat(getRemoteHead()).isEqualTo(headAfterSubmit);
}
@Test
public void submitWithValidation() throws Exception {
AtomicBoolean called = new AtomicBoolean(false);
this.addOnSubmitValidationListener(new OnSubmitValidationListener() {
@Override
public void preBranchUpdate(Arguments args) throws ValidationException {
called.set(true);
HashSet<String> refs = Sets.newHashSet(args.getCommands().keySet());
assertThat(refs).contains("refs/heads/master");
refs.remove("refs/heads/master");
if (!refs.isEmpty()){
// Some submit strategies need to insert new patchset.
assertThat(refs).hasSize(1);
assertThat(refs.iterator().next()).startsWith(RefNames.REFS_CHANGES);
}
}
});
PushOneCommit.Result change = createChange();
approve(change.getChangeId());
submit(change.getChangeId());
assertThat(called.get()).isTrue();
}
@Test
public void submitWithValidationMultiRepo() throws Exception {
assume().that(isSubmitWholeTopicEnabled()).isTrue();
String topic = "test-topic";
// Create test projects
TestRepository<?> repoA =
createProjectWithPush("project-a", null, getSubmitType());
TestRepository<?> repoB =
createProjectWithPush("project-b", null, getSubmitType());
// Create changes on project-a
PushOneCommit.Result change1 =
createChange(repoA, "master", "Change 1", "a.txt", "content", topic);
PushOneCommit.Result change2 =
createChange(repoA, "master", "Change 2", "b.txt", "content", topic);
// Create changes on project-b
PushOneCommit.Result change3 =
createChange(repoB, "master", "Change 3", "a.txt", "content", topic);
PushOneCommit.Result change4 =
createChange(repoB, "master", "Change 4", "b.txt", "content", topic);
List<PushOneCommit.Result> changes =
Lists.newArrayList(change1, change2, change3, change4);
for (PushOneCommit.Result change : changes) {
approve(change.getChangeId());
}
// Construct validator which will throw on a second call.
// Since there are 2 repos, first submit attempt will fail, the second will
// succeed.
List<String> projectsCalled = new ArrayList<>(4);
this.addOnSubmitValidationListener(new OnSubmitValidationListener() {
@Override
public void preBranchUpdate(Arguments args) throws ValidationException {
assertThat(args.getCommands().keySet()).contains("refs/heads/master");
try (RevWalk rw = args.newRevWalk()) {
rw.parseBody(rw.parseCommit(
args.getCommands().get("refs/heads/master").getNewId()));
} catch (IOException e) {
assertThat(e).isNull();
}
projectsCalled.add(args.getProject().get());
if (projectsCalled.size() == 2) {
throw new ValidationException("time to fail");
}
}
});
submitWithConflict(change4.getChangeId(), "time to fail");
assertThat(projectsCalled).containsExactly(name("project-a"),
name("project-b"));
for (PushOneCommit.Result change : changes) {
change.assertChange(Change.Status.NEW, name(topic), admin);
}
submit(change4.getChangeId());
assertThat(projectsCalled).containsExactly(name("project-a"),
name("project-b"), name("project-a"), name("project-b"));
for (PushOneCommit.Result change : changes) {
change.assertChange(Change.Status.MERGED, name(topic), admin);
}
}
private void setChangeStatusToNew(PushOneCommit.Result... changes)
throws Exception {
for (PushOneCommit.Result change : changes) {
@ -881,6 +990,11 @@ public abstract class AbstractSubmit extends AbstractDaemonTest {
return getRemoteLog(project, "master");
}
protected void addOnSubmitValidationListener(OnSubmitValidationListener listener){
assertThat(onSubmitValidatorHandle).isNull();
onSubmitValidatorHandle = onSubmitValidationListeners.add(listener);
}
private String getLatestDiff(Repository repo) throws Exception {
ObjectId oldTreeId = repo.resolve("HEAD~1^{tree}");
ObjectId newTreeId = repo.resolve("HEAD^{tree}");

4
gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
View File

@ -128,6 +128,8 @@ import com.google.gerrit.server.git.validators.MergeValidators;
import com.google.gerrit.server.git.validators.MergeValidators.ProjectConfigValidator;
import com.google.gerrit.server.git.validators.RefOperationValidationListener;
import com.google.gerrit.server.git.validators.RefOperationValidators;
import com.google.gerrit.server.git.validators.OnSubmitValidationListener;
import com.google.gerrit.server.git.validators.OnSubmitValidators;
import com.google.gerrit.server.git.validators.UploadValidationListener;
import com.google.gerrit.server.git.validators.UploadValidators;
import com.google.gerrit.server.group.GroupInfoCache;
@ -350,6 +352,7 @@ public class GerritGlobalModule extends FactoryModule {
DynamicSet.setOf(binder(), CommitValidationListener.class);
DynamicSet.setOf(binder(), ChangeMessageModifier.class);
DynamicSet.setOf(binder(), RefOperationValidationListener.class);
DynamicSet.setOf(binder(), OnSubmitValidationListener.class);
DynamicSet.setOf(binder(), MergeValidationListener.class);
DynamicSet.setOf(binder(), ProjectCreationValidationListener.class);
DynamicSet.setOf(binder(), GroupCreationValidationListener.class);
@ -391,6 +394,7 @@ public class GerritGlobalModule extends FactoryModule {
factory(AbandonOp.Factory.class);
factory(RefOperationValidators.Factory.class);
factory(OnSubmitValidators.Factory.class);
factory(MergeValidators.Factory.class);
factory(ProjectConfigValidator.Factory.class);
factory(NotesBranchUtil.Factory.class);

23
gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
View File

@ -51,6 +51,7 @@ import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.GerritServerConfig;
import com.google.gerrit.server.extensions.events.GitReferenceUpdated;
import com.google.gerrit.server.git.validators.OnSubmitValidators;
import com.google.gerrit.server.index.change.ChangeIndexer;
import com.google.gerrit.server.notedb.ChangeNotes;
import com.google.gerrit.server.notedb.ChangeUpdate;
@ -532,6 +533,7 @@ public class BatchUpdate implements AutoCloseable {
private BatchRefUpdate batchRefUpdate;
private boolean closeRepo;
private Order order;
private OnSubmitValidators onSubmitValidators;
private boolean updateChangesInParallel;
private RequestId requestId;
@ -606,6 +608,15 @@ public class BatchUpdate implements AutoCloseable {
return this;
}
/**
* Add a validation step for intended ref operations, which will be performed
* at the end of {@link RepoOnlyOp#updateRepo(RepoContext)} step.
*/
BatchUpdate setOnSubmitValidators(OnSubmitValidators onSubmitValidators) {
this.onSubmitValidators = onSubmitValidators;
return this;
}
/**
* Execute {@link Op#updateChange(ChangeContext)} in parallel for each change.
*/
@ -692,6 +703,18 @@ public class BatchUpdate implements AutoCloseable {
op.updateRepo(ctx);
}
if (onSubmitValidators != null && commands != null
&& !commands.isEmpty()) {
// Validation of refs has to take place here and not at the beginning
// executeRefUpdates. Otherwise failing validation in a second
// BatchUpdate object will happen *after* first object's
// executeRefUpdates has finished, hence after first repo's refs have
// been updated, which is too late.
onSubmitValidators.validate(project,
new ReadOnlyRepository(getRepository()),
ctx.getInserter().newReader(), commands.getCommands());
}
if (inserter != null) {
logDebug("Flushing inserter");
inserter.flush();

9
gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java
View File

@ -22,6 +22,7 @@ import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.IdentifiedUser;
import com.google.gerrit.server.git.CodeReviewCommit.CodeReviewRevWalk;
import com.google.gerrit.server.git.validators.OnSubmitValidators;
import com.google.gerrit.server.project.NoSuchProjectException;
import com.google.gerrit.server.project.ProjectCache;
import com.google.gerrit.server.project.ProjectState;
@ -107,7 +108,8 @@ public class MergeOpRepoManager implements AutoCloseable {
if (update == null) {
update = batchUpdateFactory.create(db, getProjectName(), caller, ts)
.setRepository(repo, rw, ins)
.setRequestId(submissionId);
.setRequestId(submissionId)
.setOnSubmitValidators(onSubmitValidatorsFactory.create());
}
return update;
}
@ -157,6 +159,7 @@ public class MergeOpRepoManager implements AutoCloseable {
private final Map<Project.NameKey, OpenRepo> openRepos;
private final BatchUpdate.Factory batchUpdateFactory;
private final OnSubmitValidators.Factory onSubmitValidatorsFactory;
private final GitRepositoryManager repoManager;
private final ProjectCache projectCache;
@ -169,10 +172,12 @@ public class MergeOpRepoManager implements AutoCloseable {
MergeOpRepoManager(
GitRepositoryManager repoManager,
ProjectCache projectCache,
BatchUpdate.Factory batchUpdateFactory) {
BatchUpdate.Factory batchUpdateFactory,
OnSubmitValidators.Factory onSubmitValidatorsFactory) {
this.repoManager = repoManager;
this.projectCache = projectCache;
this.batchUpdateFactory = batchUpdateFactory;
this.onSubmitValidatorsFactory = onSubmitValidatorsFactory;
openRepos = new HashMap<>();
}

4
gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
View File

@ -46,6 +46,7 @@ import com.google.gerrit.server.git.MergeTip;
import com.google.gerrit.server.git.MergeUtil;
import com.google.gerrit.server.git.SubmoduleOp;
import com.google.gerrit.server.git.TagCache;
import com.google.gerrit.server.git.validators.OnSubmitValidators;
import com.google.gerrit.server.patch.PatchSetInfoFactory;
import com.google.gerrit.server.project.ChangeControl;
import com.google.gerrit.server.project.ProjectCache;
@ -118,6 +119,7 @@ public abstract class SubmitStrategy {
final ProjectCache projectCache;
final PersonIdent serverIdent;
final RebaseChangeOp.Factory rebaseFactory;
final OnSubmitValidators.Factory onSubmitValidatorsFactory;
final TagCache tagCache;
final Branch.NameKey destBranch;
@ -158,6 +160,7 @@ public abstract class SubmitStrategy {
@GerritPersonIdent PersonIdent serverIdent,
ProjectCache projectCache,
RebaseChangeOp.Factory rebaseFactory,
OnSubmitValidators.Factory onSubmitValidatorsFactory,
TagCache tagCache,
@Assisted Branch.NameKey destBranch,
@Assisted CommitStatus commits,
@ -212,6 +215,7 @@ public abstract class SubmitStrategy {
"project not found: %s", destBranch.getParentKey());
this.mergeSorter = new MergeSorter(rw, alreadyAccepted, canMergeFlag);
this.mergeUtil = mergeUtilFactory.create(project);
this.onSubmitValidatorsFactory = onSubmitValidatorsFactory;
}
}

85
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidationListener.java Normal file
View File

@ -0,0 +1,85 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.git.validators;
import com.google.gerrit.extensions.annotations.ExtensionPoint;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.reviewdb.client.Project.NameKey;
import com.google.gerrit.server.validators.ValidationException;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.revwalk.RevWalk;
import org.eclipse.jgit.transport.ReceiveCommand;
import java.util.Map;
/**
* Listener to validate ref updates performed during submit operation.
*
* As submit strategies may generate new commits (e.g. Cherry Pick), this
* listener allows validation of resulting new commit before destination branch
* is updated and new patchset ref is created.
*
* If you only care about validating the change being submitted and not the
* resulting new commit, consider using {@link MergeValidationListener} instead.
*/
@ExtensionPoint
public interface OnSubmitValidationListener {
public class Arguments {
private Project.NameKey project;
private Repository repository;
private ObjectReader objectReader;
private Map<String, ReceiveCommand> commands;
public Arguments(NameKey project, Repository repository,
ObjectReader objectReader, Map<String, ReceiveCommand> commands) {
this.project = project;
this.repository = repository;
this.objectReader = objectReader;
this.commands = commands;
}
public Project.NameKey getProject() {
return project;
}
/**
* @return a read only repository
*/
public Repository getRepository() {
return repository;
}
public RevWalk newRevWalk() {
return new RevWalk(objectReader);
}
/**
* @return a map from ref to op on it covering all ref ops to be performed
* on this repository as part of ongoing submit operation.
*/
public Map<String, ReceiveCommand> getCommands(){
return commands;
}
}
/**
* Called right before branch is updated with new commit or commits as a
* result of submit.
*
* If ValidationException is thrown, submitting is aborted.
*/
void preBranchUpdate(Arguments args) throws ValidationException;
}

53
gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidators.java Normal file
View File

@ -0,0 +1,53 @@
// Copyright (C) 2017 The Android Open Source Project
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
package com.google.gerrit.server.git.validators;
import com.google.gerrit.extensions.registration.DynamicSet;
import com.google.gerrit.reviewdb.client.Project;
import com.google.gerrit.server.git.IntegrationException;
import com.google.gerrit.server.git.validators.OnSubmitValidationListener.Arguments;
import com.google.gerrit.server.validators.ValidationException;
import com.google.inject.Inject;
import org.eclipse.jgit.lib.ObjectReader;
import org.eclipse.jgit.lib.Repository;
import org.eclipse.jgit.transport.ReceiveCommand;
import java.util.Map;
public class OnSubmitValidators {
public interface Factory {
OnSubmitValidators create();
}
private final DynamicSet<OnSubmitValidationListener> listeners;
@Inject
OnSubmitValidators(DynamicSet<OnSubmitValidationListener> listeners) {
this.listeners = listeners;
}
public void validate(Project.NameKey project, Repository repo,
ObjectReader objectReader, Map<String, ReceiveCommand> commands)
throws IntegrationException {
try {
for (OnSubmitValidationListener listener : this.listeners) {
listener.preBranchUpdate(
new Arguments(project, repo, objectReader, commands));
}
} catch (ValidationException e) {
throw new IntegrationException(e.getMessage());
}
}
}