Prevent mutable structures when merging Immutable objects using merge…

…Deep (#1840)

BREAKING CHANGE: `mergeDeep()` will no longer merge lists of tuples into maps

__tests__/issues.ts

@@ -1,4 +1,13 @@
-import { List, OrderedMap, OrderedSet, Record, Seq, Set } from 'immutable';
+import {
+  fromJS,
+  List,
+  Map,
+  OrderedMap,
+  OrderedSet,
+  Record,
+  Seq,
+  Set,
+} from 'immutable';
 
 describe('Issue #1175', () => {
   it('invalid hashCode() response should not infinitly recurse', () => {
@@ -121,3 +130,51 @@ describe('Issue #1785', () => {
 
   expect(emptyRecord.merge({ id: 1 })).toBe(emptyRecord);
 });
+
+describe('Issue #1475', () => {
+  it('complex case should return first value on mergeDeep when types are incompatible', () => {
+    const a = fromJS({
+      ch: [
+        {
+          code: 8,
+        },
+      ],
+    }) as Map<unknown, unknown>;
+    const b = fromJS({
+      ch: {
+        code: 8,
+      },
+    });
+    expect(a.mergeDeep(b).equals(b)).toBe(true);
+  });
+
+  it('simple case should return first value on mergeDeep when types are incompatible', () => {
+    const a = fromJS({
+      ch: [],
+    }) as Map<unknown, unknown>;
+    const b = fromJS({
+      ch: { code: 8 },
+    });
+    expect(a.merge(b).equals(b)).toBe(true);
+  });
+});
+
+describe('Issue #1719', () => {
+  it('mergeDeep() should overwrite when types conflict', () => {
+    const objWithObj = fromJS({
+      items: {
+        '1': {
+          id: '1',
+        },
+      },
+    }) as Map<unknown, unknown>;
+    const objWithArray = fromJS({
+      items: [
+        {
+          id: '1',
+        },
+      ],
+    });
+    expect(objWithObj.mergeDeep(objWithArray).equals(objWithArray)).toBe(true);
+  });
+});

__tests__/merge.ts

@@ -210,73 +210,124 @@ describe('merge', () => {
     expect(merge(a, [], [])).toBe(a);
   });
 
-  it('mergeDeep with tuple Symbol keys', () => {
-    const a = Symbol('a');
-    const b = Symbol('b');
-    const c = Symbol('c');
-    const d = Symbol('d');
-    const e = Symbol('e');
-    const f = Symbol('f');
-    const g = Symbol('g');
-
-    // Note the use of nested Map constructors, Map() does not do a deep conversion!
-    const m1 = Map([
-      [
-        a,
-        Map([
-          [
-            b,
-            Map([
-              [c, 1],
-              [d, 2],
-            ]),
-          ],
-        ]),
-      ],
-    ]);
-
-    // mergeDeep can be directly given a nested set of `Iterable<[K, V]>`
-    const merged = m1.mergeDeep([
-      // @ts-ignore (Type definition limitation)
-      [
-        a,
-        [
-          [
-            b,
-            [
-              [c, 10],
-              [e, 20],
-              [f, 30],
-              [g, 40],
-            ],
-          ],
-        ],
-      ],
-    ]);
-
-    expect(merged).toEqual(
-      Map([
-        [
-          a,
-          Map([
-            [
-              b,
-              Map([
-                [c, 10],
-                [d, 2],
-                [e, 20],
-                [f, 30],
-                [g, 40],
-              ]),
-            ],
-          ]),
-        ],
-      ])
-    );
-  });
-
   it('merges records with a size property set to 0', () => {
     const Sizable = Record({ size: 0 });
     expect(Sizable().merge({ size: 123 }).size).toBe(123);
   });
+
+  it('mergeDeep merges partial conflicts', () => {
+    const a = fromJS({
+      ch: [
+        {
+          code: 8,
+        },
+      ],
+      banana: 'good',
+    }) as Map<unknown, unknown>;
+    const b = fromJS({
+      ch: {
+        code: 8,
+      },
+      apple: 'anti-doctor',
+    });
+    expect(
+      a.mergeDeep(b).equals(
+        fromJS({
+          ch: {
+            code: 8,
+          },
+          apple: 'anti-doctor',
+          banana: 'good',
+        })
+      )
+    ).toBe(true);
+  });
+
+  const map = { type: 'Map', value: Map({ b: 5, c: 9 }) };
+  const object = { type: 'object', value: { b: 7, d: 12 } };
+  const RecordFactory = Record({ a: 1, b: 2 });
+  const record = { type: 'Record', value: RecordFactory({ b: 3 }) };
+  const list = { type: 'List', value: List(['5']) };
+  const array = { type: 'array', value: ['9'] };
+  const set = { type: 'Set', value: Set('3') };
+
+  const incompatibleTypes = [
+    [map, list],
+    [map, array],
+    [map, set],
+    [object, list],
+    [object, array],
+    [object, set],
+    [record, list],
+    [record, array],
+    [record, set],
+    [list, set],
+  ];
+
+  for (const [
+    { type: type1, value: value1 },
+    { type: type2, value: value2 },
+  ] of incompatibleTypes) {
+    it(`mergeDeep and Map#mergeDeep replaces ${type1} and ${type2} with each other`, () => {
+      const aObject = { a: value1 };
+      const bObject = { a: value2 };
+      expect(mergeDeep(aObject, bObject)).toEqual(bObject);
+      expect(mergeDeep(bObject, aObject)).toEqual(aObject);
+
+      const aMap = Map({ a: value1 }) as Map<unknown, unknown>;
+      const bMap = Map({ a: value2 }) as Map<unknown, unknown>;
+      expect(aMap.mergeDeep(bMap).equals(bMap)).toBe(true);
+      expect(bMap.mergeDeep(aMap).equals(aMap)).toBe(true);
+    });
+  }
+
+  const compatibleTypesAndResult = [
+    [map, object, Map({ b: 7, c: 9, d: 12 })],
+    [map, record, Map({ a: 1, b: 3, c: 9 })],
+    [object, map, { b: 5, c: 9, d: 12 }],
+    [object, record, { a: 1, b: 3, d: 12 }],
+    [record, map, RecordFactory({ b: 5 })],
+    [record, object, RecordFactory({ b: 7 })],
+    [list, array, List(['5', '9'])],
+    [array, list, ['9', '5']],
+    [map, { type: 'Map', value: Map({ b: 7 }) }, Map({ b: 7, c: 9 })],
+    [object, { type: 'object', value: { d: 3 } }, { b: 7, d: 3 }],
+    [
+      record,
+      { type: 'Record', value: RecordFactory({ a: 3 }) },
+      RecordFactory({ a: 3, b: 2 }),
+    ],
+    [list, { type: 'List', value: List(['12']) }, List(['5', '12'])],
+    [array, { type: 'array', value: ['3'] }, ['9', '3']],
+    [set, { type: 'Set', value: Set(['3', '5']) }, Set(['3', '5'])],
+  ] as const;
+
+  for (const [
+    { type: type1, value: value1 },
+    { type: type2, value: value2 },
+    result,
+  ] of compatibleTypesAndResult) {
+    it(`mergeDeep and Map#mergeDeep merges ${type1} and ${type2}`, () => {
+      const aObject = { a: value1 };
+      const bObject = { a: value2 };
+      expect(mergeDeep(aObject, bObject)).toEqual({ a: result });
+
+      const aMap = Map({ a: value1 }) as Map<unknown, unknown>;
+      const bMap = Map({ a: value2 });
+      expect(aMap.mergeDeep(bMap)).toEqual(Map({ a: result }));
+    });
+  }
+
+  it('Map#mergeDeep replaces nested List with Map and Map with List', () => {
+    const a = Map({ a: List([Map({ x: 1 })]) }) as Map<unknown, unknown>;
+    const b = Map({ a: Map([[0, Map({ y: 2 })]]) }) as Map<unknown, unknown>;
+    expect(a.mergeDeep(b).equals(b)).toBe(true);
+    expect(b.mergeDeep(a).equals(a)).toBe(true);
+  });
+
+  it('functional mergeDeep replaces nested array with Map', () => {
+    const a = { a: [{ x: 1 }] };
+    const b = Map({ a: Map([[0, Map({ y: 2 })]]) });
+    expect(mergeDeep(a, b)).toEqual({ a: Map([[0, Map({ y: 2 })]]) });
+  });
 });

src/functional/merge.js

@@ -1,5 +1,8 @@
 import { isImmutable } from '../predicates/isImmutable';
+import { isIndexed } from '../predicates/isIndexed';
+import { isKeyed } from '../predicates/isKeyed';
 import { IndexedCollection, KeyedCollection } from '../Collection';
+import { Seq } from '../Seq';
 import hasOwnProperty from '../utils/hasOwnProperty';
 import isDataStructure from '../utils/isDataStructure';
 import shallowCopy from '../utils/shallowCopy';
@@ -68,11 +71,29 @@ export function mergeWithSources(collection, sources, merger) {
 
 function deepMergerWith(merger) {
   function deepMerger(oldValue, newValue, key) {
-    return isDataStructure(oldValue) && isDataStructure(newValue)
+    return isDataStructure(oldValue) &&
+      isDataStructure(newValue) &&
+      areMergeable(oldValue, newValue)
       ? mergeWithSources(oldValue, [newValue], deepMerger)
       : merger
       ? merger(oldValue, newValue, key)
       : newValue;
   }
   return deepMerger;
 }
+
+/**
+ * It's unclear what the desired behavior is for merging two collections that
+ * fall into separate categories between keyed, indexed, or set-like, so we only
+ * consider them mergeable if they fall into the same category.
+ */
+function areMergeable(oldDataStructure, newDataStructure) {
+  const oldSeq = Seq(oldDataStructure);
+  const newSeq = Seq(newDataStructure);
+  // This logic assumes that a sequence can only fall into one of the three
+  // categories mentioned above (since there's no `isSetLike()` method).
+  return (
+    isIndexed(oldSeq) === isIndexed(newSeq) &&
+    isKeyed(oldSeq) === isKeyed(newSeq)
+  );
+}

type-definitions/immutable.d.ts

@@ -1015,12 +1015,18 @@ declare namespace Immutable {
     ): this;
 
     /**
-     * Like `merge()`, but when two Collections conflict, it merges them as well,
-     * recursing deeply through the nested data.
-     *
-     * Note: Values provided to `merge` are shallowly converted before being
-     * merged. No nested values are altered unless they will also be merged at
-     * a deeper level.
+     * Like `merge()`, but when two compatible collections are encountered with
+     * the same key, it merges them as well, recursing deeply through the nested
+     * data. Two collections are considered to be compatible (and thus will be
+     * merged together) if they both fall into one of three categories: keyed
+     * (e.g., `Map`s, `Record`s, and objects), indexed (e.g., `List`s and
+     * arrays), or set-like (e.g., `Set`s). If they fall into separate
+     * categories, `mergeDeep` will replace the existing collection with the
+     * collection being merged in. This behavior can be customized by using
+     * `mergeDeepWith()`.
+     *
+     * Note: Indexed and set-like collections are merged using
+     * `concat()`/`union()` and therefore do not recurse.
      *
      * <!-- runkit:activate -->
      * ```js
@@ -1042,8 +1048,11 @@ declare namespace Immutable {
     ): this;
 
     /**
-     * Like `mergeDeep()`, but when two non-Collections conflict, it uses the
-     * `merger` function to determine the resulting value.
+     * Like `mergeDeep()`, but when two non-collections or incompatible
+     * collections are encountered at the same key, it uses the `merger`
+     * function to determine the resulting value. Collections are considered
+     * incompatible if they fall into separate categories between keyed,
+     * indexed, and set-like.
      *
      * <!-- runkit:activate -->
      * ```js
@@ -5534,8 +5543,18 @@ declare namespace Immutable {
   ): C;
 
   /**
-   * Returns a copy of the collection with the remaining collections merged in
-   * deeply (recursively).
+   * Like `merge()`, but when two compatible collections are encountered with
+   * the same key, it merges them as well, recursing deeply through the nested
+   * data. Two collections are considered to be compatible (and thus will be
+   * merged together) if they both fall into one of three categories: keyed
+   * (e.g., `Map`s, `Record`s, and objects), indexed (e.g., `List`s and
+   * arrays), or set-like (e.g., `Set`s). If they fall into separate
+   * categories, `mergeDeep` will replace the existing collection with the
+   * collection being merged in. This behavior can be customized by using
+   * `mergeDeepWith()`.
+   *
+   * Note: Indexed and set-like collections are merged using
+   * `concat()`/`union()` and therefore do not recurse.
    *
    * A functional alternative to `collection.mergeDeep()` which will also work
    * with plain Objects and Arrays.
@@ -5558,9 +5577,10 @@ declare namespace Immutable {
   ): C;
 
   /**
-   * Returns a copy of the collection with the remaining collections merged in
-   * deeply (recursively), calling the `merger` function whenever an existing
-   * value is encountered.
+   * Like `mergeDeep()`, but when two non-collections or incompatible
+   * collections are encountered at the same key, it uses the `merger` function
+   * to determine the resulting value. Collections are considered incompatible
+   * if they fall into separate categories between keyed, indexed, and set-like.
    *
    * A functional alternative to `collection.mergeDeepWith()` which will also
    * work with plain Objects and Arrays.