Make pydantic config models reject unknown keys (extra="forbid") - #762
Conversation
|
The core change is sound and the tests back it up. One bug should be fixed in this PR, plus two things to close before merge. Blocking: Verify before merge: required (no-default) fields on the LCO models. Rebase: the PR base is stale. Recorded base is Minor, non-blocking:
Verified correct: |
d472467 to
ec15dd7
Compare
|
Re-checked the new head (
Two things left before merge:
|
A task YAML misplaced guiding_config/acquisition_config inside instrument_configs; pydantic silently dropped both keys instead of erroring, and the task then failed can_run forever with no error pointing at the config. Every pyobs BaseModel/PolymorphicBaseModel now rejects unrecognized keys at load time instead of dropping them. Also declares the LCO portal models' previously-undeclared fields (state/submitter on LcoSchedulableRequest, instrument_name/ guide_camera_name/summary on LcoConfiguration, and eight fields on LcoObservation) rather than opting them out with extra="ignore" - the portal is self-hosted, so a schema mismatch should fail loudly at upgrade time, not get silently absorbed forever. Fixes surfaced along the way: - create_object/get_object now route comm/timezone/vfs/observer through pydantic's validation context for pydantic models instead of passing them as constructor kwargs, which extra="forbid" would otherwise reject. - Task is now a PolymorphicBaseModel so it pops its own `class` key. - Merit.create() left a stale `type` key in the config dict after deriving `class` from it; only surfaced once the LCO fixture-setup error that had been masking it was fixed.
- Constraint.create() had the same stale-type bug as Merit.create():
it derived config["class"] from config["type"] but never removed
type, so extra="forbid" now rejects it (Constraint is a
PolymorphicBaseModel). Fixed the same way, plus a regression test
reproducing the type-shorthand config path used by e.g.
OnDemandScheduler(constraints=[{"type": "Airmass", ...}]), which no
existing test covered (they all pass Constraint instances).
- Removed Merit.create()'s dead "dotted type" branch: it never set
class in the first place, so it was already broken before this PR;
half-fixing it by conditionally deleting type left it half-broken
in a different way. Added the equivalent regression test for
Merit.create()'s type-shorthand path.
- create_object()'s pydantic branch now passes by_alias=True to
model_validate (matching Merit.create/Constraint.create's existing
convention) and asserts against positional args, which model_validate
can't accept.
Checked the review's "verify required fields against a live portal"
concern against the actual portal source (LCO's Django app, our
self-hosted deployment) instead of guessing from fixtures.
Portal.observations() and Portal.download_schedule() hit different
endpoints with different response shapes:
- download_schedule() -> GET /api/observations/, routed through
ListAsDictMixin.list() -> Observation.as_dict() with no args ->
no_request=False -> observation_as_dict() sets all 8 fields
unconditionally. This is the shape the test fixtures modeled.
- observations() -> GET /api/requests/{id}/observations/, a custom
action that explicitly calls o.as_dict(no_request=True) -> those
8 fields are omitted entirely, and `request` stays a bare FK id
instead of an expanded object.
Made created/modified/ipp_value/name/observation_type/proposal/
request_group_id/submitter optional on LcoObservation. Added a
regression test against the actual no_request=True shape; verified
it fails with exactly the 8 missing-field errors against the
pre-fix code before restoring the fix.
0390b44 to
31b451b
Compare
…hand logic create_object's pydantic branch now raises instead of silently letting kwargs clobber colliding cfg keys, and the positional-args guard is a real raise instead of an assert (stripped under python -O). Also extracts the duplicated type->class shorthand resolution out of Merit.create/Constraint.create into a shared helper.
…pyobs-robotic-backend Verified against pyobs-robotic-backend source: task.id is never None on the reachable BackendObservationArchive+BackendTaskArchive path, and the hypothetical mixed-backend case was already broken pre-PR (PK-only ForeignKey field) independent of the class key, which the backend strips defensively anyway.
|
Reviewed the full diff plus final state of every changed file; ran Verdict: approveSound design, thorough plan, good regression coverage. Confirmed the core changes behave as claimed ( Findings, none blocking:
Nice side effect worth noting: |
retrieve_class_on_deserialization was the one model_validate call left without by_alias=True, unlike Constraint.create/Merit.create/ create_object. Dormant today (no constraint/merit uses aliased fields), but a polymorphic model with an aliased field would silently fail validation on load without this.
Records the merge commit and the final round of review fixes (create_object kwarg guards, deduped type-shorthand helper, by_alias on polymorphic dispatch). Closes #755.
…plan and add anchor/alias tests pydantic-extra-validation was merged (e398117, #762, closes #755) but still filed as draft/not-finished in the index. Also revise object-kwarg-validation's Decision: fix the comm_cfg anchor-holder leak at its source in pre_process_yaml instead of allowlisting it in Object.__init__, since reload_anchors() already identifies the leaking key by name. Add regression coverage for the include/anchor mechanism in tests/utils/test_config.py, including an xfail(strict=True) test documenting the comm_cfg leak itself.
Backend's ProjectSerializer includes a users field (project visibility per user) that Project never modeled. With extra="forbid" enabled (#762), this now raises a validation error instead of being silently dropped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
* Make pydantic config models reject unknown keys (extra="forbid")
A task YAML misplaced guiding_config/acquisition_config inside
instrument_configs; pydantic silently dropped both keys instead of
erroring, and the task then failed can_run forever with no error
pointing at the config. Every pyobs BaseModel/PolymorphicBaseModel
now rejects unrecognized keys at load time instead of dropping them.
Also declares the LCO portal models' previously-undeclared fields
(state/submitter on LcoSchedulableRequest, instrument_name/
guide_camera_name/summary on LcoConfiguration, and eight fields on
LcoObservation) rather than opting them out with extra="ignore" -
the portal is self-hosted, so a schema mismatch should fail loudly
at upgrade time, not get silently absorbed forever.
Fixes surfaced along the way:
- create_object/get_object now route comm/timezone/vfs/observer
through pydantic's validation context for pydantic models instead
of passing them as constructor kwargs, which extra="forbid" would
otherwise reject.
- Task is now a PolymorphicBaseModel so it pops its own `class` key.
- Merit.create() left a stale `type` key in the config dict after
deriving `class` from it; only surfaced once the LCO fixture-setup
error that had been masking it was fixed.
* Address review feedback on pydantic extra=forbid PR
- Constraint.create() had the same stale-type bug as Merit.create():
it derived config["class"] from config["type"] but never removed
type, so extra="forbid" now rejects it (Constraint is a
PolymorphicBaseModel). Fixed the same way, plus a regression test
reproducing the type-shorthand config path used by e.g.
OnDemandScheduler(constraints=[{"type": "Airmass", ...}]), which no
existing test covered (they all pass Constraint instances).
- Removed Merit.create()'s dead "dotted type" branch: it never set
class in the first place, so it was already broken before this PR;
half-fixing it by conditionally deleting type left it half-broken
in a different way. Added the equivalent regression test for
Merit.create()'s type-shorthand path.
- create_object()'s pydantic branch now passes by_alias=True to
model_validate (matching Merit.create/Constraint.create's existing
convention) and asserts against positional args, which model_validate
can't accept.
* Fix LcoObservation required fields: two endpoints, two shapes
Checked the review's "verify required fields against a live portal"
concern against the actual portal source (LCO's Django app, our
self-hosted deployment) instead of guessing from fixtures.
Portal.observations() and Portal.download_schedule() hit different
endpoints with different response shapes:
- download_schedule() -> GET /api/observations/, routed through
ListAsDictMixin.list() -> Observation.as_dict() with no args ->
no_request=False -> observation_as_dict() sets all 8 fields
unconditionally. This is the shape the test fixtures modeled.
- observations() -> GET /api/requests/{id}/observations/, a custom
action that explicitly calls o.as_dict(no_request=True) -> those
8 fields are omitted entirely, and `request` stays a bare FK id
instead of an expanded object.
Made created/modified/ipp_value/name/observation_type/proposal/
request_group_id/submitter optional on LcoObservation. Added a
regression test against the actual no_request=True shape; verified
it fails with exactly the 8 missing-field errors against the
pre-fix code before restoring the fix.
* Harden create_object kwarg guards, dedupe merit/constraint type-shorthand logic
create_object's pydantic branch now raises instead of silently letting
kwargs clobber colliding cfg keys, and the positional-args guard is a
real raise instead of an assert (stripped under python -O). Also
extracts the duplicated type->class shorthand resolution out of
Merit.create/Constraint.create into a shared helper.
* Close sibling-repo question on Task.model_dump() class-key leak into pyobs-robotic-backend
Verified against pyobs-robotic-backend source: task.id is never None on
the reachable BackendObservationArchive+BackendTaskArchive path, and the
hypothetical mixed-backend case was already broken pre-PR (PK-only
ForeignKey field) independent of the class key, which the backend
strips defensively anyway.
* Pass by_alias=True on polymorphic-dispatch deserialization
retrieve_class_on_deserialization was the one model_validate call left
without by_alias=True, unlike Constraint.create/Merit.create/
create_object. Dormant today (no constraint/merit uses aliased
fields), but a polymorphic model with an aliased field would silently
fail validation on load without this.
Records the merge commit and the final round of review fixes (create_object kwarg guards, deduped type-shorthand helper, by_alias on polymorphic dispatch). Closes #755.
…plan and add anchor/alias tests pydantic-extra-validation was merged (e398117, #762, closes #755) but still filed as draft/not-finished in the index. Also revise object-kwarg-validation's Decision: fix the comm_cfg anchor-holder leak at its source in pre_process_yaml instead of allowlisting it in Object.__init__, since reload_anchors() already identifies the leaking key by name. Add regression coverage for the include/anchor mechanism in tests/utils/test_config.py, including an xfail(strict=True) test documenting the comm_cfg leak itself.
Backend's ProjectSerializer includes a users field (project visibility per user) that Project never modeled. With extra="forbid" enabled (#762), this now raises a validation error instead of being silently dropped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 participant
Summary
extra="forbid"globally onpyobs.utils.serialization.BaseModel/PolymorphicBaseModel, so a misspelled or misplaced config key raisesValidationErrorat load instead of being silently dropped. Root cause: a task YAML putguiding_config/acquisition_configinsideinstrument_configsinstead of one level up onConfiguration; pydantic dropped both, theConfiguration-level defaults applied instead, and the task failedcan_runforever with no error pointing at the config.state/submitteronLcoSchedulableRequest;instrument_name/guide_camera_name/summaryonLcoConfiguration; eight fields onLcoObservation) instead of opting the family out withextra="ignore"— the portal is self-hosted, so a schema mismatch should fail loudly at upgrade time rather than being silently absorbed forever.create_object/get_objectnow routecomm/timezone/vfs/observerthrough pydantic's validationcontextfor pydantic models instead of passing them as constructor kwargs (whichextra="forbid"would otherwise reject).Taskis now aPolymorphicBaseModelso it pops its ownclasskey.Merit.create()left a staletypekey in the config dict after derivingclassfrom it.Full design/decision history:
specs/plans/2026-08-15-pydantic-extra-validation.md.Test plan
pytest -m "not integration and not xmpp": 1460 passed, 25 skipped, 0 failedruff checkclean on all changed filespyrefly check: 0 errorstests/robotic/scripts/test_imaging.py) reproducing the original misplaced-key bug and asserting it now raises