feat: ADR-0035 Amendment 1 follow-up — enrich replay errors + close message gaps
- F2-broad: replay failures now render with real schema context instead of
a contextless friendly_message(). Extract App::build_translate_context into
the shared App::translate_context_for(command, facts, verbosity); run_replay
enriches via enrich_dsl_failure + that builder. ctx_* fallbacks degrade to
neutral prose so the rare non-replay contextless callsites can't leak raw
{name} either. (SQL INSERT/UPDATE values aren't retained — ADR-0033 verbatim
— so those show real table/column + neutral "that value".)
- Gap C: SQL ALTER … ADD FOREIGN KEY on a missing child column refuses with an
SQL-appropriate "add it first", not the DSL-only --create-fk flag.
- Gap B: dropping a single-column-UNIQUE column refuses with a pointer to
`drop constraint unique from T.col` (was an opaque generic refusal).
- Gap D: 4e drop/rename CHECK-guard + 4f change-type FK-guard refusals reworded
to explain why; static_refusal reasons left as-is.
Tests: +4, 3 strengthened. 1926 pass / 0 fail / 0 skip; clippy clean.
This commit is contained in:
+24
-12
@@ -1559,20 +1559,32 @@ impl App {
|
||||
));
|
||||
}
|
||||
|
||||
/// Construct a [`TranslateContext`] by combining the
|
||||
/// runtime-supplied [`FailureContext`] (schema-resolved
|
||||
/// facts) with the operation derived from the originating
|
||||
/// [`Command`] and the App's current verbosity.
|
||||
///
|
||||
/// Schema-resolved facts win over Command-derived
|
||||
/// fallbacks where the runtime supplied them — typically
|
||||
/// the runtime knows more (the FK-relationship lookup
|
||||
/// produces `parent_table` that the Command alone can't
|
||||
/// reveal).
|
||||
/// Construct a [`TranslateContext`] from a [`Command`] + schema-
|
||||
/// resolved [`FailureContext`], using the App's current verbosity.
|
||||
/// Thin wrapper over [`Self::translate_context_for`], which is shared
|
||||
/// with the replay path (it supplies its own verbosity — ADR-0035
|
||||
/// Amendment 1, F2 follow-up).
|
||||
fn build_translate_context(
|
||||
&self,
|
||||
command: &Command,
|
||||
facts: crate::friendly::FailureContext,
|
||||
) -> crate::friendly::TranslateContext {
|
||||
Self::translate_context_for(command, facts, self.messages_verbosity)
|
||||
}
|
||||
|
||||
/// Combine the runtime-supplied [`FailureContext`] (schema-resolved
|
||||
/// facts) with the operation derived from the originating [`Command`]
|
||||
/// and an explicit `verbosity`. Schema-resolved facts win over
|
||||
/// Command-derived fallbacks where the runtime supplied them
|
||||
/// (typically the FK-relationship lookup yields a `parent_table` the
|
||||
/// Command alone can't reveal). Shared by interactive rendering and
|
||||
/// the replay failure path (ADR-0035 Amendment 1, F2 follow-up), so a
|
||||
/// replayed failing command shows real names instead of leaking
|
||||
/// `{name}` placeholders.
|
||||
pub(crate) fn translate_context_for(
|
||||
command: &Command,
|
||||
facts: crate::friendly::FailureContext,
|
||||
verbosity: crate::friendly::Verbosity,
|
||||
) -> crate::friendly::TranslateContext {
|
||||
use crate::dsl::{AlterTableAction, Command as C, IndexSelector, RelationshipSelector};
|
||||
use crate::friendly::{Operation, TranslateContext};
|
||||
@@ -1714,7 +1726,7 @@ impl App {
|
||||
// An `explain` failure (e.g. unknown table) is best
|
||||
// described by the wrapped query it failed to plan.
|
||||
C::Explain { query } => {
|
||||
return self.build_translate_context(query, facts);
|
||||
return Self::translate_context_for(query, facts, verbosity);
|
||||
}
|
||||
// App-lifecycle commands never reach this path —
|
||||
// `dispatch_input` routes them through
|
||||
@@ -1741,7 +1753,7 @@ impl App {
|
||||
value: facts.value,
|
||||
diagnostic_table: facts.diagnostic_table,
|
||||
check_rule: facts.check_rule,
|
||||
verbosity: self.messages_verbosity,
|
||||
verbosity,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -4379,6 +4379,19 @@ fn do_drop_column(
|
||||
)));
|
||||
}
|
||||
|
||||
// A single-column UNIQUE on this column (ADR-0029): the engine refuses
|
||||
// to drop a column carrying a UNIQUE constraint. Unlike a composite
|
||||
// UNIQUE (handled above), a single-column UNIQUE is removed by the
|
||||
// column-level `drop constraint` — point there (ADR-0035 Amendment 1,
|
||||
// gap B).
|
||||
if col_info.unique {
|
||||
return Err(DbError::Unsupported(format!(
|
||||
"cannot drop `{table}.{column}` — it has a UNIQUE constraint; \
|
||||
remove the constraint first (`drop constraint unique from \
|
||||
{table}.{column}`), then drop the column."
|
||||
)));
|
||||
}
|
||||
|
||||
// A CHECK (table-level, or a *different* column's column-level CHECK)
|
||||
// that references this column (ADR-0035 §4e, the 4a.3 deferral): a
|
||||
// deliberate up-front refusal — dropping the column would break that
|
||||
@@ -4387,8 +4400,10 @@ fn do_drop_column(
|
||||
// Friendly wording is H1. Guards both surfaces.
|
||||
if column_referenced_by_check(conn, table, &schema, column, false)? {
|
||||
return Err(DbError::Unsupported(format!(
|
||||
"cannot drop `{table}.{column}` while a CHECK references it; \
|
||||
drop the constraint first."
|
||||
"cannot drop `{table}.{column}` — a CHECK constraint refers to \
|
||||
it, and dropping the column would leave that rule pointing at \
|
||||
a column that no longer exists. Drop or change the CHECK \
|
||||
constraint first, then drop the column."
|
||||
)));
|
||||
}
|
||||
|
||||
@@ -4466,8 +4481,10 @@ fn do_rename_column(
|
||||
// Deliberate refusal (friendly wording is H1); guards both surfaces.
|
||||
if column_referenced_by_check(conn, table, &schema, old, true)? {
|
||||
return Err(DbError::Unsupported(format!(
|
||||
"cannot rename `{table}.{old}` while a CHECK references it; \
|
||||
drop the constraint first."
|
||||
"cannot rename `{table}.{old}` — a CHECK constraint refers to \
|
||||
it by name, and the rename would leave that rule pointing at \
|
||||
the old name. Drop or change the CHECK constraint first, then \
|
||||
rename the column."
|
||||
)));
|
||||
}
|
||||
if old == new {
|
||||
@@ -4797,8 +4814,10 @@ fn do_change_column_type(
|
||||
.map_err(DbError::from_rusqlite)?;
|
||||
if outbound_count > 0 {
|
||||
return Err(DbError::Unsupported(format!(
|
||||
"cannot change type of `{table}.{column}` while a relationship \
|
||||
uses it as a foreign key; drop the relationship first."
|
||||
"cannot change the type of `{table}.{column}` — a relationship \
|
||||
uses it as a foreign key, and changing its type could break \
|
||||
the link to the table it references. Drop the relationship \
|
||||
first, then change the type."
|
||||
)));
|
||||
}
|
||||
|
||||
@@ -7181,6 +7200,22 @@ fn do_alter_add_foreign_key(
|
||||
}
|
||||
}
|
||||
};
|
||||
// The child column must already exist for `ALTER … ADD FOREIGN KEY` —
|
||||
// there is no SQL spelling to auto-create it (the `--create-fk` option
|
||||
// is the simple-mode `add relationship` surface only). Pre-check here
|
||||
// so the refusal speaks SQL, not the DSL flag (ADR-0035 Amendment 1,
|
||||
// gap C). A missing child *table* is left to `do_add_relationship`'s
|
||||
// own "no such table".
|
||||
if let Ok(child_schema) = read_schema(conn, child_table)
|
||||
&& child_schema.columns.iter().all(|c| c.name != fk.child_column)
|
||||
{
|
||||
return Err(DbError::Unsupported(format!(
|
||||
"column `{child_table}.{child}` does not exist — add it first \
|
||||
(`alter table {child_table} add column {child} <type>`), then \
|
||||
add the foreign key.",
|
||||
child = fk.child_column,
|
||||
)));
|
||||
}
|
||||
do_add_relationship(
|
||||
conn,
|
||||
persistence,
|
||||
@@ -11269,7 +11304,15 @@ mod tests {
|
||||
)
|
||||
.await
|
||||
.unwrap_err();
|
||||
assert!(matches!(err, DbError::Unsupported(_)), "got {err:?}");
|
||||
let DbError::Unsupported(msg) = &err else {
|
||||
panic!("expected Unsupported, got {err:?}");
|
||||
};
|
||||
// The refusal explains the FK link, not just that it failed
|
||||
// (ADR-0035 Amendment 1, gap D).
|
||||
assert!(
|
||||
msg.contains("uses it as a foreign key"),
|
||||
"explains the FK link; got: {msg}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
+34
-13
@@ -707,36 +707,38 @@ fn verbose_hint(ctx: &TranslateContext, hint: String) -> Option<String> {
|
||||
}
|
||||
}
|
||||
|
||||
// Fallback markers when context can't supply a value. We use
|
||||
// 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. With runtime-side
|
||||
// enrichment (ADR-0019 §6) populating `FailureContext`,
|
||||
// these fallbacks rarely render in practice.
|
||||
// Neutral-prose fallbacks when context can't supply a value
|
||||
// (ADR-0035 Amendment 1, F2 follow-up — the safety net). Runtime-side
|
||||
// enrichment (ADR-0019 §6) fills `FailureContext` on the interactive and
|
||||
// replay paths, so these rarely render; but the few contextless
|
||||
// `friendly_message()` callsites (undo / rebuild / export) must NOT
|
||||
// surface a raw `{name}` placeholder, which reads like a bug. The earlier
|
||||
// `{name}`-marker form was a developer-facing tell that predated those
|
||||
// callsites rendering in practice; neutral prose degrades gracefully
|
||||
// instead.
|
||||
|
||||
fn ctx_table(ctx: &TranslateContext) -> String {
|
||||
ctx.table.clone().unwrap_or_else(|| "{table}".to_string())
|
||||
ctx.table.clone().unwrap_or_else(|| "the table".to_string())
|
||||
}
|
||||
|
||||
fn ctx_column(ctx: &TranslateContext) -> String {
|
||||
ctx.column.clone().unwrap_or_else(|| "{column}".to_string())
|
||||
ctx.column.clone().unwrap_or_else(|| "the column".to_string())
|
||||
}
|
||||
|
||||
fn ctx_value(ctx: &TranslateContext) -> String {
|
||||
ctx.value.clone().unwrap_or_else(|| "{value}".to_string())
|
||||
ctx.value.clone().unwrap_or_else(|| "that value".to_string())
|
||||
}
|
||||
|
||||
fn ctx_parent_table(ctx: &TranslateContext) -> String {
|
||||
ctx.parent_table.clone().unwrap_or_else(|| "{parent_table}".to_string())
|
||||
ctx.parent_table.clone().unwrap_or_else(|| "the referenced table".to_string())
|
||||
}
|
||||
|
||||
fn ctx_parent_column(ctx: &TranslateContext) -> String {
|
||||
ctx.parent_column.clone().unwrap_or_else(|| "{parent_column}".to_string())
|
||||
ctx.parent_column.clone().unwrap_or_else(|| "the referenced column".to_string())
|
||||
}
|
||||
|
||||
fn ctx_child_table(ctx: &TranslateContext) -> String {
|
||||
ctx.child_table.clone().unwrap_or_else(|| "{child_table}".to_string())
|
||||
ctx.child_table.clone().unwrap_or_else(|| "the referencing table".to_string())
|
||||
}
|
||||
|
||||
/// Extract `T.col` from a message like
|
||||
@@ -1063,6 +1065,25 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn constraint_templates_degrade_to_prose_without_context() {
|
||||
// F2 follow-up safety net: a constraint error rendered via a
|
||||
// contextless `friendly_message()` (no facts) degrades to neutral
|
||||
// prose, never a raw `{name}` marker.
|
||||
for kind in [
|
||||
SqliteErrorKind::UniqueViolation,
|
||||
SqliteErrorKind::Other,
|
||||
SqliteErrorKind::NoSuchColumn,
|
||||
] {
|
||||
let err = sqlite("constraint failed", kind);
|
||||
let rendered = translate(&err, &TranslateContext::default()).render();
|
||||
assert!(
|
||||
!rendered.contains('{'),
|
||||
"placeholder marker leaked for {kind:?}:\n{rendered}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
// ---- passthrough variants ----
|
||||
|
||||
#[test]
|
||||
|
||||
+15
-1
@@ -1855,6 +1855,9 @@ pub async fn run_replay(
|
||||
// CSVs) fires as if the user had typed each line. The
|
||||
// source re-journalled is the *extracted* command, not the
|
||||
// raw `<ts>|ok|…` record (ADR-0034 §3).
|
||||
// Retain a clone for failure enrichment (the command is moved into
|
||||
// dispatch). ADR-0035 Amendment 1, F2 follow-up.
|
||||
let command_for_ctx = command.clone();
|
||||
let outcome =
|
||||
execute_command_typed(database, command, command_text.clone()).await;
|
||||
match outcome {
|
||||
@@ -1877,11 +1880,22 @@ pub async fn run_replay(
|
||||
return events;
|
||||
}
|
||||
Err(e) => {
|
||||
// Enrich like the interactive path (ADR-0019 §6) so a
|
||||
// replayed failing command shows the real table/column/
|
||||
// value instead of a contextless, `{name}`-leaking message
|
||||
// (ADR-0035 Amendment 1, F2 follow-up). Verbose to match
|
||||
// the prior `friendly_message()` rendering.
|
||||
let facts = enrich_dsl_failure(database, &command_for_ctx, &e).await;
|
||||
let ctx = crate::app::App::translate_context_for(
|
||||
&command_for_ctx,
|
||||
facts,
|
||||
crate::friendly::Verbosity::default(),
|
||||
);
|
||||
events.push(AppEvent::ReplayFailed {
|
||||
path: path.to_string(),
|
||||
line_number,
|
||||
command: command_text.clone(),
|
||||
error: e.friendly_message(),
|
||||
error: crate::friendly::translate_error(&e, &ctx).render(),
|
||||
});
|
||||
return events;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user