diff --git a/java/com/google/gerrit/extensions/registration/DynamicSet.java b/java/com/google/gerrit/extensions/registration/DynamicSet.java index 8c2e566b3d..35f3567b95 100644 --- a/java/com/google/gerrit/extensions/registration/DynamicSet.java +++ b/java/com/google/gerrit/extensions/registration/DynamicSet.java @@ -36,6 +36,8 @@ import java.util.Iterator; import java.util.NoSuchElementException; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Stream; +import java.util.stream.StreamSupport; /** * A set of members that can be modified as plugins reload. @@ -291,6 +293,10 @@ public class DynamicSet implements Iterable { return new ReloadableHandle(ref, key, ref.get()); } + public Stream stream() { + return StreamSupport.stream(spliterator(), false); + } + private class ReloadableHandle implements ReloadableRegistrationHandle { private final AtomicReference> ref; private final Key key; diff --git a/java/com/google/gerrit/server/git/receive/BranchCommitValidator.java b/java/com/google/gerrit/server/git/receive/BranchCommitValidator.java index 64f54d6b79..a2d8e94406 100644 --- a/java/com/google/gerrit/server/git/receive/BranchCommitValidator.java +++ b/java/com/google/gerrit/server/git/receive/BranchCommitValidator.java @@ -90,6 +90,29 @@ public class BranchCommitValidator { NoteMap rejectCommits, @Nullable Change change) throws IOException { + return validCommit(objectReader, cmd, commit, isMerged, messages, rejectCommits, change, false); + } + + /** + * Validates a single commit. If the commit does not validate, the command is rejected. + * + * @param objectReader the object reader to use. + * @param cmd the ReceiveCommand executing the push. + * @param commit the commit being validated. + * @param isMerged whether this is a merge commit created by magicBranch --merge option + * @param change the change for which this is a new patchset. + * @param skipValidation whether 'skip-validation' was requested. + */ + public boolean validCommit( + ObjectReader objectReader, + ReceiveCommand cmd, + RevCommit commit, + boolean isMerged, + List messages, + NoteMap rejectCommits, + @Nullable Change change, + boolean skipValidation) + throws IOException { try (CommitReceivedEvent receiveEvent = new CommitReceivedEvent(cmd, project, branch.get(), objectReader, commit, user)) { CommitValidators validators; @@ -105,7 +128,8 @@ public class BranchCommitValidator { sshInfo, rejectCommits, receiveEvent.revWalk, - change); + change, + skipValidation); } for (CommitValidationMessage m : validators.validate(receiveEvent)) { diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 0abaed4f02..49bb8e1511 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -3013,10 +3013,12 @@ class ReceiveCommits { */ private void validateRegularPushCommits(Branch.NameKey branch, ReceiveCommand cmd) throws PermissionBackendException { - if (!RefNames.REFS_CONFIG.equals(cmd.getRefName()) - && !(MagicBranch.isMagicBranch(cmd.getRefName()) - || NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches()) - && pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION)) { + boolean skipValidation = + !RefNames.REFS_CONFIG.equals(cmd.getRefName()) + && !(MagicBranch.isMagicBranch(cmd.getRefName()) + || NEW_PATCHSET_PATTERN.matcher(cmd.getRefName()).matches()) + && pushOptions.containsKey(PUSH_OPTION_SKIP_VALIDATION); + if (skipValidation) { if (projectState.is(BooleanProjectConfig.USE_SIGNED_OFF_BY)) { reject(cmd, "requireSignedOffBy prevents option " + PUSH_OPTION_SKIP_VALIDATION); return; @@ -3031,11 +3033,8 @@ class ReceiveCommits { if (!Iterables.isEmpty(rejectCommits)) { reject(cmd, "reject-commits prevents " + PUSH_OPTION_SKIP_VALIDATION); } - logger.atFine().log("Short-circuiting new commit validation"); - return; } - BranchCommitValidator validator = commitValidatorFactory.create(projectState, branch, user); RevWalk walk = receivePack.getRevWalk(); walk.reset(); walk.sort(RevSort.NONE); @@ -3050,7 +3049,7 @@ class ReceiveCommits { int limit = receiveConfig.maxBatchCommits; int n = 0; for (RevCommit c; (c = walk.next()) != null; ) { - if (++n > limit) { + if (++n > limit && !skipValidation) { logger.atFine().log("Number of new commits exceeds limit of %d", limit); reject( cmd, @@ -3062,8 +3061,9 @@ class ReceiveCommits { continue; } + BranchCommitValidator validator = commitValidatorFactory.create(projectState, branch, user); if (!validator.validCommit( - walk.getObjectReader(), cmd, c, false, messages, rejectCommits, null)) { + walk.getObjectReader(), cmd, c, false, messages, rejectCommits, null, skipValidation)) { break; } } diff --git a/java/com/google/gerrit/server/git/validators/CommitValidationListener.java b/java/com/google/gerrit/server/git/validators/CommitValidationListener.java index d9fab05d11..fbc582bbbf 100644 --- a/java/com/google/gerrit/server/git/validators/CommitValidationListener.java +++ b/java/com/google/gerrit/server/git/validators/CommitValidationListener.java @@ -35,4 +35,14 @@ public interface CommitValidationListener { */ List onCommitReceived(CommitReceivedEvent receiveEvent) throws CommitValidationException; + + /** + * Whether this validator should validate all commits. + * + * @return {@code true} if this validator should validate all commits, even when the {@code + * skip-validation} push option was specified. + */ + default boolean shouldValidateAllCommits() { + return false; + } } diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java index c6d26b58e1..7ab2ada0dc 100644 --- a/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -132,7 +132,8 @@ public class CommitValidators { SshInfo sshInfo, NoteMap rejectCommits, RevWalk rw, - @Nullable Change change) + @Nullable Change change, + boolean skipValidation) throws IOException { PermissionBackend.ForRef perm = forProject.ref(branch.get()); ProjectState projectState = projectCache.checkedGet(branch.getParentKey()); @@ -153,7 +154,7 @@ public class CommitValidators { change), new ConfigValidator(branch, user, rw, allUsers, allProjects), new BannedCommitsValidator(rejectCommits), - new PluginCommitValidationListener(pluginValidators), + new PluginCommitValidationListener(pluginValidators, skipValidation), new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker), new AccountCommitValidator(repoManager, allUsers, accountValidator), new GroupCommitValidator(allUsers))); @@ -474,27 +475,50 @@ public class CommitValidators { /** Execute commit validation plug-ins */ public static class PluginCommitValidationListener implements CommitValidationListener { + private boolean skipValidation; private final PluginSetContext commitValidationListeners; public PluginCommitValidationListener( final PluginSetContext commitValidationListeners) { + this(commitValidationListeners, false); + } + + public PluginCommitValidationListener( + final PluginSetContext commitValidationListeners, + boolean skipValidation) { + this.skipValidation = skipValidation; this.commitValidationListeners = commitValidationListeners; } + private void runValidator( + CommitValidationListener validator, + List messages, + CommitReceivedEvent receiveEvent) + throws CommitValidationException { + if (skipValidation && !validator.shouldValidateAllCommits()) { + return; + } + messages.addAll(validator.onCommitReceived(receiveEvent)); + } + @Override public List onCommitReceived(CommitReceivedEvent receiveEvent) throws CommitValidationException { List messages = new ArrayList<>(); try { commitValidationListeners.runEach( - l -> messages.addAll(l.onCommitReceived(receiveEvent)), - CommitValidationException.class); + l -> runValidator(l, messages, receiveEvent), CommitValidationException.class); } catch (CommitValidationException e) { messages.addAll(e.getMessages()); throw new CommitValidationException(e.getMessage(), messages); } return messages; } + + @Override + public boolean shouldValidateAllCommits() { + return commitValidationListeners.stream().anyMatch(v -> v.shouldValidateAllCommits()); + } } public static class SignedOffByValidator implements CommitValidationListener { diff --git a/java/com/google/gerrit/server/plugincontext/PluginSetContext.java b/java/com/google/gerrit/server/plugincontext/PluginSetContext.java index a297e58d16..b64cfeb39f 100644 --- a/java/com/google/gerrit/server/plugincontext/PluginSetContext.java +++ b/java/com/google/gerrit/server/plugincontext/PluginSetContext.java @@ -23,6 +23,7 @@ import com.google.gerrit.server.plugincontext.PluginContext.PluginMetrics; import com.google.inject.Inject; import java.util.Iterator; import java.util.SortedSet; +import java.util.stream.Stream; /** * Context to invoke extensions from a {@link DynamicSet}. @@ -147,6 +148,10 @@ public class PluginSetContext implements Iterable> { .forEach(p -> PluginContext.runLogExceptions(pluginMetrics, p, extensionImplConsumer)); } + public Stream stream() { + return dynamicSet.stream(); + } + /** * Invokes each extension in the set. All exceptions from the plugin extensions except exceptions * of the specified type are caught and logged. diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 0e663f3ab0..2ae0fe7f40 100644 --- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -92,7 +92,6 @@ import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.git.receive.NoteDbPushOption; import com.google.gerrit.server.git.receive.ReceiveConstants; -import com.google.gerrit.server.git.validators.CommitValidationException; import com.google.gerrit.server.git.validators.CommitValidationListener; import com.google.gerrit.server.git.validators.CommitValidationMessage; import com.google.gerrit.server.git.validators.CommitValidators.ChangeIdValidator; @@ -2268,14 +2267,27 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { private static class TestValidator implements CommitValidationListener { private final AtomicInteger count = new AtomicInteger(); + private final boolean validateAll; + + TestValidator(boolean validateAll) { + this.validateAll = validateAll; + } + + TestValidator() { + this(false); + } @Override - public List onCommitReceived(CommitReceivedEvent receiveEvent) - throws CommitValidationException { + public List onCommitReceived(CommitReceivedEvent receiveEvent) { count.incrementAndGet(); return Collections.emptyList(); } + @Override + public boolean shouldValidateAllCommits() { + return validateAll; + } + public int count() { return count.get(); } @@ -2286,6 +2298,7 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { String master = "refs/heads/master"; TestValidator validator = new TestValidator(); RegistrationHandle handle = commitValidators.add("test-validator", validator); + RegistrationHandle handle2 = null; try { // Validation listener is called on normal push @@ -2312,8 +2325,25 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { r = push3.to(master); r.assertOkStatus(); assertThat(validator.count()).isEqualTo(1); + + // Validation listener that needs to validate all commits gets called even + // when the skip option is used. + TestValidator validator2 = new TestValidator(true); + handle2 = commitValidators.add("test-validator-2", validator2); + PushOneCommit push4 = + pushFactory.create(db, admin.getIdent(), testRepo, "change2", "b.txt", "content"); + push4.setPushOptions(ImmutableList.of(PUSH_OPTION_SKIP_VALIDATION)); + r = push4.to(master); + r.assertOkStatus(); + // First listener was not called; its count remains the same. + assertThat(validator.count()).isEqualTo(1); + // Second listener was called. + assertThat(validator2.count()).isEqualTo(1); } finally { handle.remove(); + if (handle2 != null) { + handle2.remove(); + } } }