diff --git a/java/com/google/gerrit/server/restapi/change/Submit.java b/java/com/google/gerrit/server/restapi/change/Submit.java index 01364f55d8..b62e475df7 100644 --- a/java/com/google/gerrit/server/restapi/change/Submit.java +++ b/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 mergeOpProvider; private final Provider mergeSuperSet; private final AccountResolver accountResolver; @@ -127,7 +125,6 @@ public class Submit GitRepositoryManager repoManager, PermissionBackend permissionBackend, ChangeData.Factory changeDataFactory, - ChangeNotes.Factory changeNotesFactory, Provider mergeOpProvider, Provider 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())); } /** diff --git a/java/com/google/gerrit/server/submit/MergeOp.java b/java/com/google/gerrit/server/submit/MergeOp.java index 6c3d48b66d..024880f038 100644 --- a/java/com/google/gerrit/server/submit/MergeOp.java +++ b/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 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 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) { diff --git a/java/com/google/gerrit/server/submit/SubmitStrategy.java b/java/com/google/gerrit/server/submit/SubmitStrategy.java index 73cbc8f613..d174e93bdc 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategy.java +++ b/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 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 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); } } diff --git a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java b/java/com/google/gerrit/server/submit/SubmitStrategyOp.java index ce6862f964..bcffbc9c14 100644 --- a/java/com/google/gerrit/server/submit/SubmitStrategyOp.java +++ b/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 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);