diff --git a/.changeset/file-hydration-tombstone-agreement.md b/.changeset/file-hydration-tombstone-agreement.md new file mode 100644 index 0000000000..90141af4a5 --- /dev/null +++ b/.changeset/file-hydration-tombstone-agreement.md @@ -0,0 +1,10 @@ +--- +"@objectstack/objectql": minor +"@objectstack/service-storage": patch +--- + +Record file-field hydration now answers the same question about a `sys_file` tombstone that the download path answers (#11427). `#10246` stopped `GET /api/v1/storage/files/:id` treating a tombstone (`status: 'deleted'` + `deleted_at`) as the last word — it asks the reap guard's own `findFileHolder` and serves the row for as long as something still holds it — but the record read kept the older `status === 'committed'` rule. One `sys_file` row therefore answered `200` at the download endpoint and a bare id inside a record payload, which UI and export render as "this record has no attachment". + +The population is narrow and unchanged in every other respect: `claimFile` already un-tombstones a field file synchronously when a record re-points at it, and attachments-scope files are never reached by field hydration, so what this closes is the residual the reap guard's sweep-time re-verification names — hook races, direct-driver writes, and future trash restore. A tombstone nothing holds still hydrates as a bare id, and a `pending` upload is untouched. + +The predicate is not re-derived in the engine. `ObjectQL` gains `registerHeldFileResolver` (type `HeldFileResolver`), which the storage plugin fills with `findHeldFiles` — the batched form of `findFileHolder`, asking the same union of `sys_attachment` join rows and the `ref_*` ownership columns. Batched because hydration runs over many rows per read: a read with no tombstone costs nothing, and the residual case costs one extra query for the whole read rather than one per file. Engines with no storage plugin keep tombstones un-hydrated exactly as before. diff --git a/content/docs/permissions/attachments-access.mdx b/content/docs/permissions/attachments-access.mdx index 04df23e41a..c7faba4edf 100644 --- a/content/docs/permissions/attachments-access.mdx +++ b/content/docs/permissions/attachments-access.mdx @@ -109,13 +109,17 @@ can be shared across records). Reclamation is handled by the platform LifecycleS attachments-scope file is deleted, the file is tombstoned; a reap guard re-verifies zero references at sweep time and deletes the storage bytes before the row is reaped (abandoned `pending` uploads are reaped too). - **A tombstone is recoverable state, not a delete — and downloads treat it that - way.** Re-attaching the file, or re-claiming it through a record field, makes - it downloadable again *immediately*, with no sweep in between: the download - endpoints ask the same "is anything still holding this file?" question the - reap guard asks before it reclaims anything. The row itself stays tombstoned - until a sweep tidies it, and a file with no holder left still answers - `FILE_NOT_FOUND` (404). + **A tombstone is recoverable state, not a delete — and every reader treats it + that way.** Re-attaching the file, or re-claiming it through a record field, + makes it readable again *immediately*, with no sweep in between: the download + endpoints **and record file-field hydration** ask the same "is anything still + holding this file?" question the reap guard asks before it reclaims anything. + Both reach it through that one predicate rather than each deciding for itself, + which is what stops the two surfaces answering differently about one row — a + file `GET /storage/files/:id` serves is a file a record read expands into + `{ id, name, size, mimeType, url }`. The row itself stays tombstoned until a + sweep tidies it, and a file with no holder left still answers + `FILE_NOT_FOUND` (404) and keeps its bare id in a record payload. - **`sys_upload_session`** — abandoned/terminal chunked-upload sessions are reaped, and a reap guard aborts the underlying backend multipart upload (S3 `AbortMultipartUpload` / local parts dir) first, so already-uploaded diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index f02cf6cff3..14b5eb407f 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1898,6 +1898,24 @@ export type EngineMiddleware = ( next: () => Promise ) => Promise; +/** + * "Which of these tombstoned `sys_file` rows is something still holding?" + * (#11427). + * + * Takes the whole tombstoned set from ONE record read and returns the subset + * still held, as a set of stringified ids. Batched rather than per-row on + * purpose: hydration runs over many rows per read, so asking per file would be + * N queries per read. + * + * The engine declares this shape but never implements it — "still held" has one + * definition (`findFileHolder`, the union of `sys_attachment` join rows and the + * `ref_*` ownership columns) and it lives in the storage package. See + * `ObjectQL.registerHeldFileResolver`. + */ +export type HeldFileResolver = ( + rows: Array>, +) => Promise>; + /** * The stack collections the engine decomposes into individual registry items — * ONE list, read by the ONE body both registration seams run @@ -3072,6 +3090,33 @@ export class ObjectQL implements IObjectQLEngine { } } + /** + * "Which of these tombstoned `sys_file` rows is something still holding?" + * (#11427) — supplied by the storage plugin, never derived here. + * + * File-field hydration must answer the same question the download path + * answers (#10246) or one row gets two answers. That question has exactly one + * definition, `findFileHolder`, and it lives in `@objectstack/service-storage` + * — a package this one does not and must not depend on. So the engine + * declares the seam and the storage plugin fills it, the same handover + * `resolveFileHolder` makes to the download routes. + * + * BATCHED on purpose: hydration runs over many rows per read, so a per-row + * holder check would be N queries per read. The resolver takes the whole + * tombstoned set and returns the ids still held. + */ + private _heldFileResolver?: HeldFileResolver; + + /** + * Wire the batched holder question (#11427). Last registration wins; leaving + * it unwired keeps tombstoned files un-hydrated, which is what this engine + * did before the seam existed. + */ + registerHeldFileResolver(fn: HeldFileResolver): void { + this._heldFileResolver = fn; + this.logger.debug('Registered held-file resolver for sys_file hydration'); + } + /** * Register a middleware function * Middlewares execute in onion model around every data operation. @@ -8078,8 +8123,52 @@ export class ObjectQL implements IObjectQLEngine { } const fileMap = new Map(); + // [#11427] `committed` is servable and always was. A TOMBSTONE + // (`status: 'deleted'` + `deleted_at`) is recoverable state, not a delete: + // it is a claim about the future (this row is reapable once the grace + // window ends) that the sweep re-checks and often withdraws. #10246 already + // stopped the two download endpoints treating it as the last word — they + // ask the reap guard's own `findFileHolder` and serve the row for as long + // as something still holds it. This pass did not, so one `sys_file` row + // answered 200 at `/files/:id` and a bare id here: two read surfaces, two + // answers, and consumers render the bare id as "no attachment". + // + // ⛔ The predicate is NOT re-derived here. "Still held" has ONE definition + // (`findFileHolder`, a deliberate union of `sys_attachment` join rows AND + // the `ref_*` ownership columns) and it lives in the storage package, which + // this one cannot import. A copy narrower by a limb would hide files the + // sweep refuses to reap — the same defect one limb over — so the question + // arrives through {@link registerHeldFileResolver} instead, the same + // handover `resolveFileHolder` makes to the download routes. Unwired (bare + // kernel, no storage plugin, tests): tombstones stay hidden, exactly as + // before this existed. + const tombstoned: any[] = []; for (const row of fileRows) { - if (row?.id != null && row.status === 'committed') fileMap.set(String(row.id), row); + if (row?.id == null) continue; + if (row.status === 'committed') fileMap.set(String(row.id), row); + else if (row.status === 'deleted') tombstoned.push(row); + } + // Lazy by construction: a batch with no tombstone — every ordinary read — + // costs nothing at all, and the resolver is BATCHED, so the residual case + // costs one extra query for the whole read rather than one per row. + if (tombstoned.length > 0 && this._heldFileResolver) { + try { + const held = await this._heldFileResolver(tombstoned); + for (const row of tombstoned) { + if (held?.has(String(row.id))) fileMap.set(String(row.id), row); + } + } catch (error) { + // Unreadable evidence is not evidence of a holder. Keep the ids + // un-hydrated — the answer this pass gave before #11427, and the same + // direction the download path fails in (`isServableForDownload`) and + // the reap guard fails in (it vetoes rather than reaps when it cannot + // tell). Distinct from the #6116 catch above, which covers the + // `sys_file` read itself and is untouched. + this.logger.warn( + 'sys_file holder check failed; tombstoned file fields keep their raw ids for this read', + { object: objectName, tombstonedIds: tombstoned.length, error: (error as Error)?.message }, + ); + } } if (fileMap.size === 0) return records; diff --git a/packages/objectql/src/index.ts b/packages/objectql/src/index.ts index d91d240a2a..7e8e0569db 100644 --- a/packages/objectql/src/index.ts +++ b/packages/objectql/src/index.ts @@ -63,7 +63,7 @@ export type { CompanionFieldMeta, CompanionObjectMeta } from './search-companion // Export Engine export { ObjectQL, ObjectRepository, ScopedContext } from './engine.js'; -export type { HookHandler, HookEntry, OperationContext, EngineMiddleware } from './engine.js'; +export type { HookHandler, HookEntry, OperationContext, EngineMiddleware, HeldFileResolver } from './engine.js'; export type { AdmittedValueShapeViolationTally } from './engine.js'; export { SummaryRecomputeError } from './summary-errors.js'; export type { SummaryRecomputeFailure } from './summary-errors.js'; diff --git a/packages/services/service-storage/package.json b/packages/services/service-storage/package.json index eac112af63..b6a10e6076 100644 --- a/packages/services/service-storage/package.json +++ b/packages/services/service-storage/package.json @@ -38,6 +38,7 @@ }, "devDependencies": { "@objectstack/objectql": "workspace:*", + "@objectstack/driver-sql": "workspace:*", "@types/node": "^26.2.0", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/packages/services/service-storage/src/attachment-lifecycle.ts b/packages/services/service-storage/src/attachment-lifecycle.ts index 7d1b0dfda2..2e0349f542 100644 --- a/packages/services/service-storage/src/attachment-lifecycle.ts +++ b/packages/services/service-storage/src/attachment-lifecycle.ts @@ -365,6 +365,54 @@ export async function findFileHolder( return hasFieldReferenceOwner(row) ? 'field-owner' : null; } +/** + * The BATCHED form of {@link findFileHolder} — "which of these files is still + * held?" — for callers holding many rows at once (#11427). + * + * Record file-field hydration is such a caller: it must reach the same verdict + * the download path reaches (#10246) or one `sys_file` row gets two answers, + * but it runs over many rows per read, so asking {@link findFileHolder} per + * file would be N queries per read. This asks the SAME union in at most one + * extra query for the whole batch. + * + * ⚠️ Same union, same limbs, deliberately in the cheaper order. {@link + * findFileHolder} asks the join-row limb first because it must NAME the + * surface; this one only needs "held or not", so it takes the free limb first: + * {@link hasFieldReferenceOwner} is a pure column test on rows the caller has + * already read, and every id it settles is an id the join-row query never has + * to carry. `||` commutes, so the verdict is identical either way — pinned as + * an equivalence in `tombstone-hydration-download-agreement.test.ts` rather + * than asserted here. + * + * Cost, stated rather than assumed: + * - no rows, or every row settled by the columns → ZERO queries; + * - otherwise → exactly ONE `$in` read of `sys_attachment`, whatever the + * number of files or records involved. + */ +export async function findHeldFiles( + engine: Pick, + rows: Array>, +): Promise> { + const held = new Set(); + const needJoinCheck: string[] = []; + for (const row of rows) { + if (row?.id == null) continue; + const id = String(row.id); + // The free limb first — a pure test on a row already in hand. + if (hasFieldReferenceOwner(row)) held.add(id); + else needJoinCheck.push(id); + } + if (needJoinCheck.length === 0) return held; + const refs = await engine.find('sys_attachment', { + where: { file_id: { $in: needJoinCheck } }, + context: { ...SYSTEM_CTX }, + }); + for (const ref of refs ?? []) { + if (ref?.file_id != null) held.add(String(ref.file_id)); + } + return held; +} + /** * The `sys_file` reap guard ({@link LifecycleReapGuard} shape from * `@objectstack/objectql`, duck-typed here to avoid the dependency). diff --git a/packages/services/service-storage/src/storage-service-plugin.ts b/packages/services/service-storage/src/storage-service-plugin.ts index 00fa41cd83..a531236de2 100644 --- a/packages/services/service-storage/src/storage-service-plugin.ts +++ b/packages/services/service-storage/src/storage-service-plugin.ts @@ -21,7 +21,7 @@ import { StorageMetadataStore } from './metadata-store.js'; import type { FileRecord } from './metadata-store.js'; import { registerStorageRoutes } from './storage-routes.js'; import type { FileReadVerdict } from './storage-routes.js'; -import { installAttachmentLifecycleHooks, createSysFileReapGuard, createUploadSessionReapGuard, findFileHolder } from './attachment-lifecycle.js'; +import { installAttachmentLifecycleHooks, createSysFileReapGuard, createUploadSessionReapGuard, findFileHolder, findHeldFiles } from './attachment-lifecycle.js'; import { installFileReferenceHooks } from './file-reference-lifecycle.js'; import { installAttachmentAccessHooks, installAttachmentReadVisibility } from './attachment-access-hooks.js'; import { SystemFile, SystemUploadSession } from './objects/index.js'; @@ -353,6 +353,22 @@ export class StorageServicePlugin implements Plugin { // guard below re-verifies the ownership columns — and re-reads the // deployment flag, fresh — before any byte is deleted. installFileReferenceHooks(engine as any, () => this.storage, ctx.logger); + // "Is anything still holding this tombstone?" for RECORD FILE-FIELD + // HYDRATION (#11427). The download path got this question in #10246; + // the record read kept the older `status === 'committed'` rule, so one + // `sys_file` row answered 200 at `/files/:id` and a bare id inside a + // record payload — which UI and export render as "no attachment". + // + // Handed over rather than re-derived, exactly like `resolveFileHolder` + // below, and BATCHED: hydration runs over many rows per read, so the + // engine passes the whole tombstoned set and `findHeldFiles` answers it + // in at most one extra query. Duck-typed so an older engine without the + // seam simply keeps tombstones un-hydrated. + if (typeof (engine as any).registerHeldFileResolver === 'function') { + (engine as any).registerHeldFileResolver( + (rows: Array>) => findHeldFiles(engine as any, rows), + ); + } try { const lifecycle = ctx.getService('lifecycle'); if (lifecycle && typeof lifecycle.registerReapGuard === 'function') { diff --git a/packages/services/service-storage/src/tombstone-hydration-download-agreement.test.ts b/packages/services/service-storage/src/tombstone-hydration-download-agreement.test.ts new file mode 100644 index 0000000000..9bb43b49fa --- /dev/null +++ b/packages/services/service-storage/src/tombstone-hydration-download-agreement.test.ts @@ -0,0 +1,385 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #11427 — record file-field hydration and the download path must answer the + * SAME question about one `sys_file` row. + * + * ## The defect + * + * #10246 stopped the two download endpoints treating a `sys_file` tombstone as + * the last word: they ask the reap guard's own `findFileHolder` and serve a + * `status='deleted'` row for as long as something still holds it. + * + * The record read was not part of that ruling. `resolveFileReferences` + * (`packages/objectql/src/engine.ts`) — the pass that turns a stored `sys_file` + * id into `{ id, name, size, mimeType, url }` — kept the older, narrower rule, + * `row.status === 'committed'`. A file failing it keeps its bare id, which UI + * and export render as "this record has no attachment". + * + * So for ONE row, post-#10246: `GET /api/v1/storage/files/:id` answers 200 + * while a record read hydrating that same id answers a bare id. Two read + * surfaces, two answers about one row. + * + * ## The population this pins — bounded, not a re-litigation of #10246 + * + * Most field files never reach this state: `claimFile` + * (`file-reference-lifecycle.ts`) un-tombstones synchronously at re-point time, + * patching `status: 'committed'`, `deleted_at: null` — "a file re-referenced + * within its grace window comes back to life". Attachments-scope files are not + * involved either; that surface is `sys_attachment` join rows, not record file + * fields, so hydration never asks about them. + * + * What is left is exactly the residual the reap guard's sweep-time + * re-verification exists for and names: hook races, direct-driver writes, and + * future trash restore. Every fixture below is a row in that state. + * + * ## What is pinned — the PAIR and its counter-direction, never one side + * + * Asserting only "the held tombstone now hydrates" would score green for an + * implementation that hydrates everything, which would be a far worse defect + * than the one being fixed. So each case asks BOTH surfaces of ONE shared + * fixture, and the two directions are pinned together: + * + * - tombstoned + a live holder ⇒ download serves it AND hydration enriches it; + * - tombstoned + nothing holding it ⇒ download 404s AND hydration keeps the + * bare id, exactly as before this change; + * - `pending` ⇒ unchanged on both surfaces (only the `deleted` limb moved). + * + * Both limbs of the holder union get their own case, because `findFileHolder` + * is a deliberate union — `sys_attachment` join rows OR the `ref_*` ownership + * columns — and a hydration side that re-derived a narrower question would + * hide files the sweep refuses to reap: the same defect, one limb over. + * + * ## Why ONE engine and a real driver + * + * The download verdict and the hydration verdict must come from the SAME rows + * or the pair proves nothing, so both surfaces are driven off one real + * `ObjectQL` engine over sqlite `:memory:` — the project's ruled test backend + * (#5499 froze investment in the in-memory driver; #5704 migrated the test + * backends), and a real driver that really filters, including the `$in` the + * batched holder check issues. Nothing here turns on a driver's distinct + * semantics: what the fixture needs is shared state across two read surfaces, + * which sqlite carries identically. No engine double is involved, so no + * write-verb dispatch contract applies. + * + * The seam is reached by DUCK TYPING, exactly as `StorageServicePlugin` wires + * it. That is deliberate: it lets this file state the divergence as a + * behavioural red on a tree where the seam does not exist yet, instead of + * failing at import and measuring nothing. + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { promises as fs } from 'node:fs'; +import { join } from 'node:path'; +import { tmpdir } from 'node:os'; +import type { IHttpResponse, RouteHandler } from '@objectstack/spec/contracts'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { LocalStorageAdapter } from './local-storage-adapter.js'; +import { StorageMetadataStore } from './metadata-store.js'; +import { registerStorageRoutes } from './storage-routes.js'; +// Namespace import on purpose — a missing named export reads as `undefined` +// here rather than exploding at module load, which is what keeps the pre-fix +// run a MEASUREMENT of the divergence instead of an import crash. +import * as lifecycle from './attachment-lifecycle.js'; + +const URL_ROUTE = '/api/v1/storage/files/:fileId/url'; +const RAW_ROUTE = '/api/v1/storage/files/:fileId'; + +/** Tombstoned, held through the `ref_*` ownership columns (field-owner limb). */ +const HELD_BY_COLUMNS = 'f_heldByColumns_7kQ2'; +/** Tombstoned, held through a live `sys_attachment` join row (attachment limb). */ +const HELD_BY_JOIN = 'f_heldByJoinRow_4mZ8'; +/** Tombstoned and genuinely unheld — the counter-direction control. */ +const UNHELD = 'f_unheld_9tRw3'; +/** Never completed. Only the `deleted` limb moved; this pins that. */ +const PENDING = 'f_pending_2bXy'; + +const silentLogger = () => ({ + info: vi.fn(), warn: vi.fn(), debug: vi.fn(), error: vi.fn(), + trace: vi.fn(), fatal: vi.fn(), child() { return this; }, +}); + +function createMockHttpServer() { + const routes = new Map(); + const put = (m: string) => vi.fn((path: string, handler: RouteHandler) => { routes.set(`${m}:${path}`, handler); }); + return { + get: put('GET'), post: put('POST'), put: put('PUT'), + delete: vi.fn(), patch: vi.fn(), use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + _getHandler(method: string, path: string) { return routes.get(`${method}:${path}`); }, + }; +} + +function createMockRes(): IHttpResponse & { _status: number; _json: any; _headers: Record } { + const res: any = { + _status: 200, _json: null, _headers: {}, + json(data: any) { res._json = data; }, + send(data: any) { res._sent = data; }, + status(code: number) { res._status = code; return res; }, + header(name: string, value: string) { res._headers[name] = value; return res; }, + }; + return res; +} + +const tombstone = (id: string, extra: Record = {}) => ({ + id, + key: `files/${id}.bin`, + name: 'signed.pdf', + size: 2048, + mime_type: 'application/pdf', + scope: 'field', + acl: 'private', + status: 'deleted', + deleted_at: new Date().toISOString(), + ...extra, +}); + +/** The enriched form `resolveFileReferences` owes a held file. */ +const hydrated = (id: string) => ({ + id, + name: 'signed.pdf', + size: 2048, + mimeType: 'application/pdf', + url: `/api/v1/storage/files/${id}`, +}); + +describe('#11427 — file-field hydration and the download path agree about one sys_file row', () => { + let rootDir: string; + let adapter: LocalStorageAdapter; + let engine: ObjectQL; + let server: ReturnType; + + beforeEach(async () => { + rootDir = join(tmpdir(), `os-11427-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await fs.mkdir(rootDir, { recursive: true }); + adapter = new LocalStorageAdapter({ rootDir, signingSecret: 'test-secret' }); + + engine = new ObjectQL({ logger: silentLogger() } as any); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }) as any, true); + await engine.init(); + + engine.registry.registerObject({ + name: 'sys_file', + fields: { + name: { type: 'text' }, size: { type: 'number' }, mime_type: { type: 'text' }, + key: { type: 'text' }, scope: { type: 'text' }, acl: { type: 'text' }, + status: { type: 'text' }, deleted_at: { type: 'datetime' }, + ref_object: { type: 'text' }, ref_id: { type: 'text' }, ref_field: { type: 'text' }, + }, + } as any, 'test-11427'); + engine.registry.registerObject({ + name: 'sys_attachment', + fields: { + file_id: { type: 'text' }, parent_object: { type: 'text' }, parent_id: { type: 'text' }, + }, + } as any, 'test-11427'); + engine.registry.registerObject({ + name: 'contract', + fields: { title: { type: 'text' }, signed_pdf: { type: 'file' } }, + } as any, 'test-11427'); + // Real DDL for all three tables before the first row is written. + await engine.syncSchemas(); + + // ── The shared fixture: four sys_file rows in the residual state ────── + const files = [ + tombstone(HELD_BY_COLUMNS, { ref_object: 'contract', ref_id: 'c1', ref_field: 'signed_pdf' }), + tombstone(HELD_BY_JOIN), + tombstone(UNHELD), + tombstone(PENDING, { status: 'pending', deleted_at: null }), + ]; + for (const f of files) { + await engine.insert('sys_file', f as any); + await adapter.upload(String(f.key), Buffer.from(`bytes of ${f.id}`)); + } + // Only HELD_BY_JOIN carries a join row. + await engine.insert('sys_attachment', { + id: 'att-1', file_id: HELD_BY_JOIN, parent_object: 'project', parent_id: 'p1', + } as any); + + // One record per file, so hydration is asked about every case in ONE read. + await engine.insert('contract', { id: 'c1', title: 'MSA', signed_pdf: HELD_BY_COLUMNS } as any); + await engine.insert('contract', { id: 'c2', title: 'NDA', signed_pdf: HELD_BY_JOIN } as any); + await engine.insert('contract', { id: 'c3', title: 'SOW', signed_pdf: UNHELD } as any); + await engine.insert('contract', { id: 'c4', title: 'DPA', signed_pdf: PENDING } as any); + + // ── The two read surfaces, both over the rows just seeded ───────────── + server = createMockHttpServer(); + registerStorageRoutes(server as any, adapter, new StorageMetadataStore(engine as any), { + basePath: '/api/v1/storage', + resolveFileHolder: (f: any) => (lifecycle as any).findFileHolder(engine as any, f.id, f), + }); + + // The hydration seam, wired exactly as StorageServicePlugin wires it. + (engine as any).registerHeldFileResolver?.( + (rows: any[]) => (lifecycle as any).findHeldFiles(engine as any, rows), + ); + }); + + afterEach(async () => { + // Closes the `:memory:` database with the engine that owns it. + try { await engine?.destroy(); } catch { /* already torn down */ } + if (rootDir) await fs.rm(rootDir, { recursive: true, force: true }); + }); + + /** What the download path says about one file id. */ + const download = async (fileId: string) => { + const url = createMockRes(); + await server._getHandler('GET', URL_ROUTE)!( + { params: { fileId }, query: {}, headers: {}, method: 'GET', path: URL_ROUTE } as any, url); + const raw = createMockRes(); + await server._getHandler('GET', RAW_ROUTE)!( + { params: { fileId }, query: {}, headers: {}, method: 'GET', path: RAW_ROUTE } as any, raw); + return { url, raw }; + }; + + /** What the record read says about the same file id — by record IDENTITY. */ + const hydrationOf = async (recordId: string) => { + const rows = await engine.find('contract', { where: { id: recordId } }); + return rows[0]?.signed_pdf; + }; + + it('the seam and its ONE batched implementation both exist', () => { + // Hydration cannot ask the holder question without a seam to ask through, + // and the answer must come from the storage package's single definition — + // never a copy re-derived inside the engine. + expect(typeof (engine as any).registerHeldFileResolver).toBe('function'); + expect(typeof (lifecycle as any).findHeldFiles).toBe('function'); + }); + + // ── The pair: a held tombstone, both limbs of the holder union ────────── + + it('held through the ref_* columns — download and hydration AGREE it is there', async () => { + const { url, raw } = await download(HELD_BY_COLUMNS); + + // The download path, post-#10246. + expect(url._status).toBe(200); + expect(url._json.data.url).toContain('/_local/raw/'); + expect(raw._status).toBe(302); + + // …and the record read must say the same thing about the same row. + expect(await hydrationOf('c1')).toEqual(hydrated(HELD_BY_COLUMNS)); + }); + + it('held through a sys_attachment join row — download and hydration AGREE it is there', async () => { + const { url, raw } = await download(HELD_BY_JOIN); + + expect(url._status).toBe(200); + expect(raw._status).toBe(302); + + // The union's other limb. A hydration side that only read `ref_*` would + // hide this row while the download path serves it — the same divergence, + // one limb over. + expect(await hydrationOf('c2')).toEqual(hydrated(HELD_BY_JOIN)); + }); + + // ── The counter-direction: nothing holds it, so BOTH surfaces hide it ─── + + it('a genuinely unheld tombstone stays absent on BOTH surfaces', async () => { + const { url, raw } = await download(UNHELD); + + expect(url._status).toBe(404); + expect(url._json?.error?.code).toBe('FILE_NOT_FOUND'); + expect(raw._status).toBe(404); + expect(raw._json?.error?.code).toBe('FILE_NOT_FOUND'); + + // The control that a widening cannot fake: still the BARE ID, by identity. + expect(await hydrationOf('c3')).toBe(UNHELD); + }); + + it('a pending upload stays absent on BOTH surfaces — only the deleted limb moved', async () => { + const { url, raw } = await download(PENDING); + + expect(url._status).toBe(404); + expect(raw._status).toBe(404); + expect(await hydrationOf('c4')).toBe(PENDING); + }); + + // ── One read, every case at once: the agreement is per-row, not per-read ─ + + it('one read carrying all four files enriches exactly the held two, by identity', async () => { + const rows = await engine.find('contract'); + const byId = new Map(rows.map((r: any) => [r.id, r.signed_pdf])); + + expect(byId.get('c1')).toEqual(hydrated(HELD_BY_COLUMNS)); + expect(byId.get('c2')).toEqual(hydrated(HELD_BY_JOIN)); + expect(byId.get('c3')).toBe(UNHELD); + expect(byId.get('c4')).toBe(PENDING); + }); +}); + +/** + * The batched question and the single-file question must be the SAME question. + * + * `findHeldFiles` exists only because asking `findFileHolder` per row would be + * N queries per read. The moment the two disagree, the divergence #11427 fixes + * reopens one layer down — hydration and the download path would once again be + * reading one row through two predicates. So the equivalence is pinned over a + * matrix that exercises both limbs and both of their absences, rather than + * asserted in a comment. + */ +describe('#11427 — findHeldFiles is findFileHolder, batched', () => { + /** A fake that honours the `$in` the batch form issues, and equality for the single form. */ + const engineOver = (joinRows: Array>) => ({ + async find(_object: string, options: any) { + const cond = options?.where?.file_id; + const wanted = cond && typeof cond === 'object' && '$in' in cond + ? (cond.$in as string[]).map(String) + : [String(cond)]; + const hit = joinRows.filter((r) => wanted.includes(String(r.file_id))); + return typeof options?.limit === 'number' ? hit.slice(0, options.limit) : hit; + }, + }) as any; + + const MATRIX: Array<{ label: string; row: Record; joined: boolean }> = [ + { label: 'both limbs', row: { id: 'f1', ref_object: 'p', ref_id: 'r' }, joined: true }, + { label: 'columns only', row: { id: 'f2', ref_object: 'p', ref_id: 'r' }, joined: false }, + { label: 'join row only', row: { id: 'f3' }, joined: true }, + { label: 'neither', row: { id: 'f4' }, joined: false }, + { label: 'an EMPTY ref_id is not an owner', row: { id: 'f5', ref_object: 'p', ref_id: '' }, joined: false }, + ]; + + it('agrees with findFileHolder on every combination, asked in ONE batch', async () => { + const joinRows = MATRIX.filter((c) => c.joined).map((c) => ({ id: `att-${c.row.id}`, file_id: c.row.id })); + const engine = engineOver(joinRows); + + const held = await (lifecycle as any).findHeldFiles(engine, MATRIX.map((c) => c.row)); + + for (const { label, row } of MATRIX) { + const single = await (lifecycle as any).findFileHolder(engine, row.id, row); + expect(held.has(String(row.id)), label).toBe(single !== null); + } + // …and by identity, so a resolver that returned everything cannot pass. + expect([...held].sort()).toEqual(['f1', 'f2', 'f3']); + }); + + it('asks NOTHING when every row is settled by the ownership columns', async () => { + const find = vi.fn(async (_object: string, _options: any) => [] as any[]); + const held = await (lifecycle as any).findHeldFiles({ find } as any, [ + { id: 'f1', ref_object: 'p', ref_id: 'r1' }, + { id: 'f2', ref_object: 'p', ref_id: 'r2' }, + ]); + + // The free limb settles both, so the join-row read is never issued: an + // ordinary read pays nothing for this feature. + expect(find).not.toHaveBeenCalled(); + expect([...held].sort()).toEqual(['f1', 'f2']); + }); + + it('asks ONCE for a whole batch, however many files are unsettled', async () => { + const find = vi.fn(async (_object: string, _options: any) => [{ id: 'att-1', file_id: 'f2' }]); + const held = await (lifecycle as any).findHeldFiles({ find } as any, [ + { id: 'f1' }, { id: 'f2' }, { id: 'f3' }, { id: 'f4' }, + ]); + + // ONE query, not one per file — the whole point of the batched form. + expect(find).toHaveBeenCalledTimes(1); + expect(find.mock.calls[0][1].where.file_id.$in).toEqual(['f1', 'f2', 'f3', 'f4']); + expect([...held]).toEqual(['f2']); + }); +}); diff --git a/packages/services/service-storage/vitest.config.ts b/packages/services/service-storage/vitest.config.ts index efd6cade49..ec37aa0bf9 100644 --- a/packages/services/service-storage/vitest.config.ts +++ b/packages/services/service-storage/vitest.config.ts @@ -42,6 +42,25 @@ export default defineConfig({ // `@objectstack/core/logger` and resolve it to `core/src/index.ts/logger` // (ENOTDIR). Same reasoning, and same shape, as `service-knowledge`'s // config. - alias: [{ find: /^@objectstack\/core$/, replacement: path.resolve(__dirname, '../../core/src/index.ts') }], + // `@objectstack/driver-sql` joins it for the same reason (#11427): the + // hydration/download agreement pin drives a REAL engine over a real driver + // — sqlite `:memory:`, the project's ruled test backend (#5499 froze + // investment in the in-memory driver, #5704 migrated the test backends) — + // and a driver read from `dist/` would make that pin a verdict about build + // state rather than about the source beside it. + // + // `@objectstack/objectql` is deliberately NOT aliased here — it is a + // registered entry in `KNOWN_UNALIASED_TEST_IMPORTS` + // (`scripts/check-test-source-alias.mjs`), so this package's tests resolve + // the engine through its `exports` to `dist/`. That is load-bearing rather + // than incidental: it is what lets an ablation of engine source, rebuilt, + // actually change what these tests observe. + // + // Anchored patterns in array form, per the note above: a bare key whose + // replacement is a FILE swallows the package's subpaths. + alias: [ + { find: /^@objectstack\/core$/, replacement: path.resolve(__dirname, '../../core/src/index.ts') }, + { find: /^@objectstack\/driver-sql$/, replacement: path.resolve(__dirname, '../../drivers/driver-sql/src/index.ts') }, + ], }, }); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index acab809f60..63874b16ec 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2595,6 +2595,9 @@ importers: specifier: workspace:* version: link:../../types devDependencies: + '@objectstack/driver-sql': + specifier: workspace:* + version: link:../../drivers/driver-sql '@objectstack/objectql': specifier: workspace:* version: link:../../objectql