diff --git a/src/osw/core.py b/src/osw/core.py index 75c67e75..94774358 100644 --- a/src/osw/core.py +++ b/src/osw/core.py @@ -1227,93 +1227,101 @@ def load_entity( # enable cache to speed up loading self.site.enable_cache() - entities = [] - pages = self.site.get_page( - WtSite.GetPageParam(titles=param.titles, offline_pages=param.offline_pages) - ).pages - for page in pages: - entity = None - schemas = [] - schemas_fetched = True - jsondata = page.get_slot_content("jsondata") - if param.remove_empty: - remove_empty(jsondata) - if jsondata: - for category in jsondata["type"]: - schema = ( - self.site - .get_page( - WtSite.GetPageParam( - titles=[category], offline_pages=param.offline_pages + # the restore runs in a finally block so that an exception from any of the + # calls below cannot leave the cache enabled for the rest of the process + try: + entities = [] + pages = self.site.get_page( + WtSite.GetPageParam( + titles=param.titles, offline_pages=param.offline_pages + ) + ).pages + for page in pages: + entity = None + schemas = [] + schemas_fetched = True + jsondata = page.get_slot_content("jsondata") + if param.remove_empty: + remove_empty(jsondata) + if jsondata: + for category in jsondata["type"]: + schema = ( + self.site + .get_page( + WtSite.GetPageParam( + titles=[category], offline_pages=param.offline_pages + ) ) + .pages[0] + .get_slot_content("jsonschema") ) - .pages[0] - .get_slot_content("jsonschema") - ) - schemas.append(schema) - # generate model if not already exists - cls_name: str = schema["title"] - # If a schema_to_use is provided, we do not need to check if the - # model exists - if not param.model_to_use: - if not hasattr(model, cls_name): - if param.autofetch_schema: - self.fetch_schema( - OSW.FetchSchemaParam( - schema_title=category, - mode="append", - offline_pages=param.offline_pages, + schemas.append(schema) + # generate model if not already exists + cls_name: str = schema["title"] + # If a schema_to_use is provided, we do not need to check if + # the model exists + if not param.model_to_use: + if not hasattr(model, cls_name): + if param.autofetch_schema: + self.fetch_schema( + OSW.FetchSchemaParam( + schema_title=category, + mode="append", + offline_pages=param.offline_pages, + ) ) + if not hasattr(model, cls_name): + schemas_fetched = False + print( + f"Error: Model {cls_name} not found. Schema {category} " + f"needs to be fetched first." ) - if not hasattr(model, cls_name): - schemas_fetched = False - print( - f"Error: Model {cls_name} not found. Schema {category} " - f"needs to be fetched first." - ) - if not schemas_fetched: - continue - - try: - if param.model_to_use: - entity: model.OswBaseModel = param.model_to_use(**jsondata) + if not schemas_fetched: + continue - elif len(schemas) == 0: - _logger.error("Error: no schema defined") + try: + if param.model_to_use: + entity: model.OswBaseModel = param.model_to_use(**jsondata) - elif len(schemas) == 1: - cls: Type[model.Entity] = getattr(model, schemas[0]["title"]) - entity: model.Entity = cls(**jsondata) + elif len(schemas) == 0: + _logger.error("Error: no schema defined") - else: - bases = [] - for schema in schemas: - bases.append(getattr(model, schema["title"])) - cls = create_model("Test", __base__=tuple(bases)) - entity: model.Entity = cls(**jsondata) - except Exception as e: - _logger.error(f"Error creating entity from page {page.title}: {e}") - # legacy: `entity` is annotated as OswBaseModel and Entity above - entity = None # ty: ignore[conflicting-declarations] + elif len(schemas) == 1: + cls: Type[model.Entity] = getattr(model, schemas[0]["title"]) + entity: model.Entity = cls(**jsondata) - if entity is not None: - # make sure we do not override existing metadata - if not hasattr(entity, "meta") or entity.meta is None: - entity.meta = model.Meta() - if ( - not hasattr(entity.meta, "wiki_page") - or entity.meta.wiki_page is None - ): - entity.meta.wiki_page = model.WikiPage() - entity.meta.wiki_page.namespace = namespace_from_full_title(page.title) - entity.meta.wiki_page.title = title_from_full_title(page.title) + else: + bases = [] + for schema in schemas: + bases.append(getattr(model, schema["title"])) + cls = create_model("Test", __base__=tuple(bases)) + entity: model.Entity = cls(**jsondata) + except Exception as e: + _logger.error(f"Error creating entity from page {page.title}: {e}") + # legacy: `entity` is annotated as OswBaseModel and Entity above + entity = None # ty: ignore[conflicting-declarations] + + if entity is not None: + # make sure we do not override existing metadata + if not hasattr(entity, "meta") or entity.meta is None: + entity.meta = model.Meta() + if ( + not hasattr(entity.meta, "wiki_page") + or entity.meta.wiki_page is None + ): + entity.meta.wiki_page = model.WikiPage() + entity.meta.wiki_page.namespace = namespace_from_full_title( + page.title + ) + entity.meta.wiki_page.title = title_from_full_title(page.title) - entities.append(entity) - # restore original cache state - if cache_state: - self.site.enable_cache() - else: - self.site.disable_cache() + entities.append(entity) + finally: + # restore original cache state + if cache_state: + self.site.enable_cache() + else: + self.site.disable_cache() if isinstance(entity_title, str): # single title if len(entities) >= 1: @@ -1701,8 +1709,11 @@ class StoreEntityResult(OswBaseModel): """The pages that have been successfully stored, keyed by full page title. On partial failure this contains only the successfully-stored pages.""" failed: Dict[str, Exception] = {} - """Entities that could not be stored, keyed by full page title and mapped to - the exception that caused the failure. Empty on full success.""" + """Entities that could not be stored, mapped to the exception that caused the + failure. Empty on full success. The key is the full page title where one could + be determined. For an entity whose title or namespace could not be resolved it + falls back to the entity name, then to its uuid, then to 'unknown', so do not + parse this key as 'namespace:title'.""" class Config: arbitrary_types_allowed = True @@ -1859,20 +1870,23 @@ def store_entity_( entity_name = getattr(entity_, "name", None) or getattr( entity_, "uuid", "unknown" ) - _logger.error(f"Error getting title for entity '{entity_name}': {e}") - return + # raise instead of returning: a plain return is not an exception, + # so the collector loop below would record the entity in neither + # created_pages nor failed and store_entity would report success + raise ValueError( + f"Error getting title for entity '{entity_name}': {e}" + ) from e if namespace_ is None: namespace_ = get_namespace(entity_) if namespace_ is None or title_ is None: entity_name = getattr(entity_, "name", None) or getattr( entity_, "uuid", "unknown" ) - _logger.error( + raise TypeError( f"Unsupported entity type: namespace={namespace_}, " f"title={title_}, entity name='{entity_name}', " f"type={type(entity_).__name__}" ) - return if overwrite_class_param is None: raise TypeError("'overwrite_class_param' must not be None!") entity_title = namespace_ + ":" + title_ diff --git a/tests/test_load_entity_cache_restore.py b/tests/test_load_entity_cache_restore.py new file mode 100644 index 00000000..3f63854e --- /dev/null +++ b/tests/test_load_entity_cache_restore.py @@ -0,0 +1,108 @@ +"""Unit tests for the cache-state restore in load_entity(). + +Regression guard for #183: https://github.com/OpenSemanticLab/osw-python/issues/183 +load_entity() saved the original cache state before fetching pages and restored +it afterwards, but the restore was not in a finally block. An exception raised +while fetching or parsing a page left the cache enabled (or disabled) for the +rest of the process instead of restoring the state the caller had before the +call. + +These run fully offline: the fake site never touches the network. +""" + +from types import SimpleNamespace + +import pytest + +from osw.core import OSW + + +class _FakeSite: + def __init__(self, cache_enabled, pages=None, fails=False): + self.cache_enabled = cache_enabled + self._pages = pages if pages is not None else [] + self.fails = fails + + def get_cache_enabled(self): + return self.cache_enabled + + def enable_cache(self): + self.cache_enabled = True + + def disable_cache(self): + self.cache_enabled = False + + def get_page(self, param): + if self.fails: + raise RuntimeError("the wiki is not reachable") + return SimpleNamespace(pages=self._pages) + + +@pytest.mark.parametrize("disable_cache", [False, True]) +def test_cache_stays_disabled_when_get_page_raises(disable_cache): + site = _FakeSite(cache_enabled=False, fails=True) + osw_obj = OSW.construct(site=site) + + with pytest.raises(RuntimeError): + osw_obj.load_entity( + OSW.LoadEntityParam(titles=["Item:Foo"], disable_cache=disable_cache) + ) + + assert site.get_cache_enabled() is False + + +@pytest.mark.parametrize("disable_cache", [False, True]) +def test_cache_stays_enabled_when_get_page_raises(disable_cache): + site = _FakeSite(cache_enabled=True, fails=True) + osw_obj = OSW.construct(site=site) + + with pytest.raises(RuntimeError): + osw_obj.load_entity( + OSW.LoadEntityParam(titles=["Item:Foo"], disable_cache=disable_cache) + ) + + assert site.get_cache_enabled() is True + + +class _FailingPage: + """A page whose slot content cannot be read. + + Raises from inside the per-page loop rather than from get_page, so that the + try block is shown to cover the whole body and not only its first call. + """ + + title = "Item:Foo" + + def get_slot_content(self, slot): + raise RuntimeError("the slot content is not readable") + + +@pytest.mark.parametrize("cache_enabled", [False, True]) +def test_cache_is_restored_when_the_page_loop_raises(cache_enabled): + site = _FakeSite(cache_enabled=cache_enabled, pages=[_FailingPage()]) + osw_obj = OSW.construct(site=site) + + with pytest.raises(RuntimeError): + osw_obj.load_entity( + OSW.LoadEntityParam(titles=["Item:Foo"], disable_cache=True) + ) + + assert site.get_cache_enabled() is cache_enabled + + +def test_cache_state_is_restored_after_normal_path_disabled(): + site = _FakeSite(cache_enabled=False) + osw_obj = OSW.construct(site=site) + + osw_obj.load_entity(OSW.LoadEntityParam(titles=["Item:Foo"])) + + assert site.get_cache_enabled() is False + + +def test_cache_state_is_restored_after_normal_path_enabled(): + site = _FakeSite(cache_enabled=True) + osw_obj = OSW.construct(site=site) + + osw_obj.load_entity(OSW.LoadEntityParam(titles=["Item:Foo"])) + + assert site.get_cache_enabled() is True diff --git a/tests/test_store_entity_silent_drop.py b/tests/test_store_entity_silent_drop.py new file mode 100644 index 00000000..8e8db0c0 --- /dev/null +++ b/tests/test_store_entity_silent_drop.py @@ -0,0 +1,144 @@ +"""Unit tests for store_entity() dropping entities without recording a failure. + +Regression guard for #183: https://github.com/OpenSemanticLab/osw-python/issues/183 +store_entity_() returned plainly (instead of raising) when it could not determine +an entity's title or namespace. A plain return is not an exception, so the +collector loop in store_entity() never recorded the entity in failed, and it was +never in created_pages either: the entity was silently dropped without being +reported to the caller. store_entity_() must now raise in both cases so the +existing collector loop records them. + +These run fully offline: WtPage.init, the overwrite policy, WtPage.edit and the +write verification are stubbed, so no network is required. +""" + +import pytest + +import osw.core as core_mod +import osw.model.entity as model +from osw.core import OSW +from osw.utils.wiki import get_namespace, get_title +from osw.wtsite import WtPage + + +def _title(entity): + return f"{get_namespace(entity)}:{get_title(entity)}" + + +@pytest.fixture +def offline_osw(monkeypatch): + # no network when a WtPage is constructed with do_init=True; mimic the + # do_init=False branch of WtPage.__init__ which sets .exists + monkeypatch.setattr(WtPage, "init", lambda self: setattr(self, "exists", False)) + # bypass the overwrite policy: return the page that was built for the entity + monkeypatch.setattr( + OSW, "_apply_overwrite_policy", staticmethod(lambda param: param.page) + ) + monkeypatch.setattr(WtPage, "edit", lambda self, *a, **kw: None) + # the write verification would query the wiki. These tests are not about + # verification, so report every edited page as existing. See + # tests/test_store_entity_verify.py for the write verification tests. + monkeypatch.setattr(OSW, "_get_missing_page_titles", lambda self, titles: []) + return OSW.construct(site=object()) + + +def test_title_failure_is_reported_serial(offline_osw, monkeypatch): + def _raise_get_title(entity): + raise RuntimeError("boom") + + monkeypatch.setattr(core_mod, "get_title", _raise_get_title) + item = model.Item(label=[model.Label(text="Solo")]) + + with pytest.raises(OSW.StoreEntityPartialError) as exc_info: + offline_osw.store_entity(OSW.StoreEntityParam(entities=[item], parallel=False)) + + err = exc_info.value + assert len(err.failed) == 1 + (exc,) = err.failed.values() + assert isinstance(exc, ValueError) + assert "Error getting title for entity" in str(exc) + + +def test_title_failure_is_reported_parallel(offline_osw, monkeypatch): + def _raise_get_title(entity): + raise RuntimeError("boom") + + monkeypatch.setattr(core_mod, "get_title", _raise_get_title) + item = model.Item(label=[model.Label(text="Solo")]) + + with pytest.raises(OSW.StoreEntityPartialError) as exc_info: + offline_osw.store_entity(OSW.StoreEntityParam(entities=[item], parallel=True)) + + err = exc_info.value + assert len(err.failed) == 1 + (exc,) = err.failed.values() + assert isinstance(exc, ValueError) + assert "Error getting title for entity" in str(exc) + + +def test_missing_namespace_is_reported(offline_osw, monkeypatch): + monkeypatch.setattr(core_mod, "get_namespace", lambda entity: None) + item = model.Item(label=[model.Label(text="NoNamespace")]) + + with pytest.raises(OSW.StoreEntityPartialError) as exc_info: + offline_osw.store_entity(OSW.StoreEntityParam(entities=[item], parallel=False)) + + err = exc_info.value + assert len(err.failed) == 1 + (exc,) = err.failed.values() + assert isinstance(exc, TypeError) + assert "Unsupported entity type" in str(exc) + + +def test_missing_title_is_reported(offline_osw, monkeypatch): + monkeypatch.setattr(core_mod, "get_title", lambda entity: None) + item = model.Item(label=[model.Label(text="NoTitle")]) + + with pytest.raises(OSW.StoreEntityPartialError) as exc_info: + offline_osw.store_entity(OSW.StoreEntityParam(entities=[item], parallel=False)) + + err = exc_info.value + assert len(err.failed) == 1 + (exc,) = err.failed.values() + assert isinstance(exc, TypeError) + assert "Unsupported entity type" in str(exc) + + +def test_only_the_failing_entity_of_a_batch_is_reported(offline_osw, monkeypatch): + real_get_title = get_title + good_item = model.Item(label=[model.Label(text="Good")]) + bad_item = model.Item(label=[model.Label(text="Bad")]) + good_title = _title(good_item) + + # store_entity() re-validates its 'entities' param, so the object identity + # of good_item/bad_item is not preserved past that point. Recognize the + # failing entity by its label instead. + def _get_title(entity): + if entity.label and entity.label[0].text == "Bad": + raise RuntimeError("boom") + return real_get_title(entity) + + monkeypatch.setattr(core_mod, "get_title", _get_title) + + with pytest.raises(OSW.StoreEntityPartialError) as exc_info: + offline_osw.store_entity( + OSW.StoreEntityParam(entities=[good_item, bad_item], parallel=False) + ) + + err = exc_info.value + assert len(err.result.pages) == 1 + assert len(err.failed) == 1 + assert good_title in err.result.pages + assert err.stored == [good_title] + + +def test_normal_path_is_unaffected(offline_osw): + item = model.Item(label=[model.Label(text="Fine")]) + title = _title(item) + + result = offline_osw.store_entity( + OSW.StoreEntityParam(entities=[item], parallel=False) + ) + + assert set(result.pages.keys()) == {title} + assert result.failed == {}