opendev/gerrit
Code Issues Proposed changes

Files REST endpoint: Fail with ISE on PatchListNotAvailableException

Browse Source 
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 <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2016-08-29 15:34:53 +02:00
parent 6d998b9162
commit 9fd63cb5e9
3 changed files with 29 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
gerrit-server/src/main/java/com/google/gerrit/server/api/changes/RevisionApiImpl.java
View File

@@ -59,6 +59,7 @@ import com.google.gerrit.server.change.Submit;
import com.google.gerrit.server.change.TestSubmitType; import com.google.gerrit.server.change.TestSubmitType;
import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.GitRepositoryManager;
import com.google.gerrit.server.git.UpdateException; import com.google.gerrit.server.git.UpdateException;
import com.google.gerrit.server.patch.PatchListNotAvailableException;
import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.project.NoSuchChangeException;
import com.google.gwtorm.server.OrmException; import com.google.gwtorm.server.OrmException;
import com.google.inject.Inject; import com.google.inject.Inject;
@@ -264,7 +265,7 @@ class RevisionApiImpl implements RevisionApi {
return ImmutableSet.copyOf((Iterable<String>) listFiles return ImmutableSet.copyOf((Iterable<String>) listFiles
.setReviewed(true) .setReviewed(true)
.apply(revision).value()); .apply(revision).value());
} catch (OrmException | IOException e) { } catch (OrmException | IOException | PatchListNotAvailableException e) {
throw new RestApiException("Cannot list reviewed files", e); throw new RestApiException("Cannot list reviewed files", e);
} }
} }
@@ -293,7 +294,7 @@ class RevisionApiImpl implements RevisionApi {
public Map<String, FileInfo> files() throws RestApiException { public Map<String, FileInfo> files() throws RestApiException {
try { try {
return (Map<String, FileInfo>)listFiles.apply(revision).value(); return (Map<String, FileInfo>)listFiles.apply(revision).value();
} catch (OrmException | IOException e) { } catch (OrmException | IOException | PatchListNotAvailableException e) {
throw new RestApiException("Cannot retrieve files", e); throw new RestApiException("Cannot retrieve files", e);
} }
} }
@@ -304,7 +305,7 @@ class RevisionApiImpl implements RevisionApi {
try { try {
return (Map<String, FileInfo>) listFiles.setBase(base) return (Map<String, FileInfo>) listFiles.setBase(base)
.apply(revision).value(); .apply(revision).value();
} catch (OrmException | IOException e) { } catch (OrmException | IOException | PatchListNotAvailableException e) {
throw new RestApiException("Cannot retrieve files", e); throw new RestApiException("Cannot retrieve files", e);
} }
} }
@@ -315,7 +316,7 @@ class RevisionApiImpl implements RevisionApi {
try { try {
return (Map<String, FileInfo>) listFiles.setParent(parentNum) return (Map<String, FileInfo>) listFiles.setParent(parentNum)
.apply(revision).value(); .apply(revision).value();
} catch (OrmException | IOException e) { } catch (OrmException | IOException | PatchListNotAvailableException e) {
throw new RestApiException("Cannot retrieve files", e); throw new RestApiException("Cannot retrieve files", e);
} }
} }

43
gerrit-server/src/main/java/com/google/gerrit/server/change/Files.java
View File

@@ -138,9 +138,10 @@ public class Files implements ChildCollection<RevisionResource, FileResource> {
} }
@Override @Override
public Response<?> apply(RevisionResource resource) throws AuthException, public Response<?> apply(RevisionResource resource)
BadRequestException, ResourceNotFoundException, OrmException, throws AuthException, BadRequestException, ResourceNotFoundException,
RepositoryNotFoundException, IOException { OrmException, RepositoryNotFoundException, IOException,
PatchListNotAvailableException {
checkOptions(); checkOptions();
if (reviewed) { if (reviewed) {
return Response.ok(reviewed(resource)); return Response.ok(reviewed(resource));
@@ -149,26 +150,22 @@ public class Files implements ChildCollection<RevisionResource, FileResource> {
} }
Response<Map<String, FileInfo>> r; Response<Map<String, FileInfo>> r;
try { if (base != null) {
if (base != null) { RevisionResource baseResource = revisions.parse(
RevisionResource baseResource = revisions.parse( resource.getChangeResource(), IdString.fromDecoded(base));
resource.getChangeResource(), IdString.fromDecoded(base)); r = Response.ok(fileInfoJson.toFileInfoMap(
r = Response.ok(fileInfoJson.toFileInfoMap( resource.getChange(),
resource.getChange(), resource.getPatchSet().getRevision(),
resource.getPatchSet().getRevision(), baseResource.getPatchSet()));
baseResource.getPatchSet())); } else if (parentNum > 0) {
} else if (parentNum > 0) { r = Response.ok(fileInfoJson.toFileInfoMap(
r = Response.ok(fileInfoJson.toFileInfoMap( resource.getChange(),
resource.getChange(), resource.getPatchSet().getRevision(),
resource.getPatchSet().getRevision(), parentNum - 1));
parentNum - 1)); } else {
} else { r = Response.ok(fileInfoJson.toFileInfoMap(
r = Response.ok(fileInfoJson.toFileInfoMap( resource.getChange(),
resource.getChange(), resource.getPatchSet()));
resource.getPatchSet()));
}
} catch (PatchListNotAvailableException e) {
throw new ResourceNotFoundException(e.getMessage());
} }
if (resource.isCacheable()) { if (resource.isCacheable()) {

4
gerrit-server/src/main/java/com/google/gerrit/server/patch/PatchListNotAvailableException.java
View File

@@ -21,6 +21,10 @@ public class PatchListNotAvailableException extends Exception {
super(message); super(message);
} }
public PatchListNotAvailableException(String message, Throwable cause) {
super(message, cause);
}
public PatchListNotAvailableException(Throwable cause) { public PatchListNotAvailableException(Throwable cause) {
super(cause); super(cause);
} }