Relationship property access now always resolves synchronously

resources/DESIGN.md

@@ -169,6 +169,8 @@ Tests: `../unitTests/resources/defineResource.test.js`, `../unitTests/resources/
 - **Overriding a static entry point takes over its whole contract.** Assigning `static post`/`put`/`patch` on a subclass shadows the `transactional()`-wrapped static, so none of that wrapper runs — including the `when(data, ...)` that resolves `data` and the `allowCreate`/`allowUpdate` gate. That is by design (it is how `login.ts` implements a deliberately pre-authentication endpoint), and it means an override owns two obligations the wrapper would otherwise have met:
   - **`data` is a `MaybePromise` — `await` it.** Protocol callers pass `request.data` through unresolved; over REST that is the streaming deserializer's pending promise (`server/REST.ts` → `getDeserializer(…, true)`). A promise has no own enumerable properties, so skipping the `await` fails silently rather than loudly: `JSON.stringify(body) === '{}'` and every field reads `undefined`.
   - **The override makes the access-control decision.** No `allow*` predicate runs for it. An override that is not meant to be public must check authorization itself.
+- **FK-side relationship accessors (`relationship.from`) are a synchronous contract** (v4 parity). The resolvers built in `Table.updatedAttributes` must read through `getSync`/`getEntry` (which block on a RocksDB block-cache miss, like v4's LMDB page faults) — never `get()`, whose `async: true` path returns a Promise on a cache miss and would leak an intermittent value-or-Promise to `record.<relation>` in user code. Pinned by the "relationship property access is synchronous" test in `../unitTests/resources/query.test.js`, which stubs `store.get` to throw.
+  - **Known gap, not covered by that test:** the `.to` side (one-to-many/many-to-many, resolved via `relatedTable.search(...).asArray`) is synchronous for a local read, but `asArray` can still hand back a Promise when the search lands on a **caching** table whose row needs source revalidation — `transformEntryForSelect` returns `loadingFromSource.then(transform)` for an expired/invalidated entry. Same bug class this PR closes on the FK side; not yet fixed here.
 - When adding a new early-return path inside a commit handler in `_writeUpdate`, follow the blob-cleanup protocol documented in `../DESIGN.md` ("Blob orphan cleanup").
 - If you add a new top-level section to `Table.ts`, drop a `// #section: <name>` marker at its start and add a row to the section map above.
 - Tests for this layer live in `../unitTests/resources/`.

resources/Table.ts

