Merge "Submit: Return updated changes from SubmitStrategyOp"

java/com/google/gerrit/server/restapi/change/Submit.java

@@ -49,7 +49,6 @@ import com.google.gerrit.server.change.ChangeResource;
 import com.google.gerrit.server.change.RevisionResource;
 import com.google.gerrit.server.config.GerritServerConfig;
 import com.google.gerrit.server.git.GitRepositoryManager;
-import com.google.gerrit.server.notedb.ChangeNotes;
 import com.google.gerrit.server.permissions.ChangePermission;
 import com.google.gerrit.server.permissions.PermissionBackend;
 import com.google.gerrit.server.permissions.PermissionBackendException;
@@ -107,7 +106,6 @@ public class Submit
   private final GitRepositoryManager repoManager;
   private final PermissionBackend permissionBackend;
   private final ChangeData.Factory changeDataFactory;
-  private final ChangeNotes.Factory changeNotesFactory;
   private final Provider<MergeOp> mergeOpProvider;
   private final Provider<MergeSuperSet> mergeSuperSet;
   private final AccountResolver accountResolver;
@@ -127,7 +125,6 @@ public class Submit
       GitRepositoryManager repoManager,
       PermissionBackend permissionBackend,
       ChangeData.Factory changeDataFactory,
-      ChangeNotes.Factory changeNotesFactory,
       Provider<MergeOp> mergeOpProvider,
       Provider<MergeSuperSet> mergeSuperSet,
       AccountResolver accountResolver,
@@ -138,7 +135,6 @@ public class Submit
     this.repoManager = repoManager;
     this.permissionBackend = permissionBackend;
     this.changeDataFactory = changeDataFactory;
-    this.changeNotesFactory = changeNotesFactory;
     this.mergeOpProvider = mergeOpProvider;
     this.mergeSuperSet = mergeSuperSet;
     this.accountResolver = accountResolver;
@@ -206,26 +202,19 @@ public class Submit
     }
 
     try (MergeOp op = mergeOpProvider.get()) {
-      op.merge(change, submitter, true, input, false);
+      Change updatedChange = op.merge(change, submitter, true, input, false);
+
+      if (updatedChange.isMerged()) {
+        return change;
+      }
+
+      String msg =
+          String.format(
+              "change %s of project %s unexpectedly had status %s after submit attempt",
+              updatedChange.getId(), updatedChange.getProject(), updatedChange.getStatus());
+      logger.atWarning().log(msg);
+      throw new RestApiException(msg);
     }
-
-    // Read the ChangeNotes only after MergeOp is fully done (including MergeOp#close) to be sure
-    // to have the correct state of the repo.
-    ChangeNotes changeNotes = changeNotesFactory.createChecked(change.getProject(), change.getId());
-    change = changeNotes.getChange();
-
-    if (change.isMerged()) {
-      return change;
-    }
-
-    logger.atWarning().log(
-        "change %s of project %s unexpectedly had status %s after submit attempt,"
-            + " change notes were read from %s",
-        change.getId(), change.getProject(), change.getStatus(), changeNotes.getMetaId());
-    throw new RestApiException(
-        String.format(
-            "change %s unexpectedly had status %s after submit attempt",
-            change.getId(), change.getStatus()));
   }
 
   /**

java/com/google/gerrit/server/submit/MergeOp.java

@@ -233,6 +233,9 @@ public class MergeOp implements AutoCloseable {
   private final RetryHelper retryHelper;
   private final ChangeData.Factory changeDataFactory;
 
+  // Changes that were updated by this MergeOp.
+  private final Map<Change.Id, Change> updatedChanges;
+
   private Timestamp ts;
   private RequestId submissionId;
   private IdentifiedUser caller;
@@ -273,6 +276,7 @@ public class MergeOp implements AutoCloseable {
     this.retryHelper = retryHelper;
     this.topicMetrics = topicMetrics;
     this.changeDataFactory = changeDataFactory;
+    this.updatedChanges = new HashMap<>();
   }
 
   @Override
@@ -428,8 +432,9 @@ public class MergeOp implements AutoCloseable {
    * @throws RestApiException if an error occurred.
    * @throws PermissionBackendException if permissions can't be checked
    * @throws IOException an error occurred reading from NoteDb.
+   * @return the merged change
    */
