From 02c335836aac5c08c450f0ccd7ad86b59bc40d87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kamil=20Og=C3=B3rek?= Date: Wed, 5 Feb 2020 12:18:32 +0100 Subject: [PATCH] fix: Properly remove undefined keys from apm payload --- packages/apm/src/span.ts | 57 +++++++++++--------------- packages/apm/test/span.test.ts | 32 +++++++++++++++ packages/utils/src/object.ts | 25 ++++++++++- packages/utils/test/object.test.ts | 66 +++++++++++++++++++++++++++++- 4 files changed, 145 insertions(+), 35 deletions(-) diff --git a/packages/apm/src/span.ts b/packages/apm/src/span.ts index d1a6c1203aec..2fca37e62188 100644 --- a/packages/apm/src/span.ts +++ b/packages/apm/src/span.ts @@ -2,7 +2,7 @@ import { getCurrentHub, Hub } from '@sentry/hub'; import { Span as SpanInterface, SpanContext, SpanStatus } from '@sentry/types'; -import { isInstanceOf, logger, timestampWithMs, uuid4 } from '@sentry/utils'; +import { dropUndefinedKeys, isInstanceOf, logger, timestampWithMs, uuid4 } from '@sentry/utils'; // TODO: Should this be exported? export const TRACEPARENT_REGEXP = new RegExp( @@ -314,43 +314,34 @@ export class Span implements SpanInterface, SpanContext { * @inheritDoc */ public getTraceContext(): object { - // JSON.parse + JSON.stringify to remove undefined values - // tslint:disable-next-line: no-unsafe-any - return JSON.parse( - JSON.stringify({ - data: this.data, - description: this.description, - op: this.op, - parent_span_id: this._parentSpanId, - span_id: this._spanId, - // Undefined status will be dropped by `JSON.stringify` anyway so it's safe to read it directly like that - status: this.tags.status, - tags: this.tags, - trace_id: this._traceId, - }), - ); + return dropUndefinedKeys({ + data: this.data, + description: this.description, + op: this.op, + parent_span_id: this._parentSpanId, + span_id: this._spanId, + status: this.tags.status, + tags: this.tags, + trace_id: this._traceId, + }); } /** * @inheritDoc */ public toJSON(): object { - // JSON.parse + JSON.stringify to remove undefined values - // tslint:disable-next-line: no-unsafe-any - return JSON.parse( - JSON.stringify({ - data: this.data, - description: this.description, - op: this.op, - parent_span_id: this._parentSpanId, - sampled: this.sampled, - span_id: this._spanId, - start_timestamp: this.startTimestamp, - tags: this.tags, - timestamp: this.timestamp, - trace_id: this._traceId, - transaction: this.transaction, - }), - ); + return dropUndefinedKeys({ + data: this.data, + description: this.description, + op: this.op, + parent_span_id: this._parentSpanId, + sampled: this.sampled, + span_id: this._spanId, + start_timestamp: this.startTimestamp, + tags: this.tags, + timestamp: this.timestamp, + trace_id: this._traceId, + transaction: this.transaction, + }); } } diff --git a/packages/apm/test/span.test.ts b/packages/apm/test/span.test.ts index eea2cb47b7cb..2a36f1ca97ab 100644 --- a/packages/apm/test/span.test.ts +++ b/packages/apm/test/span.test.ts @@ -154,6 +154,27 @@ describe('Span', () => { expect(serialized).toHaveProperty('span_id', 'd'); expect(serialized).toHaveProperty('trace_id', 'c'); }); + + test('should drop all `undefined` values', () => { + const spanA = new Span({ traceId: 'a', spanId: 'b' }) as any; + const spanB = new Span({ + parentSpanId: spanA._spanId, + sampled: false, + spanId: 'd', + traceId: 'c', + }); + const serialized = spanB.toJSON(); + expect(serialized).toHaveProperty('start_timestamp'); + delete (serialized as { start_timestamp: number }).start_timestamp; + expect(serialized).toStrictEqual({ + data: {}, + parent_span_id: 'b', + sampled: false, + span_id: 'd', + tags: {}, + trace_id: 'c', + }); + }); }); describe('finish', () => { @@ -231,5 +252,16 @@ describe('Span', () => { const context = span.getTraceContext(); expect((context as any).status).toBe('resource_exhausted'); }); + + test('should drop all `undefined` values', () => { + const spanB = new Span({ spanId: 'd', traceId: 'c' }); + const context = spanB.getTraceContext(); + expect(context).toStrictEqual({ + data: {}, + span_id: 'd', + tags: {}, + trace_id: 'c', + }); + }); }); }); diff --git a/packages/utils/src/object.ts b/packages/utils/src/object.ts index 4f2970ede042..1a2c1ed0fe5c 100644 --- a/packages/utils/src/object.ts +++ b/packages/utils/src/object.ts @@ -1,6 +1,6 @@ import { ExtendedError, WrappedFunction } from '@sentry/types'; -import { isElement, isError, isEvent, isInstanceOf, isPrimitive, isSyntheticEvent } from './is'; +import { isElement, isError, isEvent, isInstanceOf, isPlainObject, isPrimitive, isSyntheticEvent } from './is'; import { Memo } from './memo'; import { getFunctionName, htmlTreeAsString } from './misc'; import { truncate } from './string'; @@ -351,3 +351,26 @@ export function extractExceptionKeysForMessage(exception: any, maxLength: number return ''; } + +/** + * Given any object, return the new object with removed keys that value was `undefined`. + * Works recursively on objects and arrays. + */ +export function dropUndefinedKeys(val: T): T { + if (isPlainObject(val)) { + const obj = val as { [key: string]: any }; + const rv: { [key: string]: any } = {}; + for (const key of Object.keys(obj)) { + if (typeof obj[key] !== 'undefined') { + rv[key] = dropUndefinedKeys(obj[key]); + } + } + return rv as T; + } + + if (Array.isArray(val)) { + return val.map(dropUndefinedKeys) as any; + } + + return val; +} diff --git a/packages/utils/test/object.test.ts b/packages/utils/test/object.test.ts index ae72d65215d6..24b64d11b65c 100644 --- a/packages/utils/test/object.test.ts +++ b/packages/utils/test/object.test.ts @@ -1,4 +1,4 @@ -import { extractExceptionKeysForMessage, fill, normalize, urlEncode } from '../src/object'; +import { dropUndefinedKeys, extractExceptionKeysForMessage, fill, normalize, urlEncode } from '../src/object'; describe('fill()', () => { test('wraps a method by calling a replacement function on it', () => { @@ -551,3 +551,67 @@ describe('extractExceptionKeysForMessage()', () => { expect(extractExceptionKeysForMessage({ barbazquxfoo: '_', baz: '_', qux: '_' }, 10)).toEqual('barbazquxf...'); }); }); + +describe('dropUndefinedKeys()', () => { + test('simple case', () => { + expect( + dropUndefinedKeys({ + a: 1, + b: undefined, + c: null, + d: 'd', + }), + ).toStrictEqual({ + a: 1, + c: null, + d: 'd', + }); + }); + + test('arrays', () => { + expect( + dropUndefinedKeys({ + a: [ + 1, + undefined, + { + a: 1, + b: undefined, + }, + ], + }), + ).toStrictEqual({ + a: [ + 1, + undefined, + { + a: 1, + }, + ], + }); + }); + + test('nested objects', () => { + expect( + dropUndefinedKeys({ + a: 1, + b: { + c: 2, + d: undefined, + e: { + f: 3, + g: undefined, + }, + }, + }), + ).toStrictEqual({ + a: 1, + b: { + c: 2, + e: { + f: 3, + }, + }, + }); + }); +});