From 890b046e3989ae1e9339807d90ebe82241962489 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eder=20S=C3=A1nchez?= Date: Fri, 31 Jul 2026 09:22:36 -0600 Subject: [PATCH 1/4] fix: index extracted Portable Text prose in FTS, not raw JSON MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FTS5 tables were external-content (content='ec_'), which forces the index to mirror raw column values — and Portable Text fields store JSON, so structural tokens polluted the index (27-29% of it on an audited production database). Searching "normal" (a PT style value) matched 870/906 posts, "_type" matched every document, and snippets showed JSON fragments. Rebuild the FTS tables as self-contained FTS5 whose Portable Text columns hold extracted prose: every JSON string under a text, alt, caption, or code key (span text, image alt/caption, code blocks — the same semantics as extractPlainText). Extraction lives in SQL (json_tree) because the sync triggers cannot call into JS, with json_valid guarding legacy bare-string rows. Self-contained tables also retire the external-content 'delete' choreography and its corruption modes (migration 039's subject): removal is a plain DELETE, a harmless no-op for never-indexed rows, and INSERT OR REPLACE makes concurrent D1 populates converge. Migration 055 rebuilds every search-enabled collection's index and triggers on upgrade; the trigger SQL is lock-step with FTSManager per 039's precedent. The search query layer is unchanged — it joins ec_* by id for metadata, and snippet() now reads the stored prose. Co-Authored-By: Claude Fable 5 --- .changeset/fts-plain-text.md | 5 + .../database/migrations/056_fts_plain_text.ts | 230 ++++++++++++++++++ .../core/src/database/migrations/runner.ts | 2 + packages/core/src/search/fts-manager.ts | 171 +++++++------ .../integration/database/migrations.test.ts | 1 + .../search/portable-text-indexing.test.ts | 112 +++++++++ .../migrations/039_fix_fts5_triggers.test.ts | 68 ++++-- .../migrations/056_fts_plain_text.test.ts | 157 ++++++++++++ 8 files changed, 656 insertions(+), 90 deletions(-) create mode 100644 .changeset/fts-plain-text.md create mode 100644 packages/core/src/database/migrations/056_fts_plain_text.ts create mode 100644 packages/core/tests/integration/search/portable-text-indexing.test.ts create mode 100644 packages/core/tests/unit/database/migrations/056_fts_plain_text.test.ts diff --git a/.changeset/fts-plain-text.md b/.changeset/fts-plain-text.md new file mode 100644 index 0000000000..54d71fcb81 --- /dev/null +++ b/.changeset/fts-plain-text.md @@ -0,0 +1,5 @@ +--- +"emdash": patch +--- + +Fixes full-text search matching Portable Text's internal JSON instead of just prose. Searches for structural tokens like "normal", "span", or "block" no longer match documents whose visible text doesn't contain them, and search snippets show prose instead of JSON fragments. Existing search indexes are rebuilt automatically by a migration on upgrade — no manual reindex needed. diff --git a/packages/core/src/database/migrations/056_fts_plain_text.ts b/packages/core/src/database/migrations/056_fts_plain_text.ts new file mode 100644 index 0000000000..6b353ad7ed --- /dev/null +++ b/packages/core/src/database/migrations/056_fts_plain_text.ts @@ -0,0 +1,230 @@ +import type { Kysely } from "kysely"; +import { sql } from "kysely"; + +import { SEARCH_TOKENIZERS } from "../../search/types.js"; +import { isSqlite } from "../dialect-helpers.js"; +import { validateIdentifier } from "../validate.js"; + +/** + * Migration: Rebuild FTS5 indexes as self-contained tables indexing + * extracted Portable Text prose + * + * Background: FTS tables were external-content (`content='ec_'`), + * which forces the index to mirror the raw column values — and Portable + * Text fields store JSON, so the index was polluted with structural tokens + * (`_type`, `span`, style values like `normal`, `_key` ULIDs). Searches for + * those tokens matched nearly every document and snippets showed JSON + * fragments. + * + * The fix rebuilds each search-enabled collection's FTS table as a + * self-contained FTS5 table (no `content=` option) whose Portable Text + * columns hold extracted prose — every JSON string under a `text`, `alt`, + * `caption`, or `code` key — with sync triggers computing the same + * extraction in SQL. Self-contained tables also retire the external-content + * `'delete'` choreography and its corruption modes (see migration 039). + * The rebuilt table keeps the collection's configured `tokenize` from + * search_config rather than resetting it to the default. + * + * The SQL emitted here MUST stay in lock-step with + * `FTSManager.createTriggers` / `createFtsTable` / `populateFromContent` in + * `src/search/fts-manager.ts`. If those change again, add a new migration + * rather than editing this one — migrations are forward-only. + * + * Postgres: no-op. FTS5 is SQLite-only. + * + * D1: idempotent at the granularity we care about (drop-then-create + + * repopulate with `INSERT OR REPLACE`, so concurrent migrators converge). + * A partial apply that drops the FTS table without recreating it is healed + * by the next `verifyAndRepairIndex` call at runtime. + */ + +interface CollectionRow { + slug: string; + search_config: string | null; +} + +interface FieldRow { + slug: string; + type: string; +} + +export async function up(db: Kysely): Promise { + if (!isSqlite(db)) return; + + const collections = await sql` + SELECT slug, search_config FROM _emdash_collections + WHERE search_config IS NOT NULL + `.execute(db); + + for (const collection of collections.rows) { + if (!isSearchEnabled(collection.search_config)) continue; + + // Defensive re-validation before raw SQL interpolation, mirroring 039. + try { + validateIdentifier(collection.slug, "collection slug"); + } catch (error) { + console.warn( + `[migration 056] skipping FTS rebuild for collection "${collection.slug}": ${ + error instanceof Error ? error.message : String(error) + }`, + ); + continue; + } + + const fields = await getSearchableFields(db, collection.slug); + if (fields.length === 0) continue; + + await rebuildIndex(db, collection.slug, fields, searchTokenizer(collection.search_config)); + } +} + +/** + * Forward-only. Down is a no-op: the FTS tables are managed by FTSManager + * at runtime and the self-contained shape remains fully functional for + * older code paths that only MATCH and join on id. + */ +export async function down(_db: Kysely): Promise { + // no-op +} + +function isSearchEnabled(searchConfig: string | null): boolean { + if (!searchConfig) return false; + try { + const parsed: unknown = JSON.parse(searchConfig); + return ( + typeof parsed === "object" && + parsed !== null && + "enabled" in parsed && + parsed.enabled === true + ); + } catch { + return false; + } +} + +/** + * Tokenizer for the rebuilt table, from the collection's search_config. + * Values outside the allowlist (or unparsable config) fall back to the + * default rather than reaching the raw CREATE VIRTUAL TABLE statement. + */ +function searchTokenizer(searchConfig: string | null): string { + if (!searchConfig) return "porter unicode61"; + try { + const parsed: unknown = JSON.parse(searchConfig); + if (typeof parsed === "object" && parsed !== null && "tokenize" in parsed) { + const configured = SEARCH_TOKENIZERS.find((tokenizer) => tokenizer === parsed.tokenize); + if (configured !== undefined) return configured; + } + } catch { + return "porter unicode61"; + } + return "porter unicode61"; +} + +async function getSearchableFields( + db: Kysely, + collectionSlug: string, +): Promise { + const rows = await sql` + SELECT f.slug, f.type FROM _emdash_fields f + INNER JOIN _emdash_collections c ON c.id = f.collection_id + WHERE c.slug = ${collectionSlug} AND f.searchable = 1 + `.execute(db); + + const out: FieldRow[] = []; + for (const row of rows.rows) { + try { + validateIdentifier(row.slug, "searchable field name"); + out.push(row); + } catch { + console.warn( + `[migration 056] skipping invalid searchable field "${row.slug}" on collection "${collectionSlug}"`, + ); + } + } + return out; +} + +/** Indexed-value expression for one field; lock-step with FTSManager.searchValueExpr. */ +function searchValueExpr(ref: string, fieldType: string): string { + if (fieldType !== "portableText") return ref; + return ( + `CASE WHEN ${ref} IS NULL THEN NULL ` + + `WHEN json_valid(${ref}) THEN (` + + `SELECT group_concat(j.value, ' ') FROM json_tree(${ref}) AS j ` + + `WHERE j.key IN ('text', 'alt', 'caption', 'code') AND j.type = 'text') ` + + `ELSE ${ref} END` + ); +} + +async function rebuildIndex( + db: Kysely, + collectionSlug: string, + fields: FieldRow[], + tokenizer: string, +): Promise { + const ftsTable = `_emdash_fts_${collectionSlug}`; + const contentTable = `ec_${collectionSlug}`; + const slugs = fields.map((f) => f.slug); + const columnList = ["id UNINDEXED", "locale UNINDEXED", ...slugs].join(", "); + const fieldList = slugs.join(", "); + const newValueList = fields.map((f) => searchValueExpr(`NEW.${f.slug}`, f.type)).join(", "); + const selectValueList = fields.map((f) => searchValueExpr(`"${f.slug}"`, f.type)).join(", "); + + await sql.raw(`DROP TRIGGER IF EXISTS "${ftsTable}_insert"`).execute(db); + await sql.raw(`DROP TRIGGER IF EXISTS "${ftsTable}_update"`).execute(db); + await sql.raw(`DROP TRIGGER IF EXISTS "${ftsTable}_delete"`).execute(db); + await sql.raw(`DROP TABLE IF EXISTS "${ftsTable}"`).execute(db); + + await sql + .raw(` + CREATE VIRTUAL TABLE IF NOT EXISTS "${ftsTable}" USING fts5( + ${columnList}, + tokenize='${tokenizer}' + ) + `) + .execute(db); + + await sql + .raw(` + CREATE TRIGGER IF NOT EXISTS "${ftsTable}_insert" + AFTER INSERT ON "${contentTable}" + WHEN NEW.deleted_at IS NULL + BEGIN + INSERT OR REPLACE INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) + VALUES (NEW.rowid, NEW.id, NEW.locale, ${newValueList}); + END + `) + .execute(db); + + await sql + .raw(` + CREATE TRIGGER IF NOT EXISTS "${ftsTable}_update" + AFTER UPDATE ON "${contentTable}" + BEGIN + DELETE FROM "${ftsTable}" WHERE rowid = OLD.rowid; + INSERT INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) + SELECT NEW.rowid, NEW.id, NEW.locale, ${newValueList} + WHERE NEW.deleted_at IS NULL; + END + `) + .execute(db); + + await sql + .raw(` + CREATE TRIGGER IF NOT EXISTS "${ftsTable}_delete" + AFTER DELETE ON "${contentTable}" + BEGIN + DELETE FROM "${ftsTable}" WHERE rowid = OLD.rowid; + END + `) + .execute(db); + + await sql + .raw(` + INSERT OR REPLACE INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) + SELECT rowid, id, locale, ${selectValueList} FROM "${contentTable}" + WHERE deleted_at IS NULL + `) + .execute(db); +} diff --git a/packages/core/src/database/migrations/runner.ts b/packages/core/src/database/migrations/runner.ts index 33b8eecfde..2d77fad461 100644 --- a/packages/core/src/database/migrations/runner.ts +++ b/packages/core/src/database/migrations/runner.ts @@ -58,6 +58,7 @@ import * as m052 from "./052_media_usage_read_index.js"; import * as m053 from "./053_plugin_mcp_tools.js"; import * as m054 from "./054_media_upload_attempts.js"; import * as m055 from "./055_content_translation_group_locale_index.js"; +import * as m056 from "./056_fts_plain_text.js"; const MIGRATIONS: Readonly> = Object.freeze({ "001_initial": m001, @@ -114,6 +115,7 @@ const MIGRATIONS: Readonly> = Object.freeze({ "053_plugin_mcp_tools": m053, "054_media_upload_attempts": m054, "055_content_translation_group_locale_index": m055, + "056_fts_plain_text": m056, }); /** Total number of registered migrations. Exported for use in tests. */ diff --git a/packages/core/src/search/fts-manager.ts b/packages/core/src/search/fts-manager.ts index d13dbc5de3..a39f2118c2 100644 --- a/packages/core/src/search/fts-manager.ts +++ b/packages/core/src/search/fts-manager.ts @@ -93,25 +93,23 @@ export class FTSManager { if (!isSqlite(this.db)) return; this.validateInputs(collectionSlug, searchableFields); const ftsTable = this.getFtsTableName(collectionSlug); - const contentTable = this.getContentTableName(collectionSlug); // Build the column list for FTS5 // id and locale are UNINDEXED (used for joining/filtering, not searched) const columns = ["id UNINDEXED", "locale UNINDEXED", ...searchableFields].join(", "); - // Create the FTS5 virtual table. - // `content=''` makes this an *external content* FTS5 table: - // the inverted index lives in the FTS shadow tables, but the actual - // row data lives in the backing content table. The triggers in - // `createTriggers` keep the index in sync; they MUST use the - // external-content-safe `'delete'` command (see notes there) to - // avoid `SQLITE_CORRUPT_VTAB` on UPDATE/DELETE. + // Create the FTS5 virtual table. The table stores its own copy of the + // indexed values (no `content=` option): Portable Text fields are + // indexed as extracted plain text — see searchValueExpr — which cannot + // mirror the raw JSON in the ec_* column, and external-content FTS5 + // requires the index to exactly mirror the backing table's values + // (snippet() reads them, and the 'delete' command must be fed the + // inserted values or the index corrupts — see migration 039's history). + // Storing the extracted text also makes snippet() return prose. await sql .raw(` CREATE VIRTUAL TABLE IF NOT EXISTS "${ftsTable}" USING fts5( ${columns}, - content='${contentTable}', - content_rowid='rowid', tokenize='${tokenizer}' ) `) @@ -121,6 +119,51 @@ export class FTSManager { await this.createTriggers(collectionSlug, searchableFields); } + /** + * SQL expression producing the indexed value for one searchable field. + * + * Portable Text fields are stored as JSON; indexing the raw JSON pollutes + * the index with structural tokens (`_type`, style values like `normal`, + * `_key` ULIDs) and makes snippets show JSON fragments. Extract the prose + * instead: every JSON string under a `text`, `alt`, `caption`, or `code` + * key (span text, image alt/caption, code blocks — mirroring + * `extractPlainText` in text-extraction.ts). `json_valid` guards legacy + * rows holding a bare string, which is indexed as-is; extraction must live + * in SQL because the sync triggers cannot call into JS. + * + * `ref` must be a validated column reference (`NEW.x`, `OLD.x`, `"x"`). + */ + private searchValueExpr(ref: string, fieldType: string | undefined): string { + if (fieldType !== "portableText") return ref; + return ( + `CASE WHEN ${ref} IS NULL THEN NULL ` + + `WHEN json_valid(${ref}) THEN (` + + `SELECT group_concat(j.value, ' ') FROM json_tree(${ref}) AS j ` + + `WHERE j.key IN ('text', 'alt', 'caption', 'code') AND j.type = 'text') ` + + `ELSE ${ref} END` + ); + } + + /** + * Field type per slug for a collection, for choosing the indexed-value + * expression. Fields missing from the schema fall back to raw indexing. + */ + private async getFieldTypes(collectionSlug: string): Promise> { + const collection = await this.db + .selectFrom("_emdash_collections") + .select("id") + .where("slug", "=", collectionSlug) + .executeTakeFirst(); + if (!collection) return new Map(); + + const rows = await this.db + .selectFrom("_emdash_fields") + .select(["slug", "type"]) + .where("collection_id", "=", collection.id) + .execute(); + return new Map(rows.map((r) => [r.slug, r.type])); + } + /** * Create triggers to keep FTS table in sync with content table. * @@ -129,31 +172,20 @@ export class FTSManager { * search index and ensures the FTS row count matches the non-deleted * content count (which `verifyAndRepairIndex` relies on). * - * IMPORTANT: The FTS5 virtual table is created with `content='ec_'` - * which makes it an *external content* FTS5 table. For external-content - * tables, removing a row must use the documented `'delete'` command and - * supply the OLD column values explicitly, e.g.: + * The FTS table stores its own values (no `content=` option), so removal + * is a plain `DELETE FROM fts WHERE rowid = OLD.rowid` — a harmless no-op + * for rows that were never indexed (soft-deleted content). The + * external-content `'delete'`-command choreography and its corruption + * modes (migration 039) do not apply to self-contained tables. * - * INSERT INTO fts(fts, rowid, col1, col2) - * VALUES('delete', OLD.rowid, OLD.col1, OLD.col2); + * `INSERT OR REPLACE` keeps the insert path idempotent: re-running a + * populate (D1 has no migration lock, so two isolates can race) converges + * on one index row per content row instead of failing on the rowid + * constraint. * - * Using `DELETE FROM fts WHERE rowid = OLD.rowid` is the correct form - * for *contentless* tables but is unsafe for external-content tables: - * FTS5 then reads column values from the backing content table, which - * in an AFTER UPDATE trigger already holds the NEW values. The wrong - * tokens get removed and the inverted index drifts out of sync until - * SQLite raises `SQLITE_CORRUPT_VTAB` on the next mutation. See - * https://www.sqlite.org/fts5.html#external_content_tables. - * - * The UPDATE and DELETE triggers gate the `'delete'` on - * `OLD.deleted_at IS NULL` because the INSERT trigger never indexed - * rows that were already soft-deleted. Issuing `'delete'` for a rowid - * that was never inserted into the FTS index is itself a corruption - * trigger -- FTS5's `'delete'` is not a no-op on missing rowids and - * raises `SQLITE_CORRUPT_VTAB`. Affected paths include restore-from- - * trash (UPDATE where `OLD.deleted_at IS NOT NULL`), permanent-delete - * from trash (DELETE on a soft-deleted row), and any edit on a row - * that's currently in the trash. + * The trigger SQL emitted here MUST stay in lock-step with migration + * `056_fts_plain_text.ts`. If this changes again, add a new migration + * rather than editing that one — migrations are forward-only. */ private async createTriggers(collectionSlug: string, searchableFields: string[]): Promise { this.validateInputs(collectionSlug, searchableFields); @@ -165,64 +197,48 @@ export class FTSManager { } const ftsTable = this.getFtsTableName(collectionSlug); const contentTable = this.getContentTableName(collectionSlug); + const fieldTypes = await this.getFieldTypes(collectionSlug); const fieldList = searchableFields.join(", "); - const newFieldList = searchableFields.map((f) => `NEW.${f}`).join(", "); - // `'delete'` takes the FTS5 virtual table name as the first column, - // then the rowid being removed, then the OLD value of every column - // declared on the FTS5 table (in declaration order: id, locale, - // then each searchable field). - const oldFieldList = searchableFields.map((f) => `OLD.${f}`).join(", "); + const newValueList = searchableFields + .map((f) => this.searchValueExpr(`NEW.${f}`, fieldTypes.get(f))) + .join(", "); // Insert trigger - only index non-deleted content await sql .raw(` - CREATE TRIGGER IF NOT EXISTS "${ftsTable}_insert" - AFTER INSERT ON "${contentTable}" + CREATE TRIGGER IF NOT EXISTS "${ftsTable}_insert" + AFTER INSERT ON "${contentTable}" WHEN NEW.deleted_at IS NULL BEGIN - INSERT INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) - VALUES (NEW.rowid, NEW.id, NEW.locale, ${newFieldList}); + INSERT OR REPLACE INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) + VALUES (NEW.rowid, NEW.id, NEW.locale, ${newValueList}); END `) .execute(this.db); - // Update trigger - remove the old row from the FTS index using the - // external-content-safe `'delete'` command (which uses OLD column - // values, captured before the row was modified), then re-insert - // the new values when the row is still visible. - // - // `'delete'` is gated on `OLD.deleted_at IS NULL` because rows that - // were soft-deleted are not in the FTS index (the INSERT trigger - // skips them). Issuing `'delete'` for a missing rowid raises - // `SQLITE_CORRUPT_VTAB`, which would break restore-from-trash and - // edits to soft-deleted rows. + // Update trigger - drop the old index row, re-insert when the row is + // still visible. Trash (deleted_at set) ends at DELETE only; restore + // ends at DELETE (no-op) + re-insert. await sql .raw(` - CREATE TRIGGER IF NOT EXISTS "${ftsTable}_update" - AFTER UPDATE ON "${contentTable}" + CREATE TRIGGER IF NOT EXISTS "${ftsTable}_update" + AFTER UPDATE ON "${contentTable}" BEGIN - INSERT INTO "${ftsTable}"("${ftsTable}", rowid, id, locale, ${fieldList}) - SELECT 'delete', OLD.rowid, OLD.id, OLD.locale, ${oldFieldList} - WHERE OLD.deleted_at IS NULL; + DELETE FROM "${ftsTable}" WHERE rowid = OLD.rowid; INSERT INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) - SELECT NEW.rowid, NEW.id, NEW.locale, ${newFieldList} + SELECT NEW.rowid, NEW.id, NEW.locale, ${newValueList} WHERE NEW.deleted_at IS NULL; END `) .execute(this.db); - // Delete trigger - same external-content-safe `'delete'` form, - // gated on `OLD.deleted_at IS NULL` for the same reason as the - // UPDATE trigger: permanent-delete from trash hits a row whose - // `deleted_at` is already set and which was never indexed. + // Delete trigger await sql .raw(` - CREATE TRIGGER IF NOT EXISTS "${ftsTable}_delete" - AFTER DELETE ON "${contentTable}" + CREATE TRIGGER IF NOT EXISTS "${ftsTable}_delete" + AFTER DELETE ON "${contentTable}" BEGIN - INSERT INTO "${ftsTable}"("${ftsTable}", rowid, id, locale, ${fieldList}) - SELECT 'delete', OLD.rowid, OLD.id, OLD.locale, ${oldFieldList} - WHERE OLD.deleted_at IS NULL; + DELETE FROM "${ftsTable}" WHERE rowid = OLD.rowid; END `) .execute(this.db); @@ -279,20 +295,27 @@ export class FTSManager { } /** - * Populate the FTS table from existing content + * Populate the FTS table from existing content. + * + * `INSERT OR REPLACE` so a concurrent double-populate (D1 has no + * migration lock) converges instead of failing on the rowid constraint. */ async populateFromContent(collectionSlug: string, searchableFields: string[]): Promise { if (!isSqlite(this.db)) return; this.validateInputs(collectionSlug, searchableFields); const ftsTable = this.getFtsTableName(collectionSlug); const contentTable = this.getContentTableName(collectionSlug); + const fieldTypes = await this.getFieldTypes(collectionSlug); const fieldList = searchableFields.join(", "); + const valueList = searchableFields + .map((f) => this.searchValueExpr(`"${f}"`, fieldTypes.get(f))) + .join(", "); // Insert all existing content into FTS table await sql .raw(` - INSERT INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) - SELECT rowid, id, locale, ${fieldList} FROM "${contentTable}" + INSERT OR REPLACE INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) + SELECT rowid, id, locale, ${valueList} FROM "${contentTable}" WHERE deleted_at IS NULL `) .execute(this.db); @@ -534,10 +557,8 @@ export class FTSManager { return true; } - // Row count parity check. For external-content FTS tables, COUNT(*) - // on the virtual table is answered from the backing content table - // (including soft-deleted rows), so we use the docsize shadow table - // which tracks rows actually present in the full-text index. + // Row count parity check against the docsize shadow table, which + // tracks rows actually present in the full-text index. const contentCount = await sql<{ count: number }>` SELECT COUNT(*) as count FROM ${sql.ref(contentTable)} WHERE deleted_at IS NULL diff --git a/packages/core/tests/integration/database/migrations.test.ts b/packages/core/tests/integration/database/migrations.test.ts index df9af0f17a..529c752279 100644 --- a/packages/core/tests/integration/database/migrations.test.ts +++ b/packages/core/tests/integration/database/migrations.test.ts @@ -142,6 +142,7 @@ describe("Database Migrations (Integration)", () => { "053_plugin_mcp_tools", "054_media_upload_attempts", "055_content_translation_group_locale_index", + "056_fts_plain_text", ]; await db.deleteFrom("_emdash_migrations").where("name", "in", trailing).execute(); diff --git a/packages/core/tests/integration/search/portable-text-indexing.test.ts b/packages/core/tests/integration/search/portable-text-indexing.test.ts new file mode 100644 index 0000000000..a2b977e936 --- /dev/null +++ b/packages/core/tests/integration/search/portable-text-indexing.test.ts @@ -0,0 +1,112 @@ +/** + * FTS indexes Portable Text prose, not its JSON structure. + * + * Portable Text fields are stored as JSON in the content table. Feeding that + * raw JSON to FTS5 pollutes the index with structural tokens — every post + * matches searches for "normal" (a style value), "span", or "markDefs", and + * snippets show JSON fragments instead of prose. The index must contain only + * extracted text: span text, image alt/caption, code content. + */ + +import type { Kysely } from "kysely"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { ContentRepository } from "../../../src/database/repositories/content.js"; +import type { Database } from "../../../src/database/types.js"; +import { SchemaRegistry } from "../../../src/schema/registry.js"; +import { FTSManager } from "../../../src/search/fts-manager.js"; +import { searchWithDb } from "../../../src/search/query.js"; +import { setupTestDatabase, teardownTestDatabase } from "../../utils/test-db.js"; + +describe("Portable Text FTS indexing", () => { + let db: Kysely; + let registry: SchemaRegistry; + let repo: ContentRepository; + let ftsManager: FTSManager; + + beforeEach(async () => { + db = await setupTestDatabase(); + registry = new SchemaRegistry(db); + repo = new ContentRepository(db); + ftsManager = new FTSManager(db); + + await registry.createCollection({ + slug: "pages", + label: "Pages", + labelSingular: "Page", + supports: ["drafts", "revisions", "search"], + }); + // content first: searchSingleCollection snippets the first searchable + // field (FTS column 2), and these tests assert content snippets. + await registry.createField("pages", { + slug: "content", + label: "Content", + type: "portableText", + searchable: true, + }); + await registry.createField("pages", { + slug: "title", + label: "Title", + type: "string", + required: true, + searchable: true, + }); + + await ftsManager.enableSearch("pages"); + + await repo.create({ + type: "pages", + slug: "haunted-cinema", + status: "published", + data: { + title: "Opening Night", + content: [ + { + _type: "block", + _key: "b1", + style: "normal", + markDefs: [], + children: [ + { _type: "span", _key: "s1", text: "The haunted cinema screens forbidden films." }, + ], + }, + { _type: "image", _key: "b2", alt: "festival poster", caption: "official artwork" }, + { _type: "code", _key: "b3", code: "SELECT midnight FROM screenings" }, + ], + }, + }); + }); + + afterEach(async () => { + await teardownTestDatabase(db); + }); + + it("does not match Portable Text structural tokens", async () => { + for (const structural of ["normal", "span", "markDefs", "block"]) { + const { items } = await searchWithDb(db, structural, { collections: ["pages"] }); + expect(items, `"${structural}" must not match`).toEqual([]); + } + }); + + it("matches prose inside spans", async () => { + const { items } = await searchWithDb(db, "haunted", { collections: ["pages"] }); + expect(items).toHaveLength(1); + expect(items[0]!.slug).toBe("haunted-cinema"); + }); + + it("matches image alt text, captions, and code content", async () => { + for (const term of ["poster", "artwork", "midnight"]) { + const { items } = await searchWithDb(db, term, { collections: ["pages"] }); + expect(items, `"${term}" must match`).toHaveLength(1); + } + }); + + it("returns prose snippets, not JSON fragments", async () => { + const { items } = await searchWithDb(db, "forbidden", { collections: ["pages"] }); + expect(items).toHaveLength(1); + const snippet = items[0]!.snippet ?? ""; + expect(snippet).toContain("forbidden"); + expect(snippet).not.toContain("_type"); + expect(snippet).not.toContain("{"); + }); +}); diff --git a/packages/core/tests/unit/database/migrations/039_fix_fts5_triggers.test.ts b/packages/core/tests/unit/database/migrations/039_fix_fts5_triggers.test.ts index 3496bb69a2..6d08f71537 100644 --- a/packages/core/tests/unit/database/migrations/039_fix_fts5_triggers.test.ts +++ b/packages/core/tests/unit/database/migrations/039_fix_fts5_triggers.test.ts @@ -74,6 +74,42 @@ describe("migration 039: rebuild FTS5 triggers", () => { .execute(db); } + /** + * Rebuild the collection's FTS table in the pre-fix *external-content* + * shape (`content='ec_'`) that every site running a pre-fix EmDash + * version actually had. The current FTSManager builds self-contained FTS + * tables — on those, the "broken" contentless-style triggers sync + * correctly, so the historical corruption can only be reproduced against + * the historical table shape. + */ + async function installExternalContentFts( + collectionSlug: string, + fields: string[], + ): Promise { + const ftsTable = `_emdash_fts_${collectionSlug}`; + const contentTable = `ec_${collectionSlug}`; + const fieldList = fields.join(", "); + + await sql.raw(`DROP TABLE IF EXISTS "${ftsTable}"`).execute(db); + await sql + .raw(` + CREATE VIRTUAL TABLE "${ftsTable}" USING fts5( + id UNINDEXED, locale UNINDEXED, ${fieldList}, + content='${contentTable}', + content_rowid='rowid', + tokenize='porter unicode61' + ) + `) + .execute(db); + await sql + .raw(` + INSERT INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) + SELECT rowid, id, locale, ${fieldList} FROM "${contentTable}" + WHERE deleted_at IS NULL + `) + .execute(db); + } + async function setupSearchEnabledPages(): Promise { await registry.createCollection({ slug: "pages", @@ -137,11 +173,12 @@ describe("migration 039: rebuild FTS5 triggers", () => { data: { title: "About", body: "Some searchable body text." }, }); - // Simulate the pre-fix state: broken triggers + a published row. - // The legacy triggers are functional on INSERT (the contentless and - // external-content forms agree there), so the row is in the index - // at this point. The migration must replace the triggers without - // losing that row. + // Simulate the pre-fix state: external-content FTS table + broken + // triggers + a published row. The legacy triggers are functional on + // INSERT (the contentless and external-content forms agree there), + // so the row is in the index at this point. The migration must + // replace the triggers without losing that row. + await installExternalContentFts("pages", ["title", "body"]); await installPreFixTriggers("pages", ["title", "body"]); await runMigration039(); @@ -187,16 +224,17 @@ describe("migration 039: rebuild FTS5 triggers", () => { data: { title: "Corrupt me", body: "Original aardvark body." }, }); - // Install the broken triggers and then *fire them* by issuing the - // kind of UPDATE the publish path does. The broken trigger's - // `DELETE FROM fts WHERE rowid = OLD.rowid` on an external-content - // table reads NEW values from the content table when removing - // tokens, so the OLD tokens are left behind in the inverted index - // even though the content table no longer holds them. The result - // is a stale-token leak: searches for words from the OLD body - // keep matching the (now updated) row, and segment metadata - // drifts out of sync until SQLite eventually surfaces it as - // SQLITE_CORRUPT_VTAB. + // Install the historical external-content table and broken triggers, + // then *fire them* by issuing the kind of UPDATE the publish path + // does. The broken trigger's `DELETE FROM fts WHERE rowid = OLD.rowid` + // on an external-content table reads NEW values from the content + // table when removing tokens, so the OLD tokens are left behind in + // the inverted index even though the content table no longer holds + // them. The result is a stale-token leak: searches for words from + // the OLD body keep matching the (now updated) row, and segment + // metadata drifts out of sync until SQLite eventually surfaces it + // as SQLITE_CORRUPT_VTAB. + await installExternalContentFts("pages", ["title", "body"]); await installPreFixTriggers("pages", ["title", "body"]); // Sanity check: the OLD content's unique token is indexed before diff --git a/packages/core/tests/unit/database/migrations/056_fts_plain_text.test.ts b/packages/core/tests/unit/database/migrations/056_fts_plain_text.test.ts new file mode 100644 index 0000000000..60d777d535 --- /dev/null +++ b/packages/core/tests/unit/database/migrations/056_fts_plain_text.test.ts @@ -0,0 +1,157 @@ +/** + * Migration 056 rebuilds FTS indexes as self-contained tables indexing + * extracted Portable Text prose. These tests exercise the migration against + * the pre-fix state a real upgrade hits: an external-content FTS table whose + * index holds raw Portable Text JSON, so structural tokens ("normal", + * "span") match documents whose prose never contains them. + */ + +import type { Kysely } from "kysely"; +import { sql } from "kysely"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { ContentRepository } from "../../../../src/database/repositories/content.js"; +import type { Database } from "../../../../src/database/types.js"; +import { SchemaRegistry } from "../../../../src/schema/registry.js"; +import { FTSManager } from "../../../../src/search/fts-manager.js"; +import { setupTestDatabase, teardownTestDatabase } from "../../../utils/test-db.js"; + +describe("migration 056: FTS indexes extracted Portable Text prose", () => { + let db: Kysely; + let registry: SchemaRegistry; + let repo: ContentRepository; + + beforeEach(async () => { + db = await setupTestDatabase(); + registry = new SchemaRegistry(db); + repo = new ContentRepository(db); + + await registry.createCollection({ + slug: "pages", + label: "Pages", + labelSingular: "Page", + supports: ["search"], + }); + await registry.createField("pages", { + slug: "content", + label: "Content", + type: "portableText", + searchable: true, + }); + await new FTSManager(db).enableSearch("pages"); + + await repo.create({ + type: "pages", + slug: "haunted", + status: "published", + data: { + content: [ + { + _type: "block", + _key: "b1", + style: "normal", + children: [{ _type: "span", _key: "s1", text: "The haunted cinema." }], + }, + ], + }, + }); + }); + + afterEach(async () => { + await teardownTestDatabase(db); + }); + + /** + * Rebuild the pages FTS table in the pre-056 shape: external-content + * FTS5 indexing the raw column values (Portable Text JSON included). + */ + async function installPreFixFts(tokenizer = "porter unicode61"): Promise { + await sql.raw(`DROP TRIGGER IF EXISTS "_emdash_fts_pages_insert"`).execute(db); + await sql.raw(`DROP TRIGGER IF EXISTS "_emdash_fts_pages_update"`).execute(db); + await sql.raw(`DROP TRIGGER IF EXISTS "_emdash_fts_pages_delete"`).execute(db); + await sql.raw(`DROP TABLE IF EXISTS "_emdash_fts_pages"`).execute(db); + await sql + .raw(` + CREATE VIRTUAL TABLE "_emdash_fts_pages" USING fts5( + id UNINDEXED, locale UNINDEXED, content, + content='ec_pages', + content_rowid='rowid', + tokenize='${tokenizer}' + ) + `) + .execute(db); + await sql + .raw(` + INSERT INTO "_emdash_fts_pages"(rowid, id, locale, content) + SELECT rowid, id, locale, content FROM "ec_pages" + WHERE deleted_at IS NULL + `) + .execute(db); + } + + async function matches(term: string): Promise { + const result = await sql<{ count: number }>` + SELECT COUNT(*) as count FROM "_emdash_fts_pages" + WHERE "_emdash_fts_pages" MATCH ${term} + `.execute(db); + return Number(result.rows[0]?.count ?? 0); + } + + async function runMigration056(): Promise { + const { up } = await import("../../../../src/database/migrations/056_fts_plain_text.js"); + await up(db as unknown as Kysely); + } + + it("replaces the JSON-polluted index with extracted prose", async () => { + await installPreFixFts(); + + // Pre-migration: structural tokens match — the pollution being fixed. + expect(await matches("normal")).toBe(1); + expect(await matches("span")).toBe(1); + + await runMigration056(); + + expect(await matches("normal")).toBe(0); + expect(await matches("span")).toBe(0); + expect(await matches("haunted")).toBe(1); + }); + + it("installs working sync triggers alongside the rebuilt index", async () => { + await installPreFixFts(); + await runMigration056(); + + const rows = await sql<{ id: string }>`SELECT id FROM ec_pages`.execute(db); + await repo.update("pages", rows.rows[0]!.id, { + data: { + content: [ + { + _type: "block", + _key: "b1", + style: "normal", + children: [{ _type: "span", _key: "s1", text: "A midnight screening." }], + }, + ], + }, + }); + + expect(await matches("midnight")).toBe(1); + expect(await matches("haunted")).toBe(0); + expect(await matches("normal")).toBe(0); + }); + + it("honors a configured non-default tokenizer when rebuilding", async () => { + await new FTSManager(db).enableSearch("pages", { tokenize: "trigram" }); + await installPreFixFts("trigram"); + + await runMigration056(); + + // trigram matches substrings; porter unicode61 would not match "aunted". + expect(await matches("aunted")).toBe(1); + expect(await matches("normal")).toBe(0); + }); + + it("is a no-op on databases with no search-enabled collections", async () => { + await new FTSManager(db).disableSearch("pages"); + await expect(runMigration056()).resolves.toBeUndefined(); + }); +}); From ba3168020ccf4c0f971da1fb49bbdbcf81c215c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eder=20S=C3=A1nchez?= Date: Fri, 31 Jul 2026 09:59:57 -0600 Subject: [PATCH 2/4] fix: qualify column references in the FTS extraction subquery json_tree exposes output columns named key/value/type/path and friends; a bare column reference inside the extraction subquery binds to those instead of the outer ec_* column, so populating a Portable Text field slugged with one of these names silently indexed NULL. Triggers were unaffected (NEW.-qualified). Qualify the populate and migration references with the content table name. Co-Authored-By: Claude Fable 5 --- .../database/migrations/056_fts_plain_text.ts | 6 ++- packages/core/src/search/fts-manager.ts | 5 +- .../search/portable-text-indexing.test.ts | 53 +++++++++++++++++++ 3 files changed, 62 insertions(+), 2 deletions(-) diff --git a/packages/core/src/database/migrations/056_fts_plain_text.ts b/packages/core/src/database/migrations/056_fts_plain_text.ts index 6b353ad7ed..07e623434e 100644 --- a/packages/core/src/database/migrations/056_fts_plain_text.ts +++ b/packages/core/src/database/migrations/056_fts_plain_text.ts @@ -169,7 +169,11 @@ async function rebuildIndex( const columnList = ["id UNINDEXED", "locale UNINDEXED", ...slugs].join(", "); const fieldList = slugs.join(", "); const newValueList = fields.map((f) => searchValueExpr(`NEW.${f.slug}`, f.type)).join(", "); - const selectValueList = fields.map((f) => searchValueExpr(`"${f.slug}"`, f.type)).join(", "); + // Table-qualified: a bare column reference inside the json_tree extraction + // subquery binds to json_tree's own key/value/type/... columns. + const selectValueList = fields + .map((f) => searchValueExpr(`"${contentTable}"."${f.slug}"`, f.type)) + .join(", "); await sql.raw(`DROP TRIGGER IF EXISTS "${ftsTable}_insert"`).execute(db); await sql.raw(`DROP TRIGGER IF EXISTS "${ftsTable}_update"`).execute(db); diff --git a/packages/core/src/search/fts-manager.ts b/packages/core/src/search/fts-manager.ts index a39f2118c2..c74a30454e 100644 --- a/packages/core/src/search/fts-manager.ts +++ b/packages/core/src/search/fts-manager.ts @@ -307,8 +307,11 @@ export class FTSManager { const contentTable = this.getContentTableName(collectionSlug); const fieldTypes = await this.getFieldTypes(collectionSlug); const fieldList = searchableFields.join(", "); + // Table-qualified references: json_tree exposes columns named + // key/value/type/path/..., and inside the extraction subquery a bare + // column reference binds to those instead of the ec_* column. const valueList = searchableFields - .map((f) => this.searchValueExpr(`"${f}"`, fieldTypes.get(f))) + .map((f) => this.searchValueExpr(`"${contentTable}"."${f}"`, fieldTypes.get(f))) .join(", "); // Insert all existing content into FTS table diff --git a/packages/core/tests/integration/search/portable-text-indexing.test.ts b/packages/core/tests/integration/search/portable-text-indexing.test.ts index a2b977e936..c3bb82cf49 100644 --- a/packages/core/tests/integration/search/portable-text-indexing.test.ts +++ b/packages/core/tests/integration/search/portable-text-indexing.test.ts @@ -110,3 +110,56 @@ describe("Portable Text FTS indexing", () => { expect(snippet).not.toContain("{"); }); }); + +describe("Portable Text FTS indexing — json_tree column-name collisions", () => { + let db: Kysely; + + beforeEach(async () => { + db = await setupTestDatabase(); + }); + + afterEach(async () => { + await teardownTestDatabase(db); + }); + + it("populates fields whose slug collides with a json_tree output column", async () => { + // json_tree exposes columns named key/value/type/path/...; an + // unqualified column reference inside the extraction subquery binds to + // those instead of the ec_* column, silently indexing NULL. + const registry = new SchemaRegistry(db); + await registry.createCollection({ + slug: "notes", + label: "Notes", + labelSingular: "Note", + supports: ["search"], + }); + await registry.createField("notes", { + slug: "value", + label: "Value", + type: "portableText", + searchable: true, + }); + + // Create before enabling search so the row flows through + // populateFromContent (the bare-reference path), not the triggers. + await new ContentRepository(db).create({ + type: "notes", + slug: "n1", + status: "published", + data: { + value: [ + { + _type: "block", + _key: "b1", + style: "normal", + children: [{ _type: "span", _key: "s1", text: "A spectral apparition." }], + }, + ], + }, + }); + await new FTSManager(db).enableSearch("notes"); + + const { items } = await searchWithDb(db, "spectral", { collections: ["notes"] }); + expect(items).toHaveLength(1); + }); +}); From 36b60c4a3312e6b4fc0b375a40a004d361d6b737 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eder=20S=C3=A1nchez?= Date: Fri, 31 Jul 2026 09:28:24 -0600 Subject: [PATCH 3/4] fix: guard FTS triggers so only real content changes re-tokenize MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The FTS update trigger fired on ANY row UPDATE, deleting and re-inserting the document's full index entry even when no searchable column changed. Metadata-only saves — status flips, scheduling, autosave version bumps — and the publish path's rewrite-identical-values UPDATEs each paid full re-tokenization: measured 49x CPU on metadata-only saves and 78-89% of a save's WAL bytes on an audited production deployment. Add a WHEN guard comparing raw column values with null-safe IS NOT: the trigger fires only when an indexed value, the row's locale, or its trash state actually changed. deleted_at stays in the guard so trash/restore keep syncing the index. Raw-column comparison remains valid change detection for Portable Text fields whose indexed values are extracted text. Migration 056 recreates the triggers on existing deployments — trigger swap only, index contents untouched. Co-Authored-By: Claude Fable 5 --- .changeset/fts-trigger-when-guards.md | 5 + .../migrations/057_fts_trigger_when_guards.ts | 185 ++++++++++++++++++ .../core/src/database/migrations/runner.ts | 2 + packages/core/src/search/fts-manager.ts | 17 +- .../integration/database/migrations.test.ts | 1 + .../search/fts-write-amplification.test.ts | 131 +++++++++++++ .../057_fts_trigger_when_guards.test.ts | 115 +++++++++++ 7 files changed, 454 insertions(+), 2 deletions(-) create mode 100644 .changeset/fts-trigger-when-guards.md create mode 100644 packages/core/src/database/migrations/057_fts_trigger_when_guards.ts create mode 100644 packages/core/tests/integration/search/fts-write-amplification.test.ts create mode 100644 packages/core/tests/unit/database/migrations/057_fts_trigger_when_guards.test.ts diff --git a/.changeset/fts-trigger-when-guards.md b/.changeset/fts-trigger-when-guards.md new file mode 100644 index 0000000000..88405fdb96 --- /dev/null +++ b/.changeset/fts-trigger-when-guards.md @@ -0,0 +1,5 @@ +--- +"emdash": patch +--- + +Fixes content saves re-tokenizing the full search index even when nothing searchable changed. Metadata-only saves (status changes, scheduling, autosave version bumps) no longer rewrite the FTS index, cutting save CPU and write-ahead-log volume on large documents. Existing deployments get the updated triggers automatically via a migration. diff --git a/packages/core/src/database/migrations/057_fts_trigger_when_guards.ts b/packages/core/src/database/migrations/057_fts_trigger_when_guards.ts new file mode 100644 index 0000000000..f47b51eff3 --- /dev/null +++ b/packages/core/src/database/migrations/057_fts_trigger_when_guards.ts @@ -0,0 +1,185 @@ +import type { Kysely } from "kysely"; +import { sql } from "kysely"; + +import { isSqlite } from "../dialect-helpers.js"; +import { validateIdentifier } from "../validate.js"; + +/** + * Migration: Recreate FTS sync triggers with change-detection WHEN guards + * + * Background: the FTS update trigger fired on ANY row UPDATE, re-tokenizing + * the full document even when no indexed column changed. Metadata-only + * saves (status flips, scheduling, version bumps) and the publish path's + * rewrite-identical-values UPDATEs paid full re-tokenization on every + * statement — the dominant driver of save CPU and WAL volume on large + * documents. + * + * This migration drops and recreates the three sync triggers for every + * search-enabled collection with a WHEN guard on the update trigger that + * compares raw column values (null-safe IS NOT): the trigger now fires only + * when an indexed value, the row's locale, or its trash state actually + * changed. Index contents are untouched — no repopulate needed. + * + * The trigger SQL emitted here MUST stay in lock-step with + * `FTSManager.createTriggers` in `src/search/fts-manager.ts`. If that + * changes again, add a new migration rather than editing this one — + * migrations are forward-only. + * + * Postgres: no-op. FTS5 is SQLite-only. + * + * D1: idempotent — DROP IF EXISTS / CREATE IF NOT EXISTS, so concurrent + * migrators converge. + */ + +interface CollectionRow { + slug: string; + search_config: string | null; +} + +interface FieldRow { + slug: string; + type: string; +} + +export async function up(db: Kysely): Promise { + if (!isSqlite(db)) return; + + const collections = await sql` + SELECT slug, search_config FROM _emdash_collections + WHERE search_config IS NOT NULL + `.execute(db); + + for (const collection of collections.rows) { + if (!isSearchEnabled(collection.search_config)) continue; + + try { + validateIdentifier(collection.slug, "collection slug"); + } catch (error) { + console.warn( + `[migration 057] skipping trigger rebuild for collection "${collection.slug}": ${ + error instanceof Error ? error.message : String(error) + }`, + ); + continue; + } + + const fields = await getSearchableFields(db, collection.slug); + if (fields.length === 0) continue; + + await recreateTriggers(db, collection.slug, fields); + } +} + +/** + * Forward-only. Down is a no-op: the guarded triggers remain correct for + * older code, they just skip no-op re-tokenizations. + */ +export async function down(_db: Kysely): Promise { + // no-op +} + +function isSearchEnabled(searchConfig: string | null): boolean { + if (!searchConfig) return false; + try { + const parsed: unknown = JSON.parse(searchConfig); + return ( + typeof parsed === "object" && + parsed !== null && + "enabled" in parsed && + parsed.enabled === true + ); + } catch { + return false; + } +} + +async function getSearchableFields( + db: Kysely, + collectionSlug: string, +): Promise { + const rows = await sql` + SELECT f.slug, f.type FROM _emdash_fields f + INNER JOIN _emdash_collections c ON c.id = f.collection_id + WHERE c.slug = ${collectionSlug} AND f.searchable = 1 + `.execute(db); + + const out: FieldRow[] = []; + for (const row of rows.rows) { + try { + validateIdentifier(row.slug, "searchable field name"); + out.push(row); + } catch { + console.warn( + `[migration 057] skipping invalid searchable field "${row.slug}" on collection "${collectionSlug}"`, + ); + } + } + return out; +} + +/** Indexed-value expression for one field; lock-step with FTSManager.searchValueExpr. */ +function searchValueExpr(ref: string, fieldType: string): string { + if (fieldType !== "portableText") return ref; + return ( + `CASE WHEN ${ref} IS NULL THEN NULL ` + + `WHEN json_valid(${ref}) THEN (` + + `SELECT group_concat(j.value, ' ') FROM json_tree(${ref}) AS j ` + + `WHERE j.key IN ('text', 'alt', 'caption', 'code') AND j.type = 'text') ` + + `ELSE ${ref} END` + ); +} + +async function recreateTriggers( + db: Kysely, + collectionSlug: string, + fields: FieldRow[], +): Promise { + const ftsTable = `_emdash_fts_${collectionSlug}`; + const contentTable = `ec_${collectionSlug}`; + const slugs = fields.map((f) => f.slug); + const fieldList = slugs.join(", "); + const newValueList = fields.map((f) => searchValueExpr(`NEW.${f.slug}`, f.type)).join(", "); + const changedCondition = ["deleted_at", "locale", ...slugs] + .map((f) => `OLD.${f} IS NOT NEW.${f}`) + .join(" OR "); + + await sql.raw(`DROP TRIGGER IF EXISTS "${ftsTable}_insert"`).execute(db); + await sql.raw(`DROP TRIGGER IF EXISTS "${ftsTable}_update"`).execute(db); + await sql.raw(`DROP TRIGGER IF EXISTS "${ftsTable}_delete"`).execute(db); + + await sql + .raw(` + CREATE TRIGGER IF NOT EXISTS "${ftsTable}_insert" + AFTER INSERT ON "${contentTable}" + WHEN NEW.deleted_at IS NULL + BEGIN + INSERT OR REPLACE INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) + VALUES (NEW.rowid, NEW.id, NEW.locale, ${newValueList}); + END + `) + .execute(db); + + await sql + .raw(` + CREATE TRIGGER IF NOT EXISTS "${ftsTable}_update" + AFTER UPDATE ON "${contentTable}" + WHEN ${changedCondition} + BEGIN + DELETE FROM "${ftsTable}" WHERE rowid = OLD.rowid; + INSERT INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) + SELECT NEW.rowid, NEW.id, NEW.locale, ${newValueList} + WHERE NEW.deleted_at IS NULL; + END + `) + .execute(db); + + await sql + .raw(` + CREATE TRIGGER IF NOT EXISTS "${ftsTable}_delete" + AFTER DELETE ON "${contentTable}" + BEGIN + DELETE FROM "${ftsTable}" WHERE rowid = OLD.rowid; + END + `) + .execute(db); +} diff --git a/packages/core/src/database/migrations/runner.ts b/packages/core/src/database/migrations/runner.ts index 2d77fad461..6e2ba91eb2 100644 --- a/packages/core/src/database/migrations/runner.ts +++ b/packages/core/src/database/migrations/runner.ts @@ -59,6 +59,7 @@ import * as m053 from "./053_plugin_mcp_tools.js"; import * as m054 from "./054_media_upload_attempts.js"; import * as m055 from "./055_content_translation_group_locale_index.js"; import * as m056 from "./056_fts_plain_text.js"; +import * as m057 from "./057_fts_trigger_when_guards.js"; const MIGRATIONS: Readonly> = Object.freeze({ "001_initial": m001, @@ -116,6 +117,7 @@ const MIGRATIONS: Readonly> = Object.freeze({ "054_media_upload_attempts": m054, "055_content_translation_group_locale_index": m055, "056_fts_plain_text": m056, + "057_fts_trigger_when_guards": m057, }); /** Total number of registered migrations. Exported for use in tests. */ diff --git a/packages/core/src/search/fts-manager.ts b/packages/core/src/search/fts-manager.ts index c74a30454e..7563905708 100644 --- a/packages/core/src/search/fts-manager.ts +++ b/packages/core/src/search/fts-manager.ts @@ -184,8 +184,9 @@ export class FTSManager { * constraint. * * The trigger SQL emitted here MUST stay in lock-step with migration - * `056_fts_plain_text.ts`. If this changes again, add a new migration - * rather than editing that one — migrations are forward-only. + * `057_fts_trigger_when_guards.ts` (the latest migration that emits these + * triggers). If this changes again, add a new migration rather than + * editing shipped ones — migrations are forward-only. */ private async createTriggers(collectionSlug: string, searchableFields: string[]): Promise { this.validateInputs(collectionSlug, searchableFields); @@ -219,10 +220,22 @@ export class FTSManager { // Update trigger - drop the old index row, re-insert when the row is // still visible. Trash (deleted_at set) ends at DELETE only; restore // ends at DELETE (no-op) + re-insert. + // + // The WHEN guard compares raw column values (null-safe IS NOT) so the + // trigger fires only when an indexed value, the row's locale, or its + // trash state actually changed. Without it every UPDATE re-tokenizes + // the whole document — metadata-only saves (status flips, scheduling, + // version bumps) and the publish path's rewrite-identical-values + // UPDATEs dominate save CPU and WAL volume. deleted_at must stay in + // the guard or trash/restore stop syncing the index. + const changedCondition = ["deleted_at", "locale", ...searchableFields] + .map((f) => `OLD.${f} IS NOT NEW.${f}`) + .join(" OR "); await sql .raw(` CREATE TRIGGER IF NOT EXISTS "${ftsTable}_update" AFTER UPDATE ON "${contentTable}" + WHEN ${changedCondition} BEGIN DELETE FROM "${ftsTable}" WHERE rowid = OLD.rowid; INSERT INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) diff --git a/packages/core/tests/integration/database/migrations.test.ts b/packages/core/tests/integration/database/migrations.test.ts index 529c752279..b127746365 100644 --- a/packages/core/tests/integration/database/migrations.test.ts +++ b/packages/core/tests/integration/database/migrations.test.ts @@ -143,6 +143,7 @@ describe("Database Migrations (Integration)", () => { "054_media_upload_attempts", "055_content_translation_group_locale_index", "056_fts_plain_text", + "057_fts_trigger_when_guards", ]; await db.deleteFrom("_emdash_migrations").where("name", "in", trailing).execute(); diff --git a/packages/core/tests/integration/search/fts-write-amplification.test.ts b/packages/core/tests/integration/search/fts-write-amplification.test.ts new file mode 100644 index 0000000000..0fbdc1afd9 --- /dev/null +++ b/packages/core/tests/integration/search/fts-write-amplification.test.ts @@ -0,0 +1,131 @@ +/** + * FTS triggers only re-tokenize when an indexed value actually changed. + * + * The update trigger fires on ANY row UPDATE. Without a WHEN guard it + * rewrites the document's index entry even when no searchable column + * changed — metadata-only saves (status flips, scheduling, version bumps) + * re-tokenize the full document, dominating save CPU and WAL volume. The + * FTS `_data` shadow table holds the index segments, so a byte-identical + * dump across a metadata-only update proves no re-tokenization happened. + */ + +import type { Kysely } from "kysely"; +import { sql } from "kysely"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { ContentRepository } from "../../../src/database/repositories/content.js"; +import type { Database } from "../../../src/database/types.js"; +import { SchemaRegistry } from "../../../src/schema/registry.js"; +import { FTSManager } from "../../../src/search/fts-manager.js"; +import { searchWithDb } from "../../../src/search/query.js"; +import { setupTestDatabase, teardownTestDatabase } from "../../utils/test-db.js"; + +describe("FTS write amplification", () => { + let db: Kysely; + let registry: SchemaRegistry; + let repo: ContentRepository; + let entryId: string; + + beforeEach(async () => { + db = await setupTestDatabase(); + registry = new SchemaRegistry(db); + repo = new ContentRepository(db); + + await registry.createCollection({ + slug: "pages", + label: "Pages", + labelSingular: "Page", + supports: ["drafts", "revisions", "search"], + }); + await registry.createField("pages", { + slug: "content", + label: "Content", + type: "portableText", + searchable: true, + }); + await registry.createField("pages", { + slug: "title", + label: "Title", + type: "string", + searchable: true, + }); + await new FTSManager(db).enableSearch("pages"); + + const created = await repo.create({ + type: "pages", + slug: "haunted", + status: "published", + data: { + title: "Opening Night", + content: [ + { + _type: "block", + _key: "b1", + style: "normal", + children: [{ _type: "span", _key: "s1", text: "The haunted cinema." }], + }, + ], + }, + }); + entryId = created.id; + }); + + afterEach(async () => { + await teardownTestDatabase(db); + }); + + /** Byte-level dump of the FTS index segments. */ + async function indexSegments(): Promise { + const rows = await sql<{ id: number; block: string }>` + SELECT id, quote(block) as block FROM "_emdash_fts_pages_data" ORDER BY id + `.execute(db); + return rows.rows.map((r) => `${r.id}:${r.block}`); + } + + it("does not re-tokenize on a metadata-only update", async () => { + const before = await indexSegments(); + + await sql` + UPDATE ec_pages SET scheduled_at = '2027-01-01T00:00:00.000Z', version = version + 1 + WHERE id = ${entryId} + `.execute(db); + + expect(await indexSegments()).toEqual(before); + }); + + it("does not re-tokenize when the publish path rewrites identical data values", async () => { + const before = await indexSegments(); + + // The publish path SETs every data column even when values are + // unchanged; only value comparison suppresses those re-tokenizations. + const row = await sql<{ content: string; title: string }>` + SELECT content, title FROM ec_pages WHERE id = ${entryId} + `.execute(db); + await sql` + UPDATE ec_pages + SET content = ${row.rows[0]!.content}, title = ${row.rows[0]!.title}, status = 'published' + WHERE id = ${entryId} + `.execute(db); + + expect(await indexSegments()).toEqual(before); + }); + + it("still re-indexes when a searchable field changes", async () => { + await repo.update("pages", entryId, { + data: { title: "Closing Night", content: null }, + }); + + const { items } = await searchWithDb(db, "closing", { collections: ["pages"] }); + expect(items).toHaveLength(1); + const stale = await searchWithDb(db, "opening", { collections: ["pages"] }); + expect(stale.items).toEqual([]); + }); + + it("still removes trashed entries from the index and restores them", async () => { + await sql`UPDATE ec_pages SET deleted_at = datetime('now') WHERE id = ${entryId}`.execute(db); + expect((await searchWithDb(db, "haunted", { collections: ["pages"] })).items).toEqual([]); + + await sql`UPDATE ec_pages SET deleted_at = NULL WHERE id = ${entryId}`.execute(db); + expect((await searchWithDb(db, "haunted", { collections: ["pages"] })).items).toHaveLength(1); + }); +}); diff --git a/packages/core/tests/unit/database/migrations/057_fts_trigger_when_guards.test.ts b/packages/core/tests/unit/database/migrations/057_fts_trigger_when_guards.test.ts new file mode 100644 index 0000000000..8c64338a05 --- /dev/null +++ b/packages/core/tests/unit/database/migrations/057_fts_trigger_when_guards.test.ts @@ -0,0 +1,115 @@ +/** + * Migration 057 recreates FTS sync triggers with change-detection WHEN + * guards. These tests exercise the upgrade path: a database whose update + * trigger fires on any UPDATE gets guarded triggers, without touching the + * index contents. + */ + +import type { Kysely } from "kysely"; +import { sql } from "kysely"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { ContentRepository } from "../../../../src/database/repositories/content.js"; +import type { Database } from "../../../../src/database/types.js"; +import { SchemaRegistry } from "../../../../src/schema/registry.js"; +import { FTSManager } from "../../../../src/search/fts-manager.js"; +import { setupTestDatabase, teardownTestDatabase } from "../../../utils/test-db.js"; + +describe("migration 057: FTS trigger WHEN guards", () => { + let db: Kysely; + let entryId: string; + + beforeEach(async () => { + db = await setupTestDatabase(); + const registry = new SchemaRegistry(db); + const repo = new ContentRepository(db); + + await registry.createCollection({ + slug: "pages", + label: "Pages", + labelSingular: "Page", + supports: ["search"], + }); + await registry.createField("pages", { + slug: "title", + label: "Title", + type: "string", + searchable: true, + }); + await new FTSManager(db).enableSearch("pages"); + + const created = await repo.create({ + type: "pages", + slug: "haunted", + status: "published", + data: { title: "The haunted cinema" }, + }); + entryId = created.id; + + // Pre-057 state: the update trigger fires on ANY row UPDATE. + await sql.raw(`DROP TRIGGER IF EXISTS "_emdash_fts_pages_update"`).execute(db); + await sql + .raw(` + CREATE TRIGGER "_emdash_fts_pages_update" + AFTER UPDATE ON "ec_pages" + BEGIN + DELETE FROM "_emdash_fts_pages" WHERE rowid = OLD.rowid; + INSERT INTO "_emdash_fts_pages"(rowid, id, locale, title) + SELECT NEW.rowid, NEW.id, NEW.locale, NEW.title + WHERE NEW.deleted_at IS NULL; + END + `) + .execute(db); + }); + + afterEach(async () => { + await teardownTestDatabase(db); + }); + + async function indexSegments(): Promise { + const rows = await sql<{ id: number; block: string }>` + SELECT id, quote(block) as block FROM "_emdash_fts_pages_data" ORDER BY id + `.execute(db); + return rows.rows.map((r) => `${r.id}:${r.block}`); + } + + async function metadataOnlyUpdate(): Promise { + await sql` + UPDATE ec_pages SET version = version + 1 WHERE id = ${entryId} + `.execute(db); + } + + async function runMigration057(): Promise { + const { up } = + await import("../../../../src/database/migrations/057_fts_trigger_when_guards.js"); + await up(db as unknown as Kysely); + } + + it("stops metadata-only updates from re-tokenizing", async () => { + // Baseline with teeth: the pre-057 trigger rewrites index segments + // on a metadata-only update. + const before = await indexSegments(); + await metadataOnlyUpdate(); + expect(await indexSegments()).not.toEqual(before); + + await runMigration057(); + + const guarded = await indexSegments(); + await metadataOnlyUpdate(); + expect(await indexSegments()).toEqual(guarded); + }); + + it("still syncs the index when a searchable field changes", async () => { + await runMigration057(); + + await sql` + UPDATE ec_pages SET title = 'A midnight screening' WHERE id = ${entryId} + `.execute(db); + + const matches = await sql<{ count: number }>` + SELECT COUNT(*) as count FROM "_emdash_fts_pages" + WHERE "_emdash_fts_pages" MATCH 'midnight' + `.execute(db); + expect(Number(matches.rows[0]?.count)).toBe(1); + }); +}); From ee073a7638bdb78f9dd76695f153feec9e10d9dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eder=20S=C3=A1nchez?= Date: Fri, 31 Jul 2026 10:08:37 -0600 Subject: [PATCH 4/4] fix: repopulate after the 057 trigger swap to heal concurrent edits --- .../migrations/057_fts_trigger_when_guards.ts | 18 +++++++++++- .../057_fts_trigger_when_guards.test.ts | 29 +++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/packages/core/src/database/migrations/057_fts_trigger_when_guards.ts b/packages/core/src/database/migrations/057_fts_trigger_when_guards.ts index f47b51eff3..5c482486ad 100644 --- a/packages/core/src/database/migrations/057_fts_trigger_when_guards.ts +++ b/packages/core/src/database/migrations/057_fts_trigger_when_guards.ts @@ -18,7 +18,10 @@ import { validateIdentifier } from "../validate.js"; * search-enabled collection with a WHEN guard on the update trigger that * compares raw column values (null-safe IS NOT): the trigger now fires only * when an indexed value, the row's locale, or its trash state actually - * changed. Index contents are untouched — no repopulate needed. + * changed. A repopulate follows the trigger swap: D1 has no migration lock, + * so a concurrent isolate can write inside the drop/create window, and a + * lost UPDATE leaves row counts equal — invisible to verifyAndRepairIndex's + * parity check. INSERT OR REPLACE makes the heal idempotent. * * The trigger SQL emitted here MUST stay in lock-step with * `FTSManager.createTriggers` in `src/search/fts-manager.ts`. If that @@ -139,6 +142,11 @@ async function recreateTriggers( const slugs = fields.map((f) => f.slug); const fieldList = slugs.join(", "); const newValueList = fields.map((f) => searchValueExpr(`NEW.${f.slug}`, f.type)).join(", "); + // Table-qualified: a bare column reference inside the json_tree extraction + // subquery binds to json_tree's own key/value/type/... columns. + const selectValueList = fields + .map((f) => searchValueExpr(`"${contentTable}"."${f.slug}"`, f.type)) + .join(", "); const changedCondition = ["deleted_at", "locale", ...slugs] .map((f) => `OLD.${f} IS NOT NEW.${f}`) .join(" OR "); @@ -182,4 +190,12 @@ async function recreateTriggers( END `) .execute(db); + + await sql + .raw(` + INSERT OR REPLACE INTO "${ftsTable}"(rowid, id, locale, ${fieldList}) + SELECT rowid, id, locale, ${selectValueList} FROM "${contentTable}" + WHERE deleted_at IS NULL + `) + .execute(db); } diff --git a/packages/core/tests/unit/database/migrations/057_fts_trigger_when_guards.test.ts b/packages/core/tests/unit/database/migrations/057_fts_trigger_when_guards.test.ts index 8c64338a05..ab475284a7 100644 --- a/packages/core/tests/unit/database/migrations/057_fts_trigger_when_guards.test.ts +++ b/packages/core/tests/unit/database/migrations/057_fts_trigger_when_guards.test.ts @@ -112,4 +112,33 @@ describe("migration 057: FTS trigger WHEN guards", () => { `.execute(db); expect(Number(matches.rows[0]?.count)).toBe(1); }); + + it("heals an edit that landed while the triggers were absent", async () => { + // D1 has no migration lock, so a concurrent isolate can write inside + // the drop/create window. A lost UPDATE leaves row counts equal, which + // verifyAndRepairIndex's parity check cannot detect. + await sql.raw(`DROP TRIGGER IF EXISTS "_emdash_fts_pages_update"`).execute(db); + await sql` + UPDATE ec_pages SET title = 'A midnight screening' WHERE id = ${entryId} + `.execute(db); + + const lost = await sql<{ count: number }>` + SELECT COUNT(*) as count FROM "_emdash_fts_pages" + WHERE "_emdash_fts_pages" MATCH 'midnight' + `.execute(db); + expect(Number(lost.rows[0]?.count)).toBe(0); + + await runMigration057(); + + const healed = await sql<{ count: number }>` + SELECT COUNT(*) as count FROM "_emdash_fts_pages" + WHERE "_emdash_fts_pages" MATCH 'midnight' + `.execute(db); + expect(Number(healed.rows[0]?.count)).toBe(1); + const stale = await sql<{ count: number }>` + SELECT COUNT(*) as count FROM "_emdash_fts_pages" + WHERE "_emdash_fts_pages" MATCH 'haunted' + `.execute(db); + expect(Number(stale.rows[0]?.count)).toBe(0); + }); });