Repository navigation
Fix Config subclass direct access returning env Field - #313
claude-tmgbedu wants to merge 2 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SchgSwsSKsBpqmyLiFEuM1
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
claude-tmgbedu
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES. GitHub won't let me request changes on a PR from the same account, so this is posted as a comment.
Suite passes locally (3294 passed, Postgres ignored). The direct repro is fixed and the decorated-only path is unchanged. One inheritance case regresses, though.
-
support/config.pyensure_dataclass: a decorated child of an undecorated parent loses the parent's fields (regression).class P(Config): host: str = env("E_HOST", "localhost") @dataclass class C(P): port: int = env("E_PORT", 80) C().host # AttributeError; fields(C) == ['port']
@dataclassonCruns beforePhas been processed, soCnever inheritshost. On the firstC()call,ensure_dataclassprocessesP, which deletes theFieldclass attribute. Onmainthis returned the rawField; now it raisesAttributeError. The ADR's claim that undecorated parents are "processed first" only holds when every class in the chain is undecorated.
Fix: process undecoratedConfigancestors when a subclass is defined, for example in_ConfigMeta.__init__, or inConfig.__init_subclass__, overcls.__mro__[1:]. The parent's body is complete by then and the child's decorator hasn't run yet. Only the leaf would stay lazy. Add a test for this case. -
The
Configbase itself is mutated on first instantiation.reversed(cls.__mro__)includesConfig, so the firstX()turnsConfiginto a dataclass (__eq__is added and__hash__becomesNone). That is global, order-dependent state. Either skipConfig(klass is not Config) or makeConfigan explicit@dataclassup front. -
Thread safety (non-blocking, but say how it's handled). The check-then-decorate is not atomic. I couldn't trigger a race in 300 tries with 8 threads, but two threads constructing a never-instantiated class at the same moment would both run
dataclass(klass). Either guard it with athreading.Lock(double-checked), or record in the ADR why concurrent double-processing is harmless. Also,dataclass(Q); dataclass(Q); Q()worked for me on CPython 3.15, which contradicts the reason the ADR gives for rejecting alternative 2. Please re-check that claim, or state which Python versions it applies to.
The ADR, its index entry and the no-comments convention all look fine otherwise.
… base plain Decorated children now inherit fields from undecorated parents, Config itself is never a dataclass, automatic processing keeps identity hashing, and a lock prevents concurrent double processing. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SchgSwsSKsBpqmyLiFEuM1
Fixes task #3504.
Root cause
env()returns adataclasses.Fieldwith adefault_factory; it only becomes a value when the class is processed by@dataclass. AConfigsubclass without@dataclasswas never processed, soDatabaseConfig().defaultfell through to the class attribute — the rawField.Fix
_ConfigMeta.__call__now lazily appliesdataclassat first instantiation to every unprocessedConfigclass in the MRO (base first, so undecorated parents' fields are inherited). Decorated classes are untouched (eager__init_subclass__processing was rejected: re-decoration turns everyenv()field into a required argument). Casting follows existingenv()rules (int/bool by default type).ADR 035 added and indexed.
Tests
tests/support/test_config_without_dataclass.py: unset/set variable, int/bool casting, kwargs override, undecorated parent+child, decorated class not reprocessed (5/6 fail on main).--ignore=tests/masoniteorm/postgres): 3294 passed, coverage 91.4%.🤖 Generated with Claude Code
https://claude.ai/code/session_01SchgSwsSKsBpqmyLiFEuM1