CheckMergeability: Do not fail with 500 ISE on NoMergeBaseException
If NoMergeBaseException is thrown it's the state of the repository that
prevents the merge (e.g. the reason is
CONFLICTS_DURING_MERGE_BASE_CALCULATION). We should return '409
Conflict' in this case as we do in other REST endpoints.
It would be better to return a MergeableInfo with mergeable = false, but
if we do that now the caller cannot find out the reason why the change
is not mergeable since MergeableInfo doesn't have any field to carry a
message or a reason. We could add this field, but then the frontend also
needs to be adapted to show that message to the user. That's more work
that we leave for later. For now returning '409 Conflict' is better than
failing with '500 Internal Server Error'.
Change-Id: I307aab2bd94495634c77eda0976dea435673bdcb
Signed-off-by: Edwin Kempin <ekempin@google.com>
diff --git a/java/com/google/gerrit/server/restapi/project/CheckMergeability.java b/java/com/google/gerrit/server/restapi/project/CheckMergeability.java
index 4864fde..da6ff14 100644
--- a/java/com/google/gerrit/server/restapi/project/CheckMergeability.java
+++ b/java/com/google/gerrit/server/restapi/project/CheckMergeability.java
@@ -18,6 +18,7 @@
import com.google.gerrit.extensions.client.SubmitType;
import com.google.gerrit.extensions.common.MergeableInfo;
import com.google.gerrit.extensions.restapi.BadRequestException;
+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;
@@ -28,6 +29,7 @@
import com.google.gerrit.server.project.BranchResource;
import com.google.inject.Inject;
import java.io.IOException;
+import org.eclipse.jgit.errors.NoMergeBaseException;
import org.eclipse.jgit.lib.Config;
import org.eclipse.jgit.lib.ObjectInserter;
import org.eclipse.jgit.lib.Ref;
@@ -78,7 +80,8 @@
@Override
public Response<MergeableInfo> apply(BranchResource resource)
- throws IOException, BadRequestException, ResourceNotFoundException {
+ throws IOException, BadRequestException, ResourceNotFoundException,
+ ResourceConflictException {
if (!(submitType.equals(SubmitType.MERGE_ALWAYS)
|| submitType.equals(SubmitType.MERGE_IF_NECESSARY))) {
throw new BadRequestException("Submit type: " + submitType + " is not supported");
@@ -123,6 +126,12 @@
}
} catch (InvalidMergeStrategyException e) {
throw new BadRequestException(e.getMessage());
+ } catch (NoMergeBaseException e) {
+ // TODO(ekempin) Rather return MergeableInfo with mergeable = false. But then we need a new
+ // field in MergeableInfo to carry the message to the client and the frontend needs to be
+ // adapted to show the message to the user.
+ throw new ResourceConflictException(
+ String.format("Change cannot be merged: %s", e.getMessage()), e);
}
return Response.ok(result);
}
