In ViewFilter, if a revision cannot be found then throw an exception

jacksondt · jacksondt · commit 1ffd4a1f72a4 · 2026-04-01T15:37:38.000-07:00
instead of returning null.

Change-Id: I9c00409c67688d319f6c440afa4ad73fb5ecb7a9
diff --git a/java/com/google/gitiles/ViewFilter.java b/java/com/google/gitiles/ViewFilter.java
@@ -194,7 +194,7 @@ private GitilesView.Builder parseNoCommand(HttpServletRequest req, String repoNa
       return null;
     }
     RevisionParser.Result result = parseRevision(req, path);
-    if (result == null || result.getOldRevision() != null) {
+    if (result.getOldRevision() != null) {
       return null;
     }
     return GitilesView.archive()
@@ -212,9 +212,6 @@ private GitilesView.Builder parseNoCommand(HttpServletRequest req, String repoNa
       return null;
     }
     RevisionParser.Result result = parseRevision(req, path);
-    if (result == null) {
-      return null;
-    }
     if (result.getOldRevision() != null) {
       return parseDiffCommand(repoName, result);
     }
@@ -234,7 +231,7 @@ private GitilesView.Builder parseNoCommand(HttpServletRequest req, String repoNa
       return null;
     }
     RevisionParser.Result result = parseRevision(req, path);
-    if (result == null || result.getOldRevision() != null || result.getPath().isEmpty()) {
+    if (result.getOldRevision() != null || result.getPath().isEmpty()) {
       return null;
     }
     return GitilesView.blame()
@@ -257,9 +254,6 @@ private GitilesView.Builder parseDiffCommand(HttpServletRequest req, String repo
 
   private @Nullable GitilesView.Builder parseDiffCommand(
       String repoName, RevisionParser.Result result) {
-    if (result == null) {
-      return null;
-    }
     return GitilesView.diff()
         .setRepositoryName(repoName)
         .setRevision(result.getRevision())
@@ -273,9 +267,6 @@ private GitilesView.Builder parseDiffCommand(HttpServletRequest req, String repo
       return GitilesView.log().setRepositoryName(repoName);
     }
     RevisionParser.Result result = parseRevision(req, path);
-    if (result == null) {
-      return null;
-    }
     return GitilesView.log()
         .setRepositoryName(repoName)
         .setRevision(result.getRevision())
@@ -294,7 +285,7 @@ private GitilesView.Builder parseShowCommand(HttpServletRequest req, String repo
 
   private @Nullable GitilesView.Builder parseShowCommand(
       String repoName, RevisionParser.Result result) {
-    if (result == null || result.getOldRevision() != null) {
+    if (result.getOldRevision() != null) {
       return null;
     }
     if (result.getPath().isEmpty()) {
@@ -313,7 +304,7 @@ private GitilesView.Builder parseDocCommand(HttpServletRequest req, String repoN
 
   private @Nullable GitilesView.Builder parseDocCommand(
       String repoName, RevisionParser.Result result) {
-    if (result == null || result.getOldRevision() != null) {
+    if (result.getOldRevision() != null) {
       return null;
     }
     GitilesView.Builder b =
@@ -332,7 +323,11 @@ private RevisionParser.Result parseRevision(HttpServletRequest req, String path)
             accessFactory.forRequest(req),
             visibilityCache,
             getBranchRedirect(req));
-    return revParser.parse(checkLeadingSlash(path));
+    RevisionParser.Result rev = revParser.parse(checkLeadingSlash(path));
+    if (rev == null) {
+      throw new GitilesRequestFailureException(FailureReason.OBJECT_NOT_FOUND);
+    }
+    return rev;
   }
 
   private BranchRedirect getBranchRedirect(HttpServletRequest req) {
diff --git a/javatests/com/google/gitiles/ViewFilterTest.java b/javatests/com/google/gitiles/ViewFilterTest.java
@@ -17,6 +17,7 @@
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.Truth.assertWithMessage;
 import static com.google.gitiles.MoreAssert.assertThrows;
+import static java.lang.String.format;
 
 import com.google.common.net.HttpHeaders;
 import java.io.IOException;
@@ -239,6 +240,11 @@ public void path() throws Exception {
     assertThat(view.getRevision().getId()).isEqualTo(master);
     assertThat(view.getPathPart()).isEqualTo("");
 
+    view = getView(format("/repo/+show/%s/", master.getId().name()));
+    assertThat(view.getType()).isEqualTo(GitilesView.Type.PATH);
+    assertThat(view.getRevision().getId()).isEqualTo(master);
+    assertThat(view.getPathPart()).isEqualTo("");
+
     view = getView("/repo/+show/master/foo");
     assertThat(view.getType()).isEqualTo(GitilesView.Type.PATH);
     assertThat(view.getRevision().getId()).isEqualTo(master);
@@ -258,6 +264,18 @@ public void path() throws Exception {
         GitilesRequestFailureException.class, () -> getView("/repo/+show/stable..master/foo"));
   }
 
+  @Test
+  public void revisionNotFound() throws Exception {
+    var exception = assertThrows(GitilesRequestFailureException.class, () -> getView("/repo/+show/0123456789abcdef"));
+    assertThat(exception.getReason()).isEqualTo(GitilesRequestFailureException.FailureReason.OBJECT_NOT_FOUND);
+  }
+
+  @Test
+  public void cannotParse() throws Exception {
+    var exception = assertThrows(GitilesRequestFailureException.class, () -> getView("/repo/+showmaster"));
+    assertThat(exception.getReason()).isEqualTo(GitilesRequestFailureException.FailureReason.CANNOT_PARSE_GITILES_VIEW);
+  }
+
   @Test
   public void doc() throws Exception {
     RevCommit master = repo.branch(MASTER).commit().create();
