diff --git a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java index 7fe0c046f3..023848019f 100644 --- a/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/AsyncReceiveCommits.java @@ -94,6 +94,7 @@ public class AsyncReceiveCommits implements PreReceiveHook { // Don't expose the binding for ReceiveCommits.Factory. All callers should // be using AsyncReceiveCommits.Factory instead. install(new FactoryModuleBuilder().build(ReceiveCommits.Factory.class)); + install(new FactoryModuleBuilder().build(CommitValidatorCache.Factory.class)); } @Provides diff --git a/java/com/google/gerrit/server/git/receive/CommitValidatorCache.java b/java/com/google/gerrit/server/git/receive/CommitValidatorCache.java new file mode 100644 index 0000000000..ff5b04b1f1 --- /dev/null +++ b/java/com/google/gerrit/server/git/receive/CommitValidatorCache.java @@ -0,0 +1,150 @@ +// Copyright (C) 2018 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.receive; + +import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_REASON; + +import com.google.auto.value.AutoValue; +import com.google.common.flogger.FluentLogger; +import com.google.gerrit.common.Nullable; +import com.google.gerrit.reviewdb.client.Branch; +import com.google.gerrit.reviewdb.client.Change; +import com.google.gerrit.reviewdb.client.Project; +import com.google.gerrit.reviewdb.client.RevId; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.events.CommitReceivedEvent; +import com.google.gerrit.server.git.validators.CommitValidationException; +import com.google.gerrit.server.git.validators.CommitValidationMessage; +import com.google.gerrit.server.git.validators.CommitValidators; +import com.google.gerrit.server.git.validators.ValidationMessage; +import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.project.ProjectState; +import com.google.gerrit.server.ssh.SshInfo; +import com.google.inject.Inject; +import com.google.inject.assistedinject.Assisted; +import java.io.IOException; +import java.util.HashSet; +import java.util.List; +import java.util.Set; +import org.eclipse.jgit.lib.ObjectId; +import org.eclipse.jgit.lib.ObjectReader; +import org.eclipse.jgit.notes.NoteMap; +import org.eclipse.jgit.revwalk.RevCommit; +import org.eclipse.jgit.transport.ReceiveCommand; + +/** Validates single commits, but uses a cache to avoid duplicating work. */ +public class CommitValidatorCache { + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + + private final CommitValidators.Factory commitValidatorsFactory; + private final IdentifiedUser user; + private final PermissionBackend.ForProject permissions; + private final Project project; + private final SshInfo sshInfo; + + @AutoValue + protected abstract static class ValidCommitKey { + abstract ObjectId getObjectId(); + + abstract Branch.NameKey getBranch(); + } + + private final Set validCommits; + + interface Factory { + CommitValidatorCache create(ProjectState projectState, IdentifiedUser user); + } + + @Inject + CommitValidatorCache( + CommitValidators.Factory commitValidatorsFactory, + PermissionBackend permissionBackend, + SshInfo sshInfo, + @Assisted ProjectState projectState, + @Assisted IdentifiedUser user) { + this.sshInfo = sshInfo; + this.user = user; + this.commitValidatorsFactory = commitValidatorsFactory; + project = projectState.getProject(); + permissions = permissionBackend.user(user).project(project.getNameKey()); + validCommits = new HashSet<>(); + } + + /** + * Validates a single commit. If the commit does not validate, the command is rejected. + * + * @param objectReader the object reader to use. + * @param branch the branch to which this commit is pushed + * @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. + */ + public boolean validCommit( + ObjectReader objectReader, + Branch.NameKey branch, + ReceiveCommand cmd, + RevCommit commit, + boolean isMerged, + List messages, + NoteMap rejectCommits, + @Nullable Change change) + throws IOException { + ValidCommitKey key = new AutoValue_CommitValidatorCache_ValidCommitKey(commit.copy(), branch); + if (validCommits.contains(key)) { + return true; + } + + try (CommitReceivedEvent receiveEvent = + new CommitReceivedEvent(cmd, project, branch.get(), objectReader, commit, user)) { + CommitValidators validators; + if (isMerged) { + validators = + commitValidatorsFactory.forMergedCommits(permissions, branch, user.asIdentifiedUser()); + } else { + validators = + commitValidatorsFactory.forReceiveCommits( + permissions, + branch, + user.asIdentifiedUser(), + sshInfo, + rejectCommits, + receiveEvent.revWalk, + change); + } + + for (CommitValidationMessage m : validators.validate(receiveEvent)) { + messages.add( + new CommitValidationMessage(messageForCommit(commit, m.getMessage()), m.isError())); + } + } catch (CommitValidationException e) { + logger.atFine().log("Commit validation failed on %s", commit.name()); + for (CommitValidationMessage m : e.getMessages()) { + // The non-error messages may contain background explanation for the + // fatal error, so have to preserve all messages. + messages.add( + new CommitValidationMessage(messageForCommit(commit, m.getMessage()), m.isError())); + } + cmd.setResult(REJECTED_OTHER_REASON, messageForCommit(commit, e.getMessage())); + return false; + } + validCommits.add(key); + return true; + } + + private String messageForCommit(RevCommit c, String msg) { + return String.format("commit %s: %s", c.abbreviate(RevId.ABBREV_LEN).name(), msg); + } +} diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index ffd6287609..183992f300 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -39,7 +39,6 @@ import static org.eclipse.jgit.transport.ReceiveCommand.Result.OK; import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_MISSING_OBJECT; import static org.eclipse.jgit.transport.ReceiveCommand.Result.REJECTED_OTHER_REASON; -import com.google.auto.value.AutoValue; import com.google.common.base.Function; import com.google.common.base.Joiner; import com.google.common.base.Splitter; @@ -106,7 +105,6 @@ import com.google.gerrit.server.config.PluginConfig; import com.google.gerrit.server.config.ProjectConfigEntry; import com.google.gerrit.server.edit.ChangeEdit; import com.google.gerrit.server.edit.ChangeEditUtil; -import com.google.gerrit.server.events.CommitReceivedEvent; import com.google.gerrit.server.git.BanCommit; import com.google.gerrit.server.git.ChangeReportFormatter; import com.google.gerrit.server.git.GroupCollector; @@ -116,9 +114,7 @@ import com.google.gerrit.server.git.MultiProgressMonitor.Task; import com.google.gerrit.server.git.ReceivePackInitializer; import com.google.gerrit.server.git.TagCache; import com.google.gerrit.server.git.ValidationError; -import com.google.gerrit.server.git.validators.CommitValidationException; import com.google.gerrit.server.git.validators.CommitValidationMessage; -import com.google.gerrit.server.git.validators.CommitValidators; import com.google.gerrit.server.git.validators.RefOperationValidationException; import com.google.gerrit.server.git.validators.RefOperationValidators; import com.google.gerrit.server.git.validators.ValidationMessage; @@ -145,7 +141,6 @@ import com.google.gerrit.server.project.ProjectConfig; import com.google.gerrit.server.project.ProjectState; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.query.change.InternalChangeQuery; -import com.google.gerrit.server.ssh.SshInfo; import com.google.gerrit.server.submit.MergeOp; import com.google.gerrit.server.submit.MergeOpRepoManager; import com.google.gerrit.server.submit.SubmoduleException; @@ -328,7 +323,7 @@ class ReceiveCommits { private final ChangeNotes.Factory notesFactory; private final ChangeReportFormatter changeFormatter; private final CmdLineParser.Factory optionParserFactory; - private final CommitValidators.Factory commitValidatorsFactory; + private final CommitValidatorCache.Factory commitValidatorFactory; private final CreateGroupPermissionSyncer createGroupPermissionSyncer; private final CreateRefControl createRefControl; private final DynamicMap pluginConfigEntries; @@ -350,7 +345,6 @@ class ReceiveCommits { private final ReviewDb db; private final Sequences seq; private final SetHashtagsOp.Factory hashtagsFactory; - private final SshInfo sshInfo; private final SubmoduleOp.Factory subOpFactory; private final TagCache tagCache; @@ -379,15 +373,6 @@ class ReceiveCommits { private final ListMultimap pushOptions; private final Map replaceByChange; - @AutoValue - protected abstract static class ValidCommitKey { - abstract ObjectId getObjectId(); - - abstract Branch.NameKey getBranch(); - } - - private final Set validCommits; - // Collections lazily populated during processing. private ListMultimap refsByChange; private ListMultimap refsById; @@ -415,7 +400,7 @@ class ReceiveCommits { ChangeNotes.Factory notesFactory, DynamicItem changeFormatterProvider, CmdLineParser.Factory optionParserFactory, - CommitValidators.Factory commitValidatorsFactory, + CommitValidatorCache.Factory commitValidatorFactory, CreateGroupPermissionSyncer createGroupPermissionSyncer, CreateRefControl createRefControl, DynamicMap pluginConfigEntries, @@ -437,7 +422,6 @@ class ReceiveCommits { ReviewDb db, Sequences seq, SetHashtagsOp.Factory hashtagsFactory, - SshInfo sshInfo, SubmoduleOp.Factory subOpFactory, TagCache tagCache, @Assisted ProjectState projectState, @@ -454,7 +438,7 @@ class ReceiveCommits { this.batchUpdateFactory = batchUpdateFactory; this.changeFormatter = changeFormatterProvider.get(); this.changeInserterFactory = changeInserterFactory; - this.commitValidatorsFactory = commitValidatorsFactory; + this.commitValidatorFactory = commitValidatorFactory; this.createRefControl = createRefControl; this.createGroupPermissionSyncer = createGroupPermissionSyncer; this.db = db; @@ -480,7 +464,6 @@ class ReceiveCommits { this.retryHelper = retryHelper; this.requestScopePropagator = requestScopePropagator; this.seq = seq; - this.sshInfo = sshInfo; this.subOpFactory = subOpFactory; this.tagCache = tagCache; @@ -505,7 +488,6 @@ class ReceiveCommits { pushOptions = LinkedListMultimap.create(); replaceByChange = new LinkedHashMap<>(); updateGroups = new ArrayList<>(); - validCommits = new HashSet<>(); this.allowProjectOwnersToChangeParent = cfg.getBoolean("receive", "allowProjectOwnersToChangeParent", false); @@ -1910,15 +1892,17 @@ class ReceiveCommits { return; } + CommitValidatorCache validator = commitValidatorFactory.create(projectState, user); try { - if (validCommit( + if (validator.validCommit( receivePack.getRevWalk().getObjectReader(), changeEnt.getDest(), cmd, newCommit, false, - changeEnt, - rejectCommits)) { + messages, + rejectCommits, + changeEnt)) { logger.atFine().log("Replacing change %s", changeEnt.getId()); requestReplace(cmd, true, changeEnt, newCommit); } @@ -1975,6 +1959,7 @@ class ReceiveCommits { GroupCollector groupCollector = GroupCollector.create(changeRefsById(), db, psUtil, notesFactory, project.getNameKey()); + CommitValidatorCache validator = commitValidatorFactory.create(projectState, user); try { RevCommit start = setUpWalkForSelectingChanges(); if (start == null) { @@ -2074,14 +2059,15 @@ class ReceiveCommits { logger.atFine().log("Creating new change for %s even though it is already tracked", name); } - if (!validCommit( + if (!validator.validCommit( receivePack.getRevWalk().getObjectReader(), magicBranch.dest, magicBranch.cmd, c, magicBranch.merged, - null, - rejectCommits)) { + messages, + rejectCommits, + null)) { // Not a change the user can propose? Abort as early as possible. logger.atFine().log("Aborting early due to invalid commit"); return Collections.emptyList(); @@ -3040,6 +3026,7 @@ class ReceiveCommits { return; } + CommitValidatorCache validator = commitValidatorFactory.create(projectState, user); boolean missingFullName = Strings.isNullOrEmpty(user.getAccount().getFullName()); RevWalk walk = receivePack.getRevWalk(); walk.reset(); @@ -3067,7 +3054,8 @@ class ReceiveCommits { continue; } - if (!validCommit(walk.getObjectReader(), branch, cmd, c, false, null, rejectCommits)) { + if (!validator.validCommit( + walk.getObjectReader(), branch, cmd, c, false, messages, rejectCommits, null)) { break; } @@ -3084,71 +3072,6 @@ class ReceiveCommits { } } - private String messageForCommit(RevCommit c, String msg) { - return String.format("commit %s: %s", c.abbreviate(RevId.ABBREV_LEN).name(), msg); - } - - /** - * Validates a single commit. If the commit does not validate, the command is rejected. - * - * @param objectReader the object reader to use. - * @param branch the branch to which this commit is pushed - * @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. - */ - private boolean validCommit( - ObjectReader objectReader, - Branch.NameKey branch, - ReceiveCommand cmd, - RevCommit commit, - boolean isMerged, - @Nullable Change change, - NoteMap rejectCommits) - throws IOException { - - ValidCommitKey key = new AutoValue_ReceiveCommits_ValidCommitKey(commit.copy(), branch); - if (validCommits.contains(key)) { - return true; - } - - try (CommitReceivedEvent receiveEvent = - new CommitReceivedEvent(cmd, project, branch.get(), objectReader, commit, user)) { - CommitValidators validators; - if (isMerged) { - validators = - commitValidatorsFactory.forMergedCommits(permissions, branch, user.asIdentifiedUser()); - } else { - validators = - commitValidatorsFactory.forReceiveCommits( - permissions, - branch, - user.asIdentifiedUser(), - sshInfo, - rejectCommits, - receiveEvent.revWalk, - change); - } - - for (CommitValidationMessage m : validators.validate(receiveEvent)) { - messages.add( - new CommitValidationMessage(messageForCommit(commit, m.getMessage()), m.isError())); - } - } catch (CommitValidationException e) { - logger.atFine().log("Commit validation failed on %s", commit.name()); - for (CommitValidationMessage m : e.getMessages()) { - // TODO(hanwen): drop the non-error messages? - messages.add( - new CommitValidationMessage(messageForCommit(commit, m.getMessage()), m.isError())); - } - reject(cmd, messageForCommit(commit, e.getMessage())); - return false; - } - validCommits.add(key); - return true; - } - private void autoCloseChanges(ReceiveCommand cmd, Task progress) { logger.atFine().log("Starting auto-closing of changes"); String refName = cmd.getRefName(); diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java index bcb910a446..7930fe89e2 100644 --- a/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -75,6 +75,9 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.util.SystemReader; +/** + * Represents a list of CommitValidationListeners to run for a push to one branch of one project. + */ public class CommitValidators { private static final FluentLogger logger = FluentLogger.forEnclosingClass();