From 3c2795009d01ed41ae3883f9113b7aeaf37b775b Mon Sep 17 00:00:00 2001 From: Ryankey Date: Wed, 17 Aug 2022 14:43:04 -0400 Subject: [PATCH 1/5] Wrap function to protect against nulls/undefined A simple solution for #1570. It feels like we could also just skip wrapping anything that isn't a function on L11, and immediately returning a no-op stop, but I was less comfortable making that change. --- packages/core/src/tools/instrumentMethod.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tools/instrumentMethod.ts b/packages/core/src/tools/instrumentMethod.ts index 1b8ae70448..7a73346f93 100644 --- a/packages/core/src/tools/instrumentMethod.ts +++ b/packages/core/src/tools/instrumentMethod.ts @@ -13,8 +13,10 @@ export function instrumentMethod { - // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return instrumentation.apply(this, arguments as unknown as Parameters) + if (typeof instrumentation === 'function') { + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return instrumentation.apply(this, arguments as unknown as Parameters) + } } object[method] = instrumentationWrapper as OBJECT[METHOD] From ffae327c59213e55abea239b77b9f8e6a96a190e Mon Sep 17 00:00:00 2001 From: Ryan Keyes Date: Wed, 17 Aug 2022 15:39:19 -0400 Subject: [PATCH 2/5] add unit tests for instrument method when null --- .../core/src/tools/instrumentMethod.spec.ts | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/packages/core/src/tools/instrumentMethod.spec.ts b/packages/core/src/tools/instrumentMethod.spec.ts index b64111af85..6af9953675 100644 --- a/packages/core/src/tools/instrumentMethod.spec.ts +++ b/packages/core/src/tools/instrumentMethod.spec.ts @@ -54,6 +54,17 @@ describe('instrumentMethod', () => { expect(instrumentationSpy).toHaveBeenCalled() }) + it('handles undefined methods with third party wrappers after stop', () => { + const object: { method?: () => number } = {} + const instrumentationStub = () => 2 + const { stop } = instrumentMethod(object, 'method', () => instrumentationStub) + + thirdPartyInstrumentation(object) + + stop() + expect(object.method).not.toThrow() + }) + describe('stop()', () => { it('restores the original behavior', () => { const object = { method: () => 1 } @@ -102,9 +113,11 @@ describe('instrumentMethod', () => { }) }) - function thirdPartyInstrumentation(object: { method: () => number }) { + function thirdPartyInstrumentation(object: { method?: () => number }) { const originalMethod = object.method - object.method = () => originalMethod() + 2 + if (typeof originalMethod === 'function') { + object.method = () => originalMethod() + 2 + } } }) From df4522a2fba9ff1efa11690741b4d66b01f4d953 Mon Sep 17 00:00:00 2001 From: Ryan Keyes Date: Wed, 17 Aug 2022 16:17:52 -0400 Subject: [PATCH 3/5] fix type to allow null return --- packages/core/src/tools/instrumentMethod.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/core/src/tools/instrumentMethod.ts b/packages/core/src/tools/instrumentMethod.ts index 7a73346f93..5f9a156200 100644 --- a/packages/core/src/tools/instrumentMethod.ts +++ b/packages/core/src/tools/instrumentMethod.ts @@ -12,11 +12,12 @@ export function instrumentMethod { - if (typeof instrumentation === 'function') { - // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return instrumentation.apply(this, arguments as unknown as Parameters) - } + const instrumentationWrapper = function (this: OBJECT): ReturnType | null { + if (typeof instrumentation === 'function') { + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return instrumentation.apply(this, arguments as unknown as Parameters) + } + return null } object[method] = instrumentationWrapper as OBJECT[METHOD] From 0f4701c85e78386d49b5979117ea11534344e645 Mon Sep 17 00:00:00 2001 From: Ryan Keyes Date: Thu, 18 Aug 2022 09:24:36 -0400 Subject: [PATCH 4/5] change to undefined and a clearer order --- packages/core/src/tools/instrumentMethod.ts | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/core/src/tools/instrumentMethod.ts b/packages/core/src/tools/instrumentMethod.ts index 5f9a156200..23d783b237 100644 --- a/packages/core/src/tools/instrumentMethod.ts +++ b/packages/core/src/tools/instrumentMethod.ts @@ -12,12 +12,12 @@ export function instrumentMethod | null { - if (typeof instrumentation === 'function') { - // eslint-disable-next-line @typescript-eslint/no-unsafe-return - return instrumentation.apply(this, arguments as unknown as Parameters) - } - return null + const instrumentationWrapper = function (this: OBJECT): ReturnType | undefined { + if (typeof instrumentation !== 'function') { + return undefined + } + // eslint-disable-next-line @typescript-eslint/no-unsafe-return + return instrumentation.apply(this, arguments as unknown as Parameters) } object[method] = instrumentationWrapper as OBJECT[METHOD] From e23da723c198c7ba27c6a6264d4d4e93bc991ddb Mon Sep 17 00:00:00 2001 From: Ryan Keyes Date: Mon, 22 Aug 2022 09:14:55 -0400 Subject: [PATCH 5/5] move test and rename --- .../core/src/tools/instrumentMethod.spec.ts | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/packages/core/src/tools/instrumentMethod.spec.ts b/packages/core/src/tools/instrumentMethod.spec.ts index 6af9953675..1b98b31350 100644 --- a/packages/core/src/tools/instrumentMethod.spec.ts +++ b/packages/core/src/tools/instrumentMethod.spec.ts @@ -54,17 +54,6 @@ describe('instrumentMethod', () => { expect(instrumentationSpy).toHaveBeenCalled() }) - it('handles undefined methods with third party wrappers after stop', () => { - const object: { method?: () => number } = {} - const instrumentationStub = () => 2 - const { stop } = instrumentMethod(object, 'method', () => instrumentationStub) - - thirdPartyInstrumentation(object) - - stop() - expect(object.method).not.toThrow() - }) - describe('stop()', () => { it('restores the original behavior', () => { const object = { method: () => 1 } @@ -110,6 +99,18 @@ describe('instrumentMethod', () => { expect(instrumentationSpy).not.toHaveBeenCalled() }) + + it('should not throw errors if original method was undefined', () => { + const object: { method?: () => number } = {} + const instrumentationStub = () => 2 + const { stop } = instrumentMethod(object, 'method', () => instrumentationStub) + + thirdPartyInstrumentation(object) + + stop() + + expect(object.method).not.toThrow() + }) }) })