Merge "New extension to validate branch updates by submit strategies."
diff --git a/Documentation/config-validation.txt b/Documentation/config-validation.txt
index cccfe5c..0d0717e 100644
--- a/Documentation/config-validation.txt
+++ b/Documentation/config-validation.txt
@@ -45,6 +45,18 @@
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
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
index 45da3d8..2a39ffd 100644
--- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
+++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/AbstractSubmit.java
@@ -29,6 +29,7 @@
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.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.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.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.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 @@
@Inject
private BatchUpdate.Factory updateFactory;
+ @Inject
+ private DynamicSet<OnSubmitValidationListener> onSubmitValidationListeners;
+ private RegistrationHandle onSubmitValidatorHandle;
+
private String systemTimeZone;
@Before
@@ -129,6 +143,13 @@
db.close();
}
+ @After
+ public void removeOnSubmitValidator(){
+ if (onSubmitValidatorHandle != null){
+ onSubmitValidatorHandle.remove();
+ }
+ }
+
protected abstract SubmitType getSubmitType();
@Test
@@ -647,6 +668,94 @@
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 @@
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}");
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
index df96a54..6012142 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/config/GerritGlobalModule.java
@@ -128,6 +128,8 @@
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 @@
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 @@
factory(AbandonOp.Factory.class);
factory(RefOperationValidators.Factory.class);
+ factory(OnSubmitValidators.Factory.class);
factory(MergeValidators.Factory.class);
factory(ProjectConfigValidator.Factory.class);
factory(NotesBranchUtil.Factory.class);
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
index 3c09cb0..7419dab 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/BatchUpdate.java
@@ -51,6 +51,7 @@
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 @@
private BatchRefUpdate batchRefUpdate;
private boolean closeRepo;
private Order order;
+ private OnSubmitValidators onSubmitValidators;
private boolean updateChangesInParallel;
private RequestId requestId;
@@ -607,6 +609,15 @@
}
/**
+ * 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.
*/
public BatchUpdate updateChangesInParallel() {
@@ -692,6 +703,18 @@
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();
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java
index 5196ebe..1244ad3 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/MergeOpRepoManager.java
@@ -22,6 +22,7 @@
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 @@
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 @@
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 @@
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<>();
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
index e0e2ae7..692bc98 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/strategy/SubmitStrategy.java
@@ -46,6 +46,7 @@
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 @@
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 @@
@GerritPersonIdent PersonIdent serverIdent,
ProjectCache projectCache,
RebaseChangeOp.Factory rebaseFactory,
+ OnSubmitValidators.Factory onSubmitValidatorsFactory,
TagCache tagCache,
@Assisted Branch.NameKey destBranch,
@Assisted CommitStatus commits,
@@ -212,6 +215,7 @@
"project not found: %s", destBranch.getParentKey());
this.mergeSorter = new MergeSorter(rw, alreadyAccepted, canMergeFlag);
this.mergeUtil = mergeUtilFactory.create(project);
+ this.onSubmitValidatorsFactory = onSubmitValidatorsFactory;
}
}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidationListener.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidationListener.java
new file mode 100644
index 0000000..f2f397c
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidationListener.java
@@ -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;
+}
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidators.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidators.java
new file mode 100644
index 0000000..568c597
--- /dev/null
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/validators/OnSubmitValidators.java
@@ -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());
+ }
+ }
+}
