From 46d1ea9d5d6aad53d77fec0b4437fca9254d4db5 Mon Sep 17 00:00:00 2001 From: David Matejka Date: Tue, 25 Aug 2026 14:01:57 +0200 Subject: [PATCH] fix(bindx): stop nested collection recursing through relation cycles MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Collecting an update walks unchanged hasOne targets looking for nested changes, and collecting a create walks its temp targets the same way. Neither walk tracked where it had been, so a schema where two hasOne relations point back at each other (a page holding its published revision, that revision holding its page) recursed until the stack blew. The persist then rejected with `RangeError: Maximum call stack size exceeded` before a single mutation was built: nothing reached the server, the store stayed dirty, and no persist notification fired — the save button simply did nothing. It needs both sides of the cycle loaded in the store, so it stayed hidden until a query pulled the back edge in. Guard both entry points with the set of entities already on the collection stack. Re-entering one means an ancestor frame is already emitting its changes, so the inner edge has nothing left to contribute. Fixes #88 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01EmqgPtZvAfpvWKymfFZCEX --- .../src/persistence/MutationCollector.ts | 57 +++++-- .../relationCycleCollection.test.ts | 144 ++++++++++++++++++ 2 files changed, 190 insertions(+), 11 deletions(-) create mode 100644 tests/unit/persistence/relationCycleCollection.test.ts diff --git a/packages/bindx/src/persistence/MutationCollector.ts b/packages/bindx/src/persistence/MutationCollector.ts index 0263a10..077e888 100644 --- a/packages/bindx/src/persistence/MutationCollector.ts +++ b/packages/bindx/src/persistence/MutationCollector.ts @@ -102,6 +102,14 @@ export class MutationCollector implements MutationDataCollector { private readonly _relationFields = new Map() private readonly _nestedCreates: CollectedNestedCreate[] = [] private readonly _nestedUpdates: CollectedNestedUpdate[] = [] + /** + * Entities whose data collection is still on the stack. A schema may point two + * hasOne relations back at each other (`page.publishedRevision` ↔ `revision.page`), + * and once both sides are in the store the nested-update walk would recurse until + * the stack blows. Re-entering an entity means an ancestor frame already emits its + * changes, so the inner edge contributes nothing (see issue #88). + */ + private readonly _collecting = new Set() constructor( private readonly store: SnapshotStore, @@ -133,6 +141,7 @@ export class MutationCollector implements MutationDataCollector { this._relationFields.clear() this._nestedCreates.length = 0 this._nestedUpdates.length = 0 + this._collecting.clear() } setExcludedEntityKeys(keys: ReadonlySet): void { @@ -313,21 +322,30 @@ export class MutationCollector implements MutationDataCollector { throw new Error(`Entity type '${entityType}' not found in schema`) } - const mutation: Record = {} + const key = this.entityKey(entityType, entityId) + if (this._collecting.has(key)) { + return null + } + this._collecting.add(key) + try { + const mutation: Record = {} - // Collect scalar field changes - this.collectScalarChanges(entityType, snapshot, mutation) + // Collect scalar field changes + this.collectScalarChanges(entityType, snapshot, mutation) - // Materialize placeholder-backed creating-state hasOne relations before - // collecting, so the collection phase can remain a pure read over the - // store. Embedded data is not materialized here — for existing - // (server-side) parents, hasMany state is already authoritative. - this.materializeForUpdate(entityType, entityId) + // Materialize placeholder-backed creating-state hasOne relations before + // collecting, so the collection phase can remain a pure read over the + // store. Embedded data is not materialized here — for existing + // (server-side) parents, hasMany state is already authoritative. + this.materializeForUpdate(entityType, entityId) - // Collect relation changes - this.collectRelationChanges(entityType, entityId, mutation) + // Collect relation changes + this.collectRelationChanges(entityType, entityId, mutation) - return Object.keys(mutation).length > 0 ? mutation : null + return Object.keys(mutation).length > 0 ? mutation : null + } finally { + this._collecting.delete(key) + } } /** @@ -347,6 +365,23 @@ export class MutationCollector implements MutationDataCollector { throw new Error(`Entity type '${entityType}' not found in schema`) } + const key = this.entityKey(entityType, entityId) + if (this._collecting.has(key)) { + return null + } + this._collecting.add(key) + try { + return this.buildCreateData(entityType, entityId, snapshot) + } finally { + this._collecting.delete(key) + } + } + + private buildCreateData( + entityType: string, + entityId: string, + snapshot: EntitySnapshot, + ): Record | null { const data = snapshot.data as Record const createData: Record = {} diff --git a/tests/unit/persistence/relationCycleCollection.test.ts b/tests/unit/persistence/relationCycleCollection.test.ts new file mode 100644 index 0000000..bf85eea --- /dev/null +++ b/tests/unit/persistence/relationCycleCollection.test.ts @@ -0,0 +1,144 @@ +import { beforeEach, describe, expect, test } from 'bun:test' +import { + ActionDispatcher, + BatchPersister, + ContemberSchemaMutationAdapter, + MutationCollector, + SnapshotStore, + type BackendAdapter, + type SchemaNames, +} from '@contember/bindx' + +/** + * `Page.publishedRevision` and `Revision.page` point back at each other, which is an + * ordinary shape for a schema that keeps a pointer to the currently live version. + */ +const schema: SchemaNames = { + entities: { + Page: { + name: 'Page', + scalars: ['id', 'title'], + fields: { + id: { type: 'column' }, + title: { type: 'column' }, + publishedRevision: { type: 'one', entity: 'Revision' }, + }, + }, + Revision: { + name: 'Revision', + scalars: ['id', 'name'], + fields: { + id: { type: 'column' }, + name: { type: 'column' }, + page: { type: 'one', entity: 'Page' }, + }, + }, + }, + enums: {}, +} + +interface PersistCall { + readonly entityType: string + readonly entityId: string + readonly changes: Record +} + +function createAdapter(calls: PersistCall[]): BackendAdapter { + return { + query: () => Promise.resolve([]), + persist: (entityType, entityId, changes) => { + calls.push({ entityType, entityId, changes }) + return Promise.resolve({ ok: true, data: { id: entityId } }) + }, + create: (_entityType, data) => Promise.resolve({ ok: true, data: { id: 'created-1', ...data } }), + delete: () => Promise.resolve({ ok: true }), + } +} + +function connect(store: SnapshotStore, entityType: string, entityId: string, fieldName: string, targetId: string): void { + store.getOrCreateRelation(entityType, entityId, fieldName, { + currentId: targetId, + serverId: targetId, + state: 'connected', + serverState: 'connected', + placeholderData: {}, + }) +} + +/** + * Collecting an update walks unchanged hasOne targets looking for nested changes. With a + * relation cycle in the schema that walk revisits an entity already on the stack, and + * without a guard it recurses until `RangeError: Maximum call stack size exceeded` — the + * persist then rejects, so nothing reaches the server and the store stays dirty. + */ +describe('relation cycles during collection', () => { + let store: SnapshotStore + let dispatcher: ActionDispatcher + let calls: PersistCall[] + let persister: BatchPersister + + beforeEach(() => { + store = new SnapshotStore() + dispatcher = new ActionDispatcher(store) + calls = [] + persister = new BatchPersister(createAdapter(calls), store, dispatcher, { + mutationCollector: new MutationCollector(store, new ContemberSchemaMutationAdapter(schema)), + }) + + store.setEntityData('Page', 'page-1', { id: 'page-1', title: 'Page' }, true) + store.setEntityData('Revision', 'rev-draft', { id: 'rev-draft', name: 'Draft' }, true) + store.setEntityData('Revision', 'rev-published', { id: 'rev-published', name: 'Published' }, true) + + connect(store, 'Revision', 'rev-draft', 'page', 'page-1') + connect(store, 'Page', 'page-1', 'publishedRevision', 'rev-published') + connect(store, 'Revision', 'rev-published', 'page', 'page-1') + }) + + test('an update through a hasOne cycle persists instead of overflowing the stack', async () => { + store.setFieldValue('Revision', 'rev-draft', ['name'], 'Renamed draft') + + const result = await persister.persistAll() + + expect(result.success).toBe(true) + expect(calls).toHaveLength(1) + expect(calls[0]?.entityType).toBe('Revision') + expect(calls[0]?.entityId).toBe('rev-draft') + expect(calls[0]?.changes).toEqual({ name: 'Renamed draft' }) + }) + + test('a real change on the far side of the cycle still travels with its parent', async () => { + store.setFieldValue('Revision', 'rev-draft', ['name'], 'Renamed draft') + store.setFieldValue('Page', 'page-1', ['title'], 'Renamed page') + + const result = await persister.persistAll() + + expect(result.success).toBe(true) + const revisionCall = calls.find(call => call.entityId === 'rev-draft') + const pageCall = calls.find(call => call.entityId === 'page-1') + expect(revisionCall?.changes).toEqual({ name: 'Renamed draft' }) + expect(pageCall?.changes).toEqual({ title: 'Renamed page' }) + }) + + test('a create whose nested target points back at it does not recurse forever', async () => { + const pageId = store.createEntity('Page', { title: 'New page' }) + const revisionId = store.createEntity('Revision', { name: 'New revision' }) + store.getOrCreateRelation('Page', pageId, 'publishedRevision', { + currentId: revisionId, + serverId: null, + state: 'connected', + serverState: 'disconnected', + placeholderData: {}, + }) + store.getOrCreateRelation('Revision', revisionId, 'page', { + currentId: pageId, + serverId: null, + state: 'connected', + serverState: 'disconnected', + placeholderData: {}, + }) + + const result = await persister.persistAll() + + expect(result.success).toBe(true) + }) +})