From 6c7d87f7078e5871a35c5817cc40d06580d87742 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20FIDRY?= Date: Fri, 16 Dec 2022 19:03:03 +0100 Subject: [PATCH 1/3] ref(angular) Add error-like objects handling Closes #6332. As discussed in the linked issue, it is more beneficial to handle error shaped objects instead of just instances of `Error`s. --- packages/angular/src/errorhandler.ts | 41 ++++++++++++++++++---- packages/angular/test/errorhandler.test.ts | 33 +++++------------ 2 files changed, 44 insertions(+), 30 deletions(-) diff --git a/packages/angular/src/errorhandler.ts b/packages/angular/src/errorhandler.ts index 8cfe2427f409..9f6757f4b3c0 100644 --- a/packages/angular/src/errorhandler.ts +++ b/packages/angular/src/errorhandler.ts @@ -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,35 @@ function extractHttpModuleError(error: HttpErrorResponse): string | Error { return error.message; } +function isObject(value: unknown): value is Record { + return value !== null && typeof value === 'object'; +} + +function hasOwnProperty, PropertyType extends PropertyKey>( + object: ObjectType, + propertyName: PropertyType, +): object is ObjectType & Record { + return Object.prototype.hasOwnProperty.call(object, propertyName); +} + +function hasErrorName(value: Record): value is Pick { + return hasOwnProperty(value, 'name') && typeof value.name === 'string'; +} + +function hasErrorMessage(value: Record): value is Pick { + return hasOwnProperty(value, 'message') && typeof value.message === 'string'; +} + +function hasErrorStack(value: Record): value is Pick { + return !hasOwnProperty(value, 'stack') || typeof value.stack === 'string'; +} + +function isErrorOrErrorLikeObject(value: unknown): value is Error { + return ( + value instanceof Error || (isObject(value) && hasErrorName(value) && hasErrorMessage(value) && hasErrorStack(value)) + ); +} + /** * Implementation of Angular's ErrorHandler provider that can be used as a drop-in replacement for the stock one. */ @@ -117,16 +146,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', () => { From 513af89b091f673a1aa28fd03ded4e8ac44d151d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20FIDRY?= Date: Tue, 20 Dec 2022 17:31:07 +0100 Subject: [PATCH 2/3] Inline --- packages/angular/src/errorhandler.ts | 35 ++++++++-------------------- 1 file changed, 10 insertions(+), 25 deletions(-) diff --git a/packages/angular/src/errorhandler.ts b/packages/angular/src/errorhandler.ts index 9f6757f4b3c0..52c565c38458 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'; @@ -50,32 +50,17 @@ function extractHttpModuleError(error: HttpErrorResponse): string | Error { return error.message; } -function isObject(value: unknown): value is Record { - return value !== null && typeof value === 'object'; -} - -function hasOwnProperty, PropertyType extends PropertyKey>( - object: ObjectType, - propertyName: PropertyType, -): object is ObjectType & Record { - return Object.prototype.hasOwnProperty.call(object, propertyName); -} - -function hasErrorName(value: Record): value is Pick { - return hasOwnProperty(value, 'name') && typeof value.name === 'string'; -} - -function hasErrorMessage(value: Record): value is Pick { - return hasOwnProperty(value, 'message') && typeof value.message === 'string'; -} - -function hasErrorStack(value: Record): value is Pick { - return !hasOwnProperty(value, 'stack') || typeof value.stack === 'string'; -} - function isErrorOrErrorLikeObject(value: unknown): value is Error { + if (value instanceof Error) { + return true; + } + return ( - value instanceof Error || (isObject(value) && hasErrorName(value) && hasErrorMessage(value) && hasErrorStack(value)) + value !== null && + typeof value === 'object' && + isString((value as Partial).name) && + isString((value as Partial).message) && + (undefined === (value as Partial).stack || isString((value as Partial).stack)) ); } From a25e6c5c8a9060618c38edf9d69b281d376b7d45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20FIDRY?= Date: Wed, 21 Dec 2022 10:36:58 +0100 Subject: [PATCH 3/3] Simplify the condition --- packages/angular/src/errorhandler.ts | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/packages/angular/src/errorhandler.ts b/packages/angular/src/errorhandler.ts index 52c565c38458..cf89b0e69eec 100644 --- a/packages/angular/src/errorhandler.ts +++ b/packages/angular/src/errorhandler.ts @@ -50,17 +50,28 @@ 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 ( - value !== null && - typeof value === 'object' && - isString((value as Partial).name) && - isString((value as Partial).message) && - (undefined === (value as Partial).stack || isString((value as Partial).stack)) + isString(candidate.name) && + isString(candidate.name) && + isString(candidate.message) && + (undefined === candidate.stack || isString(candidate.stack)) ); }