Repository navigation
feat(schema): retired enum labels (#627) - #633
Conversation
…627) __ferro_retired_labels__ on a StrEnum stops this release writing a label while the member, the Postgres type and any db_check keep it. Construction and attribute assignment refuse it; hydrated rows save back unchanged. The IR carries enum_retired_labels on every column of the enum; no DDL. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
0x054
left a comment
There was a problem hiding this comment.
Spec: PASS
- Minor · docs/examples/migrations_retire_label.py:40 at 22ee776 · Misunderstood · the PROBE's "a row release A wrote hydrates" uses
Order.model_construct(...), not a query, although the script has just inserted the row; the ticket's hydrate-and-save path is pinned for real bytest_a_row_holding_the_retired_label_hydrates_and_saves_after_another_change(sqlite + postgres), so this is a docs-fidelity nit only. - Checked: every acceptance criterion and Test-scenario line has a test in
tests/test_retired_labels.py(shape/unknown-labelTypeError, construction + assignment in both styles, real hydrate → changenote→save()on both dialects,bulk_createbeside a hydrated retired row,new= "no schema change" +checkexit 0 + liveenum_range/ck_rmlorder_statusstill listcanceled, member+declaration deletion = the existingSWAPstatements). - Checked I-1: no planner, renderer or DDL change;
tests/test_generate_enum_removal.pyand the IR vectors unchanged againstorigin/feat/pinch-migration-needs;enum_retired_labelsisserde(default, skip_serializing_if = "Option::is_none")and the compiler omits the key when undeclared. - Checked I-7: both tabs backed by
migrations_retire_label{,_annotated}.py, run bytests/test_docs_migrations.py(byte-identical migrations across styles) andtests/test_docs_examples.py. - Declared files outside Files owned accepted: the
SchemaColumnliterals (noDefaulton the struct),step_class.rsretired: false(confirmed: at merge-base c5b9e72,cargo test -p ferro-migrate --no-runfails withE0063 missing field 'retired'atcrates/ferro-migrate/src/run_plan/step_class.rs:409), the docs-test list entry, andmodels.py(the validator seam the ticket asks for).
Cannot verify:closingIssuesReferencesis empty because the base is not the default branch; the body carriesCloses #627, which takes effect when the integration branch reachesmain.
Quality: FAIL
- Important · src/ferro/models.py:397 at 22ee776 · the construction guard is a
model_validator(mode="after")that scans every guarded field of the whole instance. Pydantic re-runs model after-validators on every assignment whenvalidate_assignment=True(a configdocs/pages/guide/models-and-fields.md:541says works), so on such a model a row that holdscanceledcan no longer have another field changed:O.model_construct(id=1, status=St.CANCELED); o.note = "b"raisesValidationError: St.CANCELED ('canceled') is retired in this release…(reproduced on the PR head). That breaks the ticket's "changing another field and saving succeeds" for everyvalidate_assignmentmodel. Fix: guard the enum field's own value (a per-field after validator on the retired-label fields, which Pydantic runs at construction and, undervalidate_assignment, only when that field is assigned), plus a test withvalidate_assignment=True. - Important · src/ferro/models.py:376 at 22ee776 ·
__setattr__is defined on theModelbase, so every attribute assignment on every model pays a Python frame, a class-dict lookup and asuper()call for a feature only enums that declare__ferro_retired_labels__use. Measured on the PR head (pydantic 2.12.5):Model.__setattr__207 ns vsBaseModel.__setattr__96 ns per assignment on a model with no retired labels, roughly 2x, and it also puts a Python frame in front of pydantic's memoized__pydantic_setattr_handlers__fast path. The metaclass already knows the guarded fields (src/ferro/metaclass.py:673), so install the assignment guard only on classes whose__ferro_retired_enum_labels__is non-empty (and skip it whenvalidate_assignment=True, where the per-field validator above already covers assignment). Cost elsewhere is then zero. The same applies to the base-level after-validator, which now runs on every construction of every model. - Minor · src/ferro/models.py:407 at 22ee776 · construction raises a
value_errorwith an emptylocand the prefix "Value error, ", while assignment raisesretired_enum_labelatloc=(name,). The two refusals of one rule have different error types and locations. A per-field validator raising the samePydanticCustomErrorwould make them one.
Verify: passed on 22ee776 (uv run maturin develop 0; cargo test -p ferro-schema-ir 0, 47 passed; pytest tests/test_ir_vectors_contract.py 0, 14 passed; the sqlite+postgres pytest set 0, 419 passed; uv run prek run --all-files 0, all hooks Passed). No sdd-verify note on the head commit.
) An AfterValidator on exactly the enum fields that retire a label replaces the model-level validator, so validate_assignment no longer blocks changing another field of a row holding the label. The assignment guard is installed only on classes that retire a label and only without validate_assignment; Model.__setattr__ is untouched. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
0x054
left a comment
There was a problem hiding this comment.
Scoped re-review 22ee776..3bb112f
- ADDRESSED · Important · src/ferro/models.py:397 (validate_assignment) ·
src/ferro/models.pyis byte-identical toorigin/feat/pinch-migration-needsat 3bb112f.src/ferro/metaclass.py:735-739now appends anAfterValidatoronly to the guarded enum fields'FieldInfo.metadataand rebuilds the class. Undervalidate_assignment=True, assigningnoteon amodel_constructedstatus=CANCELEDinstance now succeeds; assigning the retired label is still refused (loc=('status',)).test_a_hydrated_retired_row_changes_another_field_and_savespasses for [sqlite|postgres] × [False|True] (4/4), run here; theTruecase is the one that failed at 22ee776. - ADDRESSED · Important · src/ferro/models.py:376 (
__setattr__on the base) · the base is untouched (Model.__setattr__ is BaseModel.__setattr__).src/ferro/metaclass.py:741assignscls.__setattr__only when the class has retired-label fields andvalidate_assignmentis off. Checked: a sibling model defined after a retiring one has no__setattr__of its own, the retiring class has its own, and avalidate_assignmentclass has none. The guard survivesresolve_relationships()'s forced rebuild on a model with a FK; construction is still refused afterwards.test_a_model_without_retired_labels_gains_no_setattr_or_validatorpasses. - Minor (previous) · partly addressed · construction now reports
loc=('status',)like assignment does, but the error type still differs:value_errorfrom theAfterValidator(src/ferro/metaclass.py:108) andretired_enum_labelfrom the__setattr__(src/ferro/metaclass.py:127). Does not block.
New Critical/Important in the fix diff: none.
Checked: test_assigning_the_label_fails_with_the_ticket_text_under_both_configs [False|True] passes. Hydration still bypasses both checks (I-2): the hydrate→change→save tests above pass on both dialects. The sdd-verify note exists on 3bb112f.
Verify: passed on 3bb112f (uv run maturin develop 0; cargo test -p ferro-schema-ir 0, 47 passed; pytest tests/test_ir_vectors_contract.py 0, 14 passed; the sqlite+postgres pytest set 0, 426 passed; uv run prek run --all-files 0, all hooks Passed).
The planner's test column gains enum_retired_labels; the rustfmt-only hunks in files this change does not touch are dropped; the serial-sequence test is split into its up-built and pass-built halves. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
…627) (#633) `__ferro_retired_labels__ = frozenset({"canceled"})` on a `StrEnum` keeps the member, so rows holding the label still hydrate and save, while assigning the label at construction or by attribute set is refused with an error naming it as retired in this release. The label stays in the Postgres enum type and in any `db_check` check; retiring renders no DDL and writes no migration. The guard is a per-field validator attached only to enum fields whose enum retires a label; models without retired labels gain nothing. Also carries the `retired` field a `SchemaColumn` test literal in the step-class golden table was missing after #625 and #626 merged independently. Closes #627 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR
Closes #627
What
A
StrEnumcan declare__ferro_retired_labels__ = frozenset({"canceled"}). The member stays, so rows holding the label still load, and the Postgres type and anydb_checkkeep listing it.Order(status="canceled")andorder.status = Status.CANCELEDare refused with a validation error ("Status.CANCELED ('canceled') is retired in this release: the previous release may still write it, this one does not"). A loaded row holding the label saves back after another field changes.ferro migrate newsays "no schema change" andcheckis clean. Deleting the member and the declaration together is the existing label removal, unchanged.Verify
Ran the ticket's Verify block on the final commit: passed (419 pytest on sqlite+postgres, cargo test -p ferro-schema-ir, IR vector contract, prek).
TDD: RED
pytest tests/test_retired_labels.py --db-backends=sqlitefailed (7 failed: construction, assignment, IR field, hydrate/save); GREEN same command passed, 23/23 on sqlite+postgres.Verify block corrected: no
Notes for the reviewer
Outside Files owned:
SchemaColumnhas noDefault, so the newenum_retired_labelsfield needed one line in every struct literal:src/{codec_plan,state,schema,live_ir}.rs,crates/ferro-migrate/src/{generate/mod,plan,tests,revision_tests,run_plan/step_class}.rs,crates/ferro-ddl-lowering/src/lib.rs.step_class.rstest literal was also missingretired(compile error under--all-targetssince #625); fixed. #628 toucheslive_ir.rs: expect a one-line textual conflict at most.src/ferro/models.pygains a__setattr__(dict lookup, no-op for models without retired labels) and an after-validator.tests/test_docs_migrations.pylists the new example.Fix report (review round 1), head 3bb112f
Model.__setattr__are gone;models.pyis untouched. The metaclass now attaches anAfterValidatorto exactly the enum fields whose enum retires a label (thenmodel_rebuild), and installs a__setattr__guard only on those classes, only whenvalidate_assignmentis off. Undervalidate_assignment=TruePydantic runs the field validator for the assigned field alone, so changingnoteon a row holdingcanceledworks. Models without retired labels gain nothing.tests/test_retired_labels.py):test_a_hydrated_retired_row_changes_another_field_and_saves[sqlite/postgres x validate_assignment False/True],test_assigning_the_label_fails_with_the_ticket_text_under_both_configs,test_a_model_without_retired_labels_gains_no_setattr_or_validator, plus the earlier construction, assignment, bulk_create and migration tests.UV_FROZEN=1andFERRO_POSTGRES_URLexported: maturin develop 0,cargo test -p ferro-schema-ir0,tests/test_ir_vectors_contract.py0, the Verify pytest list on sqlite+postgres: 426 passed,prek run --all-files0. Full suite (sqlite): 2826 passed, 324 skipped, 2 xfailed. sdd-verify note recorded on HEAD and pushed.🤖 Generated with Claude Code
https://claude.ai/code/session_014ERLk8yFhJy7rdDyU7rGdR