fix: Properly remove undefined keys from apm payload (getsentry#2414)

kamilogorek · web-flow · commit b74e19925414 · 2020-02-05T14:48:49.000+01:00
diff --git 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
@@ -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
@@ -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<T>(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
@@ -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,
+        },
+      },
+    });
+  });
+});
