diff --git a/.basedpyright/baseline.bfabric.json b/.basedpyright/baseline.bfabric.json index 0c8b62536..63755b412 100644 --- a/.basedpyright/baseline.bfabric.json +++ b/.basedpyright/baseline.bfabric.json @@ -1530,14 +1530,6 @@ "endColumn": 101, "lineCount": 1 } - }, - { - "code": "reportReturnType", - "range": { - "startColumn": 15, - "endColumn": 84, - "lineCount": 1 - } } ], "./bfabric/src/bfabric/entities/core/mixins/user_created_mixin.py": [ @@ -5736,22 +5728,6 @@ "endColumn": 25, "lineCount": 1 } - }, - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 4, - "endColumn": 14, - "lineCount": 1 - } - }, - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 4, - "endColumn": 14, - "lineCount": 1 - } } ], "./bfabric/src/bfabric/utils/paginator.py": [ diff --git a/.basedpyright/baseline.bfabric_app_runner.json b/.basedpyright/baseline.bfabric_app_runner.json index 374b1fdaf..c994c26e2 100644 --- a/.basedpyright/baseline.bfabric_app_runner.json +++ b/.basedpyright/baseline.bfabric_app_runner.json @@ -1223,14 +1223,6 @@ "lineCount": 1 } }, - { - "code": "reportReturnType", - "range": { - "startColumn": 15, - "endColumn": 33, - "lineCount": 1 - } - }, { "code": "reportUnusedCallResult", "range": { @@ -1363,14 +1355,6 @@ "lineCount": 1 } }, - { - "code": "reportArgumentType", - "range": { - "startColumn": 34, - "endColumn": 62, - "lineCount": 1 - } - }, { "code": "reportAny", "range": { @@ -2101,14 +2085,6 @@ "lineCount": 1 } }, - { - "code": "reportOptionalMemberAccess", - "range": { - "startColumn": 61, - "endColumn": 70, - "lineCount": 1 - } - }, { "code": "reportUnknownMemberType", "range": { @@ -2258,312 +2234,6 @@ } } ], - "./bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/resolver.py": [ - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 13, - "endColumn": 20, - "lineCount": 1 - } - }, - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 13, - "endColumn": 43, - "lineCount": 1 - } - }, - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 13, - "endColumn": 44, - "lineCount": 1 - } - }, - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 13, - "endColumn": 52, - "lineCount": 1 - } - }, - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 13, - "endColumn": 52, - "lineCount": 1 - } - }, - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 13, - "endColumn": 39, - "lineCount": 1 - } - }, - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 13, - "endColumn": 39, - "lineCount": 1 - } - }, - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 13, - "endColumn": 47, - "lineCount": 1 - } - }, - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 13, - "endColumn": 46, - "lineCount": 1 - } - }, - { - "code": "reportUnannotatedClassAttribute", - "range": { - "startColumn": 13, - "endColumn": 32, - "lineCount": 1 - } - }, - { - "code": "reportUnknownVariableType", - "range": { - "startColumn": 12, - "endColumn": 25, - "lineCount": 1 - } - }, - { - "code": "reportUnknownMemberType", - "range": { - "startColumn": 28, - "endColumn": 53, - "lineCount": 1 - } - }, - { - "code": "reportUnknownVariableType", - "range": { - "startColumn": 27, - "endColumn": 37, - "lineCount": 1 - } - }, - { - "code": "reportUnknownMemberType", - "range": { - "startColumn": 20, - "endColumn": 32, - "lineCount": 1 - } - }, - { - "code": "reportUnknownArgumentType", - "range": { - "startColumn": 65, - "endColumn": 75, - "lineCount": 1 - } - }, - { - "code": "reportUnknownMemberType", - "range": { - "startColumn": 20, - "endColumn": 32, - "lineCount": 1 - } - }, - { - "code": "reportUnknownArgumentType", - "range": { - "startColumn": 65, - "endColumn": 75, - "lineCount": 1 - } - }, - { - "code": "reportUnknownMemberType", - "range": { - "startColumn": 20, - "endColumn": 32, - "lineCount": 1 - } - }, - { - "code": "reportUnknownArgumentType", - "range": { - "startColumn": 70, - "endColumn": 80, - "lineCount": 1 - } - }, - { - "code": "reportUnknownMemberType", - "range": { - "startColumn": 20, - "endColumn": 32, - "lineCount": 1 - } - }, - { - "code": "reportUnknownArgumentType", - "range": { - "startColumn": 78, - "endColumn": 88, - "lineCount": 1 - } - }, - { - "code": "reportUnknownMemberType", - "range": { - "startColumn": 20, - "endColumn": 32, - "lineCount": 1 - } - }, - { - "code": "reportUnknownArgumentType", - "range": { - "startColumn": 78, - "endColumn": 88, - "lineCount": 1 - } - }, - { - "code": "reportUnknownMemberType", - "range": { - "startColumn": 20, - "endColumn": 32, - "lineCount": 1 - } - }, - { - "code": "reportUnknownArgumentType", - "range": { - "startColumn": 69, - "endColumn": 79, - "lineCount": 1 - } - }, - { - "code": "reportUnknownMemberType", - "range": { - "startColumn": 20, - "endColumn": 32, - "lineCount": 1 - } - }, - { - "code": "reportUnknownArgumentType", - "range": { - "startColumn": 73, - "endColumn": 83, - "lineCount": 1 - } - }, - { - "code": "reportUnknownMemberType", - "range": { - "startColumn": 20, - "endColumn": 32, - "lineCount": 1 - } - }, - { - "code": "reportUnknownArgumentType", - "range": { - "startColumn": 72, - "endColumn": 82, - "lineCount": 1 - } - }, - { - "code": "reportUnknownMemberType", - "range": { - "startColumn": 20, - "endColumn": 32, - "lineCount": 1 - } - }, - { - "code": "reportUnknownArgumentType", - "range": { - "startColumn": 58, - "endColumn": 68, - "lineCount": 1 - } - }, - { - "code": "reportUnknownArgumentType", - "range": { - "startColumn": 33, - "endColumn": 42, - "lineCount": 1 - } - }, - { - "code": "reportUnknownArgumentType", - "range": { - "startColumn": 36, - "endColumn": 41, - "lineCount": 1 - } - }, - { - "code": "reportUnknownParameterType", - "range": { - "startColumn": 8, - "endColumn": 28, - "lineCount": 1 - } - }, - { - "code": "reportMissingTypeArgument", - "range": { - "startColumn": 71, - "endColumn": 75, - "lineCount": 1 - } - }, - { - "code": "reportUnknownVariableType", - "range": { - "startColumn": 8, - "endColumn": 15, - "lineCount": 1 - } - }, - { - "code": "reportUnknownMemberType", - "range": { - "startColumn": 12, - "endColumn": 38, - "lineCount": 1 - } - }, - { - "code": "reportUnknownVariableType", - "range": { - "startColumn": 15, - "endColumn": 22, - "lineCount": 1 - } - } - ], "./bfabric_app_runner/src/bfabric_app_runner/output_registration/register.py": [ { "code": "reportOptionalMemberAccess", @@ -2621,14 +2291,6 @@ "lineCount": 1 } }, - { - "code": "reportOptionalMemberAccess", - "range": { - "startColumn": 65, - "endColumn": 76, - "lineCount": 1 - } - }, { "code": "reportOptionalMemberAccess", "range": { @@ -2653,14 +2315,6 @@ "lineCount": 1 } }, - { - "code": "reportOptionalMemberAccess", - "range": { - "startColumn": 65, - "endColumn": 75, - "lineCount": 1 - } - }, { "code": "reportAny", "range": { @@ -2904,16 +2558,6 @@ } } ], - "./bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_annotation_spec.py": [ - { - "code": "reportUnusedParameter", - "range": { - "startColumn": 31, - "endColumn": 37, - "lineCount": 1 - } - } - ], "./bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_dataset_spec.py": [ { "code": "reportUnannotatedClassAttribute", @@ -2932,14 +2576,6 @@ "endColumn": 16, "lineCount": 1 } - }, - { - "code": "reportUnusedParameter", - "range": { - "startColumn": 31, - "endColumn": 37, - "lineCount": 1 - } } ], "./bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_resource_archive_spec.py": [ @@ -2952,7 +2588,7 @@ } } ], - "./bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_resource_dataset.py": [ + "./bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_resource_dataset_spec.py": [ { "code": "reportUnannotatedClassAttribute", "range": { @@ -2972,26 +2608,6 @@ } } ], - "./bfabric_app_runner/src/bfabric_app_runner/specs/inputs/file_spec.py": [ - { - "code": "reportUnusedParameter", - "range": { - "startColumn": 31, - "endColumn": 37, - "lineCount": 1 - } - } - ], - "./bfabric_app_runner/src/bfabric_app_runner/specs/inputs/static_file_spec.py": [ - { - "code": "reportUnusedParameter", - "range": { - "startColumn": 31, - "endColumn": 37, - "lineCount": 1 - } - } - ], "./bfabric_app_runner/src/bfabric_app_runner/specs/inputs/static_yaml_spec.py": [ { "code": "reportMissingTypeArgument", diff --git a/AGENTS.md b/AGENTS.md index da466c00d..f930cf28a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -108,11 +108,32 @@ Each package's docs live alongside its source. Skim the index when working in a read on its own (drop a prefix only when the class already conveys it). - Ruff linting is currently only enforced on the `bfabric` package (scripts, wrapper_creator, tests, noxfile are excluded via per-file-ignores) - Line length: 120 (ruff and black) -- Do not restate a parameter's default value in its docstring when the signature already shows it (e.g. `client_id: str = DEFAULT_CLIENT_ID`). Writing `(default "CLI")` in the `:param:` line just duplicates the signature and drifts out of sync when the default changes. Keep notes that explain what a value *means* (e.g. `(``0`` = auto-assign)`), not ones that merely repeat it. This also applies to class/model docstrings that restate a field's default shown a few lines below (prefer "see `field_name`" over repeating the literal value). Note the common case where the signature default is a sentinel like `None` but the docstring explains what it resolves to at runtime (e.g. `max_results: int | None = 100` documented as `` (``None`` for all) ``, or `path: Path | None = None` documented as `` (``None`` writes to ``./output.yml``) ``) — that is the *meaning* case, not the restatement case, and should be kept; phrase it as "``None`` does/means X", not "(default: X)", so it isn't mistaken for a literal restatement. +- Docstrings should explain what a value *means*, never restate a default the signature already shows. Drop `(default "CLI")` for `client_id: str = DEFAULT_CLIENT_ID`; keep `(``0`` = auto-assign)`. Same for model fields (prefer "see `field_name`" over repeating the literal). The exception is a sentinel default whose runtime meaning differs — document `max_results: int | None = 100` as `` (``None`` for all) `` — but phrase it as "``None`` means X", not "(default: X)". - basedpyright uses per-package baseline files at `.basedpyright/baseline.{package}.json` — **do not edit baseline files to silence new errors**; fix the code or add a targeted `# pyright: ignore[...]` comment on the offending line. Baselines only exist to grandfather in pre-existing errors. - Integration tests live in a separate repository - Use TDD: write a failing test first, verify it fails, then fix the code, then verify the test passes +## Production code conventions + +Machine-enforced style (annotations, `X | None` over `Optional`, `pathlib` over `os.path`, naming) is +covered by ruff (`nox -s code_style`) and basedpyright — don't re-document it here. These are the +conventions the tooling *doesn't* catch: + +- **Entity access** — read entities via the client's `EntityReader`: `client.reader.query(...)` / + `query_one(...)` / `read_ids(...)`, then use the entity's typed properties and `HasOne`/`HasMany` + relationships. Do **not** use the deprecated `FindMixin` (`Entity.find*`) or `ResultContainer`'s + `to_list_dict()` for new code. +- **Client construction** — obtain a `Bfabric` via `Bfabric.connect()` (config file) or + `Bfabric.connect_webapp()` (token auth); in CLI commands take it from the `@use_client` decorator + rather than constructing one by hand. `Bfabric` and the OAuth credential provider must stay + picklable (used across FastAPI workers / interactive sessions). +- **Exceptions** — raise a specific `Bfabric*Error` subclass of `RuntimeError` from the package's + `errors.py` (core: `bfabric/errors.py`; subpackages keep their own, e.g. `transfer/errors.py`). + Subclass an existing error to refine it (`BfabricTokenExpiredError(BfabricTokenValidationFailedError)`) + rather than raising a bare `RuntimeError`/`ValueError` for a domain failure. +- **Logging** — use loguru (`from loguru import logger`); never `print()` for diagnostics and don't + create stdlib `logging.getLogger(...)` loggers. + ## Releases Each package is versioned and released independently (own `pyproject.toml` version, own `docs/changelog.md`, own `/` git tag). Preparing a release means: bump the `version` in that package's `pyproject.toml` and promote its changelog `[Unreleased]` section to a dated version heading. The release pipeline extracts the changelog section matching the tag and publishes it as the GitHub release notes. diff --git a/bfabric/docs/changelog.md b/bfabric/docs/changelog.md index 0b36f2222..f7c33aa74 100644 --- a/bfabric/docs/changelog.md +++ b/bfabric/docs/changelog.md @@ -12,6 +12,9 @@ Minor breaking changes are still possible in `1.X.Y` but we try to announce them - `ResultContainer.to_polars()` returns an empty DataFrame for an empty result set instead of raising `polars.exceptions.NoDataError`, fixing a crash in `bfabric-cli api read` when a query matched no records. - PKCE login: the browser callback page now renders a distinct, styled "Login failed" page showing the provider's error (e.g. a two-factor-enrollment requirement) instead of always claiming "Login successful". - OAuth token-acquisition failures (expired/revoked refresh token, unreachable token endpoint) now raise a clear `BfabricOAuthError` instead of leaking an `authlib`/`requests` traceback. +- `EntityReader` lookups (`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; the string form (with optional `expected_type`) still works. +- Internal: new `bfabric.entities.core.reader_utils` helpers `entities_by_id` / `present_entities` to reshape `EntityReader` results (re-key by id dropping not-found, or drop not-found from the values); the deprecated `FindMixin` now delegates to them. Class→endpoint inference is centralized in `import_entity.entity_type_of` (the inverse of `import_entity`). +- bfabric now follows loguru's convention for libraries and emits **no log records by default** — including the version banner previously logged at `INFO` on every client construction — so library, server, and test contexts stay quiet. The command-line tools re-enable it automatically; other applications can opt in with `logger.enable("bfabric")`. ## \[1.20.0rc2\] - 2026-07-15 diff --git a/bfabric/src/bfabric/__init__.py b/bfabric/src/bfabric/__init__.py index 91671b709..72e78d503 100644 --- a/bfabric/src/bfabric/__init__.py +++ b/bfabric/src/bfabric/__init__.py @@ -1,9 +1,17 @@ import importlib.metadata +from loguru import logger + from bfabric.bfabric import Bfabric from bfabric.config.bfabric_auth import BfabricAuth from bfabric.config.bfabric_client_config import BfabricAPIEngineType, BfabricClientConfig +# bfabric follows loguru's convention for libraries: it emits no log records unless the +# application opts in with ``logger.enable("bfabric")``. The command-line entry points do this +# via ``bfabric.utils.cli_integration.setup_script_logging``. This keeps library, server, and test +# contexts quiet by default -- most visibly the version banner logged on every client construction. +logger.disable("bfabric") + __all__ = [ "Bfabric", "BfabricAPIEngineType", diff --git a/bfabric/src/bfabric/bfabric.py b/bfabric/src/bfabric/bfabric.py index 83e01420d..be41a32bc 100644 --- a/bfabric/src/bfabric/bfabric.py +++ b/bfabric/src/bfabric/bfabric.py @@ -668,8 +668,7 @@ def get_system_auth( if config_path: # NOTE: If user explicitly specifies a path to a wrong config file, this has to be an exception raise OSError(f"Explicitly specified config file does not exist: {resolved_path}") - # TODO: Convert to log - print(f"Warning: could not find the config file in the default location: {resolved_path}") + logger.warning(f"could not find the config file in the default location: {resolved_path}") config = BfabricClientConfig(base_url=base_url) auth = None if login is None or password is None else BfabricAuth(login=login, password=password) diff --git a/bfabric/src/bfabric/entities/core/entity_reader.py b/bfabric/src/bfabric/entities/core/entity_reader.py index 6a6d832a3..a8297c630 100644 --- a/bfabric/src/bfabric/entities/core/entity_reader.py +++ b/bfabric/src/bfabric/entities/core/entity_reader.py @@ -1,12 +1,12 @@ from __future__ import annotations -from typing import TYPE_CHECKING, TypeGuard, TypeVar, cast +from typing import TYPE_CHECKING, TypeGuard, TypeVar, cast, overload from loguru import logger from bfabric.entities.cache.context import get_cache_stack from bfabric.entities.core.entity import Entity -from bfabric.entities.core.import_entity import instantiate_entity +from bfabric.entities.core.import_entity import entity_type_of, instantiate_entity from bfabric.entities.core.uri import EntityUri, GroupedUris from bfabric.experimental import MultiQuery @@ -20,6 +20,17 @@ EntityT = TypeVar("EntityT", bound="Entity") +def _resolve_entity_type(entity_type: str | type[EntityT], expected_type: type[EntityT]) -> tuple[str, type[EntityT]]: + """Normalize the ``entity_type`` argument of the reader lookups. + + An entity *class* infers its endpoint string (via :func:`entity_type_of`) and doubles as the + ``expected_type``; a *string* endpoint is returned unchanged alongside the given ``expected_type``. + """ + if isinstance(entity_type, type): + return entity_type_of(entity_type), entity_type + return entity_type, expected_type + + class EntityReader: """Reads entities from B-Fabric by URI or ID, respecting the cache stack if configured. @@ -113,21 +124,37 @@ def read_uris( return cast("dict[EntityUri, EntityT | None]", results) + @overload + def read_id( + self, entity_type: type[EntityT], entity_id: int | str, bfabric_instance: str | None = None + ) -> EntityT | None: ... + @overload def read_id( self, entity_type: str, entity_id: int | str, bfabric_instance: str | None = None, *, + expected_type: type[EntityT], + ) -> EntityT | None: ... + @overload + def read_id(self, entity_type: str, entity_id: int | str, bfabric_instance: str | None = None) -> Entity | None: ... + def read_id( + self, + entity_type: str | type[EntityT], + entity_id: int | str, + bfabric_instance: str | None = None, + *, expected_type: type[EntityT] = Entity, ) -> EntityT | None: """Read a single entity by its type and ID. Args: - entity_type: B-Fabric entity type (e.g., "sample", "project") + entity_type: B-Fabric entity type — either the endpoint string (e.g. ``"sample"``) or an + entity class (e.g. ``Sample``), in which case the endpoint and ``expected_type`` are inferred entity_id: Numeric ID of entity bfabric_instance: B-Fabric instance URL (defaults to client's configured instance) - expected_type: Entity class to validate and cast the result + expected_type: Entity class to validate and cast the result (ignored when a class is passed) Returns: Entity object or ``None`` if not found @@ -135,6 +162,7 @@ def read_id( Raises: ValueError: If instance doesn't match the client's configuration """ + entity_type, expected_type = _resolve_entity_type(entity_type, expected_type) results = self.read_ids( entity_type=entity_type, entity_ids=[int(entity_id)], @@ -143,25 +171,44 @@ def read_id( ) return list(results.values())[0] + @overload + def read_ids( + self, entity_type: type[EntityT], entity_ids: Sequence[int | str], bfabric_instance: str | None = None + ) -> dict[EntityUri, EntityT | None]: ... + @overload def read_ids( self, entity_type: str, entity_ids: Sequence[int | str], bfabric_instance: str | None = None, *, + expected_type: type[EntityT], + ) -> dict[EntityUri, EntityT | None]: ... + @overload + def read_ids( + self, entity_type: str, entity_ids: Sequence[int | str], bfabric_instance: str | None = None + ) -> dict[EntityUri, Entity | None]: ... + def read_ids( + self, + entity_type: str | type[EntityT], + entity_ids: Sequence[int | str], + bfabric_instance: str | None = None, + *, expected_type: type[EntityT] = Entity, ) -> dict[EntityUri, EntityT | None]: """Read multiple entities of the same type by their IDs. Args: - entity_type: B-Fabric entity type (e.g., "sample") + entity_type: B-Fabric entity type — either the endpoint string (e.g. ``"sample"``) or an + entity class (e.g. ``Sample``), in which case the endpoint and ``expected_type`` are inferred entity_ids: List of numeric IDs bfabric_instance: B-Fabric instance URL (defaults to client's configured instance) - expected_type: Entity class to validate and cast all results + expected_type: Entity class to validate and cast all results (ignored when a class is passed) Returns: Dictionary mapping entity URIs to their objects (or ``None`` if not found) """ + entity_type, expected_type = _resolve_entity_type(entity_type, expected_type) bfabric_instance = bfabric_instance if bfabric_instance is not None else self._client.config.base_url uris = [ EntityUri.from_components(bfabric_instance=bfabric_instance, entity_type=entity_type, entity_id=int(id)) @@ -169,6 +216,15 @@ def read_ids( ] return self.read_uris(uris, expected_type=expected_type) + @overload + def query( + self, + entity_type: type[EntityT], + obj: ApiRequestObjectType, + bfabric_instance: str | None = None, + max_results: int | None = 100, + ) -> dict[EntityUri, EntityT]: ... + @overload def query( self, entity_type: str, @@ -176,6 +232,23 @@ def query( bfabric_instance: str | None = None, max_results: int | None = 100, *, + expected_type: type[EntityT], + ) -> dict[EntityUri, EntityT]: ... + @overload + def query( + self, + entity_type: str, + obj: ApiRequestObjectType, + bfabric_instance: str | None = None, + max_results: int | None = 100, + ) -> dict[EntityUri, Entity]: ... + def query( + self, + entity_type: str | type[EntityT], + obj: ApiRequestObjectType, + bfabric_instance: str | None = None, + max_results: int | None = 100, + *, expected_type: type[EntityT] = Entity, ) -> dict[EntityUri, EntityT]: """Query entities by search criteria and return them as Entity objects. @@ -183,11 +256,12 @@ def query( Combines ``client.read()`` with automatic entity instantiation and caching. Args: - entity_type: B-Fabric entity type to query + entity_type: B-Fabric entity type — either the endpoint string (e.g. ``"sample"``) or an + entity class (e.g. ``Sample``), in which case the endpoint and ``expected_type`` are inferred obj: Dictionary of search criteria (e.g., ``{"name": "MySample"}``) bfabric_instance: B-Fabric instance URL (defaults to client's configured instance) max_results: Maximum number of results to return (``None`` for all) - expected_type: Entity class to validate and cast all results + expected_type: Entity class to validate and cast all results (ignored when a class is passed) Returns: Dictionary mapping entity URIs to their objects @@ -195,6 +269,7 @@ def query( Raises: TypeError: If any matched entity is not an instance of ``expected_type`` """ + entity_type, expected_type = _resolve_entity_type(entity_type, expected_type) bfabric_instance = bfabric_instance if bfabric_instance is not None else self._client.config.base_url # TODO limitation of the current implementation if bfabric_instance != self._client.config.base_url: @@ -215,12 +290,29 @@ def query( cache_stack.item_put_all(entities=entities.values()) return cast("dict[EntityUri, EntityT]", entities) + @overload + def query_one( + self, entity_type: type[EntityT], obj: ApiRequestObjectType, bfabric_instance: str | None = None + ) -> EntityT | None: ... + @overload def query_one( self, entity_type: str, obj: ApiRequestObjectType, bfabric_instance: str | None = None, *, + expected_type: type[EntityT], + ) -> EntityT | None: ... + @overload + def query_one( + self, entity_type: str, obj: ApiRequestObjectType, bfabric_instance: str | None = None + ) -> Entity | None: ... + def query_one( + self, + entity_type: str | type[EntityT], + obj: ApiRequestObjectType, + bfabric_instance: str | None = None, + *, expected_type: type[EntityT] = Entity, ) -> EntityT | None: """Query for a single entity by search criteria. @@ -229,10 +321,11 @@ def query_one( look-up-by-field pattern. Returns ``None`` if no match. Args: - entity_type: B-Fabric entity type to query + entity_type: B-Fabric entity type — either the endpoint string (e.g. ``"user"``) or an + entity class (e.g. ``User``), in which case the endpoint and ``expected_type`` are inferred obj: Dictionary of search criteria (e.g., ``{"login": "alice"}``) bfabric_instance: B-Fabric instance URL (defaults to client's configured instance) - expected_type: Entity class to validate and cast the result + expected_type: Entity class to validate and cast the result (ignored when a class is passed) Returns: Entity object (typed as ``expected_type``) or ``None`` if not found @@ -240,6 +333,7 @@ def query_one( Raises: TypeError: If the matched entity is not an instance of ``expected_type`` """ + entity_type, expected_type = _resolve_entity_type(entity_type, expected_type) results = self.query( entity_type, obj, bfabric_instance=bfabric_instance, max_results=1, expected_type=expected_type ) diff --git a/bfabric/src/bfabric/entities/core/import_entity.py b/bfabric/src/bfabric/entities/core/import_entity.py index dc9de2a0c..13cd18812 100644 --- a/bfabric/src/bfabric/entities/core/import_entity.py +++ b/bfabric/src/bfabric/entities/core/import_entity.py @@ -26,6 +26,16 @@ def import_entity(entity_class_name: str) -> type[Entity]: return Entity +def entity_type_of(entity_class: type[Entity]) -> str: + """The B-Fabric endpoint / ``classname`` string for an entity *class*. + + Class-level counterpart of the instance :attr:`Entity.classname` property, using the same + lowercase-class-name convention that :func:`import_entity` relies on for the string→class direction + (so the two are inverses). + """ + return entity_class.__name__.lower() + + def instantiate_entity(data_dict: ApiResponseObjectType, client: Bfabric | None, bfabric_instance: str) -> Entity: """Instantiates an entity given its data dictionary with the most specific class possible.""" entity_class_name = data_dict["classname"] diff --git a/bfabric/src/bfabric/entities/core/mixins/find_mixin.py b/bfabric/src/bfabric/entities/core/mixins/find_mixin.py index 5bf535546..1574afd15 100644 --- a/bfabric/src/bfabric/entities/core/mixins/find_mixin.py +++ b/bfabric/src/bfabric/entities/core/mixins/find_mixin.py @@ -1,10 +1,12 @@ from __future__ import annotations import warnings -from typing import TYPE_CHECKING, Protocol, TypeVar +from typing import TYPE_CHECKING, Protocol, TypeVar, cast from loguru import logger +from bfabric.entities.core.reader_utils import entities_by_id + if TYPE_CHECKING: from collections.abc import Mapping, Sequence @@ -40,13 +42,10 @@ def find_all(cls: type[T], ids: Sequence[int | str], client: Bfabric) -> dict[in warnings.warn( "FindMixin is deprecated and will be removed in future versions.", DeprecationWarning, stacklevel=2 ) - results = EntityReader.for_client(client=client).read_ids( - entity_type=cls.ENDPOINT, - entity_ids=list(ids), - expected_type=cls, # pyright: ignore[reportArgumentType] - ) - results_by_id = {uri.components.entity_id: item for uri, item in results.items() if item is not None} - return _ensure_results_order(ids, results_by_id) + results = EntityReader.for_client(client=client).read_ids(cls.ENDPOINT, list(ids)) + # ``entities_by_id`` is typed for ``Entity`` values, whereas ``FindMixin.T`` is the deprecated + # protocol-bound typevar; the runtime dict is correct, so bridge the two with a cast. + return _ensure_results_order(ids, cast("dict[int, T]", entities_by_id(results))) @classmethod def find_by( @@ -63,7 +62,7 @@ def find_by( results = reader.query( entity_type=cls.ENDPOINT, obj=obj, bfabric_instance=bfabric_instance, max_results=max_results ) - return {uri.components.entity_id: entity for uri, entity in results.items()} + return cast("dict[int, T]", entities_by_id(results)) def _ensure_results_order( diff --git a/bfabric/src/bfabric/entities/core/reader_utils.py b/bfabric/src/bfabric/entities/core/reader_utils.py new file mode 100644 index 000000000..8d56a88c6 --- /dev/null +++ b/bfabric/src/bfabric/entities/core/reader_utils.py @@ -0,0 +1,29 @@ +"""Small pure helpers for reshaping :class:`EntityReader` results. + +The reader methods (``read_ids``, ``read_uris``, ``query``) return entities keyed by +:class:`EntityUri` and, for the id/uri lookups, include ``None`` for not-found entries. These helpers +turn that raw result into the two shapes callers actually want, without each site re-implementing the +re-key / ``None``-filter by hand. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, TypeVar + +if TYPE_CHECKING: + from collections.abc import Mapping + + from bfabric.entities.core.entity import Entity + from bfabric.entities.core.uri import EntityUri + +T = TypeVar("T", bound="Entity") + + +def entities_by_id(result: Mapping[EntityUri, T | None]) -> dict[int, T]: + """Re-key an entity-reader result by integer entity id, dropping not-found (``None``) entries.""" + return {uri.components.entity_id: entity for uri, entity in result.items() if entity is not None} + + +def present_entities(result: Mapping[EntityUri, T | None]) -> list[T]: + """Return the found entities from an entity-reader result, dropping not-found (``None``) entries.""" + return [entity for entity in result.values() if entity is not None] diff --git a/bfabric/src/bfabric/entities/core/users.py b/bfabric/src/bfabric/entities/core/users.py index 1de98338a..c01893c92 100644 --- a/bfabric/src/bfabric/entities/core/users.py +++ b/bfabric/src/bfabric/entities/core/users.py @@ -41,9 +41,7 @@ def get_by_login(self, bfabric_instance: str, login: str) -> User | None: return user # retrieve - user = self._entity_reader.query_one( - "user", {"login": login}, bfabric_instance=bfabric_instance, expected_type=UserEntity - ) + user = self._entity_reader.query_one(UserEntity, {"login": login}, bfabric_instance=bfabric_instance) if user is None: return None diff --git a/bfabric/src/bfabric/entities/user.py b/bfabric/src/bfabric/entities/user.py index 998d538aa..336b6dc67 100644 --- a/bfabric/src/bfabric/entities/user.py +++ b/bfabric/src/bfabric/entities/user.py @@ -13,7 +13,7 @@ class User(Entity): @classmethod def find_by_login(cls, login: str, client: Bfabric) -> User | None: """Finds a user by their login name.""" - return client.reader.query_one("user", {"login": login}, expected_type=cls) + return client.reader.query_one(cls, {"login": login}) @property def is_employee(self) -> bool: diff --git a/bfabric/src/bfabric/experimental/multi_query.py b/bfabric/src/bfabric/experimental/multi_query.py index e6c175d2d..c9424a98b 100644 --- a/bfabric/src/bfabric/experimental/multi_query.py +++ b/bfabric/src/bfabric/experimental/multi_query.py @@ -2,6 +2,8 @@ from copy import deepcopy +from loguru import logger + from bfabric.results.result_container import ResultContainer from bfabric.utils.paginator import page_iter from typing import TYPE_CHECKING @@ -82,7 +84,7 @@ def delete_multi(self, endpoint: str, id_list: list[int]) -> ResultContainer: response_tot = ResultContainer([], total_pages_api=0, errors=[]) if not id_list: - print("Warning, empty list provided for deletion, ignoring") + logger.warning("empty list provided for deletion, ignoring") return response_tot # Iterate over request chunks that fit into a single API page @@ -100,7 +102,7 @@ def exists_multi(self, endpoint: str, key: str, value: list[int | str] | int | s :return: Return a single bool or a list of bools for each value For each value, test if a key with that value is found in the API. """ - is_scalar = isinstance(value, (int, str)) + is_scalar = isinstance(value, int | str) if is_scalar: return self._client.exists(endpoint=endpoint, key=key, value=value, check=True) elif not isinstance(value, list): diff --git a/bfabric/src/bfabric/operations/dataset/operations.py b/bfabric/src/bfabric/operations/dataset/operations.py index 3c45f9792..63d3cb858 100644 --- a/bfabric/src/bfabric/operations/dataset/operations.py +++ b/bfabric/src/bfabric/operations/dataset/operations.py @@ -76,7 +76,7 @@ def preview_dataset_update( Does not write. Intended for interactive flows that want to confirm before calling `update_dataset`. """ - existing = client.reader.read_id("dataset", dataset_id, expected_type=Dataset) + existing = client.reader.read_id(Dataset, dataset_id) if existing is None: raise RuntimeError(f"Dataset {dataset_id} not found") changes = identify_changes(old_df=existing.to_polars(), new_df=table) diff --git a/bfabric/src/bfabric/results/result_container.py b/bfabric/src/bfabric/results/result_container.py index f1a7ad206..35ff06e1f 100644 --- a/bfabric/src/bfabric/results/result_container.py +++ b/bfabric/src/bfabric/results/result_container.py @@ -5,13 +5,13 @@ from loguru import logger import bfabric.results.response_format_dict as formatter +from bfabric.errors import BfabricRequestError if TYPE_CHECKING: from collections.abc import Iterator import polars - from bfabric.errors import BfabricRequestError from bfabric.typing import ApiResponseObjectType @@ -72,7 +72,7 @@ def assert_success(self) -> None: """Asserts that the query was successful. Raises a `RuntimeError` if it was not.""" if not self.is_success: details = "; ".join(str(e) for e in self._errors) - raise RuntimeError(f"Query was not successful: {details}") + raise BfabricRequestError(f"Query was not successful: {details}") @property def is_success(self) -> bool: diff --git a/bfabric/src/bfabric/utils/cli_integration.py b/bfabric/src/bfabric/utils/cli_integration.py index 647d92846..02d155386 100644 --- a/bfabric/src/bfabric/utils/cli_integration.py +++ b/bfabric/src/bfabric/utils/cli_integration.py @@ -5,7 +5,7 @@ import os import sys from pathlib import Path -from typing import TYPE_CHECKING, Annotated, TypeVar, cast +from typing import TYPE_CHECKING, Annotated, ClassVar, TypeVar, cast from loguru import logger from rich.highlighter import RegexHighlighter @@ -14,7 +14,7 @@ from bfabric.config import DEFAULT_CONFIG_FILE if TYPE_CHECKING: - from collections.abc import Callable + from collections.abc import Callable, Sequence T = TypeVar("T") @@ -113,8 +113,8 @@ def wrapper(*args: object, **kwargs: object) -> T: class HostnameHighlighter(RegexHighlighter): """Highlights hostnames in URLs.""" - base_style = "bfabric." - highlights = [r"https://(?P[^.]+)"] + base_style: ClassVar[str] = "bfabric." + highlights: ClassVar[Sequence[str]] = [r"https://(?P[^.]+)"] def setup_script_logging(debug: bool = False) -> None: @@ -143,9 +143,11 @@ def setup_script_logging(debug: bool = False) -> None: logger.disable(package) elif level == "DEBUG": for package in packages: + logger.enable(package) _ = logger.add(sys.stderr, filter=package, level="DEBUG") else: for package in packages: + logger.enable(package) _ = logger.add(sys.stderr, filter=package, level=level, format="{level} {message}") os.environ[setup_flag_key] = "1" diff --git a/bfabric_app_runner/docs/changelog.md b/bfabric_app_runner/docs/changelog.md index 63aeb11ea..0522ad33c 100644 --- a/bfabric_app_runner/docs/changelog.md +++ b/bfabric_app_runner/docs/changelog.md @@ -4,6 +4,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ## \[Unreleased\] +- Internal: migrated all read-path call sites off the deprecated `Entity.find`/`find_all`/`find_by` (`FindMixin`) API onto the modern `client.reader` (`read_id`/`read_ids`/`query_one`); no behavior change. + ## \[0.7.0rc1\] - 2026-07-15 - `SaveDatasetSpec` (the `bfabric_dataset` output) gains a `format` field (`csv` default, or `parquet`), so an output dataset can be registered from Parquet; `separator` is now optional (csv-only) ([#359](https://github.com/fgcz/bfabricPy/issues/359)). diff --git a/bfabric_app_runner/src/bfabric_app_runner/actions/execute.py b/bfabric_app_runner/src/bfabric_app_runner/actions/execute.py index dbf0d8bc9..a82a034da 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/actions/execute.py +++ b/bfabric_app_runner/src/bfabric_app_runner/actions/execute.py @@ -148,7 +148,7 @@ def _register_workflow_step( # Load the workflow template step. A misconfigured step id (or a step without a workflow template) is a # hard error: raising here aborts output registration before the workunit is finalized to 'available', # rather than silently skipping the workflow-step linkage. - workflow_template_step = WorkflowTemplateStep.find(id=workflow_template_step_id, client=client) + workflow_template_step = client.reader.read_id(WorkflowTemplateStep, workflow_template_step_id) if not workflow_template_step: raise ValueError(f"Misconfigured workflow_template_step_id={workflow_template_step_id!r}: not found.") @@ -182,8 +182,8 @@ def _register_workflow_step( def _create_workflow_step_if_not_exists( workflow: dict[str, Any], workflow_step_data: dict[str, Any], workunit_id: int, client: Bfabric ) -> None: - workflow_step = WorkflowStep.find_by(workflow_step_data, client=client) - if workflow_step: + workflow_step = client.reader.query_one(WorkflowStep, workflow_step_data) + if workflow_step is not None: logger.info(f"Workflow step already exists: {workflow_step}, skipping creation.") return resp = client.save("workflowstep", workflow_step_data) diff --git a/bfabric_app_runner/src/bfabric_app_runner/bfabric_integration/slurm.py b/bfabric_app_runner/src/bfabric_app_runner/bfabric_integration/slurm.py index a1436f372..361e9a9bd 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/bfabric_integration/slurm.py +++ b/bfabric_app_runner/src/bfabric_app_runner/bfabric_integration/slurm.py @@ -37,12 +37,12 @@ def submitter( """Submitter implementation for simple_submitter.""" if entity_type == "externaljob": # Find the workunit to process - external_job = ExternalJob.find(id=j, client=client) + external_job = client.reader.read_id(ExternalJob, j) workunit = external_job.workunit if workunit is None: raise RuntimeError(f"External job {j} does not belong to a workunit (or it was deleted).") else: - workunit = Workunit.find(id=j, client=client) + workunit = client.reader.read_id(Workunit, j) _submit_workunit(workunit=workunit, config_path=config_path) diff --git a/bfabric_app_runner/src/bfabric_app_runner/commands/command_python_env.py b/bfabric_app_runner/src/bfabric_app_runner/commands/command_python_env.py index badef716a..38c6acb13 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/commands/command_python_env.py +++ b/bfabric_app_runner/src/bfabric_app_runner/commands/command_python_env.py @@ -46,7 +46,10 @@ def log_packages(self, level: str = "DEBUG") -> None: @cached_property def _uv_bin(self) -> str: - return shutil.which("uv") + uv = shutil.which("uv") + if uv is None: + raise RuntimeError("uv executable not found on PATH") + return uv def _create_virtual_environment(self) -> None: """Create a virtual environment using uv venv.""" diff --git a/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_individual_resources.py b/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_individual_resources.py index cbf219abd..ced7233c9 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_individual_resources.py +++ b/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_individual_resources.py @@ -1,6 +1,6 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Any +from typing import TYPE_CHECKING, Any, cast from bfabric.entities import Dataset, Resource from pydantic import BaseModel, ConfigDict, model_validator @@ -91,14 +91,12 @@ def _dispatch_jobs_dataset_flow(self, definition: WorkunitDefinition, params: di config = self._config.dataset_flow if config is None: raise ValueError("dataset_flow is not configured") - dataset = Dataset.find(id=definition.execution.dataset, client=self._client) + dataset = self._client.reader.read_id(Dataset, cast("int", definition.execution.dataset)) if dataset is None: msg = f"Dataset with id {definition.execution.dataset} not found" raise ValueError(msg) dataset_df = dataset.to_polars() - resources = self._client.reader.read_ids( - "resource", dataset_df[config.resource_column].unique().to_list(), expected_type=Resource - ) + resources = self._client.reader.read_ids(Resource, dataset_df[config.resource_column].unique().to_list()) resources_by_id = {uri.components.entity_id: resource for uri, resource in resources.items()} paths = [] for row in dataset_df.iter_rows(named=True): diff --git a/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_resource_flow.py b/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_resource_flow.py index 89cd37d47..5449b23b1 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_resource_flow.py +++ b/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_resource_flow.py @@ -7,6 +7,7 @@ import yaml from bfabric import Bfabric from bfabric.entities import Resource +from bfabric.entities.core.reader_utils import present_entities from bfabric.experimental.workunit_definition import WorkunitDefinition from bfabric.utils.cli_integration import use_client from pandera import Field @@ -86,7 +87,7 @@ def dispatch( @staticmethod def _build_input_resources_df(resource_ids: list[int], client: Bfabric) -> DataFrame[InputTable]: """Creates the InputTable DataFrame from the list of resource IDs.""" - resources = Resource.find_all(resource_ids, client=client).values() + resources = present_entities(client.reader.read_ids(Resource, resource_ids)) if not resources: raise ValueError("No resources to dispatch") attributes = [ diff --git a/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_single_dataset_flow.py b/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_single_dataset_flow.py index 08613a0f2..5559b2ab9 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_single_dataset_flow.py +++ b/bfabric_app_runner/src/bfabric_app_runner/dispatch/dispatch_single_dataset_flow.py @@ -21,6 +21,6 @@ def dispatch_workunit(self, workunit: WorkunitDefinition) -> None: logger.error("No dataset found in workunit.") return - dataset = Dataset.find(id=workunit.execution.dataset, client=self._client) + dataset = self._client.reader.read_id(Dataset, workunit.execution.dataset) path = self.dispatch_job(dataset=dataset, workunit=workunit) write_chunks_file(out_dir=self._out_dir, chunks=[path]) diff --git a/bfabric_app_runner/src/bfabric_app_runner/dispatch/resource_flow.py b/bfabric_app_runner/src/bfabric_app_runner/dispatch/resource_flow.py index 5841d4b53..7cb454617 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/dispatch/resource_flow.py +++ b/bfabric_app_runner/src/bfabric_app_runner/dispatch/resource_flow.py @@ -10,7 +10,7 @@ def get_resource_flow_input_resources( filter_suffix: str | None, ) -> list[Resource]: """Returns the input resources for a resource flow workunit, applying e.g. a filter suffix.""" - all_resources = client.reader.read_ids("resource", definition.execution.resources, expected_type=Resource) + all_resources = client.reader.read_ids(Resource, definition.execution.resources) result_resources: list[Resource] = [] for resource_uri, resource in all_resources.items(): if resource is None: diff --git a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_dataset_specs.py b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_dataset_specs.py index d961337d8..071406978 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_dataset_specs.py +++ b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_dataset_specs.py @@ -5,24 +5,26 @@ from bfabric_app_runner.inputs.resolve.resolved_inputs import ResolvedStaticFile from bfabric.entities import Dataset +from bfabric.entities.core.reader_utils import entities_by_id if TYPE_CHECKING: from bfabric_app_runner.specs.inputs.bfabric_dataset_spec import BfabricDatasetSpec - from bfabric import Bfabric + from bfabric.entities.core.entity_reader import EntityReader class ResolveBfabricDatasetSpecs: - def __init__(self, client: Bfabric) -> None: - self._client = client + def __init__(self, reader: EntityReader) -> None: + self._reader = reader def __call__(self, specs: list[BfabricDatasetSpec]) -> list[ResolvedStaticFile]: """Convert dataset specifications to file specifications.""" if not specs: return [] - # Fetch all datasets in bulk + # Fetch all datasets in bulk, re-keyed by id (missing ids are dropped, so a downstream + # ``datasets[dataset_id]`` raises ``KeyError`` for a not-found dataset, as before). dataset_ids = [spec.id for spec in specs] - datasets = Dataset.find_all(ids=dataset_ids, client=self._client) + datasets = entities_by_id(self._reader.read_ids(Dataset, dataset_ids)) # Resolve each dataset specification to its serialized content return [ diff --git a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_order_fasta_specs.py b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_order_fasta_specs.py index af92b1549..a997d7e3d 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_order_fasta_specs.py +++ b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_order_fasta_specs.py @@ -27,7 +27,7 @@ def _get_order_fasta(self, spec: BfabricOrderFastaSpec) -> str: """Extract FASTA sequence from an order or workunit.""" # find the order if spec.entity == "workunit": - workunit = Workunit.find(id=spec.id, client=self._client) + workunit = self._client.reader.read_id("workunit", spec.id, expected_type=Workunit) if not isinstance(workunit.container, Order): msg = f"Workunit {spec.id} is not associated with an order" if spec.required: @@ -36,7 +36,7 @@ def _get_order_fasta(self, spec: BfabricOrderFastaSpec) -> str: return "" order = workunit.container elif spec.entity == "order": - order = Order.find(id=spec.id, client=self._client) + order = self._client.reader.read_id("order", spec.id, expected_type=Order) if order is None: msg = f"Order {spec.id} not found" if spec.required: diff --git a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_archive_specs.py b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_archive_specs.py index 6f5bbdbc0..b280d6aa1 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_archive_specs.py +++ b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_archive_specs.py @@ -49,7 +49,7 @@ def __call__(self, specs: list[BfabricResourceArchiveSpec]) -> list[ResolvedDire def _get_resources(self, specs: list[BfabricResourceArchiveSpec]) -> dict[EntityUri, Resource]: resource_ids = [spec.id for spec in specs] - resources = self._reader.read_ids("resource", resource_ids, expected_type=Resource) + resources = self._reader.read_ids(Resource, resource_ids) if not _no_none_value(resources): raise ValueError("Some resource IDs are invalid") return resources diff --git a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_dataset_specs.py b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_dataset_specs.py index ab2caddbc..7f5e5cbee 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_dataset_specs.py +++ b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_dataset_specs.py @@ -7,18 +7,19 @@ import polars as pl from bfabric.entities import Dataset, Resource +from bfabric.entities.core.reader_utils import present_entities from bfabric_app_runner.inputs._filter_files import filter_dataframe from bfabric_app_runner.inputs.resolve._common import get_ssh_file_source from bfabric_app_runner.inputs.resolve.resolved_inputs import ResolvedFile, ResolvedStaticFile if TYPE_CHECKING: - from bfabric import Bfabric - from bfabric_app_runner.specs.inputs.bfabric_resource_dataset import BfabricResourceDatasetSpec + from bfabric.entities.core.entity_reader import EntityReader + from bfabric_app_runner.specs.inputs.bfabric_resource_dataset_spec import BfabricResourceDatasetSpec class ResolveBfabricResourceDatasetSpecs: - def __init__(self, client: Bfabric) -> None: - self._client = client + def __init__(self, reader: EntityReader) -> None: + self._reader = reader def __call__(self, specs: list[BfabricResourceDatasetSpec]) -> list[ResolvedFile | ResolvedStaticFile]: # Note: We process each spec individually here, this could be optimized should it become necessary @@ -82,7 +83,8 @@ def _resolve_unfiltered_dataset(self, spec: BfabricResourceDatasetSpec) -> pl.Da - tmp_resource_source: The source (URL or local path) of the resource """ # Obtain dataset information - data = Dataset.find(id=spec.id, client=self._client).to_polars() + dataset = self._reader.read_id(Dataset, spec.id) + data = dataset.to_polars() # pyright: ignore[reportOptionalMemberAccess] input_column = self._select_input_column(data, spec.column) data = data.with_columns(pl.col(input_column).cast(pl.Int64)) @@ -96,7 +98,7 @@ def _resolve_unfiltered_dataset(self, spec: BfabricResourceDatasetSpec) -> pl.Da "tmp_resource_relative_path": r.storage_relative_path, "tmp_resource_source": get_ssh_file_source(r), } - for r in Resource.find_all(ids=resource_ids, client=self._client).values() + for r in present_entities(self._reader.read_ids(Resource, resource_ids)) ] # Merge diff --git a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_specs.py b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_specs.py index 8b039ecd8..84a2359e3 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_specs.py +++ b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/_resolve_bfabric_resource_specs.py @@ -3,6 +3,7 @@ from typing import TYPE_CHECKING, final from bfabric.entities import Resource +from bfabric.entities.core.reader_utils import entities_by_id from bfabric_app_runner.inputs.resolve._common import get_http_file_source, get_ssh_file_source from bfabric_app_runner.inputs.resolve.resolved_inputs import ResolvedFile @@ -23,9 +24,11 @@ def __call__(self, specs: list[BfabricResourceSpec]) -> list[ResolvedFile]: if not specs: return [] - # Fetch all resources and their storage information in bulk + # Fetch all resources and their storage information in bulk, re-keyed by id (missing ids are + # dropped, so a downstream ``resources[resource_id]`` raises ``KeyError`` for a not-found + # resource, as before). resource_ids = [spec.id for spec in specs] - resources = Resource.find_all(ids=resource_ids, client=self._client) + resources = entities_by_id(self._client.reader.read_ids(Resource, resource_ids)) # Create the file specs result: list[ResolvedFile] = [] diff --git a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/resolver.py b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/resolver.py index cfa5aaf0e..28545434c 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/resolver.py +++ b/bfabric_app_runner/src/bfabric_app_runner/inputs/resolve/resolver.py @@ -1,7 +1,7 @@ from __future__ import annotations from collections import defaultdict -from typing import TYPE_CHECKING, assert_never +from typing import TYPE_CHECKING, get_args from bfabric.entities.cache.context import cache_entities from bfabric_app_runner.inputs.resolve._resolve_bfabric_annotation_specs import ResolveBfabricAnnotationSpecs @@ -13,72 +13,80 @@ from bfabric_app_runner.inputs.resolve._resolve_file_specs import ResolveFileSpecs from bfabric_app_runner.inputs.resolve._resolve_static_file_specs import ResolveStaticFileSpecs from bfabric_app_runner.inputs.resolve._resolve_static_yaml_specs import ResolveStaticYamlSpecs -from bfabric_app_runner.inputs.resolve.resolved_inputs import ResolvedInputs -from bfabric_app_runner.specs.inputs.bfabric_annotation_spec import ( - BfabricAnnotationSpec, -) +from bfabric_app_runner.inputs.resolve.resolved_inputs import ResolvedInput, ResolvedInputs +from bfabric_app_runner.specs.inputs.bfabric_annotation_spec import BfabricAnnotationSpec from bfabric_app_runner.specs.inputs.bfabric_dataset_spec import BfabricDatasetSpec from bfabric_app_runner.specs.inputs.bfabric_order_fasta_spec import BfabricOrderFastaSpec from bfabric_app_runner.specs.inputs.bfabric_resource_archive_spec import BfabricResourceArchiveSpec -from bfabric_app_runner.specs.inputs.bfabric_resource_dataset import BfabricResourceDatasetSpec +from bfabric_app_runner.specs.inputs.bfabric_resource_dataset_spec import BfabricResourceDatasetSpec from bfabric_app_runner.specs.inputs.bfabric_resource_spec import BfabricResourceSpec from bfabric_app_runner.specs.inputs.file_spec import FileSpec from bfabric_app_runner.specs.inputs.static_file_spec import StaticFileSpec from bfabric_app_runner.specs.inputs.static_yaml_spec import StaticYamlSpec +from bfabric_app_runner.specs.inputs_spec import InputSpecType if TYPE_CHECKING: + from collections.abc import Callable, Sequence + from bfabric import Bfabric - from bfabric_app_runner.specs.inputs_spec import InputSpecType class Resolver: """Resolves input specifications into standardized file specifications.""" def __init__(self, client: Bfabric) -> None: - self._client = client - self._resolve_bfabric_dataset_specs = ResolveBfabricDatasetSpecs(client=client) - self._resolve_bfabric_resource_specs = ResolveBfabricResourceSpecs(client=client) - self._resolve_bfabric_resource_archive_specs = ResolveBfabricResourceArchiveSpecs(client=client) - self._resolve_bfabric_resource_dataset_specs = ResolveBfabricResourceDatasetSpecs(client=client) - self._resolve_static_yaml_specs = ResolveStaticYamlSpecs() - self._resolve_static_file_specs = ResolveStaticFileSpecs() - self._resolve_bfabric_order_fasta_specs = ResolveBfabricOrderFastaSpecs(client=client) - self._resolve_bfabric_annotation_specs = ResolveBfabricAnnotationSpecs(client=client) - self._resolve_file_specs = ResolveFileSpecs() + self._client: Bfabric = client + # Maps each concrete input-spec class to the resolver handling it. Adding an input type is a + # single new entry here (plus its spec model, InputSpecType union member, and resolver class). + # The parameter type of each resolver is elided (``...``) because each accepts a different + # spec subtype; the registry keys carry the routing information instead. + self._registry: dict[type, Callable[..., Sequence[ResolvedInput]]] = { + StaticYamlSpec: ResolveStaticYamlSpecs(), + StaticFileSpec: ResolveStaticFileSpecs(), + FileSpec: ResolveFileSpecs(), + BfabricResourceSpec: ResolveBfabricResourceSpecs(client=client), + BfabricResourceArchiveSpec: ResolveBfabricResourceArchiveSpecs(client=client), + BfabricResourceDatasetSpec: ResolveBfabricResourceDatasetSpecs(reader=client.reader), + BfabricDatasetSpec: ResolveBfabricDatasetSpecs(reader=client.reader), + BfabricOrderFastaSpec: ResolveBfabricOrderFastaSpecs(client=client), + BfabricAnnotationSpec: ResolveBfabricAnnotationSpecs(client=client), + } + self._check_registry_exhaustive() def resolve(self, specs: list[InputSpecType]) -> ResolvedInputs: """Convert input specifications to resolved file specifications.""" with cache_entities(entities=["application", "dataset", "resource", "storage"], max_size=500): - grouped_specs = self._group_specs_by_type(specs=specs) - files = [] - for spec_type, specs_list in grouped_specs.items(): - if issubclass(spec_type, StaticYamlSpec): - files.extend(self._resolve_static_yaml_specs(specs_list)) - elif issubclass(spec_type, StaticFileSpec): - files.extend(self._resolve_static_file_specs(specs_list)) - elif issubclass(spec_type, BfabricResourceSpec): - files.extend(self._resolve_bfabric_resource_specs(specs_list)) - elif issubclass(spec_type, BfabricResourceArchiveSpec): - files.extend(self._resolve_bfabric_resource_archive_specs(specs_list)) - elif issubclass(spec_type, BfabricResourceDatasetSpec): - files.extend(self._resolve_bfabric_resource_dataset_specs(specs_list)) - elif issubclass(spec_type, BfabricDatasetSpec): - files.extend(self._resolve_bfabric_dataset_specs(specs_list)) - elif issubclass(spec_type, BfabricOrderFastaSpec): - files.extend(self._resolve_bfabric_order_fasta_specs(specs_list)) - elif issubclass(spec_type, BfabricAnnotationSpec): - files.extend(self._resolve_bfabric_annotation_specs(specs_list)) - elif issubclass(spec_type, FileSpec): - files.extend(self._resolve_file_specs(specs_list)) - else: - assert_never(spec_type) - + files: list[ResolvedInput] = [] + for spec_type, specs_list in self._group_specs_by_type(specs=specs).items(): + files.extend(self._resolver_for(spec_type)(specs_list)) return ResolvedInputs(files=files) + def _resolver_for(self, spec_type: type) -> Callable[..., Sequence[ResolvedInput]]: + """Return the resolver for a spec class, walking the MRO on an exact-key miss. + + The MRO walk (``issubclass``, not exact equality) keeps dispatch working if a registered base + class (e.g. the ``BfabricAnnotationSpec`` family) ever becomes a union of concrete subtypes. + """ + resolver = self._registry.get(spec_type) + if resolver is not None: + return resolver + for base, candidate in self._registry.items(): + if issubclass(spec_type, base): + return candidate + raise TypeError(f"No resolver registered for input spec type {spec_type!r}") + + def _check_registry_exhaustive(self) -> None: + """Fail loudly at construction if any ``InputSpecType`` member has no registered resolver.""" + # InputSpecType is ``Annotated[, Field(...)]``; unwrap the annotation, then the union. + members: tuple[type, ...] = get_args(get_args(InputSpecType)[0]) + missing = [member for member in members if not any(issubclass(member, base) for base in self._registry)] + if missing: + raise TypeError(f"Resolver registry is missing entries for input spec types: {missing!r}") + @staticmethod - def _group_specs_by_type(specs: list[InputSpecType]) -> dict[type, list]: - """Group specifications by their type.""" - grouped = defaultdict(list) + def _group_specs_by_type(specs: list[InputSpecType]) -> dict[type, list[InputSpecType]]: + """Group specifications by their concrete type.""" + grouped: dict[type, list[InputSpecType]] = defaultdict(list) for spec in specs: grouped[type(spec)].append(spec) return grouped diff --git a/bfabric_app_runner/src/bfabric_app_runner/output_registration/register.py b/bfabric_app_runner/src/bfabric_app_runner/output_registration/register.py index ceb9df835..006f9b8e7 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/output_registration/register.py +++ b/bfabric_app_runner/src/bfabric_app_runner/output_registration/register.py @@ -88,15 +88,15 @@ def _identify_existing_resource_id( if spec.update_existing in (UpdateExisting.IF_EXISTS, UpdateExisting.REQUIRED): # TODO maybe it would be more accurate to use relativepath here, however historically it would often start # with `/` which can be confusing. - resources = Resource.find_by( + resource = client.reader.query_one( + Resource, { "name": spec.store_entry_path.name, "workunitid": workunit_definition.registration.workunit_id, }, - client=client, - ).values() - if resources: - return list(resources)[0].id + ) + if resource is not None: + return resource.id elif spec.update_existing == UpdateExisting.REQUIRED: raise ValueError(f"Resource {spec.store_entry_path.name} not found in workunit {workunit_definition.id}") return None @@ -167,7 +167,7 @@ def _save_dataset(spec: SaveDatasetSpec, client: Bfabric, workunit_definition: W table = _read_dataset_table(spec) check_for_invalid_characters(table=table, invalid_characters=spec.invalid_characters) - existing = client.reader.query_one("dataset", {"workunitid": registration.workunit_id}, expected_type=Dataset) + existing = client.reader.query_one(Dataset, {"workunitid": registration.workunit_id}) _check_update_existing_policy( existing is not None, spec.update_existing, @@ -230,7 +230,8 @@ def _save_link(spec: SaveLinkSpec, client: Bfabric, workunit_definition: Workuni def find_default_resource_id(workunit_definition: WorkunitDefinition, client: Bfabric) -> int | None: """Finds the default resource's id for the workunit. Maybe in the future, this will be always `None`.""" - workunit = Workunit.find(id=workunit_definition.registration.workunit_id, client=client) + workunit_id = workunit_definition.registration.workunit_id # pyright: ignore[reportOptionalMemberAccess] + workunit = client.reader.read_id(Workunit, workunit_id) candidate_resources = [ resource for resource in workunit.resources if resource["name"] not in ["slurm_stdout", "slurm_stderr"] ] @@ -287,7 +288,8 @@ def _get_storage( ) -> Storage | None: if any(isinstance(spec, CopyResourceSpec) for spec in specs_list): if force_storage is None: - return Storage.find(workunit_definition.registration.storage_id, client=client) + storage_id = workunit_definition.registration.storage_id # pyright: ignore[reportOptionalMemberAccess] + return client.reader.read_id(Storage, storage_id) else: return Storage(yaml.safe_load(force_storage.read_text()), client=client) # TODO replace this later (to avoid versioning issues, i hardcode the above) diff --git a/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_annotation_spec.py b/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_annotation_spec.py index f422f00b6..75385f4d8 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_annotation_spec.py +++ b/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_annotation_spec.py @@ -1,12 +1,9 @@ from __future__ import annotations -from typing import Literal, TYPE_CHECKING, Annotated +from typing import Literal from bfabric_app_runner.specs.common_types import RelativeFilePath -from pydantic import BaseModel, Field - -if TYPE_CHECKING: - from bfabric import Bfabric +from pydantic import BaseModel class _AnnotationSpec(BaseModel): @@ -18,10 +15,6 @@ class _AnnotationSpec(BaseModel): filename: RelativeFilePath """Target filename (relative to the chunk directory) to write the annotation table to.""" - def resolve_filename(self, client: Bfabric) -> str: - """Returns the target filename for this annotation.""" - return self.filename - class BfabricAnnotationResourceSampleSpec(_AnnotationSpec): """Annotation table joining each given resource with its associated sample, one row per resource.""" @@ -40,4 +33,3 @@ class BfabricAnnotationResourceSampleSpec(_AnnotationSpec): BfabricAnnotationSpec = BfabricAnnotationResourceSampleSpec -BfabricAnnotationSpecField = Annotated[BfabricAnnotationSpec, Field(discriminator="annotation")] diff --git a/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_order_fasta_spec.py b/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_order_fasta_spec.py index 776c3f90e..468f50775 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_order_fasta_spec.py +++ b/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_order_fasta_spec.py @@ -1,13 +1,10 @@ from __future__ import annotations -from typing import Literal, TYPE_CHECKING +from typing import Literal from pydantic import BaseModel, ConfigDict from bfabric_app_runner.specs.common_types import RelativeFilePath -if TYPE_CHECKING: - from bfabric import Bfabric - class BfabricOrderFastaSpec(BaseModel): """Writes the FASTA sequence attached to a B-Fabric order to a file.""" @@ -27,7 +24,3 @@ class BfabricOrderFastaSpec(BaseModel): required: bool = False """If True, a missing order or FASTA sequence raises an error; otherwise an empty file is written.""" - - def resolve_filename(self, client: Bfabric) -> str: - """Returns the target filename for this input.""" - return self.filename diff --git a/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_resource_dataset.py b/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_resource_dataset_spec.py similarity index 100% rename from bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_resource_dataset.py rename to bfabric_app_runner/src/bfabric_app_runner/specs/inputs/bfabric_resource_dataset_spec.py diff --git a/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/file_spec.py b/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/file_spec.py index 4ea559985..03d76a01f 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/file_spec.py +++ b/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/file_spec.py @@ -1,13 +1,10 @@ from __future__ import annotations -from typing import Literal, TYPE_CHECKING, Self +from typing import Literal, Self from bfabric_app_runner.specs.common_types import RelativeFilePath, AbsoluteFilePath from pydantic import BaseModel, model_validator -if TYPE_CHECKING: - from bfabric import Bfabric - class FileSourceLocal(BaseModel): """A file already present on the local filesystem of the machine running the app.""" @@ -104,7 +101,3 @@ def validate_no_user_supplied_auth(self) -> Self: def get_filename(self) -> str: """Returns the filename, extracting it from the source if it was omitted.""" return self.filename if self.filename else self.source.get_filename() - - def resolve_filename(self, client: Bfabric) -> str: - # TODO delete - return self.get_filename() diff --git a/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/static_file_spec.py b/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/static_file_spec.py index 1f42805e3..eca540246 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/static_file_spec.py +++ b/bfabric_app_runner/src/bfabric_app_runner/specs/inputs/static_file_spec.py @@ -1,11 +1,8 @@ from __future__ import annotations -from typing import TYPE_CHECKING, Literal +from typing import Literal from pydantic import BaseModel -if TYPE_CHECKING: - from bfabric import Bfabric - class StaticFileSpec(BaseModel): """Writes inline text or binary content, provided in the spec itself, to a local file.""" @@ -16,6 +13,3 @@ class StaticFileSpec(BaseModel): """The text or binary content to write.""" filename: str """The target filename to write to.""" - - def resolve_filename(self, client: Bfabric) -> str: - return self.filename diff --git a/bfabric_app_runner/src/bfabric_app_runner/specs/inputs_spec.py b/bfabric_app_runner/src/bfabric_app_runner/specs/inputs_spec.py index a9240465e..82dbcbe92 100644 --- a/bfabric_app_runner/src/bfabric_app_runner/specs/inputs_spec.py +++ b/bfabric_app_runner/src/bfabric_app_runner/specs/inputs_spec.py @@ -9,7 +9,7 @@ from bfabric_app_runner.specs.inputs.bfabric_dataset_spec import BfabricDatasetSpec from bfabric_app_runner.specs.inputs.bfabric_order_fasta_spec import BfabricOrderFastaSpec from bfabric_app_runner.specs.inputs.bfabric_resource_archive_spec import BfabricResourceArchiveSpec -from bfabric_app_runner.specs.inputs.bfabric_resource_dataset import BfabricResourceDatasetSpec +from bfabric_app_runner.specs.inputs.bfabric_resource_dataset_spec import BfabricResourceDatasetSpec from bfabric_app_runner.specs.inputs.bfabric_resource_spec import BfabricResourceSpec from bfabric_app_runner.specs.inputs.file_spec import FileSpec from bfabric_app_runner.specs.inputs.static_file_spec import StaticFileSpec diff --git a/bfabric_scripts/src/bfabric_scripts/cli/api/parser.py b/bfabric_scripts/src/bfabric_scripts/cli/api/parser.py index ad7d1f6ff..2b307d355 100644 --- a/bfabric_scripts/src/bfabric_scripts/cli/api/parser.py +++ b/bfabric_scripts/src/bfabric_scripts/cli/api/parser.py @@ -16,6 +16,7 @@ from suds.xsd.query import TypeQuery # pyright: ignore[reportMissingTypeStubs] from bfabric import Bfabric +from bfabric.config.bfabric_client_config import BfabricAPIEngineType from bfabric_scripts.cli.api.namespaces import NAMESPACES @@ -61,10 +62,19 @@ def parse_method_signature( Dictionary mapping parameter names to ParameterModel instances. Raises: + RuntimeError: If the client is not configured to use the SUDS engine. AttributeError: If endpoint or method doesn't exist """ + # WSDL introspection below is written against SUDS internals and has no Zeep + # equivalent, so reject other engines up front with a clear message instead of + # leaking an AttributeError from the private engine internals. + if client.config.engine != BfabricAPIEngineType.SUDS: + raise RuntimeError( + f"'api inspect' is only supported with the SUDS engine (got: {client.config.engine}). " + f"Set engine: SUDS in your bfabricpy config." + ) + # Get the SUDS service - # Note: This only works with EngineSuds, not EngineZeep service = client._engine._get_suds_service(endpoint) # type: ignore[attr-defined] # pyright: ignore[reportPrivateUsage,reportAttributeAccessIssue,reportUnknownVariableType,reportUnknownMemberType] # Get the specified method diff --git a/tests/bfabric/conftest.py b/tests/bfabric/conftest.py index f456dc414..bd9fbc415 100644 --- a/tests/bfabric/conftest.py +++ b/tests/bfabric/conftest.py @@ -1,5 +1,6 @@ import os import pytest +from loguru import logger def pytest_runtest_setup() -> None: @@ -7,6 +8,14 @@ def pytest_runtest_setup() -> None: os.environ["BFABRICPY_CONFIG_ENV"] = "__MOCK" +@pytest.fixture(autouse=True) +def _enable_bfabric_logging() -> None: + # bfabric calls logger.disable("bfabric") at import (loguru's library convention), which filters + # its records before they reach any sink -- including logot's capturer. Re-enable per test so + # logot.assert_logged works regardless of random test order. + logger.enable("bfabric") + + @pytest.fixture def bfabric_instance() -> str: return "https://bfabric.example.org/bfabric/" diff --git a/tests/bfabric/entities/core/test_entity_reader.py b/tests/bfabric/entities/core/test_entity_reader.py index bd93c3cb8..a437e75bd 100644 --- a/tests/bfabric/entities/core/test_entity_reader.py +++ b/tests/bfabric/entities/core/test_entity_reader.py @@ -345,6 +345,35 @@ def test_read_multiple_entities( endpoint="project", obj={}, multi_query_key="id", multi_query_vals=[100, 200] ) + def test_accepts_entity_class( + self, + entity_reader, + mock_cache_stack, + mock_multi_query, + mock_instantiate_entity, + uri_project_1, + mock_client, + bfabric_instance, + ): + """Passing an entity class infers the endpoint string (and the expected type).""" + from bfabric.entities.project import Project + + project = Project( + data_dict={"id": 100, "classname": "project", "name": "Project 1"}, + client=mock_client, + bfabric_instance=bfabric_instance, + ) + mock_cache_stack.item_get_all.return_value = {} + mock_multi_query.read_multi.return_value = [{"id": 100, "classname": "project", "name": "Project 1"}] + mock_instantiate_entity.return_value = project + + result = entity_reader.read_ids(Project, [100]) + + assert result == {uri_project_1: project} + mock_multi_query.read_multi.assert_called_once_with( + endpoint="project", obj={}, multi_query_key="id", multi_query_vals=[100] + ) + def test_some_missing_entities( self, entity_reader, @@ -537,6 +566,26 @@ def test_raises_on_type_mismatch( with pytest.raises(TypeError, match="Expected User"): entity_reader.query_one("user", {"login": "alice"}, expected_type=User) + def test_accepts_entity_class( + self, entity_reader, mock_cache_stack, mock_client, bfabric_instance, mock_instantiate_entity + ): + """Passing an entity class infers the endpoint string (and the expected type).""" + from bfabric.entities import User + + mock_cache_stack.item_get_all.return_value = {} + mock_client.read.return_value = [{"id": 1, "classname": "user", "login": "alice"}] + user = User( + data_dict={"id": 1, "classname": "user", "login": "alice"}, + client=mock_client, + bfabric_instance=bfabric_instance, + ) + mock_instantiate_entity.return_value = user + + result = entity_reader.query_one(User, {"login": "alice"}) + + assert result is user + mock_client.read.assert_called_once_with("user", obj={"login": "alice"}, max_results=1) + class TestRetrieveEntities: def test_retrieve_entities( diff --git a/tests/bfabric/entities/core/test_import_entity.py b/tests/bfabric/entities/core/test_import_entity.py index 13bbdfb0a..dd428c7dc 100644 --- a/tests/bfabric/entities/core/test_import_entity.py +++ b/tests/bfabric/entities/core/test_import_entity.py @@ -1,7 +1,7 @@ import pytest from bfabric.entities.dataset import Dataset from bfabric.entities.externaljob import ExternalJob -from bfabric.entities.core.import_entity import import_entity +from bfabric.entities.core.import_entity import entity_type_of, import_entity from bfabric.entities.core.entity import Entity @@ -21,3 +21,14 @@ def test_import_entity(name, expected): entity = import_entity(name) assert entity is expected + + +@pytest.mark.parametrize("entity_class", [Dataset, ExternalJob]) +def test_entity_type_of_is_lowercase_class_name(entity_class): + assert entity_type_of(entity_class) == entity_class.__name__.lower() + + +@pytest.mark.parametrize("entity_class", [Dataset, ExternalJob]) +def test_entity_type_of_round_trips_with_import_entity(entity_class): + # entity_type_of is the class→string inverse of import_entity's string→class mapping + assert import_entity(entity_type_of(entity_class)) is entity_class diff --git a/tests/bfabric/entities/core/test_reader_utils.py b/tests/bfabric/entities/core/test_reader_utils.py new file mode 100644 index 000000000..f041fb6d0 --- /dev/null +++ b/tests/bfabric/entities/core/test_reader_utils.py @@ -0,0 +1,43 @@ +import pytest + +from bfabric.entities.core.reader_utils import entities_by_id, present_entities +from bfabric.entities.core.uri import EntityUri + + +@pytest.fixture +def uri(bfabric_instance): + def _make(entity_id: int, entity_type: str = "resource") -> EntityUri: + return EntityUri(f"{bfabric_instance}{entity_type}/show.html?id={entity_id}") + + return _make + + +class TestEntitiesById: + def test_rekeys_by_int_id(self, mocker, uri): + a, b = mocker.MagicMock(name="a"), mocker.MagicMock(name="b") + result = entities_by_id({uri(1): a, uri(2): b}) + assert result == {1: a, 2: b} + + def test_drops_missing_entries(self, mocker, uri): + found = mocker.MagicMock(name="found") + result = entities_by_id({uri(1): found, uri(2): None}) + assert result == {1: found} + + def test_empty_input(self): + assert entities_by_id({}) == {} + + def test_all_missing(self, uri): + assert entities_by_id({uri(1): None, uri(2): None}) == {} + + +class TestPresentEntities: + def test_drops_missing_and_preserves_order(self, mocker, uri): + a, b = mocker.MagicMock(name="a"), mocker.MagicMock(name="b") + result = present_entities({uri(1): a, uri(2): None, uri(3): b}) + assert result == [a, b] + + def test_empty_input(self): + assert present_entities({}) == [] + + def test_all_missing(self, uri): + assert present_entities({uri(1): None, uri(2): None}) == [] diff --git a/tests/bfabric/entities/core/test_users.py b/tests/bfabric/entities/core/test_users.py index 05d81bb60..ad19afb3f 100644 --- a/tests/bfabric/entities/core/test_users.py +++ b/tests/bfabric/entities/core/test_users.py @@ -52,7 +52,7 @@ def test_not_cached(entity_reader, users, bfabric_instance, mock_user): user = users.get_by_login(bfabric_instance, login="testuser") assert user is mock_user entity_reader.query_one.assert_called_once_with( - "user", {"login": "testuser"}, bfabric_instance=bfabric_instance, expected_type=UserEntity + UserEntity, {"login": "testuser"}, bfabric_instance=bfabric_instance ) assert mock_user in users._users diff --git a/tests/bfabric/utils/test_cli_integration.py b/tests/bfabric/utils/test_cli_integration.py index 4897575f4..cc0d794d5 100644 --- a/tests/bfabric/utils/test_cli_integration.py +++ b/tests/bfabric/utils/test_cli_integration.py @@ -328,3 +328,20 @@ def my_function(arg1: str, *, client: Bfabric) -> str: mock_setup_logging.assert_called_once() # Verify the function still works assert result == "test" + + +class TestSetupScriptLogging: + """Tests for setup_script_logging's loguru configuration.""" + + def test_enables_bfabric_logging(self, mocker): + """bfabric is disabled at import; the CLI must re-enable it so its logs (incl. the version banner) show.""" + from bfabric.utils.cli_integration import setup_script_logging + + os.environ.pop("BFABRICPY_SCRIPT_LOGGING_SETUP", None) + mock_enable = mocker.patch("bfabric.utils.cli_integration.logger.enable") + mocker.patch("bfabric.utils.cli_integration.logger.add") + mocker.patch("bfabric.utils.cli_integration.logger.remove") + + setup_script_logging() + + assert mocker.call("bfabric") in mock_enable.mock_calls diff --git a/tests/bfabric_app_runner/actions/test_execute.py b/tests/bfabric_app_runner/actions/test_execute.py index 7895bb623..0ebb697f6 100644 --- a/tests/bfabric_app_runner/actions/test_execute.py +++ b/tests/bfabric_app_runner/actions/test_execute.py @@ -4,6 +4,7 @@ import yaml from pytest_mock import MockerFixture +from bfabric.entities import WorkflowStep, WorkflowTemplateStep from bfabric_app_runner.actions.execute import execute_run, execute_outputs, _ensure_dispatched, _register_workflow_step from bfabric_app_runner.actions.types import ActionDispatch, ActionRun, ActionInputs, ActionProcess, ActionOutputs @@ -401,13 +402,11 @@ def test_register_workflow_step_creates_workflow_and_step(mocker, mock_client, m mock_workflow_template = mocker.Mock(id=999) mock_workflow_template_step = mocker.Mock(id=789, workflow_template=mock_workflow_template) - # Mock WorkflowTemplateStep.find - mock_wts_find = mocker.patch("bfabric_app_runner.actions.execute.WorkflowTemplateStep.find") - mock_wts_find.return_value = mock_workflow_template_step + # Mock reader.read_id for the workflow template step + mock_client.reader.read_id.return_value = mock_workflow_template_step - # Mock WorkflowStep.find_by to return None (not found, so will create) - mock_ws_find_by = mocker.patch("bfabric_app_runner.actions.execute.WorkflowStep.find_by") - mock_ws_find_by.return_value = None + # Mock reader.query_one to return None (workflow step not found, so it will be created) + mock_client.reader.query_one.return_value = None # Setup client mock responses for workflow lookup and creation mock_client.read.side_effect = [ @@ -429,8 +428,8 @@ def test_register_workflow_step_creates_workflow_and_step(mocker, mock_client, m "workunitid": 123, "datasetid": 9999, } - mock_wts_find.assert_called_once_with(id=789, client=mock_client) - mock_ws_find_by.assert_called_once_with(expected_workflowstep, client=mock_client) + mock_client.reader.read_id.assert_called_once_with(WorkflowTemplateStep, 789) + mock_client.reader.query_one.assert_called_once_with(WorkflowStep, expected_workflowstep) # Verify client calls for workflow creation mock_client.read.assert_called_once_with("workflow", {"containerid": 456, "workflowtemplateid": 999}) @@ -444,8 +443,7 @@ def test_register_workflow_step_creates_workflow_and_step(mocker, mock_client, m def test_register_workflow_step_raises_when_step_not_found(mocker, mock_client, mock_workunit_definition): """A misconfigured step id must abort (raise), not silently skip and let the workunit finalize.""" - mock_wts_find = mocker.patch("bfabric_app_runner.actions.execute.WorkflowTemplateStep.find") - mock_wts_find.return_value = None + mock_client.reader.read_id.return_value = None with pytest.raises(ValueError, match="workflow_template_step_id=789"): _register_workflow_step(789, mock_workunit_definition, mock_client) @@ -456,8 +454,7 @@ def test_register_workflow_step_raises_when_step_not_found(mocker, mock_client, def test_register_workflow_step_raises_when_no_workflow_template(mocker, mock_client, mock_workunit_definition): """A step whose workflow template is absent must abort (raise), not create a step against None.""" mock_workflow_template_step = mocker.Mock(id=789, workflow_template=None) - mock_wts_find = mocker.patch("bfabric_app_runner.actions.execute.WorkflowTemplateStep.find") - mock_wts_find.return_value = mock_workflow_template_step + mock_client.reader.read_id.return_value = mock_workflow_template_step with pytest.raises(ValueError, match="has no workflow template"): _register_workflow_step(789, mock_workunit_definition, mock_client) diff --git a/tests/bfabric_app_runner/dispatch/test_dispatch_resource_flow.py b/tests/bfabric_app_runner/dispatch/test_dispatch_resource_flow.py index 72348254e..5d9782e12 100644 --- a/tests/bfabric_app_runner/dispatch/test_dispatch_resource_flow.py +++ b/tests/bfabric_app_runner/dispatch/test_dispatch_resource_flow.py @@ -8,12 +8,17 @@ from bfabric import Bfabric from bfabric.entities import Resource +from bfabric.entities.core.uri import EntityUri from bfabric.experimental.workunit_definition import WorkunitDefinition from bfabric_app_runner.dispatch.dispatch_resource_flow import ( ResourceDispatcher, ) +def _resource_uri(resource_id: int) -> EntityUri: + return EntityUri(f"https://fgcz-bfabric.uzh.ch/bfabric/resource/show.html?id={resource_id}") + + @pytest.fixture def mock_bfabric(mocker): return mocker.Mock(name="mock_bfabric", spec=Bfabric) @@ -38,8 +43,10 @@ def sample_resources(): @pytest.fixture -def mock_resource_find_all(mocker, sample_resources): - mocker.patch.object(Resource, "find_all", return_value=sample_resources) +def mock_resource_find_all(mock_bfabric, sample_resources): + mock_bfabric.reader.read_ids.return_value = { + _resource_uri(resource_id): resource for resource_id, resource in sample_resources.items() + } @pytest.fixture @@ -96,12 +103,24 @@ def test_build_input_resources_df(resource_dispatcher, mock_bfabric, mock_resour pl.testing.assert_frame_equal(result, expected) -def test_build_input_resources_df_when_empty(resource_dispatcher, mock_bfabric, mocker): - mocker.patch.object(Resource, "find_all", return_value={}) +def test_build_input_resources_df_when_empty(resource_dispatcher, mock_bfabric): + mock_bfabric.reader.read_ids.return_value = {} with pytest.raises(ValueError, match="No resources to dispatch"): resource_dispatcher._build_input_resources_df([1, 2, 3], mock_bfabric) +def test_build_input_resources_df_drops_missing_resources(resource_dispatcher, mock_bfabric, sample_resources): + """A not-found resource is None in read_ids' result and must be filtered out (matching the previous + find_all behavior of silently dropping misses), so only found resources reach the DataFrame.""" + mock_bfabric.reader.read_ids.return_value = { + _resource_uri(1): sample_resources[1], + _resource_uri(2): None, # id=2 not found + _resource_uri(3): sample_resources[3], + } + result = resource_dispatcher._build_input_resources_df([1, 2, 3], mock_bfabric) + assert result["resource_id"].to_list() == [1, 3] + + def test_build_inputs_spec(resource_dispatcher, sample_output_df): result = resource_dispatcher._build_inputs_spec(sample_output_df) expected = { diff --git a/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_dataset_specs.py b/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_dataset_specs.py index fea20cb09..4fefdbb80 100644 --- a/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_dataset_specs.py +++ b/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_dataset_specs.py @@ -2,6 +2,11 @@ from bfabric_app_runner.inputs.resolve._resolve_bfabric_dataset_specs import ResolveBfabricDatasetSpecs from bfabric.entities import Dataset +from bfabric.entities.core.uri import EntityUri + + +def _dataset_uri(dataset_id: int) -> EntityUri: + return EntityUri(f"https://fgcz-bfabric.uzh.ch/bfabric/dataset/show.html?id={dataset_id}") @pytest.fixture @@ -11,7 +16,7 @@ def mock_client(mocker): @pytest.fixture def resolver(mock_client): - return ResolveBfabricDatasetSpecs(mock_client) + return ResolveBfabricDatasetSpecs(mock_client.reader) def test_call(resolver, mocker, mock_client): @@ -19,8 +24,8 @@ def test_call(resolver, mocker, mock_client): mock_dataset = mocker.MagicMock(name="mock_dataset") mock_dataset.get_csv.return_value = "csv content" - # Mock Dataset.find_all to return our mock dataset - mocker.patch.object(Dataset, "find_all", return_value={1: mock_dataset}) + # Mock reader.read_ids to return our mock dataset keyed by URI + mock_client.reader.read_ids.return_value = {_dataset_uri(1): mock_dataset} # Create mock specs mock_spec = mocker.MagicMock(name="mock_spec") @@ -40,10 +45,33 @@ def test_call(resolver, mocker, mock_client): assert result[0].content == "csv content" # Verify the correct methods were called - Dataset.find_all.assert_called_once_with(ids=[1], client=mock_client) + mock_client.reader.read_ids.assert_called_once_with(Dataset, [1]) mock_dataset.get_csv.assert_called_once_with(separator=",") +def test_call_when_dataset_missing_raises_key_error(resolver, mocker, mock_client): + """A not-found dataset is None in read_ids' result and is filtered out on re-key, so indexing it + by id raises KeyError downstream (matching the previous find_all behavior of dropping misses).""" + found = mocker.MagicMock(name="found_dataset") + found.get_csv.return_value = "csv content" + # id=1 found, id=2 missing (None) + mock_client.reader.read_ids.return_value = {_dataset_uri(1): found, _dataset_uri(2): None} + + spec_found = mocker.MagicMock(name="spec_found") + spec_found.id = 1 + spec_found.format = "csv" + spec_found.separator = "," + spec_found.filename = "found.csv" + spec_missing = mocker.MagicMock(name="spec_missing") + spec_missing.id = 2 + spec_missing.format = "csv" + spec_missing.separator = "," + spec_missing.filename = "missing.csv" + + with pytest.raises(KeyError): + resolver([spec_found, spec_missing]) + + def test_call_when_empty(resolver): specs = [] result = resolver(specs) diff --git a/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_order_fasta_specs.py b/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_order_fasta_specs.py index d6158de43..1bcc1d14f 100644 --- a/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_order_fasta_specs.py +++ b/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_order_fasta_specs.py @@ -19,10 +19,10 @@ def test_call(resolver, mocker, mock_client): mock_order = mocker.MagicMock(name="mock_order", spec=Order) mock_order.data_dict = {"fastasequence": "ACGT"} - # Mock Workunit.find to return a workunit with our mock order + # Mock reader.read_id to return a workunit with our mock order mock_workunit = mocker.MagicMock(name="mock_workunit") mock_workunit.container = mock_order - mocker.patch.object(Workunit, "find", return_value=mock_workunit) + mock_client.reader.read_id.return_value = mock_workunit # Create mock spec mock_spec = mocker.MagicMock(name="mock_spec") @@ -38,7 +38,7 @@ def test_call(resolver, mocker, mock_client): assert len(result) == 1 assert result[0].filename == "test.fasta" assert result[0].content == "ACGT" - Workunit.find.assert_called_once_with(id=1, client=mock_client) + mock_client.reader.read_id.assert_called_once_with("workunit", 1, expected_type=Workunit) def test_call_when_empty(resolver): @@ -52,10 +52,10 @@ def test_get_order_fasta_when_workunit(mocker, resolver, mock_client): mock_order = mocker.MagicMock(name="mock_order", spec=Order) mock_order.data_dict = {"fastasequence": "ACGT"} - # Mock Workunit.find to return a workunit with our mock order + # Mock reader.read_id to return a workunit with our mock order mock_workunit = mocker.MagicMock(name="mock_workunit") mock_workunit.container = mock_order - mocker.patch.object(Workunit, "find", return_value=mock_workunit) + mock_client.reader.read_id.return_value = mock_workunit # Create mock spec mock_spec = mocker.MagicMock(name="mock_spec") @@ -68,14 +68,14 @@ def test_get_order_fasta_when_workunit(mocker, resolver, mock_client): # Assert the result assert result == "ACGT" - Workunit.find.assert_called_once_with(id=1, client=mock_client) + mock_client.reader.read_id.assert_called_once_with("workunit", 1, expected_type=Workunit) def test_get_order_fasta_when_order(mocker, resolver, mock_client): # Mock the Order with a FASTA sequence mock_order = mocker.MagicMock(name="mock_order") mock_order.data_dict = {"fastasequence": "TAGC"} - mocker.patch.object(Order, "find", return_value=mock_order) + mock_client.reader.read_id.return_value = mock_order # Create mock spec mock_spec = mocker.MagicMock(name="mock_spec") @@ -87,4 +87,4 @@ def test_get_order_fasta_when_order(mocker, resolver, mock_client): # Assert the result assert result == "TAGC" - Order.find.assert_called_once_with(id=2, client=mock_client) + mock_client.reader.read_id.assert_called_once_with("order", 2, expected_type=Order) diff --git a/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_resource_dataset_specs.py b/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_resource_dataset_specs.py index 031859a4f..a2e402781 100644 --- a/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_resource_dataset_specs.py +++ b/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_resource_dataset_specs.py @@ -2,13 +2,17 @@ import pytest from inline_snapshot import snapshot -from bfabric.entities import Dataset, Resource +from bfabric.entities.core.uri import EntityUri from bfabric_app_runner.inputs.resolve._resolve_bfabric_resource_dataset_specs import ResolveBfabricResourceDatasetSpecs from bfabric_app_runner.inputs.resolve.resolved_inputs import ResolvedFile -from bfabric_app_runner.specs.inputs.bfabric_resource_dataset import BfabricResourceDatasetSpec +from bfabric_app_runner.specs.inputs.bfabric_resource_dataset_spec import BfabricResourceDatasetSpec from bfabric_app_runner.specs.inputs.file_spec import FileSourceSsh, FileSourceSshValue +def _resource_uri(resource_id: int) -> EntityUri: + return EntityUri(f"https://fgcz-bfabric.uzh.ch/bfabric/resource/show.html?id={resource_id}") + + @pytest.fixture def scenario(request): if request.param in ("minimal", "complex", "complex_output_column_conflict", "dataset_only"): @@ -25,10 +29,10 @@ def original_dataset_df(scenario): @pytest.fixture(autouse=True) -def original_dataset(mocker, original_dataset_df): +def original_dataset(mocker, original_dataset_df, client): dataset = mocker.MagicMock(nmae="original_dataset_entity", spec=["to_polars"]) dataset.to_polars.return_value = original_dataset_df - mocker.patch.object(Dataset, "find").return_value = dataset + client.reader.read_id.return_value = dataset return dataset @@ -40,7 +44,7 @@ def storage(mocker): @pytest.fixture(autouse=True) -def original_resources(mocker, storage): +def original_resources(mocker, storage, client): resources = [] for i in range(10, 13): resource = mocker.MagicMock( @@ -56,10 +60,11 @@ def original_resources(mocker, storage): resource.__getitem__.side_effect = lambda key, _attrs=attrs: _attrs[key] resources.append(resource) - def mock_find_all(ids, client): - return {r.id: r for r in resources if r.id in ids} + def mock_read_ids(entity_type, entity_ids, *args, **kwargs): + # URI-keyed like the real reader; misses would be absent (and None-filtered downstream). + return {_resource_uri(r.id): r for r in resources if r.id in entity_ids} - mocker.patch.object(Resource, "find_all").side_effect = mock_find_all + client.reader.read_ids.side_effect = mock_read_ids return resources @@ -75,12 +80,12 @@ def spec(scenario): @pytest.fixture def client(mocker): - return mocker.MagicMock(name="client", spec=[]) + return mocker.MagicMock(name="client") @pytest.fixture def resolver(client): - return ResolveBfabricResourceDatasetSpecs(client=client) + return ResolveBfabricResourceDatasetSpecs(reader=client.reader) class TestInternals: diff --git a/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_resource_specs.py b/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_resource_specs.py index 342f3a389..e08c51367 100644 --- a/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_resource_specs.py +++ b/tests/bfabric_app_runner/inputs/resolve/test_resolve_bfabric_resource_specs.py @@ -4,7 +4,12 @@ from bfabric_app_runner.inputs.resolve._resolve_bfabric_resource_specs import ResolveBfabricResourceSpecs from bfabric_app_runner.inputs.resolve.resolved_inputs import ResolvedFile -from bfabric.entities import Resource, Storage +from bfabric.entities import Resource +from bfabric.entities.core.uri import EntityUri + + +def _resource_uri(resource_id: int) -> EntityUri: + return EntityUri(f"https://fgcz-bfabric.uzh.ch/bfabric/resource/show.html?id={resource_id}") @pytest.fixture @@ -28,8 +33,8 @@ def test_call(resolver, mocker, mock_client): ) mock_resource.__getitem__.side_effect = lambda key: {"filechecksum": "abc123"}[key] - # Mock Resource.find_all to return our mock resource - mocker.patch.object(Resource, "find_all", return_value={42: mock_resource}) + # Mock reader.read_ids to return our mock resource keyed by URI + mock_client.reader.read_ids.return_value = {_resource_uri(42): mock_resource} # Create mock spec mock_spec = mocker.MagicMock(name="mock_spec", id=42, filename="renamed_file.txt", check_checksum=True) @@ -47,7 +52,7 @@ def test_call(resolver, mocker, mock_client): assert result[0].source.ssh.path == "/data/path/to/file.txt" # Verify the correct methods were called - Resource.find_all.assert_called_once_with(ids=[42], client=mock_client) + mock_client.reader.read_ids.assert_called_once_with(Resource, [42]) def test_call_when_empty(resolver): @@ -81,9 +86,10 @@ def test_call_multiple_resources(resolver, mocker, mock_client): mock_resources[101].__getitem__.side_effect = lambda key: {"filechecksum": "abc123"}[key] mock_resources[102].__getitem__.side_effect = lambda key: {"filechecksum": "def456"}[key] - # Mock find_all methods - mocker.patch.object(Resource, "find_all", return_value=mock_resources) - mocker.patch.object(Storage, "find_all", return_value=mock_storages) + # Mock reader.read_ids to return the resources keyed by URI + mock_client.reader.read_ids.return_value = { + _resource_uri(resource_id): resource for resource_id, resource in mock_resources.items() + } # Create mock specs mock_spec1 = mocker.MagicMock(name="mock_spec1") @@ -115,6 +121,26 @@ def test_call_multiple_resources(resolver, mocker, mock_client): assert result[1].source.ssh.path == "/data2/path/to/file2.txt" +def test_call_when_resource_missing_raises_key_error(resolver, mocker, mock_client): + """A not-found resource is None in read_ids' result and filtered out on re-key, so indexing it by + id raises KeyError downstream (matching the previous find_all behavior of dropping misses).""" + mock_resource = mocker.MagicMock( + name="mock_resource", + storage={"host": "example.com", "basepath": "/data"}, + storage_absolute_path="/data/path/to/file.txt", + spec=["storage", "storage_absolute_path", "__getitem__"], + ) + mock_resource.__getitem__.side_effect = lambda key: {"filechecksum": "abc123"}[key] + # id=101 found, id=102 missing (None) + mock_client.reader.read_ids.return_value = {_resource_uri(101): mock_resource, _resource_uri(102): None} + + spec_found = mocker.MagicMock(name="spec_found", id=101, filename="found.txt", check_checksum=True) + spec_missing = mocker.MagicMock(name="spec_missing", id=102, filename="missing.txt", check_checksum=True) + + with pytest.raises(KeyError): + resolver([spec_found, spec_missing]) + + def test_get_file_spec(resolver, mocker): # Setup test data mock_spec = mocker.MagicMock(name="mock_spec") diff --git a/tests/bfabric_app_runner/inputs/resolve/test_resolver.py b/tests/bfabric_app_runner/inputs/resolve/test_resolver.py index 92968c1d6..7b5c01399 100644 --- a/tests/bfabric_app_runner/inputs/resolve/test_resolver.py +++ b/tests/bfabric_app_runner/inputs/resolve/test_resolver.py @@ -80,11 +80,32 @@ def test_resolver_initialization( mock_annotation_resolver, ): assert resolver._client == mock_bfabric - assert resolver._resolve_bfabric_dataset_specs == mock_dataset_resolver - assert resolver._resolve_bfabric_resource_specs == mock_resource_resolver - assert resolver._resolve_static_yaml_specs == mock_yaml_resolver - assert resolver._resolve_bfabric_order_fasta_specs == mock_order_fasta_resolver - assert resolver._resolve_bfabric_annotation_specs == mock_annotation_resolver + assert resolver._registry[BfabricDatasetSpec] == mock_dataset_resolver + assert resolver._registry[BfabricResourceSpec] == mock_resource_resolver + assert resolver._registry[StaticYamlSpec] == mock_yaml_resolver + assert resolver._registry[BfabricOrderFastaSpec] == mock_order_fasta_resolver + assert resolver._registry[BfabricAnnotationSpec] == mock_annotation_resolver + + +def test_registry_covers_all_input_spec_types(resolver) -> None: + """The registry must have an entry for every InputSpecType member (enforced at construction).""" + from typing import get_args + + from bfabric_app_runner.specs.inputs_spec import InputSpecType + + union_members = get_args(get_args(InputSpecType)[0]) + for member in union_members: + assert any(issubclass(member, base) for base in resolver._registry) + + +def test_resolver_for_unregistered_type_raises(resolver) -> None: + """A spec type with no registered resolver fails loudly.""" + + class UnregisteredSpec: + pass + + with pytest.raises(TypeError, match="No resolver registered"): + resolver._resolver_for(UnregisteredSpec) def test_group_specs_by_type(mocker) -> None: diff --git a/tests/bfabric_app_runner/output_registration/test_save_dataset.py b/tests/bfabric_app_runner/output_registration/test_save_dataset.py index cbd742bed..73a21dd79 100644 --- a/tests/bfabric_app_runner/output_registration/test_save_dataset.py +++ b/tests/bfabric_app_runner/output_registration/test_save_dataset.py @@ -64,7 +64,7 @@ def test_save_dataset_no_existing_creates(mock_client, mock_workunit_definition, _save_dataset(spec, mock_client, mock_workunit_definition) - mock_client.reader.query_one.assert_called_once_with("dataset", {"workunitid": 42}, expected_type=Dataset) + mock_client.reader.query_one.assert_called_once_with(Dataset, {"workunitid": 42}) mock_operations["create"].assert_called_once() _, table, params = mock_operations["create"].call_args.args assert params.name == "my_dataset" diff --git a/tests/bfabric_scripts/test_api_parser.py b/tests/bfabric_scripts/test_api_parser.py index 87b7abd8c..78ee77cd3 100644 --- a/tests/bfabric_scripts/test_api_parser.py +++ b/tests/bfabric_scripts/test_api_parser.py @@ -5,6 +5,7 @@ import pytest from pytest_mock import MockerFixture +from bfabric.config.bfabric_client_config import BfabricAPIEngineType from bfabric_scripts.cli.api.parser import ( FieldModel, ParameterModel, @@ -239,6 +240,7 @@ def test_parse_method_signature_basic(self, mocker: MockerFixture) -> None: # Mock client client_mock = mocker.MagicMock() + client_mock.config.engine = BfabricAPIEngineType.SUDS client_mock._engine._get_suds_service.return_value = service_mock mocker.patch.object(service_mock, "testMethod", method_mock) client_mock._engine._get_suds_service.return_value = mocker.MagicMock(testMethod=method_mock) @@ -268,6 +270,7 @@ def test_parse_method_signature_empty_parameters(self, mocker: MockerFixture) -> service_mock.testMethod = method_mock client_mock = mocker.MagicMock() + client_mock.config.engine = BfabricAPIEngineType.SUDS client_mock._engine._get_suds_service.return_value = service_mock # Call function @@ -276,6 +279,17 @@ def test_parse_method_signature_empty_parameters(self, mocker: MockerFixture) -> # Should return empty dict assert result == {} + def test_parse_method_signature_rejects_non_suds_engine(self, mocker: MockerFixture) -> None: + """Test that a non-SUDS engine raises a clear error instead of leaking an AttributeError.""" + client_mock = mocker.MagicMock() + client_mock.config.engine = BfabricAPIEngineType.ZEEP + + with pytest.raises(RuntimeError, match="only supported with the SUDS engine"): + parse_method_signature(client_mock, "test_endpoint", "testMethod") + + # Guard must fire before any private engine access. + client_mock._engine._get_suds_service.assert_not_called() + if __name__ == "__main__": pytest.main([__file__, "-v"])