Support foreign keys in forms and reserve relation names#1135
Draft
audreyfeldroy wants to merge 11 commits into
Draft
Support foreign keys in forms and reserve relation names#1135audreyfeldroy wants to merge 11 commits into
audreyfeldroy wants to merge 11 commits into
Conversation
Defines the API: AirField(foreign_key=Maker) declares an integer column that references another model's primary key. Three layers: SQL generation emits REFERENCES constraints, forms auto-render FK fields as <select> dropdowns with fetched options, and the metadata follows Air's existing BasePresentation pattern. Deliberately excludes cascading deletes, reverse relations, and join queries — those are separate features.
The initial spec covered the happy path but left structural gaps:
forward references break when the target model isn't defined yet,
table creation order ignores FK dependencies, form rendering can't
fetch options without going async, _add_column_sql omits REFERENCES,
and ForeignKey inherits the wrong base class.
Key design decisions:
- String forward references ("Maker") resolve lazily against the
registry at first use, not at class definition time
- Topological sort (Kahn's algorithm) in create_tables() so FK
targets exist before dependents
- render() stays sync; views pre-fetch FK choices explicitly via
fk_choices parameter and a new as_choices() helper on AirModel
- New BaseStructure base class separates structural metadata (DDL)
from presentation metadata (rendering); PrimaryKey moves there too
- foreign_key and choices are mutually exclusive, validated at
AirField() call time
Automatic option fetching in forms, cascading deletes, reverse
relations, and join queries are all explicitly out of scope.
The spec now handles self-referential FKs (Category -> Category), multiple FKs to the same target, eager validation of non-AirModel arguments, and misspelled choice keys. It documents what happens when you delete a referenced row, what to do when adding a constraint to an existing column, and why FK columns get indexes. Key design decisions: - Dropped BaseStructure; ForeignKey inherits BasePresentation like PrimaryKey, since no consumer queries the base class - resolve(registry) takes the registry as a parameter instead of importing it, keeping field/types.py free of model imports - Renamed fk_choices to a general choices parameter on AirForm so there's one mechanism for dynamic select options, not two - as_choices() accepts order_by and limit to prevent unbounded queries on large tables - Eager issubclass check in AirField() catches foreign_key=str or foreign_key=42 at definition time FK existence validation on form submit is explicitly out of scope (Pydantic checks the type, PostgreSQL enforces the constraint).
reverse queries, and eager loading
The spec previously deferred three features users would need
immediately after declaring foreign keys. All three are now
designed: on_delete controls what PostgreSQL does when a parent
is deleted, related() lets a parent query its children, and
select_related fetches parent records in batch to avoid N+1.
Key design decisions:
- on_delete is a string ("cascade", "set_null", "restrict") that
maps directly to SQL ON DELETE clauses. PostgreSQL handles it,
no Python-level logic needed. Default is "restrict".
- related() discovers FK fields on the child model automatically.
When multiple FKs point to the same parent (created_by,
updated_by), it raises with a message directing users to
filter() with the specific field name.
- select_related uses batch queries (one per FK field), not JOINs.
JOINs change the result set shape and complicate row-to-model
mapping. Batch queries are simpler and sufficient for typical
use. Works on all(), filter(), and get().
- Attached attributes use a naming convention: maker_id becomes
maker_ (strip _id, add _). Fields without _id suffix get _rel
appended. Trailing underscore avoids collisions with existing
model fields.
Composite FKs, automatic form validation of FK existence, and
constraint migration for existing columns remain permanently out
of scope.
The spec had grown to 879 lines of pseudocode wrapped in prose,
with every decision buried inside a function body the implementer
would have to write twice. It now reads as decisions and
constraints: API contracts, SQL output examples, and design
rationale for the non-obvious calls.
Key design decisions kept or refined:
- on_delete is now Literal["cascade", "set_null", "restrict"] for
IDE autocomplete and type-check-time validation
- select_related accepts relation names ("maker") not field names
("maker_id"), and attaches attributes under the same name so
users query and read with the same word
- as_choices() detects whether a model overrides __str__ and falls
back to "ClassName #pk". Pydantic's default __str__ produces
"Maker(id=1, name='Kuma-san')" which is unusable in a dropdown.
- Dynamic form choices reach the renderer by growing the widget
callable protocol with a choices parameter. Mutating
field_info.metadata at render time is class-level shared state
and non-reentrant under async concurrent requests. Synthesizing
a per-render model class pollutes _table_registry. Threading a
parameter is honest about what's happening.
- filter()'s empty-kwargs delegation to all() gets removed during
implementation. Each method handles its own query building so
select_related (and any future parameter) can't silently drop.
Stripped every implementation code block that prose + test cases
already describe without ambiguity: resolve() body, Kahn's
algorithm, _load_related, AirField validation logic, _column_defs
integration, render() injection flow. Kept the ForeignKey
dataclass definition, AirField signature, SQL output examples,
as_choices helper, and all user-facing API examples, since those
are the contract the implementer must meet.
Dynamic choices from AirForm(choices=...) reach the widget as list[tuple[Any, str]] with typically-int PK values. The existing select renderer calls html.escape(opt_val) and str(value) == opt_val, which crashes on ints and silently fails the pre-selection comparison. Hardening the render site (escape(str(opt_val)), str(value) == str(opt_val)) fixes both the incoming dynamic-choices path and a latent bug in the Enum path for int-valued enums. Also tightens two other spec details the code review caught: _get_options needs to grow a field_name parameter to look up dynamic choices (not just the widget protocol), and the DangoOptional test assertion now checks for the specific string '"order_id" INTEGER NOT NULL' instead of a brittle split that happened to work only because order_id was the last column.
audreyfeldroy
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AirModelchoices=render arbitrary form fields as<select>elementsWhy
The foreign key spec relied on behavior that was not fully enforced in code:
AirModelandBaseModelattributeschoices=did not turn plain fields into selects, even though the spec said it wouldImpact
AirForm(..., choices=...)can drive select rendering for plain fields, not just staticChoices/Enum/Literal caseschoiceskeywordRoot Cause
AirFieldused the parameter nametype, which shadowed Python's built-intypeduring foreign-key validationValidation
uv run python -m pytest tests/test_model.py tests/test_form.py -q