[1a] Typed model construction via @dataclass_transform - #83
Conversation
@dataclass_transform on ModelBase synthesizes a type-checked __init__ from each model's Field[T]-annotated declarations, so Model(field=value) flags wrong value types, unknown field names, and missing required fields. Field is now exported from plain.postgres as the public annotation. DB-owned fields (id, create_now/update_now, generate=True, RandomStringField) are excluded via init=False; nullable column-backed fields accept default=None so they read as optional. PasswordField ships a Field[str] stub like the core fields.
Every shipped model and example-app model now annotates its fields with Field[T] (custom querysets use ClassVar so they aren't treated as fields). Cache.set_many constructs items without created_at -- now a DB-owned create_now field -- and stamps it from the shared now after construction.
Rewrite the postgres rule and package READMEs to teach Field[T] annotations: value type per field, nullable as Field[T | None] with default=None, DB-owned fields auto-excluded, and custom querysets via query: ClassVar[...].
Annotate every test-app model with Field[T] and update the postgres characterization tests for the typed constructor, including the init=False / hand-set-pk edge cases.
@dataclass_transform on ModelBase synthesized constructor params for annotated non-field attributes, so the type checker accepted Model(that=...) that the runtime rejects. A field-membership conformance check found four leak classes: model_options and _model_meta (now ClassVar), ManyToManyField (now init=False in the stub), and reverse-relation descriptors (now ClassVar; init=False is not honored for class specifiers, so ClassVar is the fix). The new postgres.field_leaks_into_constructor preflight re-derives the synthesized field set and flags any non-field that leaks in -- catching the class at startup, including in user models.
- Demote CheckTypedConstruction from a registered preflight check to a test-only guard over Plain's own models; detecting constructor leaks from raw annotations is too fragile to ship into every user app's startup - Let nullable forward-ref/self-ref FKs be optional in the constructor by adding default=None to the string-ref ForeignKeyField overload; apply it to TreeNode.parent and CircA.partner - Move admin's User import under TYPE_CHECKING so plain.admin.models doesn't hard-import the app's modules at runtime - Give DateTimeField the near-now fixed-default warning DateField/TimeField already have, now that it accepts literal defaults - Add default=None to nullable fields that were missing it (JobResult.retry_job_request_uuid, the encrypted config fixture) - Delegate ForeignKeyField default validation to ColumnField; drop dead task_id/tag_id annotations - Add a drift guard asserting field_specifiers matches the exported fields, and clarify in docs that default= (not nullability) makes a field optional
Textual conflicts (4): - fields/related.py — master dropped BLANK_CHOICE_DASH/limit_choices_to; keep the branch's NOT_PROVIDED import and its default=None rationale, minus the dead limit_choices_to reference - tests/app/examples/models/delete.py — master removed db_constraint, so UnconstrainedChild goes with it - plain-admin/tests/app/users/models.py — take master's username_upper property alongside the Field[T] annotations; drop the query redeclaration - preflight.py — master split it into a package, so CheckTypedConstruction moves into preflight/models.py and the internal test's import follows Semantic integration: plain-oauthserver landed on master after this branch was cut, so its models were never annotated — under the metaclass transform an unannotated model synthesizes an empty __init__ and every kwarg errors (31 diagnostics). Migrated its 4 models plus the test-app User to Field[T], dropped 5 query redeclarations, and adapted two call sites the typed constructor newly rejects: - views.py passed user=self.user into a non-null FK, but AuthView.user is `User | None` and login_required=True doesn't narrow it — a latent hole the untyped **kwargs constructor hid entirely. Asserted locally; typing login_required views is its own change. - test_units.py built partial AuthorizationCode instances to exercise pure methods, which the typed constructor rejects. Added an unsaved_code() helper that supplies the four required-but-irrelevant values. Whole-repo ty check clean; full suite 1878 passed.
|
Next steps:
|
There was a problem hiding this comment.
Code review
No issues found. Checked for bugs and CLAUDE.md compliance.
Reviewed the engine carefully (base.py @dataclass_transform wiring, types.pyi init=False/default= overloads, fields/base.py + fields/related.py + fields/temporal.py, CheckTypedConstruction and its conformance test) and sampled the ~300 mechanical annotations.
Points specifically verified:
DateTimeField:ColumnField→DefaultableFieldis behavior-preserving."default"is inDefaultableField.non_migration_attrsand is stripped by both the autodetector andschema.py, so the new deconstructeddefault=Noneproduces no migration drift;has_persistent_literal_default()is False forNone, so noDEFAULTclause is emitted.ColumnField.__init__default=gate correctly reads the localallow_nullparam (notself.allow_null, unset at that point) and deliberately doesn't storeself.default— nothing readsfield.defaultoff a plainColumnField.- Dropping the 55 per-model
query = QuerySet()redeclarations is runtime-safe:QuerySet.__get__binds viafrom_model(owner), so the inheritedModel.queryresolves to the concrete subclass. plain-cacheset_many: post-constructionitem.created_at = nowis equivalent to the old constructor kwarg — same attribute, same sharednow,created_atstill out ofupdate_fields.- Import-time coupling from dereferencing all 24
types.*constructors atModelBasecreation: no module reachable fromtypes.pyimportsplain.postgres.baseat module scope, so no partially-initialized cycle. - Rule sync:
plain-postgres/plain/postgres/agents/.claude/rules/plain-postgres.mdand the top-level.claude/rules/plain-postgres.mdare byte-identical, peragent-guidance.md. - Tests layout:
test_typed_construction_preflight.pycorrectly lives intests/internal/— it pins__dataclass_transform__and an unregistered preflight check.
Non-blocking, and ./scripts/fix handles it: in plain-postgres/plain/postgres/__init__.py, Field is inserted between BinaryField and BooleanField in the from .fields import (...) block, which is out of isort order.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9dd1bc4933
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| query: postgres.QuerySet[User] = postgres.QuerySet() | ||
| email: Field[str] = types.EmailField() | ||
| password: Field[str] = PasswordField() |
There was a problem hiding this comment.
Preserve requiredness for custom model fields
When a model uses PasswordField (or any other custom Field subclass), this assignment is not recognized as a field specifier because ModelBase.field_specifiers contains only the exact core constructors. The type checker therefore treats the assigned descriptor as a normal default and accepts User(email="...") even though password is required at runtime; this defeats the missing-field check for the password model updated here. Custom field constructors need to participate in the transform rather than silently becoming optional.
Useful? React with 👍 / 👎.
| @dataclass_transform( | ||
| kw_only_default=True, | ||
| field_specifiers=( |
There was a problem hiding this comment.
Include fields inherited from supported mixins
When a regular Python model mixin declares a constructor field, this transform does not synthesize parameters from it because the mixin itself is not a transformed base class. For example, class Common: tenant: Field[str] = types.TextField() followed by class Item(Common, postgres.Model) makes the checker reject the runtime-valid Item(tenant="t", ...) as an unknown argument, even though Meta._create_and_cache() explicitly traverses the full MRO and registers the mixin field. Typed construction therefore breaks the documented field-sharing mechanism for any non-DB-owned mixin field.
Useful? React with 👍 / 👎.
Gives
Model(field=…)a per-field typed__init__— wrong value types, unknown field names, and missing required fields become type errors instead of runtime surprises. Django can't do this (Model(**kwargs)isAny, and django-stubs can't type it per-field), so it's a genuine improvement over the framework Plain forked.Mechanism
PEP 681
@dataclass_transformon theModelBasemetaclass, listing thetypes.*constructors asfield_specifiers. Purely a type-checker affordance — the runtime__init__is untouched, and the full suite passes unchanged.The catch is that PEP 681 only sees annotated class attributes, so this reverses the current "don't annotate fields" rule:
Access typing is unchanged:
Article.titleis stillField[str](the class-level reference the typed query API will build on),article.titleis stillstr.The one rule to learn
A constructor param is required unless the field has a
default=or is DB-owned.default=id,create_now/update_now,generate=True,RandomStringField— via signature-levelinit=Falseoverloads in the stubsNullability folds into this with no special case: a nullable field is optional iff it declares
default=None, exactly as a string is optional iff it declaresdefault="".What's in here
@dataclass_transformonModelBase,Fieldexposed publicly asfrom plain.postgres import Fieldinit=Falseoverloads for DB-owned fields;DateTimeFieldand nullableForeignKeyFieldacceptdefault=None_ForeignKeyDescriptorsubclassesField[V]so FK annotations check on pyright too, not just tyModel.queryis aClassVar— default-queryset models declare nothing and inherit it; only custom-queryset models (cache, jobs) writequery: ClassVar[CustomQuerySet]queryredeclarations removedtest_typed_construction_preflight.py) that re-derives the synthesized field set and asserts each entry is a real field — it caught four leak classes during review (model_options/_model_meta, M2M, reverse FK/M2M accessors), two of which a human reviewer missed. Deliberately a test, not a registered preflight: detecting leaks from raw annotations is too fragile to run in every user app's startup.Known wart
~20 non-nullable
required=Falsefields type as required even though the runtime would supply"". Fixing that narrowly means addingdefault="", which — becausedefault=currently does double duty as the persistent DBDEFAULT— drags in a redundant 20-column migration driven purely by the type checker. Not worth churning schema for typing; the accurate fix is to decouple those two meanings, which is a separate change.The imprecision errs in the safe direction: you're asked to provide a value or declare a default, never allowed to omit something that would break.
Migration
All-or-nothing by construction — an unannotated model synthesizes an empty
__init__, so a half-migrated model silently gets a wrong constructor. The annotation is mechanically derivable from the field call, so/plain-upgradecan do it; the changelog needs upgrade instructions covering the annotation, thedefault=Nonefor nullables, droppingqueryredeclarations, andClassVarfor reverse accessors.This branch was cut in June and merged forward here. The merge is a useful preview of that all-or-nothing property:
plain-oauthserverlanded on master afterward, was never annotated, and produced 31 errors on its own until migrated. Two of its call sites needed real adaptation:views.pypasseduser=self.userinto a non-null FK, butAuthView.userisUser | Noneandlogin_required = Truedoesn't narrow it — a latent hole the untyped constructor hid entirely. Asserted locally; typinglogin_requiredviews properly is its own change.AuthorizationCodeinstances to exercise pure methods, which the typed constructor rejects. Needed a helper supplying four required-but-irrelevant values. This is the deliberate tradeoff — the type models the persistence contract while the runtime stays permissive for construct-then-fill — but it's the friction users will feel most.Review shape
Tiered: the engine (
base.py,types.pyi,fields/related.py, the conformance test) carefully; the ~300 mechanical annotations sampled.Verification
uv run ty check— clean across the whole repo./scripts/test— 28 suites, 1878 passed, 0 failedOne gap worth knowing: with
@dataclass_transformon, ty performs no assignability check between the annotation and the field, so a wrongField[T](title: Field[int] = types.TextField()) passes silently. Pyright catches it. Since the migration is ~300 generated annotations, the upgrade path should either gate on pyright or grow a third conformance check comparing each annotation'sTto the field's value type.