Fix model defaults - #1277
Open
mxsrc wants to merge 2 commits into
Open
Conversation
mxsrc
force-pushed
the
fix-model-defaults
branch
from
August 26, 2026 15:20
39c645d to
de5df37
Compare
BaseModel resolves a field's default with getattr on the class, so a literal `nodes: List[str] = []` handed every instance the same list. One `lvol.nodes.append(...)` was visible on every other lvol and on every instance built later in the process, and the next write_to_db persisted the result. 56 fields across 12 model modules declared their defaults this way. Declare them with `default_factory(list)` / `default_factory(dict)`, mirroring dataclasses.field(default_factory=): the class holds only a marker and get_attrs_map calls the factory once per instance, so from_dict keeps merging into existing instance state by identity. The helper's declared return type is the factory's result, so mypy still sees the field's real type. Class attributes that are shared on purpose (_STATUS_CODE_MAP, Singleton._instances, RPC_NO_PRINT_OUTPUT, ServerHandler.server_session) are annotated ClassVar instead, and _annotated_attrs now skips ClassVar so a public constant such as Cluster.STATUS_CODE_MAP is not treated as a field and serialized into the FDB record. RUF012 enforces the distinction from here on. Collect the model tests under tests/unit/models/. The tier-wide guard there constructs every model twice and fails on any field that hands out the same container, whatever form the declaration takes.
BaseModel.from_dict stored the caller's own containers on the model: the non-model List branch keeps data[attr] verbatim, Dict[...] and bare List match no branch and fall through to it, and the remaining fallback only shallow-copies. Model and payload then aliased each other, so an append on either side showed up on the other and on every further model built from the same dict — including the copy a caller kept and mutated after handing it over. Detach the value before storing it. _detached recurses into plain lists and dicts (so the nested dicts of a List[dict] are copied too) and stops at BaseModel children, which from_dict already builds fresh: the fat StorageNode path pays one list allocation, not a deep copy of 97 device dicts. This is the payload-level half of the sharing the default_factory change closed at class level; the tier-wide guard there only constructs models with no data, so it could not see it.
mxsrc
force-pushed
the
fix-model-defaults
branch
from
August 27, 2026 09:55
de5df37 to
72365f6
Compare
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
2 participants
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.
In Python,
{}and[]create an instance. When this is used as a default value for an attribute, that is shared among all instances. If multiple instances are established from these default values, modification to one of them will apply to all others, which corrupts the database when they are written back. This went unspotted because we disable lots of linter rules that are violated across the codebase. This PR enables the rule that would have caught this and fixes it consistently. A The second commit fixes a similar bug class that might lead to sharing of specific attributes across instances if they are instantiated from the same payload or model dump. This might happen when using a base entity as a template for multiple others.