From f677cc51820d7b5989891f16903790d220e36062 Mon Sep 17 00:00:00 2001 From: Leonardo Schwarz Date: Wed, 19 Aug 2026 16:52:20 +0200 Subject: [PATCH 1/2] feat(bfabric): record entity provenance in dump_yaml / load_yaml A dumped entity was a bare data dictionary, which lost the instance it came from. Loading it back without passing bfabric_instance produced a half-broken object: both .uri and .refs call EntityUri.from_components(None, ...) and raise. A dump of workunit 1234 on the test instance was also indistinguishable from one on production, since ids are only unique within an instance. Wrap the data in a versioned envelope carrying the entity URI, the dump time and the bfabricPy version. The URI is the only provenance field needed, as it encodes instance, entity type and id, and it is already validated. A model validator cross-checks it against the data's id and classname, so a mismatched or hand-edited file fails at load instead of silently producing a wrong entity. load_yaml now dispatches to the most specific class for the dumped type, and rejects a file holding a different type when called on a subclass. Files written by earlier versions still load, with a DeprecationWarning. The data dictionary stays unvalidated: it is an opaque API payload, and pydantic cannot resolve ApiResponseObjectType anyway (an implicit recursive alias, which hits a RecursionError), so validating it would risk coercing the payload. Closes #351 --- .../docs/api_reference/entity_types/index.md | 6 ++ bfabric/docs/changelog.md | 8 ++ .../working_with_entities/index.md | 32 ++++++ bfabric/src/bfabric/entities/core/entity.py | 43 ++++++-- .../bfabric/entities/core/serialization.py | 78 +++++++++++++++ tests/bfabric/entities/core/test_entity.py | 97 +++++++++++++++---- 6 files changed, 237 insertions(+), 27 deletions(-) create mode 100644 bfabric/src/bfabric/entities/core/serialization.py diff --git a/bfabric/docs/api_reference/entity_types/index.md b/bfabric/docs/api_reference/entity_types/index.md index 666d5f0b..9e305497 100644 --- a/bfabric/docs/api_reference/entity_types/index.md +++ b/bfabric/docs/api_reference/entity_types/index.md @@ -9,6 +9,12 @@ Complete reference for all B-Fabric entity types. :show-inheritance: ``` +```{eval-rst} +.. autoclass:: bfabric.entities.core.serialization.EntityDump + :members: + :show-inheritance: +``` + ```{eval-rst} .. automodule:: bfabric.entities :members: diff --git a/bfabric/docs/changelog.md b/bfabric/docs/changelog.md index 8373a732..223d353f 100644 --- a/bfabric/docs/changelog.md +++ b/bfabric/docs/changelog.md @@ -9,6 +9,14 @@ Minor breaking changes are still possible in `1.X.Y` but we try to announce them ## \[Unreleased\] +### Added + +- `Entity.dump_yaml` records the entity's URI, the dump time and the bfabricPy version alongside the data, so `Entity.load_yaml` restores a working `uri` and `refs` without being told the B-Fabric instance. See `bfabric.entities.core.serialization.EntityDump` for the file layout. + +### Changed + +- `Entity.load_yaml` returns the most specific entity class for the dumped type, and raises `TypeError` when called on a subclass that does not match. Files written by earlier versions still load, with a `DeprecationWarning`, and `dump_yaml` now requires the entity to have a `bfabric_instance`. + ## \[1.21.0\] - 2026-08-20 ### Added diff --git a/bfabric/docs/user_guides/working_with_entities/index.md b/bfabric/docs/user_guides/working_with_entities/index.md index e404b779..02e151cf 100644 --- a/bfabric/docs/user_guides/working_with_entities/index.md +++ b/bfabric/docs/user_guides/working_with_entities/index.md @@ -161,6 +161,38 @@ print(f"User ID: {user.id}") print(f"User name: {user['name']}") ``` +## Saving and Loading Entities + +Any entity can be written to a YAML file and read back later, e.g. to keep a fixture for offline development. + +```python +from pathlib import Path +from bfabric.entities.core.entity import Entity + +workunit = client.reader.read_id(entity_type="workunit", entity_id=1234) +workunit.dump_yaml(Path("workunit.yml")) + +# Returns a Workunit, and needs no client unless relationships are accessed +loaded = Entity.load_yaml(Path("workunit.yml")) +``` + +Besides the entity's data, the file records the entity's URI, so the instance it came from is never guessed: + +```yaml +format_version: 1 +uri: https://fgcz-bfabric.uzh.ch/bfabric/workunit/show.html?id=1234 +dumped_at: '2026-08-19T09:41:03Z' +bfabricpy_version: 1.20.0 +data: + id: 1234 + classname: workunit + name: Example workunit +``` + +`Entity.load_yaml` returns the most specific class for the recorded entity type, while loading through a subclass +(e.g. `Workunit.load_yaml`) rejects a file holding a different type. Passing a `bfabric_instance` that disagrees +with the file is an error — entity ids are only unique within one instance, so a mismatch is never intended. + ## Custom Entity Methods Some entity types provide custom class methods: diff --git a/bfabric/src/bfabric/entities/core/entity.py b/bfabric/src/bfabric/entities/core/entity.py index 6994581e..36fccd58 100644 --- a/bfabric/src/bfabric/entities/core/entity.py +++ b/bfabric/src/bfabric/entities/core/entity.py @@ -4,7 +4,7 @@ import warnings from functools import cached_property -from typing import TYPE_CHECKING, Self, TypeGuard +from typing import TYPE_CHECKING, Self, TypeGuard, cast from bfabric.entities.core.mixins.find_mixin import FindMixin from bfabric.entities.core.uri import EntityUri @@ -140,22 +140,45 @@ def __repr__(self) -> str: __str__ = __repr__ def dump_yaml(self, path: Path) -> None: - """Writes the entity's data dictionary to a YAML file.""" - # TODO (#351): to be extended + """Writes the entity's data dictionary, and the provenance needed to reload it, to a YAML file. + + :raises ValueError: if the entity has no ``bfabric_instance``, since the dump could not be reloaded + """ import yaml - with path.open("w") as file: - yaml.safe_dump(self.__data_dict, file) + from bfabric.entities.core.serialization import EntityDump + + if self.__bfabric_instance is None: + msg = "Cannot dump an entity that has no bfabric_instance, as the file could not be reloaded." + raise ValueError(msg) + dump = EntityDump.create(uri=self.uri, data=self.__data_dict) + _ = path.write_text(yaml.safe_dump(dump.model_dump(mode="json"), sort_keys=False)) @classmethod def load_yaml(cls, path: Path, client: Bfabric | None = None, bfabric_instance: str | None = None) -> Self: - """Loads an entity from a YAML file.""" - # TODO (#351): to be extended + """Loads an entity from a YAML file written by :meth:`dump_yaml`. + + Called on ``Entity`` this returns the most specific class for the dumped entity type, whereas calling it on a + subclass rejects a file holding a different entity type. ``bfabric_instance`` is only needed for files + written before the provenance metadata existed, and otherwise has to agree with the file. + + :raises TypeError: if the file holds an entity type other than ``cls`` + """ import yaml - with path.open("r") as file: - data = yaml.safe_load(file) - return cls(data, client=client, bfabric_instance=bfabric_instance) + from bfabric.entities.core.import_entity import entity_type_of, import_entity + from bfabric.entities.core.serialization import parse_document + + data, bfabric_instance = parse_document(yaml.safe_load(path.read_text()), bfabric_instance) + classname = data.get("classname") + if cls is Entity: + entity_class = import_entity(classname) if isinstance(classname, str) else Entity + elif classname != entity_type_of(cls): + msg = f"{path} holds a {classname!r} entity, which cannot be loaded as {cls.__name__}" + raise TypeError(msg) + else: + entity_class = cls + return cast("Self", entity_class(data, client=client, bfabric_instance=bfabric_instance)) def _is_custom_attributes_list(custom_attributes: ApiResponseDataType) -> TypeGuard[list[dict[str, str]]]: diff --git a/bfabric/src/bfabric/entities/core/serialization.py b/bfabric/src/bfabric/entities/core/serialization.py new file mode 100644 index 00000000..6b14ccbd --- /dev/null +++ b/bfabric/src/bfabric/entities/core/serialization.py @@ -0,0 +1,78 @@ +from __future__ import annotations + +import datetime +import importlib.metadata +import warnings +from typing import TYPE_CHECKING, Any, Literal, Self, cast + +from pydantic import BaseModel, model_validator + +from bfabric.entities.core.uri import EntityUri + +if TYPE_CHECKING: + from bfabric.typing import ApiResponseObjectType + + +class EntityDump(BaseModel): + """The on-disk form of a single entity: its API data plus the provenance needed to reload it. + + ``uri`` is the point of the envelope, as it records the B-Fabric instance, entity type and id the ``data`` came + from — none of which the payload alone identifies, since ids are only unique within one instance. + """ + + format_version: Literal[1] = 1 + uri: EntityUri + dumped_at: datetime.datetime + bfabricpy_version: str + data: dict[str, Any] # pyright: ignore[reportExplicitAny] + """The entity's data dictionary, verbatim and unvalidated.""" + + @classmethod + def create(cls, uri: EntityUri, data: ApiResponseObjectType) -> Self: + """Builds a dump of ``data``, stamped with the current time and bfabricPy version.""" + return cls( + uri=uri, + dumped_at=datetime.datetime.now(datetime.UTC), + bfabricpy_version=importlib.metadata.version("bfabric"), + data=dict(data), + ) + + @model_validator(mode="after") + def _check_data_matches_uri(self) -> Self: + components = self.uri.components + for field, expected in (("id", components.entity_id), ("classname", components.entity_type)): + actual = self.data.get(field) + if actual != expected: + msg = f"data[{field!r}] is {actual!r}, but the URI says {expected!r}: {self.uri}" + raise ValueError(msg) + return self + + +def parse_document(document: object, bfabric_instance: str | None) -> tuple[ApiResponseObjectType, str | None]: + """Extracts the data dictionary and its B-Fabric instance from a parsed entity YAML document. + + Files written before the provenance metadata existed are bare data dictionaries; those are still accepted, with + a warning, and fall back to the passed ``bfabric_instance``. + + :raises ValueError: if the document is not a mapping, or was dumped from another B-Fabric instance than the one + requested + """ + if not isinstance(document, dict): + msg = f"Expected a mapping at the top level of the entity file, found {type(document).__name__}" + raise ValueError(msg) + document = cast("ApiResponseObjectType", document) + + if "format_version" not in document: + warnings.warn( + "Entity files without a 'format_version' are deprecated; dump the entity again to record its URI.", + DeprecationWarning, + stacklevel=3, + ) + return document, bfabric_instance + + dump = EntityDump.model_validate(document) + dumped_instance = str(dump.uri.components.bfabric_instance) + if bfabric_instance is not None and bfabric_instance.rstrip("/") != dumped_instance.rstrip("/"): + msg = f"Entity was dumped from {dumped_instance}, which is not the requested instance {bfabric_instance}" + raise ValueError(msg) + return dump.data, dumped_instance diff --git a/tests/bfabric/entities/core/test_entity.py b/tests/bfabric/entities/core/test_entity.py index 0bda242d..9878280b 100644 --- a/tests/bfabric/entities/core/test_entity.py +++ b/tests/bfabric/entities/core/test_entity.py @@ -1,7 +1,12 @@ +import datetime +from importlib.metadata import version from pathlib import Path import pytest +import yaml +from pydantic import ValidationError +from bfabric.entities import Resource, Workunit from bfabric.entities.core.entity import Entity from bfabric.entities.core.entity_reader import EntityReader, EntityResult from bfabric.entities.core.uri import EntityUri @@ -178,25 +183,83 @@ def test_find_by_when_not_found(mocker, mock_client) -> None: mock_client.read.assert_called_once_with("testendpoint", obj={"id": 1}, max_results=100) -def test_dump_yaml(mocker, mock_entity) -> None: - mock_yaml_dump = mocker.patch("yaml.safe_dump") - mock_path = mocker.MagicMock(spec=Path) - mock_entity.dump_yaml(mock_path) - mock_path.open.assert_called_once_with("w") - mock_yaml_dump.assert_called_once_with(mock_entity.data_dict, mock_path.open.return_value.__enter__.return_value) - - -def test_load_yaml(mocker) -> None: - mock_yaml_load = mocker.patch("yaml.safe_load", return_value={"key": "value"}) - mock_path = mocker.MagicMock(spec=Path) - mock_client = mocker.MagicMock() +class TestSerialization: + @pytest.fixture + def workunit_data_dict(self) -> dict: + return {"id": 1234, "classname": "workunit", "name": "Test Workunit", "status": "AVAILABLE"} - entity = Entity.load_yaml(mock_path, client=mock_client) + @pytest.fixture + def workunit(self, workunit_data_dict, mock_client, bfabric_instance) -> Workunit: + return Workunit(workunit_data_dict, mock_client, bfabric_instance) - mock_path.open.assert_called_once_with("r") - mock_yaml_load.assert_called_once_with(mock_path.open.return_value.__enter__.return_value) - assert entity.data_dict == {"key": "value"} - assert isinstance(entity, Entity) + @pytest.fixture + def dump_path(self, tmp_path, workunit) -> Path: + path = tmp_path / "entity.yml" + workunit.dump_yaml(path) + return path + + def test_dump_yaml_writes_metadata(self, dump_path, workunit, workunit_data_dict) -> None: + document = yaml.safe_load(dump_path.read_text()) + assert document["format_version"] == 1 + assert document["uri"] == str(workunit.uri) + assert document["bfabricpy_version"] == version("bfabric") + assert datetime.datetime.fromisoformat(document["dumped_at"]).tzinfo is not None + assert document["data"] == workunit_data_dict + + def test_dump_yaml_when_no_bfabric_instance(self, tmp_path, workunit_data_dict) -> None: + with pytest.warns(DeprecationWarning): + entity = Entity(workunit_data_dict) + with pytest.raises(ValueError, match="bfabric_instance"): + entity.dump_yaml(tmp_path / "entity.yml") + + def test_load_yaml_round_trip(self, dump_path, workunit, workunit_data_dict, bfabric_instance) -> None: + loaded = Entity.load_yaml(dump_path) + assert type(loaded) is Workunit + assert loaded.data_dict == workunit_data_dict + assert loaded.bfabric_instance == bfabric_instance + assert loaded.uri == workunit.uri + assert loaded._client is None + + def test_load_yaml_passes_client(self, dump_path, mock_client) -> None: + assert Entity.load_yaml(dump_path, client=mock_client)._client == mock_client + + def test_load_yaml_when_subclass_matches(self, dump_path, workunit_data_dict) -> None: + loaded = Workunit.load_yaml(dump_path) + assert type(loaded) is Workunit + assert loaded.data_dict == workunit_data_dict + + def test_load_yaml_when_subclass_mismatch(self, dump_path) -> None: + with pytest.raises(TypeError, match="'workunit'.*Resource"): + _ = Resource.load_yaml(dump_path) + + def test_load_yaml_when_instance_conflicts(self, dump_path) -> None: + with pytest.raises(ValueError, match="was dumped from"): + _ = Entity.load_yaml(dump_path, bfabric_instance="https://other.example.org/bfabric/") + + def test_load_yaml_when_data_mismatches_uri(self, tmp_path, dump_path) -> None: + document = yaml.safe_load(dump_path.read_text()) + document["data"]["id"] = 5678 + path = tmp_path / "tampered.yml" + _ = path.write_text(yaml.safe_dump(document)) + with pytest.raises(ValidationError, match="'id'"): + _ = Entity.load_yaml(path) + + def test_load_yaml_when_not_a_mapping(self, tmp_path) -> None: + path = tmp_path / "list.yml" + _ = path.write_text(yaml.safe_dump([{"id": 1234, "classname": "workunit"}])) + with pytest.raises(ValueError, match="found list"): + _ = Entity.load_yaml(path) + + def test_load_yaml_when_legacy(self, tmp_path, workunit_data_dict, bfabric_instance) -> None: + path = tmp_path / "legacy.yml" + _ = path.write_text(yaml.safe_dump(workunit_data_dict)) + + with pytest.warns(DeprecationWarning, match="format_version"): + loaded = Entity.load_yaml(path, bfabric_instance=bfabric_instance) + + assert type(loaded) is Workunit + assert loaded.data_dict == workunit_data_dict + assert loaded.bfabric_instance == bfabric_instance def test_getitem(mock_entity) -> None: From 2313568494b03e6accbe27d8bf94a874a1a7a1cd Mon Sep 17 00:00:00 2001 From: Leonardo Schwarz Date: Thu, 20 Aug 2026 16:25:40 +0200 Subject: [PATCH 2/2] refactor(bfabric): tighten the entity dump/load implementation Collapse the load_yaml class-dispatch ladder, trim the docstrings to the contract the signatures cannot show, and drop a redundant local rebind in parse_document. Bind the parsed YAML document to an object-annotated local so basedpyright does not see an Any argument, and drop the two baseline entries the rewrite made obsolete. No behaviour change. --- .basedpyright/baseline.bfabric.json | 16 ------------ bfabric/src/bfabric/entities/core/entity.py | 16 +++++------- .../bfabric/entities/core/serialization.py | 26 ++++++++----------- 3 files changed, 18 insertions(+), 40 deletions(-) diff --git a/.basedpyright/baseline.bfabric.json b/.basedpyright/baseline.bfabric.json index 516d4b5c..84f6a435 100644 --- a/.basedpyright/baseline.bfabric.json +++ b/.basedpyright/baseline.bfabric.json @@ -1012,22 +1012,6 @@ "endColumn": 11, "lineCount": 1 } - }, - { - "code": "reportAny", - "range": { - "startColumn": 12, - "endColumn": 16, - "lineCount": 1 - } - }, - { - "code": "reportAny", - "range": { - "startColumn": 19, - "endColumn": 23, - "lineCount": 1 - } } ], "./bfabric/src/bfabric/entities/core/entity_reader.py": [ diff --git a/bfabric/src/bfabric/entities/core/entity.py b/bfabric/src/bfabric/entities/core/entity.py index 36fccd58..ccef3dc6 100644 --- a/bfabric/src/bfabric/entities/core/entity.py +++ b/bfabric/src/bfabric/entities/core/entity.py @@ -158,9 +158,9 @@ def dump_yaml(self, path: Path) -> None: def load_yaml(cls, path: Path, client: Bfabric | None = None, bfabric_instance: str | None = None) -> Self: """Loads an entity from a YAML file written by :meth:`dump_yaml`. - Called on ``Entity`` this returns the most specific class for the dumped entity type, whereas calling it on a - subclass rejects a file holding a different entity type. ``bfabric_instance`` is only needed for files - written before the provenance metadata existed, and otherwise has to agree with the file. + Called on ``Entity`` this returns the most specific class for the dumped type, whereas calling it on a + subclass rejects a file holding a different type. ``bfabric_instance`` is only needed for files predating + the provenance metadata, and otherwise has to agree with the file. :raises TypeError: if the file holds an entity type other than ``cls`` """ @@ -169,15 +169,13 @@ def load_yaml(cls, path: Path, client: Bfabric | None = None, bfabric_instance: from bfabric.entities.core.import_entity import entity_type_of, import_entity from bfabric.entities.core.serialization import parse_document - data, bfabric_instance = parse_document(yaml.safe_load(path.read_text()), bfabric_instance) + document: object = yaml.safe_load(path.read_text()) # pyright: ignore[reportAny] + data, bfabric_instance = parse_document(document, bfabric_instance) classname = data.get("classname") - if cls is Entity: - entity_class = import_entity(classname) if isinstance(classname, str) else Entity - elif classname != entity_type_of(cls): + if cls is not Entity and classname != entity_type_of(cls): msg = f"{path} holds a {classname!r} entity, which cannot be loaded as {cls.__name__}" raise TypeError(msg) - else: - entity_class = cls + entity_class = import_entity(classname) if cls is Entity and isinstance(classname, str) else cls return cast("Self", entity_class(data, client=client, bfabric_instance=bfabric_instance)) diff --git a/bfabric/src/bfabric/entities/core/serialization.py b/bfabric/src/bfabric/entities/core/serialization.py index 6b14ccbd..8eb8f92d 100644 --- a/bfabric/src/bfabric/entities/core/serialization.py +++ b/bfabric/src/bfabric/entities/core/serialization.py @@ -14,10 +14,10 @@ class EntityDump(BaseModel): - """The on-disk form of a single entity: its API data plus the provenance needed to reload it. + """The on-disk form of a single entity: its API data, plus the ``uri`` it came from. - ``uri`` is the point of the envelope, as it records the B-Fabric instance, entity type and id the ``data`` came - from — none of which the payload alone identifies, since ids are only unique within one instance. + The URI is the point of the envelope, since ids are only unique within one B-Fabric instance and so the data + alone does not identify the entity. """ format_version: Literal[1] = 1 @@ -25,7 +25,6 @@ class EntityDump(BaseModel): dumped_at: datetime.datetime bfabricpy_version: str data: dict[str, Any] # pyright: ignore[reportExplicitAny] - """The entity's data dictionary, verbatim and unvalidated.""" @classmethod def create(cls, uri: EntityUri, data: ApiResponseObjectType) -> Self: @@ -41,8 +40,7 @@ def create(cls, uri: EntityUri, data: ApiResponseObjectType) -> Self: def _check_data_matches_uri(self) -> Self: components = self.uri.components for field, expected in (("id", components.entity_id), ("classname", components.entity_type)): - actual = self.data.get(field) - if actual != expected: + if (actual := self.data.get(field)) != expected: msg = f"data[{field!r}] is {actual!r}, but the URI says {expected!r}: {self.uri}" raise ValueError(msg) return self @@ -51,26 +49,24 @@ def _check_data_matches_uri(self) -> Self: def parse_document(document: object, bfabric_instance: str | None) -> tuple[ApiResponseObjectType, str | None]: """Extracts the data dictionary and its B-Fabric instance from a parsed entity YAML document. - Files written before the provenance metadata existed are bare data dictionaries; those are still accepted, with - a warning, and fall back to the passed ``bfabric_instance``. + Files predating the provenance metadata are bare data dictionaries; those still load, with a warning, falling + back to the passed ``bfabric_instance``. - :raises ValueError: if the document is not a mapping, or was dumped from another B-Fabric instance than the one - requested + :raises ValueError: if the document is not a mapping, or was dumped from another instance than requested """ if not isinstance(document, dict): msg = f"Expected a mapping at the top level of the entity file, found {type(document).__name__}" raise ValueError(msg) - document = cast("ApiResponseObjectType", document) - - if "format_version" not in document: + data = cast("ApiResponseObjectType", document) + if "format_version" not in data: warnings.warn( "Entity files without a 'format_version' are deprecated; dump the entity again to record its URI.", DeprecationWarning, stacklevel=3, ) - return document, bfabric_instance + return data, bfabric_instance - dump = EntityDump.model_validate(document) + dump = EntityDump.model_validate(data) dumped_instance = str(dump.uri.components.bfabric_instance) if bfabric_instance is not None and bfabric_instance.rstrip("/") != dumped_instance.rstrip("/"): msg = f"Entity was dumped from {dumped_instance}, which is not the requested instance {bfabric_instance}"