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
This commit is contained in:
@@ -21,6 +21,7 @@ import com.google.gerrit.entities.PatchSet;
|
|||||||
import com.google.gerrit.entities.Project;
|
import com.google.gerrit.entities.Project;
|
||||||
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
|
import com.google.gerrit.extensions.client.DiffPreferencesInfo.Whitespace;
|
||||||
import com.google.gerrit.extensions.common.FileInfo;
|
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.PatchList;
|
||||||
import com.google.gerrit.server.patch.PatchListCache;
|
import com.google.gerrit.server.patch.PatchListCache;
|
||||||
import com.google.gerrit.server.patch.PatchListEntry;
|
import com.google.gerrit.server.patch.PatchListEntry;
|
||||||
@@ -30,6 +31,8 @@ import com.google.inject.Inject;
|
|||||||
import com.google.inject.Singleton;
|
import com.google.inject.Singleton;
|
||||||
import java.util.Map;
|
import java.util.Map;
|
||||||
import java.util.TreeMap;
|
import java.util.TreeMap;
|
||||||
|
import java.util.concurrent.ExecutionException;
|
||||||
|
import org.eclipse.jgit.errors.NoMergeBaseException;
|
||||||
import org.eclipse.jgit.lib.ObjectId;
|
import org.eclipse.jgit.lib.ObjectId;
|
||||||
|
|
||||||
@Singleton
|
@Singleton
|
||||||
@@ -42,31 +45,44 @@ public class FileInfoJson {
|
|||||||
}
|
}
|
||||||
|
|
||||||
public Map<String, FileInfo> toFileInfoMap(Change change, PatchSet patchSet)
|
public Map<String, FileInfo> toFileInfoMap(Change change, PatchSet patchSet)
|
||||||
throws PatchListNotAvailableException {
|
throws ResourceConflictException, PatchListNotAvailableException {
|
||||||
return toFileInfoMap(change, patchSet.commitId(), null);
|
return toFileInfoMap(change, patchSet.commitId(), null);
|
||||||
}
|
}
|
||||||
|
|
||||||
public Map<String, FileInfo> toFileInfoMap(
|
public Map<String, FileInfo> toFileInfoMap(
|
||||||
Change change, ObjectId objectId, @Nullable PatchSet base)
|
Change change, ObjectId objectId, @Nullable PatchSet base)
|
||||||
throws PatchListNotAvailableException {
|
throws ResourceConflictException, PatchListNotAvailableException {
|
||||||
ObjectId a = base != null ? base.commitId() : null;
|
ObjectId a = base != null ? base.commitId() : null;
|
||||||
return toFileInfoMap(change, PatchListKey.againstCommit(a, objectId, Whitespace.IGNORE_NONE));
|
return toFileInfoMap(change, PatchListKey.againstCommit(a, objectId, Whitespace.IGNORE_NONE));
|
||||||
}
|
}
|
||||||
|
|
||||||
public Map<String, FileInfo> toFileInfoMap(Change change, ObjectId objectId, int parent)
|
public Map<String, FileInfo> toFileInfoMap(Change change, ObjectId objectId, int parent)
|
||||||
throws PatchListNotAvailableException {
|
throws ResourceConflictException, PatchListNotAvailableException {
|
||||||
return toFileInfoMap(
|
return toFileInfoMap(
|
||||||
change, PatchListKey.againstParentNum(parent + 1, objectId, Whitespace.IGNORE_NONE));
|
change, PatchListKey.againstParentNum(parent + 1, objectId, Whitespace.IGNORE_NONE));
|
||||||
}
|
}
|
||||||
|
|
||||||
private Map<String, FileInfo> toFileInfoMap(Change change, PatchListKey key)
|
private Map<String, FileInfo> toFileInfoMap(Change change, PatchListKey key)
|
||||||
throws PatchListNotAvailableException {
|
throws ResourceConflictException, PatchListNotAvailableException {
|
||||||
return toFileInfoMap(change.getProject(), key);
|
return toFileInfoMap(change.getProject(), key);
|
||||||
}
|
}
|
||||||
|
|
||||||
public Map<String, FileInfo> toFileInfoMap(Project.NameKey project, PatchListKey key)
|
public Map<String, FileInfo> toFileInfoMap(Project.NameKey project, PatchListKey key)
|
||||||
throws PatchListNotAvailableException {
|
throws ResourceConflictException, PatchListNotAvailableException {
|
||||||
PatchList list = patchListCache.get(key, project);
|
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<>();
|
Map<String, FileInfo> files = new TreeMap<>();
|
||||||
for (PatchListEntry e : list.getPatches()) {
|
for (PatchListEntry e : list.getPatches()) {
|
||||||
|
@@ -49,6 +49,7 @@ import com.google.gerrit.extensions.config.DownloadScheme;
|
|||||||
import com.google.gerrit.extensions.registration.DynamicMap;
|
import com.google.gerrit.extensions.registration.DynamicMap;
|
||||||
import com.google.gerrit.extensions.registration.Extension;
|
import com.google.gerrit.extensions.registration.Extension;
|
||||||
import com.google.gerrit.extensions.restapi.AuthException;
|
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.AnonymousUser;
|
||||||
import com.google.gerrit.server.CurrentUser;
|
import com.google.gerrit.server.CurrentUser;
|
||||||
import com.google.gerrit.server.GpgException;
|
import com.google.gerrit.server.GpgException;
|
||||||
@@ -311,9 +312,13 @@ public class RevisionJson {
|
|||||||
}
|
}
|
||||||
|
|
||||||
if (has(ALL_FILES) || (out.isCurrent && has(CURRENT_FILES))) {
|
if (has(ALL_FILES) || (out.isCurrent && has(CURRENT_FILES))) {
|
||||||
out.files = fileInfoJson.toFileInfoMap(c, in);
|
try {
|
||||||
out.files.remove(Patch.COMMIT_MSG);
|
out.files = fileInfoJson.toFileInfoMap(c, in);
|
||||||
out.files.remove(Patch.MERGE_LIST);
|
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()) {
|
if (out.isCurrent && has(CURRENT_ACTIONS) && userProvider.get().isIdentifiedUser()) {
|
||||||
|
@@ -184,7 +184,8 @@ public class ChangeEdits implements ChildCollection<ChangeResource, ChangeEditRe
|
|||||||
|
|
||||||
@Override
|
@Override
|
||||||
public Response<EditInfo> apply(ChangeResource rsrc)
|
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());
|
Optional<ChangeEdit> edit = editUtil.byChange(rsrc.getNotes(), rsrc.getUser());
|
||||||
if (!edit.isPresent()) {
|
if (!edit.isPresent()) {
|
||||||
return Response.none();
|
return Response.none();
|
||||||
|
@@ -20,6 +20,7 @@ import com.google.gerrit.extensions.common.FileInfo;
|
|||||||
import com.google.gerrit.extensions.registration.DynamicMap;
|
import com.google.gerrit.extensions.registration.DynamicMap;
|
||||||
import com.google.gerrit.extensions.restapi.ChildCollection;
|
import com.google.gerrit.extensions.restapi.ChildCollection;
|
||||||
import com.google.gerrit.extensions.restapi.IdString;
|
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.ResourceNotFoundException;
|
||||||
import com.google.gerrit.extensions.restapi.Response;
|
import com.google.gerrit.extensions.restapi.Response;
|
||||||
import com.google.gerrit.extensions.restapi.RestReadView;
|
import com.google.gerrit.extensions.restapi.RestReadView;
|
||||||
@@ -86,7 +87,7 @@ public class FilesInCommitCollection implements ChildCollection<CommitResource,
|
|||||||
|
|
||||||
@Override
|
@Override
|
||||||
public Response<Map<String, FileInfo>> apply(CommitResource resource)
|
public Response<Map<String, FileInfo>> apply(CommitResource resource)
|
||||||
throws PatchListNotAvailableException {
|
throws ResourceConflictException, PatchListNotAvailableException {
|
||||||
RevCommit commit = resource.getCommit();
|
RevCommit commit = resource.getCommit();
|
||||||
PatchListKey key;
|
PatchListKey key;
|
||||||
|
|
||||||
|
Reference in New Issue
Block a user