From e19da53b686fe8e83c9f31218eb0db398cf7e9e0 Mon Sep 17 00:00:00 2001 From: Artem Zakharchenko Date: Sun, 30 Aug 2026 13:41:30 +0200 Subject: [PATCH] fix: prevent deep cloning relations on apply --- .gitignore | 3 +- src/collection.ts | 56 ++++++--------- tests/relations/many-to-many.test.ts | 104 +++++++++++++++++++++++++++ 3 files changed, 129 insertions(+), 34 deletions(-) diff --git a/.gitignore b/.gitignore index 6ad7ad0..acafbe4 100644 --- a/.gitignore +++ b/.gitignore @@ -4,4 +4,5 @@ build lib dist test-results -./tests/.tmp/** \ No newline at end of file +./tests/.tmp/** +*.tsbuildinfo diff --git a/src/collection.ts b/src/collection.ts index 83ed789..e3c2b16 100644 --- a/src/collection.ts +++ b/src/collection.ts @@ -806,17 +806,14 @@ export class Collection { * @note Collapse per-index patches under a relation path into a single * relation-level event. `draft.posts.push(x)` emits `{path: ['posts', N]}`, * but relation handlers expect `{path: ['posts'], nextValue: }`. + * Whole-value patches (`draft.posts = [x]`) are grouped the same way. */ const relationPaths: Array> = [] for (const serializedPath of prevRecord[kRelationMap].keys()) { relationPaths.push(serializedPath.split('.')) } - type RelationPatchGroup = { - relationPath: Array - patchIndices: Array - } - const relationGroups = new Map() + const updatedRelationPaths = new Map>() const passthroughIndices: Array = [] for (let i = 0; i < patches.length; i++) { @@ -826,10 +823,13 @@ export class Collection { } const matchingRelationPath = relationPaths.find((relationPath) => { - if (patch.path.length !== relationPath.length + 1) { + if (!relationPath.every((key, index) => key === patch.path[index])) { return false } - if (!relationPath.every((key, index) => key === patch.path[index])) { + if (patch.path.length === relationPath.length) { + return true + } + if (patch.path.length !== relationPath.length + 1) { return false } const nextSegment = patch.path[relationPath.length] @@ -837,13 +837,10 @@ export class Collection { }) if (matchingRelationPath) { - const groupKey = matchingRelationPath.join('.') - const group = relationGroups.get(groupKey) ?? { - relationPath: matchingRelationPath, - patchIndices: [], - } - group.patchIndices.push(i) - relationGroups.set(groupKey, group) + updatedRelationPaths.set( + matchingRelationPath.join('.'), + matchingRelationPath, + ) } else { passthroughIndices.push(i) } @@ -878,33 +875,26 @@ export class Collection { } } - for (const group of relationGroups.values()) { + for (const relationPath of updatedRelationPaths.values()) { const updateEvent = new TypedEvent('update', { data: { prevRecord: frozenPrevRecord, nextRecord: maybeNextRecord, - path: group.relationPath, - prevValue: get(prevRecord, group.relationPath), - nextValue: get(maybeNextRecord, group.relationPath), + path: relationPath, + prevValue: get(prevRecord, relationPath), + nextValue: get(maybeNextRecord, relationPath), }, }) + /** + * @note Relation updates are always prevented by the relation handler + * and are never undone. Relational values are resolved via getters + * re-defined on the final record below, so the drafted value is discarded + * regardless. Undoing them would make `apply` deep-clone the live foreign + * records from the inverse patches, recursing infinitely through + * their relational getters if those records reference each other. + */ this.hooks.emit(updateEvent) - - if (updateEvent.defaultPrevented) { - for (const i of group.patchIndices) { - const inversePatch = inversePatches[i] - - invariant( - inversePatch != null, - 'Failed to update a record (%j): missing inverse patch at index %d', - prevRecord, - i, - ) - - patchesToUndo.push(inversePatch) - } - } } const nextRecord = diff --git a/tests/relations/many-to-many.test.ts b/tests/relations/many-to-many.test.ts index 4e1e300..4f70796 100644 --- a/tests/relations/many-to-many.test.ts +++ b/tests/relations/many-to-many.test.ts @@ -240,3 +240,107 @@ it('resolves a cyclic self referencing many-to-many relation', async () => { expect.soft(bob.parents).toEqual([expect.objectContaining({ id: 1 })]) expect.soft(bob.children).toEqual([expect.objectContaining({ id: 1 })]) }) + +it('removes a foreign record that has a cyclic self referencing relation', async () => { + const userSchema = z.object({ + id: z.number(), + get followedBy() { + return z.array(userSchema) + }, + }) + const articleSchema = z.object({ + id: z.number(), + get favouritedBy() { + return z.array(userSchema) + }, + }) + + const users = new Collection({ schema: userSchema }) + const articles = new Collection({ schema: articleSchema }) + + users.defineRelations(({ many }) => ({ + followedBy: many(users), + })) + articles.defineRelations(({ many }) => ({ + favouritedBy: many(users), + })) + + const john = await users.create({ id: 1, followedBy: [] }) + const jane = await users.create({ id: 2, followedBy: [] }) + + await users.update(john, { + data(user) { + user.followedBy.push(jane) + }, + }) + await users.update(jane, { + data(user) { + user.followedBy.push(john) + }, + }) + + const article = await articles.create({ id: 1, favouritedBy: [jane] }) + + await articles.update(article, { + data(article) { + article.favouritedBy.splice(0, 1) + }, + }) + + expect.soft(article.favouritedBy).toEqual([]) + expect.soft(john.followedBy).toEqual([expect.objectContaining({ id: 2 })]) + expect.soft(jane.followedBy).toEqual([expect.objectContaining({ id: 1 })]) +}) + +it('replaces foreign records that have a cyclic self referencing relation', async () => { + const userSchema = z.object({ + id: z.number(), + get followedBy() { + return z.array(userSchema) + }, + }) + const articleSchema = z.object({ + id: z.number(), + get favouritedBy() { + return z.array(userSchema) + }, + }) + + const users = new Collection({ schema: userSchema }) + const articles = new Collection({ schema: articleSchema }) + + users.defineRelations(({ many }) => ({ + followedBy: many(users), + })) + articles.defineRelations(({ many }) => ({ + favouritedBy: many(users), + })) + + const john = await users.create({ id: 1, followedBy: [] }) + const jane = await users.create({ id: 2, followedBy: [] }) + + await users.update(john, { + data(user) { + user.followedBy.push(jane) + }, + }) + await users.update(jane, { + data(user) { + user.followedBy.push(john) + }, + }) + + const article = await articles.create({ id: 1, favouritedBy: [jane] }) + + await articles.update(article, { + data(article) { + article.favouritedBy = [john] + }, + }) + + expect + .soft(article.favouritedBy) + .toEqual([expect.objectContaining({ id: 1 })]) + expect.soft(john.followedBy).toEqual([expect.objectContaining({ id: 2 })]) + expect.soft(jane.followedBy).toEqual([expect.objectContaining({ id: 1 })]) +})