Error handling for non-API routes

[bajtos]

In #1611, we added API allowing app developers to serve static assets from their applications. The implementation was focused on enabling the happy path (serve a file that exists and can be fully read) but neglected error handling.

When a static asset is not found and/or cannot not be read, the reject error handler is not invoked and a very bare-bone fallback handler is used instead.

https://github.com/strongloop/loopback-next/blob/0364b59409ce3691babf5f17b5cd444b4e7f72aa/packages/rest/src/rest.server.ts#L654-L666

Let's find a way that enables LB4 app developers to control the error responses for static assets (and any non-API routes in general).

[bajtos]

I have already spent some time thinking about this problem, here are few solutions for consideration.

1. Implement static middleware as a virtual route

#1611 (comment)

In LB4, API requests are handled by a Sequence, which is designed with the assumption that the Sequence is going to handle all requests and implement error handling too (including 404 not found).

IMO, removing this assumption and changing the design of Sequence to add a third possible outcome - let somebody else handle the request - would be a big step backwards and we should avoid that.

Also mounting serve-static middleware at Express layer is bypassing the sequence entirely. This is likely to have unintended consequences that may bite us hard. For example, happens serve-static cannot read a file, e.g. because of a permission problem, then it forward the error via next(err) for the error-handling middleware to take care of that (see here). The problem is that we don't have a way how to invoke the reject sequence action to handle that error! In fact, the sequence may not even use reject and can handle errors differently.

I am wondering: Can we define a special kind of a catch-all route that will:

• match any URL that did not match any API endpoint; i.e. the sequence action findRoute returns this catch-all route if no API endpoint matched the requested URL
• run the express Router where static assets were mounted; i.e. the sequence action invoke runs express routing to handle static assets
• throw HttpError.NotFound when no static asset was found, i.e. the sequence action invoke throws the 404 error or the express Router for static assets called next().

In my mind, such solution would solve most if not all issues:

• static middleware is invoked after any routes
• static middleware is invoked as part of the sequence
• 404 errors are still handled by the sequence

2. Move reject from Sequence to Server

Remove error handling from the Sequence and use the reject action inside RestServer.

class DefaultSequence {
  constructor(/* inject dependencies */) {}

  async handle(context: RequestContext): Promise<void> {
      const {request, response} = context;
      const route = this.findRoute(request);
      const args = await this.parseParams(request, route);
      const result = await this.invoke(route, args);

      debug('%s result -', route.describe(), result);
      this.send(response, result);
  }
}

class RestServer {
  // (...)

  protected _onUnhandledError(req: Request, res: Response, err: Error) { 
    const reject = // get Reject from request context
    reject({request: req, response: res}, err);
  }
}

3. Make final handler customizable

Allow app developers to provide their own error handler function to be used by RestServer.prototype._onUnhandledError, this error handler can implement different behavior than the reject action. For example, reject can always return JSON errors, while server-level handler can implement content-negotiation and return HTML errors to browsers.

Personally, I would prefer to have a single error handler for both API and non-API routes. This can be easily achieved by defining the following reject action:

app.bind(SequenceActions.REJECT).to(
  function forwardErrorToServer(context: HandlerContext, error: Error) {
    throw error;
  }
);

With such a dummy reject action, the design becomes pretty much the same as in 2.

[hacksparrow]

Btw, if the static asset is not found, the request is passed on to the Sequence, and eventually it is caught by the catch in handle. So 404 error is handled by Sequence.

On the other hand, a read permission error is unhandled and actually crashes the app.

[bajtos]

Btw, if the static asset is not found, the request is passed on to the Sequence, and eventually it is caught by the catch in handle. So 404 error is handled by Sequence.

This is true in the current design, where static middleware handlers are executed before our sequence.

Such design has severe performance implications. If you mount a static middleware handler at / (e.g. to server /index.html), then every API request will hit the filesystem first, because the static middleware would try to serve it as a file.

The issue #1785 is keeping track of this problem and possible solutions. One option is to mount static middleware handler after our sequence, in which case 404 errors will be no longer handled by the sequence.

[hacksparrow]

