From cad3cb37ea6088b6c643e223ec821d3bd3a7be4e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aaron=20D=C3=B6ppner?= <58708656+aarondpn@users.noreply.github.com> Date: Mon, 27 Jul 2026 09:53:02 +0200 Subject: [PATCH] Fix revertChangeset for index-based arrays revertArrayChange processed $index subchanges in their original ascending order. Reverting an ADD splices the element out, so an earlier splice shifts the remaining elements and later indices land on the wrong slot: revertChangeset(['a','b','c'], diff(['a'], ['a','b','c'])) returned ['a','c']. ADDs are now undone from the highest index down, mirroring the ordering applyArrayChange already uses for REMOVE operations. Two further defects in the same path surfaced while verifying the fix: - revertLeafChange called addKeyValue without embeddedKey, so reverting a REMOVE appended the value instead of splicing it back at its index. This affected every type change, since treatTypeChangeAsReplace defaults to true and encodes those as a REMOVE/ADD pair at the same index. - The leaf/branch guard tested subchange.value != null, routing null-valued subchanges down the branch path where they were silently dropped. It now keys off the presence of nested changes, matching applyArrayChange. Verified by exhaustive round-trip over all arrays of length 0-4 built from four distinct types (116281 pairs, both directions): no apply or revert failures. Existing snapshots are unchanged. --- src/jsonDiff.ts | 18 +++++-- tests/__fixtures__/jsonDiff.fixture.ts | 23 ++++++++ tests/jsonDiff.test.ts | 74 +++++++++++++++++++++++++- 3 files changed, 111 insertions(+), 4 deletions(-) diff --git a/src/jsonDiff.ts b/src/jsonDiff.ts index 20458b1..7b422c1 100644 --- a/src/jsonDiff.ts +++ b/src/jsonDiff.ts @@ -821,7 +821,7 @@ const revertLeafChange = (obj: any, change: any, embeddedKey = '$index', isPath? case Operation.UPDATE: return modifyKeyValue(obj, key, oldValue); case Operation.REMOVE: - return addKeyValue(obj, key, value); + return addKeyValue(obj, key, value, embeddedKey); } }; @@ -836,8 +836,20 @@ const revertLeafChange = (obj: any, change: any, embeddedKey = '$index', isPath? * consistency with other functions. */ const revertArrayChange = (arr: any[], change: any) => { - for (const subchange of change.changes) { - if (subchange.value != null || subchange.type === Operation.REMOVE) { + let changes = change.changes; + if (change.embeddedKey === '$index') { + changes = [...changes].sort((a, b) => { + if (a.type === Operation.ADD && b.type === Operation.ADD) { + return Number(b.key) - Number(a.key); + } + if (a.type === Operation.ADD) return -1; + if (b.type === Operation.ADD) return 1; + return Number(a.key) - Number(b.key); + }); + } + + for (const subchange of changes) { + if (!subchange.changes) { revertLeafChange(arr, subchange, change.embeddedKey, change.embeddedKeyIsPath); } else { let element; diff --git a/tests/__fixtures__/jsonDiff.fixture.ts b/tests/__fixtures__/jsonDiff.fixture.ts index 9d96f26..50c8a85 100644 --- a/tests/__fixtures__/jsonDiff.fixture.ts +++ b/tests/__fixtures__/jsonDiff.fixture.ts @@ -399,3 +399,26 @@ export const assortedDiffs: { expectedUpdate: [] }, ]; + +export const indexArrayRevertCases: { + name: string; + oldVal: unknown[]; + newVal: unknown[]; +}[] = [ + { name: 'array grown at the end', oldVal: ['a'], newVal: ['a', 'b', 'c'] }, + { name: 'array shrunk at the end', oldVal: ['a', 'b', 'c'], newVal: ['a'] }, + { name: 'array filled from empty', oldVal: [], newVal: ['a', 'b', 'c'] }, + { name: 'array emptied', oldVal: ['a', 'b', 'c'], newVal: [] }, + { name: 'replace pair at a single index', oldVal: ['a', 2], newVal: [false, 2] }, + { name: 'every element replaced by another type', oldVal: ['a', 1, 'b', 2], newVal: [1, 'a', 2, 'b'] }, + { name: 'replace pair with trailing removes', oldVal: ['a', 'b', 'c'], newVal: [true] }, + { name: 'replace pair with trailing adds', oldVal: [true], newVal: ['a', 'b', 'c'] }, + { name: 'null values added', oldVal: [], newVal: [null, 'a', null] }, + { name: 'null values removed', oldVal: [null, 'a', null], newVal: ['a'] }, + { name: 'nested arrays changed on both sides', oldVal: [['a', 'b'], ['c']], newVal: [['a'], ['c', 'd']] }, + { + name: 'objects replaced by primitives', + oldVal: [{ a: 1 }, { b: 2 }], + newVal: [{ a: 9 }, 'prim'] + } +]; diff --git a/tests/jsonDiff.test.ts b/tests/jsonDiff.test.ts index f3ccfe5..d52f188 100644 --- a/tests/jsonDiff.test.ts +++ b/tests/jsonDiff.test.ts @@ -332,11 +332,83 @@ describe('jsonDiff#revertChangeset', () => { // Reverting a REMOVE operation should restore the object const obj = {}; const changeset = [{ key: '$root', type: Operation.REMOVE, value: { x: 'y', z: 123 } }]; - + // Expected result is the original object that was removed const result = revertChangeset(obj, changeset); expect(result).toEqual({ x: 'y', z: 123 }); }); + + describe('index-based arrays', () => { + it('should revert multiple ADDs without leaving later elements behind', () => { + // Reverting an ADD splices the element out, so the ADDs have to be undone + // from the highest index down. Undoing index 1 first shifts 'c' down into + // that slot, which used to leave the array as ['a', 'c']. + const oldArr = ['a']; + const newArr = ['a', 'b', 'c']; + + const changes = diff(oldArr, newArr); + const result = revertChangeset(structuredClone(newArr), changes); + expect(result).toEqual(oldArr); + }); + + it('should revert multiple ADDs on an array nested in a property', () => { + const oldObj = { list: ['a'] }; + const newObj = { list: ['a', 'b', 'c', 'd'] }; + + const changes = diff(oldObj, newObj); + const result = revertChangeset(structuredClone(newObj), changes); + expect(result).toEqual(oldObj); + }); + + it('should restore the original position when reverting a REMOVE/ADD replace pair', () => { + // A type change is encoded as REMOVE + ADD at the same index. Reverting + // the REMOVE has to splice the value back in at that index rather than + // append it, otherwise the element ends up at the end of the array. + const oldArr = ['a', 2]; + const newArr = [false, 2]; + + const changes = diff(oldArr, newArr); + const result = revertChangeset(structuredClone(newArr), changes); + expect(result).toEqual(oldArr); + }); + + it('should revert ADDs carrying null values', () => { + // Null-valued subchanges must still be treated as leaf changes; routing + // them down the branch path silently dropped them. + const oldArr: unknown[] = []; + const newArr = [null, 'a', null]; + + const changes = diff(oldArr, newArr); + const result = revertChangeset(structuredClone(newArr), changes); + expect(result).toEqual(oldArr); + }); + + it('should revert mixed ADD and REMOVE operations in a single changeset', () => { + // Produces REMOVE@0, ADD@0 (the type change) plus ADD@2 and ADD@3. + const oldArr = ['a', 'b']; + const newArr = [1, 'b', 2, 'c']; + + const changes = diff(oldArr, newArr); + const result = revertChangeset(structuredClone(newArr), changes); + expect(result).toEqual(oldArr); + }); + + it('should revert a nested branch change alongside a replace pair', () => { + // Branch subchange at index 0, replace pair at index 1, then trailing ADDs. + const oldObj = { list: [{ a: 1 }, { b: 2 }] }; + const newObj = { list: [{ a: 9 }, 'prim', { e: 5 }, 'tail'] }; + + const changes = diff(oldObj, newObj); + const result = revertChangeset(structuredClone(newObj), changes); + expect(result).toEqual(oldObj); + }); + + it.each(fixtures.indexArrayRevertCases)('should round-trip a revert for $name', ({ oldVal, newVal }) => { + const changes = diff(oldVal, newVal); + const result = revertChangeset(structuredClone(newVal), changes); + expect(result).toEqual(oldVal); + }); + }); }); describe('jsonDiff#flatten', () => {