ReceiveCommits: move commit validation into a separate class

Change-Id: Ifbc2794e5fc86a71395fea058b70baeef93c6459

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

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<CommitValidatorCache.ValidCommitKey> 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<ValidationMessage> 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);
+  }
+}

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<ProjectConfigEntry> 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<String, String> pushOptions;
   private final Map<Change.Id, ReplaceRequest> replaceByChange;
 
-  @AutoValue
-  protected abstract static class ValidCommitKey {
-    abstract ObjectId getObjectId();
-
-    abstract Branch.NameKey getBranch();
-  }
-
-  private final Set<ValidCommitKey> validCommits;
-
   // Collections lazily populated during processing.
   private ListMultimap<Change.Id, Ref> refsByChange;
   private ListMultimap<ObjectId, Ref> refsById;
@@ -415,7 +400,7 @@ class ReceiveCommits {
       ChangeNotes.Factory notesFactory,
       DynamicItem<ChangeReportFormatter> changeFormatterProvider,
       CmdLineParser.Factory optionParserFactory,
-      CommitValidators.Factory commitValidatorsFactory,
+      CommitValidatorCache.Factory commitValidatorFactory,
       CreateGroupPermissionSyncer createGroupPermissionSyncer,
       CreateRefControl createRefControl,
       DynamicMap<ProjectConfigEntry> 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,
+          messages,
-          rejectCommits)) {
+          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,
+            messages,
-            rejectCommits)) {
+            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();

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();