From cb5812259f9d1929ea745b2a33493f5ae699d3eb Mon Sep 17 00:00:00 2001 From: Aleksey Smaga Date: Thu, 30 Apr 2026 22:43:17 +0200 Subject: [PATCH 1/2] feat(checkers): add fk_without_on_delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Flag foreign keys whose ON DELETE action resolves to NO ACTION — the SQL default applied silently when the FK is declared without an ON DELETE clause. Severity info: NO ACTION is widespread and context-dependent, so the rule educates rather than blocks. --- README.md | 1 + docs/rules/fk_without_on_delete.md | 99 +++++++++++++ src/pgsleuth/checkers/__init__.py | 1 + src/pgsleuth/checkers/fk_without_on_delete.py | 82 +++++++++++ tests/checkers/test_fk_without_on_delete.py | 137 ++++++++++++++++++ 5 files changed, 320 insertions(+) create mode 100644 docs/rules/fk_without_on_delete.md create mode 100644 src/pgsleuth/checkers/fk_without_on_delete.py create mode 100644 tests/checkers/test_fk_without_on_delete.py diff --git a/README.md b/README.md index 684ba91..bd7e2c1 100644 --- a/README.md +++ b/README.md @@ -73,6 +73,7 @@ Exit codes: `0` = clean, `1` = issues found, `2` = error. | [`column_value_at_risk`](docs/rules/column_value_at_risk.md) | Sequence-backed columns whose sequence is past 70% of its type's max value. | | [`missing_fk_index`](docs/rules/missing_fk_index.md) | Foreign-key columns without a covering index — slow joins, slow `ON DELETE`. | | [`fk_type_mismatch`](docs/rules/fk_type_mismatch.md) | FK columns whose type differs from the referenced PK/unique column. | +| [`fk_without_on_delete`](docs/rules/fk_without_on_delete.md) | FKs with no explicit `ON DELETE` (defaults to `NO ACTION`, rarely the intended choice). | | [`redundant_index`](docs/rules/redundant_index.md) | Indexes whose column list is a strict prefix of another index on the table. | | [`not_valid_constraints`](docs/rules/not_valid_constraints.md) | FK and CHECK constraints stuck at `NOT VALID` (`convalidated = false`). | | [`sequence_drift`](docs/rules/sequence_drift.md) | Sequences whose `nextval` would collide with rows already in the table. | diff --git a/docs/rules/fk_without_on_delete.md b/docs/rules/fk_without_on_delete.md new file mode 100644 index 0000000..75fefdd --- /dev/null +++ b/docs/rules/fk_without_on_delete.md @@ -0,0 +1,99 @@ +# fk_without_on_delete + +> **Severity:** info +> Foreign keys with no explicit `ON DELETE` policy. + +## What it catches + +Any foreign key whose `ON DELETE` action is `NO ACTION` — both the implicit case (no `ON DELETE` clause at all) and the explicit `ON DELETE NO ACTION`. The catalog can't tell them apart, so the rule treats them the same. + +```sql +-- ⚠️ flagged: implicit NO ACTION +ALTER TABLE child + ADD FOREIGN KEY (parent_id) REFERENCES parent(id); + +-- ⚠️ flagged: explicit NO ACTION +ALTER TABLE child + ADD FOREIGN KEY (parent_id) REFERENCES parent(id) ON DELETE NO ACTION; + +-- ok: an intentional choice +ALTER TABLE child + ADD FOREIGN KEY (parent_id) REFERENCES parent(id) ON DELETE CASCADE; +ALTER TABLE child + ADD FOREIGN KEY (parent_id) REFERENCES parent(id) ON DELETE RESTRICT; +ALTER TABLE child + ADD FOREIGN KEY (parent_id) REFERENCES parent(id) ON DELETE SET NULL; +``` + +## Why it matters + +`NO ACTION` is the SQL standard's default — and the one Postgres applies silently if you leave the `ON DELETE` clause off. It is rarely the deliberate choice. The other options force the team to decide what should happen to dependent rows when a parent goes away: + +| Action | What it does | +| --- | --- | +| `RESTRICT` | Reject the parent delete immediately. Strict, explicit, easy to reason about. | +| `CASCADE` | Delete the child rows too. Right when the child has no meaning without the parent. | +| `SET NULL` | Null the FK column on the child. Right when the child is independent but related. | +| `SET DEFAULT` | Replace the FK with the column's default. Rare; mostly used with sentinel rows. | +| `NO ACTION` | Reject the parent delete *at the end of the statement*, allowing transient violations inside it. Almost identical to `RESTRICT` in practice — the difference is the deferral window. | + +The hidden cost of leaving it as the default isn't usually a correctness bug *today* — it's that the schema doesn't record the team's *intent*. When the question comes up later ("can I delete this parent? what happens to the children?"), the answer is "whatever `NO ACTION` does," which most engineers can't recite from memory. Stating the policy explicitly turns a hidden assumption into a reviewable line of SQL. + +There's also a real (if narrow) operational difference between `NO ACTION` and `RESTRICT`: + +- `NO ACTION` defers the check to the end of the statement; combined with `INITIALLY DEFERRED`, it can defer to the end of the transaction. Inside that window, the schema temporarily contains orphan rows. +- `RESTRICT` rejects the offending modification *immediately*, before any other rows in the same statement are processed. + +Most teams want `RESTRICT`-or-explicit-cascade semantics; `NO ACTION` lands them in deferred-check territory by accident. + +## How to fix + +Pick the action that matches the relationship and rewrite the constraint. Postgres can't change `ON DELETE` in place — drop and re-add: + +```sql +ALTER TABLE child + DROP CONSTRAINT child_parent_id_fkey, + ADD CONSTRAINT child_parent_id_fkey + FOREIGN KEY (parent_id) REFERENCES parent(id) + ON DELETE RESTRICT; +``` + +Both operations happen in a single transaction, so the constraint is never absent from the schema. The new constraint is `VALIDATED` immediately because all existing rows already satisfy it (the previous FK enforced the same referential rule). + +For very large child tables, an alternative is to add a *new* `NOT VALID` constraint with the desired `ON DELETE` policy, validate it, then drop the old one: + +```sql +ALTER TABLE child + ADD CONSTRAINT child_parent_id_fkey_new + FOREIGN KEY (parent_id) REFERENCES parent(id) + ON DELETE RESTRICT + NOT VALID; + +ALTER TABLE child VALIDATE CONSTRAINT child_parent_id_fkey_new; + +ALTER TABLE child DROP CONSTRAINT child_parent_id_fkey; +ALTER TABLE child RENAME CONSTRAINT child_parent_id_fkey_new TO child_parent_id_fkey; +``` + +This avoids the brief `ACCESS EXCLUSIVE` window of the drop-and-add, at the cost of a longer migration script. + +## When to ignore + +Suppress the rule project-wide if your team has a deliberate policy of relying on `NO ACTION` (rare but valid — e.g. you depend on deferred-check semantics inside a multi-statement transaction): + +```toml +[pgsleuth.checkers.fk_without_on_delete] +enabled = false +``` + +Or scope it to a specific table where the deferred-check window is intentional: + +```toml +[pgsleuth] +exclude_tables = ["^ledger_entries$"] +``` + +## See also + +- [PostgreSQL — `CREATE TABLE`: foreign key actions](https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-PARMS-REFERENCES) +- [`missing_fk_index`](missing_fk_index.md) — pairs naturally; `ON DELETE CASCADE` without an index on the FK column is the slowest delete in your schema. diff --git a/src/pgsleuth/checkers/__init__.py b/src/pgsleuth/checkers/__init__.py index d4ea1a7..0893a62 100644 --- a/src/pgsleuth/checkers/__init__.py +++ b/src/pgsleuth/checkers/__init__.py @@ -4,6 +4,7 @@ from pgsleuth.checkers import ( # noqa: F401 column_value_at_risk, fk_type_mismatch, + fk_without_on_delete, json_over_jsonb, missing_fk_index, missing_primary_key, diff --git a/src/pgsleuth/checkers/fk_without_on_delete.py b/src/pgsleuth/checkers/fk_without_on_delete.py new file mode 100644 index 0000000..e76d506 --- /dev/null +++ b/src/pgsleuth/checkers/fk_without_on_delete.py @@ -0,0 +1,82 @@ +"""Foreign keys with no explicit ON DELETE policy. + +Postgres' default `ON DELETE` action is `NO ACTION`, applied silently when +the FK is declared without an `ON DELETE` clause. NO ACTION is rarely the +deliberate choice: teams usually want `RESTRICT` (be strict and explicit +about it), `CASCADE`, or `SET NULL`. Flagging FKs that resolved to NO +ACTION nudges the team to make the choice intentionally — and to revisit +it when business rules change. + +The catalog stores only the resulting action, not whether the user wrote +`ON DELETE NO ACTION` explicitly. This rule treats both the implicit and +explicit cases the same: if the team genuinely wants NO ACTION, they can +suppress the rule on the constraint. +""" + +from __future__ import annotations + +from typing import ClassVar + +from pgsleuth.checkers.base import Issue, RowChecker, Severity, register +from pgsleuth.context import CheckerContext + +_SQL = """ +SELECT + n.nspname AS schema, + t.relname AS table, + c.conname AS constraint_name, + (SELECT array_agg(a.attname ORDER BY i.ord) + FROM unnest(c.conkey) WITH ORDINALITY AS i(attnum, ord) + JOIN pg_attribute a + ON a.attrelid = c.conrelid AND a.attnum = i.attnum) AS fk_columns, + rn.nspname AS referenced_schema, + rt.relname AS referenced_table, + (SELECT array_agg(a.attname ORDER BY i.ord) + FROM unnest(c.confkey) WITH ORDINALITY AS i(attnum, ord) + JOIN pg_attribute a + ON a.attrelid = c.confrelid AND a.attnum = i.attnum) AS referenced_columns +FROM pg_constraint c +JOIN pg_class t ON t.oid = c.conrelid +JOIN pg_namespace n ON n.oid = t.relnamespace +JOIN pg_class rt ON rt.oid = c.confrelid +JOIN pg_namespace rn ON rn.oid = rt.relnamespace +WHERE c.contype = 'f' + AND c.confdeltype = 'a' + {schema_filter} +ORDER BY n.nspname, t.relname, c.conname; +""" + + +class ForeignKeyWithoutOnDelete(RowChecker): + name: ClassVar[str] = "fk_without_on_delete" + description: ClassVar[str] = ( + "Foreign keys with no explicit ON DELETE policy (default NO ACTION)." + ) + default_severity: ClassVar[Severity] = Severity.INFO + sql: ClassVar[str] = _SQL + + def check_row(self, ctx: CheckerContext, row: dict) -> Issue | None: + fk_cols = ", ".join(row["fk_columns"]) + ref_cols = ", ".join(row["referenced_columns"]) + child = f"{row['schema']}.{row['table']}" + parent = f"{row['referenced_schema']}.{row['referenced_table']}" + obj = f"{child}({fk_cols})" + return self.issue( + ctx, + object_type="constraint", + object_name=obj, + message=( + f"Foreign key {row['constraint_name']!r} on {obj} has no explicit " + f"ON DELETE policy (defaults to NO ACTION)." + ), + suggestion=( + f"ALTER TABLE {child} " + f"DROP CONSTRAINT {row['constraint_name']}, " + f"ADD CONSTRAINT {row['constraint_name']} " + f"FOREIGN KEY ({fk_cols}) REFERENCES {parent}({ref_cols}) " + f"ON DELETE RESTRICT; -- or CASCADE / SET NULL, per intent" + ), + ) + + +register(ForeignKeyWithoutOnDelete) diff --git a/tests/checkers/test_fk_without_on_delete.py b/tests/checkers/test_fk_without_on_delete.py new file mode 100644 index 0000000..61c3272 --- /dev/null +++ b/tests/checkers/test_fk_without_on_delete.py @@ -0,0 +1,137 @@ +from pgsleuth.checkers.fk_without_on_delete import ForeignKeyWithoutOnDelete + + +def test_clean_when_on_delete_cascade(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE parent (id bigserial PRIMARY KEY)") + cur.execute( + "CREATE TABLE child (" + " id bigserial PRIMARY KEY," + " parent_id bigint REFERENCES parent(id) ON DELETE CASCADE" + ")" + ) + + issues = [i for i in ForeignKeyWithoutOnDelete().run(ctx) if i.object_name.startswith(schema)] + assert issues == [] + + +def test_clean_when_on_delete_restrict(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE parent (id bigserial PRIMARY KEY)") + cur.execute( + "CREATE TABLE child (" + " id bigserial PRIMARY KEY," + " parent_id bigint REFERENCES parent(id) ON DELETE RESTRICT" + ")" + ) + + issues = [i for i in ForeignKeyWithoutOnDelete().run(ctx) if i.object_name.startswith(schema)] + assert issues == [] + + +def test_clean_when_on_delete_set_null(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE parent (id bigserial PRIMARY KEY)") + cur.execute( + "CREATE TABLE child (" + " id bigserial PRIMARY KEY," + " parent_id bigint REFERENCES parent(id) ON DELETE SET NULL" + ")" + ) + + issues = [i for i in ForeignKeyWithoutOnDelete().run(ctx) if i.object_name.startswith(schema)] + assert issues == [] + + +def test_flags_implicit_no_action(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE parent (id bigserial PRIMARY KEY)") + cur.execute( + "CREATE TABLE child (" + " id bigserial PRIMARY KEY," + " parent_id bigint REFERENCES parent(id)" + ")" + ) + + issues = [i for i in ForeignKeyWithoutOnDelete().run(ctx) if i.object_name.startswith(schema)] + assert len(issues) == 1 + assert "child" in issues[0].object_name + assert "parent_id" in issues[0].object_name + + +def test_flags_explicit_no_action(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE parent (id bigserial PRIMARY KEY)") + cur.execute( + "CREATE TABLE child (" + " id bigserial PRIMARY KEY," + " parent_id bigint REFERENCES parent(id) ON DELETE NO ACTION" + ")" + ) + + issues = [i for i in ForeignKeyWithoutOnDelete().run(ctx) if i.object_name.startswith(schema)] + assert len(issues) == 1 + + +def test_composite_fk_columns_listed_in_order(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE parent (a bigint, b bigint, PRIMARY KEY (a, b))") + cur.execute( + "CREATE TABLE child (" + " id bigserial PRIMARY KEY," + " pa bigint," + " pb bigint," + " FOREIGN KEY (pa, pb) REFERENCES parent(a, b)" + ")" + ) + + issues = [i for i in ForeignKeyWithoutOnDelete().run(ctx) if i.object_name.startswith(schema)] + assert len(issues) == 1 + assert "(pa, pb)" in issues[0].object_name + + +def test_no_fks_means_no_findings(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE solo (id bigserial PRIMARY KEY, name text)") + + issues = [i for i in ForeignKeyWithoutOnDelete().run(ctx) if i.object_name.startswith(schema)] + assert issues == [] + + +def test_suggestion_references_actual_parent(ctx, conn, schema): + with conn.cursor() as cur: + cur.execute("CREATE TABLE parent (id bigserial PRIMARY KEY)") + cur.execute( + "CREATE TABLE child (" + " id bigserial PRIMARY KEY," + " parent_id bigint REFERENCES parent(id)" + ")" + ) + + issues = [i for i in ForeignKeyWithoutOnDelete().run(ctx) if i.object_name.startswith(schema)] + assert len(issues) == 1 + assert f"REFERENCES {schema}.parent(id)" in issues[0].suggestion + + +def test_suggestion_qualifies_cross_schema_parent(ctx, conn, schema): + other = f"{schema}_other" + with conn.cursor() as cur: + cur.execute(f"CREATE SCHEMA {other}") + try: + cur.execute(f"CREATE TABLE {other}.parent (id bigserial PRIMARY KEY)") + cur.execute( + f"CREATE TABLE child (" + f" id bigserial PRIMARY KEY," + f" parent_id bigint REFERENCES {other}.parent(id)" + f")" + ) + + issues = [ + i + for i in ForeignKeyWithoutOnDelete().run(ctx) + if i.object_name.startswith(schema) + ] + assert len(issues) == 1 + assert f"REFERENCES {other}.parent(id)" in issues[0].suggestion + finally: + cur.execute(f"DROP SCHEMA {other} CASCADE") From ca5fdcf0e58f3697368de08a0369f10d358f9b2b Mon Sep 17 00:00:00 2001 From: Aleksey Smaga Date: Thu, 30 Apr 2026 23:21:25 +0200 Subject: [PATCH 2/2] style: collapse cross-schema-parent test list-comp under ruff format --- tests/checkers/test_fk_without_on_delete.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/checkers/test_fk_without_on_delete.py b/tests/checkers/test_fk_without_on_delete.py index 61c3272..8890fb2 100644 --- a/tests/checkers/test_fk_without_on_delete.py +++ b/tests/checkers/test_fk_without_on_delete.py @@ -127,9 +127,7 @@ def test_suggestion_qualifies_cross_schema_parent(ctx, conn, schema): ) issues = [ - i - for i in ForeignKeyWithoutOnDelete().run(ctx) - if i.object_name.startswith(schema) + i for i in ForeignKeyWithoutOnDelete().run(ctx) if i.object_name.startswith(schema) ] assert len(issues) == 1 assert f"REFERENCES {other}.parent(id)" in issues[0].suggestion