ADR-0019 §6: runtime enrichment + row pinpointing

Closes the placeholder-substitution gap reported during manual testing: FK violations were rendering `<value>` and `<column>` literally because the App had no schema awareness. With this change the runtime resolves the schema-dependent facts before the App ever sees the failure. ## Architecture - **Database** gains two public methods backed by new worker Request variants: - `read_relationships(table)` → (outbound, inbound) FK list (lifts the previously-private `read_relationships_*` pair into the public surface, behind a `RelationshipsReply` type alias). - `find_rows_matching(table, column, value, limit)` → `DataResult` for row pinpoint queries. - **friendly module** gets: - New `FailureContext` struct: schema-resolved facts the runtime builds (table, column, value, parent_table, parent_column, child_table, optional diagnostic_table). - `TranslateContext` loses its lifetime parameter and gains `parent_table` / `parent_column` fields. All string fields are now `Option<String>` for ownership simplicity. - `TranslateContext::from_facts(operation, verbosity, facts)` helper. - Translator's FK paths now use `ctx.parent_table` / `ctx.parent_column` for child-side wording; FK Update gets a dedicated `fk_child_side_update` arm. - FK dispatch is enrichment-driven first (`parent_table` set → child-side; `child_table` set → parent-side), with operation as the tiebreaker. - The translator forwards `ctx.diagnostic_table` onto the `FriendlyError` so pinpointed rows render through the existing ADR-0017 §7 bordered renderer. - **Event** `DslFailed` carries `(command, error, facts)`. The runtime populates `facts` via `enrich_dsl_failure` before posting the event. - **Runtime** `enrich_dsl_failure(database, command, error)` classifies and resolves: - UNIQUE INSERT/UPDATE: parses `T.col` from engine message, finds the user's attempted value (with schema fallback for natural-order multi-value INSERT — including the serial/shortid auto-skip rule from `do_insert`), pinpoints the existing conflicting row(s) via `find_rows_matching` and renders as a `DiagnosticTable`. - NOT NULL INSERT/UPDATE: parses `T.col`; no value (definitionally null) and no pinpoint (engine doesn't identify the row). - FK INSERT/UPDATE: outbound relationship lookup picks the FK column the user is touching; resolves `parent_table`/`parent_column`/`value`. UPDATE falls back to inbound (parent-side) when no outbound match. - FK DELETE: inbound relationship lookup picks a child_table that references this row. - **App** drops its old `attempted_value_for` / `column_from_qualified_target` helpers (their work moved to runtime where the Database is in scope). `build_translate_context` combines the runtime-supplied facts with the operation derived from the Command and the App's verbosity. ## Manual-test fixes folded in Two issues surfaced during manual testing of the initial implementation, both fixed: 1. Natural-order multi-value INSERT (`insert into Orders values (4, 11.99)`) skipped FK enrichment because `user_value_for_column` only knew the single-value short form. The schema-aware lookup (`user_value_for_column_with_schema`) now mirrors `do_insert`'s position-mapping rule (auto-generated columns skipped), so positional INSERTs onto tables with serial/shortid PKs resolve correctly. Regression test: `enrich_fk_insert_natural_order_multi_value_resolves_via_schema`. 2. The arity error on INSERT now lists the columns it expected — `expected 3 value(s) for (id, Name, Email), got 2` instead of the bare count. Surfaces what the user needs to fix without making them go check the schema. ## Tests `tests/friendly_enrichment.rs` (+8 integration tests): - UNIQUE INSERT with explicit columns: facts.{table, column, value, diagnostic_table} all resolved; pinpoint shows conflicting row. - UNIQUE INSERT natural-order short form: schema fallback resolves the value. - UNIQUE UPDATE: value pulled from assignments. - NOT NULL INSERT: table+column resolved, value None (correct), no pinpoint. - FK INSERT: parent_table, parent_column, value all resolved via outbound relationship lookup. - FK INSERT natural-order multi-value: schema-aware lookup with auto-skip resolves correctly (regression for the manual-test bug). - FK DELETE: child_table resolved via inbound relationship lookup. - DbError::Unsupported: enrichment returns default FailureContext (no false positives). App-level tests updated to populate `FailureContext` directly (simulating runtime enrichment) for the verbosity / threading checks. ## Tally 610 tests passing (was 603: +8 enrichment integration tests minus 1 obsolete App-side helper test that the runtime absorbed). Clippy clean with nursery lints. Release builds.
2026-05-09 22:10:05 +00:00
parent eac7e5b81d
commit 431645ae60
8 changed files with 1231 additions and 218 deletions
@@ -31,7 +31,7 @@

 use crate::db::{DbError, SqliteErrorKind};
 use crate::dsl::Type;
