From aaed59ac3ffab89833c9627cb79fa64c0fbdbef9 Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 14:02:34 +0100 Subject: [PATCH 01/35] feat(bylines): types and storage interfaces for custom fields --- .../core/src/database/repositories/types.ts | 11 ++ packages/core/src/database/types.ts | 44 +++++++ packages/core/src/schema/types.ts | 116 ++++++++++++++++++ 3 files changed, 171 insertions(+) diff --git a/packages/core/src/database/repositories/types.ts b/packages/core/src/database/repositories/types.ts index 4ecea29a5..ba9dd34c8 100644 --- a/packages/core/src/database/repositories/types.ts +++ b/packages/core/src/database/repositories/types.ts @@ -1,3 +1,4 @@ +import type { CustomFieldValue } from "../../schema/types.js"; import { encodeBase64, decodeBase64 } from "../../utils/base64.js"; /** @@ -76,6 +77,16 @@ export interface BylineSummary { * populate it. */ translationGroup: string | null; + /** + * Custom field values registered via the byline-fields schema (migration + * 041, Discussion #1174). Optional in the TypeScript shape so existing + * object-literal consumers (test fixtures, plugin renderers) stay + * source-compatible; the runtime always returns `{}` when no fields are + * registered. Translatable values reflect this row's locale; non- + * translatable values are shared across every locale variant of the + * byline's `translation_group`. + */ + customFields?: Record; } export interface ContentBylineCredit { diff --git a/packages/core/src/database/types.ts b/packages/core/src/database/types.ts index 6da3a6c2f..cc64ce639 100644 --- a/packages/core/src/database/types.ts +++ b/packages/core/src/database/types.ts @@ -437,6 +437,9 @@ export interface Database { _emdash_404_log: NotFoundLogTable; _emdash_bylines: BylineTable; _emdash_content_bylines: ContentBylineTable; + _emdash_byline_fields: BylineFieldTable; + _emdash_byline_field_values: BylineFieldValueTable; + _emdash_byline_field_group_values: BylineFieldGroupValueTable; _emdash_rate_limits: RateLimitTable; } @@ -529,6 +532,47 @@ export interface ContentBylineTable { created_at: Generated; } +// Byline custom fields (migration 041, Discussion #1174) +// +// `_emdash_byline_fields` stores definitions; values land in either +// `_emdash_byline_field_values` (translatable, keyed by byline row id) or +// `_emdash_byline_field_group_values` (non-translatable, keyed by +// translation_group). Per-field `translatable` flag picks the home table. + +export interface BylineFieldTable { + id: string; + slug: string; + label: string; + /** One of: 'string', 'text', 'url', 'boolean', 'select'. v1 subset. */ + type: string; + required: Generated; // 0 or 1 + /** 0 = group-shared, 1 = per-locale. Defaults to 1 at the DB level. */ + translatable: Generated; + /** JSON: `{ options?: string[] }` for `select`-type fields. */ + validation: string | null; + sort_order: Generated; + created_at: Generated; + updated_at: Generated; +} + +export interface BylineFieldValueTable { + byline_id: string; + field_id: string; + /** JSON-encoded value (`CustomFieldValue` after parse). */ + value: string | null; + created_at: Generated; + updated_at: Generated; +} + +export interface BylineFieldGroupValueTable { + translation_group: string; + field_id: string; + /** JSON-encoded value (`CustomFieldValue` after parse). */ + value: string | null; + created_at: Generated; + updated_at: Generated; +} + // Rate Limits export interface RateLimitTable { diff --git a/packages/core/src/schema/types.ts b/packages/core/src/schema/types.ts index ec16698fc..5a74e0970 100644 --- a/packages/core/src/schema/types.ts +++ b/packages/core/src/schema/types.ts @@ -312,3 +312,119 @@ export const RESERVED_COLLECTION_SLUGS = [ "options", "audit_logs", ]; + +/** + * Byline custom fields (Discussion #1174). + * + * Sites declare site-specific byline metadata (`job_title`, `pronouns`, + * `twitter_handle`, `company`, …) without touching emdash core. Definitions + * live in `_emdash_byline_fields`; values in either + * `_emdash_byline_field_values` (translatable, keyed by `byline_id`) or + * `_emdash_byline_field_group_values` (non-translatable, keyed by + * `translation_group`). The per-field `translatable` flag decides which + * value table is used. See migration 041. + */ + +/** + * The five v1 field types supported on byline custom fields. Deliberately + * narrower than the content `FieldType` union — bylines don't need + * `portableText`, `reference`, `image`, etc. v2 may extend this; v1 keeps + * the storage and UI surfaces small. + */ +export type BylineFieldType = "string" | "text" | "url" | "boolean" | "select"; + +export const BYLINE_FIELD_TYPES: readonly BylineFieldType[] = [ + "string", + "text", + "url", + "boolean", + "select", +] as const; + +/** + * Validation rules for a byline custom field. v1 only exposes `options` + * (the choice list for `select` fields). The shape mirrors the content- + * field convention so the admin UI patterns transfer. + */ +export interface BylineFieldValidation { + /** Choices for `select`-type fields. Ignored for other types. */ + options?: string[]; +} + +/** + * Runtime shape of a registered byline custom field. Stored in + * `_emdash_byline_fields` (see migration 041). + */ +export interface BylineFieldDefinition { + id: string; + slug: string; + label: string; + type: BylineFieldType; + required: boolean; + /** + * Whether values are stored per-locale (`true`, in + * `_emdash_byline_field_values` keyed by `byline_id`) or shared across + * every locale variant of the same byline identity (`false`, in + * `_emdash_byline_field_group_values` keyed by `translation_group`). + * Defaults to `true` at the DB level. + */ + translatable: boolean; + validation: BylineFieldValidation | null; + sortOrder: number; + createdAt: string; + updatedAt: string; +} + +/** + * Input for creating a byline custom field. `slug` and `type` are not + * updatable post-create — changing either would invalidate stored values. + */ +export interface CreateBylineFieldInput { + slug: string; + label: string; + type: BylineFieldType; + required?: boolean; + translatable?: boolean; + validation?: BylineFieldValidation | null; + sortOrder?: number; +} + +/** + * Input for updating a byline custom field. `slug` and `type` are + * intentionally not present — see `CreateBylineFieldInput`. + */ +export interface UpdateBylineFieldInput { + label?: string; + required?: boolean; + translatable?: boolean; + validation?: BylineFieldValidation | null; + sortOrder?: number; +} + +/** + * Runtime value type for a byline custom field. The narrow union mirrors + * what the five v1 field types can produce: `string`/`text`/`url`/`select` + * → string, `boolean` → boolean, plus `null` for cleared values. + */ +export type CustomFieldValue = string | boolean | null; + +/** + * Reserved byline-field slugs. These collide with fixed columns on + * `_emdash_bylines` (migrations 031 + 040); allowing a custom field with + * one of these slugs would shadow the column on hydration. Enforced at the + * registry layer (Phase 2) and the admin API zod layer (Phase 4). + */ +export const RESERVED_BYLINE_FIELD_SLUGS = [ + "id", + "slug", + "display_name", + "bio", + "avatar_media_id", + "website_url", + "user_id", + "is_guest", + "locale", + "translation_group", + "created_at", + "updated_at", +] as const; From 23a9e2f32d2755a52ade1a02276688270a9409bf Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 14:02:45 +0100 Subject: [PATCH 02/35] feat(bylines): migration 041 for custom field tables --- .../database/migrations/041_byline_fields.ts | 157 ++++++++ .../core/src/database/migrations/runner.ts | 2 + .../integration/database/migrations.test.ts | 4 + .../migrations/041_byline_fields.test.ts | 361 ++++++++++++++++++ 4 files changed, 524 insertions(+) create mode 100644 packages/core/src/database/migrations/041_byline_fields.ts create mode 100644 packages/core/tests/unit/database/migrations/041_byline_fields.test.ts diff --git a/packages/core/src/database/migrations/041_byline_fields.ts b/packages/core/src/database/migrations/041_byline_fields.ts new file mode 100644 index 000000000..f8e343482 --- /dev/null +++ b/packages/core/src/database/migrations/041_byline_fields.ts @@ -0,0 +1,157 @@ +import type { Kysely } from "kysely"; +import { sql } from "kysely"; + +import { currentTimestamp } from "../dialect-helpers.js"; + +/** + * Byline custom fields (Discussion #1174). Adds three tables and a + * version-counter row in `options`. Purely additive: no change to + * `_emdash_bylines` columns landed by migration 040, no change to + * `_emdash_content_bylines`. + * + * Storage model (D11 in the design spec): + * + * - `_emdash_byline_fields` — definitions. One row per registered field. + * - `_emdash_byline_field_values` — translatable values, keyed by + * `(byline_id, field_id)`. One value per locale variant of a byline. + * - `_emdash_byline_field_group_values` — non-translatable values, keyed + * by `(translation_group, field_id)`. One value shared across every + * locale variant of the byline's translation group. + * + * The per-field `translatable` flag (column on `_emdash_byline_fields`) + * decides which value table a field writes to. The split is at the + * storage level rather than per-row so a single SELECT … IN per locale + * bucket suffices for batched hydration (see Phase 3 of the PR plan). + * + * `options('byline_fields_version', '0')` is the version counter the + * field-definitions cache reads (Phase 3). Every mutation on + * `_emdash_byline_fields` bumps this row; cached defs invalidate on a + * mismatch. Storing it in `options` (the same table `settings/index.ts` + * uses) means we get the request-cache + persisted-version pattern for + * free — no new infrastructure. + * + * Idempotency: every `CREATE TABLE` and `CREATE INDEX` uses + * `.ifNotExists()`, so a partial prior run (a crash mid-migration, retried + * after the runner's race-recovery path or after a manual fix) re-applies + * cleanly — including any indexes that landed in the failed pass after the + * table itself. A coarse table-level guard would skip the index step if the + * table existed but indexes didn't (the realistic crash window between + * `CREATE TABLE` and the next `CREATE INDEX`). The runner records applied + * migrations only after a successful pass, so a crashed-then-retried `up()` + * is the normal recovery path here. + * + * D1 has no transactions — the schema builder and `INSERT` for the + * version row execute one statement at a time. Order matters: parent + * tables first (`_emdash_byline_fields`), then the value tables that + * reference it. If `down()` runs after a partial `up()`, missing tables + * are tolerated (`DROP TABLE IF EXISTS`). + */ + +export async function up(db: Kysely): Promise { + await db.schema + .createTable("_emdash_byline_fields") + .ifNotExists() + .addColumn("id", "text", (col) => col.primaryKey()) + .addColumn("slug", "text", (col) => col.notNull().unique()) + .addColumn("label", "text", (col) => col.notNull()) + .addColumn("type", "text", (col) => col.notNull()) + .addColumn("required", "integer", (col) => col.notNull().defaultTo(0)) + .addColumn("translatable", "integer", (col) => col.notNull().defaultTo(1)) + .addColumn("validation", "text") // JSON: { options?: string[] } + .addColumn("sort_order", "integer", (col) => col.notNull().defaultTo(0)) + .addColumn("created_at", "text", (col) => col.defaultTo(currentTimestamp(db))) + .addColumn("updated_at", "text", (col) => col.defaultTo(currentTimestamp(db))) + .execute(); + + await db.schema + .createIndex("idx__emdash_byline_fields_sort_order") + .ifNotExists() + .on("_emdash_byline_fields") + .column("sort_order") + .execute(); + + await db.schema + .createTable("_emdash_byline_field_values") + .ifNotExists() + .addColumn("byline_id", "text", (col) => + col.notNull().references("_emdash_bylines.id").onDelete("cascade"), + ) + .addColumn("field_id", "text", (col) => + col.notNull().references("_emdash_byline_fields.id").onDelete("cascade"), + ) + .addColumn("value", "text") // JSON-encoded CustomFieldValue + .addColumn("created_at", "text", (col) => col.defaultTo(currentTimestamp(db))) + .addColumn("updated_at", "text", (col) => col.defaultTo(currentTimestamp(db))) + .addPrimaryKeyConstraint("_emdash_byline_field_values_pk", ["byline_id", "field_id"]) + .execute(); + + await db.schema + .createIndex("idx__emdash_byline_field_values_byline") + .ifNotExists() + .on("_emdash_byline_field_values") + .column("byline_id") + .execute(); + await db.schema + .createIndex("idx__emdash_byline_field_values_field") + .ifNotExists() + .on("_emdash_byline_field_values") + .column("field_id") + .execute(); + + await db.schema + .createTable("_emdash_byline_field_group_values") + .ifNotExists() + .addColumn("translation_group", "text", (col) => col.notNull()) + .addColumn("field_id", "text", (col) => + col.notNull().references("_emdash_byline_fields.id").onDelete("cascade"), + ) + .addColumn("value", "text") // JSON-encoded CustomFieldValue + .addColumn("created_at", "text", (col) => col.defaultTo(currentTimestamp(db))) + .addColumn("updated_at", "text", (col) => col.defaultTo(currentTimestamp(db))) + .addPrimaryKeyConstraint("_emdash_byline_field_group_values_pk", [ + "translation_group", + "field_id", + ]) + .execute(); + + await db.schema + .createIndex("idx__emdash_byline_field_group_values_group") + .ifNotExists() + .on("_emdash_byline_field_group_values") + .column("translation_group") + .execute(); + await db.schema + .createIndex("idx__emdash_byline_field_group_values_field") + .ifNotExists() + .on("_emdash_byline_field_group_values") + .column("field_id") + .execute(); + + // Version-counter row read by the field-definitions cache (Phase 3). + // `options.value` stores JSON, so the initial counter is the JSON literal + // `0`. `INSERT … ON CONFLICT DO NOTHING` so a retry after a crash that + // landed the row but failed later in the migration does not double-insert. + await sql` + INSERT INTO options (name, value) VALUES ('byline_fields_version', '0') + ON CONFLICT (name) DO NOTHING + `.execute(db); +} + +/** + * Reverses the schema additions and removes the version-counter row. + * Used by the test suite and by `rollbackMigration`; the runner itself + * never invokes `down()` automatically. + * + * The drops are unconditional `IF EXISTS` so a `down()` after a partial + * `up()` (only one or two tables landed) still settles the database back + * to its pre-041 state. + */ +export async function down(db: Kysely): Promise { + // Child tables first to avoid FK reference issues on Postgres. SQLite + + // D1 accept either order with `IF EXISTS`, but explicit ordering keeps + // the dialects parity-safe. + await db.schema.dropTable("_emdash_byline_field_group_values").ifExists().execute(); + await db.schema.dropTable("_emdash_byline_field_values").ifExists().execute(); + await db.schema.dropTable("_emdash_byline_fields").ifExists().execute(); + await sql`DELETE FROM options WHERE name = 'byline_fields_version'`.execute(db); +} diff --git a/packages/core/src/database/migrations/runner.ts b/packages/core/src/database/migrations/runner.ts index 24e70d6c2..12b1cbbb1 100644 --- a/packages/core/src/database/migrations/runner.ts +++ b/packages/core/src/database/migrations/runner.ts @@ -42,6 +42,7 @@ import * as m037 from "./037_credential_algorithm.js"; import * as m038 from "./038_registry_plugin_state.js"; import * as m039 from "./039_fix_fts5_triggers.js"; import * as m040 from "./040_byline_i18n.js"; +import * as m041 from "./041_byline_fields.js"; const MIGRATIONS: Readonly> = Object.freeze({ "001_initial": m001, @@ -83,6 +84,7 @@ const MIGRATIONS: Readonly> = Object.freeze({ "038_registry_plugin_state": m038, "039_fix_fts5_triggers": m039, "040_byline_i18n": m040, + "041_byline_fields": m041, }); /** Total number of registered migrations. Exported for use in tests. */ diff --git a/packages/core/tests/integration/database/migrations.test.ts b/packages/core/tests/integration/database/migrations.test.ts index 546334103..4fa6b22db 100644 --- a/packages/core/tests/integration/database/migrations.test.ts +++ b/packages/core/tests/integration/database/migrations.test.ts @@ -45,6 +45,9 @@ describe("Database Migrations (Integration)", () => { "_emdash_sections", "_emdash_bylines", "_emdash_content_bylines", + "_emdash_byline_fields", + "_emdash_byline_field_values", + "_emdash_byline_field_group_values", ]; for (const table of tables) { @@ -120,6 +123,7 @@ describe("Database Migrations (Integration)", () => { "038_registry_plugin_state", "039_fix_fts5_triggers", "040_byline_i18n", + "041_byline_fields", ]; await db.deleteFrom("_emdash_migrations").where("name", "in", trailing).execute(); diff --git a/packages/core/tests/unit/database/migrations/041_byline_fields.test.ts b/packages/core/tests/unit/database/migrations/041_byline_fields.test.ts new file mode 100644 index 000000000..51d40f4b6 --- /dev/null +++ b/packages/core/tests/unit/database/migrations/041_byline_fields.test.ts @@ -0,0 +1,361 @@ +import type { Kysely } from "kysely"; +import { sql } from "kysely"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { createDatabase } from "../../../../src/database/connection.js"; +import { up as up040 } from "../../../../src/database/migrations/040_byline_i18n.js"; +import { down, up } from "../../../../src/database/migrations/041_byline_fields.js"; +import type { Database } from "../../../../src/database/types.js"; + +/** + * Seed the pre-041 schema: byline tables in their post-040 shape, plus the + * `options` table the version-counter row writes to. 041 doesn't touch + * `_emdash_content_bylines` or `ec_*`, but 040 needs the support tables to + * apply — set them up so the "applies on a #1146-era DB" case is exercised + * the same way as a real upgrade. + */ +async function seedPostMigration040Schema(db: Kysely): Promise { + await sql` + CREATE TABLE users ( + id TEXT PRIMARY KEY, + email TEXT + ) + `.execute(db); + + await sql` + CREATE TABLE media ( + id TEXT PRIMARY KEY + ) + `.execute(db); + + await sql` + CREATE TABLE options ( + name TEXT PRIMARY KEY, + value TEXT NOT NULL + ) + `.execute(db); + + // _emdash_bylines in its pre-040 shape; up040 will widen it. + await sql` + CREATE TABLE _emdash_bylines ( + id TEXT PRIMARY KEY, + slug TEXT NOT NULL UNIQUE, + display_name TEXT NOT NULL, + bio TEXT, + avatar_media_id TEXT REFERENCES media(id) ON DELETE SET NULL, + website_url TEXT, + user_id TEXT REFERENCES users(id) ON DELETE SET NULL, + is_guest INTEGER NOT NULL DEFAULT 0, + created_at TEXT DEFAULT (datetime('now')), + updated_at TEXT DEFAULT (datetime('now')) + ) + `.execute(db); + + await sql` + CREATE UNIQUE INDEX idx_bylines_user_id_unique + ON _emdash_bylines (user_id) WHERE user_id IS NOT NULL + `.execute(db); + await sql`CREATE INDEX idx_bylines_slug ON _emdash_bylines(slug)`.execute(db); + await sql`CREATE INDEX idx_bylines_display_name ON _emdash_bylines(display_name)`.execute(db); + + await sql` + CREATE TABLE _emdash_content_bylines ( + id TEXT PRIMARY KEY, + collection_slug TEXT NOT NULL, + content_id TEXT NOT NULL, + byline_id TEXT NOT NULL REFERENCES _emdash_bylines(id) ON DELETE CASCADE, + sort_order INTEGER NOT NULL DEFAULT 0, + role_label TEXT, + created_at TEXT DEFAULT (datetime('now')), + UNIQUE(collection_slug, content_id, byline_id) + ) + `.execute(db); + + await sql` + CREATE INDEX idx_content_bylines_content + ON _emdash_content_bylines(collection_slug, content_id, sort_order) + `.execute(db); + await sql` + CREATE INDEX idx_content_bylines_byline + ON _emdash_content_bylines(byline_id) + `.execute(db); + + await sql` + CREATE TABLE _emdash_collections ( + slug TEXT PRIMARY KEY + ) + `.execute(db); + + await up040(db); +} + +async function tableNames(db: Kysely): Promise> { + const tables = await db.introspection.getTables(); + return new Set(tables.map((t) => t.name)); +} + +async function indexNames(db: Kysely): Promise> { + const rows = await sql<{ name: string }>` + SELECT name FROM sqlite_master WHERE type = 'index' AND name LIKE 'idx_%' + `.execute(db); + return new Set(rows.rows.map((r) => r.name)); +} + +async function getVersionRow(db: Kysely): Promise { + const row = await sql<{ value: string }>` + SELECT value FROM options WHERE name = 'byline_fields_version' + `.execute(db); + return row.rows[0]?.value ?? null; +} + +describe("041_byline_fields migration", () => { + let db: Kysely; + + beforeEach(async () => { + db = createDatabase({ url: ":memory:" }); + await seedPostMigration040Schema(db); + }); + + afterEach(async () => { + await db.destroy(); + }); + + describe("up()", () => { + it("creates the three byline-field tables", async () => { + await up(db); + + const names = await tableNames(db); + expect(names).toContain("_emdash_byline_fields"); + expect(names).toContain("_emdash_byline_field_values"); + expect(names).toContain("_emdash_byline_field_group_values"); + }); + + it("creates expected indexes", async () => { + await up(db); + + const names = await indexNames(db); + expect(names).toContain("idx__emdash_byline_fields_sort_order"); + expect(names).toContain("idx__emdash_byline_field_values_byline"); + expect(names).toContain("idx__emdash_byline_field_values_field"); + expect(names).toContain("idx__emdash_byline_field_group_values_group"); + expect(names).toContain("idx__emdash_byline_field_group_values_field"); + }); + + it("inserts the byline_fields_version counter into options", async () => { + await up(db); + + // `options.value` stores JSON; the initial counter is the JSON + // literal `0`. Phase 3's cache parses with JSON.parse and reads + // it as `number 0`. + expect(await getVersionRow(db)).toBe("0"); + }); + + it("_emdash_byline_fields enforces slug uniqueness", async () => { + await up(db); + + await sql` + INSERT INTO _emdash_byline_fields (id, slug, label, type) + VALUES ('f1', 'job_title', 'Job title', 'string') + `.execute(db); + + await expect( + sql` + INSERT INTO _emdash_byline_fields (id, slug, label, type) + VALUES ('f2', 'job_title', 'Other label', 'string') + `.execute(db), + ).rejects.toThrow(); + }); + + it("translatable defaults to 1 (per-locale storage is the default)", async () => { + await up(db); + + await sql` + INSERT INTO _emdash_byline_fields (id, slug, label, type) + VALUES ('f1', 'job_title', 'Job title', 'string') + `.execute(db); + + const row = await sql<{ translatable: number; required: number; sort_order: number }>` + SELECT translatable, required, sort_order FROM _emdash_byline_fields WHERE id = 'f1' + `.execute(db); + expect(row.rows[0]?.translatable).toBe(1); + expect(row.rows[0]?.required).toBe(0); + expect(row.rows[0]?.sort_order).toBe(0); + }); + + it("_emdash_byline_field_values composite PK rejects duplicate (byline_id, field_id)", async () => { + await up(db); + + await sql` + INSERT INTO _emdash_bylines (id, slug, display_name, locale, translation_group) + VALUES ('b1', 'jane', 'Jane Doe', 'en', 'b1') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_fields (id, slug, label, type) + VALUES ('f1', 'job_title', 'Job title', 'string') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES ('b1', 'f1', '"Editor"') + `.execute(db); + + await expect( + sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES ('b1', 'f1', '"Senior Editor"') + `.execute(db), + ).rejects.toThrow(); + }); + + it("_emdash_byline_field_group_values composite PK rejects duplicate (translation_group, field_id)", async () => { + await up(db); + + await sql` + INSERT INTO _emdash_byline_fields (id, slug, label, type, translatable) + VALUES ('f1', 'twitter_handle', 'Twitter', 'string', 0) + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES ('g1', 'f1', '"@jane"') + `.execute(db); + + await expect( + sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES ('g1', 'f1', '"@other"') + `.execute(db), + ).rejects.toThrow(); + }); + + it("ON DELETE CASCADE: deleting a byline removes its translatable values", async () => { + await up(db); + + await sql`PRAGMA foreign_keys = ON`.execute(db); + await sql` + INSERT INTO _emdash_bylines (id, slug, display_name, locale, translation_group) + VALUES ('b1', 'jane', 'Jane Doe', 'en', 'b1') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_fields (id, slug, label, type) + VALUES ('f1', 'job_title', 'Job title', 'string') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES ('b1', 'f1', '"Editor"') + `.execute(db); + + await sql`DELETE FROM _emdash_bylines WHERE id = 'b1'`.execute(db); + + const remaining = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_values + `.execute(db); + expect(Number(remaining.rows[0]?.count ?? -1)).toBe(0); + }); + + it("ON DELETE CASCADE: deleting a field removes its translatable AND group-shared values", async () => { + await up(db); + + await sql`PRAGMA foreign_keys = ON`.execute(db); + await sql` + INSERT INTO _emdash_bylines (id, slug, display_name, locale, translation_group) + VALUES ('b1', 'jane', 'Jane Doe', 'en', 'b1') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_fields (id, slug, label, type) + VALUES ('f1', 'job_title', 'Job title', 'string') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES ('b1', 'f1', '"Editor"') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES ('b1', 'f1', '"@jane"') + `.execute(db); + + await sql`DELETE FROM _emdash_byline_fields WHERE id = 'f1'`.execute(db); + + const trCount = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_values + `.execute(db); + expect(Number(trCount.rows[0]?.count ?? -1)).toBe(0); + const grpCount = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_group_values + `.execute(db); + expect(Number(grpCount.rows[0]?.count ?? -1)).toBe(0); + }); + + it("is idempotent on partial re-application (table-exists guard)", async () => { + await up(db); + + // Simulate a crashed partial prior run: tables exist but the + // version row is gone. `INSERT … ON CONFLICT DO NOTHING` should + // leave existing state untouched if present, restore if missing. + await sql`DELETE FROM options WHERE name = 'byline_fields_version'`.execute(db); + + await expect(up(db)).resolves.not.toThrow(); + expect(await getVersionRow(db)).toBe("0"); + }); + + it("recreates missing indexes when a crash dropped them mid-up()", async () => { + // Realistic partial-failure scenario: CREATE TABLE landed in the + // failed pass, CREATE INDEX did not. A coarse table-level guard + // would skip the index on retry; per-statement `.ifNotExists()` + // on the createIndex calls keeps the migration retry-safe (Phase + // 1 AC: "applies cleanly … on a DB with bylines+content from + // #1146" includes the partial-failure case). + await up(db); + await sql`DROP INDEX idx__emdash_byline_field_values_byline`.execute(db); + await sql`DROP INDEX idx__emdash_byline_field_group_values_group`.execute(db); + + await expect(up(db)).resolves.not.toThrow(); + + const names = await indexNames(db); + expect(names).toContain("idx__emdash_byline_field_values_byline"); + expect(names).toContain("idx__emdash_byline_field_group_values_group"); + }); + + it("preserves a non-zero version counter on re-application", async () => { + await up(db); + // Simulate post-cache-bump state: version has been incremented by + // a registry mutation (Phase 2). A second `up()` (recovery path) + // must not reset it. + await sql` + UPDATE options SET value = '5' WHERE name = 'byline_fields_version' + `.execute(db); + + await up(db); + + expect(await getVersionRow(db)).toBe("5"); + }); + }); + + describe("down()", () => { + it("drops all three tables and removes the version row", async () => { + await up(db); + await down(db); + + const names = await tableNames(db); + expect(names).not.toContain("_emdash_byline_fields"); + expect(names).not.toContain("_emdash_byline_field_values"); + expect(names).not.toContain("_emdash_byline_field_group_values"); + expect(await getVersionRow(db)).toBeNull(); + }); + + it("up -> down -> up settles to the same state as a single up", async () => { + await up(db); + await down(db); + await up(db); + + const names = await tableNames(db); + expect(names).toContain("_emdash_byline_fields"); + expect(names).toContain("_emdash_byline_field_values"); + expect(names).toContain("_emdash_byline_field_group_values"); + expect(await getVersionRow(db)).toBe("0"); + }); + + it("tolerates running on a partially-applied up() (missing tables)", async () => { + // No tables created — down() should still be a no-op rather than throw. + await expect(down(db)).resolves.not.toThrow(); + }); + }); +}); From d4f6b8764c2d1093ae4de0c88a03b5ce3a20aa62 Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 14:44:45 +0100 Subject: [PATCH 03/35] test(migrations): assert byline-field tables in dialect-compat fresh-run --- .../core/tests/integration/database/dialect-compat.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/core/tests/integration/database/dialect-compat.test.ts b/packages/core/tests/integration/database/dialect-compat.test.ts index 7860f6acf..cddf1b840 100644 --- a/packages/core/tests/integration/database/dialect-compat.test.ts +++ b/packages/core/tests/integration/database/dialect-compat.test.ts @@ -63,6 +63,9 @@ describeEachDialect("Migrations", (dialect) => { "_emdash_sections", "_emdash_bylines", "_emdash_content_bylines", + "_emdash_byline_fields", + "_emdash_byline_field_values", + "_emdash_byline_field_group_values", ]; for (const table of tables) { From b41750c77faa3a6b09e444266c89eea22e5a3e74 Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 14:44:54 +0100 Subject: [PATCH 04/35] feat(bylines): BylineSchemaRegistry with version counter --- packages/core/src/schema/byline-registry.ts | 590 ++++++++++++++++++ .../database/byline-fields-races.test.ts | 147 +++++ .../tests/unit/schema/byline-registry.test.ts | 536 ++++++++++++++++ 3 files changed, 1273 insertions(+) create mode 100644 packages/core/src/schema/byline-registry.ts create mode 100644 packages/core/tests/integration/database/byline-fields-races.test.ts create mode 100644 packages/core/tests/unit/schema/byline-registry.test.ts diff --git a/packages/core/src/schema/byline-registry.ts b/packages/core/src/schema/byline-registry.ts new file mode 100644 index 000000000..884a134fc --- /dev/null +++ b/packages/core/src/schema/byline-registry.ts @@ -0,0 +1,590 @@ +import type { Kysely } from "kysely"; +import { sql } from "kysely"; +import { ulid } from "ulidx"; + +import { withTransaction } from "../database/transaction.js"; +import type { BylineFieldTable, Database } from "../database/types.js"; +import { validateIdentifier } from "../database/validate.js"; +import { + BYLINE_FIELD_TYPES, + RESERVED_BYLINE_FIELD_SLUGS, + type BylineFieldDefinition, + type BylineFieldType, + type BylineFieldValidation, + type CreateBylineFieldInput, + type UpdateBylineFieldInput, +} from "./types.js"; + +const RESERVED_SET: ReadonlySet = new Set(RESERVED_BYLINE_FIELD_SLUGS); +const TYPE_SET: ReadonlySet = new Set(BYLINE_FIELD_TYPES); + +const VERSION_KEY = "byline_fields_version"; + +/** Hard cap on the choices array for a `select`-type field. */ +const MAX_SELECT_OPTIONS = 200; +/** Hard cap on a slug — mirrors `SchemaRegistry.validateSlug`. */ +const MAX_SLUG_LENGTH = 63; +/** Hard cap on a label. Bigger than slugs because labels are display strings. */ +const MAX_LABEL_LENGTH = 200; + +/** + * Error thrown for byline-schema validation failures. Mirrors + * `SchemaError` in `registry.ts` so the admin API layer can map a small + * set of codes to HTTP statuses without inspecting messages. + * + * Codes: + * - `INVALID_SLUG` — slug fails identifier rules or length cap + * - `RESERVED_SLUG` — slug collides with a fixed `_emdash_bylines` column + * - `INVALID_TYPE` — type is not one of the five v1 field types + * - `INVALID_LABEL` — label missing or exceeds length cap + * - `INVALID_VALIDATION` — validation payload malformed (e.g. `select` with + * no `options`, duplicates in `options`) + * - `FIELD_EXISTS` — slug already registered + * - `FIELD_NOT_FOUND` — slug not registered + * - `TRANSLATABLE_LOCKED` — attempt to flip `translatable` while stored + * values reference the field + * - `REORDER_MISMATCH` — reorder input doesn't match the registered set + */ +export class BylineSchemaError extends Error { + constructor( + message: string, + public code: string, + public details?: Record, + ) { + super(message); + this.name = "BylineSchemaError"; + } +} + +/** + * Registry for byline custom fields (Discussion #1174). + * + * Owns CRUD over `_emdash_byline_fields` plus the persisted + * `options.byline_fields_version` counter that the field-defs cache reads. + * Every mutation bumps the counter in a single set-based UPDATE so the + * increment is atomic on every supported dialect (SQLite serialises + * writes; D1 inherits that; Postgres handles concurrent UPDATEs via row- + * level locking). + * + * **Bump-before-mutate ordering (D1 partial-failure safety).** D1 has no + * transactions; `withTransaction` falls back to direct execution. Every + * mutation bumps the version counter *before* applying the schema change + * — not after — so a crash between the two statements leaves the system + * in a recoverable state: + * + * - Crash before bump: nothing happened. + * - Crash after bump but before mutation: the schema is unchanged but + * caches invalidate on the higher version. They refetch the (unchanged) + * field list once. The original caller retries the mutation, which + * bumps again and lands. Net cost: one spurious cache refresh. + * + * The bump-after order would leave caches *permanently* stale on a crash + * between the mutation and the bump — the schema would have changed but + * the cache would have no signal to refetch, and a retry of the same + * op would throw `FIELD_EXISTS` / `FIELD_NOT_FOUND` without reaching the + * bump line. Bump-before trades that for a smaller, self-healing reader + * race during the mutation window. + * + * **Application-level cascade in `deleteField`.** Migration 041 declares + * FK ON DELETE CASCADE on both value tables, and at runtime SQLite + D1 + + * Postgres all enforce it. `deleteField` *also* deletes value rows + * explicitly inside the same `withTransaction` block as a defense-in- + * depth measure — the explicit DELETE is independent of FK pragma config + * (useful for tests and ad-hoc scripts that bypass `connection.ts`), + * mirrors the precedent set by `BylineRepository.delete` (which had to + * implement app-level cascade because migration 040 dropped its FK + * entirely), and makes the deletion semantics readable at the call site. + * + * The registry intentionally does not touch the value tables + * (`_emdash_byline_field_values`, `_emdash_byline_field_group_values`) + * *during write operations on byline values* — those are owned by + * `BylineRepository.update` (Phase 3). `deleteField`'s value-row cleanup + * is a schema-side concern (removing definitions), not a value-side one. + * + * Reserved-slug rejection happens at the API layer (zod schema, Phase 4) + * *and* here — the registry never trusts callers to have already + * validated. The double check keeps the registry safe to use from + * non-HTTP code paths (seeds, scripts, future internal callers). + */ +export class BylineSchemaRegistry { + constructor(private db: Kysely) {} + + async listFields(): Promise { + const rows = await this.db + .selectFrom("_emdash_byline_fields") + .selectAll() + .orderBy("sort_order", "asc") + .orderBy("created_at", "asc") + .execute(); + return rows.map((row) => mapFieldRow(row)); + } + + async getField(slug: string): Promise { + const row = await this.db + .selectFrom("_emdash_byline_fields") + .selectAll() + .where("slug", "=", slug) + .executeTakeFirst(); + return row ? mapFieldRow(row) : null; + } + + async getFieldById(id: string): Promise { + const row = await this.db + .selectFrom("_emdash_byline_fields") + .selectAll() + .where("id", "=", id) + .executeTakeFirst(); + return row ? mapFieldRow(row) : null; + } + + async createField(input: CreateBylineFieldInput): Promise { + this.validateSlug(input.slug); + this.validateLabel(input.label); + this.validateType(input.type); + const validation = this.normaliseValidation(input.type, input.validation ?? null); + + const existing = await this.getField(input.slug); + if (existing) { + throw new BylineSchemaError(`Byline field "${input.slug}" already exists`, "FIELD_EXISTS", { + slug: input.slug, + }); + } + + const id = ulid(); + const sortOrder = input.sortOrder ?? (await this.nextSortOrder()); + + // Bump-before-mutate: see class JSDoc for the D1 partial-failure + // rationale. All validation has already run above; the only way the + // INSERT can fail now is a concurrent insert of the same slug + // (UNIQUE constraint), which leaves the bump as a spurious + // cache-refresh signal — strictly better than a stale cache. + await this.bumpVersion(); + + await this.db + .insertInto("_emdash_byline_fields") + .values({ + id, + slug: input.slug, + label: input.label, + type: input.type, + required: input.required ? 1 : 0, + translatable: input.translatable === false ? 0 : 1, + validation: validation ? JSON.stringify(validation) : null, + sort_order: sortOrder, + }) + .execute(); + + const created = await this.getFieldById(id); + if (!created) { + // Should be unreachable on a working DB — but a typed error + // beats letting the route returning null on a successful path. + throw new BylineSchemaError("Failed to load created field", "FIELD_NOT_FOUND", { + id, + }); + } + return created; + } + + async updateField(slug: string, input: UpdateBylineFieldInput): Promise { + const field = await this.getField(slug); + if (!field) { + throw new BylineSchemaError(`Byline field "${slug}" not found`, "FIELD_NOT_FOUND", { + slug, + }); + } + + const updates: Partial<{ + label: string; + required: number; + translatable: number; + validation: string | null; + sort_order: number; + updated_at: string; + }> = {}; + + if (input.label !== undefined) { + this.validateLabel(input.label); + updates.label = input.label; + } + + if (input.required !== undefined) { + updates.required = input.required ? 1 : 0; + } + + if (input.validation !== undefined) { + // Validation payload is normalised against the *current* field + // type — `type` is not updatable, so it's safe to use `field.type`. + const validation = this.normaliseValidation(field.type, input.validation); + updates.validation = validation ? JSON.stringify(validation) : null; + } + + if (input.translatable !== undefined && input.translatable !== field.translatable) { + // Flipping `translatable` would orphan any values already stored + // in the table matching the *current* flag. Reject when any + // value rows reference this field — admins can delete the field + // (cascading the values) and re-create it with the new flag if + // they want a clean re-start. Migrating values across tables is + // out of scope (Discussion #1174 doesn't authorise it). + const usage = await this.countFieldValues(field.id); + if (usage > 0) { + throw new BylineSchemaError( + `Cannot change "translatable" on field "${slug}" while ${usage} value row(s) exist. ` + + `Delete the values (or the field) and re-create with the new setting.`, + "TRANSLATABLE_LOCKED", + { slug, valueCount: usage }, + ); + } + updates.translatable = input.translatable ? 1 : 0; + } + + if (input.sortOrder !== undefined) { + updates.sort_order = input.sortOrder; + } + + if (Object.keys(updates).length === 0) { + // No-op update — still return the current field shape so callers + // don't have to special-case "nothing changed". Don't bump the + // version counter: caches stay valid. + return field; + } + + updates.updated_at = new Date().toISOString(); + + // Bump-before-mutate: see class JSDoc. All validation (including + // translatable-flip safety) has run; the UPDATE is idempotent, so a + // crash here recovers cleanly on retry. + await this.bumpVersion(); + + await this.db + .updateTable("_emdash_byline_fields") + .set(updates) + .where("id", "=", field.id) + .execute(); + + const updated = await this.getFieldById(field.id); + if (!updated) { + throw new BylineSchemaError("Failed to load updated field", "FIELD_NOT_FOUND", { + slug, + }); + } + return updated; + } + + async deleteField(slug: string): Promise { + const field = await this.getField(slug); + if (!field) { + throw new BylineSchemaError(`Byline field "${slug}" not found`, "FIELD_NOT_FOUND", { + slug, + }); + } + + // Bump-before-mutate. See class JSDoc. + await this.bumpVersion(); + + // Application-level cascade. The FKs in migration 041 already + // ON DELETE CASCADE, but doing it explicitly here: + // 1. removes the dependency on `PRAGMA foreign_keys = ON` + // (set in production via `connection.ts:60`, but easy to miss + // in tests and one-off scripts); + // 2. mirrors `BylineRepository.delete`'s app-level cascade, which + // had to be implemented when migration 040 dropped its FK; + // 3. makes the cleanup ordering explicit on D1 — value rows first, + // definition row last, so a crash leaves the definition still + // present (deletable on retry) rather than orphan values + // pointing at a vanished definition id. + // + // withTransaction gives us real atomicity on Node SQLite + PG; + // on D1 it falls back to direct sequenced execution. + await withTransaction(this.db, async (trx) => { + await trx + .deleteFrom("_emdash_byline_field_values") + .where("field_id", "=", field.id) + .execute(); + await trx + .deleteFrom("_emdash_byline_field_group_values") + .where("field_id", "=", field.id) + .execute(); + await trx.deleteFrom("_emdash_byline_fields").where("id", "=", field.id).execute(); + }); + } + + /** + * Reorder fields by slug. The input must be the *exact* set of + * currently registered slugs — no adds, no drops, no duplicates. This + * keeps the operation invertible (any reorder is followed by a reverse + * reorder) and removes a class of "did I forget a field?" bugs at the + * API layer. + */ + async reorderFields(slugs: string[]): Promise { + if (new Set(slugs).size !== slugs.length) { + throw new BylineSchemaError("Reorder input contains duplicate slugs", "REORDER_MISMATCH", { + slugs, + }); + } + + const registered = await this.listFields(); + const registeredSlugs = registered.map((f) => f.slug).toSorted(); + const inputSlugs = slugs.toSorted(); + + if (registeredSlugs.length !== inputSlugs.length) { + throw new BylineSchemaError( + `Reorder input has ${inputSlugs.length} slug(s); ${registeredSlugs.length} registered`, + "REORDER_MISMATCH", + { registered: registeredSlugs, input: inputSlugs }, + ); + } + for (let i = 0; i < registeredSlugs.length; i++) { + if (registeredSlugs[i] !== inputSlugs[i]) { + throw new BylineSchemaError( + "Reorder input does not match the registered field set", + "REORDER_MISMATCH", + { registered: registeredSlugs, input: inputSlugs }, + ); + } + } + + // Bump-before-mutate: see class JSDoc. The reorder loop is + // idempotent (each statement writes a fixed sort_order to a fixed + // slug), so a crash mid-loop recovers cleanly on retry. + await this.bumpVersion(); + + // Update sort_order for each field. The loop is per-slug rather than + // a single CASE expression because the per-statement form is dialect- + // agnostic and tiny field sets (typical: 3–10) make the cost + // irrelevant. Mirrors `SchemaRegistry.reorderFields`. Wrapped in + // withTransaction for real atomicity on Node SQLite + PG. + const now = new Date().toISOString(); + await withTransaction(this.db, async (trx) => { + for (let i = 0; i < slugs.length; i++) { + const slug = slugs[i]; + if (slug === undefined) continue; + await trx + .updateTable("_emdash_byline_fields") + .set({ sort_order: i, updated_at: now }) + .where("slug", "=", slug) + .execute(); + } + }); + } + + /** + * Read the persisted version counter. Used by the field-defs cache + * (Phase 3) to detect invalidation. Returns `0` when the row is + * missing — covers the "tests that didn't run migration 041" case + * without throwing. + */ + async getVersion(): Promise { + const row = await this.db + .selectFrom("options") + .select("value") + .where("name", "=", VERSION_KEY) + .executeTakeFirst(); + if (!row) return 0; + const parsed = Number.parseInt(row.value, 10); + return Number.isFinite(parsed) ? parsed : 0; + } + + // ============================================ + // Private helpers + // ============================================ + + /** + * Atomic version increment. A single set-based UPDATE means every + * concurrent mutation lands as its own increment — no read-modify-write + * window. SQLite + D1 serialise writes; Postgres handles the contention + * via row-level locking on the `options` row. + * + * Stored as a JSON number literal (`5`, not `"5"`) so `JSON.parse` reads + * it back as a number on the cache path. + */ + private async bumpVersion(): Promise { + await sql` + UPDATE options + SET value = CAST(CAST(value AS INTEGER) + 1 AS TEXT) + WHERE name = ${VERSION_KEY} + `.execute(this.db); + } + + private async nextSortOrder(): Promise { + const row = await this.db + .selectFrom("_emdash_byline_fields") + .select(({ fn }) => [fn.max("sort_order").as("max")]) + .executeTakeFirst(); + const max = row?.max ?? null; + return max === null ? 0 : max + 1; + } + + private async countFieldValues(fieldId: string): Promise { + // Count both per-locale and group-shared values. A field can only + // store in one table at a time (translatable picks), but historic + // rows might exist in the other if a prior version of this code + // allowed the flip — count both to be safe. + const tr = await this.db + .selectFrom("_emdash_byline_field_values") + .select(({ fn }) => [fn.count("field_id").as("count")]) + .where("field_id", "=", fieldId) + .executeTakeFirst(); + const grp = await this.db + .selectFrom("_emdash_byline_field_group_values") + .select(({ fn }) => [fn.count("field_id").as("count")]) + .where("field_id", "=", fieldId) + .executeTakeFirst(); + return Number(tr?.count ?? 0) + Number(grp?.count ?? 0); + } + + private validateSlug(slug: string): void { + if (!slug || typeof slug !== "string") { + throw new BylineSchemaError("Byline field slug is required", "INVALID_SLUG", { slug }); + } + if (slug.length > MAX_SLUG_LENGTH) { + throw new BylineSchemaError( + `Byline field slug must be ${MAX_SLUG_LENGTH} characters or less`, + "INVALID_SLUG", + { slug }, + ); + } + // `validateIdentifier` enforces /^[a-z][a-z0-9_]*$/ — rejects + // camelCase, PascalCase, hyphens, leading digits, and identifiers + // over 128 characters. We hit the 63-char cap above first, which + // matches the content-collection slug cap. + try { + validateIdentifier(slug, "byline field slug"); + } catch (error) { + throw new BylineSchemaError( + error instanceof Error ? error.message : "Invalid byline field slug", + "INVALID_SLUG", + { slug }, + ); + } + if (RESERVED_SET.has(slug)) { + throw new BylineSchemaError(`Byline field slug "${slug}" is reserved`, "RESERVED_SLUG", { + slug, + }); + } + } + + private validateLabel(label: string): void { + if (!label || typeof label !== "string") { + throw new BylineSchemaError("Byline field label is required", "INVALID_LABEL", { + label, + }); + } + if (label.length > MAX_LABEL_LENGTH) { + throw new BylineSchemaError( + `Byline field label must be ${MAX_LABEL_LENGTH} characters or less`, + "INVALID_LABEL", + { length: label.length }, + ); + } + } + + private validateType(type: BylineFieldType): void { + if (!TYPE_SET.has(type)) { + throw new BylineSchemaError( + `Byline field type "${type}" is not supported. Valid types: ${[...TYPE_SET].join(", ")}`, + "INVALID_TYPE", + { type }, + ); + } + } + + /** + * Normalise + validate a validation payload for a given field type. + * + * - `select`: `options` is required, must be a non-empty array of unique + * non-empty strings, capped at `MAX_SELECT_OPTIONS`. + * - any other type: `options` is silently dropped if present (a future + * field type might use it, but v1 doesn't). + * + * Returns `null` when the resulting validation object is empty, so the + * storage column stays NULL rather than carrying `'{}'`. + */ + private normaliseValidation( + type: BylineFieldType, + validation: BylineFieldValidation | null, + ): BylineFieldValidation | null { + if (type === "select") { + const options = validation?.options; + if (!Array.isArray(options) || options.length === 0) { + throw new BylineSchemaError( + `Byline field of type "select" requires non-empty "validation.options"`, + "INVALID_VALIDATION", + { type }, + ); + } + if (options.length > MAX_SELECT_OPTIONS) { + throw new BylineSchemaError( + `Byline field "select" cannot have more than ${MAX_SELECT_OPTIONS} options`, + "INVALID_VALIDATION", + { count: options.length }, + ); + } + const seen = new Set(); + for (const option of options) { + if (typeof option !== "string" || option.length === 0) { + throw new BylineSchemaError( + `Byline field "select" options must be non-empty strings`, + "INVALID_VALIDATION", + { option }, + ); + } + if (seen.has(option)) { + throw new BylineSchemaError( + `Byline field "select" options must be unique`, + "INVALID_VALIDATION", + { option }, + ); + } + seen.add(option); + } + return { options }; + } + + if (validation == null) return null; + // Non-select: drop `options` if present. Strip nothing else — future + // field types might extend the shape and we don't want to lose + // payload silently. Today's `BylineFieldValidation` is `{ options? }` + // only, so this branch is a pass-through; left explicit for clarity. + const { options: _drop, ...rest } = validation; + return Object.keys(rest).length === 0 ? null : (rest as BylineFieldValidation); + } +} + +function mapFieldRow(row: { + id: string; + slug: string; + label: string; + type: string; + required: number; + translatable: number; + validation: string | null; + sort_order: number; + created_at: string; + updated_at: string; +}): BylineFieldDefinition { + return { + id: row.id, + slug: row.slug, + label: row.label, + // `type` is stored as TEXT but `createField` rejects anything outside + // `BYLINE_FIELD_TYPES` before inserting. The assertion narrows on + // that write-time guarantee. + // eslint-disable-next-line typescript/no-unsafe-type-assertion -- validated at write + type: row.type as BylineFieldType, + required: row.required === 1, + translatable: row.translatable === 1, + // `validation` is JSON-encoded `BylineFieldValidation | null`, written + // only through `normaliseValidation`. The cast matches the + // `JSON.parse(...) as T` pattern in `OptionsRepository`. + // eslint-disable-next-line typescript/no-unsafe-type-assertion -- validated at write + validation: row.validation ? (JSON.parse(row.validation) as BylineFieldValidation) : null, + sortOrder: row.sort_order, + createdAt: row.created_at, + updatedAt: row.updated_at, + }; +} + +// Re-export the table type for callers that want to spell it explicitly. +// Most callers should rely on the Database interface; this is convenience +// for tests that hand-roll Kysely queries. +export type { BylineFieldTable }; diff --git a/packages/core/tests/integration/database/byline-fields-races.test.ts b/packages/core/tests/integration/database/byline-fields-races.test.ts new file mode 100644 index 000000000..795e6790a --- /dev/null +++ b/packages/core/tests/integration/database/byline-fields-races.test.ts @@ -0,0 +1,147 @@ +/** + * Byline field registry — concurrent-mutation safety + * + * Phase 2 of Discussion #1174 mandates that every mutation on + * `BylineSchemaRegistry` bumps `options.byline_fields_version` atomically, + * so two concurrent mutations don't collapse into one increment. The + * registry implements this with a single set-based UPDATE + * (`SET value = CAST(CAST(value AS INTEGER) + 1 AS TEXT)`); this suite + * proves that contract end-to-end on every supported dialect. + * + * SQLite + D1: writes are serialised at the database level, so "concurrent" + * here means "fired in parallel via `Promise.all`" — the test verifies the + * code path is set-based rather than read-modify-write (the latter would + * deadlock or drop increments under serialisation). + * + * Postgres: real row-level concurrency. The atomic UPDATE relies on PG's + * default `READ COMMITTED` isolation behaviour on UPDATE statements, + * which acquires a row-level lock for the duration of the statement. + * Concurrent UPDATEs serialise behind the lock; each one observes the + * latest committed value and applies its +1. + * + * Activate Postgres parity by exporting `EMDASH_TEST_PG=1` and pointing + * `PG_CONNECTION_STRING` at a writable test database. + */ + +import { beforeEach, afterEach, expect, it } from "vitest"; + +import { BylineSchemaRegistry } from "../../../src/schema/byline-registry.js"; +import { + describeEachDialect, + setupForDialect, + teardownForDialect, + type DialectTestContext, +} from "../../utils/test-db.js"; + +describeEachDialect("BylineSchemaRegistry concurrency", (dialect) => { + let ctx: DialectTestContext; + let registry: BylineSchemaRegistry; + + beforeEach(async () => { + ctx = await setupForDialect(dialect); + registry = new BylineSchemaRegistry(ctx.db); + }); + + afterEach(async () => { + await teardownForDialect(ctx); + }); + + it("parallel createField calls each land their own version increment", async () => { + const startVersion = await registry.getVersion(); + const slugs = Array.from({ length: 10 }, (_, i) => `field_${i}`); + + const results = await Promise.allSettled( + slugs.map((slug) => registry.createField({ slug, label: slug, type: "string" })), + ); + + // Every create either succeeded (FIELD_EXISTS shouldn't fire because + // slugs are unique) — collect successes and count them. + const succeeded = results.filter((r) => r.status === "fulfilled").length; + expect(succeeded).toBe(slugs.length); + + const endVersion = await registry.getVersion(); + expect(endVersion - startVersion).toBe(slugs.length); + }); + + it("parallel updateField calls don't lose version increments", async () => { + const field = await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + }); + const baseline = await registry.getVersion(); + + const labels = Array.from({ length: 10 }, (_, i) => `Label ${i}`); + await Promise.allSettled(labels.map((label) => registry.updateField("job_title", { label }))); + + const after = await registry.getVersion(); + // Every update is non-trivial (label changes), so every call should + // have bumped the counter. PG: serialised by the row-level lock on + // `_emdash_byline_fields.id` during UPDATE; SQLite: serialised by + // the database lock. Either way, increments are not lost. + expect(after - baseline).toBe(labels.length); + + // And the final state is a single, well-defined row — no duplicate + // definitions, label is one of the inputs. + const reloaded = await registry.getField("job_title"); + expect(reloaded?.id).toBe(field.id); + expect(labels).toContain(reloaded?.label); + }); + + it("mixed parallel mutations all bump the version", async () => { + await registry.createField({ slug: "a", label: "A", type: "string" }); + await registry.createField({ slug: "b", label: "B", type: "string" }); + const baseline = await registry.getVersion(); + + // 3 mutations in parallel: one create, one update, one reorder. None + // of them conflict — they target different rows or properties. + const ops: Array> = [ + registry.createField({ slug: "c", label: "C", type: "string" }), + registry.updateField("a", { label: "Aa" }), + ]; + + await Promise.all(ops); + // Reorder runs serially after both — it reads the full set so it + // can't safely race against parallel creates. (The registry itself + // permits the race; this test just keeps the assertion meaningful.) + await registry.reorderFields(["c", "a", "b"]); + + const after = await registry.getVersion(); + expect(after - baseline).toBe(3); + }); + + it("parallel deletes against distinct fields don't lose increments", async () => { + for (let i = 0; i < 6; i++) { + await registry.createField({ slug: `del_${i}`, label: `del_${i}`, type: "string" }); + } + const baseline = await registry.getVersion(); + + await Promise.all(Array.from({ length: 6 }, (_, i) => registry.deleteField(`del_${i}`))); + + const after = await registry.getVersion(); + expect(after - baseline).toBe(6); + expect((await registry.listFields()).map((f) => f.slug)).not.toContain("del_0"); + }); + + it("createField duplicate slugs: one succeeds, the other surfaces FIELD_EXISTS", async () => { + const baseline = await registry.getVersion(); + + // Fire two creates with the same slug. On SQLite/D1 (serialised + // writes) one will land first and the second will hit the + // FIELD_EXISTS check. On PG the same property holds via the UNIQUE + // index on slug — the loser sees a UNIQUE constraint error, but the + // registry's getField pre-check catches the racy case for most runs. + // We assert the *outcome*: exactly one row exists and the version + // counter advanced by at least one (the winner). + const results = await Promise.allSettled([ + registry.createField({ slug: "dupe", label: "Dupe A", type: "string" }), + registry.createField({ slug: "dupe", label: "Dupe B", type: "string" }), + ]); + + const succeeded = results.filter((r) => r.status === "fulfilled"); + expect(succeeded.length).toBeGreaterThanOrEqual(1); + const rows = await registry.listFields(); + expect(rows.filter((f) => f.slug === "dupe")).toHaveLength(1); + expect((await registry.getVersion()) - baseline).toBeGreaterThanOrEqual(1); + }); +}); diff --git a/packages/core/tests/unit/schema/byline-registry.test.ts b/packages/core/tests/unit/schema/byline-registry.test.ts new file mode 100644 index 000000000..9f457340b --- /dev/null +++ b/packages/core/tests/unit/schema/byline-registry.test.ts @@ -0,0 +1,536 @@ +import Database from "better-sqlite3"; +import { Kysely, SqliteDialect, sql } from "kysely"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { runMigrations } from "../../../src/database/migrations/runner.js"; +import type { Database as EmDashDatabase } from "../../../src/database/types.js"; +import { BylineSchemaError, BylineSchemaRegistry } from "../../../src/schema/byline-registry.js"; +import { RESERVED_BYLINE_FIELD_SLUGS } from "../../../src/schema/types.js"; + +describe("BylineSchemaRegistry", () => { + let db: Kysely; + let registry: BylineSchemaRegistry; + + beforeEach(async () => { + const sqlite = new Database(":memory:"); + db = new Kysely({ + dialect: new SqliteDialect({ database: sqlite }), + }); + await runMigrations(db); + registry = new BylineSchemaRegistry(db); + }); + + afterEach(async () => { + await db.destroy(); + }); + + describe("createField", () => { + it("creates a string field with sensible defaults", async () => { + const field = await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + }); + + expect(field.slug).toBe("job_title"); + expect(field.label).toBe("Job title"); + expect(field.type).toBe("string"); + expect(field.required).toBe(false); + expect(field.translatable).toBe(true); + expect(field.validation).toBeNull(); + expect(field.sortOrder).toBe(0); + expect(field.id).toBeDefined(); + }); + + it("persists required + translatable=false + validation when provided", async () => { + const field = await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + required: true, + translatable: false, + }); + + expect(field.required).toBe(true); + expect(field.translatable).toBe(false); + + // Round-trip via getField to confirm storage. + const reloaded = await registry.getField("twitter_handle"); + expect(reloaded?.required).toBe(true); + expect(reloaded?.translatable).toBe(false); + }); + + it("auto-assigns increasing sort_order when omitted", async () => { + const a = await registry.createField({ slug: "a", label: "A", type: "string" }); + const b = await registry.createField({ slug: "b", label: "B", type: "string" }); + const c = await registry.createField({ slug: "c", label: "C", type: "string" }); + + expect(a.sortOrder).toBe(0); + expect(b.sortOrder).toBe(1); + expect(c.sortOrder).toBe(2); + }); + + it("rejects camelCase slugs with INVALID_SLUG", async () => { + await expect( + registry.createField({ slug: "jobTitle", label: "Job title", type: "string" }), + ).rejects.toMatchObject({ name: "BylineSchemaError", code: "INVALID_SLUG" }); + }); + + it("rejects PascalCase slugs with INVALID_SLUG", async () => { + await expect( + registry.createField({ slug: "JobTitle", label: "Job title", type: "string" }), + ).rejects.toMatchObject({ code: "INVALID_SLUG" }); + }); + + it("rejects slugs with hyphens or leading digits", async () => { + await expect( + registry.createField({ slug: "job-title", label: "Job title", type: "string" }), + ).rejects.toMatchObject({ code: "INVALID_SLUG" }); + await expect( + registry.createField({ slug: "1job", label: "Job", type: "string" }), + ).rejects.toMatchObject({ code: "INVALID_SLUG" }); + }); + + it("rejects every reserved slug with RESERVED_SLUG", async () => { + for (const slug of RESERVED_BYLINE_FIELD_SLUGS) { + await expect( + registry.createField({ slug, label: "Reserved", type: "string" }), + ).rejects.toMatchObject({ code: "RESERVED_SLUG", details: { slug } }); + } + }); + + it("rejects unsupported field types with INVALID_TYPE", async () => { + // `portableText` is a valid content-field type but not a byline + // field type — the registry must reject it at the typed-error + // layer, not at the SQL layer. + await expect( + registry.createField({ + slug: "rich", + label: "Rich", + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- intentionally crossing the type boundary + type: "portableText" as any, + }), + ).rejects.toMatchObject({ code: "INVALID_TYPE" }); + }); + + it("rejects duplicate slugs with FIELD_EXISTS (not a raw SQL UNIQUE error)", async () => { + await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); + + await expect( + registry.createField({ slug: "job_title", label: "Other label", type: "string" }), + ).rejects.toMatchObject({ name: "BylineSchemaError", code: "FIELD_EXISTS" }); + }); + + it("requires non-empty validation.options for select fields", async () => { + await expect( + registry.createField({ slug: "role", label: "Role", type: "select" }), + ).rejects.toMatchObject({ code: "INVALID_VALIDATION" }); + await expect( + registry.createField({ + slug: "role", + label: "Role", + type: "select", + validation: { options: [] }, + }), + ).rejects.toMatchObject({ code: "INVALID_VALIDATION" }); + }); + + it("rejects duplicate or empty select options", async () => { + await expect( + registry.createField({ + slug: "role", + label: "Role", + type: "select", + validation: { options: ["a", "a", "b"] }, + }), + ).rejects.toMatchObject({ code: "INVALID_VALIDATION" }); + await expect( + registry.createField({ + slug: "role", + label: "Role", + type: "select", + validation: { options: ["a", ""] }, + }), + ).rejects.toMatchObject({ code: "INVALID_VALIDATION" }); + }); + + it("strips select-only validation from non-select fields", async () => { + const field = await registry.createField({ + slug: "title", + label: "Title", + type: "string", + validation: { options: ["junk"] }, + }); + expect(field.validation).toBeNull(); + }); + }); + + describe("listFields / getField", () => { + it("returns an empty list when no fields are registered", async () => { + expect(await registry.listFields()).toEqual([]); + expect(await registry.getField("anything")).toBeNull(); + }); + + it("orders by sort_order then created_at", async () => { + const a = await registry.createField({ slug: "a", label: "A", type: "string" }); + const b = await registry.createField({ slug: "b", label: "B", type: "string" }); + + // Manually reorder by sort_order to verify ordering. + await registry.reorderFields(["b", "a"]); + const list = await registry.listFields(); + expect(list.map((f) => f.slug)).toEqual(["b", "a"]); + expect(a.id).not.toBe(b.id); + }); + }); + + describe("updateField", () => { + it("updates label + required + validation and bumps the version counter", async () => { + const before = await registry.getVersion(); + const created = await registry.createField({ + slug: "role", + label: "Role", + type: "select", + validation: { options: ["editor", "author"] }, + }); + const afterCreate = await registry.getVersion(); + + const updated = await registry.updateField("role", { + label: "Role (renamed)", + required: true, + validation: { options: ["editor", "author", "guest"] }, + }); + + expect(updated.label).toBe("Role (renamed)"); + expect(updated.required).toBe(true); + expect(updated.validation?.options).toEqual(["editor", "author", "guest"]); + expect(updated.id).toBe(created.id); + expect(await registry.getVersion()).toBeGreaterThan(afterCreate); + expect(afterCreate).toBeGreaterThan(before); + }); + + it("no-op updates return the field without bumping the version", async () => { + await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); + const v = await registry.getVersion(); + + const result = await registry.updateField("job_title", {}); + + expect(result.slug).toBe("job_title"); + expect(await registry.getVersion()).toBe(v); + }); + + it("rejects unknown slugs with FIELD_NOT_FOUND", async () => { + await expect(registry.updateField("missing", { label: "x" })).rejects.toMatchObject({ + code: "FIELD_NOT_FOUND", + }); + }); + + it("allows flipping translatable when no values exist", async () => { + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + translatable: false, + }); + + const flipped = await registry.updateField("twitter_handle", { translatable: true }); + expect(flipped.translatable).toBe(true); + }); + + it("rejects flipping translatable when per-locale values exist", async () => { + await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); + const field = await registry.getField("job_title"); + + // Seed a byline + a per-locale value. + await sql` + INSERT INTO _emdash_bylines (id, slug, display_name, locale, translation_group) + VALUES ('b1', 'jane', 'Jane', 'en', 'b1') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES ('b1', ${field?.id}, '"Editor"') + `.execute(db); + + await expect( + registry.updateField("job_title", { translatable: false }), + ).rejects.toMatchObject({ code: "TRANSLATABLE_LOCKED" }); + }); + + it("rejects flipping translatable when group-shared values exist", async () => { + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + translatable: false, + }); + const field = await registry.getField("twitter_handle"); + + await sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES ('g1', ${field?.id}, '"@jane"') + `.execute(db); + + await expect( + registry.updateField("twitter_handle", { translatable: true }), + ).rejects.toMatchObject({ code: "TRANSLATABLE_LOCKED" }); + }); + }); + + describe("deleteField", () => { + it("removes the field and bumps the version counter", async () => { + await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); + const v = await registry.getVersion(); + + await registry.deleteField("job_title"); + + expect(await registry.getField("job_title")).toBeNull(); + expect(await registry.getVersion()).toBeGreaterThan(v); + }); + + it("clears values via application-level cascade (works without FK pragma)", async () => { + await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); + const field = await registry.getField("job_title"); + + // Explicitly leave FK enforcement OFF (better-sqlite3 default in + // the test connection) to prove the cleanup is app-level, not + // FK-dependent. Production (`connection.ts:60`) and D1 keep FK + // ON; this test verifies the registry doesn't *rely* on that. + await sql`PRAGMA foreign_keys = OFF`.execute(db); + await sql` + INSERT INTO _emdash_bylines (id, slug, display_name, locale, translation_group) + VALUES ('b1', 'jane', 'Jane', 'en', 'b1') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES ('b1', ${field?.id}, '"Editor"') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES ('b1', ${field?.id}, '"@jane"') + `.execute(db); + + await registry.deleteField("job_title"); + + const tr = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_values + `.execute(db); + const grp = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_group_values + `.execute(db); + expect(Number(tr.rows[0]?.count ?? -1)).toBe(0); + expect(Number(grp.rows[0]?.count ?? -1)).toBe(0); + }); + + it("FK ON DELETE CASCADE still serves as defense-in-depth", async () => { + // Companion test to the app-level cascade above: with FK ON, + // even if a future regression removed the app-level DELETEs the + // schema constraint would catch it. Useful contract to keep + // asserted so neither layer rots. + await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); + const field = await registry.getField("job_title"); + + await sql`PRAGMA foreign_keys = ON`.execute(db); + await sql` + INSERT INTO _emdash_bylines (id, slug, display_name, locale, translation_group) + VALUES ('b1', 'jane', 'Jane', 'en', 'b1') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES ('b1', ${field?.id}, '"Editor"') + `.execute(db); + + // Bypass the registry — DELETE the definition row directly, + // simulating the layered FK fallback. Values should still be gone. + await sql`DELETE FROM _emdash_byline_fields WHERE id = ${field?.id}`.execute(db); + + const tr = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_values + `.execute(db); + expect(Number(tr.rows[0]?.count ?? -1)).toBe(0); + }); + + it("rejects unknown slugs with FIELD_NOT_FOUND", async () => { + await expect(registry.deleteField("missing")).rejects.toMatchObject({ + code: "FIELD_NOT_FOUND", + }); + }); + }); + + describe("reorderFields", () => { + it("rewrites sort_order to match the input order and bumps the version", async () => { + await registry.createField({ slug: "a", label: "A", type: "string" }); + await registry.createField({ slug: "b", label: "B", type: "string" }); + await registry.createField({ slug: "c", label: "C", type: "string" }); + const v = await registry.getVersion(); + + await registry.reorderFields(["c", "a", "b"]); + + const list = await registry.listFields(); + expect(list.map((f) => f.slug)).toEqual(["c", "a", "b"]); + expect(list.map((f) => f.sortOrder)).toEqual([0, 1, 2]); + expect(await registry.getVersion()).toBeGreaterThan(v); + }); + + it("rejects duplicate slugs with REORDER_MISMATCH", async () => { + await registry.createField({ slug: "a", label: "A", type: "string" }); + await registry.createField({ slug: "b", label: "B", type: "string" }); + await expect(registry.reorderFields(["a", "a"])).rejects.toMatchObject({ + code: "REORDER_MISMATCH", + }); + }); + + it("rejects input that adds or drops slugs vs the registered set", async () => { + await registry.createField({ slug: "a", label: "A", type: "string" }); + await registry.createField({ slug: "b", label: "B", type: "string" }); + + await expect(registry.reorderFields(["a"])).rejects.toMatchObject({ + code: "REORDER_MISMATCH", + }); + await expect(registry.reorderFields(["a", "b", "c"])).rejects.toMatchObject({ + code: "REORDER_MISMATCH", + }); + await expect(registry.reorderFields(["a", "c"])).rejects.toMatchObject({ + code: "REORDER_MISMATCH", + }); + }); + + it("empty registered set + empty input is a no-op (does not throw)", async () => { + const v = await registry.getVersion(); + await expect(registry.reorderFields([])).resolves.toBeUndefined(); + // No fields, no version bump warranted — but consistent behaviour + // matters: the version IS bumped because the operation completes. + // Document the observed behaviour rather than over-specifying. + expect(await registry.getVersion()).toBeGreaterThanOrEqual(v); + }); + }); + + describe("version counter", () => { + it("starts at 0 after migration 041", async () => { + expect(await registry.getVersion()).toBe(0); + }); + + it("monotonically increases across mutations", async () => { + const v0 = await registry.getVersion(); + await registry.createField({ slug: "a", label: "A", type: "string" }); + const v1 = await registry.getVersion(); + await registry.updateField("a", { label: "Aa" }); + const v2 = await registry.getVersion(); + await registry.createField({ slug: "b", label: "B", type: "string" }); + const v3 = await registry.getVersion(); + await registry.reorderFields(["b", "a"]); + const v4 = await registry.getVersion(); + await registry.deleteField("a"); + const v5 = await registry.getVersion(); + + expect(v0).toBe(0); + expect(v1).toBe(1); + expect(v2).toBe(2); + expect(v3).toBe(3); + expect(v4).toBe(4); + expect(v5).toBe(5); + }); + + it("getVersion returns 0 when the row is missing", async () => { + await sql`DELETE FROM options WHERE name = 'byline_fields_version'`.execute(db); + expect(await registry.getVersion()).toBe(0); + }); + }); + + describe("bump-before-mutate ordering", () => { + // These tests pin the observable behaviour that the registry bumps + // the version counter *before* it applies the schema mutation. The + // rationale lives in BylineSchemaRegistry's class JSDoc: on D1 (no + // transactions) a crash between bump and mutation leaves the system + // recoverable; the opposite order leaves caches stuck stale. + // + // "Observable behaviour" here means: when a mutation would fail + // for a reason that only becomes visible *after* the bump runs + // (e.g. a UNIQUE-constraint race), the version counter still + // advanced. Tests that assert "version unchanged on validation + // rejection" further confirm that pre-bump validation is correct. + + it("createField bumps the version before INSERT (validation errors do NOT bump)", async () => { + const v0 = await registry.getVersion(); + + // Pre-bump validation: an invalid slug never reaches the bump. + await expect( + registry.createField({ slug: "JobTitle", label: "Job title", type: "string" }), + ).rejects.toMatchObject({ code: "INVALID_SLUG" }); + expect(await registry.getVersion()).toBe(v0); + + // Successful path: bump lands. + await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); + expect(await registry.getVersion()).toBe(v0 + 1); + + // Duplicate-slug check is pre-bump too (we call getField first). + await expect( + registry.createField({ slug: "job_title", label: "Other", type: "string" }), + ).rejects.toMatchObject({ code: "FIELD_EXISTS" }); + expect(await registry.getVersion()).toBe(v0 + 1); + }); + + it("deleteField bumps the version before the DELETE (FIELD_NOT_FOUND does NOT bump)", async () => { + const v0 = await registry.getVersion(); + await expect(registry.deleteField("missing")).rejects.toMatchObject({ + code: "FIELD_NOT_FOUND", + }); + expect(await registry.getVersion()).toBe(v0); + + await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); + const v1 = await registry.getVersion(); + + await registry.deleteField("job_title"); + expect(await registry.getVersion()).toBe(v1 + 1); + }); + + it("updateField bumps the version before the UPDATE (TRANSLATABLE_LOCKED does NOT bump)", async () => { + await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); + const field = await registry.getField("job_title"); + await sql` + INSERT INTO _emdash_bylines (id, slug, display_name, locale, translation_group) + VALUES ('b1', 'jane', 'Jane', 'en', 'b1') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES ('b1', ${field?.id}, '"Editor"') + `.execute(db); + + const v0 = await registry.getVersion(); + // Pre-bump validation catches TRANSLATABLE_LOCKED — version must + // not advance on a rejected flip. + await expect( + registry.updateField("job_title", { translatable: false }), + ).rejects.toMatchObject({ code: "TRANSLATABLE_LOCKED" }); + expect(await registry.getVersion()).toBe(v0); + + // Successful update bumps. + await registry.updateField("job_title", { label: "Job Title" }); + expect(await registry.getVersion()).toBe(v0 + 1); + }); + + it("reorderFields bumps the version before the UPDATE loop (REORDER_MISMATCH does NOT bump)", async () => { + await registry.createField({ slug: "a", label: "A", type: "string" }); + await registry.createField({ slug: "b", label: "B", type: "string" }); + const v0 = await registry.getVersion(); + + await expect(registry.reorderFields(["a", "c"])).rejects.toMatchObject({ + code: "REORDER_MISMATCH", + }); + expect(await registry.getVersion()).toBe(v0); + + await registry.reorderFields(["b", "a"]); + expect(await registry.getVersion()).toBe(v0 + 1); + }); + }); + + describe("typed errors carry stable codes", () => { + it("error instances are BylineSchemaError with .code", async () => { + try { + await registry.createField({ slug: "id", label: "ID", type: "string" }); + expect.fail("should have thrown"); + } catch (error) { + expect(error).toBeInstanceOf(BylineSchemaError); + expect((error as BylineSchemaError).code).toBe("RESERVED_SLUG"); + } + }); + }); +}); From 1bb4e488bd4d2fa7e0b70e634c648a51c1d29990 Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 16:46:10 +0100 Subject: [PATCH 05/35] feat(bylines): per-isolate field-defs cache + request-cache invalidation helper --- packages/core/src/bylines/field-defs-cache.ts | 138 ++++++++++++++++++ packages/core/src/request-cache.ts | 23 +++ 2 files changed, 161 insertions(+) create mode 100644 packages/core/src/bylines/field-defs-cache.ts diff --git a/packages/core/src/bylines/field-defs-cache.ts b/packages/core/src/bylines/field-defs-cache.ts new file mode 100644 index 000000000..047de587e --- /dev/null +++ b/packages/core/src/bylines/field-defs-cache.ts @@ -0,0 +1,138 @@ +/** + * Byline field-definitions cache + * + * Discussion #1174 / Phase 3. Two-tier cache for the byline custom-field + * registry, mirroring the `settings/index.ts` pattern. + * + * **Tier 1 — per-isolate (globalThis).** Field definitions change rarely + * but are read on every byline hydration (admin pages, content rendering, + * API responses). Caching at the isolate level drops the SELECT-from- + * `_emdash_byline_fields` from once-per-hydration to once-per-isolate- + * after-bump. The cache holds a Promise (not the resolved value) so + * concurrent cold-isolate readers share the in-flight query. + * + * Stored on globalThis under `Symbol.for("emdash:byline-field-defs")` so + * Vite SSR chunk duplication can't produce two independent caches (same + * pattern as `request-cache.ts` and `request-context.ts`). + * + * **Tier 2 — per-request.** Wraps both the version read and the defs + * fetch in `requestCached` so a single page render that hits byline + * hydration multiple times (e.g. list view + individual byline lookups + * in a sidebar) pays at most one version read and one defs fetch in + * total. The defs cache key includes the version, so a (highly + * unlikely) mid-request bump still produces a self-consistent view — + * the second call sees a different key and refetches. + * + * **Invalidation.** `options.byline_fields_version` is bumped by every + * `BylineSchemaRegistry` mutation (Phase 2). Each isolate independently + * reads the persisted version on the next request and compares against + * its cached version; mismatch triggers a refetch and overwrite. Other + * isolates see the change within one request after the bump propagates. + * + * **Isolated databases bypass the global cache.** Playground and DO + * preview sessions set `requestContext.dbIsIsolated = true`, signalling + * the per-request `db` points at an isolated schema that may diverge + * from the singleton. Schema-derived caches keyed by the singleton's + * version would silently leak the singleton's defs into the isolated + * request. We follow the `loader.ts:74` `getTaxonomyNames` precedent: + * skip both reading from and writing to the global holder when the + * request is isolated. The per-request cache (`requestCached`) is keyed + * by the WeakMap'd `EmDashRequestContext`, so it can't cross-pollinate + * between requests — it stays in play even for isolated DBs. + * + * **Why a versioned cache and not a TTL?** The version counter gives + * deterministic invalidation without the staleness window a TTL would + * impose. Field-definition changes need to be visible to the next + * request, not eventually. The cost is one cheap `options` read per + * request — cheaper than the field-defs fetch it replaces, and cheaper + * than maintaining a TTL state machine. + */ + +import type { Kysely } from "kysely"; + +import type { Database } from "../database/types.js"; +import { requestCached } from "../request-cache.js"; +import { getRequestContext } from "../request-context.js"; +import { BylineSchemaRegistry } from "../schema/byline-registry.js"; +import type { BylineFieldDefinition } from "../schema/types.js"; + +interface FieldDefsHolder { + /** Cached defs from the last successful fetch. Null until first read. */ + cached: BylineFieldDefinition[] | null; + /** Persisted-version value that `cached` was fetched against. */ + cachedVersion: number; +} + +const HOLDER_KEY = Symbol.for("emdash:byline-field-defs"); +const g = globalThis as Record; +const holder: FieldDefsHolder = + // eslint-disable-next-line typescript/no-unsafe-type-assertion -- globalThis singleton pattern (see request-cache.ts) + (g[HOLDER_KEY] as FieldDefsHolder | undefined) ?? + (() => { + const h: FieldDefsHolder = { cached: null, cachedVersion: -1 }; + g[HOLDER_KEY] = h; + return h; + })(); + +const REQUEST_CACHE_KEY_VERSION = "byline-fields-version"; +const REQUEST_CACHE_KEY_DEFS_PREFIX = "byline-field-defs:"; + +/** + * Read the persisted `options.byline_fields_version` counter. Cached for + * the duration of the current request via `requestCached`. Returns `0` + * when the row is missing (matches `BylineSchemaRegistry.getVersion`). + */ +async function getBylineFieldsVersion(db: Kysely): Promise { + return requestCached(REQUEST_CACHE_KEY_VERSION, () => new BylineSchemaRegistry(db).getVersion()); +} + +/** + * Resolve the registered byline custom-field definitions for the current + * request. Two-tier caching as described in the module header: + * + * 1. Per-request: at most one version read and one defs fetch per + * request, regardless of how many `BylineRepository` calls happen. + * 2. Per-isolate: defs survive across requests until the persisted + * version changes — **except when the request uses an isolated + * database**, in which case the global holder is bypassed. + * + * Always returns an array — never throws on a missing version row or an + * empty registry. An empty array means "no custom fields registered", + * which is the opt-in default for sites that haven't declared any. + */ +export async function getBylineFieldDefs(db: Kysely): Promise { + const isolated = getRequestContext()?.dbIsIsolated === true; + const version = await getBylineFieldsVersion(db); + return requestCached(`${REQUEST_CACHE_KEY_DEFS_PREFIX}${version}`, async () => { + // Skip the global holder for isolated requests — playground and DO + // preview point at schemas that may diverge from the singleton, and + // a version-number collision must not leak the singleton's defs. + // The per-request cache above is still in play because it's keyed + // by the request context itself, so the isolated request still + // dedupes within its own lifetime. + if (isolated) { + return new BylineSchemaRegistry(db).listFields(); + } + if (holder.cached !== null && holder.cachedVersion === version) { + return holder.cached; + } + const defs = await new BylineSchemaRegistry(db).listFields(); + holder.cached = defs; + holder.cachedVersion = version; + return defs; + }); +} + +/** + * Test/internal helper: clear the per-isolate cache. Useful for unit + * tests that mutate the registry directly and need to force a refetch + * without going through the full version-bump path. + * + * Production code paths should rely on the version counter for + * invalidation — calling this from a write path would bypass the + * coordination that lets other isolates see the change. + */ +export function resetBylineFieldDefsCacheForTests(): void { + holder.cached = null; + holder.cachedVersion = -1; +} diff --git a/packages/core/src/request-cache.ts b/packages/core/src/request-cache.ts index 3df07c7ff..ab8f007b2 100644 --- a/packages/core/src/request-cache.ts +++ b/packages/core/src/request-cache.ts @@ -111,3 +111,26 @@ export function setRequestCacheEntry(key: string, value: T): void { if (cache.has(key)) return; cache.set(key, Promise.resolve(value)); } + +/** + * Remove a key from the request-scoped cache. + * + * Used by write paths that need to invalidate a downstream read cache — + * `setRequestCacheEntry` deliberately doesn't overwrite, and `requestCached` + * returns the cached promise even after the underlying data has changed. + * Without an explicit clear, a `read → write → read` sequence in a single + * request can return stale data on the second read. + * + * Concrete case: `BylineRepository.update` invalidates + * `byline-field-group-values:${translation_group}` after writing a + * non-translatable custom-field value, so the post-update `findById` + * (and any later reads in the same request) see the fresh value. + * + * No-ops outside a request context. + */ +export function clearRequestCacheEntry(key: string): void { + const ctx = getRequestContext(); + if (!ctx) return; + const cache = store.get(ctx); + cache?.delete(key); +} From 5069cba169cf4e27b0479b098ae3cdd7740cd1c0 Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 16:46:21 +0100 Subject: [PATCH 06/35] feat(bylines): atomic version bumps around schema mutations for cache coherence --- packages/core/src/schema/byline-registry.ts | 162 ++++++++++-------- .../database/byline-fields-races.test.ts | 19 +- .../tests/unit/schema/byline-registry.test.ts | 59 ++++--- 3 files changed, 131 insertions(+), 109 deletions(-) diff --git a/packages/core/src/schema/byline-registry.ts b/packages/core/src/schema/byline-registry.ts index 884a134fc..8d9dd7cbf 100644 --- a/packages/core/src/schema/byline-registry.ts +++ b/packages/core/src/schema/byline-registry.ts @@ -66,24 +66,38 @@ export class BylineSchemaError extends Error { * writes; D1 inherits that; Postgres handles concurrent UPDATEs via row- * level locking). * - * **Bump-before-mutate ordering (D1 partial-failure safety).** D1 has no - * transactions; `withTransaction` falls back to direct execution. Every - * mutation bumps the version counter *before* applying the schema change - * — not after — so a crash between the two statements leaves the system - * in a recoverable state: + * **Bump-twice inside a transaction (crash + concurrency safety).** Every + * mutation method wraps `bumpVersion → mutate → bumpVersion` in + * `withTransaction`. On Node SQLite + Postgres the three statements are + * atomic — cache readers see either the pre-mutation state at the old + * version or the post-mutation state at the new version, never a partial. + * On D1 (no transactions) the calls run sequentially and the cache can + * briefly observe an intermediate state; the second bump narrows that + * window to the millisecond gap between mutation and second-bump SQL. * - * - Crash before bump: nothing happened. - * - Crash after bump but before mutation: the schema is unchanged but - * caches invalidate on the higher version. They refetch the (unchanged) - * field list once. The original caller retries the mutation, which - * bumps again and lands. Net cost: one spurious cache refresh. + * Why bump twice (the obvious "bump once after" leaves a worse window): * - * The bump-after order would leave caches *permanently* stale on a crash - * between the mutation and the bump — the schema would have changed but - * the cache would have no signal to refetch, and a retry of the same - * op would throw `FIELD_EXISTS` / `FIELD_NOT_FOUND` without reaching the - * bump line. Bump-before trades that for a smaller, self-healing reader - * race during the mutation window. + * - **Bump after only**: a crash between mutation and bump leaves the + * schema changed but the version unchanged. Caches stay stuck stale + * *until the next successful mutation* — potentially forever. A retry + * of the same op throws `FIELD_EXISTS`/`FIELD_NOT_FOUND` and never + * reaches the bump line. + * - **Bump before only**: a concurrent hydration request between bump + * and mutation can read the new version, fetch the *old* defs, and + * cache them under the new version — stuck stale until the next + * mutation. The Phase 3 reviewer caught this. + * - **Bump before + bump after**: covers both. The first bump + * invalidates existing caches before the mutation lands (crash here + * is safe — caches refresh to the still-old schema, retry recovers). + * The second bump invalidates caches that captured the mid-transition + * state (race here is bounded by the bump-to-bump duration). Cost is + * one extra `UPDATE options` per mutation, single set-based SQL, + * negligible at the scale schema mutations happen. + * + * The residual unsafe window on D1 — crash between mutation and second + * bump — is the same fundamental D1 limitation that affects every + * multi-statement consistency invariant in this codebase. It's bounded + * by the time-to-issue-one-SQL-UPDATE (~ms) rather than unbounded. * * **Application-level cascade in `deleteField`.** Migration 041 declares * FK ON DELETE CASCADE on both value tables, and at runtime SQLite + D1 + @@ -153,26 +167,28 @@ export class BylineSchemaRegistry { const id = ulid(); const sortOrder = input.sortOrder ?? (await this.nextSortOrder()); - // Bump-before-mutate: see class JSDoc for the D1 partial-failure - // rationale. All validation has already run above; the only way the - // INSERT can fail now is a concurrent insert of the same slug - // (UNIQUE constraint), which leaves the bump as a spurious - // cache-refresh signal — strictly better than a stale cache. - await this.bumpVersion(); - - await this.db - .insertInto("_emdash_byline_fields") - .values({ - id, - slug: input.slug, - label: input.label, - type: input.type, - required: input.required ? 1 : 0, - translatable: input.translatable === false ? 0 : 1, - validation: validation ? JSON.stringify(validation) : null, - sort_order: sortOrder, - }) - .execute(); + // Bump-twice inside a transaction: see class JSDoc. All validation + // (slug, label, type, validation payload, FIELD_EXISTS) has already + // run above, so the only way this can fail now is a concurrent + // insert of the same slug (UNIQUE), which rolls back the whole tx + // on Node SQLite + PG — keeping the bumps and the row atomic. + await withTransaction(this.db, async (trx) => { + await this.bumpVersion(trx); + await trx + .insertInto("_emdash_byline_fields") + .values({ + id, + slug: input.slug, + label: input.label, + type: input.type, + required: input.required ? 1 : 0, + translatable: input.translatable === false ? 0 : 1, + validation: validation ? JSON.stringify(validation) : null, + sort_order: sortOrder, + }) + .execute(); + await this.bumpVersion(trx); + }); const created = await this.getFieldById(id); if (!created) { @@ -250,16 +266,18 @@ export class BylineSchemaRegistry { updates.updated_at = new Date().toISOString(); - // Bump-before-mutate: see class JSDoc. All validation (including - // translatable-flip safety) has run; the UPDATE is idempotent, so a - // crash here recovers cleanly on retry. - await this.bumpVersion(); - - await this.db - .updateTable("_emdash_byline_fields") - .set(updates) - .where("id", "=", field.id) - .execute(); + // Bump-twice inside a transaction: see class JSDoc. All validation + // (including translatable-flip safety) has run; the UPDATE is + // idempotent. + await withTransaction(this.db, async (trx) => { + await this.bumpVersion(trx); + await trx + .updateTable("_emdash_byline_fields") + .set(updates) + .where("id", "=", field.id) + .execute(); + await this.bumpVersion(trx); + }); const updated = await this.getFieldById(field.id); if (!updated) { @@ -278,24 +296,17 @@ export class BylineSchemaRegistry { }); } - // Bump-before-mutate. See class JSDoc. - await this.bumpVersion(); - - // Application-level cascade. The FKs in migration 041 already - // ON DELETE CASCADE, but doing it explicitly here: - // 1. removes the dependency on `PRAGMA foreign_keys = ON` - // (set in production via `connection.ts:60`, but easy to miss - // in tests and one-off scripts); - // 2. mirrors `BylineRepository.delete`'s app-level cascade, which - // had to be implemented when migration 040 dropped its FK; - // 3. makes the cleanup ordering explicit on D1 — value rows first, - // definition row last, so a crash leaves the definition still - // present (deletable on retry) rather than orphan values - // pointing at a vanished definition id. - // - // withTransaction gives us real atomicity on Node SQLite + PG; - // on D1 it falls back to direct sequenced execution. + // Bump-twice inside the same transaction as the cascade. See class + // JSDoc. Application-level cascade order matters on D1 (no tx): + // value rows first, definition row last, so a crash leaves the + // definition still present (deletable on retry) rather than orphan + // values pointing at a vanished definition id. The FKs in migration + // 041 already ON DELETE CASCADE; doing it explicitly here adds + // defense-in-depth against FK-pragma misconfig and mirrors + // `BylineRepository.delete`'s app-level cascade for the bylines + // domain. await withTransaction(this.db, async (trx) => { + await this.bumpVersion(trx); await trx .deleteFrom("_emdash_byline_field_values") .where("field_id", "=", field.id) @@ -305,6 +316,7 @@ export class BylineSchemaRegistry { .where("field_id", "=", field.id) .execute(); await trx.deleteFrom("_emdash_byline_fields").where("id", "=", field.id).execute(); + await this.bumpVersion(trx); }); } @@ -343,18 +355,12 @@ export class BylineSchemaRegistry { } } - // Bump-before-mutate: see class JSDoc. The reorder loop is - // idempotent (each statement writes a fixed sort_order to a fixed - // slug), so a crash mid-loop recovers cleanly on retry. - await this.bumpVersion(); - - // Update sort_order for each field. The loop is per-slug rather than - // a single CASE expression because the per-statement form is dialect- - // agnostic and tiny field sets (typical: 3–10) make the cost - // irrelevant. Mirrors `SchemaRegistry.reorderFields`. Wrapped in - // withTransaction for real atomicity on Node SQLite + PG. + // Bump-twice inside the transaction with the reorder loop. See + // class JSDoc. Each UPDATE writes a fixed sort_order to a fixed + // slug, so the loop is idempotent on retry. const now = new Date().toISOString(); await withTransaction(this.db, async (trx) => { + await this.bumpVersion(trx); for (let i = 0; i < slugs.length; i++) { const slug = slugs[i]; if (slug === undefined) continue; @@ -364,6 +370,7 @@ export class BylineSchemaRegistry { .where("slug", "=", slug) .execute(); } + await this.bumpVersion(trx); }); } @@ -396,13 +403,18 @@ export class BylineSchemaRegistry { * * Stored as a JSON number literal (`5`, not `"5"`) so `JSON.parse` reads * it back as a number on the cache path. + * + * The optional `conn` parameter lets callers run the bump inside an + * outer transaction so it lands atomically with the mutation it pairs + * with — see the class JSDoc for the bump-twice ordering rationale. */ - private async bumpVersion(): Promise { + private async bumpVersion(conn?: Kysely): Promise { + const target = conn ?? this.db; await sql` UPDATE options SET value = CAST(CAST(value AS INTEGER) + 1 AS TEXT) WHERE name = ${VERSION_KEY} - `.execute(this.db); + `.execute(target); } private async nextSortOrder(): Promise { diff --git a/packages/core/tests/integration/database/byline-fields-races.test.ts b/packages/core/tests/integration/database/byline-fields-races.test.ts index 795e6790a..a6211af8a 100644 --- a/packages/core/tests/integration/database/byline-fields-races.test.ts +++ b/packages/core/tests/integration/database/byline-fields-races.test.ts @@ -60,7 +60,9 @@ describeEachDialect("BylineSchemaRegistry concurrency", (dialect) => { expect(succeeded).toBe(slugs.length); const endVersion = await registry.getVersion(); - expect(endVersion - startVersion).toBe(slugs.length); + // Each successful mutation bumps the counter twice (before + after + // the schema change). 10 creates → 20 increments. + expect(endVersion - startVersion).toBe(slugs.length * 2); }); it("parallel updateField calls don't lose version increments", async () => { @@ -76,10 +78,11 @@ describeEachDialect("BylineSchemaRegistry concurrency", (dialect) => { const after = await registry.getVersion(); // Every update is non-trivial (label changes), so every call should - // have bumped the counter. PG: serialised by the row-level lock on - // `_emdash_byline_fields.id` during UPDATE; SQLite: serialised by - // the database lock. Either way, increments are not lost. - expect(after - baseline).toBe(labels.length); + // have bumped the counter twice. PG: serialised by the row-level + // lock on `_emdash_byline_fields.id` during UPDATE; SQLite: + // serialised by the database lock. Either way, increments are not + // lost. + expect(after - baseline).toBe(labels.length * 2); // And the final state is a single, well-defined row — no duplicate // definitions, label is one of the inputs. @@ -107,7 +110,8 @@ describeEachDialect("BylineSchemaRegistry concurrency", (dialect) => { await registry.reorderFields(["c", "a", "b"]); const after = await registry.getVersion(); - expect(after - baseline).toBe(3); + // 3 mutations × 2 bumps each = 6 increments. + expect(after - baseline).toBe(6); }); it("parallel deletes against distinct fields don't lose increments", async () => { @@ -119,7 +123,8 @@ describeEachDialect("BylineSchemaRegistry concurrency", (dialect) => { await Promise.all(Array.from({ length: 6 }, (_, i) => registry.deleteField(`del_${i}`))); const after = await registry.getVersion(); - expect(after - baseline).toBe(6); + // 6 deletes × 2 bumps each = 12 increments. + expect(after - baseline).toBe(12); expect((await registry.listFields()).map((f) => f.slug)).not.toContain("del_0"); }); diff --git a/packages/core/tests/unit/schema/byline-registry.test.ts b/packages/core/tests/unit/schema/byline-registry.test.ts index 9f457340b..7adabadf2 100644 --- a/packages/core/tests/unit/schema/byline-registry.test.ts +++ b/packages/core/tests/unit/schema/byline-registry.test.ts @@ -408,7 +408,13 @@ describe("BylineSchemaRegistry", () => { expect(await registry.getVersion()).toBe(0); }); - it("monotonically increases across mutations", async () => { + it("monotonically increases across mutations (bump-twice = +2 per op)", async () => { + // Each mutation bumps the version twice — once before the + // schema change, once after. The first bump invalidates + // existing caches (crash safety); the second invalidates + // caches that captured the in-flight mid-transition state + // (race safety for cross-isolate readers). See the registry + // class JSDoc for the full rationale. const v0 = await registry.getVersion(); await registry.createField({ slug: "a", label: "A", type: "string" }); const v1 = await registry.getVersion(); @@ -422,11 +428,11 @@ describe("BylineSchemaRegistry", () => { const v5 = await registry.getVersion(); expect(v0).toBe(0); - expect(v1).toBe(1); - expect(v2).toBe(2); - expect(v3).toBe(3); - expect(v4).toBe(4); - expect(v5).toBe(5); + expect(v1).toBe(2); + expect(v2).toBe(4); + expect(v3).toBe(6); + expect(v4).toBe(8); + expect(v5).toBe(10); }); it("getVersion returns 0 when the row is missing", async () => { @@ -435,20 +441,19 @@ describe("BylineSchemaRegistry", () => { }); }); - describe("bump-before-mutate ordering", () => { + describe("bump-twice ordering", () => { // These tests pin the observable behaviour that the registry bumps - // the version counter *before* it applies the schema mutation. The - // rationale lives in BylineSchemaRegistry's class JSDoc: on D1 (no - // transactions) a crash between bump and mutation leaves the system - // recoverable; the opposite order leaves caches stuck stale. + // the version counter twice per mutation — once before the schema + // change (crash safety: caches refresh on the higher version) and + // once after (race safety: caches that captured the mid-transition + // state get invalidated on the second bump). The rationale lives + // in BylineSchemaRegistry's class JSDoc. // - // "Observable behaviour" here means: when a mutation would fail - // for a reason that only becomes visible *after* the bump runs - // (e.g. a UNIQUE-constraint race), the version counter still - // advanced. Tests that assert "version unchanged on validation - // rejection" further confirm that pre-bump validation is correct. + // "Observable behaviour" here means: a successful mutation bumps + // the counter by exactly 2, and a mutation rejected at validation + // (before any DB write) does not bump at all. - it("createField bumps the version before INSERT (validation errors do NOT bump)", async () => { + it("createField bumps the version by 2 (validation errors do NOT bump)", async () => { const v0 = await registry.getVersion(); // Pre-bump validation: an invalid slug never reaches the bump. @@ -457,18 +462,18 @@ describe("BylineSchemaRegistry", () => { ).rejects.toMatchObject({ code: "INVALID_SLUG" }); expect(await registry.getVersion()).toBe(v0); - // Successful path: bump lands. + // Successful path: bumps land before + after. await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); - expect(await registry.getVersion()).toBe(v0 + 1); + expect(await registry.getVersion()).toBe(v0 + 2); // Duplicate-slug check is pre-bump too (we call getField first). await expect( registry.createField({ slug: "job_title", label: "Other", type: "string" }), ).rejects.toMatchObject({ code: "FIELD_EXISTS" }); - expect(await registry.getVersion()).toBe(v0 + 1); + expect(await registry.getVersion()).toBe(v0 + 2); }); - it("deleteField bumps the version before the DELETE (FIELD_NOT_FOUND does NOT bump)", async () => { + it("deleteField bumps the version by 2 (FIELD_NOT_FOUND does NOT bump)", async () => { const v0 = await registry.getVersion(); await expect(registry.deleteField("missing")).rejects.toMatchObject({ code: "FIELD_NOT_FOUND", @@ -479,10 +484,10 @@ describe("BylineSchemaRegistry", () => { const v1 = await registry.getVersion(); await registry.deleteField("job_title"); - expect(await registry.getVersion()).toBe(v1 + 1); + expect(await registry.getVersion()).toBe(v1 + 2); }); - it("updateField bumps the version before the UPDATE (TRANSLATABLE_LOCKED does NOT bump)", async () => { + it("updateField bumps the version by 2 (TRANSLATABLE_LOCKED does NOT bump)", async () => { await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); const field = await registry.getField("job_title"); await sql` @@ -502,12 +507,12 @@ describe("BylineSchemaRegistry", () => { ).rejects.toMatchObject({ code: "TRANSLATABLE_LOCKED" }); expect(await registry.getVersion()).toBe(v0); - // Successful update bumps. + // Successful update bumps before + after. await registry.updateField("job_title", { label: "Job Title" }); - expect(await registry.getVersion()).toBe(v0 + 1); + expect(await registry.getVersion()).toBe(v0 + 2); }); - it("reorderFields bumps the version before the UPDATE loop (REORDER_MISMATCH does NOT bump)", async () => { + it("reorderFields bumps the version by 2 (REORDER_MISMATCH does NOT bump)", async () => { await registry.createField({ slug: "a", label: "A", type: "string" }); await registry.createField({ slug: "b", label: "B", type: "string" }); const v0 = await registry.getVersion(); @@ -518,7 +523,7 @@ describe("BylineSchemaRegistry", () => { expect(await registry.getVersion()).toBe(v0); await registry.reorderFields(["b", "a"]); - expect(await registry.getVersion()).toBe(v0 + 1); + expect(await registry.getVersion()).toBe(v0 + 2); }); }); From 5ead669c85326438f6c8881bdda56c00b849b848 Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 16:46:33 +0100 Subject: [PATCH 07/35] feat(bylines): hydrate customFields in BylineRepository --- .../core/src/database/repositories/byline.ts | 632 ++++++++++- .../unit/database/repositories/byline.test.ts | 981 +++++++++++++++++- 2 files changed, 1589 insertions(+), 24 deletions(-) diff --git a/packages/core/src/database/repositories/byline.ts b/packages/core/src/database/repositories/byline.ts index 25ba87135..6f6ef424c 100644 --- a/packages/core/src/database/repositories/byline.ts +++ b/packages/core/src/database/repositories/byline.ts @@ -1,6 +1,13 @@ import { sql, type Kysely, type Selectable } from "kysely"; import { ulid } from "ulidx"; +import { getBylineFieldDefs } from "../../bylines/field-defs-cache.js"; +import { + clearRequestCacheEntry, + peekRequestCache, + setRequestCacheEntry, +} from "../../request-cache.js"; +import type { BylineFieldDefinition, CustomFieldValue } from "../../schema/types.js"; import { chunks, SQL_BATCH_SIZE } from "../../utils/chunks.js"; import { listTablesLike } from "../dialect-helpers.js"; import { withTransaction } from "../transaction.js"; @@ -8,6 +15,7 @@ import type { BylineTable, Database } from "../types.js"; import { validateIdentifier } from "../validate.js"; import { decodeCursor, + EmDashValidationError, encodeCursor, type BylineSummary, type ContentBylineCredit, @@ -46,6 +54,24 @@ export interface UpdateBylineInput { websiteUrl?: string | null; userId?: string | null; isGuest?: boolean; + /** + * Byline custom-field values to write (Phase 3 of Discussion #1174). + * + * Each key must match a registered slug in `_emdash_byline_fields`; + * unknown keys throw `EmDashValidationError`. Per-field writes route + * to `_emdash_byline_field_values` (when the field's `translatable` + * flag is true) or `_emdash_byline_field_group_values` (when false). + * A value of `null` clears the row. + * + * Values are validated against the field's type: + * - `string` / `text` / `url` accept a `string` + * - `boolean` accepts a `boolean` + * - `select` accepts a `string` that appears in `validation.options` + * + * Writes are idempotent (`INSERT … ON CONFLICT DO UPDATE`), so + * retrying the same update produces the same DB state. + */ + customFields?: Record; } export interface ContentBylineInput { @@ -70,6 +96,97 @@ function rowToByline(row: BylineRow): BylineSummary { }; } +/** + * Merge a single decoded value into a `BylineSummary.customFields` map. + * Centralised so the merge semantics (null storage, JSON.parse failure + * handling) live in one place across both translatable and group-shared + * paths. + * + * A stored row with `value = NULL` (representing an explicit null) is + * surfaced as `null` in `customFields`. A row with a malformed JSON + * payload is dropped silently with a `console.warn` — a corrupted + * payload shouldn't break the entire byline hydration; the field-defs + * cache will let admins replace the value, and the warning makes the + * issue debuggable. (Storage path uses `JSON.stringify`, so the only + * way to get malformed JSON is direct DB tampering or a future + * migration bug.) + */ +function assignCustomFieldValue( + summary: BylineSummary, + field: BylineFieldDefinition, + stored: string | null, +): void { + const target = summary.customFields ?? {}; + if (stored === null) { + target[field.slug] = null; + } else { + try { + // eslint-disable-next-line typescript/no-unsafe-type-assertion -- coerceFieldValue ran at write time, see field-defs-cache.ts + target[field.slug] = JSON.parse(stored) as CustomFieldValue; + } catch { + console.warn( + `[BylineRepository] dropping malformed JSON for byline=${summary.id} ` + + `field=${field.slug}: ${stored.slice(0, 60)}`, + ); + return; + } + } + summary.customFields = target; +} + +/** + * Coerce a raw write-path value to a `CustomFieldValue` for the given + * field, throwing `EmDashValidationError` on type mismatch. Mirrors what + * the Phase 4 zod schema will enforce at the API layer; this guard runs + * regardless of caller so internal code paths (seeds, tests, future + * non-HTTP callers) can't slip past it. + * + * `null` is always accepted — it represents "clear this field" and + * triggers a DELETE rather than an upsert in the write path. + */ +function coerceFieldValue(field: BylineFieldDefinition, raw: unknown): CustomFieldValue { + if (raw === null) return null; + + switch (field.type) { + case "string": + case "text": + case "url": { + if (typeof raw !== "string") { + throw new EmDashValidationError( + `Byline field "${field.slug}" expects a string value (received ${typeof raw})`, + { slug: field.slug, type: field.type, received: typeof raw }, + ); + } + return raw; + } + case "boolean": { + if (typeof raw !== "boolean") { + throw new EmDashValidationError( + `Byline field "${field.slug}" expects a boolean value (received ${typeof raw})`, + { slug: field.slug, type: field.type, received: typeof raw }, + ); + } + return raw; + } + case "select": { + if (typeof raw !== "string") { + throw new EmDashValidationError( + `Byline field "${field.slug}" expects a string value (received ${typeof raw})`, + { slug: field.slug, type: field.type, received: typeof raw }, + ); + } + const options = field.validation?.options ?? []; + if (!options.includes(raw)) { + throw new EmDashValidationError( + `Byline field "${field.slug}" value "${raw}" is not one of the registered choices`, + { slug: field.slug, value: raw, options }, + ); + } + return raw; + } + } +} + /** * Byline repository for content credits. * @@ -91,13 +208,248 @@ function rowToByline(row: BylineRow): BylineSummary { export class BylineRepository { constructor(private db: Kysely) {} + // ============================================ + // Custom-field hydration (Phase 3 of #1174) + // ============================================ + + /** + * Merge `customFields` onto each `BylineSummary` produced from the + * given rows. Two batched queries total — one against + * `_emdash_byline_field_values` (keyed by `byline_id`), one against + * `_emdash_byline_field_group_values` (keyed by `translation_group`) + * — both chunked at `SQL_BATCH_SIZE` for D1's bound-parameter cap. + * + * When zero fields are registered, every row gets `customFields = {}` + * with no value-table reads (the field-defs cache returns `[]`). + * Group-shared values are looked up via the row's `translation_group`, + * so every locale sibling of the same byline identity sees the same + * non-translatable value without re-reading per row. + * + * **Duplicate-row handling.** Callers (notably `getContentBylinesMany` + * for list views with repeated authors) can pass the same byline row + * multiple times. We assign values by *iterating both `rows` and + * `summaries` in lockstep by index*, not by deduping into a Map keyed + * on byline id. A Map approach silently drops earlier duplicates' merge + * step (last writer wins, earlier instances keep their initial `{}`). + * Iterating by index gives every duplicate its own merged copy. + * + * Hydration is *strict per row* — values are merged onto whichever + * `BylineRow` produced them. Fallback semantics (e.g. "if no value + * for this locale, show the default-locale value") are not the + * repository's concern; consumers layer them on top if wanted, the + * same way `BylineRepository` doesn't resolve locale fallback for + * the base byline lookup. + */ + private async withCustomFields(rows: BylineRow[]): Promise { + const summaries = rows.map(rowToByline); + // Always populate `customFields = {}` (PR plan AC #6) — even when + // no fields are registered, every BylineSummary carries the empty + // object. A fresh object per summary so duplicate rows don't share + // state. + for (const summary of summaries) { + summary.customFields = {}; + } + await this.applyCustomFieldsTo(summaries); + return summaries; + } + + private async withCustomFieldsOne(row: BylineRow | undefined): Promise { + if (!row) return null; + const [result] = await this.withCustomFields([row]); + return result ?? null; + } + + /** + * Hydrate `customFields` on each `BylineSummary`, mutating in place. + * + * The public entry point for callers that fetch byline rows in + * multiple passes (e.g. `getBylinesForEntries`, which buckets by + * locale and calls `getContentBylinesMany` per bucket) and want a + * single batched hydration over the union of bylines, not one per + * pass. Use with the `skipHydration` option on the read methods to + * defer customFields work to a single call here. + * + * Two batched queries total (translatable + group-shared) regardless + * of how many bylines, locales, or translation_groups are in the + * input — meets the Phase 3 query-count envelope for mixed-locale + * list views even when sibling locales reference disjoint + * translation_groups. + * + * Replaces any existing `customFields` on each summary with a freshly + * fetched map. Callers that want to merge rather than replace should + * not use this entry point. + */ + async hydrateBylineCustomFields(summaries: BylineSummary[]): Promise { + for (const summary of summaries) { + summary.customFields = {}; + } + await this.applyCustomFieldsTo(summaries); + } + + /** + * Shared merge engine for `withCustomFields` and + * `hydrateBylineCustomFields`. Reads field defs (cached), batches the + * translatable + group-shared fetches, and walks `summaries` directly + * to apply values. + * + * Iterates `summaries` (not a `summaryById` map) so duplicate + * `BylineSummary` objects sharing the same `id` — e.g. the same + * author credited to multiple entries — each get their own merged + * values. The previous Map-based dedup silently dropped earlier + * duplicates' merge step. + */ + private async applyCustomFieldsTo(summaries: BylineSummary[]): Promise { + if (summaries.length === 0) return; + + const defs = await getBylineFieldDefs(this.db); + if (defs.length === 0) return; + + const fieldById = new Map(defs.map((d) => [d.id, d])); + + // Translatable values, batched by byline_id (unique per locale, so + // IDs across different locale buckets don't collide — one batched + // query covers everything). + const translatableByByline = new Map>(); + const bylineIds = [...new Set(summaries.map((s) => s.id))]; + for (const chunk of chunks(bylineIds, SQL_BATCH_SIZE)) { + const trRows = await this.db + .selectFrom("_emdash_byline_field_values") + .select(["byline_id", "field_id", "value"]) + .where("byline_id", "in", chunk) + .execute(); + for (const trRow of trRows) { + let fieldMap = translatableByByline.get(trRow.byline_id); + if (!fieldMap) { + fieldMap = new Map(); + translatableByByline.set(trRow.byline_id, fieldMap); + } + fieldMap.set(trRow.field_id, trRow.value); + } + } + + // Group-shared values, batched over the union of translation_groups, + // with per-group request-cache priming so subsequent calls within + // the same request share the lookup. Together with the + // `hydrateBylineCustomFields` + `skipHydration` flow in + // `getBylinesForEntries`, this keeps mixed-locale list views to + // **one** group-shared query per request, even for disjoint + // translation_groups across locale buckets. + const groups = [ + ...new Set( + summaries + .map((s) => s.translationGroup) + .filter((g): g is string => typeof g === "string" && g.length > 0), + ), + ]; + const groupByGroup = await this.loadGroupValuesByIds(groups); + + // Apply values directly to each summary. `summaries[i]` is its own + // object even for duplicates of the same byline_id, so writes don't + // collide. + for (const summary of summaries) { + const trValues = translatableByByline.get(summary.id); + if (trValues) { + for (const [fieldId, value] of trValues) { + const field = fieldById.get(fieldId); + if (!field) continue; + assignCustomFieldValue(summary, field, value); + } + } + + if (summary.translationGroup) { + const grpValues = groupByGroup.get(summary.translationGroup); + if (grpValues) { + for (const [fieldId, value] of grpValues) { + const field = fieldById.get(fieldId); + if (!field) continue; + assignCustomFieldValue(summary, field, value); + } + } + } + } + } + + /** + * Resolve the group-shared custom-field values for a set of + * translation_groups, sharing work across hydration calls within the + * same request via per-group `requestCached` entries. + * + * The non-translatable storage table (`_emdash_byline_field_group_values`) + * is keyed by `translation_group`, which is locale-agnostic. Combining + * this method with `skipHydration` on `getContentBylinesMany` and a + * single `hydrateBylineCustomFields` call (see + * `getBylinesForEntries`) keeps mixed-locale list hydration to **one** + * batched group-shared SQL per request — even with disjoint + * translation_groups across locale buckets. Solo callers (`findById`, + * `findMany`, etc.) still get the same per-call batching they had + * before; the cache simply means a second call in the same request + * for an overlapping group is free. + * + * Cache key: `byline-field-group-values:${groupId}` — one entry per + * group. Writes use `setRequestCacheEntry` (idempotent, doesn't + * overwrite); `BylineRepository.update` calls `clearRequestCacheEntry` + * after a group-shared write to keep the cache fresh within the same + * request. + */ + private async loadGroupValuesByIds( + groups: string[], + ): Promise>> { + const result = new Map>(); + if (groups.length === 0) return result; + + // First pass: pull any already-cached groups from the request scope. + const missing: string[] = []; + for (const g of groups) { + const cached = peekRequestCache>(`byline-field-group-values:${g}`); + if (cached) { + result.set(g, await cached); + } else { + missing.push(g); + } + } + + if (missing.length === 0) return result; + + // Second pass: one batched SQL for the union of all missing groups + // (chunked for D1's bound-parameter cap). Initialise empty maps for + // missing groups so the primed cache covers "this group has no + // values" — preventing a re-fetch on subsequent calls. + const fetched = new Map>(); + for (const g of missing) fetched.set(g, new Map()); + for (const chunk of chunks(missing, SQL_BATCH_SIZE)) { + const grpRows = await this.db + .selectFrom("_emdash_byline_field_group_values") + .select(["translation_group", "field_id", "value"]) + .where("translation_group", "in", chunk) + .execute(); + for (const grpRow of grpRows) { + const fieldMap = fetched.get(grpRow.translation_group); + if (!fieldMap) continue; + fieldMap.set(grpRow.field_id, grpRow.value); + } + } + + for (const g of missing) { + const m = fetched.get(g); + if (!m) continue; + setRequestCacheEntry(`byline-field-group-values:${g}`, m); + result.set(g, m); + } + + return result; + } + + // ============================================ + // Reads + // ============================================ + async findById(id: string): Promise { const row = await this.db .selectFrom("_emdash_bylines") .selectAll() .where("id", "=", id) .executeTakeFirst(); - return row ? rowToByline(row) : null; + return this.withCustomFieldsOne(row); } /** @@ -109,7 +461,7 @@ export class BylineRepository { let query = this.db.selectFrom("_emdash_bylines").selectAll().where("slug", "=", slug); if (options?.locale !== undefined) query = query.where("locale", "=", options.locale); const row = await query.orderBy("locale", "asc").executeTakeFirst(); - return row ? rowToByline(row) : null; + return this.withCustomFieldsOne(row); } /** @@ -122,7 +474,7 @@ export class BylineRepository { let query = this.db.selectFrom("_emdash_bylines").selectAll().where("user_id", "=", userId); if (options?.locale !== undefined) query = query.where("locale", "=", options.locale); const row = await query.orderBy("locale", "asc").executeTakeFirst(); - return row ? rowToByline(row) : null; + return this.withCustomFieldsOne(row); } async findMany(options?: { @@ -176,7 +528,8 @@ export class BylineRepository { } const rows = await query.execute(); - const items = rows.slice(0, limit).map(rowToByline); + const pageRows = rows.slice(0, limit); + const items = await this.withCustomFields(pageRows); const result: FindManyResult = { items }; if (rows.length > limit) { @@ -211,7 +564,7 @@ export class BylineRepository { .where("translation_group", "=", translationGroup) .orderBy("locale", "asc") .execute(); - return rows.map(rowToByline); + return this.withCustomFields(rows); } async create(input: CreateBylineInput): Promise { @@ -258,6 +611,31 @@ export class BylineRepository { const existing = await this.findById(id); if (!existing) return null; + // Resolve and validate `customFields` **before** any DB write so a + // validation error (unknown slug, type mismatch, select-choice + // mismatch) surfaces without partial-state side effects. Validation + // reuses the same field-defs cache that hydration uses — the same + // version-counter invalidation guarantees the write sees a fresh + // definition set after a Phase 2 mutation in any isolate. + const customFieldWrites: Array<{ + field: BylineFieldDefinition; + value: CustomFieldValue; + }> = []; + if (input.customFields && Object.keys(input.customFields).length > 0) { + const defs = await getBylineFieldDefs(this.db); + const bySlug = new Map(defs.map((d) => [d.slug, d])); + for (const [slug, raw] of Object.entries(input.customFields)) { + const field = bySlug.get(slug); + if (!field) { + throw new EmDashValidationError(`Unknown byline custom field "${slug}"`, { + slug, + registered: defs.map((d) => d.slug), + }); + } + customFieldWrites.push({ field, value: coerceFieldValue(field, raw) }); + } + } + const updates: Record = { updated_at: new Date().toISOString(), }; @@ -271,18 +649,122 @@ export class BylineRepository { if (input.isGuest !== undefined) updates.is_guest = input.isGuest ? 1 : 0; await this.db.updateTable("_emdash_bylines").set(updates).where("id", "=", id).execute(); + + if (customFieldWrites.length > 0) { + const now = new Date().toISOString(); + const group = existing.translationGroup ?? existing.id; + // Track whether any group-shared write happened so we can + // invalidate the per-request cache for this group AFTER the + // transaction commits. The earlier `findById(id)` call at the + // top of `update` populated the cache for this group; without + // invalidation, the closing `findById` (and any later reads in + // the same request) would return stale customFields. + let touchedGroupShared = false; + // withTransaction wraps the value writes so Node SQLite + PG get + // real atomicity across all per-field upserts. On D1 the + // transaction falls back to direct execution — each upsert is + // independently idempotent (`ON CONFLICT DO UPDATE`), so a + // crash mid-loop is recoverable on retry. + await withTransaction(this.db, async (trx) => { + for (const { field, value } of customFieldWrites) { + if (!field.translatable) touchedGroupShared = true; + if (field.translatable) { + if (value === null) { + await trx + .deleteFrom("_emdash_byline_field_values") + .where("byline_id", "=", id) + .where("field_id", "=", field.id) + .execute(); + } else { + const encoded = JSON.stringify(value); + await trx + .insertInto("_emdash_byline_field_values") + .values({ + byline_id: id, + field_id: field.id, + value: encoded, + created_at: now, + updated_at: now, + }) + .onConflict((oc) => + oc.columns(["byline_id", "field_id"]).doUpdateSet({ + value: encoded, + updated_at: now, + }), + ) + .execute(); + } + } else { + if (value === null) { + await trx + .deleteFrom("_emdash_byline_field_group_values") + .where("translation_group", "=", group) + .where("field_id", "=", field.id) + .execute(); + } else { + const encoded = JSON.stringify(value); + await trx + .insertInto("_emdash_byline_field_group_values") + .values({ + translation_group: group, + field_id: field.id, + value: encoded, + created_at: now, + updated_at: now, + }) + .onConflict((oc) => + oc.columns(["translation_group", "field_id"]).doUpdateSet({ + value: encoded, + updated_at: now, + }), + ) + .execute(); + } + } + } + }); + + // Invalidate the request-scope group-shared cache after the + // transaction has committed. Done outside the transaction so + // the invalidation order is "write, then expose the change to + // readers" — readers that picked up the cache before this + // point are out of date; the next read will fetch fresh. + if (touchedGroupShared) { + clearRequestCacheEntry(`byline-field-group-values:${group}`); + } + } + return await this.findById(id); } /** * Delete a byline row. When this row is the last sibling in its - * translation group, also drops every junction row pointing at the group - * and clears `primary_byline_id` references. When other siblings remain - * in the group, junctions and `primary_byline_id` pointers stay intact — - * the credit lives on at other locales. + * translation group, also drops every junction row pointing at the group, + * clears `primary_byline_id` references, and removes the byline's + * non-translatable custom-field values. When other siblings remain in + * the group, junctions, `primary_byline_id` pointers, and group-shared + * custom-field values stay intact — the credit (and its shared metadata) + * lives on at other locales. + * + * **Application-level cascade.** The byline domain has standardised on + * app-level cascade rather than trusting FK ON DELETE CASCADE, partly + * because migration 040 had to strip its own FK to support the + * translation_group remap (#1021), and partly so cleanup doesn't + * depend on `PRAGMA foreign_keys = ON` (set in production via + * `connection.ts:60`, but easy to bypass in tests, scripts, and + * one-off tools). Every byline-related deletion table is cleared + * explicitly here: * - * Migration 040 dropped the FK on `_emdash_content_bylines.byline_id`, so - * this cascade is implemented here in application code. + * - `_emdash_byline_field_values` (per-byline translatable values) — + * migration 041 declares FK ON DELETE CASCADE on `byline_id`; the + * explicit DELETE removes the dependency on that pragma. + * - `_emdash_content_bylines` — migration 040 dropped its FK. + * - `ec_*.primary_byline_id` — never had an FK. + * - `_emdash_byline_field_group_values` (translation-group-keyed) — + * keyed by a text column with no FK to bylines, so app-level cleanup + * is the only path. + * + * The FKs that remain (migration 041) serve as defense-in-depth. */ async delete(id: string): Promise { const existing = await this.findById(id); @@ -291,6 +773,14 @@ export class BylineRepository { const group = existing.translationGroup ?? existing.id; await withTransaction(this.db, async (trx) => { + // Per-row translatable custom-field values. Done BEFORE the + // byline row delete so the application-level cleanup is + // observable in the transaction log even if FK enforcement is + // off; migration 041's FK ON DELETE CASCADE would catch any + // row we miss, but the explicit DELETE is what the rest of + // the byline domain expects to see. + await trx.deleteFrom("_emdash_byline_field_values").where("byline_id", "=", id).execute(); + await trx.deleteFrom("_emdash_bylines").where("id", "=", id).execute(); // Count remaining siblings in the translation group. If none @@ -307,6 +797,19 @@ export class BylineRepository { // Last sibling gone: cascade in application code. await trx.deleteFrom("_emdash_content_bylines").where("byline_id", "=", group).execute(); + // Group-shared custom-field values are keyed by translation_group + // (no FK to bylines), so they don't cascade with the byline row. + // Clean them up explicitly so deleting the last sibling of an + // identity doesn't leave orphan group values pointing at a + // vanished translation group. Per-row translatable values + // (`_emdash_byline_field_values` keyed by byline_id) already + // cascaded when each sibling row was deleted, so no extra + // cleanup is needed for that table. + await trx + .deleteFrom("_emdash_byline_field_group_values") + .where("translation_group", "=", group) + .execute(); + const tableNames = await listTablesLike(trx, "ec_%"); for (const tableName of tableNames) { validateIdentifier(tableName, "content table"); @@ -358,11 +861,39 @@ export class BylineRepository { if (options?.locale !== undefined) query = query.where("b.locale", "=", options.locale); const rows = await query.execute(); - return rows.map((row) => ({ - byline: rowToByline(row), - sortOrder: row.sort_order, - roleLabel: row.role_label, + // Reconstruct minimal byline rows to feed `withCustomFields`. The + // JOIN selects the same columns as `BylineRow`, just under the + // `b.` alias — the reshape is mechanical and keeps the hydration + // helper unaware of the JOIN shape. + const bylineRows: BylineRow[] = rows.map((row) => ({ + id: row.id, + slug: row.slug, + display_name: row.display_name, + bio: row.bio, + avatar_media_id: row.avatar_media_id, + website_url: row.website_url, + user_id: row.user_id, + is_guest: row.is_guest, + created_at: row.created_at, + updated_at: row.updated_at, + locale: row.locale, + translation_group: row.translation_group, })); + const hydrated = await this.withCustomFields(bylineRows); + return rows.map((row, i) => { + const byline = hydrated[i]; + if (!byline) { + // Defensive: hydrated and rows are produced in lock-step; + // this branch is unreachable unless `withCustomFields` + // breaks its contract. + throw new Error("getContentBylines: hydration row count mismatch"); + } + return { + byline, + sortOrder: row.sort_order, + roleLabel: row.role_label, + }; + }); } /** @@ -414,11 +945,20 @@ export class BylineRepository { * When callers need per-entry-locale filtering (e.g. a list endpoint * returning entries at mixed locales), they should group the input ids by * the entry's locale and call this method once per group. + * + * When the caller will issue multiple `getContentBylinesMany` calls in + * one request (e.g. per locale bucket) and wants a *single* batched + * customFields hydration over the union of returned bylines, pass + * `skipHydration: true` on each call and finish with + * `hydrateBylineCustomFields(allBylines)`. The returned bylines carry + * `customFields = {}` until that hydration call runs — matching the + * "always populated" invariant from AC #6 — so callers that forget to + * hydrate get an empty map rather than `undefined`. */ async getContentBylinesMany( collectionSlug: string, contentIds: string[], - options?: { locale?: string }, + options?: { locale?: string; skipHydration?: boolean }, ): Promise> { const result = new Map(); if (contentIds.length === 0) return result; @@ -451,11 +991,41 @@ export class BylineRepository { if (options?.locale !== undefined) query = query.where("b.locale", "=", options.locale); const rows = await query.execute(); + const bylineRows: BylineRow[] = rows.map((row) => ({ + id: row.id, + slug: row.slug, + display_name: row.display_name, + bio: row.bio, + avatar_media_id: row.avatar_media_id, + website_url: row.website_url, + user_id: row.user_id, + is_guest: row.is_guest, + created_at: row.created_at, + updated_at: row.updated_at, + locale: row.locale, + translation_group: row.translation_group, + })); - for (const row of rows) { + // When `skipHydration` is set, return BylineSummary objects with + // `customFields = {}`. The caller is responsible for batching + // `hydrateBylineCustomFields` across multiple + // `getContentBylinesMany` calls. Otherwise hydrate per-call — + // the historical behaviour for solo callers. + let bylines: BylineSummary[]; + if (options?.skipHydration === true) { + bylines = bylineRows.map(rowToByline); + for (const b of bylines) b.customFields = {}; + } else { + bylines = await this.withCustomFields(bylineRows); + } + + for (let i = 0; i < rows.length; i++) { + const row = rows[i]; + const byline = bylines[i]; + if (!row || !byline) continue; const contentId = row.content_id; const credit: ContentBylineCredit = { - byline: rowToByline(row), + byline, sortOrder: row.sort_order, roleLabel: row.role_label, }; @@ -474,10 +1044,18 @@ export class BylineRepository { /** * Batch-fetch byline profiles linked to user IDs in a single query. * Strict-locale variant of `findByUserId`. + * + * `skipHydration: true` returns bylines with `customFields = {}` so + * callers issuing multiple `findByUserIds` calls in one request (e.g. + * the per-locale-bucket author-fallback path in `getBylinesForEntries`) + * can defer customFields hydration to a single batched + * `hydrateBylineCustomFields` call across the union — keeping the + * Phase 3 query-count envelope at "+1 group-shared query per + * hydration pass" even when buckets fetch disjoint author bylines. */ async findByUserIds( userIds: string[], - options?: { locale?: string }, + options?: { locale?: string; skipHydration?: boolean }, ): Promise> { const result = new Map(); if (userIds.length === 0) return result; @@ -487,11 +1065,19 @@ export class BylineRepository { if (options?.locale !== undefined) query = query.where("locale", "=", options.locale); const rows = await query.execute(); + let bylines: BylineSummary[]; + if (options?.skipHydration === true) { + bylines = rows.map(rowToByline); + for (const b of bylines) b.customFields = {}; + } else { + bylines = await this.withCustomFields(rows); + } - for (const row of rows) { - if (row.user_id) { - result.set(row.user_id, rowToByline(row)); - } + for (let i = 0; i < rows.length; i++) { + const row = rows[i]; + const summary = bylines[i]; + if (!row || !summary || !row.user_id) continue; + result.set(row.user_id, summary); } } return result; diff --git a/packages/core/tests/unit/database/repositories/byline.test.ts b/packages/core/tests/unit/database/repositories/byline.test.ts index 039ebf6c4..5a38e01fb 100644 --- a/packages/core/tests/unit/database/repositories/byline.test.ts +++ b/packages/core/tests/unit/database/repositories/byline.test.ts @@ -1,9 +1,17 @@ -import type { Kysely } from "kysely"; +import { sql, type Kysely } from "kysely"; import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import { + getBylineFieldDefs, + resetBylineFieldDefsCacheForTests, +} from "../../../../src/bylines/field-defs-cache.js"; import { BylineRepository } from "../../../../src/database/repositories/byline.js"; import { ContentRepository } from "../../../../src/database/repositories/content.js"; +import { EmDashValidationError } from "../../../../src/database/repositories/types.js"; import type { Database } from "../../../../src/database/types.js"; +import { peekRequestCache } from "../../../../src/request-cache.js"; +import { runWithContext } from "../../../../src/request-context.js"; +import { BylineSchemaRegistry } from "../../../../src/schema/byline-registry.js"; import { SQL_BATCH_SIZE } from "../../../../src/utils/chunks.js"; import { setupTestDatabaseWithCollections, teardownTestDatabase } from "../../../utils/test-db.js"; @@ -863,4 +871,975 @@ describe("BylineRepository", () => { ).rejects.toThrow(); }); }); + + describe("customFields hydration (Phase 3, #1174)", () => { + let registry: BylineSchemaRegistry; + + beforeEach(() => { + // Each test mutates the registry directly; the per-isolate + // field-defs cache must start fresh so version-counter reads + // see this test's mutations and not a leftover from a sibling. + resetBylineFieldDefsCacheForTests(); + registry = new BylineSchemaRegistry(db); + }); + + it("populates customFields as {} when no fields are registered", async () => { + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + + const found = await bylineRepo.findById(byline.id); + expect(found?.customFields).toEqual({}); + + const list = await bylineRepo.findMany(); + expect(list.items[0]?.customFields).toEqual({}); + }); + + it("hydrates translatable values per locale variant", async () => { + const enRow = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + const frRow = await bylineRepo.create({ + slug: "jeanne", + displayName: "Jeanne", + locale: "fr", + translationOf: enRow.id, + }); + + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + translatable: true, + }); + const field = await registry.getField("job_title"); + + // Seed translatable values via raw INSERT — Phase 3's write + // path is exercised separately below. + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES (${enRow.id}, ${field?.id}, '"Editor"') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES (${frRow.id}, ${field?.id}, '"Rédacteur"') + `.execute(db); + resetBylineFieldDefsCacheForTests(); + + const en = await bylineRepo.findById(enRow.id); + const fr = await bylineRepo.findById(frRow.id); + expect(en?.customFields?.job_title).toBe("Editor"); + expect(fr?.customFields?.job_title).toBe("Rédacteur"); + }); + + it("hydrates group-shared values identically across every locale variant", async () => { + const enRow = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + const frRow = await bylineRepo.create({ + slug: "jeanne", + displayName: "Jeanne", + locale: "fr", + translationOf: enRow.id, + }); + const deRow = await bylineRepo.create({ + slug: "jane-de", + displayName: "Jane (DE)", + locale: "de", + translationOf: enRow.id, + }); + + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + translatable: false, + }); + const field = await registry.getField("twitter_handle"); + + await sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES (${enRow.translationGroup ?? enRow.id}, ${field?.id}, '"@jane"') + `.execute(db); + resetBylineFieldDefsCacheForTests(); + + const en = await bylineRepo.findById(enRow.id); + const fr = await bylineRepo.findById(frRow.id); + const de = await bylineRepo.findById(deRow.id); + expect(en?.customFields?.twitter_handle).toBe("@jane"); + expect(fr?.customFields?.twitter_handle).toBe("@jane"); + expect(de?.customFields?.twitter_handle).toBe("@jane"); + }); + + it("mixes translatable and group-shared values on the same byline", async () => { + const en = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + const fr = await bylineRepo.create({ + slug: "jeanne", + displayName: "Jeanne", + locale: "fr", + translationOf: en.id, + }); + + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + translatable: true, + }); + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + translatable: false, + }); + const job = await registry.getField("job_title"); + const tw = await registry.getField("twitter_handle"); + + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES (${en.id}, ${job?.id}, '"Editor"') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES (${fr.id}, ${job?.id}, '"Rédacteur"') + `.execute(db); + await sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES (${en.translationGroup ?? en.id}, ${tw?.id}, '"@jane"') + `.execute(db); + resetBylineFieldDefsCacheForTests(); + + const reloadedEn = await bylineRepo.findById(en.id); + expect(reloadedEn?.customFields).toEqual({ + job_title: "Editor", + twitter_handle: "@jane", + }); + const reloadedFr = await bylineRepo.findById(fr.id); + expect(reloadedFr?.customFields).toEqual({ + job_title: "Rédacteur", + twitter_handle: "@jane", + }); + }); + + it("hydrates customFields across findMany / findByTranslationGroup / findByUserIds", async () => { + await sql` + INSERT INTO users (id, email, role) VALUES ('u1', 'u1@example.com', 50) + `.execute(db); + + const en = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + userId: "u1", + }); + const fr = await bylineRepo.create({ + slug: "jeanne", + displayName: "Jeanne", + locale: "fr", + translationOf: en.id, + }); + + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + translatable: true, + }); + const job = await registry.getField("job_title"); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES (${en.id}, ${job?.id}, '"Editor"'), + (${fr.id}, ${job?.id}, '"Rédacteur"') + `.execute(db); + resetBylineFieldDefsCacheForTests(); + + const many = await bylineRepo.findMany(); + for (const item of many.items) { + expect(item.customFields).toBeDefined(); + } + + const siblings = await bylineRepo.findByTranslationGroup(en.translationGroup ?? en.id); + expect(siblings.find((b) => b.locale === "en")?.customFields?.job_title).toBe("Editor"); + expect(siblings.find((b) => b.locale === "fr")?.customFields?.job_title).toBe("Rédacteur"); + + const byUser = await bylineRepo.findByUserIds(["u1"], { locale: "en" }); + expect(byUser.get("u1")?.customFields?.job_title).toBe("Editor"); + }); + + it("hydrates customFields onto ContentBylineCredit via getContentBylines", async () => { + const content = await contentRepo.create({ + type: "post", + slug: "hello", + data: { title: "Hello" }, + }); + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await bylineRepo.setContentBylines("post", content.id, [{ bylineId: byline.id }]); + + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + }); + const field = await registry.getField("job_title"); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES (${byline.id}, ${field?.id}, '"Editor"') + `.execute(db); + resetBylineFieldDefsCacheForTests(); + + const credits = await bylineRepo.getContentBylines("post", content.id); + expect(credits[0]?.byline.customFields?.job_title).toBe("Editor"); + }); + + it("hydrates customFields per-row when the same byline appears multiple times in one batch", async () => { + // Regression for the Phase 3 review's "duplicate dedup" finding: + // withCustomFields used to collapse `summaries` into a Map keyed + // by byline.id, silently losing earlier duplicates' merge step. + // `getContentBylinesMany` on a list view with repeated authors + // hits this exact case — every duplicate must carry the values. + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + }); + const field = await registry.getField("job_title"); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES (${byline.id}, ${field?.id}, '"Editor"') + `.execute(db); + resetBylineFieldDefsCacheForTests(); + + // Two posts crediting the same author. getContentBylinesMany + // will return rows for both posts referencing the same byline_id. + const post1 = await contentRepo.create({ + type: "post", + slug: "post-1", + data: { title: "Post 1" }, + }); + const post2 = await contentRepo.create({ + type: "post", + slug: "post-2", + data: { title: "Post 2" }, + }); + await bylineRepo.setContentBylines("post", post1.id, [{ bylineId: byline.id }]); + await bylineRepo.setContentBylines("post", post2.id, [{ bylineId: byline.id }]); + + const credits = await bylineRepo.getContentBylinesMany("post", [post1.id, post2.id]); + const post1Credits = credits.get(post1.id) ?? []; + const post2Credits = credits.get(post2.id) ?? []; + + // Both posts must see the byline with its customFields populated + // — not just the last duplicate. + expect(post1Credits[0]?.byline.customFields?.job_title).toBe("Editor"); + expect(post2Credits[0]?.byline.customFields?.job_title).toBe("Editor"); + }); + + it("hydrateBylineCustomFields batches disjoint-group hydration in one pass", async () => { + // Regression for the Phase 3 review's "disjoint-groups still + // issue one query per locale bucket" finding. When sibling + // locales reference *different* translation_groups (the case + // where my earlier per-group cache fix degenerated to N + // queries), batched hydration via `hydrateBylineCustomFields` + // over the union of all bylines fires a SINGLE group-shared + // query — meeting the AC envelope of "+1 per hydration pass". + // + // We assert behaviourally: skipHydration returns bylines with + // customFields={}, hydrateBylineCustomFields populates them in + // one shared call, and per-group cache entries land for every + // disjoint group from the union. + const aEn = await bylineRepo.create({ slug: "a-en", displayName: "A", locale: "en" }); + const bFr = await bylineRepo.create({ slug: "b-fr", displayName: "B", locale: "fr" }); + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + translatable: false, + }); + const field = await registry.getField("twitter_handle"); + + // Two disjoint translation_groups, one per locale identity. + const aGroup = aEn.translationGroup ?? aEn.id; + const bGroup = bFr.translationGroup ?? bFr.id; + expect(aGroup).not.toBe(bGroup); + + await sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES (${aGroup}, ${field?.id}, '"@a"'), + (${bGroup}, ${field?.id}, '"@b"') + `.execute(db); + resetBylineFieldDefsCacheForTests(); + + const enContent = await contentRepo.create({ + type: "post", + slug: "post-en", + data: { title: "EN" }, + locale: "en", + }); + const frContent = await contentRepo.create({ + type: "post", + slug: "post-fr", + data: { title: "FR" }, + locale: "fr", + }); + await bylineRepo.setContentBylines("post", enContent.id, [{ bylineId: aEn.id }]); + await bylineRepo.setContentBylines("post", frContent.id, [{ bylineId: bFr.id }]); + + await runWithContext({ db: undefined, dbIsIsolated: false, metrics: undefined }, async () => { + // Per-bucket calls return un-hydrated bylines. + const enMap = await bylineRepo.getContentBylinesMany("post", [enContent.id], { + locale: "en", + skipHydration: true, + }); + const frMap = await bylineRepo.getContentBylinesMany("post", [frContent.id], { + locale: "fr", + skipHydration: true, + }); + + const enBylines = (enMap.get(enContent.id) ?? []).map((c) => c.byline); + const frBylines = (frMap.get(frContent.id) ?? []).map((c) => c.byline); + for (const b of [...enBylines, ...frBylines]) { + expect(b.customFields).toEqual({}); + } + + // Before hydration, the request cache should have NO + // group-value entries (skipHydration skips the fetch). + expect(peekRequestCache(`byline-field-group-values:${aGroup}`)).toBeUndefined(); + expect(peekRequestCache(`byline-field-group-values:${bGroup}`)).toBeUndefined(); + + // One batched hydration call over the union. + await bylineRepo.hydrateBylineCustomFields([...enBylines, ...frBylines]); + + // Values populated, cache primed for both disjoint groups. + expect(enBylines[0]?.customFields?.twitter_handle).toBe("@a"); + expect(frBylines[0]?.customFields?.twitter_handle).toBe("@b"); + expect(peekRequestCache(`byline-field-group-values:${aGroup}`)).toBeDefined(); + expect(peekRequestCache(`byline-field-group-values:${bGroup}`)).toBeDefined(); + }); + }); + + it("dedupes group-shared queries across locale buckets within one request", async () => { + // Regression for the Phase 3 review's "group-shared value query + // per locale bucket" finding. PR plan AC #8: group-shared is + // "+1 batched query per hydration pass" — meaning per request, + // not per repository call. Mixed-locale list views share + // translation_groups across sibling locales, so the second + // bucket's hydration must reuse the first's group-shared fetch. + + const en = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + const fr = await bylineRepo.create({ + slug: "jeanne", + displayName: "Jeanne", + locale: "fr", + translationOf: en.id, + }); + + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + translatable: false, + }); + const field = await registry.getField("twitter_handle"); + const group = en.translationGroup ?? en.id; + await sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES (${group}, ${field?.id}, '"@jane"') + `.execute(db); + resetBylineFieldDefsCacheForTests(); + + // Two separate findById calls within the same request context. + // The request cache should ensure the group-shared values are + // fetched once and reused on the second call. + const results = await runWithContext( + { db: undefined, dbIsIsolated: false, metrics: undefined }, + async () => { + const enResult = await bylineRepo.findById(en.id); + // After the first call primed the cache, the second + // call's loadGroupValues should hit `peekRequestCache` + // for the same translation_group. + const cached = peekRequestCache(`byline-field-group-values:${group}`); + expect(cached).toBeDefined(); + const frResult = await bylineRepo.findById(fr.id); + return { enResult, frResult }; + }, + ); + + expect(results.enResult?.customFields?.twitter_handle).toBe("@jane"); + expect(results.frResult?.customFields?.twitter_handle).toBe("@jane"); + }); + + it("tolerates orphaned value rows (definition deleted but value lingering)", async () => { + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); + const field = await registry.getField("job_title"); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES (${byline.id}, ${field?.id}, '"Editor"') + `.execute(db); + + // Bypass the registry to leave the value row behind (the + // registry's app-level cascade would normally clean it up). + await sql`DELETE FROM _emdash_byline_fields WHERE id = ${field?.id}`.execute(db); + resetBylineFieldDefsCacheForTests(); + + const reloaded = await bylineRepo.findById(byline.id); + // Orphan row's slug is no longer in the registry → it doesn't + // surface in customFields. Hydration must not throw. + expect(reloaded?.customFields).toEqual({}); + }); + }); + + describe("field-defs cache + dbIsIsolated bypass (Phase 3, #1174)", () => { + // Regression for the Phase 3 review's "global cache ignores + // dbIsIsolated" finding. Playground/DO-preview requests set + // `dbIsIsolated = true`; schema-derived caches must not be shared + // with the singleton (loader.ts:74 precedent). We assert this by + // populating the global cache from a non-isolated request, then + // observing that an isolated request bypasses the cache and reads + // fresh from its own DB. + beforeEach(() => { + resetBylineFieldDefsCacheForTests(); + }); + + it("an isolated request never consults the global holder (matched-version case)", async () => { + // Definitive test for the bypass: both DBs land at the same + // version counter with DIFFERENT defs. Without the bypass, the + // isolated request's version-keyed cache lookup would match + // the main DB's cached entry and return the WRONG defs. + const mainRegistry = new BylineSchemaRegistry(db); + await mainRegistry.createField({ slug: "main_only", label: "Main", type: "string" }); + + // Warm the global holder against the main DB. + const mainDefs = await getBylineFieldDefs(db); + expect(mainDefs.map((d) => d.slug)).toEqual(["main_only"]); + expect(await mainRegistry.getVersion()).toBe(2); // 1 create × 2 bumps + + const isolatedDb = await setupTestDatabaseWithCollections(); + try { + const isolatedRegistry = new BylineSchemaRegistry(isolatedDb); + await isolatedRegistry.createField({ + slug: "isolated_only", + label: "Isolated", + type: "string", + }); + // Both DBs are at version 2 — a version-keyed cache without + // the dbIsIsolated bypass would return `main_only` here. + expect(await isolatedRegistry.getVersion()).toBe(2); + + const result = await runWithContext({ db: isolatedDb, dbIsIsolated: true }, async () => + getBylineFieldDefs(isolatedDb), + ); + expect(result.map((d) => d.slug)).toEqual(["isolated_only"]); + expect(result.map((d) => d.slug)).not.toContain("main_only"); + } finally { + await teardownTestDatabase(isolatedDb); + } + }); + + it("an isolated request does not poison the global holder", async () => { + const isolatedDb = await setupTestDatabaseWithCollections(); + try { + const isolatedRegistry = new BylineSchemaRegistry(isolatedDb); + await isolatedRegistry.createField({ + slug: "isolated_only", + label: "Isolated", + type: "string", + }); + + const isolatedDefs = await runWithContext( + { db: isolatedDb, dbIsIsolated: true }, + async () => getBylineFieldDefs(isolatedDb), + ); + expect(isolatedDefs.map((d) => d.slug)).toEqual(["isolated_only"]); + } finally { + await teardownTestDatabase(isolatedDb); + } + + // Main DB has nothing registered. The global holder should be + // empty — the isolated read above must not have written to it. + // We verify by reading from main and asserting nothing leaks. + const mainDefs = await getBylineFieldDefs(db); + expect(mainDefs).toEqual([]); + expect(mainDefs.map((d) => d.slug)).not.toContain("isolated_only"); + }); + }); + + describe("delete with custom-field cleanup (Phase 3, #1174)", () => { + let registry: BylineSchemaRegistry; + + beforeEach(() => { + resetBylineFieldDefsCacheForTests(); + registry = new BylineSchemaRegistry(db); + }); + + it("removes group-shared values when the last sibling of an identity is deleted", async () => { + // Regression for the Phase 3 review's "_emdash_byline_field_group_values + // has no FK to bylines" finding. Deleting the last byline in a + // translation group must explicitly remove group-shared values — + // FK cascade can't reach them because the key column is just + // `translation_group TEXT`. + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + translatable: false, + }); + const field = await registry.getField("twitter_handle"); + const group = byline.translationGroup ?? byline.id; + await sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES (${group}, ${field?.id}, '"@jane"') + `.execute(db); + + await bylineRepo.delete(byline.id); + + const remaining = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_group_values + WHERE translation_group = ${group} + `.execute(db); + expect(Number(remaining.rows[0]?.count ?? -1)).toBe(0); + }); + + it("preserves group-shared values when a non-last sibling is deleted", async () => { + // The shared metadata must outlive the deletion of one locale — + // only the *last* sibling's removal should drop the group values. + const en = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await bylineRepo.create({ + slug: "jeanne", + displayName: "Jeanne", + locale: "fr", + translationOf: en.id, + }); + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + translatable: false, + }); + const field = await registry.getField("twitter_handle"); + const group = en.translationGroup ?? en.id; + await sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES (${group}, ${field?.id}, '"@jane"') + `.execute(db); + + await bylineRepo.delete(en.id); + + const remaining = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_group_values + WHERE translation_group = ${group} + `.execute(db); + // Still 1 — the French sibling lives, the group value lives. + expect(Number(remaining.rows[0]?.count ?? -1)).toBe(1); + }); + + it("clears per-row translatable values via app-level cleanup (works without FK pragma)", async () => { + // Regression for the Phase 3 review's "BylineRepository.delete + // still relies on FK cascade for _emdash_byline_field_values" + // finding. The byline domain has standardised on app-level + // cleanup so deletion doesn't depend on `PRAGMA foreign_keys`. + // Explicitly disable the pragma to prove the cleanup is + // app-level, not FK-mediated. + await sql`PRAGMA foreign_keys = OFF`.execute(db); + + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + translatable: true, + }); + const field = await registry.getField("job_title"); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES (${byline.id}, ${field?.id}, '"Editor"') + `.execute(db); + + await bylineRepo.delete(byline.id); + + const remaining = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_values + WHERE byline_id = ${byline.id} + `.execute(db); + expect(Number(remaining.rows[0]?.count ?? -1)).toBe(0); + }); + + it("FK ON DELETE CASCADE on _emdash_byline_field_values still serves as defense-in-depth", async () => { + // Companion to the test above: with FK ON, bypassing the + // repository and deleting the byline row directly should still + // trigger the FK cascade. Keeps both layers of cleanup + // asserted so neither rots. + await sql`PRAGMA foreign_keys = ON`.execute(db); + + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + translatable: true, + }); + const field = await registry.getField("job_title"); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES (${byline.id}, ${field?.id}, '"Editor"') + `.execute(db); + + await sql`DELETE FROM _emdash_bylines WHERE id = ${byline.id}`.execute(db); + + const remaining = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_values + WHERE byline_id = ${byline.id} + `.execute(db); + expect(Number(remaining.rows[0]?.count ?? -1)).toBe(0); + }); + + it("cascades per-row translatable values via FK when any sibling is deleted", async () => { + // Translatable values are keyed by byline_id and have FK ON + // DELETE CASCADE (migration 041). Deleting a byline row drops + // its own values regardless of last-sibling status. + const en = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + const fr = await bylineRepo.create({ + slug: "jeanne", + displayName: "Jeanne", + locale: "fr", + translationOf: en.id, + }); + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + translatable: true, + }); + const field = await registry.getField("job_title"); + await sql` + INSERT INTO _emdash_byline_field_values (byline_id, field_id, value) + VALUES (${en.id}, ${field?.id}, '"Editor"'), + (${fr.id}, ${field?.id}, '"Rédacteur"') + `.execute(db); + + await bylineRepo.delete(en.id); + + // en's value is gone (FK cascade). fr's value remains. + const remainingEn = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_values + WHERE byline_id = ${en.id} + `.execute(db); + const remainingFr = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_values + WHERE byline_id = ${fr.id} + `.execute(db); + expect(Number(remainingEn.rows[0]?.count ?? -1)).toBe(0); + expect(Number(remainingFr.rows[0]?.count ?? -1)).toBe(1); + }); + }); + + describe("update with customFields write path (Phase 3, #1174)", () => { + let registry: BylineSchemaRegistry; + + beforeEach(() => { + resetBylineFieldDefsCacheForTests(); + registry = new BylineSchemaRegistry(db); + }); + + it("writes a translatable value and round-trips via findById", async () => { + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + translatable: true, + }); + + const updated = await bylineRepo.update(byline.id, { + customFields: { job_title: "Editor" }, + }); + expect(updated?.customFields?.job_title).toBe("Editor"); + + const reloaded = await bylineRepo.findById(byline.id); + expect(reloaded?.customFields?.job_title).toBe("Editor"); + }); + + it("writes a non-translatable value to the group-shared table", async () => { + const en = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + const fr = await bylineRepo.create({ + slug: "jeanne", + displayName: "Jeanne", + locale: "fr", + translationOf: en.id, + }); + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + translatable: false, + }); + + await bylineRepo.update(en.id, { customFields: { twitter_handle: "@jane" } }); + + // Same value visible on every sibling. + const enRel = await bylineRepo.findById(en.id); + const frRel = await bylineRepo.findById(fr.id); + expect(enRel?.customFields?.twitter_handle).toBe("@jane"); + expect(frRel?.customFields?.twitter_handle).toBe("@jane"); + + // Storage went to the group table, not the per-row table. + const tr = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_values + `.execute(db); + const grp = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_group_values + `.execute(db); + expect(Number(tr.rows[0]?.count ?? -1)).toBe(0); + expect(Number(grp.rows[0]?.count ?? -1)).toBe(1); + }); + + it("upsert is idempotent — re-applying the same write yields the same row", async () => { + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + }); + + await bylineRepo.update(byline.id, { customFields: { job_title: "Editor" } }); + await bylineRepo.update(byline.id, { customFields: { job_title: "Editor" } }); + + const rows = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_values + WHERE byline_id = ${byline.id} + `.execute(db); + expect(Number(rows.rows[0]?.count ?? -1)).toBe(1); + }); + + it("overwrites an existing value on subsequent updates", async () => { + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + }); + + await bylineRepo.update(byline.id, { customFields: { job_title: "Editor" } }); + await bylineRepo.update(byline.id, { customFields: { job_title: "Senior Editor" } }); + + const reloaded = await bylineRepo.findById(byline.id); + expect(reloaded?.customFields?.job_title).toBe("Senior Editor"); + }); + + it("clears a value when written as null (deletes the row)", async () => { + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + }); + + await bylineRepo.update(byline.id, { customFields: { job_title: "Editor" } }); + await bylineRepo.update(byline.id, { customFields: { job_title: null } }); + + const reloaded = await bylineRepo.findById(byline.id); + // Cleared field is absent from the map (not present as null) — + // the row is deleted, so hydration finds no entry. + expect(reloaded?.customFields?.job_title).toBeUndefined(); + expect(reloaded?.customFields).toEqual({}); + }); + + it("throws EmDashValidationError on unknown slugs (no partial write)", async () => { + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + }); + + await expect( + bylineRepo.update(byline.id, { + customFields: { job_title: "Editor", unknown_field: "value" }, + }), + ).rejects.toBeInstanceOf(EmDashValidationError); + + // Validation runs before any DB write, so the registered field + // also did not get written. + const rows = await sql<{ count: number }>` + SELECT COUNT(*) AS count FROM _emdash_byline_field_values + `.execute(db); + expect(Number(rows.rows[0]?.count ?? -1)).toBe(0); + }); + + it("rejects type mismatches with EmDashValidationError", async () => { + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ slug: "active", label: "Active", type: "boolean" }); + + await expect( + bylineRepo.update(byline.id, { customFields: { active: "not a boolean" } }), + ).rejects.toBeInstanceOf(EmDashValidationError); + }); + + it("rejects select values outside the registered options", async () => { + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "role", + label: "Role", + type: "select", + validation: { options: ["editor", "author"] }, + }); + + await expect( + bylineRepo.update(byline.id, { customFields: { role: "owner" } }), + ).rejects.toBeInstanceOf(EmDashValidationError); + + // Accepted choices land. + const ok = await bylineRepo.update(byline.id, { customFields: { role: "editor" } }); + expect(ok?.customFields?.role).toBe("editor"); + }); + + it("invalidates the per-request group-shared cache after a non-translatable write", async () => { + // Regression for the Phase 3 review's finding that update() + // primes the group-shared request cache via its opening + // findById, then writes to _emdash_byline_field_group_values + // later in the same method. Without explicit invalidation, + // the closing findById (and any later reads in the same + // request) would return stale customFields. `update` calls + // `clearRequestCacheEntry` after group-shared writes so + // subsequent reads in the request see the fresh value. + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + translatable: false, + }); + + const result = await runWithContext( + { db: undefined, dbIsIsolated: false, metrics: undefined }, + async () => { + // Prime the cache by reading first — exactly what an API + // route does (load the resource before mutating it). + const before = await bylineRepo.findById(byline.id); + expect(before?.customFields).toEqual({}); + + // Write the group-shared value. + const after = await bylineRepo.update(byline.id, { + customFields: { twitter_handle: "@jane" }, + }); + // update's closing findById must reflect the write, not the + // stale cache from the opening findById. + expect(after?.customFields?.twitter_handle).toBe("@jane"); + + // A LATER findById in the same request must also see the + // new value — proves the cache is fully cleared. + const later = await bylineRepo.findById(byline.id); + return later?.customFields?.twitter_handle; + }, + ); + + expect(result).toBe("@jane"); + }); + + it("does not write customFields when the input omits the key entirely", async () => { + const byline = await bylineRepo.create({ + slug: "jane", + displayName: "Jane", + locale: "en", + }); + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + }); + await bylineRepo.update(byline.id, { customFields: { job_title: "Editor" } }); + + // Plain update without customFields key must leave existing values + // alone (no implicit clear, no implicit re-write). + await bylineRepo.update(byline.id, { displayName: "Jane Doe" }); + + const reloaded = await bylineRepo.findById(byline.id); + expect(reloaded?.displayName).toBe("Jane Doe"); + expect(reloaded?.customFields?.job_title).toBe("Editor"); + }); + }); }); From b806591c64cd1a97e5cb68e2c7b9fbcc9c16d34b Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 16:46:41 +0100 Subject: [PATCH 08/35] feat(bylines): batched customFields hydration for getBylinesForEntries --- packages/core/src/bylines/index.ts | 41 +++++- .../tests/unit/bylines/bylines-query.test.ts | 125 ++++++++++++++++++ 2 files changed, 162 insertions(+), 4 deletions(-) diff --git a/packages/core/src/bylines/index.ts b/packages/core/src/bylines/index.ts index 7ae4c133e..0f94d3588 100644 --- a/packages/core/src/bylines/index.ts +++ b/packages/core/src/bylines/index.ts @@ -220,10 +220,20 @@ export async function getBylinesForEntries( // previous "has any bylines" probe, without the extra round-trip. // Pre-migration databases (bylines table missing) fall through to the // `isMissingTableError` catch below and return empty. + // + // Each bucket's `getContentBylinesMany` call uses `skipHydration: true` + // so the per-bucket fetches return bylines with `customFields = {}`. + // We then hydrate the union of returned bylines in a SINGLE batched + // pass via `hydrateBylineCustomFields`. This keeps mixed-locale list + // hydration at one batched group-shared query (and one batched + // translatable query) per request, even when locale buckets reference + // disjoint translation_groups — the strict reading of the Phase 3 + // query-count envelope. const explicitByEntry = new Map(); const entriesNeedingAuthorCheck: BylineEntry[] = []; + const hydrationTargets: BylineSummary[] = []; for (const [locale, bucket] of buckets) { - const localeOpt = locale ? { locale } : undefined; + const localeOpt = locale ? { locale, skipHydration: true } : { skipHydration: true }; const bucketIds = bucket.map((e) => e.id); let bylinesMap; try { @@ -232,7 +242,10 @@ export async function getBylinesForEntries( if (isMissingTableError(error)) return result; throw error; } - for (const [id, list] of bylinesMap) explicitByEntry.set(id, list); + for (const [id, list] of bylinesMap) { + explicitByEntry.set(id, list); + for (const credit of list) hydrationTargets.push(credit.byline); + } for (const entry of bucket) { const hasResolved = bylinesMap.has(entry.id) && bylinesMap.get(entry.id)!.length > 0; @@ -255,19 +268,39 @@ export async function getBylinesForEntries( } for (const [locale, bucket] of authorBuckets) { - const localeOpt = locale ? { locale } : undefined; + const localeOpt: { locale?: string; skipHydration: true } = locale + ? { locale, skipHydration: true } + : { skipHydration: true }; const authorIds = bucket.map((e) => e.authorId).filter((id): id is string => id !== null); const uniqueAuthorIds = [...new Set(authorIds)]; if (uniqueAuthorIds.length === 0) continue; + // `skipHydration: true` returns bylines with `customFields = {}` + // so the fallback path participates in the single batched + // `hydrateBylineCustomFields` call below — keeping the query + // envelope at "+1 group-shared query per hydration pass" even + // when author bylines across locale buckets reference disjoint + // translation_groups. const authorBylineMap = await repo.findByUserIds(uniqueAuthorIds, localeOpt); for (const entry of bucket) { if (!entry.authorId) continue; const f = authorBylineMap.get(entry.authorId); - if (f) fallbackByEntry.set(entry.id, f); + if (f) { + fallbackByEntry.set(entry.id, f); + hydrationTargets.push(f); + } } } } + // Single batched hydration over every byline returned from both the + // per-bucket explicit-credit fetches AND the per-bucket author- + // fallback fetches. One translatable query + one group-shared query + // for the whole pass, regardless of bucket count or whether + // translation_groups overlap across locales. + if (hydrationTargets.length > 0) { + await repo.hydrateBylineCustomFields(hydrationTargets); + } + for (const { id } of entries) { const explicit = explicitByEntry.get(id); if (explicit && explicit.length > 0) { diff --git a/packages/core/tests/unit/bylines/bylines-query.test.ts b/packages/core/tests/unit/bylines/bylines-query.test.ts index 007c8ddbb..f1294167e 100644 --- a/packages/core/tests/unit/bylines/bylines-query.test.ts +++ b/packages/core/tests/unit/bylines/bylines-query.test.ts @@ -354,6 +354,131 @@ describe("Byline query functions", () => { expect(result.get(post.id)?.[0]?.byline.displayName).toBe("Batch Author"); }); + it("hydrates customFields on author-fallback bylines across disjoint locale buckets", async () => { + // Regression for the Phase 3 review's "author-fallback still + // calls findByUserIds per bucket and hydrates inside each + // call" finding. Even with the explicit-credit path batched, + // the per-bucket findByUserIds calls each fired their own + // group-shared query for the author's translation_group — and + // when authors differ per locale (the disjoint case), the + // groups don't overlap, so the per-bucket caches don't help. + // + // Fix: `findByUserIds` gains `skipHydration`, the author- + // fallback path uses it, and the fallback bylines feed into + // the same `hydrationTargets` array as the explicit credits. + // One batched `hydrateBylineCustomFields` covers all bylines + // at the end of `getBylinesForEntries`. + const userRepo = new UserRepository(db); + + // Two distinct authors (disjoint identities → disjoint + // translation_groups), one credited in each locale. + const userEn = await userRepo.create({ + email: "en-author@example.com", + displayName: "EN Author", + role: "editor", + }); + const userFr = await userRepo.create({ + email: "fr-author@example.com", + displayName: "FR Author", + role: "editor", + }); + + const bylineEn = await bylineRepo.create({ + slug: "en-author", + displayName: "EN Author", + userId: userEn.id, + locale: "en", + }); + const bylineFr = await bylineRepo.create({ + slug: "fr-author", + displayName: "FR Author", + userId: userFr.id, + locale: "fr", + }); + + // Register a non-translatable byline custom field and seed + // distinct values for each author's translation_group. Verifying + // that BOTH authors' values surface after a single + // getBylinesForEntries call proves the batched hydration + // covers the fallback path. + const { BylineSchemaRegistry } = await import("../../../src/schema/byline-registry.js"); + const { sql } = await import("kysely"); + const registry = new BylineSchemaRegistry(db); + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "string", + translatable: false, + }); + const field = await registry.getField("twitter_handle"); + + const enGroup = bylineEn.translationGroup ?? bylineEn.id; + const frGroup = bylineFr.translationGroup ?? bylineFr.id; + expect(enGroup).not.toBe(frGroup); + await sql` + INSERT INTO _emdash_byline_field_group_values (translation_group, field_id, value) + VALUES (${enGroup}, ${field?.id}, '"@en"'), + (${frGroup}, ${field?.id}, '"@fr"') + `.execute(db); + const { resetBylineFieldDefsCacheForTests } = + await import("../../../src/bylines/field-defs-cache.js"); + resetBylineFieldDefsCacheForTests(); + + const enPost = await contentRepo.create({ + type: "post", + slug: "en-post", + data: { title: "EN" }, + locale: "en", + authorId: userEn.id, + }); + const frPost = await contentRepo.create({ + type: "post", + slug: "fr-post", + data: { title: "FR" }, + locale: "fr", + authorId: userFr.id, + }); + + // Count queries against `_emdash_byline_field_group_values` + // to assert the AC envelope holds. The pre-fix path fired one + // query per locale bucket (2 here, since the buckets reference + // disjoint translation_groups). The fix defers fallback + // hydration into the single batched call and lands at exactly + // one query for the whole pass. + let groupValueQueries = 0; + const originalSelectFrom = db.selectFrom.bind(db); + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- spy on a Kysely chain entry point; the chain values flow through unchanged + const selectFromSpy = vi.spyOn(db, "selectFrom").mockImplementation(((table: any) => { + if (table === "_emdash_byline_field_group_values") groupValueQueries += 1; + return originalSelectFrom(table); + }) as any); + + let result; + try { + result = await getBylinesForEntries("post", [ + { id: enPost.id, locale: "en", authorId: enPost.authorId }, + { id: frPost.id, locale: "fr", authorId: frPost.authorId }, + ]); + } finally { + selectFromSpy.mockRestore(); + } + + const enCredits = result.get(enPost.id) ?? []; + const frCredits = result.get(frPost.id) ?? []; + expect(enCredits[0]?.source).toBe("inferred"); + expect(frCredits[0]?.source).toBe("inferred"); + // Both bylines must carry their group-shared customFields. + expect(enCredits[0]?.byline.customFields?.twitter_handle).toBe("@en"); + expect(frCredits[0]?.byline.customFields?.twitter_handle).toBe("@fr"); + + // Phase 3 AC #8: "+1 batched query per hydration pass" for + // group-shared values — meaning one query for the entire + // `getBylinesForEntries` call regardless of bucket count. The + // pre-fix author-fallback path issued one per bucket; this + // asserts it now lands at one. + expect(groupValueQueries).toBe(1); + }); + it("handles batches larger than SQL_BATCH_SIZE across explicit and inferred bylines", async () => { const userRepo = new UserRepository(db); const explicitByline = await bylineRepo.create({ From d56f33fcf8ca758499fd4d35f0a97fcda147a6a0 Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 21:59:25 +0100 Subject: [PATCH 09/35] feat(bylines): registry helpers, error codes, and reorder slug reservation for admin API --- packages/core/src/api/errors.ts | 7 ++ packages/core/src/schema/byline-registry.ts | 86 +++++++++++++++++++++ packages/core/src/schema/types.ts | 25 +++++- 3 files changed, 114 insertions(+), 4 deletions(-) diff --git a/packages/core/src/api/errors.ts b/packages/core/src/api/errors.ts index ec1569fb6..649ae1d0a 100644 --- a/packages/core/src/api/errors.ts +++ b/packages/core/src/api/errors.ts @@ -61,6 +61,11 @@ export const ErrorCode = { SCHEMA_FIELD_UPDATE_ERROR: "SCHEMA_FIELD_UPDATE_ERROR", SCHEMA_FIELD_DELETE_ERROR: "SCHEMA_FIELD_DELETE_ERROR", SCHEMA_FIELD_REORDER_ERROR: "SCHEMA_FIELD_REORDER_ERROR", + // Byline schema (Discussion #1174). Reuses RESERVED_SLUG, INVALID_SLUG, + // INVALID_TYPE, FIELD_EXISTS, NOT_FOUND, VALIDATION_ERROR where the + // semantics match; the two below are byline-domain specific: + TRANSLATABLE_LOCKED: "TRANSLATABLE_LOCKED", + REORDER_MISMATCH: "REORDER_MISMATCH", ORPHAN_LIST_ERROR: "ORPHAN_LIST_ERROR", ORPHAN_REGISTER_ERROR: "ORPHAN_REGISTER_ERROR", COLLECTION_EXISTS: "COLLECTION_EXISTS", @@ -370,6 +375,7 @@ export function mapErrorStatus(code: string | undefined): number { case ErrorCode.SSRF_BLOCKED: case ErrorCode.UNKNOWN_ACTION: case ErrorCode.AMBIGUOUS_LOCALE: + case ErrorCode.REORDER_MISMATCH: return 400; // 401 Unauthorized @@ -414,6 +420,7 @@ export function mapErrorStatus(code: string | undefined): number { case ErrorCode.ALREADY_INSTALLED: case ErrorCode.ALREADY_CONFIGURED: case ErrorCode.ALREADY_UP_TO_DATE: + case ErrorCode.TRANSLATABLE_LOCKED: return 409; // 410 Gone diff --git a/packages/core/src/schema/byline-registry.ts b/packages/core/src/schema/byline-registry.ts index 8d9dd7cbf..d160f68c3 100644 --- a/packages/core/src/schema/byline-registry.ts +++ b/packages/core/src/schema/byline-registry.ts @@ -56,6 +56,49 @@ export class BylineSchemaError extends Error { } } +/** + * Translate a `BylineSchemaError` code to a shared `ErrorCode` for the + * admin API. HTTP status is then derived by `mapErrorStatus` — this + * function deliberately doesn't carry one, so the API/handler boundary + * matches the rest of the codebase (handlers return `ApiResult` with + * a code, the route layer maps to status via `unwrapResult`). + * + * Every code on the right-hand side of `case ... return ...` is defined + * in `ErrorCode` (`api/errors.ts`). `INVALID_LABEL` and + * `INVALID_VALIDATION` are intentionally folded into the `default` + * branch (→ `VALIDATION_ERROR`) so no ad-hoc codes leak out — the + * registry's domain code names them but the HTTP surface should not. + * + * `RESERVED_SLUG` / `INVALID_SLUG` typically don't reach this layer for + * HTTP callers — the zod schema rejects them first with a clean + * `VALIDATION_ERROR`. They're still listed so non-HTTP callers (and the + * test layer) get consistent mapping. + * + * `FIELD_NOT_FOUND` is normalised to the shared `NOT_FOUND` code so the + * admin client can branch on one constant across resource types. + */ +export function mapBylineSchemaError(error: BylineSchemaError): { + code: string; + message: string; + details?: Record; +} { + switch (error.code) { + case "FIELD_NOT_FOUND": + return { code: "NOT_FOUND", message: error.message, details: error.details }; + case "FIELD_EXISTS": + case "TRANSLATABLE_LOCKED": + case "REORDER_MISMATCH": + case "INVALID_SLUG": + case "RESERVED_SLUG": + case "INVALID_TYPE": + return { code: error.code, message: error.message, details: error.details }; + default: + // Catches INVALID_LABEL, INVALID_VALIDATION, and any future + // registry codes we forget to wire up explicitly. + return { code: "VALIDATION_ERROR", message: error.message, details: error.details }; + } +} + /** * Registry for byline custom fields (Discussion #1174). * @@ -374,6 +417,49 @@ export class BylineSchemaRegistry { }); } + /** + * Per-table usage counts for a field, plus the sum. Backs the + * destructive-delete confirm dialog in the admin UI (Phase 5). + * + * Both counts are surfaced separately for diagnostic value: a + * non-zero count on the table that doesn't match the field's current + * `translatable` flag indicates historical drift (e.g. a flip from + * an older code path). Today the registry rejects such flips with + * `TRANSLATABLE_LOCKED`, so any drift originates pre-Phase-2. + * + * Throws `FIELD_NOT_FOUND` when the slug doesn't resolve — callers + * shouldn't get back zero counts for a missing field. + */ + async getFieldUsage(slug: string): Promise<{ + translatableValueCount: number; + groupValueCount: number; + totalAffectedRows: number; + }> { + const field = await this.getField(slug); + if (!field) { + throw new BylineSchemaError(`Byline field "${slug}" not found`, "FIELD_NOT_FOUND", { + slug, + }); + } + const tr = await this.db + .selectFrom("_emdash_byline_field_values") + .select(({ fn }) => [fn.count("field_id").as("count")]) + .where("field_id", "=", field.id) + .executeTakeFirst(); + const grp = await this.db + .selectFrom("_emdash_byline_field_group_values") + .select(({ fn }) => [fn.count("field_id").as("count")]) + .where("field_id", "=", field.id) + .executeTakeFirst(); + const translatableValueCount = Number(tr?.count ?? 0); + const groupValueCount = Number(grp?.count ?? 0); + return { + translatableValueCount, + groupValueCount, + totalAffectedRows: translatableValueCount + groupValueCount, + }; + } + /** * Read the persisted version counter. Used by the field-defs cache * (Phase 3) to detect invalidation. Returns `0` when the row is diff --git a/packages/core/src/schema/types.ts b/packages/core/src/schema/types.ts index 5a74e0970..285835593 100644 --- a/packages/core/src/schema/types.ts +++ b/packages/core/src/schema/types.ts @@ -409,12 +409,27 @@ export interface UpdateBylineFieldInput { export type CustomFieldValue = string | boolean | null; /** - * Reserved byline-field slugs. These collide with fixed columns on - * `_emdash_bylines` (migrations 031 + 040); allowing a custom field with - * one of these slugs would shadow the column on hydration. Enforced at the - * registry layer (Phase 2) and the admin API zod layer (Phase 4). + * Reserved byline-field slugs. Two reasons a slug ends up here: + * + * 1. **Column collision.** Slugs that match a fixed column on + * `_emdash_bylines` (migrations 031 + 040) would shadow that column + * on hydration. The first 12 entries cover this. + * 2. **Route collision.** Static file routes under + * `/_emdash/api/admin/byline-fields/` take precedence over the + * `[slug].ts` dynamic route in Astro, so a custom field whose slug + * matches a sibling static file (e.g. `reorder.ts`) is unreachable + * via single-field CRUD — the static route handles only its own + * method (POST for `reorder`) and 405s everything else. + * `reorder` is the only such sibling today; new sibling routes + * (e.g. a hypothetical `import.ts`) must be added here. + * `[slug]/usage.ts` lives a level deeper so a slug of `usage` does + * not collide — it resolves cleanly to `[slug].ts`. + * + * Enforced at the registry layer (Phase 2) and the admin API zod layer + * (Phase 4) so non-HTTP callers (seeds, scripts) get the same guarantee. */ export const RESERVED_BYLINE_FIELD_SLUGS = [ + // 1. Column-collision slugs (matches `_emdash_bylines` fixed columns). "id", "slug", "display_name", @@ -427,4 +442,6 @@ export const RESERVED_BYLINE_FIELD_SLUGS = [ "translation_group", "created_at", "updated_at", + // 2. Route-collision slugs (matches static sibling files of `[slug].ts`). + "reorder", ] as const; From 1010934713ca24547dc0bbd287e0b9d18a340ec7 Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 21:59:33 +0100 Subject: [PATCH 10/35] feat(bylines): zod schemas for byline custom-field admin API --- .../core/src/api/schemas/byline-fields.ts | 188 ++++++++++++++++++ packages/core/src/api/schemas/bylines.ts | 22 ++ packages/core/src/api/schemas/index.ts | 1 + 3 files changed, 211 insertions(+) create mode 100644 packages/core/src/api/schemas/byline-fields.ts diff --git a/packages/core/src/api/schemas/byline-fields.ts b/packages/core/src/api/schemas/byline-fields.ts new file mode 100644 index 000000000..9e238e135 --- /dev/null +++ b/packages/core/src/api/schemas/byline-fields.ts @@ -0,0 +1,188 @@ +/** + * Zod schemas for the byline-fields admin API (Discussion #1174, Phase 4). + * + * Reserved-slug + identifier validation runs at the zod layer so the + * route returns a clean 400 (`VALIDATION_ERROR` from `parseBody`) rather + * than bubbling a registry-level `BylineSchemaError` ("RESERVED_SLUG" / + * "INVALID_SLUG"). The registry repeats the same checks for non-HTTP + * callers (seeds, scripts) — see `BylineSchemaRegistry.validateSlug`. + * + * Field types are constrained to the v1 subset declared in + * `BYLINE_FIELD_TYPES`. Adding a type to the union there will require a + * corresponding update to this enum. + */ + +import { z } from "zod"; + +import { BYLINE_FIELD_TYPES, RESERVED_BYLINE_FIELD_SLUGS } from "../../schema/types.js"; + +/** + * Slug pattern for byline field definitions — matches the identifier rule + * used by `validateIdentifier` (and `slugPattern` in `common.ts`). + * Lowercase letters, digits, and underscores; must start with a letter. + */ +const bylineFieldSlugPattern = /^[a-z][a-z0-9_]*$/; + +/** Hard cap on a slug — mirrors `BylineSchemaRegistry.MAX_SLUG_LENGTH`. */ +const MAX_SLUG_LENGTH = 63; +/** Hard cap on a label — mirrors `BylineSchemaRegistry.MAX_LABEL_LENGTH`. */ +const MAX_LABEL_LENGTH = 200; +/** Hard cap on a select field's `options` list. */ +const MAX_SELECT_OPTIONS = 200; + +const RESERVED_SET: ReadonlySet = new Set(RESERVED_BYLINE_FIELD_SLUGS); + +// Enumerate the v1 byline field types explicitly so zod gets the exact +// literal union for `z.infer<>`. Mirrors `BYLINE_FIELD_TYPES`; CI's +// type-checker catches drift via the satisfies/import below. +const bylineFieldTypeValues = z.enum(["string", "text", "url", "boolean", "select"]); +// Compile-time guard: a drift here trips the satisfies check. +type _BylineFieldTypeDriftCheck = + (typeof BYLINE_FIELD_TYPES)[number] extends z.infer + ? z.infer extends (typeof BYLINE_FIELD_TYPES)[number] + ? true + : never + : never; +const _bylineFieldTypeDriftCheck: _BylineFieldTypeDriftCheck = true; +void _bylineFieldTypeDriftCheck; + +/** + * Validation payload for a byline custom field. v1 only exposes + * `options` (used by `select`-type fields). Empty/duplicate options are + * rejected at the registry layer; the zod layer only enforces shape and + * caps. Future field types may add keys here. + */ +const bylineFieldValidationSchema = z + .object({ + options: z + .array(z.string().min(1)) + .min(1, "select options must contain at least one entry") + .max(MAX_SELECT_OPTIONS, `select options cannot exceed ${MAX_SELECT_OPTIONS} entries`) + .optional(), + }) + .strict() + .nullable(); + +/** + * Slug validation chain shared by create + reorder bodies. Centralised so + * the reserved-slug message and pattern are identical everywhere. + */ +const bylineFieldSlug = z + .string() + .min(1, "Byline field slug is required") + .max(MAX_SLUG_LENGTH, `Byline field slug must be ${MAX_SLUG_LENGTH} characters or less`) + .regex( + bylineFieldSlugPattern, + "Byline field slug must contain only lowercase letters, digits, and underscores, and start with a letter", + ) + .refine((slug) => !RESERVED_SET.has(slug), { + // Surface the offending slug in the validation issue path-message + // for easier debugging from the admin UI's error toast. + message: "Byline field slug is reserved", + }); + +const bylineFieldLabel = z + .string() + .min(1, "Byline field label is required") + .max(MAX_LABEL_LENGTH, `Byline field label must be ${MAX_LABEL_LENGTH} characters or less`); + +// --------------------------------------------------------------------------- +// Request bodies +// --------------------------------------------------------------------------- + +export const bylineFieldCreateBody = z + .object({ + slug: bylineFieldSlug, + label: bylineFieldLabel, + type: bylineFieldTypeValues, + required: z.boolean().optional(), + /** + * Whether values are stored per-locale (translatable, default) or + * shared across the translation group. See `BylineFieldDefinition`. + */ + translatable: z.boolean().optional(), + validation: bylineFieldValidationSchema.optional(), + sortOrder: z.number().int().min(0).optional(), + }) + .strict() + .meta({ id: "BylineFieldCreateBody" }); + +/** + * Update body. `slug` and `type` are intentionally absent — both are + * immutable post-create (changing them would invalidate stored values). + * `translatable` flips are gated at the registry layer when value rows + * exist (`TRANSLATABLE_LOCKED`). + */ +export const bylineFieldUpdateBody = z + .object({ + label: bylineFieldLabel.optional(), + required: z.boolean().optional(), + translatable: z.boolean().optional(), + validation: bylineFieldValidationSchema.optional(), + sortOrder: z.number().int().min(0).optional(), + }) + .strict() + .meta({ id: "BylineFieldUpdateBody" }); + +export const bylineFieldReorderBody = z + .object({ + /** + * Exact set of currently registered slugs in the desired order. + * The registry rejects any drift (`REORDER_MISMATCH`); the zod + * layer enforces slug shape only. An empty array is permitted — + * `reorderFields([])` is a valid no-op when zero fields are + * registered (registry contract). Rejecting empty here would + * produce a spurious 400 for an admin UI that submits a reorder + * after deleting the last field. + */ + slugs: z.array(bylineFieldSlug), + }) + .strict() + .meta({ id: "BylineFieldReorderBody" }); + +// --------------------------------------------------------------------------- +// Response shapes +// --------------------------------------------------------------------------- + +export const bylineFieldDefinitionSchema = z + .object({ + id: z.string(), + slug: z.string(), + label: z.string(), + type: bylineFieldTypeValues, + required: z.boolean(), + translatable: z.boolean(), + validation: z + .object({ + options: z.array(z.string()).optional(), + }) + .nullable(), + sortOrder: z.number().int(), + createdAt: z.string(), + updatedAt: z.string(), + }) + .meta({ id: "BylineFieldDefinition" }); + +export const bylineFieldListResponseSchema = z + .object({ + items: z.array(bylineFieldDefinitionSchema), + }) + .meta({ id: "BylineFieldListResponse" }); + +/** + * Response shape for `GET /api/admin/byline-fields/[slug]/usage`. + * + * `translatableValueCount` counts rows in `_emdash_byline_field_values`. + * `groupValueCount` counts rows in `_emdash_byline_field_group_values`. + * `totalAffectedRows` is the sum — what the destructive-delete confirm + * dialog surfaces. Both individual counts are exposed for diagnostic + * value (e.g. inconsistency with the field's current `translatable` + * flag would show non-zero on the "wrong" side). + */ +export const bylineFieldUsageResponseSchema = z + .object({ + translatableValueCount: z.number().int().nonnegative(), + groupValueCount: z.number().int().nonnegative(), + totalAffectedRows: z.number().int().nonnegative(), + }) + .meta({ id: "BylineFieldUsageResponse" }); diff --git a/packages/core/src/api/schemas/bylines.ts b/packages/core/src/api/schemas/bylines.ts index 5ea5240e6..3a9311165 100644 --- a/packages/core/src/api/schemas/bylines.ts +++ b/packages/core/src/api/schemas/bylines.ts @@ -25,6 +25,16 @@ export const bylineSummarySchema = z * source. Nullable in storage for backwards compatibility. */ translationGroup: z.string().nullable(), + /** + * Byline custom-field values (Discussion #1174). Keys are slugs + * registered via the byline-fields admin API; values follow + * `CustomFieldValue` (`string | boolean | null`). Always present + * on hydrated responses — empty `{}` when no fields are + * registered (Phase 3 AC #6). Marked optional in the schema for + * historic-payload compatibility with pre-Phase-3 clients that + * may not send the key on writes; hydration always populates it. + */ + customFields: z.record(z.string(), z.union([z.string(), z.boolean(), z.null()])).optional(), }) .meta({ id: "BylineSummary" }); @@ -120,6 +130,18 @@ export const bylineUpdateBody = z websiteUrl: httpUrl.nullish(), userId: z.string().nullish(), isGuest: z.boolean().optional(), + /** + * Byline custom-field values (Discussion #1174, Phase 3+4). Keys + * are field slugs; values are unknown at the API layer because + * the per-field type contract lives in the registry and would + * require an extra query to enforce here. The repository's + * `coerceFieldValue` validates against the field's type and + * throws `EmDashValidationError` on mismatch — the route maps + * that to a 400 `VALIDATION_ERROR`. Reserved-slug write attempts + * fall out as `EmDashValidationError("Unknown byline custom + * field …")` because no registered field claims a reserved slug. + */ + customFields: z.record(z.string(), z.unknown()).optional(), }) .meta({ id: "BylineUpdateBody" }); diff --git a/packages/core/src/api/schemas/index.ts b/packages/core/src/api/schemas/index.ts index 7f7265bf0..eefefbe8e 100644 --- a/packages/core/src/api/schemas/index.ts +++ b/packages/core/src/api/schemas/index.ts @@ -15,3 +15,4 @@ export * from "./users.js"; export * from "./widgets.js"; export * from "./redirects.js"; export * from "./bylines.js"; +export * from "./byline-fields.js"; From 41389c83a69843b2f12fd64862074f7fe7af50bc Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 21:59:42 +0100 Subject: [PATCH 11/35] feat(bylines): handler layer for byline-fields and update routes --- .../core/src/api/handlers/byline-fields.ts | 212 ++++++++++++++++++ packages/core/src/api/handlers/bylines.ts | 54 ++++- 2 files changed, 264 insertions(+), 2 deletions(-) create mode 100644 packages/core/src/api/handlers/byline-fields.ts diff --git a/packages/core/src/api/handlers/byline-fields.ts b/packages/core/src/api/handlers/byline-fields.ts new file mode 100644 index 000000000..25b1a03d1 --- /dev/null +++ b/packages/core/src/api/handlers/byline-fields.ts @@ -0,0 +1,212 @@ +/** + * Handler layer for the byline-fields admin API (Phase 4 of Discussion + * #1174). + * + * Each handler: + * - Takes the `Kysely` from the route, returns `ApiResult`. + * - Wraps the registry call in try/catch. + * - Translates `BylineSchemaError` → shared `ErrorCode` via + * `mapBylineSchemaError`. HTTP status comes from `mapErrorStatus` at + * the route's `unwrapResult` site — handlers don't know about + * statuses. + * - Catches everything else, logs server-side, returns a 500-class + * code without leaking `error.message`. + * + * Reserved-slug + identifier validation runs at the zod layer (see + * `schemas/byline-fields.ts`); the registry repeats it for defence in + * depth (non-HTTP callers). This module assumes inputs have already + * passed through whichever zod schema the route used. + */ + +import type { Kysely } from "kysely"; + +import type { Database } from "../../database/types.js"; +import { + BylineSchemaError, + BylineSchemaRegistry, + mapBylineSchemaError, +} from "../../schema/byline-registry.js"; +import type { + BylineFieldDefinition, + CreateBylineFieldInput, + UpdateBylineFieldInput, +} from "../../schema/types.js"; +import type { ApiResult } from "../types.js"; + +/** + * Build a structured failure envelope from a `BylineSchemaError`. + * Centralised so every handler emits the same shape. + */ +function bylineSchemaErrorResult(error: BylineSchemaError): ApiResult { + const mapped = mapBylineSchemaError(error); + return { + success: false, + error: { code: mapped.code, message: mapped.message, details: mapped.details }, + }; +} + +/** + * Build a 500-class failure envelope. Logs the underlying error + * server-side; the message returned to the client is the static + * fallback to avoid leaking internals. + */ +function internalErrorResult( + error: unknown, + code: string, + fallbackMessage: string, +): ApiResult { + console.error(`[${code}]`, error); + return { + success: false, + error: { code, message: fallbackMessage }, + }; +} + +// --------------------------------------------------------------------------- +// List +// --------------------------------------------------------------------------- + +export async function handleBylineFieldList( + db: Kysely, +): Promise> { + try { + const items = await new BylineSchemaRegistry(db).listFields(); + return { success: true, data: { items } }; + } catch (error) { + return internalErrorResult(error, "SCHEMA_FIELD_LIST_ERROR", "Failed to list byline fields"); + } +} + +// --------------------------------------------------------------------------- +// Create +// --------------------------------------------------------------------------- + +export async function handleBylineFieldCreate( + db: Kysely, + input: CreateBylineFieldInput, +): Promise> { + try { + const field = await new BylineSchemaRegistry(db).createField(input); + return { success: true, data: field }; + } catch (error) { + if (error instanceof BylineSchemaError) { + return bylineSchemaErrorResult(error); + } + return internalErrorResult(error, "SCHEMA_FIELD_CREATE_ERROR", "Failed to create byline field"); + } +} + +// --------------------------------------------------------------------------- +// Get one +// --------------------------------------------------------------------------- + +export async function handleBylineFieldGet( + db: Kysely, + slug: string, +): Promise> { + try { + const field = await new BylineSchemaRegistry(db).getField(slug); + if (!field) { + return { + success: false, + error: { code: "NOT_FOUND", message: "Byline field not found" }, + }; + } + return { success: true, data: field }; + } catch (error) { + return internalErrorResult(error, "SCHEMA_FIELD_GET_ERROR", "Failed to get byline field"); + } +} + +// --------------------------------------------------------------------------- +// Update +// --------------------------------------------------------------------------- + +export async function handleBylineFieldUpdate( + db: Kysely, + slug: string, + input: UpdateBylineFieldInput, +): Promise> { + try { + const field = await new BylineSchemaRegistry(db).updateField(slug, input); + return { success: true, data: field }; + } catch (error) { + if (error instanceof BylineSchemaError) { + return bylineSchemaErrorResult(error); + } + return internalErrorResult(error, "SCHEMA_FIELD_UPDATE_ERROR", "Failed to update byline field"); + } +} + +// --------------------------------------------------------------------------- +// Delete +// --------------------------------------------------------------------------- + +export async function handleBylineFieldDelete( + db: Kysely, + slug: string, +): Promise> { + try { + await new BylineSchemaRegistry(db).deleteField(slug); + return { success: true, data: { deleted: true } }; + } catch (error) { + if (error instanceof BylineSchemaError) { + return bylineSchemaErrorResult(error); + } + return internalErrorResult(error, "SCHEMA_FIELD_DELETE_ERROR", "Failed to delete byline field"); + } +} + +// --------------------------------------------------------------------------- +// Usage +// --------------------------------------------------------------------------- + +export async function handleBylineFieldUsage( + db: Kysely, + slug: string, +): Promise< + ApiResult<{ + translatableValueCount: number; + groupValueCount: number; + totalAffectedRows: number; + }> +> { + try { + const usage = await new BylineSchemaRegistry(db).getFieldUsage(slug); + return { success: true, data: usage }; + } catch (error) { + if (error instanceof BylineSchemaError) { + return bylineSchemaErrorResult(error); + } + return internalErrorResult( + error, + "SCHEMA_FIELD_GET_ERROR", + "Failed to read byline field usage", + ); + } +} + +// --------------------------------------------------------------------------- +// Reorder +// --------------------------------------------------------------------------- + +export async function handleBylineFieldReorder( + db: Kysely, + slugs: string[], +): Promise> { + try { + const registry = new BylineSchemaRegistry(db); + await registry.reorderFields(slugs); + const items = await registry.listFields(); + return { success: true, data: { items } }; + } catch (error) { + if (error instanceof BylineSchemaError) { + return bylineSchemaErrorResult(error); + } + return internalErrorResult( + error, + "SCHEMA_FIELD_REORDER_ERROR", + "Failed to reorder byline fields", + ); + } +} diff --git a/packages/core/src/api/handlers/bylines.ts b/packages/core/src/api/handlers/bylines.ts index 8371aa70d..92701165c 100644 --- a/packages/core/src/api/handlers/bylines.ts +++ b/packages/core/src/api/handlers/bylines.ts @@ -1,7 +1,11 @@ import type { Kysely } from "kysely"; -import { BylineRepository, type CreateBylineInput } from "../../database/repositories/byline.js"; -import type { BylineSummary } from "../../database/repositories/types.js"; +import { + BylineRepository, + type CreateBylineInput, + type UpdateBylineInput, +} from "../../database/repositories/byline.js"; +import { EmDashValidationError, type BylineSummary } from "../../database/repositories/types.js"; import type { Database } from "../../database/types.js"; import { getI18nConfig } from "../../i18n/config.js"; import type { ApiResult } from "../types.js"; @@ -159,3 +163,49 @@ export async function handleBylineCreate( }; } } + +/** + * Update an existing byline. Forwards every field on `UpdateBylineInput` + * to `BylineRepository.update`, including the Phase 3 `customFields` + * map; per-field type validation lives in the repo, which throws + * `EmDashValidationError` on unknown slugs, type mismatches, or + * `select`-choice misses. This handler translates that into a clean + * `VALIDATION_ERROR` (400 via `mapErrorStatus`). + * + * Returns `NOT_FOUND` when the byline id doesn't resolve. Generic + * failures surface as `BYLINE_UPDATE_ERROR` (500) without leaking the + * underlying message. + */ +export async function handleBylineUpdate( + db: Kysely, + id: string, + input: UpdateBylineInput, +): Promise> { + try { + const repo = new BylineRepository(db); + const byline = await repo.update(id, input); + if (!byline) { + return { + success: false, + error: { code: "NOT_FOUND", message: "Byline not found" }, + }; + } + return { success: true, data: byline }; + } catch (error) { + // Unknown-key + type-mismatch + select-choice writes throw + // EmDashValidationError (Phase 3, see BylineRepository.update). + // Map to a clean 400 — the error message names the offending + // slug/type, which is safe to surface to the admin client. + if (error instanceof EmDashValidationError) { + return { + success: false, + error: { code: "VALIDATION_ERROR", message: error.message }, + }; + } + console.error("[BYLINE_UPDATE_ERROR]", error); + return { + success: false, + error: { code: "BYLINE_UPDATE_ERROR", message: "Failed to update byline" }, + }; + } +} From 01c6210ef9162825aaea4d7fe4c6f7c528afe9c4 Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 22:00:02 +0100 Subject: [PATCH 12/35] feat(bylines): admin API routes for byline custom fields --- packages/core/src/astro/integration/routes.ts | 27 ++++++ .../routes/api/admin/byline-fields/[slug].ts | 90 +++++++++++++++++++ .../api/admin/byline-fields/[slug]/usage.ts | 34 +++++++ .../routes/api/admin/byline-fields/index.ts | 60 +++++++++++++ .../routes/api/admin/byline-fields/reorder.ts | 39 ++++++++ .../routes/api/admin/bylines/[id]/index.ts | 44 ++++----- 6 files changed, 273 insertions(+), 21 deletions(-) create mode 100644 packages/core/src/astro/routes/api/admin/byline-fields/[slug].ts create mode 100644 packages/core/src/astro/routes/api/admin/byline-fields/[slug]/usage.ts create mode 100644 packages/core/src/astro/routes/api/admin/byline-fields/index.ts create mode 100644 packages/core/src/astro/routes/api/admin/byline-fields/reorder.ts diff --git a/packages/core/src/astro/integration/routes.ts b/packages/core/src/astro/integration/routes.ts index 7fda40712..b3e769f60 100644 --- a/packages/core/src/astro/integration/routes.ts +++ b/packages/core/src/astro/integration/routes.ts @@ -452,6 +452,33 @@ export function injectCoreRoutes(injectRoute: InjectRoute): void { entrypoint: resolveRoute("api/admin/bylines/[id]/translations.ts"), }); + // Byline custom-field schema routes (Discussion #1174, Phase 4). + // Order matters: the static `reorder` route must precede the dynamic + // `[slug]` route so Astro's resolver dispatches POST /byline-fields/reorder + // to the reorder handler instead of treating "reorder" as a slug. The + // `reorder` slug is also reserved at the data layer + // (RESERVED_BYLINE_FIELD_SLUGS) so the registry rejects field creation + // with that name — defence in depth. + injectRoute({ + pattern: "/_emdash/api/admin/byline-fields", + entrypoint: resolveRoute("api/admin/byline-fields/index.ts"), + }); + + injectRoute({ + pattern: "/_emdash/api/admin/byline-fields/reorder", + entrypoint: resolveRoute("api/admin/byline-fields/reorder.ts"), + }); + + injectRoute({ + pattern: "/_emdash/api/admin/byline-fields/[slug]", + entrypoint: resolveRoute("api/admin/byline-fields/[slug].ts"), + }); + + injectRoute({ + pattern: "/_emdash/api/admin/byline-fields/[slug]/usage", + entrypoint: resolveRoute("api/admin/byline-fields/[slug]/usage.ts"), + }); + injectRoute({ pattern: "/_emdash/api/admin/users/[id]", entrypoint: resolveRoute("api/admin/users/[id]/index.ts"), diff --git a/packages/core/src/astro/routes/api/admin/byline-fields/[slug].ts b/packages/core/src/astro/routes/api/admin/byline-fields/[slug].ts new file mode 100644 index 000000000..491693d47 --- /dev/null +++ b/packages/core/src/astro/routes/api/admin/byline-fields/[slug].ts @@ -0,0 +1,90 @@ +/** + * Byline custom-field schema management — single field CRUD. + * + * - GET /_emdash/api/admin/byline-fields/{slug} read one definition + * - PATCH /_emdash/api/admin/byline-fields/{slug} update label / required / + * translatable / validation / + * sort order + * - DELETE /_emdash/api/admin/byline-fields/{slug} drop the definition and all + * stored values (FK CASCADE + + * app-level cleanup; see + * `BylineSchemaRegistry.deleteField`) + * + * Thin wrappers around the handler layer; status mapping happens in + * `unwrapResult`. `slug` and `type` are immutable post-create — see + * `bylineFieldUpdateBody`. Flipping `translatable` while value rows + * exist surfaces as a 409 `TRANSLATABLE_LOCKED`. + * + * Phase 4 of Discussion #1174. + */ + +import type { APIRoute } from "astro"; + +import { requirePerm } from "#api/authorize.js"; +import { apiError, requireDb, unwrapResult } from "#api/error.js"; +import { + handleBylineFieldDelete, + handleBylineFieldGet, + handleBylineFieldUpdate, +} from "#api/handlers/byline-fields.js"; +import { isParseError, parseBody } from "#api/parse.js"; +import { bylineFieldUpdateBody } from "#api/schemas.js"; + +export const prerender = false; + +export const GET: APIRoute = async ({ params, locals }) => { + const { emdash, user } = locals; + const denied = requirePerm(user, "schema:manage"); + if (denied) return denied; + + const dbErr = requireDb(emdash?.db); + if (dbErr) return dbErr; + + const slug = params.slug; + if (!slug) return apiError("MISSING_PARAM", "Field slug is required", 400); + + const result = await handleBylineFieldGet(emdash.db, slug); + return unwrapResult(result); +}; + +export const PATCH: APIRoute = async ({ params, request, locals }) => { + const { emdash, user } = locals; + const denied = requirePerm(user, "schema:manage"); + if (denied) return denied; + + const dbErr = requireDb(emdash?.db); + if (dbErr) return dbErr; + + const slug = params.slug; + if (!slug) return apiError("MISSING_PARAM", "Field slug is required", 400); + + const body = await parseBody(request, bylineFieldUpdateBody); + if (isParseError(body)) return body; + + const result = await handleBylineFieldUpdate(emdash.db, slug, { + label: body.label, + required: body.required, + translatable: body.translatable, + // `null` clears the stored validation; `undefined` leaves it as-is. + // The zod schema makes `validation` itself optional, so an absent + // key reaches the handler as `undefined`. + validation: body.validation, + sortOrder: body.sortOrder, + }); + return unwrapResult(result); +}; + +export const DELETE: APIRoute = async ({ params, locals }) => { + const { emdash, user } = locals; + const denied = requirePerm(user, "schema:manage"); + if (denied) return denied; + + const dbErr = requireDb(emdash?.db); + if (dbErr) return dbErr; + + const slug = params.slug; + if (!slug) return apiError("MISSING_PARAM", "Field slug is required", 400); + + const result = await handleBylineFieldDelete(emdash.db, slug); + return unwrapResult(result); +}; diff --git a/packages/core/src/astro/routes/api/admin/byline-fields/[slug]/usage.ts b/packages/core/src/astro/routes/api/admin/byline-fields/[slug]/usage.ts new file mode 100644 index 000000000..49f659fb4 --- /dev/null +++ b/packages/core/src/astro/routes/api/admin/byline-fields/[slug]/usage.ts @@ -0,0 +1,34 @@ +/** + * Byline field usage counts. + * + * GET /_emdash/api/admin/byline-fields/{slug}/usage + * + * Returns `{ translatableValueCount, groupValueCount, totalAffectedRows }`. + * Backs the destructive-delete confirm dialog in the admin UI (Phase 5). + * Thin wrapper around `handleBylineFieldUsage`. + * + * Phase 4 of Discussion #1174. + */ + +import type { APIRoute } from "astro"; + +import { requirePerm } from "#api/authorize.js"; +import { apiError, requireDb, unwrapResult } from "#api/error.js"; +import { handleBylineFieldUsage } from "#api/handlers/byline-fields.js"; + +export const prerender = false; + +export const GET: APIRoute = async ({ params, locals }) => { + const { emdash, user } = locals; + const denied = requirePerm(user, "schema:manage"); + if (denied) return denied; + + const dbErr = requireDb(emdash?.db); + if (dbErr) return dbErr; + + const slug = params.slug; + if (!slug) return apiError("MISSING_PARAM", "Field slug is required", 400); + + const result = await handleBylineFieldUsage(emdash.db, slug); + return unwrapResult(result); +}; diff --git a/packages/core/src/astro/routes/api/admin/byline-fields/index.ts b/packages/core/src/astro/routes/api/admin/byline-fields/index.ts new file mode 100644 index 000000000..0b3241da8 --- /dev/null +++ b/packages/core/src/astro/routes/api/admin/byline-fields/index.ts @@ -0,0 +1,60 @@ +/** + * Byline custom-field schema management — list + create. + * + * - GET /_emdash/api/admin/byline-fields list every registered field + * - POST /_emdash/api/admin/byline-fields create a new field definition + * + * Thin wrappers around `handleBylineFieldList` / `handleBylineFieldCreate` + * in `api/handlers/byline-fields.ts`. Both endpoints require + * `schema:manage`. Reserved-slug + identifier validation runs at the + * zod layer in `bylineFieldCreateBody`; the registry repeats the check + * for defence-in-depth. Domain errors surface as typed `ErrorCode`s and + * are mapped to HTTP statuses by `unwrapResult` → `mapErrorStatus`. + * + * Phase 4 of Discussion #1174. + */ + +import type { APIRoute } from "astro"; + +import { requirePerm } from "#api/authorize.js"; +import { requireDb, unwrapResult } from "#api/error.js"; +import { handleBylineFieldCreate, handleBylineFieldList } from "#api/handlers/byline-fields.js"; +import { isParseError, parseBody } from "#api/parse.js"; +import { bylineFieldCreateBody } from "#api/schemas.js"; + +export const prerender = false; + +export const GET: APIRoute = async ({ locals }) => { + const { emdash, user } = locals; + const denied = requirePerm(user, "schema:manage"); + if (denied) return denied; + + const dbErr = requireDb(emdash?.db); + if (dbErr) return dbErr; + + const result = await handleBylineFieldList(emdash.db); + return unwrapResult(result); +}; + +export const POST: APIRoute = async ({ request, locals }) => { + const { emdash, user } = locals; + const denied = requirePerm(user, "schema:manage"); + if (denied) return denied; + + const dbErr = requireDb(emdash?.db); + if (dbErr) return dbErr; + + const body = await parseBody(request, bylineFieldCreateBody); + if (isParseError(body)) return body; + + const result = await handleBylineFieldCreate(emdash.db, { + slug: body.slug, + label: body.label, + type: body.type, + required: body.required, + translatable: body.translatable, + validation: body.validation ?? null, + sortOrder: body.sortOrder, + }); + return unwrapResult(result, 201); +}; diff --git a/packages/core/src/astro/routes/api/admin/byline-fields/reorder.ts b/packages/core/src/astro/routes/api/admin/byline-fields/reorder.ts new file mode 100644 index 000000000..d52fe6f10 --- /dev/null +++ b/packages/core/src/astro/routes/api/admin/byline-fields/reorder.ts @@ -0,0 +1,39 @@ +/** + * Byline field reorder. + * + * POST /_emdash/api/admin/byline-fields/reorder + * + * Body: `{ slugs: string[] }` — the exact set of currently registered + * slugs in the desired order. The registry rejects any drift + * (`REORDER_MISMATCH` → 400). Empty `[]` against an empty registered + * set is a no-op by registry contract; the zod schema permits it. + * + * Thin wrapper around `handleBylineFieldReorder`. + * + * Phase 4 of Discussion #1174. + */ + +import type { APIRoute } from "astro"; + +import { requirePerm } from "#api/authorize.js"; +import { requireDb, unwrapResult } from "#api/error.js"; +import { handleBylineFieldReorder } from "#api/handlers/byline-fields.js"; +import { isParseError, parseBody } from "#api/parse.js"; +import { bylineFieldReorderBody } from "#api/schemas.js"; + +export const prerender = false; + +export const POST: APIRoute = async ({ request, locals }) => { + const { emdash, user } = locals; + const denied = requirePerm(user, "schema:manage"); + if (denied) return denied; + + const dbErr = requireDb(emdash?.db); + if (dbErr) return dbErr; + + const body = await parseBody(request, bylineFieldReorderBody); + if (isParseError(body)) return body; + + const result = await handleBylineFieldReorder(emdash.db, body.slugs); + return unwrapResult(result); +}; diff --git a/packages/core/src/astro/routes/api/admin/bylines/[id]/index.ts b/packages/core/src/astro/routes/api/admin/bylines/[id]/index.ts index 2a33047ab..acbed6648 100644 --- a/packages/core/src/astro/routes/api/admin/bylines/[id]/index.ts +++ b/packages/core/src/astro/routes/api/admin/bylines/[id]/index.ts @@ -1,7 +1,8 @@ import type { APIRoute } from "astro"; import { requirePerm } from "#api/authorize.js"; -import { apiError, apiSuccess, handleError } from "#api/error.js"; +import { apiError, apiSuccess, handleError, requireDb, unwrapResult } from "#api/error.js"; +import { handleBylineUpdate } from "#api/handlers/bylines.js"; import { isParseError, parseBody } from "#api/parse.js"; import { bylineUpdateBody } from "#api/schemas.js"; import { invalidateBylineCache } from "#bylines/index.js"; @@ -33,31 +34,32 @@ export const PUT: APIRoute = async ({ params, request, locals }) => { const denied = requirePerm(user, "bylines:manage"); if (denied) return denied; - if (!emdash?.db) { - return apiError("NOT_CONFIGURED", "EmDash is not initialized", 500); - } + const dbErr = requireDb(emdash?.db); + if (dbErr) return dbErr; const body = await parseBody(request, bylineUpdateBody); if (isParseError(body)) return body; - try { - const repo = new BylineRepository(emdash.db); - const byline = await repo.update(params.id!, { - slug: body.slug, - displayName: body.displayName, - bio: body.bio ?? null, - avatarMediaId: body.avatarMediaId ?? null, - websiteUrl: body.websiteUrl ?? null, - userId: body.userId ?? null, - isGuest: body.isGuest, - }); + const result = await handleBylineUpdate(emdash.db, params.id!, { + slug: body.slug, + displayName: body.displayName, + bio: body.bio ?? null, + avatarMediaId: body.avatarMediaId ?? null, + websiteUrl: body.websiteUrl ?? null, + userId: body.userId ?? null, + isGuest: body.isGuest, + // Forward `customFields` only when present so the repo treats an + // omitted key as "leave existing values untouched". An empty + // object also no-ops by repo convention — see + // `BylineRepository.update`. Validation (unknown slug, type + // mismatch, select-choice) happens inside the repo and surfaces + // as `EmDashValidationError`, which the handler maps to a 400 + // `VALIDATION_ERROR` for `unwrapResult` / `mapErrorStatus`. + customFields: body.customFields, + }); - if (!byline) return apiError("NOT_FOUND", "Byline not found", 404); - invalidateBylineCache(); - return apiSuccess(byline); - } catch (error) { - return handleError(error, "Failed to update byline", "BYLINE_UPDATE_ERROR"); - } + if (result.success) invalidateBylineCache(); + return unwrapResult(result); }; export const DELETE: APIRoute = async ({ params, locals }) => { From 0ad8ef2c7da8d8f3bba87730e748b00b587429fc Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 22:00:11 +0100 Subject: [PATCH 13/35] test(bylines): admin API coverage for byline custom fields --- .../api/byline-fields-auth.test.ts | 449 ++++++++++++++++++ .../api/bylines-customfields-write.test.ts | 349 ++++++++++++++ .../byline-fields-route-registration.test.ts | 68 +++ 3 files changed, 866 insertions(+) create mode 100644 packages/core/tests/integration/api/byline-fields-auth.test.ts create mode 100644 packages/core/tests/integration/api/bylines-customfields-write.test.ts create mode 100644 packages/core/tests/unit/api/byline-fields-route-registration.test.ts diff --git a/packages/core/tests/integration/api/byline-fields-auth.test.ts b/packages/core/tests/integration/api/byline-fields-auth.test.ts new file mode 100644 index 000000000..5cf00f834 --- /dev/null +++ b/packages/core/tests/integration/api/byline-fields-auth.test.ts @@ -0,0 +1,449 @@ +/** + * Auth, permission, and request-validation tests for the byline-fields + * admin API routes (Phase 4 of Discussion #1174). + * + * Covers acceptance criteria from `extensible-bylines-pr.mdx` §Phase 4: + * + * - Every byline-fields endpoint returns `UNAUTHORIZED` without auth. + * - Every byline-fields endpoint returns 403 for a user without + * `schema:manage` (Editor isn't enough — it's an Admin-only perm). + * - Reserved slugs rejected at the zod layer (400 VALIDATION_ERROR). + * - Invalid slugs / types rejected at the zod layer (400). + * - Happy paths round-trip through the registry. + * - `BylineSchemaError` codes map to the documented HTTP statuses + * (`FIELD_EXISTS` → 409, `FIELD_NOT_FOUND` → 404, + * `TRANSLATABLE_LOCKED` → 409, `REORDER_MISMATCH` → 400). + * + * CSRF is enforced by the auth middleware, not the route handler — see + * `astro/middleware/auth.ts:284-294`. These tests invoke the route + * exports directly, so the middleware does not run; the AC is satisfied + * by the middleware's blanket `X-EmDash-Request` check on every + * unsafe-method API request. + */ + +import { Role } from "@emdash-cms/auth"; +import type { APIContext } from "astro"; +import type { Kysely } from "kysely"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { + GET as getOne, + PATCH as patchOne, + DELETE as deleteOne, +} from "../../../src/astro/routes/api/admin/byline-fields/[slug].js"; +import { GET as getUsage } from "../../../src/astro/routes/api/admin/byline-fields/[slug]/usage.js"; +import { + GET as listFields, + POST as createField, +} from "../../../src/astro/routes/api/admin/byline-fields/index.js"; +import { POST as reorderFields } from "../../../src/astro/routes/api/admin/byline-fields/reorder.js"; +import { resetBylineFieldDefsCacheForTests } from "../../../src/bylines/field-defs-cache.js"; +import type { Database } from "../../../src/database/types.js"; +import { BylineSchemaRegistry } from "../../../src/schema/byline-registry.js"; +import { setupTestDatabase, teardownTestDatabase } from "../../utils/test-db.js"; + +// --------------------------------------------------------------------------- +// Context builder +// --------------------------------------------------------------------------- + +interface BuildOpts { + db: Kysely; + request: Request; + params?: Record; + user: { id: string; role: (typeof Role)[keyof typeof Role] } | null; +} + +function buildContext(opts: BuildOpts): APIContext { + return { + params: opts.params ?? {}, + url: new URL(opts.request.url), + request: opts.request, + locals: { + emdash: { db: opts.db, config: {} }, + user: opts.user, + }, + // eslint-disable-next-line typescript/no-unsafe-type-assertion -- minimal stub for tests + } as unknown as APIContext; +} + +function jsonRequest(url: string, method: string, body?: unknown): Request { + return new Request(url, { + method, + headers: { + "Content-Type": "application/json", + "X-EmDash-Request": "1", + }, + body: body === undefined ? undefined : JSON.stringify(body), + }); +} + +const adminUser = { id: "admin-1", role: Role.ADMIN }; +const editorUser = { id: "editor-1", role: Role.EDITOR }; + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe("admin/byline-fields routes — auth + permissions + validation", () => { + let db: Kysely; + + beforeEach(async () => { + // See sibling test `bylines-customfields-write.test.ts` for why. + resetBylineFieldDefsCacheForTests(); + db = await setupTestDatabase(); + }); + + afterEach(async () => { + await teardownTestDatabase(db); + }); + + // =========================================== + // Auth gate (no user → 401) + // =========================================== + + describe("UNAUTHORIZED without a session", () => { + it("GET /byline-fields returns 401", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "GET"); + const res = await listFields(buildContext({ db, request: req, user: null })); + expect(res.status).toBe(401); + expect(await res.json()).toMatchObject({ error: { code: "UNAUTHORIZED" } }); + }); + + it("POST /byline-fields returns 401", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "POST", { + slug: "x", + label: "X", + type: "string", + }); + const res = await createField(buildContext({ db, request: req, user: null })); + expect(res.status).toBe(401); + }); + + it("GET /byline-fields/{slug} returns 401", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/x", "GET"); + const res = await getOne( + buildContext({ db, request: req, params: { slug: "x" }, user: null }), + ); + expect(res.status).toBe(401); + }); + + it("PATCH /byline-fields/{slug} returns 401", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/x", "PATCH", { + label: "X", + }); + const res = await patchOne( + buildContext({ db, request: req, params: { slug: "x" }, user: null }), + ); + expect(res.status).toBe(401); + }); + + it("DELETE /byline-fields/{slug} returns 401", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/x", "DELETE"); + const res = await deleteOne( + buildContext({ db, request: req, params: { slug: "x" }, user: null }), + ); + expect(res.status).toBe(401); + }); + + it("GET /byline-fields/{slug}/usage returns 401", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/x/usage", "GET"); + const res = await getUsage( + buildContext({ db, request: req, params: { slug: "x" }, user: null }), + ); + expect(res.status).toBe(401); + }); + + it("POST /byline-fields/reorder returns 401", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/reorder", "POST", { + slugs: ["x"], + }); + const res = await reorderFields(buildContext({ db, request: req, user: null })); + expect(res.status).toBe(401); + }); + }); + + // =========================================== + // Permission gate (editor → 403, admin → ok) + // =========================================== + + describe("FORBIDDEN without schema:manage", () => { + it("GET /byline-fields returns 403 for Editor", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "GET"); + const res = await listFields(buildContext({ db, request: req, user: editorUser })); + expect(res.status).toBe(403); + expect(await res.json()).toMatchObject({ error: { code: "FORBIDDEN" } }); + }); + + it("POST /byline-fields returns 403 for Editor", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "POST", { + slug: "job_title", + label: "Job Title", + type: "string", + }); + const res = await createField(buildContext({ db, request: req, user: editorUser })); + expect(res.status).toBe(403); + }); + + it("DELETE /byline-fields/{slug} returns 403 for Editor", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/x", "DELETE"); + const res = await deleteOne( + buildContext({ db, request: req, params: { slug: "x" }, user: editorUser }), + ); + expect(res.status).toBe(403); + }); + + it("POST /byline-fields/reorder returns 403 for Editor", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/reorder", "POST", { + slugs: ["x"], + }); + const res = await reorderFields(buildContext({ db, request: req, user: editorUser })); + expect(res.status).toBe(403); + }); + }); + + // =========================================== + // Happy paths — Admin can manage fields + // =========================================== + + describe("happy paths with schema:manage", () => { + it("creates and lists a field", async () => { + const createReq = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "POST", { + slug: "job_title", + label: "Job title", + type: "string", + }); + const createRes = await createField( + buildContext({ db, request: createReq, user: adminUser }), + ); + expect(createRes.status).toBe(201); + const created = (await createRes.json()) as { data: { slug: string; type: string } }; + expect(created.data.slug).toBe("job_title"); + expect(created.data.type).toBe("string"); + + const listReq = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "GET"); + const listRes = await listFields(buildContext({ db, request: listReq, user: adminUser })); + expect(listRes.status).toBe(200); + const list = (await listRes.json()) as { data: { items: { slug: string }[] } }; + expect(list.data.items).toHaveLength(1); + expect(list.data.items[0]?.slug).toBe("job_title"); + }); + + it("returns 404 when GET targets an unknown slug", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/missing", "GET"); + const res = await getOne( + buildContext({ db, request: req, params: { slug: "missing" }, user: adminUser }), + ); + expect(res.status).toBe(404); + expect(await res.json()).toMatchObject({ error: { code: "NOT_FOUND" } }); + }); + + it("PATCH updates a field's label", async () => { + const registry = new BylineSchemaRegistry(db); + await registry.createField({ slug: "pronouns", label: "Pronouns", type: "string" }); + + const req = jsonRequest( + "http://localhost/_emdash/api/admin/byline-fields/pronouns", + "PATCH", + { label: "Preferred pronouns" }, + ); + const res = await patchOne( + buildContext({ db, request: req, params: { slug: "pronouns" }, user: adminUser }), + ); + expect(res.status).toBe(200); + const updated = (await res.json()) as { data: { label: string } }; + expect(updated.data.label).toBe("Preferred pronouns"); + }); + + it("DELETE removes a field", async () => { + const registry = new BylineSchemaRegistry(db); + await registry.createField({ slug: "twitter", label: "Twitter", type: "url" }); + + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/twitter", "DELETE"); + const res = await deleteOne( + buildContext({ db, request: req, params: { slug: "twitter" }, user: adminUser }), + ); + expect(res.status).toBe(200); + expect(await registry.getField("twitter")).toBeNull(); + }); + + it("usage returns zero counts for a fresh field", async () => { + const registry = new BylineSchemaRegistry(db); + await registry.createField({ slug: "company", label: "Company", type: "string" }); + + const req = jsonRequest( + "http://localhost/_emdash/api/admin/byline-fields/company/usage", + "GET", + ); + const res = await getUsage( + buildContext({ db, request: req, params: { slug: "company" }, user: adminUser }), + ); + expect(res.status).toBe(200); + expect(await res.json()).toMatchObject({ + data: { + translatableValueCount: 0, + groupValueCount: 0, + totalAffectedRows: 0, + }, + }); + }); + + it("reorder swaps two registered fields", async () => { + const registry = new BylineSchemaRegistry(db); + await registry.createField({ slug: "a_field", label: "A", type: "string" }); + await registry.createField({ slug: "b_field", label: "B", type: "string" }); + + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/reorder", "POST", { + slugs: ["b_field", "a_field"], + }); + const res = await reorderFields(buildContext({ db, request: req, user: adminUser })); + expect(res.status).toBe(200); + const body = (await res.json()) as { data: { items: { slug: string; sortOrder: number }[] } }; + expect(body.data.items.map((i) => i.slug)).toEqual(["b_field", "a_field"]); + }); + }); + + // =========================================== + // Zod-layer rejections (400 VALIDATION_ERROR) + // =========================================== + + describe("zod-layer rejections", () => { + it("rejects a reserved slug at the zod layer", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "POST", { + slug: "display_name", // reserved (column collision) + label: "Display name", + type: "string", + }); + const res = await createField(buildContext({ db, request: req, user: adminUser })); + expect(res.status).toBe(400); + expect(await res.json()).toMatchObject({ + error: { code: "VALIDATION_ERROR" }, + }); + }); + + it("rejects `reorder` at the zod layer — would collide with the static reorder route", async () => { + // Astro's static-route precedence: `/byline-fields/reorder` resolves + // to `reorder.ts` (POST-only), so a field with slug `reorder` is + // unreachable via single-field CRUD on `[slug].ts`. Reserve at the + // system level — see `RESERVED_BYLINE_FIELD_SLUGS` JSDoc rationale. + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "POST", { + slug: "reorder", + label: "Reorder", + type: "string", + }); + const res = await createField(buildContext({ db, request: req, user: adminUser })); + expect(res.status).toBe(400); + expect(await res.json()).toMatchObject({ + error: { code: "VALIDATION_ERROR" }, + }); + }); + + it("rejects `reorder` at the registry layer (non-HTTP callers)", async () => { + const registry = new BylineSchemaRegistry(db); + await expect( + registry.createField({ slug: "reorder", label: "Reorder", type: "string" }), + ).rejects.toMatchObject({ code: "RESERVED_SLUG" }); + }); + + it("rejects an invalid slug pattern at the zod layer", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "POST", { + slug: "Bad-Slug", // hyphen + uppercase — pattern is /^[a-z][a-z0-9_]*$/ + label: "X", + type: "string", + }); + const res = await createField(buildContext({ db, request: req, user: adminUser })); + expect(res.status).toBe(400); + expect(await res.json()).toMatchObject({ + error: { code: "VALIDATION_ERROR" }, + }); + }); + + it("rejects an unsupported type at the zod layer", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "POST", { + slug: "x", + label: "X", + type: "portableText", // not in v1 byline subset + }); + const res = await createField(buildContext({ db, request: req, user: adminUser })); + expect(res.status).toBe(400); + }); + + it("rejects unknown top-level keys (strict mode)", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "POST", { + slug: "x", + label: "X", + type: "string", + bogus: true, + }); + const res = await createField(buildContext({ db, request: req, user: adminUser })); + expect(res.status).toBe(400); + }); + + it("accepts an empty reorder list when zero fields are registered (no-op)", async () => { + // Registry contract: `reorderFields([])` against an empty set + // is a valid no-op (length match passes; loop runs 0 times). + // The zod schema must agree to avoid producing a spurious 400 + // for the UI's "reorder after deleting the last field" flow. + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/reorder", "POST", { + slugs: [], + }); + const res = await reorderFields(buildContext({ db, request: req, user: adminUser })); + expect(res.status).toBe(200); + const body = (await res.json()) as { data: { items: unknown[] } }; + expect(body.data.items).toEqual([]); + }); + }); + + // =========================================== + // Registry-error mapping + // =========================================== + + describe("registry-error → HTTP mapping", () => { + it("FIELD_EXISTS → 409 on duplicate create", async () => { + const registry = new BylineSchemaRegistry(db); + await registry.createField({ slug: "job_title", label: "Job title", type: "string" }); + + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields", "POST", { + slug: "job_title", + label: "Job title (dup)", + type: "string", + }); + const res = await createField(buildContext({ db, request: req, user: adminUser })); + expect(res.status).toBe(409); + expect(await res.json()).toMatchObject({ error: { code: "FIELD_EXISTS" } }); + }); + + it("FIELD_NOT_FOUND → 404 on patch of missing slug", async () => { + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/missing", "PATCH", { + label: "Label", + }); + const res = await patchOne( + buildContext({ db, request: req, params: { slug: "missing" }, user: adminUser }), + ); + expect(res.status).toBe(404); + expect(await res.json()).toMatchObject({ error: { code: "NOT_FOUND" } }); + }); + + it("REORDER_MISMATCH → 400 when slug set differs", async () => { + const registry = new BylineSchemaRegistry(db); + await registry.createField({ slug: "a_field", label: "A", type: "string" }); + + const req = jsonRequest("http://localhost/_emdash/api/admin/byline-fields/reorder", "POST", { + slugs: ["a_field", "nonexistent_field"], + }); + const res = await reorderFields(buildContext({ db, request: req, user: adminUser })); + expect(res.status).toBe(400); + expect(await res.json()).toMatchObject({ error: { code: "REORDER_MISMATCH" } }); + }); + + it("usage endpoint returns 404 for missing slug", async () => { + const req = jsonRequest( + "http://localhost/_emdash/api/admin/byline-fields/missing/usage", + "GET", + ); + const res = await getUsage( + buildContext({ db, request: req, params: { slug: "missing" }, user: adminUser }), + ); + expect(res.status).toBe(404); + }); + }); +}); diff --git a/packages/core/tests/integration/api/bylines-customfields-write.test.ts b/packages/core/tests/integration/api/bylines-customfields-write.test.ts new file mode 100644 index 000000000..e878efb35 --- /dev/null +++ b/packages/core/tests/integration/api/bylines-customfields-write.test.ts @@ -0,0 +1,349 @@ +/** + * customFields write surface on the admin bylines PUT route. + * + * Phase 4 of Discussion #1174 extends `PUT /_emdash/api/admin/bylines/{id}` + * to accept a `customFields` map. Per-field type validation lives in + * `BylineRepository.update` (Phase 3); the route is responsible for + * forwarding the field and mapping `EmDashValidationError` to a clean + * 400 `VALIDATION_ERROR`. + * + * Covers: + * - Round-trip: PUT writes, GET reads them back. + * - Unknown slug → 400 `VALIDATION_ERROR` (rejected by repository, not + * leaked as a 500). + * - Type mismatch → 400 `VALIDATION_ERROR`. + * - Select-choice mismatch → 400 `VALIDATION_ERROR`. + * - PUT route still requires `bylines:manage` — Author can't write + * custom fields on someone else's (or any) byline; missing user is + * 401. + * - Reserved-slug writes return 400 (unknown-key path; no registered + * field claims a reserved slug because the registry rejects them). + */ + +import { Role } from "@emdash-cms/auth"; +import type { APIContext } from "astro"; +import type { Kysely } from "kysely"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { + GET as getById, + PUT as putById, +} from "../../../src/astro/routes/api/admin/bylines/[id]/index.js"; +import { resetBylineFieldDefsCacheForTests } from "../../../src/bylines/field-defs-cache.js"; +import { BylineRepository } from "../../../src/database/repositories/byline.js"; +import type { Database } from "../../../src/database/types.js"; +import { BylineSchemaRegistry } from "../../../src/schema/byline-registry.js"; +import { setupTestDatabase, teardownTestDatabase } from "../../utils/test-db.js"; + +// --------------------------------------------------------------------------- +// Helpers +// --------------------------------------------------------------------------- + +interface BuildOpts { + db: Kysely; + request: Request; + params: { id: string }; + user: { id: string; role: (typeof Role)[keyof typeof Role] } | null; +} + +function buildContext(opts: BuildOpts): APIContext { + return { + params: opts.params, + url: new URL(opts.request.url), + request: opts.request, + locals: { + emdash: { db: opts.db, config: {} }, + user: opts.user, + }, + // eslint-disable-next-line typescript/no-unsafe-type-assertion -- minimal stub for tests + } as unknown as APIContext; +} + +function putReq(id: string, body: unknown): Request { + return new Request(`http://localhost/_emdash/api/admin/bylines/${id}`, { + method: "PUT", + headers: { + "Content-Type": "application/json", + "X-EmDash-Request": "1", + }, + body: JSON.stringify(body), + }); +} + +function getReq(id: string): Request { + return new Request(`http://localhost/_emdash/api/admin/bylines/${id}`, { + method: "GET", + headers: { "X-EmDash-Request": "1" }, + }); +} + +const adminUser = { id: "admin-1", role: Role.ADMIN }; +const editorUser = { id: "editor-1", role: Role.EDITOR }; + +const basePut = { + slug: "jane-doe", + displayName: "Jane Doe", + bio: null, + avatarMediaId: null, + websiteUrl: null, + userId: null, + isGuest: true, +}; + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe("PUT /admin/bylines/{id} — customFields write surface", () => { + let db: Kysely; + let bylineId: string; + + beforeEach(async () => { + // The field-defs cache lives on globalThis (see + // `field-defs-cache.ts`); across tests in the same Vitest worker + // it would otherwise hold stale field IDs from a previous test's + // in-memory DB, producing spurious FK failures when the + // repository writes a value row keyed by a vanished field id. + // The version-counter coincidentally lands at the same value + // (4 fields × bump-twice = 8) in each test, so the version-based + // invalidation doesn't fire on its own. + resetBylineFieldDefsCacheForTests(); + db = await setupTestDatabase(); + + // Register two custom fields: one translatable (job_title), one + // group-shared (twitter_handle). Different types so we cover the + // type-validation paths. + const registry = new BylineSchemaRegistry(db); + await registry.createField({ + slug: "job_title", + label: "Job title", + type: "string", + translatable: true, + }); + await registry.createField({ + slug: "twitter_handle", + label: "Twitter", + type: "url", + translatable: false, + }); + await registry.createField({ + slug: "is_staff", + label: "Staff", + type: "boolean", + translatable: true, + }); + await registry.createField({ + slug: "tier", + label: "Tier", + type: "select", + translatable: true, + validation: { options: ["bronze", "silver", "gold"] }, + }); + + // Create a byline directly through the repository — the PUT + // route under test is for *updates*, and the create route is out + // of scope here. + const repo = new BylineRepository(db); + const byline = await repo.create({ + slug: "jane-doe", + displayName: "Jane Doe", + isGuest: true, + }); + bylineId = byline.id; + }); + + afterEach(async () => { + await teardownTestDatabase(db); + }); + + // =========================================== + // Round-trip + // =========================================== + + it("PUT writes customFields, GET reads them back", async () => { + const putBody = { + ...basePut, + customFields: { + job_title: "Senior editor", + twitter_handle: "https://twitter.com/jane", + is_staff: true, + tier: "gold", + }, + }; + const putRes = await putById( + buildContext({ + db, + request: putReq(bylineId, putBody), + params: { id: bylineId }, + user: adminUser, + }), + ); + expect(putRes.status).toBe(200); + const putJson = (await putRes.json()) as { + data: { customFields?: Record }; + }; + expect(putJson.data.customFields).toMatchObject({ + job_title: "Senior editor", + twitter_handle: "https://twitter.com/jane", + is_staff: true, + tier: "gold", + }); + + const getRes = await getById( + buildContext({ db, request: getReq(bylineId), params: { id: bylineId }, user: adminUser }), + ); + expect(getRes.status).toBe(200); + const getJson = (await getRes.json()) as { + data: { customFields?: Record }; + }; + expect(getJson.data.customFields).toMatchObject({ + job_title: "Senior editor", + twitter_handle: "https://twitter.com/jane", + is_staff: true, + tier: "gold", + }); + }); + + it("null customField value clears that slot", async () => { + const writeRes = await putById( + buildContext({ + db, + request: putReq(bylineId, { ...basePut, customFields: { job_title: "Editor" } }), + params: { id: bylineId }, + user: adminUser, + }), + ); + // Assert the initial write landed — otherwise the clear assertion + // below would pass trivially (the key is absent because the value + // was never written, not because the clear cleared it). + expect(writeRes.status).toBe(200); + + const clearRes = await putById( + buildContext({ + db, + request: putReq(bylineId, { ...basePut, customFields: { job_title: null } }), + params: { id: bylineId }, + user: adminUser, + }), + ); + expect(clearRes.status).toBe(200); + + const getRes = await getById( + buildContext({ db, request: getReq(bylineId), params: { id: bylineId }, user: adminUser }), + ); + const getJson = (await getRes.json()) as { + data: { customFields?: Record }; + }; + // `null` cleared the row; the key is absent on read (Phase 3 AC). + expect(getJson.data.customFields).not.toHaveProperty("job_title"); + }); + + // =========================================== + // Validation failures → 400 + // =========================================== + + it("unknown customField slug → 400 VALIDATION_ERROR", async () => { + const res = await putById( + buildContext({ + db, + request: putReq(bylineId, { + ...basePut, + customFields: { not_a_registered_field: "x" }, + }), + params: { id: bylineId }, + user: adminUser, + }), + ); + expect(res.status).toBe(400); + expect(await res.json()).toMatchObject({ error: { code: "VALIDATION_ERROR" } }); + }); + + it("reserved-slug write returns 400 (no registered field has that slug)", async () => { + // `display_name` is reserved, so no custom field can claim it. + // The repo therefore treats it as an unknown key → 400. + const res = await putById( + buildContext({ + db, + request: putReq(bylineId, { + ...basePut, + customFields: { display_name: "X" }, + }), + params: { id: bylineId }, + user: adminUser, + }), + ); + expect(res.status).toBe(400); + expect(await res.json()).toMatchObject({ error: { code: "VALIDATION_ERROR" } }); + }); + + it("type mismatch (string expected, boolean sent) → 400", async () => { + const res = await putById( + buildContext({ + db, + request: putReq(bylineId, { + ...basePut, + customFields: { job_title: true }, + }), + params: { id: bylineId }, + user: adminUser, + }), + ); + expect(res.status).toBe(400); + }); + + it("select-choice mismatch → 400", async () => { + const res = await putById( + buildContext({ + db, + request: putReq(bylineId, { + ...basePut, + customFields: { tier: "platinum" }, // not in validation.options + }), + params: { id: bylineId }, + user: adminUser, + }), + ); + expect(res.status).toBe(400); + }); + + // =========================================== + // Auth — bylines:manage gate still applies to customFields writes + // =========================================== + + it("returns 401 without a session", async () => { + const res = await putById( + buildContext({ + db, + request: putReq(bylineId, { ...basePut, customFields: { job_title: "X" } }), + params: { id: bylineId }, + user: null, + }), + ); + expect(res.status).toBe(401); + }); + + it("EDITOR can write customFields (bylines:manage = EDITOR)", async () => { + const res = await putById( + buildContext({ + db, + request: putReq(bylineId, { ...basePut, customFields: { job_title: "Editor" } }), + params: { id: bylineId }, + user: editorUser, + }), + ); + expect(res.status).toBe(200); + }); + + it("AUTHOR (below bylines:manage) → 403", async () => { + const authorUser = { id: "author-1", role: Role.AUTHOR }; + const res = await putById( + buildContext({ + db, + request: putReq(bylineId, { ...basePut, customFields: { job_title: "X" } }), + params: { id: bylineId }, + user: authorUser, + }), + ); + expect(res.status).toBe(403); + }); +}); diff --git a/packages/core/tests/unit/api/byline-fields-route-registration.test.ts b/packages/core/tests/unit/api/byline-fields-route-registration.test.ts new file mode 100644 index 000000000..5714f012f --- /dev/null +++ b/packages/core/tests/unit/api/byline-fields-route-registration.test.ts @@ -0,0 +1,68 @@ +/** + * Byline-fields route registration test (Phase 4 of Discussion #1174). + * + * Regression guard mirroring `email-settings-route.test.ts` (#151). + * Route files under `src/astro/routes/` are only reachable in real + * integrated apps if `injectCoreRoutes` calls `injectRoute` for them — + * the directory layout alone does nothing. This test asserts that + * every byline-fields endpoint is wired up. + * + * Order matters too: the static `/byline-fields/reorder` route must be + * registered before the dynamic `/byline-fields/[slug]` route so + * Astro's resolver dispatches POST /byline-fields/reorder to the + * reorder handler instead of treating "reorder" as a slug. The + * `reorder` slug is also reserved at the data layer + * (RESERVED_BYLINE_FIELD_SLUGS) for defence in depth, but route + * ordering is the primary mechanism. + */ + +import { describe, expect, it, vi } from "vitest"; + +import { injectCoreRoutes } from "../../../src/astro/integration/routes.js"; + +interface InjectRouteCall { + pattern: string; + entrypoint: string; +} + +describe("byline-fields route registration (Phase 4)", () => { + function collectPatterns(): { patterns: string[]; ordered: InjectRouteCall[] } { + const injectRoute = vi.fn(); + injectCoreRoutes(injectRoute); + const calls = injectRoute.mock.calls.map((call) => call[0] as InjectRouteCall); + return { patterns: calls.map((c) => c.pattern), ordered: calls }; + } + + it("registers list / create at /_emdash/api/admin/byline-fields", () => { + const { patterns } = collectPatterns(); + expect(patterns).toContain("/_emdash/api/admin/byline-fields"); + }); + + it("registers single-field CRUD at /_emdash/api/admin/byline-fields/[slug]", () => { + const { patterns } = collectPatterns(); + expect(patterns).toContain("/_emdash/api/admin/byline-fields/[slug]"); + }); + + it("registers reorder at /_emdash/api/admin/byline-fields/reorder", () => { + const { patterns } = collectPatterns(); + expect(patterns).toContain("/_emdash/api/admin/byline-fields/reorder"); + }); + + it("registers usage at /_emdash/api/admin/byline-fields/[slug]/usage", () => { + const { patterns } = collectPatterns(); + expect(patterns).toContain("/_emdash/api/admin/byline-fields/[slug]/usage"); + }); + + it("registers the static `reorder` route before the dynamic `[slug]` route", () => { + // Astro's route resolver picks static over dynamic, but the + // `routes.ts` ordering still matters for build determinism and + // matches the convention used by every other reorder/[slug] + // pairing in the file (see schema/collections/[slug]/fields/). + const { patterns } = collectPatterns(); + const reorderIdx = patterns.indexOf("/_emdash/api/admin/byline-fields/reorder"); + const slugIdx = patterns.indexOf("/_emdash/api/admin/byline-fields/[slug]"); + expect(reorderIdx).toBeGreaterThanOrEqual(0); + expect(slugIdx).toBeGreaterThanOrEqual(0); + expect(reorderIdx).toBeLessThan(slugIdx); + }); +}); From e934f25d2cba69dfabfd0b22cbb14ffaac9b222a Mon Sep 17 00:00:00 2001 From: mohamedh Date: Thu, 28 May 2026 22:50:45 +0100 Subject: [PATCH 14/35] fix(admin): RTL-safe spacing, Lingui placeholder, error states for byline-schema --- .../src/components/BylineFieldEditor.tsx | 315 ++++++++++++ packages/admin/src/routes/byline-schema.tsx | 463 ++++++++++++++++++ 2 files changed, 778 insertions(+) create mode 100644 packages/admin/src/components/BylineFieldEditor.tsx create mode 100644 packages/admin/src/routes/byline-schema.tsx diff --git a/packages/admin/src/components/BylineFieldEditor.tsx b/packages/admin/src/components/BylineFieldEditor.tsx new file mode 100644 index 000000000..f5e9647f3 --- /dev/null +++ b/packages/admin/src/components/BylineFieldEditor.tsx @@ -0,0 +1,315 @@ +/** + * BylineFieldEditor (Phase 5 of Discussion #1174). + * + * Purpose-built create/edit dialog for byline custom-field definitions. + * Constrains the type select to the five v1 types declared by the + * `BylineFieldType` union — narrower than the content-field universe — + * and exposes the `translatable` toggle (the storage-split flag) that + * the content `FieldEditor` doesn't have. + * + * Single-step form (not the type-picker → config flow used by + * `FieldEditor`): with only five types a ` handleLabelChange(e.target.value)} + placeholder={t`Job title`} + /> +
+ setField("slug", e.target.value)} + // The literal slug is an example identifier, not natural-language + // copy — translators will typically leave it as-is, but the + // AGENTS.md "every user-facing string via Lingui" rule applies + // uniformly, so the catalog extractor sees it. + placeholder={t`job_title`} + disabled={isEdit} + /> + {isEdit && ( +

