diff --git a/packages/bindx/src/persistence/MutationCollector.ts b/packages/bindx/src/persistence/MutationCollector.ts index 4ab198c..707d9cc 100644 --- a/packages/bindx/src/persistence/MutationCollector.ts +++ b/packages/bindx/src/persistence/MutationCollector.ts @@ -149,6 +149,12 @@ export class MutationCollector implements MutationDataCollector { // 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) + // Collect relation changes this.collectRelationChanges(entityType, entityId, mutation) @@ -188,7 +194,7 @@ export class MutationCollector implements MutationDataCollector { // Materialize embedded relation data into store entities, // then collect from store for consistent tracking and post-persist ID mapping - this.materializeEntityRelations(entityType, entityId) + this.materializeForCreate(entityType, entityId) // Collect relation values from store const relationFields = this.schemaProvider.getRelationFields(entityType) @@ -378,12 +384,21 @@ export class MutationCollector implements MutationDataCollector { return { delete: true } case 'creating': - // Create new entity with placeholder data - if (Object.keys(placeholderData).length > 0) { - const createData = this.processNestedData(placeholderData) - return { create: createData } + // Empty placeholderData is a legitimate no-op (user opened a create + // form but typed nothing). Non-empty placeholderData reaching this + // branch is an invariant violation — the materialization pass + // (materializeForCreate / materializeForUpdate) must run before + // collection and flip the relation to 'connected' with a tempId. + // Reaching here means a caller skipped materialization, which + // would produce a mutation the server accepts but whose returned + // ID we can't map back (see issue #23). + if (Object.keys(placeholderData).length === 0) { + return null } - return null + throw new Error( + `Invariant: hasOne relation ${entityType}.${fieldName} in 'creating' state ` + + `with non-empty placeholderData must be materialized before collection`, + ) default: return null @@ -592,15 +607,19 @@ export class MutationCollector implements MutationDataCollector { return isPersistedId(id) } - // ==================== Embedded Data Materialization ==================== + // ==================== Materialization ==================== /** - * Materializes embedded relation data in an entity's snapshot into proper - * store entities and relations. This normalizes inline objects (e.g. - * `reviews: [{ reviewType: 'expert' }]`) into tracked entities with temp IDs, - * enabling proper post-persist ID mapping and commit. + * Materializes all relation state into trackable store entities on the + * create path: placeholder-backed 'creating' hasOne relations AND embedded + * inline objects/arrays in the snapshot data. After this pass, the + * collection phase is a pure read over the store. + * + * Called recursively for every newly materialized temp entity (both from + * placeholder and embedded paths), since a newly-created entity can itself + * carry further embedded or placeholder data. */ - private materializeEntityRelations(entityType: string, entityId: string): void { + private materializeForCreate(entityType: string, entityId: string): void { if (!this.schemaProvider.hasEntity(entityType)) return const snapshot = this.store.getEntitySnapshot(entityType, entityId) @@ -610,19 +629,76 @@ export class MutationCollector implements MutationDataCollector { const relationFields = this.schemaProvider.getRelationFields(entityType) for (const fieldName of relationFields) { - const value = data[fieldName] - if (value === null || value === undefined) continue - const relationType = this.schemaProvider.getRelationType(entityType, fieldName) - if (relationType === 'hasOne' && typeof value === 'object' && !Array.isArray(value)) { - this.materializeEmbeddedHasOne(entityType, entityId, fieldName, value as Record) - } else if (relationType === 'hasMany' && Array.isArray(value) && value.length > 0) { - this.materializeEmbeddedHasMany(entityType, entityId, fieldName, value) + if (relationType === 'hasOne') { + if (this.materializePlaceholderHasOne(entityType, entityId, fieldName)) continue + + const value = data[fieldName] + if (value !== null && value !== undefined && typeof value === 'object' && !Array.isArray(value)) { + this.materializeEmbeddedHasOne(entityType, entityId, fieldName, value as Record) + } + } else if (relationType === 'hasMany') { + const value = data[fieldName] + if (Array.isArray(value) && value.length > 0) { + this.materializeEmbeddedHasMany(entityType, entityId, fieldName, value) + } } } } + /** + * Materializes placeholder-backed 'creating' hasOne relations only. Safe + * to call on existing (server-side) entities — never touches hasMany state + * or embedded snapshot data, because for an already-persisted parent the + * store-level hasMany/connected state is already authoritative. + * + * Newly created temp entities below still go through the full create-path + * materialization, since they have no server state yet. + */ + private materializeForUpdate(entityType: string, entityId: string): void { + if (!this.schemaProvider.hasEntity(entityType)) return + + for (const fieldName of this.schemaProvider.getRelationFields(entityType)) { + if (this.schemaProvider.getRelationType(entityType, fieldName) !== 'hasOne') continue + this.materializePlaceholderHasOne(entityType, entityId, fieldName) + } + } + + /** + * Materializes a hasOne relation in the 'creating' state with non-empty + * placeholderData (PlaceholderHandle pattern). Creates a real store entity + * from the placeholder data and flips the relation to 'connected' with a + * temp ID, so subsequent collection goes through the standard + * tempId-inline-create branch and post-persist ID mapping works. + * + * Returns true if materialization happened (caller should skip embedded + * processing for this field). + */ + private materializePlaceholderHasOne( + entityType: string, + entityId: string, + fieldName: string, + ): boolean { + const relation = this.store.getRelation(entityType, entityId, fieldName) + if (!relation || relation.state !== 'creating') return false + if (Object.keys(relation.placeholderData).length === 0) return false + + const targetType = this.schemaProvider.getRelationTarget(entityType, fieldName) + if (!targetType) return false + + const tempId = this.store.createEntity(targetType, relation.placeholderData) + this.store.setRelation(entityType, entityId, fieldName, { + currentId: tempId, + state: 'connected', + placeholderData: {}, + }) + + // Newly created temp entity — walk its embedded/placeholder data too. + this.materializeForCreate(targetType, tempId) + return true + } + /** * Materializes an embedded hasMany array into store entities. * Skips if the hasMany state already has store-managed entities. @@ -662,7 +738,7 @@ export class MutationCollector implements MutationDataCollector { this.store.addToHasMany(entityType, entityId, fieldName, tempId) // Recursively materialize nested relations in the new entity - this.materializeEntityRelations(targetType, tempId) + this.materializeForCreate(targetType, tempId) } // Clear embedded array from parent snapshot @@ -712,7 +788,7 @@ export class MutationCollector implements MutationDataCollector { }) // Recursively materialize nested relations - this.materializeEntityRelations(targetType, tempId) + this.materializeForCreate(targetType, tempId) } // Clear embedded data from parent snapshot diff --git a/tests/hasOneCreatingMaterialization.test.ts b/tests/hasOneCreatingMaterialization.test.ts new file mode 100644 index 0000000..b92ac03 --- /dev/null +++ b/tests/hasOneCreatingMaterialization.test.ts @@ -0,0 +1,468 @@ +import { describe, test, expect, beforeEach, mock } from 'bun:test' +import { + SnapshotStore, + MutationCollector, + ContemberSchemaMutationAdapter, + ActionDispatcher, + BatchPersister, + isTempId, + type BackendAdapter, + type TransactionMutation, + type SchemaNames, +} from '@contember/bindx' + +/** + * Regression tests for: hasOne relation in 'creating' state (placeholder-backed) + * must be materialized into a real store entity during mutation collection so + * the post-persist flow can map the server-assigned ID back onto a tracked + * entity. + * + * Without materialization, extractNestedResultsFromNode sees a relation with + * currentId = null and skips. commitNestedResults never runs for the nested + * entity, the server-assigned ID is lost, the relation stays in 'creating' + * with empty placeholderData, and HasOneHandle returns a PlaceholderHandle + * with no data — rendering fields as blank until a page refresh clears the + * store. + * + * Real-world scenario: admin create form where the parent entity (e.g. + * Lecturer) has a hasOne to a child (User) and the user fills fields via + * PlaceholderHandle which dispatches SET_PLACEHOLDER_DATA. On persist the + * bug would materialize correctly server-side but leave the client store in + * a broken state. + */ + +const schema: SchemaNames = { + entities: { + Lecturer: { + name: 'Lecturer', + scalars: ['id', 'qualification', 'status'], + fields: { + id: { type: 'column' }, + qualification: { type: 'column' }, + status: { type: 'column' }, + user: { type: 'one', entity: 'User' }, + students: { type: 'many', entity: 'Student' }, + }, + }, + User: { + name: 'User', + scalars: ['id', 'firstName', 'lastName', 'email'], + fields: { + id: { type: 'column' }, + firstName: { type: 'column' }, + lastName: { type: 'column' }, + email: { type: 'column' }, + profile: { type: 'one', entity: 'Profile' }, + }, + }, + Profile: { + name: 'Profile', + scalars: ['id', 'bio', 'avatar'], + fields: { + id: { type: 'column' }, + bio: { type: 'column' }, + avatar: { type: 'column' }, + }, + }, + Student: { + name: 'Student', + scalars: ['id', 'enrolledAt'], + fields: { + id: { type: 'column' }, + enrolledAt: { type: 'column' }, + user: { type: 'one', entity: 'User' }, + }, + }, + }, + enums: {}, +} + +describe('hasOne creating-state materialization', () => { + describe('MutationCollector', () => { + let store: SnapshotStore + let collector: MutationCollector + + beforeEach(() => { + store = new SnapshotStore() + const schemaAdapter = new ContemberSchemaMutationAdapter(schema) + collector = new MutationCollector(store, schemaAdapter) + }) + + test('materializes placeholder-backed creating relation into a real store entity', () => { + // New lecturer with placeholder-backed user (PlaceholderHandle pattern) + const lecturerId = store.createEntity('Lecturer', { status: 'active' }) + store.getOrCreateRelation('Lecturer', lecturerId, 'user', { + currentId: null, + serverId: null, + state: 'creating', + serverState: 'disconnected', + placeholderData: { + firstName: 'Jan', + lastName: 'Novák', + email: 'jan@example.com', + }, + }) + + // Sanity: no User entity exists yet, relation has no currentId + expect(store.getRelation('Lecturer', lecturerId, 'user')?.currentId).toBeNull() + + const mutation = collector.collectCreateData('Lecturer', lecturerId) + + // Mutation payload is correct + expect(mutation).toEqual({ + status: 'active', + user: { + create: { + firstName: 'Jan', + lastName: 'Novák', + email: 'jan@example.com', + }, + }, + }) + + // Without the fix, the relation stays in 'creating' with currentId = null + // and no User entity is created. With the fix: + const relation = store.getRelation('Lecturer', lecturerId, 'user') + expect(relation).not.toBeUndefined() + expect(relation!.state).toBe('connected') + expect(relation!.currentId).not.toBeNull() + expect(isTempId(relation!.currentId!)).toBe(true) + + // The materialized User entity must exist in the store with the placeholder data + const userTempId = relation!.currentId! + const userSnapshot = store.getEntitySnapshot('User', userTempId) + expect(userSnapshot).not.toBeUndefined() + expect(userSnapshot!.data).toMatchObject({ + firstName: 'Jan', + lastName: 'Novák', + email: 'jan@example.com', + }) + + // The materialized entity must be tracked for post-persist ID mapping + expect(collector.getNestedEntityIds().has(userTempId)).toBe(true) + expect(collector.getNestedEntityTypes().get(userTempId)).toBe('User') + }) + + test('materializes creating relation during update of existing parent', () => { + // Existing lecturer on server, user later added via placeholder + store.setEntityData('Lecturer', 'lect-1', { + id: 'lect-1', + qualification: 'PhD', + user: null, + }, true) + store.setExistsOnServer('Lecturer', 'lect-1', true) + + store.getOrCreateRelation('Lecturer', 'lect-1', 'user', { + currentId: null, + serverId: null, + state: 'creating', + serverState: 'disconnected', + placeholderData: { + firstName: 'Petra', + email: 'petra@example.com', + }, + }) + + const mutation = collector.collectUpdateData('Lecturer', 'lect-1') + + expect(mutation).toEqual({ + user: { + create: { + firstName: 'Petra', + email: 'petra@example.com', + }, + }, + }) + + // The relation must have been materialized and tracked + const relation = store.getRelation('Lecturer', 'lect-1', 'user') + expect(relation!.state).toBe('connected') + expect(isTempId(relation!.currentId!)).toBe(true) + expect(collector.getNestedEntityTypes().get(relation!.currentId!)).toBe('User') + }) + + test('recursively materializes nested placeholder hasOne (creating → creating)', () => { + // Lecturer.user placeholder contains its own placeholder for user.profile. + // Both levels must become real store entities with temp IDs so each can + // receive a server-assigned ID after persist. + const lecturerId = store.createEntity('Lecturer', { status: 'active' }) + store.getOrCreateRelation('Lecturer', lecturerId, 'user', { + currentId: null, + serverId: null, + state: 'creating', + serverState: 'disconnected', + placeholderData: { + firstName: 'Eva', + email: 'eva@example.com', + }, + }) + + const mutation = collector.collectCreateData('Lecturer', lecturerId) + + // Relation materialized → real store entity with tempId + const userRelation = store.getRelation('Lecturer', lecturerId, 'user') + expect(userRelation!.state).toBe('connected') + const userTempId = userRelation!.currentId! + expect(isTempId(userTempId)).toBe(true) + + // Now a deeper placeholder is attached on the newly materialized User. + // In a real flow SET_PLACEHOLDER_DATA on profile would fire through a + // HasOneHandle after the user was already materialized, but we simulate + // it post-hoc here and re-collect to exercise the nested walk. + store.getOrCreateRelation('User', userTempId, 'profile', { + currentId: null, + serverId: null, + state: 'creating', + serverState: 'disconnected', + placeholderData: { bio: 'Photographer' }, + }) + + // Re-run collection: User tempId is already tracked, so collecting + // its create-data triggers materializeForCreate on it and recursively + // handles the profile placeholder. + const mutation2 = collector.collectCreateData('Lecturer', lecturerId) + + // Outer mutation carries the full nested chain + expect(mutation2).toMatchObject({ + status: 'active', + user: { + create: { + firstName: 'Eva', + email: 'eva@example.com', + profile: { + create: { bio: 'Photographer' }, + }, + }, + }, + }) + + // Profile relation materialized, and its tempId is tracked for ID mapping + const profileRelation = store.getRelation('User', userTempId, 'profile') + expect(profileRelation!.state).toBe('connected') + const profileTempId = profileRelation!.currentId! + expect(isTempId(profileTempId)).toBe(true) + expect(collector.getNestedEntityTypes().get(profileTempId)).toBe('Profile') + + // First-pass mutation already returns the flat (profile-less) form, + // verifying the collector is idempotent on later passes adding depth. + expect(mutation).toMatchObject({ + user: { create: { firstName: 'Eva' } }, + }) + }) + + test('materializes placeholder hasOne carried on a hasMany-created item', () => { + // Lecturer has a hasMany `students` with a freshly-created Student + // whose own `user` hasOne is placeholder-backed. The whole chain must + // materialize so both the Student and its nested User get temp IDs. + const lecturerId = store.createEntity('Lecturer', { status: 'active' }) + + const studentTempId = store.createEntity('Student', { enrolledAt: '2026-04-01' }) + store.getOrCreateHasMany('Lecturer', lecturerId, 'students', []) + store.addToHasMany('Lecturer', lecturerId, 'students', studentTempId) + + store.getOrCreateRelation('Student', studentTempId, 'user', { + currentId: null, + serverId: null, + state: 'creating', + serverState: 'disconnected', + placeholderData: { + firstName: 'Pavel', + email: 'pavel@example.com', + }, + }) + + const mutation = collector.collectCreateData('Lecturer', lecturerId) + + expect(mutation).toMatchObject({ + status: 'active', + students: [ + expect.objectContaining({ + create: expect.objectContaining({ + enrolledAt: '2026-04-01', + user: { + create: { + firstName: 'Pavel', + email: 'pavel@example.com', + }, + }, + }), + alias: studentTempId, + }), + ], + }) + + // Nested user relation must be materialized and tracked + const userRelation = store.getRelation('Student', studentTempId, 'user') + expect(userRelation!.state).toBe('connected') + const userTempId = userRelation!.currentId! + expect(isTempId(userTempId)).toBe(true) + expect(collector.getNestedEntityTypes().get(userTempId)).toBe('User') + expect(collector.getNestedEntityTypes().get(studentTempId)).toBe('Student') + }) + + test('invariant throws if creating hasOne with data survives collection (materialization bypassed)', () => { + // Simulate a code path that would reach collectHasOneOperation without + // prior materialization — we do it by using a schema provider that + // can't resolve the relation target, which is how materializeFor* + // bails out silently. The invariant guard in the 'creating' branch + // must then catch it loudly instead of producing a ghost mutation. + const brokenProvider = { + getScalarFields: () => ['id', 'status'] as const, + getRelationFields: () => ['user'] as const, + getRelationType: () => 'hasOne' as const, + getRelationTarget: () => undefined, // <- this is the breakage + hasEntity: () => true, + } + const brokenCollector = new MutationCollector(store, brokenProvider) + const lecturerId = store.createEntity('Lecturer', { status: 'active' }) + store.getOrCreateRelation('Lecturer', lecturerId, 'user', { + currentId: null, + serverId: null, + state: 'creating', + serverState: 'disconnected', + placeholderData: { firstName: 'Ghost' }, + }) + + expect(() => brokenCollector.collectCreateData('Lecturer', lecturerId)).toThrow( + /must be materialized before collection/, + ) + }) + + test('returns null for empty placeholderData without mutating the store', () => { + const lecturerId = store.createEntity('Lecturer', { status: 'active' }) + store.getOrCreateRelation('Lecturer', lecturerId, 'user', { + currentId: null, + serverId: null, + state: 'creating', + serverState: 'disconnected', + placeholderData: {}, + }) + + const mutation = collector.collectCreateData('Lecturer', lecturerId) + + expect(mutation).toEqual({ status: 'active' }) + + // Relation should be untouched — no materialization for empty placeholder + const relation = store.getRelation('Lecturer', lecturerId, 'user') + expect(relation!.state).toBe('creating') + expect(relation!.currentId).toBeNull() + }) + }) + + describe('BatchPersister end-to-end', () => { + test('after persist, hasOne creating-state is connected to a server-persisted entity', async () => { + let capturedMutations: readonly TransactionMutation[] = [] + + const adapter: BackendAdapter = { + query: mock(() => Promise.resolve([])), + persist: mock(() => Promise.resolve({ ok: true })), + delete: mock(() => Promise.resolve({ ok: true })), + persistTransaction: mock((mutations: readonly TransactionMutation[]) => { + capturedMutations = mutations + // Simulate Contember response: parent gets server ID, nested user gets + // server ID, both returned as nestedResults for inline creates. + let serverIdCounter = 0 + const allocate = () => `server-${++serverIdCounter}` + + return Promise.resolve({ + ok: true, + results: mutations.map(m => { + const parentServerId = allocate() + const nestedResults: Array<{ + entityType: string + entityId: string + ok: boolean + persistedId: string + }> = [] + + // Walk the mutation payload and allocate server IDs for any + // inline creates — matches BatchPersister's extractNestedResultsFromNode. + const userOp = (m.data as Record | undefined)?.['user'] as + | { create?: Record } + | undefined + if (userOp?.create) { + const relation = store.getRelation(m.entityType, m.entityId, 'user') + if (relation?.currentId) { + nestedResults.push({ + entityType: 'User', + entityId: relation.currentId, + ok: true, + persistedId: allocate(), + }) + } + } + + return { + entityType: m.entityType, + entityId: m.entityId, + ok: true, + persistedId: parentServerId, + nestedResults: nestedResults.length > 0 ? nestedResults : undefined, + } + }), + }) + }), + } + + const store = new SnapshotStore() + const dispatcher = new ActionDispatcher(store) + const schemaAdapter = new ContemberSchemaMutationAdapter(schema) + const mutationCollector = new MutationCollector(store, schemaAdapter) + const persister = new BatchPersister(adapter, store, dispatcher, { + mutationCollector, + schema: schemaAdapter as never, + }) + + // Simulate the admin create form: new Lecturer, user filled via PlaceholderHandle. + const lecturerId = store.createEntity('Lecturer', { status: 'active' }) + store.getOrCreateRelation('Lecturer', lecturerId, 'user', { + currentId: null, + serverId: null, + state: 'creating', + serverState: 'disconnected', + placeholderData: { + firstName: 'Jan', + lastName: 'Novák', + email: 'jan@example.com', + }, + }) + + const result = await persister.persistAll() + + expect(result.success).toBe(true) + expect(capturedMutations).toHaveLength(1) + expect(capturedMutations[0]!.data).toMatchObject({ + user: { + create: { + firstName: 'Jan', + lastName: 'Novák', + email: 'jan@example.com', + }, + }, + }) + + // The relation must resolve to a persisted User entity after commit + ID mapping. + const relation = store.getRelation('Lecturer', lecturerId, 'user') + expect(relation).not.toBeUndefined() + expect(relation!.state).toBe('connected') + expect(relation!.currentId).not.toBeNull() + + // Without the fix, currentId would still be null and no User entity would exist, + // leaving HasOneHandle returning a PlaceholderHandle with empty placeholderData. + const userId = relation!.currentId! + expect(isTempId(userId)).toBe(false) // tempId mapped to persisted server ID + + const userSnapshot = store.getEntitySnapshot('User', userId) + expect(userSnapshot).not.toBeUndefined() + expect(userSnapshot!.data).toMatchObject({ + firstName: 'Jan', + lastName: 'Novák', + email: 'jan@example.com', + }) + expect(store.existsOnServer('User', userId)).toBe(true) + + // Placeholder data must be cleared after commit + expect(relation!.placeholderData).toEqual({}) + }) + }) +}) diff --git a/tests/mutationCollector.test.ts b/tests/mutationCollector.test.ts index 3929ff9..e512096 100644 --- a/tests/mutationCollector.test.ts +++ b/tests/mutationCollector.test.ts @@ -22,6 +22,7 @@ const testSchema: SchemaNames = { id: { type: 'column' }, name: { type: 'column' }, email: { type: 'column' }, + articles: { type: 'many', entity: 'Article' }, }, }, Tag: { @@ -728,17 +729,20 @@ describe('MutationCollector', () => { const mutation = collector.collectUpdateData('Article', 'a-1') - expect(mutation).toEqual({ - author: { - create: { - name: 'New Author', - articles: [ - { connect: { id: 'existing-article' } }, - { create: { title: 'New Article' } }, - ], - }, - }, - }) + expect(mutation).not.toBeNull() + const authorOp = mutation!['author'] as { create: Record } + expect(authorOp.create['name']).toBe('New Author') + + const articlesOps = authorOp.create['articles'] as Array> + expect(articlesOps).toHaveLength(2) + expect(articlesOps).toContainEqual(expect.objectContaining({ + connect: { id: 'existing-article' }, + alias: 'existing-article', + })) + expect(articlesOps).toContainEqual(expect.objectContaining({ + create: { title: 'New Article' }, + alias: expect.stringContaining('__temp_'), + })) }) test('should handle nested update for existing has-one relation', () => {