Skip to content

Resolve typeguards per database and carry CONVERT keys - #331

Open
EnRaiha wants to merge 8 commits into
mainfrom
fix/issue318-typeguard-convert
Open

EnRaiha wants to merge 8 commits into
mainfrom
fix/issue318-typeguard-convert

Conversation

@EnRaiha

@EnRaiha EnRaiha commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

Summary

Three defects on the typeguard and CONVERT declaration paths, and one gate that all of them pass through:

  1. CREATE TYPEGUARD resolved against the wrong database. The handlers read DatabaseId::DEFAULT while collections live under the session database, so a guard declared in another database answered 42P01 for a collection that exists.
  2. CONVERT dropped the source primary key. The conversion rebuilt the strict schema from the column list, and the key column lost its primary-key mark; every later insert failed with "no resolved primary key". A column list that omits the key column is now refused.
  3. A cross-field guard carried onto a column DEFAULT failed at every insert. A guard VALUE LOWER(status) is evaluated per row; carried onto a strict column it becomes a DEFAULT, which is const-folded once with no row in scope — the CONVERT was accepted and the first insert failed with UnevaluableDefault. It is now refused at the declaration, naming field and clause.

The same invariant now holds at one gate (validate_constant_clause_expr) for every producer of a column DEFAULT: CREATE COLLECTION, a CONVERT column list, a CONVERT typeguard carry, and ALTER … ADD COLUMN.

Scope statement (requested by the audit). The issue text names defect 3 only. This PR also carries defects 1 and 2, which the maintainer's resubmission conditions grouped under #318 — typeguard database_id threading, CONVERT primary-key carry, and the cross-column refusal. They ride together as in-PR prerequisites of the same declaration path; they are stated here rather than split, per the audit's remedy.

Behaviour changes

Path Before After
CREATE TYPEGUARD in a session database 42P01 declared in that database
INSERT after CONVERT … (id TEXT, v TEXT) "no resolved primary key" inserts; the source key is carried
CONVERT column list that omits the source key accepted, the key lost 42601
CREATE / CONVERT / ALTER ADD COLUMN … DEFAULT LOWER(other) accepted; the first insert failed UnevaluableDefault 42601 at declaration
A guard VALUE LOWER(status) carried by CONVERT accepted; the first insert failed 42601 at the CONVERT, naming field and clause
ALTER … ADD COLUMN is_default BOOLEAN accepted, the name mis-read as a DEFAULT 'BOOLEAN' clause accepted, no clause read

Closes #318.

Root cause

  • Database resolution: typeguard/handlers.rs built every request with DatabaseId::DEFAULT; validate.rs did the same when resolving the target collection. The session's database never reached the handler.
  • Primary key carry: convert/driver.rs built the target schema from the explicit column list alone. The source's key column (primary_key) was not consulted, so the converted schema had no key and assign_target_surrogate failed on the next insert with "no resolved primary key".
  • Cross-field guard: CompiledDefault::declare classifies a DEFAULT; a function call like LOWER(status) parses to an Expr and was accepted. Nothing checked whether the expression names a column. The trap appeared only at the first insert (UnevaluableDefault).
  • The class: the reference check lived at two CONVERT call sites, so CREATE COLLECTION (collection/create/build.rs) and a CONVERT column list (convert/column_defs.rs) still accepted a cross-column DEFAULT and failed at the first insert. The audit found ALTER … ADD COLUMN as a fourth producer that bypassed the gate entirely.

What changed

Commit 1b7dbd907 — the three defects:

  • database_id threaded through the seven typeguard handlers and validate_typeguard; the router resolves the collection in the session database.
  • CONVERT marks the source key column in the converted schema; a column list that omits it is refused with 42601.
  • nodedb-sql exposes default_expr_references_columns — the classifier's own parse, no second name list — and CONVERT refuses a carried guard whose expression names another column.

Commit 780176fac — the explicit-list path: a guard the list covers and that names another column is refused there too (the list defines the schema and carries no guard onto it).

Commit 879b1329f — one gate:

  • validate_constant_clause_expr(clause, owner, expr) is the single place that refuses an unregistered function name, an unparsable expression, or a column reference; validate_column_default becomes the DEFAULT-clause form of it, and the two CONVERT-local copies of the reference check are deleted.
  • default_expr_references_columns now mirrors classify: a generator (UUID_V7, gen_uuid_v7()) or a literal/parametric form carries no expression and is never read as a column reference. (The first cut parsed the raw text and refused DEFAULT UUID_V7 — caught by the existing sql_default_expressions and sql_typeguard_defaults modules.)

Commit 11a3c7b26ALTER … ADD COLUMN: the declared definition passes the same gate.

Commit 82ea86a91 — wording: the carry path passes the clause through, so a guard VALUE reports VALUE, not DEFAULT (audit advisory).

Commit b9bc243fb — the gate reads the parsed DEFAULT, never the definition text, and the type parser's clause search requires the keyword to start a token: a column named is_default is no longer read as a DEFAULT clause.