+ {t`Slugs cannot be changed after the field is created.`} +

+ )} +
+ + + {/* Type — disabled on edit because the storage column depends on it */} +
+ onChange(e.target.value === "" ? null : e.target.value)} + /> + ); + case "text": + return ( + onChange(e.target.value === "" ? null : e.target.value)} + rows={3} + /> + ); + case "url": + return ( + onChange(e.target.value === "" ? null : e.target.value)} + /> + ); + case "boolean": + // Booleans are always definite once the field is registered — + // `null` would mean "no row stored", which conceptually maps + // to `false` for a yes/no toggle. The Switch sends a real + // boolean and the storage path persists it verbatim. + return ( + onChange(checked)} + /> + ); + case "select": { + const options = field.validation?.options ?? []; + // The empty-string entry serves as a "clear value" affordance; + // it produces `null` on change, which deletes the stored row + // per the repo's storage contract. Server-side `required` + // enforcement (if added later) would reject the resulting + // write rather than this dropdown disabling the entry. + const items: Record = { "": t`-- Select --` }; + for (const opt of options) items[opt] = opt; + return ( + `, which accepts `.fill()` directly. + */ + +import { Toast } from "@cloudflare/kumo"; +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import * as React from "react"; +import { describe, it, expect, vi, beforeEach } from "vitest"; + +import type { BylineFieldDefinition } from "../src/lib/api/byline-fields"; +import type { BylineSummary } from "../src/lib/api/bylines"; +import { render } from "./utils/render.tsx"; + +// TanStack Router pieces consumed by the bylines route. Mocked to +// avoid wiring a full RouterProvider just for one component test. +vi.mock("@tanstack/react-router", async () => { + const actual = + await vi.importActual("@tanstack/react-router"); + return { + ...actual, + useSearch: () => ({ locale: undefined }), + useNavigate: () => vi.fn(), + }; +}); + +// Bylines API surface — every call the page makes is mocked. The +// per-test setup below configures fixtures and tracks `updateByline` +// calls so the final assertion can verify the PATCH body shape. +vi.mock("../src/lib/api/bylines", async () => { + const actual = + await vi.importActual("../src/lib/api/bylines"); + return { + ...actual, + fetchBylines: vi.fn(), + fetchByline: vi.fn(), + fetchBylineTranslations: vi.fn().mockResolvedValue({ items: [] }), + createByline: vi.fn(), + updateByline: vi.fn(), + deleteByline: vi.fn(), + createBylineTranslation: vi.fn(), + }; +}); + +vi.mock("../src/lib/api/users", async () => { + const actual = + await vi.importActual("../src/lib/api/users"); + return { + ...actual, + fetchUsers: vi.fn().mockResolvedValue({ items: [], nextCursor: undefined }), + }; +}); + +vi.mock("../src/lib/api/byline-fields", async () => { + const actual = await vi.importActual( + "../src/lib/api/byline-fields", + ); + return { + ...actual, + listBylineFields: vi.fn(), + }; +}); + +// Manifest used by the locale switcher. Single-locale stub keeps the +// multi-locale UI off — irrelevant to this AC. +vi.mock("../src/lib/api/client", async () => { + const actual = + await vi.importActual("../src/lib/api/client"); + return { + ...actual, + fetchManifest: vi.fn().mockResolvedValue({ + version: "0.0.0", + hash: "test", + collections: {}, + plugins: {}, + authMode: "passkey", + taxonomies: [], + }), + }; +}); + +const { fetchBylines, fetchByline, updateByline } = await import("../src/lib/api/bylines"); +const { listBylineFields } = await import("../src/lib/api/byline-fields"); +const { BylinesPage } = await import("../src/routes/bylines"); + +// --------------------------------------------------------------------------- +// Fixtures +// --------------------------------------------------------------------------- + +function makeByline(overrides: Partial = {}): BylineSummary { + return { + id: "byline_01", + slug: "jane-doe", + displayName: "Jane Doe", + bio: null, + avatarMediaId: null, + websiteUrl: null, + userId: null, + isGuest: true, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + locale: "en", + translationGroup: null, + customFields: {}, + ...overrides, + }; +} + +function makeField(overrides: Partial = {}): BylineFieldDefinition { + return { + id: "fld_01", + slug: "job_title", + label: "Job title", + type: "string", + required: false, + translatable: true, + validation: null, + sortOrder: 0, + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(), + ...overrides, + }; +} + +function TestWrapper({ children }: { children: React.ReactNode }) { + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false }, mutations: { retry: false } }, + }); + return ( + + {children} + + ); +} + +beforeEach(() => { + vi.clearAllMocks(); +}); + +// --------------------------------------------------------------------------- +// Tests +// --------------------------------------------------------------------------- + +describe("BylinesPage — custom-field inputs (Phase 6 of #1174)", () => { + it("renders the custom-field input after selecting a byline", async () => { + const byline = makeByline(); + vi.mocked(fetchBylines).mockResolvedValue({ items: [byline], nextCursor: undefined }); + vi.mocked(fetchByline).mockResolvedValue(byline); + vi.mocked(listBylineFields).mockResolvedValue({ + items: [makeField({ label: "Job title", slug: "job_title", type: "string" })], + }); + + const screen = await render( + + + , + ); + + // Click the byline in the sidebar list — moves the form into + // edit mode and the custom-field section becomes visible. + const bylineButton = screen.getByRole("button", { name: /Jane Doe/ }); + await bylineButton.click(); + + // AC: registered fields render as inputs alongside the fixed + // columns. + await expect.element(screen.getByText("Custom fields")).toBeInTheDocument(); + await expect.element(screen.getByLabelText("Job title")).toBeInTheDocument(); + }); + + it("does not render the custom-field section in create mode", async () => { + vi.mocked(fetchBylines).mockResolvedValue({ items: [], nextCursor: undefined }); + vi.mocked(listBylineFields).mockResolvedValue({ + items: [makeField({ slug: "job_title", type: "string" })], + }); + + const screen = await render( + + + , + ); + + // Server-side `bylineCreateBody` doesn't accept `customFields`; + // showing inputs in create mode would set expectations the API + // can't meet. The "Create byline" heading is the create-mode + // signal; the Custom fields section must NOT appear. + await expect.element(screen.getByText("Create byline")).toBeInTheDocument(); + expect(screen.getByText("Custom fields").query()).toBeNull(); + }); + + it("forwards customFields in the PATCH body on save", async () => { + const byline = makeByline({ + customFields: { job_title: "Senior editor" }, + }); + vi.mocked(fetchBylines).mockResolvedValue({ items: [byline], nextCursor: undefined }); + vi.mocked(fetchByline).mockResolvedValue(byline); + vi.mocked(listBylineFields).mockResolvedValue({ + items: [makeField({ slug: "job_title", type: "string" })], + }); + vi.mocked(updateByline).mockResolvedValue(byline); + + const screen = await render( + + + , + ); + + await screen.getByRole("button", { name: /Jane Doe/ }).click(); + + // Prefilled value from the byline's customFields hydration. + const input = screen.getByLabelText("Job title"); + await expect.element(input).toHaveValue("Senior editor"); + + // Save without modifying — proves the PATCH body still includes + // the customFields map (the form's `toFormState` populated it + // from the response, and `updateMutation` spreads it). + await screen.getByRole("button", { name: "Save" }).click(); + + // Vitest-browser-react resolves the mutation asynchronously; + // give the click a microtask before asserting. + await new Promise((resolve) => setTimeout(resolve, 50)); + + expect(vi.mocked(updateByline)).toHaveBeenCalledTimes(1); + const [bylineId, body] = vi.mocked(updateByline).mock.calls[0]!; + expect(bylineId).toBe(byline.id); + expect(body.customFields).toEqual({ job_title: "Senior editor" }); + }); + + it("omits customFields from the PATCH body when field-defs fail to load", async () => { + // Regression guard for the silent-overwrite scenario flagged in + // the Phase 6 review. When `listBylineFields` errors, the form + // can't render the inputs, so the editor cannot see what + // they'd be saving. The Save button stays enabled so editors + // can still update fixed columns (bio, slug, etc.), but the + // PATCH body must NOT include `customFields` — otherwise the + // hydrated map would round-trip and a "field deleted + // server-side mid-session" race would surface as a 400. + const byline = makeByline({ + customFields: { job_title: "Senior editor" }, + }); + vi.mocked(fetchBylines).mockResolvedValue({ items: [byline], nextCursor: undefined }); + vi.mocked(fetchByline).mockResolvedValue(byline); + vi.mocked(listBylineFields).mockRejectedValue(new Error("network blip")); + vi.mocked(updateByline).mockResolvedValue(byline); + + const screen = await render( + + + , + ); + + await screen.getByRole("button", { name: /Jane Doe/ }).click(); + + // Error surface is shown — editor is told what's happening. + await expect.element(screen.getByText("Couldn't load custom fields.")).toBeInTheDocument(); + + // Save fixed columns; PATCH body must omit `customFields`. + await screen.getByRole("button", { name: "Save" }).click(); + await new Promise((resolve) => setTimeout(resolve, 50)); + + expect(vi.mocked(updateByline)).toHaveBeenCalledTimes(1); + const [, body] = vi.mocked(updateByline).mock.calls[0]!; + expect(body).not.toHaveProperty("customFields"); + // Sanity: the fixed columns ARE in the body — proves the Save + // path actually fired and the omission is targeted. + expect(body.slug).toBe(byline.slug); + expect(body.displayName).toBe(byline.displayName); + }); + + it("renders the right Kumo input for each registered field type", async () => { + const byline = makeByline({ + customFields: { + job_title: "Editor", + bio_long: "A long bio.", + homepage: "https://example.com", + is_staff: true, + tier: "gold", + }, + }); + vi.mocked(fetchBylines).mockResolvedValue({ items: [byline], nextCursor: undefined }); + vi.mocked(fetchByline).mockResolvedValue(byline); + vi.mocked(listBylineFields).mockResolvedValue({ + items: [ + makeField({ id: "f1", slug: "job_title", label: "Job title", type: "string" }), + makeField({ id: "f2", slug: "bio_long", label: "Long bio", type: "text" }), + makeField({ id: "f3", slug: "homepage", label: "Homepage", type: "url" }), + makeField({ id: "f4", slug: "is_staff", label: "Staff", type: "boolean" }), + makeField({ + id: "f5", + slug: "tier", + label: "Tier", + type: "select", + validation: { options: ["bronze", "silver", "gold"] }, + }), + ], + }); + + const screen = await render( + + + , + ); + + await screen.getByRole("button", { name: /Jane Doe/ }).click(); + + // `string` and `url` both render as ``, but their `type` + // attribute differentiates them. `text` renders as `