Merge pull request #23 from contember/fix/placeholder-hasone-materialization

fix: materialize placeholder-backed hasOne creating relations on persist

Author: vparys

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<string, unknown>)
-			} 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<string, unknown>)
+				}
+			} 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