FoalTS / foal

Full-featured Node.js framework, with no complexity. 🚀 Simple and easy to use, TypeScript-based and well-documented.
https://foalts.org/
MIT License
1.89k stars 140 forks source link

Some suggestions on error handling #612

Closed viert closed 4 years ago

viert commented 4 years ago

Thanks for the fantastic framework!

I'd like to suggest some improvements which are not supposed to change the overall architecture but could improve UX of error handling.

In most cases the most of the work happens in services. If a service fails to do something (find an object in db, parse user input, etc.) the fastest way to stop doing anything and return an error is to throw an Error from the service, so the error handler could catch it and provide a proper response.

That's how I deal with it:

export class ApiError extends Error {
  name: string = "ApiError";
  code: number = 400;
  message: string;
  payload: { [key: string]: any } | null = null;

  constructor(
    msg: string,
    statusCode?: number,
    payload?: { [key: string]: any }
  ) {
    super(msg);
    this.message = msg;
    if (statusCode) {
      this.code = statusCode;
    }
    if (payload) {
      this.payload = payload;
    }
  }

  toObject(): { [key: string]: any } {
    let data: { [key: string]: any } = {
      error: this.message
    };
    if (this.payload) {
      data.data = this.payload;
    }
    return data;
  }
}

export class Forbidden extends ApiError {
  code = 403;
}

export class Conflict extends ApiError{
  code = 409;
}

So far so good I have implemented this and created an error handler behaving like this:

export class AppController {
  @dependency store: StoreService;
  subControllers = [controller("/api/v1/account", ApiV1AccountController)];

  async init() {
    await this.store.init();
  }

  handleError(
    error: Error,
    _ctx: Context
  ): HttpResponse | Promise<HttpResponse> {
    if (error instanceof ApiError) {
      const resp = new HttpResponseOK(error.toObject());
      resp.statusCode = error.code;
      return resp;
    }
    return new HttpResponseBadRequest(error.message);
  }
}

So, to the suggestions:

  1. I'd like to instantiate a HttpResponse object accepting statusCode as a param. Right now I have to create new HttpResponseOK and change its statusCode afterwards. I haven't found any classes providing this behavior and HttpResponse itself is an abstract class which can't be instantiated.

  2. I'd like to have a possibility to suppress logging these exceptions as they are a part of my application, thus, properly handled and may not need to be automatically logged (with a full stack trace).

LoicPoullain commented 4 years ago

Hi @viert

Sorry for the late reply, I needed to think about this.

Thanks for the fantastic framework!

Thanks!

  1. I'd like to instantiate a HttpResponse object accepting statusCode as a param. Right now I have to create new HttpResponseOK and change its statusCode afterwards. I haven't found any classes providing this behavior and HttpResponse itself is an abstract class which can't be instantiated.

The best way to do this would be to override the class HttpResponseClientError and accept a statusCode in the constructor.

class ErrorHttpResponse extends HttpResponseClientError {

  constructor(readonly statusCode: number, body?: any, options: { stream?: boolean } = {}) {
    super(body, options);
  }

}

Overriding HttpResponseClientError instead of HttpResponse is better because FoalTS treats HttpResponseClientError and HttpResponseRedirection differently for example.

2. I'd like to have a possibility to suppress logging these exceptions as they are a part of my application, thus, properly handled and may not need to be automatically logged (with a full stack trace).

A PR is on its way. It will be possible to disable error logging with the configuration key settings.logErrors.

viert commented 4 years ago

Great stuff, thanks!

LoicPoullain commented 4 years ago

Version 1.7 published. Configuration settings.logErrors is now available