Skip to content

Commit f09dcf6

Browse files
committed
fix(core): restore the page cache state once per fetch_schema call
- fetch_schema takes the cache state before the loop over schema titles - the restore runs in a finally block, so an early return or an exception in _fetch_schema can no longer leave the cache enabled - _fetch_schema no longer snapshots a state its predecessor has changed - add unit tests for the multi-title, exception and early-return paths Closes #176
1 parent 848615c commit f09dcf6

2 files changed

Lines changed: 171 additions & 21 deletions

File tree

‎src/osw/core.py‎

Lines changed: 35 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -496,24 +496,36 @@ def fetch_schema(
496496
first = True
497497
last = False
498498
results = []
499-
for schema_title in fetchSchemaParam.schema_title:
500-
last = schema_title == fetchSchemaParam.schema_title[-1]
501-
mode = fetchSchemaParam.mode
502-
if not first: # 'replace' makes only sense for the first schema
503-
mode = "append"
504-
res = self._fetch_schema(
505-
OSW._FetchSchemaParam(
506-
schema_title=schema_title,
507-
mode=mode,
508-
final=last,
509-
generate_annotations=fetchSchemaParam.generate_annotations,
510-
generator_options=fetchSchemaParam.generator_options,
511-
offline_pages=fetchSchemaParam.offline_pages,
512-
result_model_path=fetchSchemaParam.result_model_path,
499+
# the page cache is enabled once for the whole operation, because the same
500+
# schema pages are read repeatedly while $refs are resolved. The state is
501+
# taken and restored here and not per schema title: _fetch_schema would
502+
# snapshot the state its own predecessor has already changed. The restore
503+
# runs in a finally block so that an early return or an exception in
504+
# _fetch_schema cannot leave the cache enabled for the rest of the process.
505+
site_cache_state = self.site.get_cache_enabled()
506+
self.site.enable_cache()
507+
try:
508+
for schema_title in fetchSchemaParam.schema_title:
509+
last = schema_title == fetchSchemaParam.schema_title[-1]
510+
mode = fetchSchemaParam.mode
511+
if not first: # 'replace' makes only sense for the first schema
512+
mode = "append"
513+
res = self._fetch_schema(
514+
OSW._FetchSchemaParam(
515+
schema_title=schema_title,
516+
mode=mode,
517+
final=last,
518+
generate_annotations=fetchSchemaParam.generate_annotations,
519+
generator_options=fetchSchemaParam.generator_options,
520+
offline_pages=fetchSchemaParam.offline_pages,
521+
result_model_path=fetchSchemaParam.result_model_path,
522+
)
513523
)
514-
)
515-
results.append(res)
516-
first = False
524+
results.append(res)
525+
first = False
526+
finally:
527+
if not site_cache_state:
528+
self.site.disable_cache() # restore original state
517529

518530
# merge unique results and return
519531
merged_result = OSW.FetchSchemaResult(
@@ -594,9 +606,13 @@ def _fetch_schema(
594606
----------
595607
fetchSchemaParam
596608
See FetchSchemaParam, by default None
609+
610+
Notes
611+
-----
612+
The page cache is enabled and restored by the calling fetch_schema(), not
613+
here. This method is called once per schema title and recursively per $ref,
614+
so a snapshot taken here would read the state a previous call has set.
597615
"""
598-
site_cache_state = self.site.get_cache_enabled()
599-
self.site.enable_cache()
600616
if fetchSchemaParam is None:
601617
fetchSchemaParam = OSW._FetchSchemaParam()
602618
schema_title = fetchSchemaParam.schema_title
@@ -1056,8 +1072,6 @@ def _fetch_schema(
10561072

10571073
if fetchSchemaParam.final:
10581074
importlib.reload(model) # reload the updated module
1059-
if not site_cache_state:
1060-
self.site.disable_cache() # restore original state
10611075

10621076
return OSW.FetchSchemaResult(
10631077
fetched_schema_titles=fetchSchemaParam.fetched_schema_titles,

‎tests/test_fetch_schema_cache.py‎

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,136 @@
1+
"""Unit tests for the page cache handling of OSW.fetch_schema().
2+
3+
Regression guard for #176: the cache state was taken and restored once per
4+
schema title inside _fetch_schema(). With two or more titles the second
5+
snapshot already read the state the first call had set, so the restore never
6+
disabled the cache again. An early return or an exception skipped the restore
7+
as well. fetch_schema() must now take the state once and restore it in every
8+
case.
9+
"""
10+
11+
import threading
12+
13+
import pytest
14+
15+
from osw.core import OSW
16+
from osw.wtsite import WtSite
17+
18+
19+
def _make_fake_wtsite(cache_enabled: bool) -> WtSite:
20+
"""A WtSite that performs no network calls, with a known cache state."""
21+
ws = WtSite.__new__(WtSite)
22+
ws._session_lock = threading.RLock()
23+
ws._page_cache = {}
24+
ws._cache_enabled = cache_enabled
25+
return ws
26+
27+
28+
def _make_osw(cache_enabled: bool) -> OSW:
29+
"""An OSW bound to that WtSite, bypassing __init__ and validation."""
30+
return OSW.construct(site=_make_fake_wtsite(cache_enabled))
31+
32+
33+
def _stub_fetch_schema(
34+
monkeypatch, seen_states: list, fail_on: str = None, leaks: bool = False
35+
):
36+
"""Replace the per-title worker. Records the cache state it is called with.
37+
38+
With leaks=True the worker enables the cache and never restores it, which is
39+
what the previous _fetch_schema() did for every title but the last one.
40+
"""
41+
42+
def stub(self, fetchSchemaParam=None):
43+
seen_states.append(self.site.get_cache_enabled())
44+
if leaks:
45+
self.site.enable_cache()
46+
if fail_on is not None and fetchSchemaParam.schema_title == fail_on:
47+
raise RuntimeError("fetching the schema failed")
48+
return OSW.FetchSchemaResult(
49+
fetched_schema_titles=[fetchSchemaParam.schema_title]
50+
)
51+
52+
monkeypatch.setattr(OSW, "_fetch_schema", stub)
53+
54+
55+
def test_cache_is_disabled_again_after_several_titles(monkeypatch):
56+
osw_obj = _make_osw(cache_enabled=False)
57+
seen_states = []
58+
_stub_fetch_schema(monkeypatch, seen_states)
59+
60+
osw_obj.fetch_schema(
61+
OSW.FetchSchemaParam(schema_title=["Category:Item", "Category:Entity"])
62+
)
63+
64+
assert seen_states == [True, True]
65+
assert osw_obj.site.get_cache_enabled() is False
66+
67+
68+
def test_cache_is_disabled_again_after_a_single_title(monkeypatch):
69+
osw_obj = _make_osw(cache_enabled=False)
70+
seen_states = []
71+
_stub_fetch_schema(monkeypatch, seen_states)
72+
73+
osw_obj.fetch_schema(OSW.FetchSchemaParam(schema_title="Category:Item"))
74+
75+
assert seen_states == [True]
76+
assert osw_obj.site.get_cache_enabled() is False
77+
78+
79+
def test_cache_stays_enabled_if_the_caller_had_it_enabled(monkeypatch):
80+
osw_obj = _make_osw(cache_enabled=True)
81+
seen_states = []
82+
_stub_fetch_schema(monkeypatch, seen_states)
83+
84+
osw_obj.fetch_schema(
85+
OSW.FetchSchemaParam(schema_title=["Category:Item", "Category:Entity"])
86+
)
87+
88+
assert seen_states == [True, True]
89+
assert osw_obj.site.get_cache_enabled() is True
90+
91+
92+
def test_a_worker_that_leaves_the_cache_enabled_does_not_leak(monkeypatch):
93+
"""The reported defect: the per-title worker enabled the cache and kept it."""
94+
osw_obj = _make_osw(cache_enabled=False)
95+
seen_states = []
96+
_stub_fetch_schema(monkeypatch, seen_states, leaks=True)
97+
98+
osw_obj.fetch_schema(
99+
OSW.FetchSchemaParam(schema_title=["Category:Item", "Category:Entity"])
100+
)
101+
102+
assert osw_obj.site.get_cache_enabled() is False
103+
104+
105+
def test_cache_is_restored_when_a_title_raises(monkeypatch):
106+
osw_obj = _make_osw(cache_enabled=False)
107+
seen_states = []
108+
_stub_fetch_schema(monkeypatch, seen_states, fail_on="Category:Entity", leaks=True)
109+
110+
with pytest.raises(RuntimeError):
111+
osw_obj.fetch_schema(
112+
OSW.FetchSchemaParam(schema_title=["Category:Item", "Category:Entity"])
113+
)
114+
115+
assert osw_obj.site.get_cache_enabled() is False
116+
117+
118+
def test_cache_is_restored_when_the_last_title_returns_early(monkeypatch):
119+
"""A missing schema page returns before the end of _fetch_schema()."""
120+
osw_obj = _make_osw(cache_enabled=False)
121+
122+
def stub(self, fetchSchemaParam=None):
123+
# mirrors the early return for a page that does not exist, which happens
124+
# after the previous _fetch_schema() had enabled the cache
125+
self.site.enable_cache()
126+
return OSW.FetchSchemaResult(
127+
error_messages=[f"Page {fetchSchemaParam.schema_title} does not exist"]
128+
)
129+
130+
monkeypatch.setattr(OSW, "_fetch_schema", stub)
131+
132+
osw_obj.fetch_schema(
133+
OSW.FetchSchemaParam(schema_title=["Category:Item", "Category:Missing"])
134+
)
135+
136+
assert osw_obj.site.get_cache_enabled() is False

0 commit comments

Comments
 (0)