Will report my findings for each option one after.

1. Implement static middleware as a virtual route

Notes:

1. The ideal place for static assets to be registered would be to have a legit entry/entries in the routing table so that findRoute() actually can find them, and then invoke() can take the necessary action.
2. Point 1 can be implemented by using a ControllerRoute without OpenAPI specs.
3. Point 2 is impossible because we need a path. We don't know the files that will be served.
4. Point 3 can be resolved by using a "reserved path" catch-all route. The name of the path will require discussion.
5. Now we want to invoke() to run Express routing to handle static assets.
6. Point 5 is not possible since we did not register our static middleware earlier. AFAIK, new routes cannot be added to the app once it has started.
7. Looks like this approach is not feasible, unless I have misunderstood something.

@bajtos @raymondfeng

[bajtos]

Point 1 can be implemented by using a ControllerRoute without OpenAPI specs.

Our routes are extensible. We have Route that's configured with a single handler function and ControllerRoute that's invoking a method on a controller class.

I think in this particular case, the handler-function-based Route class would be a better option. We can also create a new RouteEntry subclass to handle this specific kind of a route.

Now we want to invoke() to run Express routing to handle static assets.
Point 5 is not possible since we did not register our static middleware earlier. AFAIK, new routes cannot be added to the app once it has started.

I am afraid I don't agree with this argument.

As you can see in RestServer, server.static() API is already building an Express router dedicated to handling of static routes:

https://github.com/strongloop/loopback-next/blob/f9c9c6d6d5eb2b77df641e634a41e86c4b345909/packages/rest/src/rest.server.ts#L599-L607

This router is registered with the internal Express app inside _setupRequestHandler method: https://github.com/strongloop/loopback-next/blob/f9c9c6d6d5eb2b77df641e634a41e86c4b345909/packages/rest/src/rest.server.ts#L220-L229

As I was envisioning the solution, we need to make two changes:

• don't mount _routerForStaticAssets on the internal Express app
• inside the handler function for our special route, we can invoke _routerForStaticAssets to perform the actual handling of requests for static assets

class RestServer {
  // ...
  protected _handleStaticAssetRequest(req, res) {
    return new Promise((resolve, reject) => {
      this._routerForStaticAssets.handle(req, res, (err) => {
        if (err) return reject(err);
        // router called next, which means no route was matched
        return reject(new HttpError.NotFound());
      });
    });
  }

  // somewhere in the code setting up RoutingTable
  // the anonymous RouteEntry object should be refactored into a class
  this.routingTable.catchAllRoute = <RouteEntry>{
    verb: 'get',
    path: '*',
    spec: { /* TODO: this route should be excluded from the spec */ },
    updateBindings(requestContext: Context) {
      // no-op
    },
    
    invokeHandler(
      {request, response}: Context,
      args: OperationArgs,
    ): Promise<OperationRetval> {
      return this._handleStaticAssetRequest(request, response);
    }

    describe(): string {
      return "final route to handle static assets";
    }
  };
}

There is one catch in this approach though: when this special route is invoked and the static asset was found, the "invokeHandler" method never returns, therefore the control is never returned back to the sequence.

A possible solution: wait until finish event is emitted on the response object and then resolve the promise.

class RestServer {
  // ...
 
  protected _handleStaticAssetRequest(req, res) {
    return new Promise((resolve, reject) => {
      const onFinished = () => resolve();
      req.on('finish', onFinished);
      this._routerForStaticAssets.handle(req, res, (err) => {
        req.removeListener('finish', onFinished);
        if (err) return reject(err);
        // Express router called next, which means no route was matched
        return reject(new HttpError.NotFound());
      });
    });
  }
}

The last missing piece I see is how to exclude a route from the generated API spec. Personally, I'd introduce an extension property allowing developers to mark any route as not documented. For example:

const spec: OperationObject = {
  'x-internal': true,

  description: 'return all customers',
  parameters: [
    // ...
  ],
  responses: {
    // ...
  },
};

Few alternatives to x-internal: true

• x-documented: false (in LB 3.x, remoting metadata has documented: false)
• x-private: true