Skip to content

Commit 3c4a4d0

Browse files
committed
fix(commons): prevent deepMerge from mutating source objects
deepMerge mutated source objects when merging arrays because inner objects/arrays were shared by reference rather than cloned. Changes: - handleArrayMerge: replace shallow spread with recursive clone via mergeArrayItemsByIndex into a fresh array - mergeArrayItemsByIndex: clone plain objects via mergeRecursive into fresh {}, clone arrays into fresh [], and merge array-to-array by index instead of replacing - Add source immutability unit tests - Add property-based tests using fast-check Closes #5196
1 parent 6b1a811 commit 3c4a4d0

5 files changed

Lines changed: 250 additions & 5 deletions

File tree

package-lock.json

Lines changed: 42 additions & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/commons/package.json

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,8 @@
4444
"@aws/lambda-invoke-store": "0.2.4"
4545
},
4646
"devDependencies": {
47-
"@aws-lambda-powertools/testing-utils": "file:../testing"
47+
"@aws-lambda-powertools/testing-utils": "file:../testing",
48+
"fast-check": "^4.7.0"
4849
},
4950
"main": "./lib/cjs/index.js",
5051
"types": "./lib/cjs/index.d.ts",

packages/commons/src/deepMerge.ts

Lines changed: 31 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,35 @@ const mergeArrayItemsByIndex = (
4545
continue;
4646
}
4747

