Introduce GitilesRequestFailureException and DefaultErrorHandlingFilter
This change introduces an exception for failing a request. This isolates
the error checks and the error handling. For example, some part of
Gitiles application code checks if a request URL points to a valid
object. When it finds the URL pointing to a non-existing object, the
current code is also responsible for rendering an error for a user. With
this change, the application code becomes not responsible for rendering
an error page, and it just needs to throw a
GitilesRequestFailureException with an appropriate failure reason.
This isolation has multiple benefits:
- Unified error handling code
Because the error page rendering is done in multiple places, some
code uses 400 Bad Request while others use 404 Not Found for the
same object not found case. This behavior will be unified.
- Customizable error handler
A user who uses Gitiles as a Java library can customize error
handlers. This lets them log an error in the server log.
- Exception stack trace is preserved
Previously, the exception stack trace is not used at all because
Gitiles just renders an error page with
HttpServletResponse#sendError. Now with a customized error handler,
a Gitiles user have a chance to log exception stack trace in the
server log.
* Design Decisions
** Subclass vs. enum
GitilesRequestFailureException could have defined as a abstract base
class that each subclass represents a different situation. This is
useful for situation where there are two types of callers; the ones that
do not care about the subtypes and want to handle all subtypes of
GitilesRequestFailureException in the same way, and the ones that want
to handle some subtypes of GitilesRequestFailureException differently.
For example, EOFException is defined as a subtype of IOException. For
those that don't care about EOF or not can handle all exceptions thrown
from IO with just `catch (IOException e)`, while those that want to
handle EOF as a special case can handle it separately.
This GitilesRequestFailureException is intended to be thrown when a
request cannot be processed and should be returned immediately.
Conceptually, this is same as `System.exit` but for requests. This
should be handled only by DefaultErrorHandlingFilter or its equivalent.
Handling this exception in other application code is a misuse of this
exception and they should use a different exception. By making it
possible to subclass GitilesRequestFailureException, it makes it easier
to misuse this exception since the application code can partially handle
this exception easily. Based on Scott Meyers advice "Make interfaces
easy to use correctly and hard to use incorrectly" [1], this class
doesn't take subclassing approach and use enum to represent an error
type. An enum serves enough for DefaultErrorHandlingFilter's purpose.
As a side effect by using an enum, we can now send a programmatically
parsable error type that is finer than HTTP status.
DefaultErrorHandlingFilter sets the enum to the HTTP response header.
This makes it easy for the Gitiles REST API users to handle or diagnose
errors.
** Extensibility
As seen in the constructor of GitilesServlet, Gitiles can be extended by
a user. There can be a case that user code wants to terminate a request
immediately. For example, a user would want to set a quota on the number
of requests, and they might want to response to a request immediately
when a quota is depleted with 429 Too Many Requests.
GitilesRequestFailureException and its FailureReason enum cannot be
extended by a user without modifying the Gitiles source code. They
cannot throw GitilesRequestFailureException in this case.
One approach for this problem is to define a class like
ServiceMayNotContinueException in JGit. This exception takes an HTTP
status code and JGit custom hooks can throw this exception with
arbitrary status code.
This change takes a different approach. If a user wants to return a user
defined error, they can do so by defining a user defined exception and
its corresponding error handler. For example, they can define their own
RuntimeException that looks like GitilesRequestFailureException, and
define a new error handling filter that catches it.
public final class MyRequestFailureException extends RuntimeException {
private final FailureReason reason;
public MyRequestFailureException(FailureReason reason) {
super();
this.reason = reason;
}
public FailureReason getReason() {
return reason;
}
enum FailureReason {
QUOTA_EXCEEDED(429);
}
private final int httpStatusCode;
FailureReason(int httpStatusCode) {
this.httpStatusCode = httpStatusCode;
}
public int getHttpStatusCode() {
return httpStatusCode;
}
}
public class MyErrorHandlingFilter extends AbstractHttpFilter {
private static final DefaultErrorHandlingFilter delegate =
new DefaultErrorHandlingFilter();
@Override
public void doFilter(HttpServletRequest req, HttpServletResponse res, FilterChain chain)
throws IOException, ServletException {
try {
delegate(req, res, chain);
} catch (MyRequestFailureException e) {
res.setHeader(
DefaultErrorHandlingFilter.GITILES_ERROR,
e.getReason().toString());
res.sendError(e.getReason().getHttpStatusCode());
}
}
}
Compared to ServiceMayNotContinueException approach, this gives a user
greater flexibility. ServiceMayNotContinueException lets a user specify
only HTTP status code and an error message. This custom exception
handler approach lets a user decide how an error should be surfaced
completely. For example, a user can extend the exception and add the
time that an HTTP request caller should retry to recover the quota. The
custom error handler can show such data in the error page or in the HTTP
header for a REST API client.
* Behavioral change
When an object specified by the URL is not suitable for the view (e.g.
trying to show a blob as a tree), it used to show SC_NOT_FOUND. It now
returns SC_BAD_REQUEST.
[1]: https://www.aristeia.com/Papers/IEEE_Software_JulAug_2004_revised.htm
Change-Id: I436309324e24be5de6358b96237389ba93dbe9ff
diff --git a/java/com/google/gitiles/DefaultErrorHandlingFilter.java b/java/com/google/gitiles/DefaultErrorHandlingFilter.java
new file mode 100644
index 0000000..958b800
--- /dev/null
+++ b/java/com/google/gitiles/DefaultErrorHandlingFilter.java
@@ -0,0 +1,63 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.gitiles;
+
+import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
+import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
+import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
+import static org.eclipse.jgit.http.server.GitSmartHttpTools.sendError;
+
+import java.io.IOException;
+import javax.servlet.FilterChain;
+import javax.servlet.ServletException;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.eclipse.jgit.errors.AmbiguousObjectException;
+import org.eclipse.jgit.errors.RepositoryNotFoundException;
+import org.eclipse.jgit.transport.ServiceMayNotContinueException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/** Convert exceptions into HTTP response. */
+public class DefaultErrorHandlingFilter extends AbstractHttpFilter {
+ private static final Logger log = LoggerFactory.getLogger(DefaultErrorHandlingFilter.class);
+
+ /** HTTP header that indicates an error detail. */
+ public static final String GITILES_ERROR = "X-Gitiles-Error";
+
+ @Override
+ public void doFilter(HttpServletRequest req, HttpServletResponse res, FilterChain chain)
+ throws IOException, ServletException {
+ try {
+ chain.doFilter(req, res);
+ } catch (GitilesRequestFailureException e) {
+ res.setHeader(GITILES_ERROR, e.getReason().toString());
+ String publicMessage = e.getPublicErrorMessage();
+ if (publicMessage != null) {
+ res.sendError(e.getReason().getHttpStatusCode(), publicMessage);
+ } else {
+ res.sendError(e.getReason().getHttpStatusCode());
+ }
+ } catch (RepositoryNotFoundException e) {
+ res.sendError(SC_NOT_FOUND);
+ } catch (AmbiguousObjectException e) {
+ res.sendError(SC_BAD_REQUEST);
+ } catch (ServiceMayNotContinueException e) {
+ sendError(req, res, e.getStatusCode(), e.getMessage());
+ } catch (IOException | ServletException err) {
+ log.warn("Internal server error", err);
+ res.sendError(SC_INTERNAL_SERVER_ERROR);
+ }
+ }
+}
diff --git a/java/com/google/gitiles/GitilesFilter.java b/java/com/google/gitiles/GitilesFilter.java
index 59e8acd..2c810bb 100644
--- a/java/com/google/gitiles/GitilesFilter.java
+++ b/java/com/google/gitiles/GitilesFilter.java
@@ -37,6 +37,7 @@
import java.util.Iterator;
import java.util.Map;
import java.util.regex.Pattern;
+import javax.annotation.Nullable;
import javax.servlet.Filter;
import javax.servlet.FilterChain;
import javax.servlet.FilterConfig;
@@ -174,20 +175,22 @@
private TimeCache timeCache;
private BlameCache blameCache;
private GitwebRedirectFilter gitwebRedirect;
+ private Filter errorHandler;
private boolean initialized;
GitilesFilter() {}
GitilesFilter(
Config config,
- Renderer renderer,
- GitilesUrls urls,
- GitilesAccess.Factory accessFactory,
- final RepositoryResolver<HttpServletRequest> resolver,
- VisibilityCache visibilityCache,
- TimeCache timeCache,
- BlameCache blameCache,
- GitwebRedirectFilter gitwebRedirect) {
+ @Nullable Renderer renderer,
+ @Nullable GitilesUrls urls,
+ @Nullable GitilesAccess.Factory accessFactory,
+ @Nullable RepositoryResolver<HttpServletRequest> resolver,
+ @Nullable VisibilityCache visibilityCache,
+ @Nullable TimeCache timeCache,
+ @Nullable BlameCache blameCache,
+ @Nullable GitwebRedirectFilter gitwebRedirect,
+ @Nullable Filter errorHandler) {
this.config = checkNotNull(config, "config");
this.renderer = renderer;
this.urls = urls;
@@ -199,6 +202,7 @@
if (resolver != null) {
this.resolver = resolver;
}
+ this.errorHandler = errorHandler;
}
@Override
@@ -232,6 +236,12 @@
initialized = true;
}
+ @Override
+ protected ServletBinder register(ServletBinder b) {
+ b.through(errorHandler);
+ return b;
+ }
+
public synchronized BaseServlet getDefaultHandler(GitilesView.Type view) {
checkNotInitialized();
switch (view) {
@@ -300,6 +310,7 @@
setDefaultTimeCache();
setDefaultBlameCache();
setDefaultGitwebRedirect();
+ setDefaultErrorHandler();
}
private void setDefaultConfig(FilterConfig filterConfig) throws ServletException {
@@ -407,6 +418,12 @@
}
}
+ private void setDefaultErrorHandler() {
+ if (errorHandler == null) {
+ errorHandler = new DefaultErrorHandlingFilter();
+ }
+ }
+
private static String getBaseGitUrl(Config config) throws ServletException {
String baseGitUrl = config.getString("gitiles", null, "baseGitUrl");
if (baseGitUrl == null) {
diff --git a/java/com/google/gitiles/GitilesRequestFailureException.java b/java/com/google/gitiles/GitilesRequestFailureException.java
new file mode 100644
index 0000000..91f9cf8
--- /dev/null
+++ b/java/com/google/gitiles/GitilesRequestFailureException.java
@@ -0,0 +1,163 @@
+// Copyright 2019 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// https://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.gitiles;
+
+import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
+import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;
+import static javax.servlet.http.HttpServletResponse.SC_GONE;
+import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
+import static javax.servlet.http.HttpServletResponse.SC_NOT_FOUND;
+import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;
+
+import javax.annotation.Nullable;
+
+/**
+ * Indicates the request should be failed.
+ *
+ * <p>When an HTTP request should be failed, throw this exception instead of directly setting an
+ * HTTP status code. The exception is caught by an error handler in {@link GitilesFilter}. By
+ * default, {@link DefaultErrorHandlingFilter} handles this exception and set an appropriate HTTP
+ * status code. If you want to customize how the error is surfaced, like changing the error page
+ * rendering, replace this error handler from {@link GitilesServlet}.
+ *
+ * <h2>Extending the error space</h2>
+ *
+ * <p>{@link GitilesServlet} lets you customize some parts of Gitiles, and sometimes you would like
+ * to create a new {@link FailureReason}. For example, a customized {@code RepositoryResolver} might
+ * check a request quota and reject a request if a client sends too many requests. In that case, you
+ * can define your own {@link RuntimeException} and an error handler.
+ *
+ * <pre><code>
+ * public final class MyRequestFailureException extends RuntimeException {
+ * private final FailureReason reason;
+ *
+ * public MyRequestFailureException(FailureReason reason) {
+ * super();
+ * this.reason = reason;
+ * }
+ *
+ * public FailureReason getReason() {
+ * return reason;
+ * }
+ *
+ * enum FailureReason {
+ * QUOTA_EXCEEDED(429);
+ * }
+ *
+ * private final int httpStatusCode;
+ *
+ * FailureReason(int httpStatusCode) {
+ * this.httpStatusCode = httpStatusCode;
+ * }
+ *
+ * public int getHttpStatusCode() {
+ * return httpStatusCode;
+ * }
+ * }
+ *
+ * public class MyErrorHandlingFilter extends AbstractHttpFilter {
+ * private static final DefaultErrorHandlingFilter delegate =
+ * new DefaultErrorHandlingFilter();
+ *
+ * {@literal @}Override
+ * public void doFilter(HttpServletRequest req, HttpServletResponse res, FilterChain chain)
+ * throws IOException, ServletException {
+ * try {
+ * delegate.doFilter(req, res, chain);
+ * } catch (MyRequestFailureException e) {
+ * res.setHeader(DefaultErrorHandlingFilter.GITILES_ERROR, e.getReason().toString());
+ * res.sendError(e.getReason().getHttpStatusCode());
+ * }
+ * }
+ * }
+ * </code></pre>
+ *
+ * <p>{@code RepositoryResolver} can throw {@code MyRequestFailureException} and {@code
+ * MyErrorHandlingFilter} will handle that. You can control how the error should be surfaced.
+ */
+public final class GitilesRequestFailureException extends RuntimeException {
+ private final FailureReason reason;
+ private String publicErrorMessage;
+
+ public GitilesRequestFailureException(FailureReason reason) {
+ super();
+ this.reason = reason;
+ }
+
+ public GitilesRequestFailureException(FailureReason reason, Throwable cause) {
+ super(cause);
+ this.reason = reason;
+ }
+
+ public GitilesRequestFailureException withPublicErrorMessage(String format, Object... params) {
+ this.publicErrorMessage = String.format(format, params);
+ return this;
+ }
+
+ public FailureReason getReason() {
+ return reason;
+ }
+
+ @Nullable
+ public String getPublicErrorMessage() {
+ return publicErrorMessage;
+ }
+
+ /** The request failure reason. */
+ public enum FailureReason {
+ /** The object specified by the URL is ambiguous and Gitiles cannot identify one object. */
+ AMBIGUOUS_OBJECT(SC_BAD_REQUEST),
+ /** There's nothing to show for blame (e.g. the file is empty). */
+ BLAME_REGION_NOT_FOUND(SC_NOT_FOUND),
+ /** Cannot parse URL as a Gitiles URL. */
+ CANNOT_PARSE_GITILES_VIEW(SC_NOT_FOUND),
+ /** URL parameters are not valid. */
+ INCORECT_PARAMETER(SC_BAD_REQUEST),
+ /**
+ * The object specified by the URL is not suitable for the view (e.g. trying to show a blob as a
+ * tree).
+ */
+ INCORRECT_OBJECT_TYPE(SC_BAD_REQUEST),
+ /** Markdown rendering is not enabled. */
+ MARKDOWN_NOT_ENABLED(SC_NOT_FOUND),
+ /** Request is not authorized. */
+ NOT_AUTHORIZED(SC_UNAUTHORIZED),
+ /** Object is not found. */
+ OBJECT_NOT_FOUND(SC_NOT_FOUND),
+ /** Object is too large to show. */
+ OBJECT_TOO_LARGE(SC_INTERNAL_SERVER_ERROR),
+ /** Repository is not found. */
+ REPOSITORY_NOT_FOUND(SC_NOT_FOUND),
+ /** Gitiles is not enabled for the repository. */
+ SERVICE_NOT_ENABLED(SC_FORBIDDEN),
+ /** GitWeb URL cannot be converted to Gitiles URL. */
+ UNSUPPORTED_GITWEB_URL(SC_GONE),
+ /** The specified object's type is not supported. */
+ UNSUPPORTED_OBJECT_TYPE(SC_NOT_FOUND),
+ /** The specified format type is not supported. */
+ UNSUPPORTED_RESPONSE_FORMAT(SC_BAD_REQUEST),
+ /** The specified revision names are not supported. */
+ UNSUPPORTED_REVISION_NAMES(SC_BAD_REQUEST);
+
+ private final int httpStatusCode;
+
+ FailureReason(int httpStatusCode) {
+ this.httpStatusCode = httpStatusCode;
+ }
+
+ public int getHttpStatusCode() {
+ return httpStatusCode;
+ }
+ }
+}
diff --git a/java/com/google/gitiles/GitilesServlet.java b/java/com/google/gitiles/GitilesServlet.java
index 56f4c61..6d10c0c 100644
--- a/java/com/google/gitiles/GitilesServlet.java
+++ b/java/com/google/gitiles/GitilesServlet.java
@@ -52,6 +52,30 @@
@Nullable TimeCache timeCache,
@Nullable BlameCache blameCache,
@Nullable GitwebRedirectFilter gitwebRedirect) {
+ this(
+ config,
+ renderer,
+ urls,
+ accessFactory,
+ resolver,
+ visibilityCache,
+ timeCache,
+ blameCache,
+ gitwebRedirect,
+ null);
+ }
+
+ public GitilesServlet(
+ Config config,
+ @Nullable Renderer renderer,
+ @Nullable GitilesUrls urls,
+ @Nullable GitilesAccess.Factory accessFactory,
+ @Nullable RepositoryResolver<HttpServletRequest> resolver,
+ @Nullable VisibilityCache visibilityCache,
+ @Nullable TimeCache timeCache,
+ @Nullable BlameCache blameCache,
+ @Nullable GitwebRedirectFilter gitwebRedirect,
+ @Nullable Filter errorHandler) {
super(
new GitilesFilter(
config,
@@ -62,7 +86,8 @@
visibilityCache,
timeCache,
blameCache,
- gitwebRedirect));
+ gitwebRedirect,
+ errorHandler));
}
public GitilesServlet() {
diff --git a/javatests/com/google/gitiles/DefaultErrorHandlingFilterTest.java b/javatests/com/google/gitiles/DefaultErrorHandlingFilterTest.java
new file mode 100644
index 0000000..6832658
--- /dev/null
+++ b/javatests/com/google/gitiles/DefaultErrorHandlingFilterTest.java
@@ -0,0 +1,43 @@
+package com.google.gitiles;
+
+import static com.google.common.truth.Truth.assertThat;
+import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
+
+import com.google.gitiles.GitilesRequestFailureException.FailureReason;
+import javax.servlet.http.HttpServlet;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+import org.eclipse.jgit.http.server.glue.MetaFilter;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.junit.runners.JUnit4;
+
+@RunWith(JUnit4.class)
+public class DefaultErrorHandlingFilterTest {
+ private final MetaFilter mf = new MetaFilter();
+
+ @Before
+ public void setUp() {
+ mf.serve("*").through(new DefaultErrorHandlingFilter()).with(new TestServlet());
+ }
+
+ @Test
+ public void renderError() throws Exception {
+ FakeHttpServletRequest req = FakeHttpServletRequest.newRequest();
+ req.setPathInfo("/");
+ FakeHttpServletResponse resp = new FakeHttpServletResponse();
+ mf.doFilter(req, resp, (unusedReq, unusedResp) -> {});
+
+ assertThat(resp.getStatus()).isEqualTo(SC_BAD_REQUEST);
+ assertThat(resp.getHeader(DefaultErrorHandlingFilter.GITILES_ERROR))
+ .isEqualTo("INCORECT_PARAMETER");
+ }
+
+ private static class TestServlet extends HttpServlet {
+ @Override
+ protected void doGet(HttpServletRequest req, HttpServletResponse res) {
+ throw new GitilesRequestFailureException(FailureReason.INCORECT_PARAMETER);
+ }
+ }
+}
