commit b53fc06a4b2238eb084af62490af188e2b93d36d
author Masaya Suzuki <masayasuzuki@google.com> Mon Mar 25 13:35:07 2019 -0700
committer Masaya Suzuki <masayasuzuki@google.com> Thu May 09 17:14:03 2019 -0700
tree dead0f5171317f764a13b40acf9202859e30b064
parent 76346b0810deec731737c9bb2bf54cc46a2c88cc

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