fix: Prevent snapshot being preprocessed twice (#1822 by @adamkovalsky)

Fixes a regression introduced in #1792 where a snapshot preprocessor gets called on an already preprocessed snapshot, resulting in potential errors.

This issue was occurring because `isMatchingSnapshotId` on the underlying model was proxied to run the preprocessor, but it was being called by `reconcile` with an already preprocessed snapshot.

This fix removes the proxying of `is` and `isMatchingSnapshotId` on snapshot processor's underlying type, while fixing the array's `areSame` utility to rely on `getReconciliationType` to get the underlying node's type. This is necessary because the node stored in the array is the underlying model, while the child-type is the `snapshotProcessor` which is needed to correctly match unprocessed snapshots to processed nodes.

The cast to `any` in `areSame` is needed because the `SnapshotProcessor` type doesn't inherit from `ComplexType`, so there's no easy way to get proper type safety on the call, but we can safely assume that the type is either a `ComplexType` or a `SnapshotProcessor` if the node is an `ObjectNode`.

packages/mobx-state-tree/__tests__/core/snapshotProcessor.test.ts

@@ -609,6 +609,10 @@ describe("snapshotProcessor", () => {
                 }),
                 {
                     preProcessor(sn: { id: string; y: number }) {
+                        if ("x" in sn) {
+                            // Ensure snapshot don't run through preprocessor twice
+                            throw new Error("sn has already been preprocessed")
+                        }
                         return { id: sn.id, x: sn.y }
                     }
                 }
@@ -656,6 +660,10 @@ describe("snapshotProcessor", () => {
                 }),
                 {
                     preProcessor(sn: { id: string; y: number }) {
+                        if ("x" in sn) {
+                            // Ensure snapshot don't run through preprocessor twice
+                            throw new Error("sn has already been preprocessed")
+                        }
                         return { id: sn.id, x: sn.y }
                     }
                 }
@@ -672,6 +680,29 @@ describe("snapshotProcessor", () => {
             expect(store.item.x).toBe(1)
         })
 
+        test("model with transformed identifier property is reconciled", () => {
+            const SP = types.snapshotProcessor(
+                types.model({
+                    id: types.identifier
+                }),
+                {
+                    preProcessor(sn: { foo: string }) {
+                        return { id: sn.foo }
+                    }
+                }
+            )
+            const Store = types.model({ item: SP }).actions((self) => ({
+                setItem(item: SnapshotIn<typeof SP>) {
+                    self.item = cast(item)
+                }
+            }))
+            const store = Store.create({ item: { foo: "1" } })
+            const oldNodeId = getNodeId(store.item)
+            store.setItem({ foo: "1" })
+            expect(getNodeId(store.item)).toBe(oldNodeId)
+            expect(store.item.id).toBe("1")
+        })
+
         test("1791 - model wrapped with maybe is reconciled", () => {
             const SP = types.snapshotProcessor(
                 types.model({

packages/mobx-state-tree/src/types/complex-types/array.ts

@@ -476,14 +476,19 @@ function areSame(oldNode: AnyNode, newValue: any) {
         return true
     }
 
+    // Non object nodes don't get reconciled
+    if (!(oldNode instanceof ObjectNode)) {
+        return false
+    }
+
+    const oldNodeType = oldNode.getReconciliationType()
     // new value is a snapshot with the correct identifier
     return (
-        oldNode instanceof ObjectNode &&
         oldNode.identifier !== null &&
         oldNode.identifierAttribute &&
         isPlainObject(newValue) &&
-        oldNode.type.is(newValue) &&
-        oldNode.type.isMatchingSnapshotId(oldNode, newValue)
+        oldNodeType.is(newValue) &&
+        (oldNodeType as any).isMatchingSnapshotId(oldNode, newValue)
     )
 }
 

packages/mobx-state-tree/src/types/utility-types/snapshotProcessor.ts

@@ -82,7 +82,7 @@ class SnapshotProcessor<IT extends IAnyType, CustomC, CustomS> extends BaseType<
 
     private _fixNode(node: this["N"]): void {
         // the node has to use these methods rather than the original type ones
-        proxyNodeTypeMethods(node.type, this, "create", "is", "isMatchingSnapshotId")
+        proxyNodeTypeMethods(node.type, this, "create")
 
         const oldGetSnapshot = node.getSnapshot
         node.getSnapshot = () => {
@@ -169,11 +169,7 @@ class SnapshotProcessor<IT extends IAnyType, CustomC, CustomS> extends BaseType<
             return false
         }
         const processedSn = this.preProcessSnapshot(snapshot)
-        return ComplexType.prototype.isMatchingSnapshotId.call(
-            this._subtype,
-            current as any,
-            processedSn
-        )
+        return this._subtype.isMatchingSnapshotId(current as any, processedSn)
     }
 }