Merge 44b21159a — folds origin/main (#336 instant typing, #327 constant-row cell keys) into
the branch, resolving the DDL-gate conflict by composing both sides: validate_column_default_clause
runs the constant/column-reference check and the literal type/range coercion at one gate.

Regression proof

On base main, without this change: the insert after CONVERT fails with "no resolved primary key", a column list that omits the key is accepted, the cross-column guard is accepted, CREATE TYPEGUARD in a session database answers 42P01, and a cross-column DEFAULT is accepted at CREATE/CONVERT/ALTER ("got success"). With this change each is refused or resolved as listed in Behaviour changes.

Tested

  • cargo nextest run -p nodedb --test wire -E 'test(cases::sql_convert_column_defs) | test(cases::sql_default_expressions) | test(cases::sql_typeguard_default_gate) | test(cases::sql_typeguard_defaults)'41 passed
  • unit: only_the_expression_branch_can_reference_a_column pins the classifier mirroring (generators and literals are not references; a column and a concatenation are)
  • audit's own runs: 23/23 touched-module tests, 228/228 blast-radius tests
  • cargo fmt --all -- --check, cargo clippy -p nodedb -p nodedb-sql --lib -- -D warnings, preflight: clean

Review

A separate, read-only parity audit ran over this branch before submission. Its findings and their resolutions:

Finding Resolution
The reference invariant was enforced at two CONVERT call sites, not the gate every producer calls one gate (879b1329f); CREATE, CONVERT, the explicit-list guard path and ALTER all route through it
ALTER … ADD COLUMN … DEFAULT bypassed the gate gated (11a3c7b26)
The ALTER gate read the whole definition, so a name containing default was read as the clause the gate reads the parsed default, and the parser's clause search requires the keyword to start a token (b9bc243fb)
Clause wording: a carried VALUE reported DEFAULT the clause is passed through (82ea86a91)
The issue text names one of the three defects this PR carries scope statement above; recorded on the issue

Exclusions

Commits

  • 1b7dbd907fix(typeguard): resolve in the session database and keep the CONVERT source key
  • 780176facfix(convert): refuse a cross-column guard on the explicit-list path
  • 879b1329ffix(ddl): refuse a column DEFAULT that names another column at the one gate
  • 11a3c7b26fix(ddl): gate an ALTER ADD COLUMN DEFAULT like a CREATE column DEFAULT
  • 82ea86a91fix(ddl): name the clause the author wrote on the carried guard
  • b9bc243fbfix(ddl): read an ALTER default from the parsed definition
  • 88f2edaa1style: rustfmt

Copilot AI lite review requested due to automatic review settings September 16, 2026 20:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@EnRaiha EnRaiha added the run-ci Opt this PR into the full test suite; re-add to force a re-run label Sep 16, 2026
@EnRaiha EnRaiha changed the title Fix the typeguard and CONVERT declaration gates Resolve typeguards per database and carry CONVERT keys Sep 16, 2026
@EnRaiha
EnRaiha force-pushed the fix/issue318-typeguard-convert branch from 88f2eda to 667d58f Compare September 17, 2026 02:16
@EnRaiha EnRaiha added run-ci Opt this PR into the full test suite; re-add to force a re-run and removed run-ci Opt this PR into the full test suite; re-add to force a re-run labels Sep 17, 2026
…source key

Three defects on the typeguard and CONVERT paths:

- CREATE TYPEGUARD read DatabaseId::DEFAULT while collections live under the
  session database; thread database_id through the seven handlers and
  validate_typeguard
- CONVERT rebuilt the strict schema from the column list and dropped the
  source primary key, so every later insert failed "no resolved primary key";
  mark the source key column and refuse a column list that omits it
- a guard DEFAULT or VALUE naming another column became a strict column
  DEFAULT that evaluates with no row in scope; refuse the guard at CONVERT,
  naming the field and clause
The explicit column list defines the schema and carries no guard onto it. A
guard the list covers and that names another column has no column DEFAULT
equivalent: evaluated with no row in scope it fails every insert, and
dropping it loses the guard's meaning silently. Refuse it at CONVERT, naming
field and clause — the shape the filed reproduction takes.

Coverage: the filed reproduction as a refusal, CONVERT keeping the source
primary key, a list that omits it refused, and a guard declared in a
session database (declaration resolves there).
…e gate

The reference check lived at two CONVERT call sites, so CREATE COLLECTION
and a CONVERT column list still accepted a cross-column DEFAULT and failed
at the first insert with UnevaluableDefault. The check moves into the gate
every producer calls.

- validate_constant_clause_expr: classify first, exactly as the default
  classifier does, so a generator (UUID_V7, gen_uuid_v7()) or a literal is
  never read as a column reference
- CREATE COLLECTION, a CONVERT column list, a CONVERT typeguard carry, and
  the explicit-list guard path all pass through it; the two local copies
  are gone
ALTER TABLE ADD COLUMN accepted a DEFAULT that names another column; the
first insert failed with UnevaluableDefault. The declared definition passes
the same gate CREATE and CONVERT columns pass.
A guard VALUE carried onto a column DEFAULT reported DEFAULT; the clause is
VALUE. The carry path passes the clause through the one gate, so the
refusal names what the author wrote. Same refusal either way; only the
wording changes.
The gate passed the whole definition text as type text, and the type parser
finds the clause by substring — a column named is_default was read as a
DEFAULT clause and refused. The gate now reads the parsed default, and the
parser's clause search requires the keyword to start a token, so a name that
contains the word is never the clause.
@EnRaiha
EnRaiha force-pushed the fix/issue318-typeguard-convert branch from 667d58f to ee544e7 Compare September 17, 2026 04:39
…d-convert

# Conflicts:
#	nodedb/src/control/server/shared/ddl/neutral/column_default.rs
#	nodedb/src/control/server/shared/ddl/neutral/convert/typeguard_columns.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

run-ci Opt this PR into the full test suite; re-add to force a re-run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A cross-field typeguard guard passes the DEFAULT gate at declaration and fails at every insert

2 participants