-  public void merge(
+  public Change merge(
       Change change,
       IdentifiedUser caller,
       boolean checkSubmitRules,
@@ -518,6 +523,14 @@ public class MergeOp implements AutoCloseable {
         if (projects > 1) {
           topicMetrics.topicSubmissionsCompleted.increment();
         }
+
+        // It's expected that callers invoke this method only for open changes and that the provided
+        // change either gets updated to merged or that this method fails with an exception. For
+        // safety, fall-back to return the provided change if there was no update for this change
+        // (e.g. caller provided a change that was already merged).
+        return updatedChanges.containsKey(change.getId())
+            ? updatedChanges.get(change.getId())
+            : change;
       } catch (IOException e) {
         // Anything before the merge attempt is an error
         throw new StorageException(e);
@@ -599,10 +612,17 @@ public class MergeOp implements AutoCloseable {
       SubmoduleOp submoduleOp = subOpFactory.create(branches, orm);
       List<SubmitStrategy> strategies = getSubmitStrategies(toSubmit, submoduleOp, dryrun);
       this.allProjects = submoduleOp.getProjectsInOrder();
-      BatchUpdate.execute(
-          orm.batchUpdates(allProjects),
-          new SubmitStrategyListener(submitInput, strategies, commitStatus),
-          dryrun);
+      try {
+        BatchUpdate.execute(
+            orm.batchUpdates(allProjects),
+            new SubmitStrategyListener(submitInput, strategies, commitStatus),
+            dryrun);
+      } finally {
+        // If the BatchUpdate fails it can be that merging some of the changes was actually
+        // successful. This is why we must to collect the updated changes also when an exception was
+        // thrown.
+        strategies.forEach(s -> updatedChanges.putAll(s.getUpdatedChanges()));
+      }
     } catch (NoSuchProjectException e) {
       throw new ResourceNotFoundException(e.getMessage());
     } catch (IOException | SubmoduleException e) {

java/com/google/gerrit/server/submit/SubmitStrategy.java

@@ -14,8 +14,10 @@
 
 package com.google.gerrit.server.submit;
 
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
 import static java.util.Objects.requireNonNull;
 
+import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Sets;
 import com.google.gerrit.extensions.api.changes.SubmitInput;
 import com.google.gerrit.extensions.client.SubmitType;
@@ -55,7 +57,9 @@ import com.google.inject.assistedinject.Assisted;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.HashSet;
 import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -217,8 +221,24 @@ public abstract class SubmitStrategy {
 
   final Arguments args;
 
+  private final Set<SubmitStrategyOp> submitStrategyOps;
+
   SubmitStrategy(Arguments args) {
     this.args = requireNonNull(args);
+    this.submitStrategyOps = new HashSet<>();
+  }
+
+  /**
+   * Returns the updated changed after this submit strategy has been executed.
+   *
+   * @return the updated changes after this submit strategy has been executed
+   */
+  public ImmutableMap<Change.Id, Change> getUpdatedChanges() {
+    return submitStrategyOps.stream()
+        .map(SubmitStrategyOp::getUpdatedChange)
+        .filter(Optional::isPresent)
+        .map(Optional::get)
+        .collect(toImmutableMap(c -> c.getId(), c -> c));
   }
 
   /**
@@ -249,8 +269,10 @@ public abstract class SubmitStrategy {
     for (CodeReviewCommit c : difference) {
       Change.Id id = c.change().getId();
       bu.addOp(id, args.setPrivateOpFactory.create(false, null));
-      bu.addOp(id, new ImplicitIntegrateOp(args, c));
+      ImplicitIntegrateOp implicitIntegrateOp = new ImplicitIntegrateOp(args, c);
+      bu.addOp(id, implicitIntegrateOp);
       maybeAddTestHelperOp(bu, id);
+      this.submitStrategyOps.add(implicitIntegrateOp);
     }
 
     // Then ops for explicitly merged changes
@@ -258,6 +280,7 @@ public abstract class SubmitStrategy {
       bu.addOp(op.getId(), args.setPrivateOpFactory.create(false, null));
       bu.addOp(op.getId(), op);
       maybeAddTestHelperOp(bu, op.getId());
+      this.submitStrategyOps.add(op);
     }
   }
 

java/com/google/gerrit/server/submit/SubmitStrategyOp.java

@@ -294,6 +294,16 @@ abstract class SubmitStrategyOp implements BatchUpdateOp {
     return true;
   }
 
+  /**
+   * Returns the updated change after this op has been executed.
+   *
+   * @return the updated change after this op has been executed, {@link Optional#empty()} if the op
+   *     was not executed yet, or if the execution has failed
+   */
+  public Optional<Change> getUpdatedChange() {
+    return Optional.ofNullable(updatedChange);
+  }
+
   private PatchSet getOrCreateAlreadyMergedPatchSet(ChangeContext ctx) throws IOException {
     PatchSet.Id psId = alreadyMergedCommit.getPatchsetId();
     logger.atFine().log("Fixing up already-merged patch set %s", psId);