refactor(bfabric): decouple entities from client via BfabricSession - #566
Draft
leoschwarz wants to merge 13 commits into
Draft
refactor(bfabric): decouple entities from client via BfabricSession#566leoschwarz wants to merge 13 commits into
leoschwarz wants to merge 13 commits into
Conversation
Replace all 16 deprecated Entity.find/find_all/find_by (FindMixin) read-path
call sites in bfabric_app_runner with the modern client.reader API
(read_id/read_ids/query_one). Behavior-preserving:
- find(id) -> reader.read_id(endpoint, id, expected_type=...)
- find_by(obj) -> reader.query_one(endpoint, obj, expected_type=...)
(callers only used the first/truthiness of the result)
- find_all(ids) -> reader.read_ids(endpoint, ids, expected_type=...)
read_ids is URI-keyed and includes None for misses, whereas find_all was
int-keyed and dropped misses. So:
* .values() consumers now filter out None
* id-indexed consumers re-key uri.components.entity_id -> entity and drop
None, preserving the prior "missing id -> KeyError" behavior
The four possibly-None accesses that were previously grandfathered in the
basedpyright baseline (execution.dataset arg, dataset.to_polars(),
registration.workunit_id/storage_id) are now handled with targeted
# pyright: ignore comments; the baseline shrinks accordingly (no new
suppressions).
Tests updated to mock the client.reader path (real EntityUri keys for
read_ids); added focused coverage for the re-key/None-drop paths in the
dataset, resource, and resource-flow resolvers.
…pers Deduplicate the 're-key EntityReader results by id / drop not-found' transform that FindMixin.find_all/find_by performed inline into two reusable free functions in bfabric.entities.core.reader_utils. FindMixin now delegates to entities_by_id, and the app-runner FindMixin-migration sites use the helpers instead of inline dict-comprehensions, collapsing the four bulk read_ids sites to one line each. No behavior change. The basedpyright baseline shrinks by one entry (find_by's grandfathered reportReturnType is now fixed via a localized cast).
read_id/read_ids/query/query_one now accept an entity class in place of the endpoint string (e.g. client.reader.read_id(Resource, id)), inferring both the endpoint and the result type via import_entity.entity_type_of (the class->string inverse of import_entity, keyed on the lowercase-class-name convention). The string form (with optional expected_type) still works. Adopt the class form across bfabric_app_runner + a few core sites, dropping the redundant endpoint string; narrow the read-only ResolveBfabricDatasetSpecs and ResolveBfabricResourceDatasetSpecs to take an EntityReader instead of the full Bfabric client.
Replace the free entities_by_id / present_entities helpers (and the reader_utils module) with an EntityResult dict subclass returned by read_ids / read_uris, exposing .present and .by_id views. Call sites now read reader.read_ids(Resource, ids).present instead of wrapping the result. query keeps its precise dict[EntityUri, Entity] return (never has not-found entries, so no EntityResult). Also migrate the two remaining order-fasta call sites to the class-based lookup form.
…indmixin-migration
Entities become pure data (data_dict + required bfabric_instance); the connection moves into a new ambient, read-only BfabricSession that routes reads to the right instance and owns the entity cache. Removes the two cross-instance guards in EntityReader, deletes FindMixin, and makes client.reader return a BfabricSession. Writes stay on the explicit client.
…bfabric-session Reconcile BfabricSession with #563's EntityResult refactor: BfabricSession read_uris/read_ids now return EntityResult, and the Phase-F callers that used the deleted reader_utils helpers switch to the .present/.by_id views (query call sites use .values()/query_one, since query stays a plain dict).
- dataset/show.py: move the reportAny pyright-ignore onto the assignment line after black wrapped the call (basedpyright flagged the drifted comment). - rest_proxy is_employee test: drop the removed client= arg from User(...).
client.reader is a cached_property, so a nested `with client.reader:` (e.g. the app-runner resolver running inside the @use_client CLI decorator) re-enters the same session object. The single _token/_parent fields were overwritten on the second enter, so the outer __exit__ reset an already-used token (RuntimeError) and leaked the session into the context var. Replace them with a (parent, token) frame stack pushed/popped per with-block; _parent is now the top frame's parent. Adds a same-object re-entry regression test.
Contributor
📝 "TODO" Changes DetectedSummary: ✅ 1 "TODO" removed ✅ Removed "TODO"s (1)
This comment is automatically updated when "TODO" changes are detected. |
Re-entering an already-active session recorded no parent, so instance delegation to an outer session was lost inside the nested with-block; keep the existing parent instead (with a regression test). Also refresh EntityReader docstrings that still promised the removed cross-instance ValueError guards, and note the single-instance assumption in Users.
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.
Entities embedded a live
Bfabricclient (the author flagged this as a "design mistake"), coupling immutable data to a connection, dragging credentials into every pickle/YAML round-trip, and forcing two cross-instance guards inEntityReader. This makes entities pure data and moves the connection into a new ambient, read-onlyBfabricSession.Key changes
Entity(data_dict, bfabric_instance)— theclient=parameter,Entity._client, and the deprecatedFindMixin(find/find_all/find_by) are removed.BfabricSession(returned byclient.reader) routes reads to the right connection by instance, so one process can read across multiple B-Fabric instances, and it owns the entity cache. Relationship navigation resolves the connection from the active session; the@use_clientdecorator opens one automatically.Bfabricclient — the session is read-only, so the acting authority is always visible.EntityReadercross-instance guards are gone (reads are routed, not rejected); caching is now session-scoped.Notes
LookupError(explicit-only by design; server/webapp code opens a per-request session — recipe in the newdesign/entity_session.md).BfabricSessionis re-entrant, so a nestedwith client.reader:(e.g. app-runner's resolver inside@use_client) is safe.EntityResultreader API.Verified:
bfabric/bfabric_scripts/bfabric_app_runner/bfabric_rest_proxy/bfabric_asgi_authtest suites, basedpyright, and ruff all pass.🤖 Prepared with assistance from Claude Opus 4.8 via Claude Code.