-use crate::friendly::error::FriendlyError;
+use crate::friendly::error::{DiagnosticTable, FriendlyError};
 use crate::t;

 /// Verbosity of the rendered error.
@@ -100,29 +100,76 @@ impl Operation {
    }
 }

+/// Schema-resolved facts about a failure (ADR-0019 §6).
+///
+/// Built by the runtime (where the `Database` handle is
+/// available) and passed to the App via
+/// [`crate::event::AppEvent::DslFailed`]. The App combines a
+/// `FailureContext` with its current verbosity and the
+/// operation derived from the originating `Command` to build a
+/// [`TranslateContext`].
+///
+/// Every field is optional — the runtime fills what it can
+/// resolve and leaves the rest `None`. The translator falls
+/// back to its `{name}`-form placeholders where data is
+/// missing.
+#[derive(Debug, Clone, Default)]
+pub struct FailureContext {
+    /// Operation table from the command (may differ from
+    /// `parent_table` for FK violations).
+    pub table: Option<String>,
+    /// Column name resolved from engine error (`UNIQUE
+    /// constraint failed: T.col` etc.) or via FK relationship
+    /// lookup.
+    pub column: Option<String>,
+    /// User's attempted value for the offending column.
+    pub value: Option<String>,
+    /// For child-side FK violations: the parent table the FK
+    /// references.
+    pub parent_table: Option<String>,
+    /// For child-side FK violations: the parent column the
+    /// FK references.
+    pub parent_column: Option<String>,
+    /// For parent-side FK violations: a child table that
+    /// references this row.
+    pub child_table: Option<String>,
+    /// Pinpointed offending row(s) per ADR-0019 §6 / ADR-0017
+    /// §7. Rendered through the bordered diagnostic-table
+    /// renderer when present.
+    pub diagnostic_table: Option<DiagnosticTable>,
+}
+
 /// Context the translator uses to pick catalog keys and fill
 /// placeholders. Every field is optional — the translator falls
-/// back to abstract wording where context is missing.
+/// back to its `{name}`-form placeholders where context is
+/// missing.
 #[derive(Debug, Clone, Default)]
-pub struct TranslateContext<'a> {
+pub struct TranslateContext {
    pub operation: Option<Operation>,
-    pub table: Option<&'a str>,
-    pub column: Option<&'a str>,
+    pub table: Option<String>,
+    pub column: Option<String>,
    /// For parent-side FK violations: the child table that
    /// references this row. Surfaced as `{child_table}` in
    /// `error.foreign_key.parent_side.*`.
-    pub child_table: Option<&'a str>,
+    pub child_table: Option<String>,
+    /// For child-side FK violations: the parent table the FK
+    /// references. Surfaced as `{parent_table}` in
+    /// `error.foreign_key.child_side.*`.
+    pub parent_table: Option<String>,
+    /// For child-side FK violations: the parent column the FK
+    /// references. Surfaced as `{parent_column}`.
+    pub parent_column: Option<String>,
    pub src_type: Option<Type>,
    pub target_type: Option<Type>,
-    /// User-attempted value for INSERT/UPDATE; surfaced as the
-    /// `{value}` placeholder in UNIQUE / FK / type-mismatch
-    /// wording. Best-effort: callsites populate when they have
-    /// it; translator falls back to `<value>` otherwise.
+    /// User-attempted value for INSERT/UPDATE.
    pub value: Option<String>,
+    /// Pinpointed offending row(s); rendered onto the
+    /// `FriendlyError::diagnostic_table` field when present.
+    pub diagnostic_table: Option<DiagnosticTable>,
    pub verbosity: Verbosity,
 }