@@ -4888,30 +4888,31 @@ export function makeTable(options) {
 						if (definition) {
 							propertyResolvers[attribute.name] = attribute.resolve = (object, context, entry, returnEntry?) => {
 								const ids = object[relationship.from];
-								if (ids === undefined) return undefined;
+								if (ids == null) return ids;
 								if (attribute.elements) {
-									let hasPromises;
-									const results = ids?.map((id) => {
-										const value = definition.tableClass.primaryStore[returnEntry ? 'getEntry' : 'get'](id, {
-											transaction: txnForContext(context).getReadTxn(),
-										});
-										if (value?.then) hasPromises = true;
-										// for now, we shouldn't be getting promises until rocksdb
+									// tolerate a scalar id in an array-typed FK field (legacy data written
+									// before attribute.set normalized single records to a one-element array)
+									const normalizedIds = Array.isArray(ids) ? ids : [ids];
+									// a fresh array, not `normalizedIds` itself: when ids was already an array,
+									// normalizedIds === ids is the record's own stored FK array, and returning it
+									// would let a caller's mutation (e.g. record.manyToMany.push(x)) alias into it
+									if (normalizedIds.length === 0) return [];
+									// getSync (not get): relationship accessors are a synchronous contract (as in v4);
+									// on RocksDB get() returns a Promise on a block-cache miss, which would leak an
+									// intermittent MaybePromise into user code
+									const store = definition.tableClass.primaryStore;
+									const method = returnEntry ? 'getEntry' : 'getSync';
+									const options = { transaction: txnForContext(context).getReadTxn() };
+									const results = normalizedIds.map((id) => {
+										const value = store[method](id, options);
 										if (TableResource.loadAsInstance === false) freezeRecord(returnEntry ? value?.value : value);
 										return value;
 									});
-									return relationship.filterMissing
-										? hasPromises
-											? Promise.all(results).then((results) => results.filter(exists))
-											: results.filter(exists)
-										: hasPromises
-											? Promise.all(results)
-											: results;
+									return relationship.filterMissing ? results.filter(exists) : results;
 								}
 								const value = definition.tableClass.primaryStore[returnEntry ? 'getEntry' : 'getSync'](ids, {
 									transaction: txnForContext(context).getReadTxn(),
 								});
-								// for now, we shouldn't be getting promises until rocksdb
 								if (TableResource.loadAsInstance === false) freezeRecord(returnEntry ? value?.value : value);
 								return value;
 							};
@@ -4923,7 +4924,9 @@ export function makeTable(options) {
 									object[relationship.from] = targetIds;
 								} else {
 									const targetId = related.getId?.() || related[definition.tableClass.primaryKey];
-									object[relationship.from] = targetId;
+									// an elements attribute always stores an array of ids, so a single
+									// composite (array) id is not misread as multiple scalar ids
+									object[relationship.from] = attribute.elements ? [targetId] : targetId;
 								}
 							};
 							attribute.resolve.definition = attribute.definition || attribute.elements?.definition;

unitTests/resources/query.test.js

@@ -134,6 +134,7 @@ describe('Querying through Resource API', () => {
 				name: i === 17 ? [long_str] : i === 18 ? long_str : 'many-to-many entry ' + i,
 			});
 		}
+		await ManyToMany.put({ id: ['composite', 42], name: 'composite id entry' });
 		let last;
 		for (let i = 0; i < 100; i++) {
 			let many_ids = [];
@@ -766,6 +767,76 @@ describe('Querying through Resource API', () => {
 			});
 		});
 
+		it('relationship property access is synchronous (v4 contract)', async function () {
+			const record = await QueryTable.get('id-3');
+			// pin the contract: resolvers must never touch the async get() path, which
+			// returns a Promise on a RocksDB block-cache miss
+			const stores = [RelatedTable.primaryStore, ManyToMany.primaryStore];
+			const original_gets = stores.map((store) => store.get);
+			for (const store of stores)
+				store.get = () => {
+					throw new Error('relationship resolver must use the synchronous read path');
+				};
+			try {
+				const related = record.related;
+				assert.equal(typeof related?.then, 'undefined', 'many-to-one resolves synchronously');
+				assert.equal(related.name, 'related name 3');
+				const many_to_many = record.manyToMany;
+				assert(Array.isArray(many_to_many), 'array-of-foreign-keys resolves synchronously to an array');
+				assert.equal(many_to_many.length, 3);
+				assert.equal(many_to_many[0].name, 'many-to-many entry 3');
+				// childrenOfSelf resolves via relatedTable.search(...).asArray, not primaryStore.get, so the
+				// stub above doesn't pin this branch — it only demonstrates the local (non-caching) case, and
+				// the known revalidation gap for caching tables (see DESIGN.md) is untouched by this test.
+				const children = related.childrenOfSelf;
+				assert(Array.isArray(children), 'one-to-many resolves synchronously to an array on a local table');
+			} finally {
+				stores.forEach((store, i) => (store.get = original_gets[i]));
+			}
+		});
+
+		it('array-of-FK relationship tolerates a scalar stored id and normalizes single-record sets', async function () {
+			const scalar_stored = many_to_many_attribute.resolve({ manyToManyIds: 3 });
+			assert(Array.isArray(scalar_stored), 'scalar stored id resolves to an array');
+			assert.equal(scalar_stored.length, 1);
+			assert.equal(scalar_stored[0].name, 'many-to-many entry 3');
+
+			const object = {};
+			many_to_many_attribute.set(object, { id: 7 });
+			assert.deepEqual(object.manyToManyIds, [7], 'single record set on an elements attribute stores an array');
+		});
+
+		it('array-of-FK relationship nests a composite (array) related id as one element, not spread ids', function () {
+			const object = {};
+			many_to_many_attribute.set(object, { id: ['composite', 42] });
+			assert.deepEqual(
+				object.manyToManyIds,
+				[['composite', 42]],
+				'a composite id must stay a single array element, not be spread into two scalar ids'
+			);
+
+			const resolved = many_to_many_attribute.resolve(object);
+			assert.equal(resolved.length, 1, 'the composite id resolves to exactly the one related record');
+			assert.equal(resolved[0].name, 'composite id entry');
+		});
+
+		it('array-of-FK relationship with no stored ids resolves to an empty array without reading', async function () {
+			// id-0 was written with manyToManyIds: [] (see `before`)
+			const record = await QueryTable.get('id-0');
+			assert.deepEqual(record.manyToMany, [], 'no ids to resolve, so no related records');
+		});
+
+		it('array-of-FK relationship with no stored ids returns a fresh array, not the stored FK array', function () {
+			const object = { manyToManyIds: [] };
+			const resolved = many_to_many_attribute.resolve(object);
+			resolved.push('unrelated');
+			assert.deepEqual(
+				object.manyToManyIds,
+				[],
+				'mutating the resolved empty array must not alias the stored FK array'
+			);
+		});
+
 		it('Query by join with many-to-many (reverse)', async function () {
 			let results = [];
 			for await (let record of ManyToMany.search({