From fa63c611bdeedbb44b7695630d07de7e2d87a21d Mon Sep 17 00:00:00 2001 From: Dave Gaeddert Date: Thu, 8 Oct 2026 15:41:36 -0500 Subject: [PATCH] Add `plain postgres models`, and stop doubling `%` in constraint literals MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `plain postgres models` describes the data model as the registry sees it, with no database: every field, every relation with its target and `on_delete` rule, the relations pointing back at each model with any declared reverse accessor, and constraints and indexes with their conditions compiled to SQL. `--json` carries the same facts as data, a target narrows it to a model (by class name, label or table) or a package, and `--all` adds the installed packages' models. `unique` and `1:1` mean effective uniqueness: an unconditional single-field unique constraint, never a partial one. The same facts are `plain.postgres.describe`. `quote_value` no longer doubles `%` in a string literal. The DDL it feeds runs with no parameters, so the doubled form was stored as written: a `CheckConstraint(check=~Q(name="100%"))` was created as `'100%%'` and let the one value it names through. Convergence recognizes a stored definition that only differs by the doubled literal and replaces the constraint under its own name — a check in one DROP/ADD statement then validated, a unique rebuilt concurrently under a temporary name and swapped in — instead of reporting a definition change you have to stage. A model edited from a `100%%` literal to `100%` looks the same to it and is replaced in place; the validation still has to pass. Also: `Options.unique_field_names`, `PackageConfig.is_app` in place of three prefix checks, `reversed_field` on reverse descriptors, and `plain postgres schema` sharing the same target matching. --- .claude/rules/plain-postgres.md | 4 + plain-postgres/plain/postgres/README.md | 51 ++- .../agents/.claude/rules/plain-postgres.md | 4 + plain-postgres/plain/postgres/cli/core.py | 2 + plain-postgres/plain/postgres/cli/models.py | 136 +++++++ plain-postgres/plain/postgres/cli/schema.py | 32 +- .../plain/postgres/convergence/analysis.py | 135 +++++-- .../plain/postgres/convergence/corrections.py | 81 +++- .../plain/postgres/convergence/planning.py | 4 + plain-postgres/plain/postgres/ddl.py | 19 +- plain-postgres/plain/postgres/describe.py | 367 ++++++++++++++++++ .../postgres/fields/reverse_descriptors.py | 6 + .../introspection/health/ownership.py | 2 +- plain-postgres/plain/postgres/options.py | 11 + .../plain/postgres/preflight/indexes.py | 2 +- ...edperson_describedprofile_describedslot.py | 46 +++ .../tests/app/examples/models/__init__.py | 1 + .../tests/app/examples/models/describe.py | 90 +++++ .../test_convergence_percent_literal.py | 126 ++++++ .../public/test_check_constraint_literals.py | 17 + .../tests/public/test_postgres_models_cli.py | 218 +++++++++++ plain/plain/packages/config.py | 6 + 22 files changed, 1285 insertions(+), 75 deletions(-) create mode 100644 plain-postgres/plain/postgres/cli/models.py create mode 100644 plain-postgres/plain/postgres/describe.py create mode 100644 plain-postgres/tests/app/examples/migrations/0029_describedperson_describedprofile_describedslot.py create mode 100644 plain-postgres/tests/app/examples/models/describe.py create mode 100644 plain-postgres/tests/internal/test_convergence_percent_literal.py create mode 100644 plain-postgres/tests/public/test_check_constraint_literals.py create mode 100644 plain-postgres/tests/public/test_postgres_models_cli.py diff --git a/.claude/rules/plain-postgres.md b/.claude/rules/plain-postgres.md index 65a7315e1d..63ed52362a 100644 --- a/.claude/rules/plain-postgres.md +++ b/.claude/rules/plain-postgres.md @@ -164,6 +164,10 @@ Run `uv run plain docs postgres` for full patterns with code examples. Run `uv run plain docs postgres` for full patterns with code examples. +## Reading the Data Model + +Before reasoning about how models relate — what cascades, what is one-to-one, what a reverse accessor is called — run `uv run plain postgres models` (a model name or package to scope it, `--json` for data). It needs no database. Don't infer cardinality from field definitions: uniqueness lives in constraints, and a partial unique is not a one-to-one. + ## Database Doctor Use the `/plain-postgres-doctor` skill to check overall database health — migration sync, schema correctness, and operational health. diff --git a/plain-postgres/plain/postgres/README.md b/plain-postgres/plain/postgres/README.md index 046892335a..cda80f6dc8 100644 --- a/plain-postgres/plain/postgres/README.md +++ b/plain-postgres/plain/postgres/README.md @@ -16,6 +16,7 @@ - [Fields](#fields) - [Relationships](#relationships) - [Constraints](#constraints) +- [Describing the data model](#describing-the-data-model) - [Forms](#forms) - [Architecture](#architecture) - [Diagnostics](#diagnostics) @@ -1293,6 +1294,7 @@ In development (`DEBUG=True`), sync auto-generates migrations before applying th | `plain postgres sync --check` | Exit non-zero if anything would change (for CI) | | `plain postgres schema` | Show schema state with drift detection | | `plain postgres schema --json` | Machine-readable schema output | +| `plain postgres models` | Describe the models: relations, constraints, reverse accessors | | `plain postgres converge` | Run convergence alone (advanced) | ### Structural migrations @@ -1477,6 +1479,7 @@ When you run `postgres sync`, convergence detects that these indexes and constra - **Per-operation commits** — each fix is committed independently so a single failure doesn't roll back other fixes. - **Self-healing** — detects and rebuilds `INVALID` indexes (e.g. from a previously failed `CREATE INDEX CONCURRENTLY`). - **Rename-aware** — detects renamed indexes and constraints by matching their structure, avoiding unnecessary drop + recreate. +- **Repairs doubled `%` literals** — a constraint created by an earlier plain-postgres stored `'100%'` as `'100%%'`. Convergence recognizes that one difference and replaces the constraint under its own name instead of treating it as a definition change you have to stage. **Inspecting schema state:** @@ -1484,7 +1487,7 @@ Use `postgres schema` to see what convergence would do. It shows every model's c ```bash plain postgres schema # all models -plain postgres schema User # single model +plain postgres schema User # one model, or a package plain postgres schema --json # machine-readable output ``` @@ -2123,6 +2126,52 @@ nickname: Field[str] = types.TextField(max_length=50, allow_null=True) nickname: Field[str] = types.TextField(max_length=50, default="") ``` +## Describing the data model + +`plain postgres models` prints the data model as the registry sees it: every field, every relation with its target and `on_delete` rule, the relations pointing back at each model, and the constraints and indexes with their conditions compiled to SQL. It reads the model classes, so it needs no database. + +```bash +plain postgres models # every model in the app +plain postgres models Thread # one model, by class name +plain postgres models changes.Thread # or by label, or by table name +plain postgres models changes # a whole package +plain postgres models --all # include models from installed packages +plain postgres models --json # the same facts, structured +``` + +``` +changes.Thread → changes_thread + id PrimaryKey PK + key Text unique + bot → projects.Bot CASCADE + owner → users.User RESTRICT + origin → changes.Thread SET_NULL null + ← terminals.Terminal.change filter: terminal + ← changes.Computer.thread 1:1 filter: one_offs + unique change_key_unique (key) + unique change_one_triggered_open_per_bot (bot) WHERE ("closed_at" IS NULL AND NOT ("trigger" = '')) + index change_bot_idx (bot) +``` + +The point is the facts you can't read off a `models.py` by eye: + +- **`unique` and `1:1` mean effective uniqueness.** A field is marked unique when an unconditional, single-field unique constraint covers it, and a foreign key covered that way is one-to-one. A partial unique (one with a `condition`) is listed with its `WHERE` clause and never promoted — at most one open thread per bot is not the same thing as one thread per bot. +- **Reverse relations are listed on the model they point at**, with the declared [`ReverseForeignKey`](./fields/reverse_descriptors.py#ReverseForeignKey) or [`ReverseManyToMany`](./fields/reverse_descriptors.py#ReverseManyToMany) accessor when there is one, and the name to use in `filter()` either way. +- **Check constraints, partial conditions and expression indexes are shown as the SQL Postgres enforces**, not as `Q` objects. One wrinkle: with no database connection open, a string literal that contains a backslash is written in Postgres's `E'...'` escape form, which means the same thing. +- **A relation whose target never resolved** (a typo in a string reference, a package no longer installed) shows the reference as written instead of failing the whole listing. + +`--json` carries everything above as data; its shape is the `to_dict()` of each [`ModelDescription`](./describe.py#ModelDescription). It is the input for anything built on top: an entity-relationship diagram, a containment outline, a what-deletes-what walk. Those are views, and they are left to whoever is reading, so the command stays a statement of fact. + +The same facts are available in Python through [`describe_models`](./describe.py#describe_models): + +```python +from plain.postgres.describe import describe_models + +for model in describe_models(("changes",)): + for relation in model.relations: + print(model.label, relation.name, relation.to, relation.on_delete) +``` + ## Forms Models integrate with [plain.forms](../../../plain-forms/plain/forms/README.md): diff --git a/plain-postgres/plain/postgres/agents/.claude/rules/plain-postgres.md b/plain-postgres/plain/postgres/agents/.claude/rules/plain-postgres.md index 65a7315e1d..63ed52362a 100644 --- a/plain-postgres/plain/postgres/agents/.claude/rules/plain-postgres.md +++ b/plain-postgres/plain/postgres/agents/.claude/rules/plain-postgres.md @@ -164,6 +164,10 @@ Run `uv run plain docs postgres` for full patterns with code examples. Run `uv run plain docs postgres` for full patterns with code examples. +## Reading the Data Model + +Before reasoning about how models relate — what cascades, what is one-to-one, what a reverse accessor is called — run `uv run plain postgres models` (a model name or package to scope it, `--json` for data). It needs no database. Don't infer cardinality from field definitions: uniqueness lives in constraints, and a partial unique is not a one-to-one. + ## Database Doctor Use the `/plain-postgres-doctor` skill to check overall database health — migration sync, schema correctness, and operational health. diff --git a/plain-postgres/plain/postgres/cli/core.py b/plain-postgres/plain/postgres/cli/core.py index 04868e5e69..d1f1e759d4 100644 --- a/plain-postgres/plain/postgres/cli/core.py +++ b/plain-postgres/plain/postgres/cli/core.py @@ -15,6 +15,7 @@ from .converge import converge from .decorators import cli_schema_lock, database_management_command from .diagnose import diagnose +from .models import models from .schema import schema from .sync import sync @@ -27,6 +28,7 @@ def cli() -> None: cli.add_command(converge) cli.add_command(diagnose) +cli.add_command(models) cli.add_command(schema) cli.add_command(sync) diff --git a/plain-postgres/plain/postgres/cli/models.py b/plain-postgres/plain/postgres/cli/models.py new file mode 100644 index 0000000000..8b0de526a4 --- /dev/null +++ b/plain-postgres/plain/postgres/cli/models.py @@ -0,0 +1,136 @@ +import json + +import click + +from ..describe import ModelDescription, describe_models + + +@click.command() +@click.argument("targets", nargs=-1) +@click.option("--json", "output_json", is_flag=True, help="Output as JSON") +@click.option( + "--all", + "include_packages", + is_flag=True, + help="Include models from installed packages, not just the app's", +) +def models( + targets: tuple[str, ...], + output_json: bool, + include_packages: bool, +) -> None: + """Describe the data model: fields, relations, constraints. + + TARGETS narrow the output to models (`Thread`, `changes.Thread`) or whole + packages (`changes`). Read from the model registry, so no database is needed. + """ + if include_packages and targets: + raise click.UsageError( + "--all widens the default selection; with targets it has nothing to do." + ) + + try: + descriptions = describe_models(targets, include_packages=include_packages) + except LookupError as e: + raise click.ClickException(str(e)) from e + + if output_json: + click.echo( + json.dumps({"models": [d.to_dict() for d in descriptions]}, indent=2) + ) + else: + for i, description in enumerate(descriptions): + if i > 0: + click.echo() + _render_model(description) + + +def _render_model(description: ModelDescription) -> None: + click.secho(description.label, bold=True, nl=False) + click.secho(f" → {description.table}", dim=True) + + for model_field in description.fields: + type_name = model_field.type.removesuffix("Field") + marks = [] + if model_field.primary_key: + marks.append(click.style("PK", fg="yellow")) + if model_field.unique: + marks.append(click.style("unique", fg="yellow")) + if model_field.null: + marks.append(click.style("null", dim=True)) + click.echo( + f" {model_field.name:24s} {_pad(styled=click.style(type_name, fg='cyan'), visible_length=len(type_name))} {' '.join(marks)}".rstrip() + ) + + for relation in description.relations: + if relation.type == "ManyToManyField": + target = f"⇆ {relation.to}" + marks = [click.style(f"through {relation.through}", dim=True)] + else: + target = f"→ {relation.to}" + marks = [_style_on_delete(relation.on_delete)] + if relation.unique: + marks.append(click.style("1:1", fg="yellow")) + if relation.null: + marks.append(click.style("null", dim=True)) + click.echo( + f" {relation.name:24s} {_pad(styled=click.style(target, fg='cyan'), visible_length=len(target))} {' '.join(marks)}" + ) + + for reverse in description.reverse: + source = f"{reverse.from_model}.{reverse.field}" + marks = [] + if reverse.accessor: + marks.append(click.style(f".{reverse.accessor}", fg="cyan")) + if reverse.one_to_one: + marks.append(click.style("1:1", fg="yellow")) + if reverse.type == "ManyToManyField": + marks.append(click.style("many-to-many", dim=True)) + marks.append(click.style(f"filter: {reverse.query_name}", dim=True)) + click.echo(f" {click.style('←', dim=True)} {source:48s} {' '.join(marks)}") + + for constraint in description.constraints: + if constraint.kind == "unique": + detail = _columns_detail( + columns=constraint.fields or constraint.expressions, + condition=constraint.condition, + ) + else: + detail = constraint.check or "" + _echo_schema_line(kind=constraint.kind, name=constraint.name, detail=detail) + + for index in description.indexes: + detail = _columns_detail( + columns=index.fields or index.expressions, condition=index.condition + ) + _echo_schema_line(kind="index", name=index.name, detail=detail) + + +def _columns_detail(*, columns: list[str], condition: str | None) -> str: + """`(a, b)`, plus ` WHERE ...` for a partial constraint or index. + + Expressions arrive parenthesized the way their DDL writes them, so an + expression index reads `((LOWER("name")))`, as in `CREATE INDEX`.""" + detail = f"({', '.join(columns)})" + if condition: + detail += f" WHERE {condition}" + return detail + + +def _pad(*, styled: str, visible_length: int, width: int = 22) -> str: + """Pad after the styled text, so the ANSI codes don't count toward the width.""" + return styled + " " * max(width - visible_length, 0) + + +def _echo_schema_line(*, kind: str, name: str, detail: str) -> None: + click.echo( + f" {click.style(f'{kind:8s}', dim=True)} {name} {click.style(detail, dim=True)}" + ) + + +def _style_on_delete(on_delete: str | None) -> str: + if on_delete == "CASCADE": + return click.style(on_delete, fg="red") + if on_delete == "SET_NULL": + return click.style(on_delete, fg="yellow") + return click.style(on_delete or "", fg="blue") diff --git a/plain-postgres/plain/postgres/cli/schema.py b/plain-postgres/plain/postgres/cli/schema.py index ebcffb5c5e..c23cbc0f6f 100644 --- a/plain-postgres/plain/postgres/cli/schema.py +++ b/plain-postgres/plain/postgres/cli/schema.py @@ -6,8 +6,8 @@ from ..convergence.analysis import ModelAnalysis, analyze_model from ..convergence.planning import can_auto_correct from ..db import get_connection +from ..describe import select_models from ..introspection import MANAGED_CONSTRAINT_TYPES, get_unknown_tables -from ..registry import models_registry from .decorators import database_management_command @@ -105,24 +105,20 @@ def _render_model(analysis: ModelAnalysis) -> None: @click.command() -@click.argument("model_label", required=False) +@click.argument("target", required=False) @click.option("--json", "output_json", is_flag=True, help="Output as JSON") @database_management_command -def schema(model_label: str | None, output_json: bool) -> None: - """Show database schema from models, compared against the actual database""" - models = models_registry.get_models() - - if model_label: - model_label_lower = model_label.lower() - models = [ - m - for m in models - if m.model_options.label_lower == model_label_lower - or m.model_options.db_table == model_label - or m.__name__.lower() == model_label_lower - ] - if not models: - raise click.ClickException(f"No model found matching '{model_label}'") +def schema(target: str | None, output_json: bool) -> None: + """Show database schema from models, compared against the actual database. + + TARGET narrows it to one model (by class name, label or table) or one + package. + """ + targets = (target,) if target else () + try: + models = select_models(targets, include_packages=True) + except LookupError as e: + raise click.ClickException(str(e)) from e conn = get_connection() @@ -132,7 +128,7 @@ def schema(model_label: str | None, output_json: bool) -> None: for model in models: analyses.append(analyze_model(conn, cursor, model)) - unknown_tables = get_unknown_tables(conn) if not model_label else [] + unknown_tables = get_unknown_tables(conn) if not target else [] total_issues = sum(a.issue_count for a in analyses) + len(unknown_tables) if output_json: diff --git a/plain-postgres/plain/postgres/convergence/analysis.py b/plain-postgres/plain/postgres/convergence/analysis.py index 77ecfb6dd5..2a9f487783 100644 --- a/plain-postgres/plain/postgres/convergence/analysis.py +++ b/plain-postgres/plain/postgres/convergence/analysis.py @@ -120,6 +120,31 @@ def describe(self) -> str: return f"{self.table}: constraint {self.constraint.name} definition changed" +@dataclass +class ConstraintPercentDrift: + """The DB's definition matches the model's except that every `%` in a + literal is doubled: the constraint was created by a plain-postgres that + escaped `%` for a parameter substitution the DDL never went through, so + `'100%'` was stored as `'100%%'`. Replaced under the same name. + + A stored `'100%%'` is also what a model literal of `100%%` writes now, so + a model edited from `100%%` to `100%` looks the same and is replaced in + place rather than staged under a new name. The replacement validates + existing rows and fails sync if they don't satisfy it, so the edit can't + slip through unchecked.""" + + table: str + constraint: CheckConstraint | UniqueConstraint + model: type[Model] + + kind: ClassVar[DriftKind] = DriftKind.CHANGED + + def describe(self) -> str: + return ( + f"{self.table}: constraint {self.constraint.name} has a doubled % literal" + ) + + @dataclass class ConstraintNameDrift: """An existing constraint to validate (UNVALIDATED) or drop (UNDECLARED).""" @@ -148,7 +173,12 @@ def describe(self) -> str: return f"{self.table}: constraint {self.old_name} → {self.new_name}" -ConstraintDrift = ConstraintModelDrift | ConstraintNameDrift | ConstraintRenameDrift +ConstraintDrift = ( + ConstraintModelDrift + | ConstraintPercentDrift + | ConstraintNameDrift + | ConstraintRenameDrift +) @dataclass @@ -810,6 +840,8 @@ def _compare_indexes( # Check if definition matches if db_idx.definition: + # A doubled % literal here is just a changed definition: an + # index is rebuilt without blocking, so nothing to single out. issue = _compare_normalized_index( cursor=cursor, model=model, @@ -1029,21 +1061,14 @@ def _compare_unique_constraints( cursor, model, constraint, actual[constraint.name], table ) elif actual_def := actual[constraint.name].definition: - expected_def = _get_expected_unique_definition(cursor, model, constraint) # Both sides are deparsed by pg_get_constraintdef → string equality. - if actual_def != expected_def: - if expected_def: - issue = f"definition differs: DB has {actual_def!r}, model expects {expected_def!r}" - else: - # Round-trip normalization couldn't complete; normalized - # model text is unavailable for the diagnostic. - issue = f"definition differs: DB has {actual_def!r}" - drift = ConstraintModelDrift( - table=table, - constraint=constraint, - model=model, - kind=DriftKind.CHANGED, - ) + issue, drift = _compare_constraint_definition( + table=table, + constraint=constraint, + model=model, + actual_def=actual_def, + expected_def=_get_expected_unique_definition(cursor, model, constraint), + ) statuses.append( ConstraintStatus( @@ -1132,21 +1157,14 @@ def _compare_check_constraints( kind=DriftKind.UNVALIDATED, ) elif actual_def := actual[constraint.name].definition: - expected_def = _get_expected_check_definition(cursor, model, constraint) # Both sides are deparsed by pg_get_constraintdef → string equality. - if actual_def != expected_def: - if expected_def: - issue = f"definition differs: DB has {actual_def!r}, model expects {expected_def!r}" - else: - # Round-trip normalization couldn't complete; normalized - # model text is unavailable for the diagnostic. - issue = f"definition differs: DB has {actual_def!r}" - drift = ConstraintModelDrift( - table=table, - constraint=constraint, - model=model, - kind=DriftKind.CHANGED, - ) + issue, drift = _compare_constraint_definition( + table=table, + constraint=constraint, + model=model, + actual_def=actual_def, + expected_def=_get_expected_check_definition(cursor, model, constraint), + ) statuses.append( ConstraintStatus( @@ -1608,24 +1626,59 @@ def _compare_index_only_unique( if not actual_def: return None, None - issue = _compare_normalized_index( - cursor=cursor, - model=model, + # Round-trip the model side so both are pg_get_indexdef bodies. + expected_tail = _normalize_index_def( + cursor, + model, expressions=constraint.expressions, fields_orders=[(f, "") for f in constraint.fields], opclasses=list(constraint.opclasses), condition=constraint.condition, include=constraint.include, - actual_def=actual_def, unique=True, ) - if issue: - changed = ConstraintModelDrift( - table=table, constraint=constraint, model=model, kind=DriftKind.CHANGED - ) - return issue, changed + return _compare_constraint_definition( + table=table, + constraint=constraint, + model=model, + actual_def=_index_def_tail(actual_def), + expected_def=expected_tail, + ) + + +def _compare_constraint_definition( + *, + table: str, + constraint: CheckConstraint | UniqueConstraint, + model: type[Model], + actual_def: str, + expected_def: str, +) -> tuple[str | None, ConstraintDrift | None]: + """Compare a constraint's stored definition against the model's. - return None, None + An empty `expected_def` means the model side couldn't be normalized, + which counts as a change. + """ + if actual_def == expected_def: + return None, None + if expected_def and actual_def.replace("%%", "%") == expected_def: + # The form an earlier plain-postgres stored; see ConstraintPercentDrift. + return ( + f"doubled % literal: DB has {actual_def!r}, model expects {expected_def!r}", + ConstraintPercentDrift(table=table, constraint=constraint, model=model), + ) + if expected_def: + issue = ( + f"definition differs: DB has {actual_def!r}, model expects {expected_def!r}" + ) + else: + # Round-trip normalization couldn't complete; normalized model text + # is unavailable for the diagnostic. + issue = f"definition differs: DB has {actual_def!r}" + changed = ConstraintModelDrift( + table=table, constraint=constraint, model=model, kind=DriftKind.CHANGED + ) + return issue, changed def _compare_normalized_index( @@ -1638,9 +1691,8 @@ def _compare_normalized_index( condition: Q | None, include: tuple[str, ...] | None, actual_def: str, - unique: bool = False, ) -> str | None: - """Compare a model index/constraint against pg_get_indexdef text. + """Compare a model index against pg_get_indexdef text. Round-trips the model side through Postgres so both sides come from pg_get_indexdef, then string-compares the normalized `USING ...` bodies. @@ -1655,7 +1707,6 @@ def _compare_normalized_index( opclasses=opclasses, condition=condition, include=include, - unique=unique, ) actual_tail = _index_def_tail(actual_def) if not expected_tail: diff --git a/plain-postgres/plain/postgres/convergence/corrections.py b/plain-postgres/plain/postgres/convergence/corrections.py index e465c4c2e0..2da47509d1 100644 --- a/plain-postgres/plain/postgres/convergence/corrections.py +++ b/plain-postgres/plain/postgres/convergence/corrections.py @@ -9,8 +9,10 @@ from ..constraints import BaseConstraint, CheckConstraint, UniqueConstraint from ..db import get_connection -from ..dialect import build_timeout_set_clauses, quote_name +from ..ddl import compile_expression_sql +from ..dialect import MAX_NAME_LENGTH, build_timeout_set_clauses, quote_name from ..indexes import Index +from ..utils import truncate_name if TYPE_CHECKING: from ..base import Model @@ -563,6 +565,83 @@ def apply(self) -> str: return sql +@dataclass +class ReplaceConstraintCorrection(Correction): + """Replace a constraint under its own name, from the model, for a + definition that only differs by a doubled `%` literal + (`ConstraintPercentDrift`). + + The table is never without the constraint. A check is dropped and + re-added in one statement, then validated. A unique is rebuilt as a new + index first, concurrently, and swapped in under the old name in one + transaction. + """ + + pass_order = 2 + + table: str + constraint: BaseConstraint + model: type[Model] + + def describe(self) -> str: + return f"{self.table}: replace constraint {self.constraint.name} (doubled % literal)" + + def apply(self) -> str: + if isinstance(self.constraint, CheckConstraint): + return self._replace_check(self.constraint) + if isinstance(self.constraint, UniqueConstraint): + return self._replace_unique(self.constraint) + raise TypeError(f"Cannot replace {self.constraint!r}") + + def _replace_check(self, constraint: CheckConstraint) -> str: + table = quote_name(self.table) + name = quote_name(constraint.name) + check = compile_expression_sql(self.model, constraint.check) + swap_sql = ( + f"ALTER TABLE {table} DROP CONSTRAINT {name}," + f" ADD CONSTRAINT {name} CHECK ({check}) NOT VALID" + ) + _execute_and_commit(swap_sql) + + validate_sql = _validate_constraint_sql(self.table, constraint.name) + _execute_and_commit(validate_sql, blocking=False) + return f"{swap_sql}; {validate_sql}" + + def _replace_unique(self, constraint: UniqueConstraint) -> str: + # Build the replacement under a temporary name without blocking + # writes, then drop the old one, take its name, and (for a + # constraint-backed unique) attach it, all in one transaction. + replacement = constraint.clone() + if not isinstance(replacement, UniqueConstraint): # clone keeps the class + raise TypeError(f"Expected a UniqueConstraint clone, got {replacement!r}") + replacement.name = truncate_name( + f"{constraint.name}_replacement", MAX_NAME_LENGTH + ) + create_sql = replacement.to_sql(self.model, concurrently=True) + _execute_autocommit(create_sql) + + old_name = quote_name(constraint.name) + if constraint.index_only: + drop_sql = f"DROP INDEX {old_name}" + else: + drop_sql = ( + f"ALTER TABLE {quote_name(self.table)} DROP CONSTRAINT {old_name}" + ) + rename_sql = f"ALTER INDEX {quote_name(replacement.name)} RENAME TO {old_name}" + swap = [drop_sql, rename_sql] + if not constraint.index_only: + swap.append(constraint.to_attach_sql(self.model)) + try: + _execute_and_commit(swap) + except Exception: + # The old constraint is untouched; clean up the orphaned index. + _execute_autocommit( + f"DROP INDEX CONCURRENTLY IF EXISTS {quote_name(replacement.name)}" + ) + raise + return "; ".join([create_sql, *swap]) + + @dataclass class DropConstraintCorrection(Correction): pass_order = 4 diff --git a/plain-postgres/plain/postgres/convergence/planning.py b/plain-postgres/plain/postgres/convergence/planning.py index f0526a2736..522a920b27 100644 --- a/plain-postgres/plain/postgres/convergence/planning.py +++ b/plain-postgres/plain/postgres/convergence/planning.py @@ -11,6 +11,7 @@ ColumnShouldBeNotNullDrift, ConstraintModelDrift, ConstraintNameDrift, + ConstraintPercentDrift, ConstraintRenameDrift, Drift, DriftKind, @@ -37,6 +38,7 @@ RebuildIndexCorrection, RenameConstraintCorrection, RenameIndexCorrection, + ReplaceConstraintCorrection, ReplaceForeignKeyCorrection, ResetStorageParameterCorrection, SetColumnDefaultCorrection, @@ -96,6 +98,8 @@ def _plan_drift(drift: Drift) -> PlanItem: " then remove the old one." ), ) + case ConstraintPercentDrift(table=t, constraint=c, model=m): + return PlanItem(drift, ReplaceConstraintCorrection(t, c, m)) case ConstraintNameDrift(kind=DriftKind.UNVALIDATED, table=t, name=n): return PlanItem(drift, ValidateConstraintCorrection(t, n)) case ConstraintNameDrift(kind=DriftKind.UNDECLARED, table=t, name=n): diff --git a/plain-postgres/plain/postgres/ddl.py b/plain-postgres/plain/postgres/ddl.py index 0d1ea6d11d..333df83cf6 100644 --- a/plain-postgres/plain/postgres/ddl.py +++ b/plain-postgres/plain/postgres/ddl.py @@ -20,13 +20,15 @@ def quote_value(value: Any) -> str: - """Quote a value for safe inclusion in a SQL string. + """Quote a value for inclusion in a SQL string that is executed as written. Not safe against injection from user code — intended only for SQL scripts, default values, and constraint expressions (which are not user-defined). + + A `%` is left alone. The DDL built from this runs with no parameters, so + psycopg never treats `%` as a placeholder; doubling it would store the + doubled literal in the constraint. """ - if isinstance(value, str): - value = value.replace("%", "%%") conn = get_connection() return psycopg.sql.quote(value, conn.connection) @@ -55,14 +57,9 @@ def compile_database_default_sql(expression: Any) -> str: def compile_literal_default_sql(field: Any) -> str: - """Compile a field's literal ``default=`` value as DDL-ready SQL. - - Result is executed directly (no ``%s`` interpolation), so we use - ``psycopg.sql.quote`` — not ``quote_value``, which doubles ``%``. - """ - conn = get_connection() - prepared = field.get_db_prep_save(field.default, conn) - return psycopg.sql.quote(prepared, conn.connection) + """Compile a field's literal ``default=`` value as DDL-ready SQL.""" + prepared = field.get_db_prep_save(field.default, get_connection()) + return quote_value(prepared) def compile_index_expressions_sql( diff --git a/plain-postgres/plain/postgres/describe.py b/plain-postgres/plain/postgres/describe.py new file mode 100644 index 0000000000..e52e507ae8 --- /dev/null +++ b/plain-postgres/plain/postgres/describe.py @@ -0,0 +1,367 @@ +"""Describe the data model as the registry sees it. + +`describe_models()` reads the model registry and returns one +`ModelDescription` per model: its fields, its relations with their target, +on-delete rule and nullability, the relations pointing back at it, and its +constraints and indexes with their conditions compiled to SQL. Nothing here +touches the database. + +This is what `plain postgres models` prints. It exists so that the facts a +reader can't reliably derive from a `models.py` by eye (a one-to-one that +lives in a unique constraint, a partial unique that only looks like one, a +reverse accessor's name) come from the framework instead. Views built on +these facts, such as a diagram, belong to whoever reads them. +""" + +from __future__ import annotations + +from collections.abc import Sequence +from dataclasses import asdict, dataclass +from typing import TYPE_CHECKING, Any + +from plain.packages import packages_registry + +from .constraints import CheckConstraint, UniqueConstraint +from .ddl import compile_expression_sql, compile_index_expressions_sql +from .exceptions import FieldError +from .fields.primary_key import PrimaryKeyField +from .fields.related import ForeignKeyField, RelatedField +from .fields.reverse_descriptors import BaseReverseDescriptor +from .fields.reverse_related import ManyToManyRel +from .registry import models_registry + +if TYPE_CHECKING: + from .base import Model + from .query_utils import Q + + +@dataclass +class FieldDescription: + name: str + type: str # The field class name, e.g. "TextField" + column: str + primary_key: bool + null: bool + # Effective uniqueness: true when an unconditional, single-field unique + # constraint covers this field. A partial unique never counts. + unique: bool + + +@dataclass +class RelationDescription: + """A forward relation: a foreign key or many-to-many on this model.""" + + name: str + type: str # "ForeignKeyField" or "ManyToManyField" + to: str # Target model label, e.g. "users.User" + on_delete: str | None # "CASCADE", "SET_NULL", "RESTRICT"; None for many-to-many + null: bool + unique: bool # Effective uniqueness, which makes a foreign key one-to-one + column: str | None # None for many-to-many + through: str | None # The intermediary model label, many-to-many only + query_name: str # The name used to filter from the target back to this model + + +@dataclass +class ReverseDescription: + """A relation on another model that points at this one.""" + + from_model: str # The pointing model's label + field: str # The field on the pointing model + type: str # "ForeignKeyField" or "ManyToManyField" + one_to_one: bool + query_name: str # The name used to filter this model by the pointing one + accessor: str | None # A declared ReverseForeignKey/ReverseManyToMany, if any + + def to_dict(self) -> dict[str, Any]: + # Written out rather than `asdict()`: the JSON key is "from", which + # can't be an attribute name. + return { + "from": self.from_model, + "field": self.field, + "type": self.type, + "one_to_one": self.one_to_one, + "query_name": self.query_name, + "accessor": self.accessor, + } + + +@dataclass +class ConstraintDescription: + name: str + kind: str # "unique" or "check" + fields: list[str] # Unique constraints only; empty for expression-based ones + expressions: list[str] # Unique constraints over expressions, as SQL + condition: str | None # Partial unique: the WHERE clause as SQL + check: str | None # Check constraints: the CHECK expression as SQL + + +@dataclass +class IndexDescription: + name: str + fields: list[str] + expressions: list[str] # Expression indexes, as SQL + condition: str | None + + +@dataclass +class ModelDescription: + label: str # "package.ModelName" + name: str + package: str + table: str + fields: list[FieldDescription] + relations: list[RelationDescription] + reverse: list[ReverseDescription] + constraints: list[ConstraintDescription] + indexes: list[IndexDescription] + + def to_dict(self) -> dict[str, Any]: + return { + "label": self.label, + "name": self.name, + "package": self.package, + "table": self.table, + "fields": [asdict(f) for f in self.fields], + "relations": [asdict(r) for r in self.relations], + "reverse": [r.to_dict() for r in self.reverse], + "constraints": [asdict(c) for c in self.constraints], + "indexes": [asdict(i) for i in self.indexes], + } + + +def select_models( + targets: tuple[str, ...], *, include_packages: bool +) -> list[type[Model]]: + """Pick the models to describe. + + Each target matches a model by its label (`changes.Thread`), its class + name (`Thread`), its table (`changes_thread`), or a whole package by + label (`changes`), none of them case-sensitive, and the models come back + in the order the targets name them. With no targets, every app model is + selected in registry order, plus the installed packages' models when + `include_packages` is set. + """ + all_models = models_registry.get_models() + + if not targets: + if include_packages: + return list(all_models) + app_package_labels = { + package_config.package_label + for package_config in packages_registry.get_package_configs() + if package_config.is_app + } + return [ + model + for model in all_models + if model.model_options.package_label in app_package_labels + ] + + selected: list[type[Model]] = [] + for target in targets: + target_lower = target.lower() + matches = [ + model + for model in all_models + if model.model_options.label_lower == target_lower + or model.__name__.lower() == target_lower + or model.model_options.db_table.lower() == target_lower + or model.model_options.package_label.lower() == target_lower + ] + if not matches: + raise LookupError(f"No model or package matches {target!r}") + for model in matches: + if model not in selected: + selected.append(model) + return selected + + +def describe_models( + targets: tuple[str, ...] = (), *, include_packages: bool = False +) -> list[ModelDescription]: + """Describe the selected models (see `select_models`), in that order.""" + return [ + describe_model(model) + for model in select_models(targets, include_packages=include_packages) + ] + + +def describe_model(model: type[Model]) -> ModelDescription: + options = model.model_options + meta = model._model_meta + unique_fields = options.unique_field_names + + fields: list[FieldDescription] = [] + relations: list[RelationDescription] = [] + for model_field in meta.fields: + if isinstance(model_field, ForeignKeyField): + relations.append( + RelationDescription( + name=model_field.name, + type="ForeignKeyField", + to=_label_of(model_field.remote_field.model_ref), + on_delete=model_field.remote_field.on_delete.name, + null=model_field.allow_null, + unique=model_field.name in unique_fields, + column=model_field.column, + through=None, + query_name=model_field.related_query_name(), + ) + ) + else: + fields.append( + FieldDescription( + name=model_field.name, + type=type(model_field).__name__, + column=model_field.column, + primary_key=isinstance(model_field, PrimaryKeyField), + null=model_field.allow_null, + unique=model_field.name in unique_fields, + ) + ) + + for m2m_field in meta.many_to_many: + relations.append( + RelationDescription( + name=m2m_field.name, + type="ManyToManyField", + to=_label_of(m2m_field.remote_field.model_ref), + on_delete=None, + null=False, + unique=False, + column=None, + through=_label_of(m2m_field.remote_field.through_ref), + query_name=m2m_field.related_query_name(), + ) + ) + + declared_accessors = _declared_reverse_accessors(model) + reverse: list[ReverseDescription] = [] + for rel in meta.related_objects: + pointing_field = rel.field + pointing_model = pointing_field.model + is_many_to_many = isinstance(rel, ManyToManyRel) + reverse.append( + ReverseDescription( + from_model=pointing_model.model_options.label, + field=pointing_field.name, + type="ManyToManyField" if is_many_to_many else "ForeignKeyField", + one_to_one=( + not is_many_to_many + and pointing_field.name + in pointing_model.model_options.unique_field_names + ), + query_name=rel.name, + accessor=declared_accessors.get(pointing_field), + ) + ) + + constraints: list[ConstraintDescription] = [] + for constraint in options.constraints: + if isinstance(constraint, UniqueConstraint): + constraints.append( + ConstraintDescription( + name=constraint.name, + kind="unique", + fields=list(constraint.fields), + expressions=_compile_expressions( + model=model, expressions=constraint.expressions + ), + condition=_compile_condition( + model=model, condition=constraint.condition + ), + check=None, + ) + ) + elif isinstance(constraint, CheckConstraint): + constraints.append( + ConstraintDescription( + name=constraint.name, + kind="check", + fields=[], + expressions=[], + condition=None, + check=_compile_q(model=model, q=constraint.check), + ) + ) + + indexes = [ + IndexDescription( + name=index.name, + fields=list(index.fields), + expressions=_compile_expressions( + model=model, expressions=index.expressions + ), + condition=_compile_condition(model=model, condition=index.condition), + ) + for index in options.indexes + ] + + return ModelDescription( + label=options.label, + name=options.object_name, + package=options.package_label, + table=options.db_table, + fields=fields, + relations=relations, + reverse=reverse, + constraints=constraints, + indexes=indexes, + ) + + +def _label_of(model_ref: str | type[Model]) -> str: + """A model's label, or the reference as written when it never resolved. + + A foreign key to a model that doesn't exist (a typo, a package no longer + installed) keeps its string reference; the description shows it rather + than failing for every model. + """ + if isinstance(model_ref, str): + return model_ref + return model_ref.model_options.label + + +def _compile_condition(*, model: type[Model], condition: Q | None) -> str | None: + """A partial constraint or index condition as SQL, or None when there is none.""" + if condition is None: + return None + return _compile_q(model=model, q=condition) + + +def _compile_q(*, model: type[Model], q: Q) -> str: + """The expression as SQL, or as written when it can't compile: a + condition reaching through a relation that never resolved would otherwise + fail the whole listing.""" + try: + return compile_expression_sql(model, q) + except FieldError, TypeError: + return repr(q) + + +def _compile_expressions( + *, model: type[Model], expressions: Sequence[Any] +) -> list[str]: + """Each index or constraint expression as SQL, parenthesized as in its DDL.""" + return [ + compile_index_expressions_sql(model, (expression,)) + for expression in expressions + ] + + +def _declared_reverse_accessors(model: type[Model]) -> dict[RelatedField, str]: + """Map each pointing field to the ReverseForeignKey/ReverseManyToMany + attribute declared on `model` for it. + + Every live descriptor sits in the model's own namespace: model setup + copies inherited ones onto the model before contributing them. + """ + accessors: dict[RelatedField, str] = {} + for value in vars(model).values(): + if not isinstance(value, BaseReverseDescriptor): + continue + if value.name is None or value.reversed_field is None: + continue + accessors[value.reversed_field] = value.name + return accessors diff --git a/plain-postgres/plain/postgres/fields/reverse_descriptors.py b/plain-postgres/plain/postgres/fields/reverse_descriptors.py index 60d7371344..d900a8d699 100644 --- a/plain-postgres/plain/postgres/fields/reverse_descriptors.py +++ b/plain-postgres/plain/postgres/fields/reverse_descriptors.py @@ -36,6 +36,12 @@ def __init__(self, to: str | type[T], field: str): self._resolved_model: type[T] | None = None self._resolved_field: Any = None + @property + def reversed_field(self) -> Any: + """The field on the target model this accessor reverses, or None until + the target model has resolved.""" + return self._resolved_field + def contribute_to_class(self, cls: type[Model], name: str) -> None: """ Register this reverse relation with the model class. diff --git a/plain-postgres/plain/postgres/introspection/health/ownership.py b/plain-postgres/plain/postgres/introspection/health/ownership.py index de6172f368..5fcc420b12 100644 --- a/plain-postgres/plain/postgres/introspection/health/ownership.py +++ b/plain-postgres/plain/postgres/introspection/health/ownership.py @@ -17,7 +17,7 @@ def _source_file(cls: type) -> str: owners: dict[str, TableOwner] = {} for package_config in packages_registry.get_package_configs(): - source = "app" if package_config.name.startswith("app.") else "package" + source = "app" if package_config.is_app else "package" for model in models_registry.get_models( package_label=package_config.package_label ): diff --git a/plain-postgres/plain/postgres/options.py b/plain-postgres/plain/postgres/options.py index 88fdd3d39a..fea7e18670 100644 --- a/plain-postgres/plain/postgres/options.py +++ b/plain-postgres/plain/postgres/options.py @@ -205,6 +205,17 @@ def total_unique_constraints(self) -> list[Any]: ) ] + @property + def unique_field_names(self) -> set[str]: + """Field names each made unique on their own by an unconditional unique + constraint. A partial or expression-based constraint never counts, so + this is the set a foreign key must be in to be one-to-one.""" + return { + constraint.fields[0] + for constraint in self.total_unique_constraints + if len(constraint.fields) == 1 + } + def unique_fields_match_constraint(self, field_names: set[str | None]) -> bool: """True if field_names names the primary key, or a UniqueConstraint on the model that has no condition and no expressions.""" diff --git a/plain-postgres/plain/postgres/preflight/indexes.py b/plain-postgres/plain/postgres/preflight/indexes.py index f74f39f022..6ad03ceea6 100644 --- a/plain-postgres/plain/postgres/preflight/indexes.py +++ b/plain-postgres/plain/postgres/preflight/indexes.py @@ -15,7 +15,7 @@ def _get_app_models() -> list[Any]: """Return models from the user's app packages only (not framework/third-party).""" app_models = [] for package_config in packages_registry.get_package_configs(): - if package_config.name.startswith("app."): + if package_config.is_app: app_models.extend( models_registry.get_models(package_label=package_config.package_label) ) diff --git a/plain-postgres/tests/app/examples/migrations/0029_describedperson_describedprofile_describedslot.py b/plain-postgres/tests/app/examples/migrations/0029_describedperson_describedprofile_describedslot.py new file mode 100644 index 0000000000..6d810054c9 --- /dev/null +++ b/plain-postgres/tests/app/examples/migrations/0029_describedperson_describedprofile_describedslot.py @@ -0,0 +1,46 @@ +# Generated by Plain 0.166.0 on 2026-10-08 17:03 + +from plain.postgres import migrations + +from plain import postgres + + +class Migration(migrations.Migration): + dependencies = (("examples", "0028_aliascollisionexample"),) + + operations = ( + migrations.CreateModel( + name="DescribedPerson", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("name", postgres.TextField(max_length=100)), + ], + ), + migrations.CreateModel( + name="DescribedProfile", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("bio", postgres.TextField(default="", required=False)), + ( + "person", + postgres.ForeignKeyField( + on_delete=postgres.CASCADE, to="examples.describedperson" + ), + ), + ], + ), + migrations.CreateModel( + name="DescribedSlot", + fields=[ + ("id", postgres.PrimaryKeyField()), + ("active", postgres.BooleanField(default=False)), + ("position", postgres.IntegerField(default=0)), + ( + "owner", + postgres.ForeignKeyField( + on_delete=postgres.CASCADE, to="examples.describedperson" + ), + ), + ], + ), + ) diff --git a/plain-postgres/tests/app/examples/models/__init__.py b/plain-postgres/tests/app/examples/models/__init__.py index e8054cbf9c..836bf173a4 100644 --- a/plain-postgres/tests/app/examples/models/__init__.py +++ b/plain-postgres/tests/app/examples/models/__init__.py @@ -4,6 +4,7 @@ constraints, defaults, delete, + describe, encrypted, forms, indexes, diff --git a/plain-postgres/tests/app/examples/models/describe.py b/plain-postgres/tests/app/examples/models/describe.py new file mode 100644 index 0000000000..0b85a31bc2 --- /dev/null +++ b/plain-postgres/tests/app/examples/models/describe.py @@ -0,0 +1,90 @@ +"""Models for `plain postgres models` (`plain.postgres.describe`). + +They stage the facts a reader can't get from a field definition alone: a +one-to-one that lives in a unique constraint, a partial unique that must not +be mistaken for one, a check constraint, and a declared reverse accessor. +""" + +from typing import ClassVar + +from plain.postgres import Field, types +from plain.postgres.functions import Lower +from plain.postgres.query_utils import Q + +from plain import postgres + + +@postgres.register_model +class DescribedPerson(postgres.Model): + name: Field[str] = types.TextField(max_length=100) + profiles: ClassVar[types.ReverseForeignKey[DescribedProfile]] = ( + types.ReverseForeignKey(to="DescribedProfile", field="person") + ) + + model_options = postgres.Options( + constraints=[ + # Unique over an expression: not a field, so `name` stays + # unmarked and the constraint carries the SQL. + postgres.UniqueConstraint( + Lower("name"), name="describedperson_name_lower_unique" + ), + # A literal with a `%` in it: shown as Postgres sees it. + postgres.CheckConstraint( + check=~Q(name="100%"), name="describedperson_name_not_100pct_check" + ), + # The same, in a partial unique's condition: lives as an index. + postgres.UniqueConstraint( + fields=["name"], + condition=~Q(name__endswith="%"), + name="describedperson_name_unique_unless_pct", + ), + ], + indexes=[ + postgres.Index(Lower("name"), name="describedperson_name_lower_idx"), + ], + ) + + +@postgres.register_model +class DescribedProfile(postgres.Model): + """One per person, by constraint rather than by field.""" + + person: Field[DescribedPerson] = types.ForeignKeyField( + DescribedPerson, on_delete=postgres.CASCADE + ) + bio: Field[str] = types.TextField(required=False, default="") + + model_options = postgres.Options( + constraints=[ + postgres.UniqueConstraint( + fields=["person"], name="describedprofile_person_unique" + ), + ] + ) + + +@postgres.register_model +class DescribedSlot(postgres.Model): + """At most one active slot per owner: a partial unique, not a one-to-one.""" + + owner: Field[DescribedPerson] = types.ForeignKeyField( + DescribedPerson, on_delete=postgres.CASCADE + ) + active: Field[bool] = types.BooleanField(default=False) + position: Field[int] = types.IntegerField(default=0) + + model_options = postgres.Options( + constraints=[ + postgres.UniqueConstraint( + fields=["owner"], + condition=Q(active=True), + name="describedslot_one_active_per_owner", + ), + postgres.CheckConstraint( + check=Q(position__gte=0), name="describedslot_position_check" + ), + ], + indexes=[ + postgres.Index(fields=["owner"], name="describedslot_owner_idx"), + ], + ) diff --git a/plain-postgres/tests/internal/test_convergence_percent_literal.py b/plain-postgres/tests/internal/test_convergence_percent_literal.py new file mode 100644 index 0000000000..250aac3314 --- /dev/null +++ b/plain-postgres/tests/internal/test_convergence_percent_literal.py @@ -0,0 +1,126 @@ +"""Constraints created by an earlier plain-postgres hold doubled `%` literals. + +`quote_value` used to double `%` for a parameter substitution the DDL never +went through, so a model's `'100%'` was stored as `'100%%'`. Now that the +literal is written as is, convergence meets the old definition in every +existing database. It must replace it under the same name, not block sync +the way a real definition change does. +""" + +import psycopg.sql +from app.examples.models.describe import DescribedPerson +from convergence_helpers import constraint_is_valid, execute, index_exists +from plain.postgres import ddl, get_connection +from plain.postgres.convergence import execute_plan, plan_model_convergence +from plain.postgres.convergence.analysis import ( + ConstraintModelDrift, + ConstraintPercentDrift, +) +from plain.postgres.convergence.corrections import ReplaceConstraintCorrection +from plain.postgres.testing import isolated_db +from plain.testing import patch + +TABLE = "examples_describedperson" +CHECK = "describedperson_name_not_100pct_check" +PARTIAL_UNIQUE = "describedperson_name_unique_unless_pct" + + +def _quote_value_doubling_percent(value: object) -> str: + """`quote_value` as it was.""" + if isinstance(value, str): + value = value.replace("%", "%%") + return psycopg.sql.quote(value, get_connection().connection) + + +def _declared(name: str): + return next(c for c in DescribedPerson.model_options.constraints if c.name == name) + + +def _stage_old_definition(name: str, *, index_only: bool) -> None: + """Swap the live constraint for the one the old quoting produced.""" + if index_only: + execute(f'DROP INDEX "{name}"') + else: + execute(f'ALTER TABLE "{TABLE}" DROP CONSTRAINT "{name}"') + with patch(ddl, "quote_value", _quote_value_doubling_percent): + old_sql = _declared(name).to_sql(DescribedPerson) + execute(old_sql) + + +def _plan(): + conn = get_connection() + with conn.cursor() as cursor: + return plan_model_convergence(conn, cursor, DescribedPerson) + + +def _item_for(plan, name: str): + return next( + item + for item in plan.items + if getattr(item.drift, "constraint", None) is not None + and item.drift.constraint.name == name + ) + + +def _check_definition() -> str: + with get_connection().cursor() as cursor: + cursor.execute( + "SELECT pg_get_constraintdef(oid) FROM pg_constraint WHERE conname = %s", + [CHECK], + ) + row = cursor.fetchone() + assert row is not None + return row[0] + + +@isolated_db +def test_doubled_percent_in_a_check_is_replaced_not_blocked(): + _stage_old_definition(CHECK, index_only=False) + assert "'100%%'" in _check_definition() + + item = _item_for(_plan(), CHECK) + assert isinstance(item.drift, ConstraintPercentDrift) + assert isinstance(item.correction, ReplaceConstraintCorrection) + assert item.blocks_sync is True # a failed replacement must stop sync + + result = execute_plan([item]) + assert all(r.ok for r in result.results) + assert "'100%'" in _check_definition() + assert constraint_is_valid(TABLE, CHECK) + assert not any( + getattr(i.drift, "constraint", None) is not None + and i.drift.constraint.name == CHECK + for i in _plan().items + ) + + +@isolated_db +def test_doubled_percent_in_a_partial_unique_is_replaced_not_blocked(): + _stage_old_definition(PARTIAL_UNIQUE, index_only=True) + + item = _item_for(_plan(), PARTIAL_UNIQUE) + assert isinstance(item.drift, ConstraintPercentDrift) + assert isinstance(item.correction, ReplaceConstraintCorrection) + assert item.blocks_sync is True # a failed replacement must stop sync + + result = execute_plan([item]) + assert all(r.ok for r in result.results) + assert index_exists(PARTIAL_UNIQUE) + assert not any( + getattr(i.drift, "constraint", None) is not None + and i.drift.constraint.name == PARTIAL_UNIQUE + for i in _plan().items + ) + + +@isolated_db +def test_a_genuinely_changed_check_still_blocks(): + execute(f'ALTER TABLE "{TABLE}" DROP CONSTRAINT "{CHECK}"') + execute( + f'ALTER TABLE "{TABLE}" ADD CONSTRAINT "{CHECK}" CHECK (NOT ("name" = \'200%\'))' + ) + + item = _item_for(_plan(), CHECK) + assert isinstance(item.drift, ConstraintModelDrift) + assert item.correction is None + assert item.blocks_sync is True diff --git a/plain-postgres/tests/public/test_check_constraint_literals.py b/plain-postgres/tests/public/test_check_constraint_literals.py new file mode 100644 index 0000000000..99b1d69328 --- /dev/null +++ b/plain-postgres/tests/public/test_check_constraint_literals.py @@ -0,0 +1,17 @@ +"""A check constraint's literal is enforced as written. + +DDL built from a model runs with no parameters, so a `%` in a literal is a +`%`. It used to be doubled on the way out, which stored `'100%%'` in the +constraint and let the one value the constraint names through. +""" + +from app.examples.models.describe import DescribedPerson +from plain.exceptions import ValidationError +from plain.testing import raises + + +def test_a_percent_in_a_check_literal_is_enforced_as_written(): + DescribedPerson.query.create(name="100%%") + + with raises(ValidationError): + DescribedPerson.query.create(name="100%") diff --git a/plain-postgres/tests/public/test_postgres_models_cli.py b/plain-postgres/tests/public/test_postgres_models_cli.py new file mode 100644 index 0000000000..915dff9def --- /dev/null +++ b/plain-postgres/tests/public/test_postgres_models_cli.py @@ -0,0 +1,218 @@ +"""`plain postgres models` -- the data model as the registry sees it. + +The contract is the facts a reader can't get from a field definition alone: +effective uniqueness (a one-to-one that lives in a unique constraint, a +partial unique that must not be mistaken for one), on-delete rules, the +relations pointing back at a model, and constraint conditions as SQL. None +of it needs a database. +""" + +import json + +from click.testing import CliRunner +from plain.postgres.cli.models import models as models_cli +from plain.postgres.describe import describe_models + + +def _run(*args: str) -> str: + result = CliRunner().invoke(models_cli, list(args)) + assert result.exit_code == 0, result.output + return result.output + + +def _describe(label: str) -> dict: + output = json.loads(_run(label, "--json")) + (description,) = output["models"] + return description + + +# Effective uniqueness + + +def test_foreign_key_covered_by_unique_constraint_is_one_to_one(): + profile = _describe("DescribedProfile") + (person,) = profile["relations"] + assert person["to"] == "examples.DescribedPerson" + assert person["on_delete"] == "CASCADE" + assert person["unique"] is True + + person_model = _describe("DescribedPerson") + reverse = {r["from"]: r for r in person_model["reverse"]} + assert reverse["examples.DescribedProfile"]["one_to_one"] is True + assert reverse["examples.DescribedProfile"]["accessor"] == "profiles" + + +def test_partial_unique_is_not_promoted_to_uniqueness(): + slot = _describe("DescribedSlot") + (owner,) = slot["relations"] + assert owner["unique"] is False + + constraints = {c["name"]: c for c in slot["constraints"]} + partial = constraints["describedslot_one_active_per_owner"] + assert partial["kind"] == "unique" + assert partial["fields"] == ["owner"] + assert partial["condition"] == '"active"' + + person = _describe("DescribedPerson") + reverse = {r["from"]: r for r in person["reverse"]} + assert reverse["examples.DescribedSlot"]["one_to_one"] is False + assert reverse["examples.DescribedSlot"]["accessor"] is None + + +def test_single_field_unique_constraint_marks_the_field(): + tag = _describe("Tag") + fields = {f["name"]: f for f in tag["fields"]} + assert fields["name"]["unique"] is True + assert fields["id"]["primary_key"] is True + + +# Constraints and indexes as SQL + + +def test_expression_unique_renders_as_sql_and_marks_no_field(): + person = _describe("DescribedPerson") + fields = {f["name"]: f for f in person["fields"]} + assert fields["name"]["unique"] is False + + constraints = {c["name"]: c for c in person["constraints"]} + unique = constraints["describedperson_name_lower_unique"] + assert unique["fields"] == [] + assert unique["expressions"] == ['(LOWER("name"))'] + + +def test_expression_index_renders_as_sql(): + person = _describe("DescribedPerson") + (index,) = person["indexes"] + assert index == { + "name": "describedperson_name_lower_idx", + "fields": [], + "expressions": ['(LOWER("name"))'], + "condition": None, + } + + # Written as the DDL writes it, parentheses included. + assert 'describedperson_name_lower_idx ((LOWER("name")))' in _run( + "DescribedPerson" + ) + + +def test_literals_are_shown_as_postgres_sees_them(): + # A `%` in a literal is a `%`, in the description as in the constraint. + person = _describe("DescribedPerson") + constraints = {c["name"]: c for c in person["constraints"]} + check = constraints["describedperson_name_not_100pct_check"]["check"] + assert "'100%'" in check + assert "%%" not in check + + +def test_check_constraint_and_index_render_as_sql(): + slot = _describe("DescribedSlot") + constraints = {c["name"]: c for c in slot["constraints"]} + assert constraints["describedslot_position_check"]["kind"] == "check" + assert constraints["describedslot_position_check"]["check"] == '"position" >= 0' + + (index,) = slot["indexes"] + assert index == { + "name": "describedslot_owner_idx", + "fields": ["owner"], + "expressions": [], + "condition": None, + } + + +# Relations + + +def test_on_delete_and_nullability_per_relation(): + expected = { + "ChildCascade": ("CASCADE", False), + "ChildRestrict": ("RESTRICT", False), + "ChildSetNull": ("SET_NULL", True), + } + for name, (on_delete, null) in expected.items(): + (parent,) = _describe(name)["relations"] + assert (parent["on_delete"], parent["null"]) == (on_delete, null) + + +def test_many_to_many_names_its_through_model(): + widget = _describe("Widget") + (tags,) = widget["relations"] + assert tags["type"] == "ManyToManyField" + assert tags["to"] == "examples.Tag" + assert tags["through"] == "examples.WidgetTag" + assert tags["on_delete"] is None + + tag = _describe("Tag") + reverse = {r["from"]: r for r in tag["reverse"]} + assert reverse["examples.Widget"]["type"] == "ManyToManyField" + assert reverse["examples.Widget"]["accessor"] == "widgets" + + +# Selection + + +def test_targets_select_by_model_name_label_or_package(): + by_name = json.loads(_run("Tag", "--json"))["models"] + by_label = json.loads(_run("examples.Tag", "--json"))["models"] + assert [m["label"] for m in by_name] == ["examples.Tag"] + assert by_name == by_label + + by_package = json.loads(_run("examples", "--json"))["models"] + labels = {m["label"] for m in by_package} + assert {"examples.Tag", "examples.Widget", "examples.DescribedSlot"} <= labels + + +def test_default_selection_is_app_models_only(): + default = {m["label"] for m in json.loads(_run("--json"))["models"]} + everything = {m["label"] for m in json.loads(_run("--json", "--all"))["models"]} + assert default + assert all(label.startswith("examples.") for label in default) + # The test app installs no package that brings models of its own, so + # --all can only add to the default selection, never take from it. + assert default <= everything + + +def test_targets_are_matched_without_regard_to_case_and_kept_in_order(): + output = json.loads(_run("describedslot", "EXAMPLES.DescribedPerson", "--json")) + assert [m["label"] for m in output["models"]] == [ + "examples.DescribedSlot", + "examples.DescribedPerson", + ] + + +def test_all_with_targets_is_a_usage_error(): + all_with_targets = CliRunner().invoke(models_cli, ["--all", "Tag"]) + assert all_with_targets.exit_code == 2 + assert "--all" in all_with_targets.output + + +def test_targets_match_by_table_name_too(): + (by_table,) = json.loads(_run("examples_tag", "--json"))["models"] + assert by_table["label"] == "examples.Tag" + + +def test_unknown_target_is_an_error(): + result = CliRunner().invoke(models_cli, ["NoSuchModel"]) + assert result.exit_code != 0 + assert "NoSuchModel" in result.output + + +# Text output + + +def test_text_output_shows_relations_and_constraints(): + output = _run("DescribedSlot") + assert "examples.DescribedSlot" in output + assert "→ examples.DescribedPerson" in output + assert "CASCADE" in output + assert "describedslot_one_active_per_owner" in output + assert 'WHERE "active"' in output + assert "describedslot_position_check" in output + + +# Python API + + +def test_describe_models_matches_the_json_output(): + (description,) = describe_models(("DescribedProfile",)) + assert description.to_dict() == _describe("DescribedProfile") diff --git a/plain/plain/packages/config.py b/plain/plain/packages/config.py index 5ef3bfa7ab..3d54512ac9 100644 --- a/plain/plain/packages/config.py +++ b/plain/plain/packages/config.py @@ -38,6 +38,12 @@ def __init__(self, name: str): def __repr__(self) -> str: return f"<{self.__class__.__name__}: {self.package_label}>" + @property + def is_app(self) -> bool: + """Whether this is one of the project's own packages, under `app`, + rather than an installed one.""" + return self.name.startswith("app.") + @cached_property def path(self) -> str: # Filesystem path to the application directory e.g.