diff --git a/src/osw/core.py b/src/osw/core.py index b85a4b7a..6e796283 100644 --- a/src/osw/core.py +++ b/src/osw/core.py @@ -496,24 +496,36 @@ def fetch_schema( first = True last = False results = [] - for schema_title in fetchSchemaParam.schema_title: - last = schema_title == fetchSchemaParam.schema_title[-1] - mode = fetchSchemaParam.mode - if not first: # 'replace' makes only sense for the first schema - mode = "append" - res = self._fetch_schema( - OSW._FetchSchemaParam( - schema_title=schema_title, - mode=mode, - final=last, - generate_annotations=fetchSchemaParam.generate_annotations, - generator_options=fetchSchemaParam.generator_options, - offline_pages=fetchSchemaParam.offline_pages, - result_model_path=fetchSchemaParam.result_model_path, + # the page cache is enabled once for the whole operation, because the same + # schema pages are read repeatedly while $refs are resolved. The state is + # taken and restored here and not per schema title: _fetch_schema would + # snapshot the state its own predecessor has already changed. The restore + # runs in a finally block so that an early return or an exception in + # _fetch_schema cannot leave the cache enabled for the rest of the process. + site_cache_state = self.site.get_cache_enabled() + self.site.enable_cache() + try: + for schema_title in fetchSchemaParam.schema_title: + last = schema_title == fetchSchemaParam.schema_title[-1] + mode = fetchSchemaParam.mode + if not first: # 'replace' makes only sense for the first schema + mode = "append" + res = self._fetch_schema( + OSW._FetchSchemaParam( + schema_title=schema_title, + mode=mode, + final=last, + generate_annotations=fetchSchemaParam.generate_annotations, + generator_options=fetchSchemaParam.generator_options, + offline_pages=fetchSchemaParam.offline_pages, + result_model_path=fetchSchemaParam.result_model_path, + ) ) - ) - results.append(res) - first = False + results.append(res) + first = False + finally: + if not site_cache_state: + self.site.disable_cache() # restore original state # merge unique results and return merged_result = OSW.FetchSchemaResult( @@ -594,9 +606,13 @@ def _fetch_schema( ---------- fetchSchemaParam See FetchSchemaParam, by default None + + Notes + ----- + The page cache is enabled and restored by the calling fetch_schema(), not + here. This method is called once per schema title and recursively per $ref, + so a snapshot taken here would read the state a previous call has set. """ - site_cache_state = self.site.get_cache_enabled() - self.site.enable_cache() if fetchSchemaParam is None: fetchSchemaParam = OSW._FetchSchemaParam() schema_title = fetchSchemaParam.schema_title @@ -1056,8 +1072,6 @@ def _fetch_schema( if fetchSchemaParam.final: importlib.reload(model) # reload the updated module - if not site_cache_state: - self.site.disable_cache() # restore original state return OSW.FetchSchemaResult( fetched_schema_titles=fetchSchemaParam.fetched_schema_titles, diff --git a/tests/test_fetch_schema_cache.py b/tests/test_fetch_schema_cache.py new file mode 100644 index 00000000..24972d30 --- /dev/null +++ b/tests/test_fetch_schema_cache.py @@ -0,0 +1,136 @@ +"""Unit tests for the page cache handling of OSW.fetch_schema(). + +Regression guard for #176: the cache state was taken and restored once per +schema title inside _fetch_schema(). With two or more titles the second +snapshot already read the state the first call had set, so the restore never +disabled the cache again. An early return or an exception skipped the restore +as well. fetch_schema() must now take the state once and restore it in every +case. +""" + +import threading + +import pytest + +from osw.core import OSW +from osw.wtsite import WtSite + + +def _make_fake_wtsite(cache_enabled: bool) -> WtSite: + """A WtSite that performs no network calls, with a known cache state.""" + ws = WtSite.__new__(WtSite) + ws._session_lock = threading.RLock() + ws._page_cache = {} + ws._cache_enabled = cache_enabled + return ws + + +def _make_osw(cache_enabled: bool) -> OSW: + """An OSW bound to that WtSite, bypassing __init__ and validation.""" + return OSW.construct(site=_make_fake_wtsite(cache_enabled)) + + +def _stub_fetch_schema( + monkeypatch, seen_states: list, fail_on: str = None, leaks: bool = False +): + """Replace the per-title worker. Records the cache state it is called with. + + With leaks=True the worker enables the cache and never restores it, which is + what the previous _fetch_schema() did for every title but the last one. + """ + + def stub(self, fetchSchemaParam=None): + seen_states.append(self.site.get_cache_enabled()) + if leaks: + self.site.enable_cache() + if fail_on is not None and fetchSchemaParam.schema_title == fail_on: + raise RuntimeError("fetching the schema failed") + return OSW.FetchSchemaResult( + fetched_schema_titles=[fetchSchemaParam.schema_title] + ) + + monkeypatch.setattr(OSW, "_fetch_schema", stub) + + +def test_cache_is_disabled_again_after_several_titles(monkeypatch): + osw_obj = _make_osw(cache_enabled=False) + seen_states = [] + _stub_fetch_schema(monkeypatch, seen_states) + + osw_obj.fetch_schema( + OSW.FetchSchemaParam(schema_title=["Category:Item", "Category:Entity"]) + ) + + assert seen_states == [True, True] + assert osw_obj.site.get_cache_enabled() is False + + +def test_cache_is_disabled_again_after_a_single_title(monkeypatch): + osw_obj = _make_osw(cache_enabled=False) + seen_states = [] + _stub_fetch_schema(monkeypatch, seen_states) + + osw_obj.fetch_schema(OSW.FetchSchemaParam(schema_title="Category:Item")) + + assert seen_states == [True] + assert osw_obj.site.get_cache_enabled() is False + + +def test_cache_stays_enabled_if_the_caller_had_it_enabled(monkeypatch): + osw_obj = _make_osw(cache_enabled=True) + seen_states = [] + _stub_fetch_schema(monkeypatch, seen_states) + + osw_obj.fetch_schema( + OSW.FetchSchemaParam(schema_title=["Category:Item", "Category:Entity"]) + ) + + assert seen_states == [True, True] + assert osw_obj.site.get_cache_enabled() is True + + +def test_a_worker_that_leaves_the_cache_enabled_does_not_leak(monkeypatch): + """The reported defect: the per-title worker enabled the cache and kept it.""" + osw_obj = _make_osw(cache_enabled=False) + seen_states = [] + _stub_fetch_schema(monkeypatch, seen_states, leaks=True) + + osw_obj.fetch_schema( + OSW.FetchSchemaParam(schema_title=["Category:Item", "Category:Entity"]) + ) + + assert osw_obj.site.get_cache_enabled() is False + + +def test_cache_is_restored_when_a_title_raises(monkeypatch): + osw_obj = _make_osw(cache_enabled=False) + seen_states = [] + _stub_fetch_schema(monkeypatch, seen_states, fail_on="Category:Entity", leaks=True) + + with pytest.raises(RuntimeError): + osw_obj.fetch_schema( + OSW.FetchSchemaParam(schema_title=["Category:Item", "Category:Entity"]) + ) + + assert osw_obj.site.get_cache_enabled() is False + + +def test_cache_is_restored_when_the_last_title_returns_early(monkeypatch): + """A missing schema page returns before the end of _fetch_schema().""" + osw_obj = _make_osw(cache_enabled=False) + + def stub(self, fetchSchemaParam=None): + # mirrors the early return for a page that does not exist, which happens + # after the previous _fetch_schema() had enabled the cache + self.site.enable_cache() + return OSW.FetchSchemaResult( + error_messages=[f"Page {fetchSchemaParam.schema_title} does not exist"] + ) + + monkeypatch.setattr(OSW, "_fetch_schema", stub) + + osw_obj.fetch_schema( + OSW.FetchSchemaParam(schema_title=["Category:Item", "Category:Missing"]) + ) + + assert osw_obj.site.get_cache_enabled() is False