Do not fail with 500 ISE if base of merge change cannot be computed due to NoMergeBaseException If the repository content doesn't allow us to compute an auto-merge commit for the base of a merge change because JGit cannot find a merge base (e.g. due to CONFLICTS_DURING_MERGE_BASE_CALCULATION), we now return '409 Conflict' for the ListFiles and change-edit-related REST endpoints. If the files were supposed to be included into a ChangeInfo / RevisionInfo we ignore the failure and just do not populate the file list (but we log a warning). Letting the request succeed in this case is better, as the caller can at least get the rest of the change and revision information. Treating NoMergeBaseException as '409 Conflict' is consistent with how we treat this exception in other places (e.g. in the CreateChange REST endpoint). Change-Id: I78b4636c23635048eedf2a1e4b1075eb87ecb262
diff --git a/java/com/google/gerrit/server/change/FileInfoJson.java b/java/com/google/gerrit/server/change/FileInfoJson.java index a823975..aca4fb0 100644 --- a/java/com/google/gerrit/server/change/FileInfoJson.java +++ b/java/com/google/gerrit/server/change/FileInfoJson.java
@@ -21,6 +21,7 @@ import com.google.gerrit.entities.Project; import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace; import com.google.gerrit.extensions.common.FileInfo; +import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.server.patch.PatchList; import com.google.gerrit.server.patch.PatchListCache; import com.google.gerrit.server.patch.PatchListEntry; @@ -30,6 +31,8 @@ import com.google.inject.Singleton; import java.util.Map; import java.util.TreeMap; +import java.util.concurrent.ExecutionException; +import org.eclipse.jgit.errors.NoMergeBaseException; import org.eclipse.jgit.lib.ObjectId; @Singleton @@ -42,31 +45,44 @@ } public Map<String, FileInfo> toFileInfoMap(Change change, PatchSet patchSet) - throws PatchListNotAvailableException { + throws ResourceConflictException, PatchListNotAvailableException { return toFileInfoMap(change, patchSet.commitId(), null); } public Map<String, FileInfo> toFileInfoMap( Change change, ObjectId objectId, @Nullable PatchSet base) - throws PatchListNotAvailableException { + throws ResourceConflictException, PatchListNotAvailableException { ObjectId a = base != null ? base.commitId() : null; return toFileInfoMap(change, PatchListKey.againstCommit(a, objectId, Whitespace.IGNORE_NONE)); } public Map<String, FileInfo> toFileInfoMap(Change change, ObjectId objectId, int parent) - throws PatchListNotAvailableException { + throws ResourceConflictException, PatchListNotAvailableException { return toFileInfoMap( change, PatchListKey.againstParentNum(parent + 1, objectId, Whitespace.IGNORE_NONE)); } private Map<String, FileInfo> toFileInfoMap(Change change, PatchListKey key) - throws PatchListNotAvailableException { + throws ResourceConflictException, PatchListNotAvailableException { return toFileInfoMap(change.getProject(), key); } public Map<String, FileInfo> toFileInfoMap(Project.NameKey project, PatchListKey key) - throws PatchListNotAvailableException { - PatchList list = patchListCache.get(key, project); + throws ResourceConflictException, PatchListNotAvailableException { + PatchList list; + try { + list = patchListCache.get(key, project); + } catch (PatchListNotAvailableException e) { + Throwable cause = e.getCause(); + if (cause instanceof ExecutionException) { + cause = cause.getCause(); + } + if (cause instanceof NoMergeBaseException) { + throw new ResourceConflictException( + String.format("Cannot create auto merge commit: %s", e.getMessage()), e); + } + throw e; + } Map<String, FileInfo> files = new TreeMap<>(); for (PatchListEntry e : list.getPatches()) {
diff --git a/java/com/google/gerrit/server/change/RevisionJson.java b/java/com/google/gerrit/server/change/RevisionJson.java index fbd14c4..a4994fd 100644 --- a/java/com/google/gerrit/server/change/RevisionJson.java +++ b/java/com/google/gerrit/server/change/RevisionJson.java
@@ -49,6 +49,7 @@ import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.registration.Extension; import com.google.gerrit.extensions.restapi.AuthException; +import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.server.AnonymousUser; import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.GpgException; @@ -311,9 +312,13 @@ } if (has(ALL_FILES) || (out.isCurrent && has(CURRENT_FILES))) { - out.files = fileInfoJson.toFileInfoMap(c, in); - out.files.remove(Patch.COMMIT_MSG); - out.files.remove(Patch.MERGE_LIST); + try { + out.files = fileInfoJson.toFileInfoMap(c, in); + out.files.remove(Patch.COMMIT_MSG); + out.files.remove(Patch.MERGE_LIST); + } catch (ResourceConflictException e) { + logger.atWarning().withCause(e).log("creating file list failed"); + } } if (out.isCurrent && has(CURRENT_ACTIONS) && userProvider.get().isIdentifiedUser()) {
diff --git a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java index aa36b73..5adb75a 100644 --- a/java/com/google/gerrit/server/restapi/change/ChangeEdits.java +++ b/java/com/google/gerrit/server/restapi/change/ChangeEdits.java
@@ -184,7 +184,8 @@ @Override public Response<EditInfo> apply(ChangeResource rsrc) - throws AuthException, IOException, ResourceNotFoundException, PermissionBackendException { + throws AuthException, IOException, ResourceNotFoundException, ResourceConflictException, + PermissionBackendException { Optional<ChangeEdit> edit = editUtil.byChange(rsrc.getNotes(), rsrc.getUser()); if (!edit.isPresent()) { return Response.none();
diff --git a/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java b/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java index 0ee8279..0d5ab88 100644 --- a/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java +++ b/java/com/google/gerrit/server/restapi/project/FilesInCommitCollection.java
@@ -20,6 +20,7 @@ import com.google.gerrit.extensions.registration.DynamicMap; import com.google.gerrit.extensions.restapi.ChildCollection; import com.google.gerrit.extensions.restapi.IdString; +import com.google.gerrit.extensions.restapi.ResourceConflictException; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.Response; import com.google.gerrit.extensions.restapi.RestReadView; @@ -86,7 +87,7 @@ @Override public Response<Map<String, FileInfo>> apply(CommitResource resource) - throws PatchListNotAvailableException { + throws ResourceConflictException, PatchListNotAvailableException { RevCommit commit = resource.getCommit(); PatchListKey key;