Skip to content

Commit 3c9278d

Browse files
LukasGoldLukasGold
committed
fix(core): skip a page whose category has no usable schema
- the category loop read the schema title outside any try, so a category page with no jsonschema slot, no title key, or a non-string title ended load_entity and left the remaining titles unloaded - the loop body is now wrapped in try/except, which logs the page title, the category and the exception, then skips the page like the two error paths that already exist in the same function Closes #202
1 parent 0017ef0 commit 3c9278d

2 files changed

Lines changed: 254 additions & 88 deletions

File tree

‎src/osw/core.py‎

Lines changed: 100 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -1254,101 +1254,113 @@ def load_entity(
12541254
remove_empty(jsondata)
12551255
if jsondata:
12561256
for category in jsondata["type"]:
1257-
schema = (
1258-
self.site
1259-
.get_page(
1260-
WtSite.GetPageParam(
1261-
titles=[category], offline_pages=param.offline_pages
1257+
# one unusable category must not end the whole call
1258+
try:
1259+
schema = (
1260+
self.site
1261+
.get_page(
1262+
WtSite.GetPageParam(
1263+
titles=[category],
1264+
offline_pages=param.offline_pages,
1265+
)
12621266
)
1267+
.pages[0]
1268+
.get_slot_content("jsonschema")
12631269
)
1264-
.pages[0]
1265-
.get_slot_content("jsonschema")
1266-
)
1267-
schemas.append(schema)
1268-
# generate model if not already exists
1269-
cls_name: str = schema["title"]
1270-
# If a schema_to_use is provided, we do not need to check if
1271-
# the model exists
1272-
if not param.model_to_use:
1273-
# Prefer a class already registered for this category
1274-
# IRI (e.g. a packaged model class) over compiling a
1275-
# new one. Compiling one anyway would take over the
1276-
# oold type registry entry for this category and hide
1277-
# the packaged class' typed fields/helpers.
1278-
registered_cls = oold_type_registry.get(category)
1279-
# Controllers and result wrappers (e.g.
1280-
# WikiFileController, UploadFileResult) inherit
1281-
# their category IRI from the model class they
1282-
# extend, and the registry keeps whichever class
1283-
# was defined last. Such a specialization asks for
1284-
# fields a plain page's jsondata does not carry, so
1285-
# prefer the canonical class from osw.model.entity
1286-
# over a subclass of it.
1287-
canonical_cls = getattr(model, cls_name, None)
1288-
if (
1289-
registered_cls is not None
1290-
# the schema title can also name a module
1291-
# attribute that is not a class
1292-
and isinstance(canonical_cls, type)
1293-
and registered_cls is not canonical_cls
1294-
and issubclass(registered_cls, canonical_cls)
1295-
):
1296-
_logger.debug(
1297-
f"Ignoring '{registered_cls}' registered for "
1298-
f"category '{category}': it specializes "
1299-
f"'{canonical_cls}', which is used instead."
1300-
)
1301-
registered_cls = None
1302-
if registered_cls is not None:
1303-
category_to_cls[category] = registered_cls
1304-
else:
1305-
if not hasattr(model, cls_name):
1306-
if param.autofetch_schema:
1307-
self.fetch_schema(
1308-
OSW.FetchSchemaParam(
1309-
schema_title=category,
1310-
mode="append",
1311-
offline_pages=param.offline_pages,
1312-
)
1313-
)
1314-
if not hasattr(model, cls_name):
1315-
schemas_fetched = False
1316-
_logger.error(
1317-
f"Model {cls_name} not found. Schema "
1318-
f"{category} needs to be fetched first."
1270+
schemas.append(schema)
1271+
# generate model if not already exists
1272+
cls_name: str = schema["title"]
1273+
# If a schema_to_use is provided, we do not need to check if
1274+
# the model exists
1275+
if not param.model_to_use:
1276+
# Prefer a class already registered for this category
1277+
# IRI (e.g. a packaged model class) over compiling a
1278+
# new one. Compiling one anyway would take over the
1279+
# oold type registry entry for this category and hide
1280+
# the packaged class' typed fields/helpers.
1281+
registered_cls = oold_type_registry.get(category)
1282+
# Controllers and result wrappers (e.g.
1283+
# WikiFileController, UploadFileResult) inherit
1284+
# their category IRI from the model class they
1285+
# extend, and the registry keeps whichever class
1286+
# was defined last. Such a specialization asks for
1287+
# fields a plain page's jsondata does not carry, so
1288+
# prefer the canonical class from osw.model.entity
1289+
# over a subclass of it.
1290+
canonical_cls = getattr(model, cls_name, None)
1291+
if (
1292+
registered_cls is not None
1293+
# the schema title can also name a module
1294+
# attribute that is not a class
1295+
and isinstance(canonical_cls, type)
1296+
and registered_cls is not canonical_cls
1297+
and issubclass(registered_cls, canonical_cls)
1298+
):
1299+
_logger.debug(
1300+
f"Ignoring '{registered_cls}' registered for "
1301+
f"category '{category}': it specializes "
1302+
f"'{canonical_cls}', which is used instead."
13191303
)
1304+
registered_cls = None
1305+
if registered_cls is not None:
1306+
category_to_cls[category] = registered_cls
13201307
else:
1321-
generated_cls = getattr(model, cls_name)
1322-
# The class we are about to use may have just
1323-
# claimed (or may already hold) the registry
1324-
# slot for this category. If a different class
1325-
# is registered for it, someone's registration
1326-
# was silently overwritten - do not raise, but
1327-
# make sure this does not pass silently.
1328-
conflicting_cls = oold_type_registry.get(category)
1329-
if (
1330-
conflicting_cls is not None
1331-
and conflicting_cls is not generated_cls
1332-
# the slot holder specializes
1333-
# generated_cls, e.g. a controller
1334-
# that inherits the category IRI:
1335-
# the case the guard above expects,
1336-
# not a conflict. A fetch can leave
1337-
# a non-class under the schema title.
1338-
and not (
1339-
isinstance(generated_cls, type)
1340-
and issubclass(
1341-
conflicting_cls, generated_cls
1308+
if not hasattr(model, cls_name):
1309+
if param.autofetch_schema:
1310+
self.fetch_schema(
1311+
OSW.FetchSchemaParam(
1312+
schema_title=category,
1313+
mode="append",
1314+
offline_pages=param.offline_pages,
1315+
)
13421316
)
1317+
if not hasattr(model, cls_name):
1318+
schemas_fetched = False
1319+
_logger.error(
1320+
f"Model {cls_name} not found. Schema "
1321+
f"{category} needs to be fetched first."
13431322
)
1344-
):
1345-
_logger.warning(
1346-
f"Class '{generated_cls}' generated for "
1347-
f"category '{category}' claims the oold "
1348-
f"type registry slot already held by a "
1349-
f"different class '{conflicting_cls}'."
1323+
else:
1324+
generated_cls = getattr(model, cls_name)
1325+
# The class we are about to use may have just
1326+
# claimed (or may already hold) the registry
1327+
# slot for this category. If a different class
1328+
# is registered for it, someone's registration
1329+
# was silently overwritten - do not raise, but
1330+
# make sure this does not pass silently.
1331+
conflicting_cls = oold_type_registry.get(
1332+
category
13501333
)
1351-
category_to_cls[category] = generated_cls
1334+
if (
1335+
conflicting_cls is not None
1336+
and conflicting_cls is not generated_cls
1337+
# the slot holder specializes
1338+
# generated_cls, e.g. a controller
1339+
# that inherits the category IRI:
1340+
# the case the guard above expects,
1341+
# not a conflict. A fetch can leave
1342+
# a non-class under the schema title.
1343+
and not (
1344+
isinstance(generated_cls, type)
1345+
and issubclass(
1346+
conflicting_cls, generated_cls
1347+
)
1348+
)
1349+
):
1350+
_logger.warning(
1351+
f"Class '{generated_cls}' generated for "
1352+
f"category '{category}' claims the oold "
1353+
f"type registry slot already held by a "
1354+
f"different class '{conflicting_cls}'."
1355+
)
1356+
category_to_cls[category] = generated_cls
1357+
except Exception as e:
1358+
schemas_fetched = False
1359+
_logger.error(
1360+
f"Error reading the schema of category "
1361+
f"{category} for page {page.title}: {e}"
1362+
)
1363+
continue
13521364
if not schemas_fetched:
13531365
continue
13541366
# fetch_schema() reloads osw.model.entity, which replaces every

‎tests/test_load_entity_bad_category.py‎

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
"""Unit tests for load_entity() when a category page has no usable schema.
2+
3+
Regression guard for #202: https://github.com/OpenSemanticLab/osw-python/issues/202
4+
load_entity() read each category's schema in a per-page loop without a guard
5+
around it. A category page with no jsonschema slot, a schema without a
6+
"title" key, or a title that is not a string all raised out of the loop and
7+
ended the whole call, so no further title in a multi-title load was ever
8+
processed.
9+
10+
These run fully offline: the fake site never touches the network.
11+
"""
12+
13+
import json
14+
from types import SimpleNamespace
15+
16+
import osw.model.entity as model
17+
from osw.core import OSW
18+
from osw.utils.wiki import remove_empty
19+
20+
_CATEGORY = "Category:OSWBadCategoryTest0000000000000000000"
21+
22+
23+
class _FakePage:
24+
"""A page that serves a fixed jsondata or jsonschema slot."""
25+
26+
def __init__(self, title, jsondata=None, schema=None):
27+
self.title = title
28+
self._jsondata = jsondata
29+
self._schema = schema
30+
31+
def get_slot_content(self, slot):
32+
if slot == "jsondata":
33+
return self._jsondata
34+
if slot == "jsonschema":
35+
return self._schema
36+
return None
37+
38+
39+
class _FakeSite:
40+
"""A site that resolves get_page() by title, so a page fetch and a
41+
category (schema) fetch return different content."""
42+
43+
def __init__(self, pages_by_title, cache_enabled=False):
44+
self._pages_by_title = pages_by_title
45+
self.cache_enabled = cache_enabled
46+
47+
def get_cache_enabled(self):
48+
return self.cache_enabled
49+
50+
def enable_cache(self):
51+
self.cache_enabled = True
52+
53+
def disable_cache(self):
54+
self.cache_enabled = False
55+
56+
def get_page(self, param):
57+
pages = [self._pages_by_title[title] for title in param.titles]
58+
return SimpleNamespace(pages=pages)
59+
60+
61+
def _bad_category_site(title, schema):
62+
"""A site with one page whose only category serves the given schema."""
63+
jsondata = {"type": [_CATEGORY], "uuid": "00000000-0000-0000-0000-000000000000"}
64+
entity_page = _FakePage(title, jsondata=jsondata)
65+
category_page = _FakePage(_CATEGORY, schema=schema)
66+
return _FakeSite({title: entity_page, _CATEGORY: category_page})
67+
68+
69+
def test_missing_jsonschema_slot_does_not_raise():
70+
"""get_slot_content("jsonschema") returning None must not raise."""
71+
title = "Item:BadNoSlot"
72+
site = _bad_category_site(title, schema=None)
73+
osw_obj = OSW.construct(site=site)
74+
75+
result = osw_obj.load_entity(title)
76+
77+
assert result is None
78+
79+
80+
def test_schema_without_title_key_does_not_raise():
81+
"""A schema dict with no "title" key must not raise."""
82+
title = "Item:BadNoTitle"
83+
site = _bad_category_site(title, schema={})
84+
osw_obj = OSW.construct(site=site)
85+
86+
result = osw_obj.load_entity(title)
87+
88+
assert result is None
89+
90+
91+
def test_schema_title_not_a_string_does_not_raise():
92+
"""A "title" that is not a string must not raise."""
93+
title = "Item:BadTitleType"
94+
site = _bad_category_site(title, schema={"title": 123})
95+
osw_obj = OSW.construct(site=site)
96+
97+
result = osw_obj.load_entity(title)
98+
99+
assert result is None
100+
101+
102+
def _valid_item_page(title):
103+
"""A page whose category and jsondata build a real model.Item entity."""
104+
item = model.Item(label=[model.Label(text="Test Item")])
105+
category = item.type[0]
106+
jsondata = json.loads(item.json(exclude_none=True))
107+
remove_empty(jsondata)
108+
entity_page = _FakePage(title, jsondata=jsondata)
109+
category_page = _FakePage(category, schema={"title": "Item"})
110+
return entity_page, category_page, item
111+
112+
113+
def test_bad_category_on_first_page_does_not_block_the_second():
114+
"""A multi-title load must not lose every title because one page's
115+
category is unusable; the rest of the loop must still be processed."""
116+
bad_title = "Item:BadFirst"
117+
good_title = "Item:GoodSecond"
118+
119+
bad_jsondata = {
120+
"type": [_CATEGORY],
121+
"uuid": "11111111-1111-1111-1111-111111111111",
122+
}
123+
bad_entity_page = _FakePage(bad_title, jsondata=bad_jsondata)
124+
bad_category_page = _FakePage(_CATEGORY, schema=None)
125+
126+
good_entity_page, good_category_page, item = _valid_item_page(good_title)
127+
128+
site = _FakeSite({
129+
bad_title: bad_entity_page,
130+
good_title: good_entity_page,
131+
_CATEGORY: bad_category_page,
132+
good_category_page.title: good_category_page,
133+
})
134+
osw_obj = OSW.construct(site=site)
135+
136+
result = osw_obj.load_entity([bad_title, good_title])
137+
138+
assert len(result) == 1
139+
assert result[0].uuid == item.uuid
140+
141+
142+
def test_bad_category_logs_the_page_title_and_the_category(caplog):
143+
"""The logged error must name both the failing page and the category, so
144+
the fault can be traced back to the wiki page that needs a fix."""
145+
title = "Item:BadLogged"
146+
site = _bad_category_site(title, schema=None)
147+
osw_obj = OSW.construct(site=site)
148+
149+
osw_obj.load_entity(title)
150+
151+
assert any(
152+
title in record.message and _CATEGORY in record.message
153+
for record in caplog.records
154+
)

0 commit comments

Comments
 (0)