fix(core): report dropped entities and restore the cache on error

Merge pull request #185 from OpenSemanticLab/fix/183-silent-drop-and-cache-restore

Author: LukasGold

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_

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