From f05239460073918a23bce8b2bb03bab2d2b93143 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20FIDRY?= <5175937+theofidry@users.noreply.github.com> Date: Mon, 9 Jan 2023 12:16:13 +0100 Subject: [PATCH] ref(angular) Add error-like objects handling (#6446) It is more beneficial to handle error shaped objects instead of just instances of `Error`s --- packages/angular/src/errorhandler.ts | 39 ++++++++++++++++++---- packages/angular/test/errorhandler.test.ts | 33 +++++------------- 2 files changed, 41 insertions(+), 31 deletions(-) diff --git a/packages/angular/src/errorhandler.ts b/packages/angular/src/errorhandler.ts index 8cfe2427f409..cf89b0e69eec 100644 --- a/packages/angular/src/errorhandler.ts +++ b/packages/angular/src/errorhandler.ts @@ -2,7 +2,7 @@ import { HttpErrorResponse } from '@angular/common/http'; import { ErrorHandler as AngularErrorHandler, Inject, Injectable } from '@angular/core'; import * as Sentry from '@sentry/browser'; import { captureException } from '@sentry/browser'; -import { addExceptionMechanism } from '@sentry/utils'; +import { addExceptionMechanism, isString } from '@sentry/utils'; import { runOutsideAngular } from './zone'; @@ -32,7 +32,7 @@ function tryToUnwrapZonejsError(error: unknown): unknown | Error { function extractHttpModuleError(error: HttpErrorResponse): string | Error { // The `error` property of http exception can be either an `Error` object, which we can use directly... - if (error.error instanceof Error) { + if (isErrorOrErrorLikeObject(error.error)) { return error.error; } @@ -50,6 +50,31 @@ function extractHttpModuleError(error: HttpErrorResponse): string | Error { return error.message; } +type ErrorCandidate = { + name?: unknown; + message?: unknown; + stack?: unknown; +}; + +function isErrorOrErrorLikeObject(value: unknown): value is Error { + if (value instanceof Error) { + return true; + } + + if (value === null || typeof value !== 'object') { + return false; + } + + const candidate = value as ErrorCandidate; + + return ( + isString(candidate.name) && + isString(candidate.name) && + isString(candidate.message) && + (undefined === candidate.stack || isString(candidate.stack)) + ); +} + /** * Implementation of Angular's ErrorHandler provider that can be used as a drop-in replacement for the stock one. */ @@ -117,16 +142,16 @@ class SentryErrorHandler implements AngularErrorHandler { protected _defaultExtractor(errorCandidate: unknown): unknown { const error = tryToUnwrapZonejsError(errorCandidate); - // We can handle messages and Error objects directly. - if (typeof error === 'string' || error instanceof Error) { - return error; - } - // If it's http module error, extract as much information from it as we can. if (error instanceof HttpErrorResponse) { return extractHttpModuleError(error); } + // We can handle messages and Error objects directly. + if (typeof error === 'string' || isErrorOrErrorLikeObject(error)) { + return error; + } + // Nothing was extracted, fallback to default error message. return null; } diff --git a/packages/angular/test/errorhandler.test.ts b/packages/angular/test/errorhandler.test.ts index df28e809bb26..e4398fd8aa70 100644 --- a/packages/angular/test/errorhandler.test.ts +++ b/packages/angular/test/errorhandler.test.ts @@ -33,7 +33,7 @@ class CustomError extends Error { } class ErrorLikeShapedClass implements Partial { - constructor(public message: string) {} + constructor(public name: string, public message: string) {} } function createErrorEvent(message: string, innerError: any): ErrorEvent { @@ -118,8 +118,7 @@ describe('SentryErrorHandler', () => { createErrorHandler().handleError(errorLikeWithoutStack); expect(captureExceptionSpy).toHaveBeenCalledTimes(1); - // TODO: to be changed; see https://github.com/getsentry/sentry-javascript/issues/6332 - expect(captureExceptionSpy).toHaveBeenCalledWith('Handled unknown error', expect.any(Function)); + expect(captureExceptionSpy).toHaveBeenCalledWith(errorLikeWithoutStack, expect.any(Function)); }); it('extracts an error-like object with a stack', () => { @@ -132,8 +131,7 @@ describe('SentryErrorHandler', () => { createErrorHandler().handleError(errorLikeWithStack); expect(captureExceptionSpy).toHaveBeenCalledTimes(1); - // TODO: to be changed; see https://github.com/getsentry/sentry-javascript/issues/6332 - expect(captureExceptionSpy).toHaveBeenCalledWith('Handled unknown error', expect.any(Function)); + expect(captureExceptionSpy).toHaveBeenCalledWith(errorLikeWithStack, expect.any(Function)); }); it('extracts an object that could look like an error but is not (does not have a message)', () => { @@ -150,7 +148,6 @@ describe('SentryErrorHandler', () => { it('extracts an object that could look like an error but is not (does not have an explicit name)', () => { const notErr: Partial = { - // missing name; but actually is always there as part of the Object prototype message: 'something failed.', }; @@ -194,12 +191,12 @@ describe('SentryErrorHandler', () => { }); it('extracts an instance of class not extending Error but that has an error-like shape', () => { - const err = new ErrorLikeShapedClass('something happened'); + const err = new ErrorLikeShapedClass('sentry-error', 'something happened'); createErrorHandler().handleError(err); expect(captureExceptionSpy).toHaveBeenCalledTimes(1); - expect(captureExceptionSpy).toHaveBeenCalledWith('Handled unknown error', expect.any(Function)); + expect(captureExceptionSpy).toHaveBeenCalledWith(err, expect.any(Function)); }); it('extracts an instance of a class that does not extend Error and does not have an error-like shape', () => { @@ -304,11 +301,7 @@ describe('SentryErrorHandler', () => { createErrorHandler().handleError(err); expect(captureExceptionSpy).toHaveBeenCalledTimes(1); - // TODO: to be changed; see https://github.com/getsentry/sentry-javascript/issues/6332 - expect(captureExceptionSpy).toHaveBeenCalledWith( - 'Http failure response for (unknown url): undefined undefined', - expect.any(Function), - ); + expect(captureExceptionSpy).toHaveBeenCalledWith(errorLikeWithoutStack, expect.any(Function)); }); it('extracts an `HttpErrorResponse` with error-like object with a stack', () => { @@ -322,11 +315,7 @@ describe('SentryErrorHandler', () => { createErrorHandler().handleError(err); expect(captureExceptionSpy).toHaveBeenCalledTimes(1); - // TODO: to be changed; see https://github.com/getsentry/sentry-javascript/issues/6332 - expect(captureExceptionSpy).toHaveBeenCalledWith( - 'Http failure response for (unknown url): undefined undefined', - expect.any(Function), - ); + expect(captureExceptionSpy).toHaveBeenCalledWith(errorLikeWithStack, expect.any(Function)); }); it('extracts an `HttpErrorResponse` with an object that could look like an error but is not (does not have a message)', () => { @@ -347,7 +336,6 @@ describe('SentryErrorHandler', () => { it('extracts an `HttpErrorResponse` with an object that could look like an error but is not (does not have an explicit name)', () => { const notErr: Partial = { - // missing name; but actually is always there as part of the Object prototype message: 'something failed.', }; const err = new HttpErrorResponse({ error: notErr }); @@ -453,16 +441,13 @@ describe('SentryErrorHandler', () => { }); it('extracts an `HttpErrorResponse` with an instance of class not extending Error but that has an error-like shape', () => { - const innerErr = new ErrorLikeShapedClass('something happened'); + const innerErr = new ErrorLikeShapedClass('sentry-error', 'something happened'); const err = new HttpErrorResponse({ error: innerErr }); createErrorHandler().handleError(err); expect(captureExceptionSpy).toHaveBeenCalledTimes(1); - expect(captureExceptionSpy).toHaveBeenCalledWith( - 'Http failure response for (unknown url): undefined undefined', - expect.any(Function), - ); + expect(captureExceptionSpy).toHaveBeenCalledWith(innerErr, expect.any(Function)); }); it('extracts an `HttpErrorResponse` with an instance of a class that does not extend Error and does not have an error-like shape', () => {