-impl<'a> TranslateContext<'a> {
+impl TranslateContext {
    /// Convenience constructor for the common "I just have an
    /// operation" case.
    #[must_use]
@@ -132,13 +179,36 @@ impl<'a> TranslateContext<'a> {
            ..Self::default()
        }
    }
+
+    /// Combine schema-resolved facts with operation and
+    /// verbosity to build the full translator input.
+    #[must_use]
+    pub fn from_facts(
+        operation: Operation,
+        verbosity: Verbosity,
+        facts: FailureContext,
+    ) -> Self {
+        Self {
+            operation: Some(operation),
+            table: facts.table,
+            column: facts.column,
+            child_table: facts.child_table,
+            parent_table: facts.parent_table,
+            parent_column: facts.parent_column,
+            src_type: None,
+            target_type: None,
+            value: facts.value,
+            diagnostic_table: facts.diagnostic_table,
+            verbosity,
+        }
+    }
 }

 /// Classify `error` and produce a structured [`FriendlyError`].
 /// See module docs for the classification flow.
 #[must_use]
-pub fn translate(error: &DbError, ctx: &TranslateContext<'_>) -> FriendlyError {
-    match error {
+pub fn translate(error: &DbError, ctx: &TranslateContext) -> FriendlyError {
+    let mut fe = match error {
        DbError::Sqlite { message, kind } => translate_sqlite(message, *kind, ctx),
        // Unsupported / InvalidValue carry text that is already
        // engine-neutral and friendly (constructed by our own
@@ -154,13 +224,20 @@ pub fn translate(error: &DbError, ctx: &TranslateContext<'_>) -> FriendlyError {
        DbError::WorkerGone => passthrough(
            "the database worker is no longer available — the application must restart",
        ),
+    };
+    // Attach the row pinpoint when the runtime resolved one.
+    // The translator never builds the table itself — it only
+    // forwards what enrichment supplied.
+    if fe.diagnostic_table.is_none() {
+        fe.diagnostic_table = ctx.diagnostic_table.clone();
    }
+    fe
 }

 fn translate_sqlite(
    message: &str,
    kind: SqliteErrorKind,
-    ctx: &TranslateContext<'_>,
+    ctx: &TranslateContext,
 ) -> FriendlyError {
    // `change column ... --dont-convert` lets the engine
    // accept or refuse each cell. Whatever the engine returns
@@ -184,7 +261,7 @@ fn translate_sqlite(
    }
 }

-fn translate_type_mismatch_change_column(ctx: &TranslateContext<'_>) -> FriendlyError {
+fn translate_type_mismatch_change_column(ctx: &TranslateContext) -> FriendlyError {
    let table = ctx_table(ctx);
    let column = ctx_column(ctx);
    let src_type = ctx
@@ -211,7 +288,7 @@ fn translate_type_mismatch_change_column(ctx: &TranslateContext<'_>) -> Friendly
    )
 }

-fn translate_constraint(message: &str, ctx: &TranslateContext<'_>) -> FriendlyError {
+fn translate_constraint(message: &str, ctx: &TranslateContext) -> FriendlyError {
    let lower = message.to_ascii_lowercase();
    if lower.contains("unique constraint failed") {
        translate_unique(message, ctx)
@@ -228,7 +305,7 @@ fn translate_constraint(message: &str, ctx: &TranslateContext<'_>) -> FriendlyEr

 // ---- UNIQUE -----------------------------------------------------

-fn translate_unique(message: &str, ctx: &TranslateContext<'_>) -> FriendlyError {
+fn translate_unique(message: &str, ctx: &TranslateContext) -> FriendlyError {
    let (table, column) = parse_qualified_target(message)
        .unwrap_or_else(|| (ctx_table(ctx), ctx_column(ctx)));
    let value = ctx_value(ctx);
@@ -274,27 +351,41 @@ fn translate_unique(message: &str, ctx: &TranslateContext<'_>) -> FriendlyError

 // ---- FOREIGN KEY -----------------------------------------------

-fn translate_foreign_key(ctx: &TranslateContext<'_>) -> FriendlyError {
+fn translate_foreign_key(ctx: &TranslateContext) -> FriendlyError {
    // The engine's "FOREIGN KEY constraint failed" carries no
-    // detail. Disambiguation is operation-driven: child-side
-    // happens on INSERT/UPDATE (the row being written points at
-    // a missing parent); parent-side happens on DELETE/UPDATE
-    // (the row being deleted is referenced by a child).
+    // detail. Disambiguation is enrichment-driven first
+    // (`parent_table` populated → child-side; `child_table`
+    // populated → parent-side), with operation as the
+    // tiebreaker when enrichment didn't run.
    //
-    // Without context we default to child-side INSERT, which
-    // is the more common case and matches the wording of the
-    // pre-H1 enrich_fk_message helper.
+    // - Insert always points "outward" → child-side.
+    // - Delete always points "inward" → parent-side.
+    // - Update can be either; we let the enrichment payload
+    //   choose, defaulting to child-side (the more pedagogically
+    //   common case for a learner).
+    if ctx.parent_table.is_some() {
+        return match ctx.operation {
+            Some(Operation::Update) => fk_child_side_update(ctx),
+            _ => fk_child_side_insert(ctx),
+        };
+    }
+    if ctx.child_table.is_some() {
+        return match ctx.operation {
+            Some(Operation::Update) => fk_parent_side_update(ctx),
+            _ => fk_parent_side_delete(ctx),
+        };
+    }
    match ctx.operation {
        Some(Operation::Delete) => fk_parent_side_delete(ctx),
-        Some(Operation::Update) => fk_parent_side_update(ctx),
+        Some(Operation::Update) => fk_child_side_update(ctx),
        Some(Operation::Insert) => fk_child_side_insert(ctx),
        _ => fk_child_side_insert(ctx),
    }
 }

-fn fk_child_side_insert(ctx: &TranslateContext<'_>) -> FriendlyError {
-    let parent_table = ctx_table(ctx);
-    let parent_column = ctx_column(ctx);
+fn fk_child_side_insert(ctx: &TranslateContext) -> FriendlyError {
+    let parent_table = ctx_parent_table(ctx);
+    let parent_column = ctx_parent_column(ctx);
    let value = ctx_value(ctx);
    fe(
        t!(
@@ -314,11 +405,31 @@ fn fk_child_side_insert(ctx: &TranslateContext<'_>) -> FriendlyError {
    )
 }

-fn fk_parent_side_delete(ctx: &TranslateContext<'_>) -> FriendlyError {
+fn fk_child_side_update(ctx: &TranslateContext) -> FriendlyError {
+    let parent_table = ctx_parent_table(ctx);
+    let parent_column = ctx_parent_column(ctx);
+    let value = ctx_value(ctx);
+    fe(
+        t!(
+            "error.foreign_key.child_side.update.headline",
+            parent_table = parent_table,
+            parent_column = parent_column,
+            value = value
+        ),
+        verbose_hint(
+            ctx,
+            t!(
+                "error.foreign_key.child_side.update.hint",
+                parent_table = parent_table,
+                parent_column = parent_column
+            ),
+        ),
+    )
+}
+
+fn fk_parent_side_delete(ctx: &TranslateContext) -> FriendlyError {
    let table = ctx_table(ctx);
-    let child_table = ctx
-        .child_table
-        .map_or_else(|| "another table".to_string(), str::to_string);
+    let child_table = ctx_child_table(ctx);
    fe(
        t!(
            "error.foreign_key.parent_side.delete.headline",
@@ -329,11 +440,9 @@ fn fk_parent_side_delete(ctx: &TranslateContext<'_>) -> FriendlyError {
    )
 }

-fn fk_parent_side_update(ctx: &TranslateContext<'_>) -> FriendlyError {
+fn fk_parent_side_update(ctx: &TranslateContext) -> FriendlyError {
    let table = ctx_table(ctx);
-    let child_table = ctx
-        .child_table
-        .map_or_else(|| "another table".to_string(), str::to_string);
+    let child_table = ctx_child_table(ctx);
    fe(
        t!(
            "error.foreign_key.parent_side.update.headline",
@@ -346,7 +455,7 @@ fn fk_parent_side_update(ctx: &TranslateContext<'_>) -> FriendlyError {

 // ---- NOT NULL --------------------------------------------------

-fn translate_not_null(message: &str, ctx: &TranslateContext<'_>) -> FriendlyError {
+fn translate_not_null(message: &str, ctx: &TranslateContext) -> FriendlyError {
    let (table, column) = parse_qualified_target(message)
        .unwrap_or_else(|| (ctx_table(ctx), ctx_column(ctx)));
    match ctx.operation {
@@ -371,7 +480,7 @@ fn translate_not_null(message: &str, ctx: &TranslateContext<'_>) -> FriendlyErro

 // ---- CHECK -----------------------------------------------------

-fn translate_check(_message: &str, ctx: &TranslateContext<'_>) -> FriendlyError {
+fn translate_check(_message: &str, ctx: &TranslateContext) -> FriendlyError {
    // The engine reports CHECK constraint failures by constraint
    // name, not by column. We don't have user-named CHECK
    // constraints today, so the message is rarely informative.
@@ -400,13 +509,13 @@ fn translate_check(_message: &str, ctx: &TranslateContext<'_>) -> FriendlyError

 // ---- not_found / already_exists --------------------------------

-fn translate_not_found_table(message: &str, ctx: &TranslateContext<'_>) -> FriendlyError {
+fn translate_not_found_table(message: &str, ctx: &TranslateContext) -> FriendlyError {
    let name = parse_after_colon(message)
        .map_or_else(|| ctx_table(ctx), str::to_string);
    headline_only(t!("error.not_found.table.headline", name = name))
 }

-fn translate_not_found_column(message: &str, ctx: &TranslateContext<'_>) -> FriendlyError {
+fn translate_not_found_column(message: &str, ctx: &TranslateContext) -> FriendlyError {
    let name = parse_after_colon(message).unwrap_or("");
    if let Some((table, column)) = name.split_once('.') {
        headline_only(t!(
@@ -428,7 +537,7 @@ fn translate_not_found_column(message: &str, ctx: &TranslateContext<'_>) -> Frie
    }
 }

-fn translate_already_exists(message: &str, ctx: &TranslateContext<'_>) -> FriendlyError {
+fn translate_already_exists(message: &str, ctx: &TranslateContext) -> FriendlyError {
    // Three shapes feed in:
    //   - Engine: "table T already exists"
    //   - Our own: "column `T.col` already exists; pick a different name."
@@ -469,7 +578,7 @@ fn translate_already_exists(message: &str, ctx: &TranslateContext<'_>) -> Friend

 // ---- Generic catch-all -----------------------------------------

-fn translate_generic(_message: &str, ctx: &TranslateContext<'_>) -> FriendlyError {
+fn translate_generic(_message: &str, ctx: &TranslateContext) -> FriendlyError {
    // Engine message is intentionally NOT surfaced — ADR-0002
    // posture. The catalog provides the abstract wording.
    let operation = ctx
@@ -508,7 +617,7 @@ const fn fe(headline: String, hint: Option<String>) -> FriendlyError {
    }
 }

-fn verbose_hint(ctx: &TranslateContext<'_>, hint: String) -> Option<String> {
+fn verbose_hint(ctx: &TranslateContext, hint: String) -> Option<String> {
    if ctx.verbosity == Verbosity::Verbose {
        Some(hint)
    } else {
@@ -520,23 +629,34 @@ fn verbose_hint(ctx: &TranslateContext<'_>, hint: String) -> Option<String> {
 // the catalog's `{name}` form so unfilled positions read as
 // "this placeholder was not supplied" — same shape the
 // translator's source uses, easier to grep, and visually
-// consistent with the catalog templates. Filling these
-// properly across the board needs schema-aware enrichment in
-// the runtime; that work is bundled with the row-pinpoint
-// re-query (ADR-0019 §6) since both need the Database handle.
+// consistent with the catalog templates. With runtime-side
+// enrichment (ADR-0019 §6) populating `FailureContext`,
+// these fallbacks rarely render in practice.

-fn ctx_table(ctx: &TranslateContext<'_>) -> String {
-    ctx.table.map_or_else(|| "{table}".to_string(), str::to_string)
+fn ctx_table(ctx: &TranslateContext) -> String {
+    ctx.table.clone().unwrap_or_else(|| "{table}".to_string())
 }

-fn ctx_column(ctx: &TranslateContext<'_>) -> String {
-    ctx.column.map_or_else(|| "{column}".to_string(), str::to_string)
+fn ctx_column(ctx: &TranslateContext) -> String {
+    ctx.column.clone().unwrap_or_else(|| "{column}".to_string())
 }

-fn ctx_value(ctx: &TranslateContext<'_>) -> String {
+fn ctx_value(ctx: &TranslateContext) -> String {
    ctx.value.clone().unwrap_or_else(|| "{value}".to_string())
 }

+fn ctx_parent_table(ctx: &TranslateContext) -> String {
+    ctx.parent_table.clone().unwrap_or_else(|| "{parent_table}".to_string())
+}
+
+fn ctx_parent_column(ctx: &TranslateContext) -> String {
+    ctx.parent_column.clone().unwrap_or_else(|| "{parent_column}".to_string())
+}
+
+fn ctx_child_table(ctx: &TranslateContext) -> String {
+    ctx.child_table.clone().unwrap_or_else(|| "{child_table}".to_string())
+}
+
 /// Extract `T.col` from a message like
 /// `"UNIQUE constraint failed: T.col"`. Returns `(T, col)` or
 /// `None` if the message doesn't have the expected shape.
@@ -587,7 +707,7 @@ fn parse_after_word<'a>(message: &'a str, keyword: &str) -> Option<&'a str> {
 mod tests {
    use super::*;

-    fn ctx_with(op: Operation) -> TranslateContext<'static> {
+    fn ctx_with(op: Operation) -> TranslateContext {
        TranslateContext {
            operation: Some(op),
            ..TranslateContext::default()
@@ -665,8 +785,8 @@ mod tests {
            SqliteErrorKind::UniqueViolation,
        );
        let mut ctx = ctx_with(Operation::Insert);
-        ctx.table = Some("Customers");
-        ctx.column = Some("id");
+        ctx.parent_table = Some("Customers".to_string());
+        ctx.parent_column = Some("id".to_string());
        ctx.value = Some("99".to_string());
        let f = translate(&err, &ctx);
        assert!(
@@ -674,6 +794,7 @@ mod tests {
            "expected child-side phrasing: {}",
            f.headline
        );
+        assert!(f.headline.contains("Customers"));
        assert!(f.headline.contains("`99`"));
    }

@@ -684,8 +805,8 @@ mod tests {
            SqliteErrorKind::UniqueViolation,
        );
        let mut ctx = ctx_with(Operation::Delete);
-        ctx.table = Some("Customers");
-        ctx.child_table = Some("Orders");
+        ctx.table = Some("Customers".to_string());
+        ctx.child_table = Some("Orders".to_string());
        let f = translate(&err, &ctx);
        // Anchor phrase: "referenced by".
        assert!(
@@ -720,8 +841,8 @@ mod tests {
            SqliteErrorKind::UniqueViolation,
        );
        let mut ctx = ctx_with(Operation::Insert);
-        ctx.table = Some("People");
-        ctx.column = Some("age");
+        ctx.table = Some("People".to_string());
+        ctx.column = Some("age".to_string());
        let f = translate(&err, &ctx);
        assert!(f.headline.contains("check constraint refused"));
        assert!(f.headline.contains("People"));