From 9fd63cb5e9956792fad0b8a365994266eb94240f Mon Sep 17 00:00:00 2001 From: Edwin Kempin Date: Mon, 29 Aug 2016 15:34:53 +0200 Subject: [PATCH] Files REST endpoint: Fail with ISE on PatchListNotAvailableException At the moment PatchListNotAvailableException results in 404 Not Found, however this response code is only appropriate if a resource that was specified in the URL is not found. PatchListCacheImpl throws PatchListNotAvailableException if data is inconsistent (patch set has no revision, computing patch list failed) or if the patch set was not set or failed to load. These are all cases which should rather result in a 500 Internal Server Error. Any missing revision is already handled by the Revisions REST collection which correctly fails with 404 Not Found in this case. The Revisions collection parses the revision before the Files REST endpoint is invoked, hence it is safe for the Files REST endpoint to assume that the revision exists. Handling PatchListNotAvailableException as 404 Not Found is bad because it leaks internal information to the user, e.g. the response may contain the following message: "com.google.common.util.concurrent.UncheckedExecutionException: org.eclipse.jgit.errors.LargeObjectException: unknown object exceeds size limit" Also showing an error dialog instead of the Not Found page gives better feedback to the user about what has really happened. Change-Id: I2ad01ece93561788120292affa03ed09d184eaa1 Signed-off-by: Edwin Kempin --- .../server/api/changes/RevisionApiImpl.java | 9 ++-- .../google/gerrit/server/change/Files.java | 43 +++++++++---------- .../patch/PatchListNotAvailableException.java | 4 ++ 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java index 6b5e83c635..7d0b280fa7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java @@ -59,6 +59,7 @@ import com.google.gerrit.server.change.Submit; import com.google.gerrit.server.change.TestSubmitType; import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.UpdateException; +import com.google.gerrit.server.patch.PatchListNotAvailableException; import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gwtorm.server.OrmException; import com.google.inject.Inject; @@ -264,7 +265,7 @@ class RevisionApiImpl implements RevisionApi { return ImmutableSet.copyOf((Iterable) listFiles .setReviewed(true) .apply(revision).value()); - } catch (OrmException | IOException e) { + } catch (OrmException | IOException | PatchListNotAvailableException e) { throw new RestApiException("Cannot list reviewed files", e); } } @@ -293,7 +294,7 @@ class RevisionApiImpl implements RevisionApi { public Map files() throws RestApiException { try { return (Map)listFiles.apply(revision).value(); - } catch (OrmException | IOException e) { + } catch (OrmException | IOException | PatchListNotAvailableException e) { throw new RestApiException("Cannot retrieve files", e); } } @@ -304,7 +305,7 @@ class RevisionApiImpl implements RevisionApi { try { return (Map) listFiles.setBase(base) .apply(revision).value(); - } catch (OrmException | IOException e) { + } catch (OrmException | IOException | PatchListNotAvailableException e) { throw new RestApiException("Cannot retrieve files", e); } } @@ -315,7 +316,7 @@ class RevisionApiImpl implements RevisionApi { try { return (Map) listFiles.setParent(parentNum) .apply(revision).value(); - } catch (OrmException | IOException e) { + } catch (OrmException | IOException | PatchListNotAvailableException e) { throw new RestApiException("Cannot retrieve files", e); } } diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java index 35dbec13ec..c077bbb1dd 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java @@ -138,9 +138,10 @@ public class Files implements ChildCollection { } @Override - public Response apply(RevisionResource resource) throws AuthException, - BadRequestException, ResourceNotFoundException, OrmException, - RepositoryNotFoundException, IOException { + public Response apply(RevisionResource resource) + throws AuthException, BadRequestException, ResourceNotFoundException, + OrmException, RepositoryNotFoundException, IOException, + PatchListNotAvailableException { checkOptions(); if (reviewed) { return Response.ok(reviewed(resource)); @@ -149,26 +150,22 @@ public class Files implements ChildCollection { } Response> r; - try { - if (base != null) { - RevisionResource baseResource = revisions.parse( - resource.getChangeResource(), IdString.fromDecoded(base)); - r = Response.ok(fileInfoJson.toFileInfoMap( - resource.getChange(), - resource.getPatchSet().getRevision(), - baseResource.getPatchSet())); - } else if (parentNum > 0) { - r = Response.ok(fileInfoJson.toFileInfoMap( - resource.getChange(), - resource.getPatchSet().getRevision(), - parentNum - 1)); - } else { - r = Response.ok(fileInfoJson.toFileInfoMap( - resource.getChange(), - resource.getPatchSet())); - } - } catch (PatchListNotAvailableException e) { - throw new ResourceNotFoundException(e.getMessage()); + if (base != null) { + RevisionResource baseResource = revisions.parse( + resource.getChangeResource(), IdString.fromDecoded(base)); + r = Response.ok(fileInfoJson.toFileInfoMap( + resource.getChange(), + resource.getPatchSet().getRevision(), + baseResource.getPatchSet())); + } else if (parentNum > 0) { + r = Response.ok(fileInfoJson.toFileInfoMap( + resource.getChange(), + resource.getPatchSet().getRevision(), + parentNum - 1)); + } else { + r = Response.ok(fileInfoJson.toFileInfoMap( + resource.getChange(), + resource.getPatchSet())); } if (resource.isCacheable()) { diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListNotAvailableException.java b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListNotAvailableException.java index 2ccc9f1ec4..fab66cbfe1 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListNotAvailableException.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListNotAvailableException.java @@ -21,6 +21,10 @@ public class PatchListNotAvailableException extends Exception { super(message); } + public PatchListNotAvailableException(String message, Throwable cause) { + super(message, cause); + } + public PatchListNotAvailableException(Throwable cause) { super(cause); }