Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some suggestions on error handling #612

Closed
viert opened this issue Jan 14, 2020 · 3 comments
Closed

Some suggestions on error handling #612

viert opened this issue Jan 14, 2020 · 3 comments

Comments

@viert
Copy link

viert commented Jan 14, 2020

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 LoicPoullain added this to Backlog in Issue tracking via automation Jan 31, 2020
This was referenced Feb 17, 2020
@LoicPoullain LoicPoullain moved this from Backlog to Work In Progress in Issue tracking Feb 17, 2020
@LoicPoullain
Copy link
Member

LoicPoullain commented Feb 18, 2020

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.

@LoicPoullain LoicPoullain moved this from Work In Progress to Done / Closed This Release in Issue tracking Feb 18, 2020
@viert
Copy link
Author

viert commented Feb 18, 2020

Great stuff, thanks!

@LoicPoullain
Copy link
Member

Version 1.7 published. Configuration settings.logErrors is now available

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Issue tracking
  
Done / Closed This Release
Development

No branches or pull requests

2 participants