fix(bfabric): correct owner typing in entities/core and the None defects it hid - #600
Merged
Conversation
HasOne was made generic over its return type in #533, but HasMany's __get__ still typed its owner with a bare, unbounded TypeVar. An unbounded T has no `refs` member and `obj` was declared `| None` without a guard, so that one line produced reportAttributeAccessIssue plus reportOptionalMemberAccess, and the resulting Unknown `items` cascaded into five more baselined errors. Mirror the shape has_one.py already uses: owner typed as Entity, an instance-only guard, and a single cast at the proxy call to narrow refs.get's loose Entity | list[Entity] | None. The guard also closes a runtime wart — class-level access (Workunit.resources) raised "'NoneType' object has no attribute 'refs'" instead of a clear message. Making bfabric_field required is the second-order fix: References.get takes str, and that mismatch was invisible only because refs.get resolved to Unknown. All eight call sites already pass it explicitly. A caller that omitted it was already broken, though not loudly: with optional=False it raised "Missing field: None", and with optional=True References.get(None) found no ref_info and the descriptor silently yielded an empty proxy. Both now fail at construction instead. Note this does not improve inference for consumers: __get__ already declared its return as _HasManyProxy[E], so `workunit.resources` and friends typed correctly before this change and the Unknown never escaped the function body. The 7 removed baseline entries are all within has_many.py.
…cts it hid Three members in entities/core typed their owner with something lacking the attributes their body accessed, yielding Unknown — which then hid real defects downstream. has_many (887057a) was the first; this covers the other two and the bugs they were masking. users.py: `self._users = []` was unannotated, so the cache was list[Unknown] and everything reading it decayed. That concealed get_by_id declaring `-> User | None` while calling read_id's *string* overload, which returns Entity. It now passes the User class and picks up the typed overload (entity_reader.py:146), matching what get_by_login already did with query_one. user_created_mixin.py: `self: EntityProtocol` gave the four properties an owner with only _client and data_dict — not bfabric_instance, and not the mixin's own _users. Declaring the host-supplied members on the mixin under TYPE_CHECKING types `self` as the mixin itself, so it carries both. Two latent bugs surfaced once the Unknown cleared: - `self._client.reader` was unguarded though _client is Bfabric|None, reachable via Entity(data_dict=..., bfabric_instance=...) with no client. Now raises. - created_by/modified_by declared `-> User` but get_by_login returns User | None. workunit_definition.py:76 does created_by.id unguarded, so an unresolvable login gave "'NoneType' object has no attribute 'id'" on a path that feeds app-runner. Kept `-> User` and raise a ValueError naming the login: the field is always populated in B-Fabric, so None is an error, not a value. Consumers are untouched, since the declared `-> User` already shielded them. Declaring data_dict honestly as ApiResponseObjectType rather than EntityProtocol's looser dict[str, Any] also removed the 6 reportAny this was going to leave behind, using the isinstance/ValueError narrowing entity.py already uses for id and classname. An attempt to model the owner as a Protocol extending EntityProtocol was abandoned: Workunit failed to satisfy it, breaking `workunit.created_by` outright, and it drew three reportPrivateUsage warnings. EntityProtocol now has no consumers. Baseline: 835 -> 787, all 48 removals within the four touched files, zero additions.
The preceding commit stopped annotating UserCreatedMixin's `self` with it, and that was its only consumer: mixins/__init__.py is empty, so it was never re-exported, and nothing in the docs or tests referenced it. Its `data_dict` was also declared `dict[str, Any]`, looser than Entity's actual ApiResponseObjectType, so it would have been a misleading thing to keep around as a template for new mixins. Removes 1 further baseline entry (a reportExplicitAny on that `dict[str, Any]`).
The isinstance/ValueError guard on data_dict fields was scope creep: it existed to satisfy the type checker, not any caller. cast is the house style for exactly this narrowing (has_one.py:31, workunit.py:67), and it restores main's behaviour for created_at/modified_at, so Workunit.store_output_folder no longer changes its failure mode at all. The one behaviour change left is the intended one, created_by/ modified_by raising on an unresolvable login. Also compacts the TYPE_CHECKING stub block, which black accepts without blank lines between consecutive stub defs. 65 -> 58 lines; the diff against main goes from +40/-11 to +34/-12. Note the property-stub form is load-bearing, not verbosity: plain attribute annotations (data_dict: ApiResponseObjectType) draw 18 reportIncompatibleVariableOverride errors, because every host class inherits these as properties from Entity.
leoschwarz
marked this pull request as ready for review
August 14, 2026 08:14
leoschwarz
added a commit
that referenced
this pull request
Aug 14, 2026
The merge of main brought #600's stricter ApiResponseDataType, which made Path(workunit.application.executable["program"]) a type error. Cast at the call site as the sibling WrapAppYamlTemplate already does.
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.
created_by/modified_byto raise aValueErrornaming the login when it cannot be resolved to a user, instead of returningNoneand failing later as an opaqueAttributeError.HasManydescriptor (e.g.Workunit.resources) to raiseAttributeError: 'resource' is only accessible on an instance, matchingHasOne, instead of'NoneType' object has no attribute 'refs'.HasMany'sbfabric_fieldto a required keyword argument.UserCreatedMixinproperties also raise if their underlying field is not a string, or if the entity has no client. This moves the exception type onWorkunitDefinitionbuilding andWorkunit.store_output_folderfromAttributeErrortoValueError— unreachable while the field is populated, which it always is in B-Fabric.🤖 Prepared with assistance from Claude Opus 5 via Claude Code.