Skip to content

Commit 7a0e07a

Browse files
authored
fix(core): consider negative indexes for inserts (#555)
### Description Fix negative index handling in JSONMatch `insert` patch operations to be compliant with content-lake behavior. Previously, negative indexes in `insert` operations (before/after/replace) were not being properly converted to positive indexes before processing keyed segments, leading to incorrect insertion behavior. This change ensures that negative indexes like `[-1]` are correctly resolved to their positive equivalents before any further path resolution. **Changes introduced:** - Fixed negative index conversion in `insert` operations to happen before keyed segment lookups - Updated test expectations to reflect correct behavior (negative indexes should resolve to actual array positions, not special append/prepend behavior) - Corrected a bug in the "after" operation where negative index handling was using subtraction instead of addition ### What to review **Focus areas for review:** 1. **Core logic changes in `patchOperations.ts`**: Review the negative index handling in all three `insert` operation cases (replace, before, after) 2. **Test updates**: Verify that the updated test expectations align with how content-lake handles these operations 3. **Edge cases**: Consider scenarios with out-of-bounds negative indexes and ensure they're handled consistently **Affected functionality:** - JSONMatch `insert` patch operations with negative array indexes - Any code that relies on `insert` operations with paths like `array[-1]`, `array[-2]`, etc. ### Testing Updated existing unit tests to reflect the correct behavior, but **we should add end-to-end tests** to ensure our implementation matches content-lake's behavior exactly. The current unit tests verify the logic works as intended, but we need integration tests that compare our results with actual content-lake responses for various negative index scenarios. **Current testing:** - ✅ Updated unit tests for `before`, `after`, and `replace` operations with negative indexes - ✅ Tests cover both single index and keyed segment scenarios **Missing testing:** - ❌ E2E tests comparing our results with content-lake responses - ❌ Integration tests with complex nested negative index scenarios ### Fun gif ![Debugging arrays be like](https://media.giphy.com/media/3o7TKTDn976rzVgky4/giphy.gif)
1 parent d88dc29 commit 7a0e07a

File tree

2 files changed

+7
-4
lines changed

2 files changed

+7
-4
lines changed

packages/core/src/document/patchOperations.test.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -758,10 +758,10 @@ describe('insert', () => {
758758
expect(output).toEqual({some: {array: ['a', '!', 'b', 'c']}})
759759
})
760760

761-
it('interprets a negative index for "before" as append', () => {
761+
it('interprets a negative index for "before"', () => {
762762
const input = {some: {array: ['a', 'b', 'c']}}
763763
const output = insert(input, {before: 'some.array[-1]', items: ['!']})
764-
expect(output).toEqual({some: {array: ['a', 'b', 'c', '!']}})
764+
expect(output).toEqual({some: {array: ['a', 'b', '!', 'c']}})
765765
})
766766

767767
it('inserts items after a given positive index ("after" operation)', () => {
@@ -793,10 +793,10 @@ describe('insert', () => {
793793
})
794794
})
795795

796-
it('inserts items after a negative index ("after" operation with negative index interpreted as prepend)', () => {
796+
it('inserts items after a negative index ("after" operation with negative index interpreted as append)', () => {
797797
const input = {some: {array: ['a', 'b', 'c']}}
798798
const output = insert(input, {after: 'some.array[-1]', items: ['!']})
799-
expect(output).toEqual({some: {array: ['!', 'a', 'b', 'c']}})
799+
expect(output).toEqual({some: {array: ['a', 'b', 'c', '!']}})
800800
})
801801

802802
it('replaces a single matched element ("replace" operation, single match)', () => {

packages/core/src/document/patchOperations.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -621,6 +621,7 @@ export function insert(input: unknown, {items, ...insertPatch}: InsertPatch): un
621621
let index
622622

623623
if (typeof segment === 'number') index = segment
624+
if (typeof index === 'number' && index < 0) index = arr.length + index
624625
if (isKeySegment(segment)) index = getIndexForKey(arr, segment._key)
625626
if (typeof index !== 'number') continue
626627
if (index < 0) index = arr.length + index
@@ -654,6 +655,7 @@ export function insert(input: unknown, {items, ...insertPatch}: InsertPatch): un
654655
let index
655656

656657
if (typeof segment === 'number') index = segment
658+
if (typeof index === 'number' && index < 0) index = arr.length + index
657659
if (isKeySegment(segment)) index = getIndexForKey(arr, segment._key)
658660
if (typeof index !== 'number') continue
659661
if (index < 0) index = arr.length - index
@@ -678,6 +680,7 @@ export function insert(input: unknown, {items, ...insertPatch}: InsertPatch): un
678680
let index
679681

680682
if (typeof segment === 'number') index = segment
683+
if (typeof index === 'number' && index < 0) index = arr.length + index
681684
if (isKeySegment(segment)) index = getIndexForKey(arr, segment._key)
682685
if (typeof index !== 'number') continue
683686
if (index > position) position = index

0 commit comments

Comments
 (0)