48-
// Otherwise, replace the target item with source item
48+
// Merge nested arrays by index
49+
if (Array.isArray(srcItem) && Array.isArray(tgtItem)) {
50+
if (!ancestors.includes(srcItem)) {
51+
ancestors.push(srcItem);
52+
mergeArrayItemsByIndex(tgtItem, srcItem, ancestors);
53+
ancestors.pop();
54+
}
55+
continue;
56+
}
57+
58+
// Otherwise, replace the target item with a clone of the source item
4959
if (srcItem !== undefined || tgtItem === undefined) {
50-
targetArray[i] = srcItem;
60+
if (isSrcPlainObject) {
61+
const cloned: Record<string, unknown> = {};
62+
ancestors.push(srcItem);
63+
mergeRecursive(cloned, srcItem, ancestors);
64+
ancestors.pop();
65+
targetArray[i] = cloned;
66+
} else if (Array.isArray(srcItem)) {
67+
if (!ancestors.includes(srcItem)) {
68+
const cloned: unknown[] = [];
69+
ancestors.push(srcItem);
70+
mergeArrayItemsByIndex(cloned, srcItem, ancestors);
71+
ancestors.pop();
72+
targetArray[i] = cloned;
73+
}
74+
} else {
75+
targetArray[i] = srcItem;
76+
}
5177
}
5278
}
5379
};
@@ -65,7 +91,9 @@ const handleArrayMerge = (
6591
ancestors: object[]
6692
): void => {
6793
if (!Array.isArray(targetValue)) {
68-
target[key] = [...sourceArray];
94+
const freshArray: unknown[] = [];
95+
mergeArrayItemsByIndex(freshArray, sourceArray, ancestors);
96+
target[key] = freshArray;
6997
return;
7098
}
7199

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,86 @@
1+
import fc from 'fast-check';
2+
import { describe, expect, it } from 'vitest';
3+
import { deepMerge } from '../../src/deepMerge.js';
4+
5+
/**
6+
* Arbitrary that generates JSON-like plain objects (no class instances,
7+
* no circular refs) suitable for deepMerge property tests.
8+
*/
9+
const jsonObject = (): fc.Arbitrary<Record<string, unknown>> =>
10+
fc.dictionary(
11+
fc.string({ minLength: 1, maxLength: 5 }),
12+
fc.letrec((tie) => ({
13+
value: fc.oneof(
14+
{ depthSize: 'small' },
15+
fc.string(),
16+
fc.integer(),
17+
fc.boolean(),
18+
fc.constant(null),
19+
fc.array(tie('value'), { maxLength: 3, depthSize: 'small' }),
20+
fc.dictionary(fc.string({ minLength: 1, maxLength: 5 }), tie('value'), {
21+
maxKeys: 3,
22+
})
23+
),
24+
})).value,
25+
{ maxKeys: 5 }
26+
);
27+
28+
describe('deepMerge property tests', () => {
29+
it('never mutates source objects', () => {
30+
fc.assert(
31+
fc.property(
32+
fc.array(jsonObject(), { minLength: 1, maxLength: 4 }),
33+
(sources) => {
34+
const snapshots = sources.map((s) => structuredClone(s));
35+
36+
deepMerge({}, ...sources);
37+
38+
for (let i = 0; i < sources.length; i++) {
39+
expect(sources[i]).toEqual(snapshots[i]);
40+
}
41+
}
42+
),
43+
{ numRuns: 200 }
44+
);
45+
});
46+
47+
it('returns the target reference', () => {
48+
fc.assert(
49+
fc.property(jsonObject(), jsonObject(), (target, source) => {
50+
const result = deepMerge(target, source);
51+
expect(result).toBe(target);
52+
}),
53+
{ numRuns: 200 }
54+
);
55+
});
56+
57+
it('result contains all top-level keys from all sources', () => {
58+
fc.assert(
59+
fc.property(
60+
fc.array(jsonObject(), { minLength: 1, maxLength: 4 }),
61+
(sources) => {
62+
const result = deepMerge({}, ...sources);
63+
const resultKeys = new Set(Object.keys(result));
64+
65+
for (const source of sources) {
66+
for (const key of Object.keys(source)) {
67+
expect(resultKeys.has(key)).toBe(true);
68+
}
69+
}
70+
}
71+
),
72+
{ numRuns: 200 }
73+
);
74+
});
75+
76+
it('merging a source into empty object then merging same source again is idempotent', () => {
77+
fc.assert(
78+
fc.property(jsonObject(), (source) => {
79+
const first = deepMerge({}, source);
80+
const second = deepMerge(structuredClone(first), source);
81+
expect(second).toEqual(first);
82+
}),
83+
{ numRuns: 200 }
84+
);
85+
});
86+
});

packages/commons/tests/unit/deepMerge.test.ts

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,6 +332,26 @@ describe('Function: deepMerge', () => {
332332
expect(result).toHaveProperty('arr[1]', 2);
333333
});
334334

335+
it('skips circular arrays nested inside arrays', () => {
336+
// Prepare - outer array contains itself as an element
337+
const outer: unknown[] = [1, 2];
338+
outer.push(outer); // outer[2] = outer (circular)
339+
const target = {
340+
arr: [
341+
[10, 20],
342+
[30, 40],
343+
[50, 60],
344+
],
345+
};
346+
const source = { arr: outer };
347+
348+
// Act
349+
const result = deepMerge(target, source);
350+
351+
// Assess - index 0 and 1 are primitives replacing arrays, index 2 is circular so skipped
352+
expect(result).toEqual({ arr: [1, 2, [50, 60]] });
353+
});
354+
335355
it('skips array that references an ancestor array', () => {
336356
// Prepare
337357
const arr: unknown[] = [];
@@ -556,6 +576,75 @@ describe('Function: deepMerge', () => {
556576
});
557577
});
558578

579+
describe('Source immutability', () => {
580+
it('does not mutate sources when merging arrays of objects across multiple sources', () => {
581+
// Prepare
582+
const source1 = { a: [{ a: 1 }] };
583+
const source2 = { a: [{ b: 2 }] };
584+
585+
// Act
586+
const result = deepMerge({}, source1, source2);
587+
588+
// Assess
589+
expect(result).toEqual({ a: [{ a: 1, b: 2 }] });
590+
expect(source1).toEqual({ a: [{ a: 1 }] });
591+
expect(source2).toEqual({ a: [{ b: 2 }] });
592+
});
593+
594+
it('does not mutate sources when merging nested arrays across multiple sources', () => {
595+
// Prepare
596+
const src1 = { a: [[1, 2, 3]] };
597+
const src2 = { a: [[3, 4]] };
598+
599+
// Act
600+
const result = deepMerge({}, src1, src2);
601+
602+
// Assess
603+
expect(result).toEqual({ a: [[3, 4, 3]] });
604+
expect(src1).toEqual({ a: [[1, 2, 3]] });
605+
expect(src2).toEqual({ a: [[3, 4]] });
606+
});
607+
608+
it('does not mutate a single source with array of objects merged into empty target', () => {
609+
// Prepare
610+
const source = { a: [{ x: 1 }, { y: 2 }] };
611+
612+
// Act
613+
const result = deepMerge({}, source);
614+
result.a[0].x = 999;
615+
616+
// Assess
617+
expect(source).toEqual({ a: [{ x: 1 }, { y: 2 }] });
618+
});
619+
620+
it('does not mutate sources with plain objects (non-array path)', () => {
621+
// Prepare
622+
const source1 = { nested: { a: 1 } };
623+
const source2 = { nested: { b: 2 } };
624+
625+
// Act
626+
deepMerge({}, source1, source2);
627+
628+
// Assess
629+
expect(source1).toEqual({ nested: { a: 1 } });
630+
expect(source2).toEqual({ nested: { b: 2 } });
631+
});
632+
633+
it('does not mutate sources with mixed arrays and nested objects', () => {
634+
// Prepare
635+
const source1 = { data: [{ items: [{ id: 1 }] }] };
636+
const source2 = { data: [{ items: [{ name: 'a' }] }] };
637+
638+
// Act
639+
const result = deepMerge({}, source1, source2);
640+
641+
// Assess
642+
expect(result).toEqual({ data: [{ items: [{ id: 1, name: 'a' }] }] });
643+
expect(source1).toEqual({ data: [{ items: [{ id: 1 }] }] });
644+
expect(source2).toEqual({ data: [{ items: [{ name: 'a' }] }] });
645+
});
646+
});
647+
559648
describe('Edge cases', () => {
560649
it('handles Date objects (treats as value, not merged)', () => {
561650
// Prepare

0 commit comments

Comments
 (0)