Skip to content

Commit

Permalink
fix(utils): Ensure dropUndefinedKeys() does not break class instanc…
Browse files Browse the repository at this point in the history
…es (#10245)

The old implementation of `dropUndefinedKeys()` could break things,
because it used `isPlainObject()` which actually does not check if
something is a POJO, but just any object. So any class instance found
somewhere would be broken by this.

This PR fixes this to check for POJOs better, and leaves class instances
alone.

Supersedes
https://github.com/getsentry/sentry-javascript/pull/10242/files
  • Loading branch information
mydea committed Jan 18, 2024
1 parent a380b93 commit a9e52e7
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 2 deletions.
2 changes: 1 addition & 1 deletion packages/utils/src/is.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export function isPrimitive(wat: unknown): wat is Primitive {
}

/**
* Checks whether given value's type is an object literal
* Checks whether given value's type is an object literal, or a class instance.
* {@link isPlainObject}.
*
* @param wat A value to be checked.
Expand Down
15 changes: 14 additions & 1 deletion packages/utils/src/object.ts
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ export function dropUndefinedKeys<T>(inputValue: T): T {
}

function _dropUndefinedKeys<T>(inputValue: T, memoizationMap: Map<unknown, unknown>): T {
if (isPlainObject(inputValue)) {
if (isPojo(inputValue)) {
// If this node has already been visited due to a circular reference, return the object it was mapped to in the new object
const memoVal = memoizationMap.get(inputValue);
if (memoVal !== undefined) {
Expand Down Expand Up @@ -263,6 +263,19 @@ function _dropUndefinedKeys<T>(inputValue: T, memoizationMap: Map<unknown, unkno
return inputValue;
}

function isPojo(input: unknown): input is Record<string, unknown> {
if (!isPlainObject(input)) {
return false;
}

try {
const name = (Object.getPrototypeOf(input) as { constructor: { name: string } }).constructor.name;
return !name || name === 'Object';
} catch {
return true;
}
}

/**
* Ensure that something is an object.
*
Expand Down
25 changes: 25 additions & 0 deletions packages/utils/test/is.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
isErrorEvent,
isInstanceOf,
isNaN,
isPlainObject,
isPrimitive,
isThenable,
isVueViewModel,
Expand Down Expand Up @@ -144,3 +145,27 @@ describe('isVueViewModel()', () => {
expect(isVueViewModel({ foo: true })).toEqual(false);
});
});

describe('isPlainObject', () => {
class MyClass {
public foo: string = 'bar';
}

it.each([
[{}, true],
[true, false],
[false, false],
[undefined, false],
[null, false],
['', false],
[1, false],
[0, false],
[{ aha: 'yes' }, true],
[new Object({ aha: 'yes' }), true],
[new String('aa'), false],
[new MyClass(), true],
[{ ...new MyClass() }, true],
])('%s is %s', (value, expected) => {
expect(isPlainObject(value)).toBe(expected);
});
});
23 changes: 23 additions & 0 deletions packages/utils/test/object.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,29 @@ describe('dropUndefinedKeys()', () => {
});
});

describe('class instances', () => {
class MyClass {
public a = 'foo';
public b = undefined;
}

test('ignores class instance', () => {
const instance = new MyClass();
const result = dropUndefinedKeys(instance);
expect(result).toEqual({ a: 'foo', b: undefined });
expect(result).toBeInstanceOf(MyClass);
expect(Object.prototype.hasOwnProperty.call(result, 'b')).toBe(true);
});

test('ignores nested instances', () => {
const instance = new MyClass();
const result = dropUndefinedKeys({ a: [instance] });
expect(result).toEqual({ a: [instance] });
expect(result.a[0]).toBeInstanceOf(MyClass);
expect(Object.prototype.hasOwnProperty.call(result.a[0], 'b')).toBe(true);
});
});

test('should not throw on objects with circular reference', () => {
const chicken: any = {
food: undefined,
Expand Down

0 comments on commit a9e52e7

Please sign in to comment.