Skip to content

Commit 2fc9916

Browse files
LukasGoldLukasGold
authored
Merge pull request #209 from OpenSemanticLab/fix/202-category-loop-error-handling
fix(core): skip a page whose category has no usable schema
2 parents 5dfdf17 + 3ed9616 commit 2fc9916

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
@@ -1440,101 +1440,113 @@ def load_entity(
14401440
remove_empty(jsondata)
14411441
if jsondata:
14421442
for category in jsondata["type"]:
1443-
schema = (
1444-
self.site
1445-
.get_page(
1446-
WtSite.GetPageParam(
1447-
titles=[category], offline_pages=param.offline_pages
1443+
# one unusable category must not end the whole call
1444+
try:
1445+
schema = (
1446+
self.site
1447+
.get_page(
1448+
WtSite.GetPageParam(
1449+
titles=[category],
1450+
offline_pages=param.offline_pages,
1451+
)
14481452
)
1453+
.pages[0]
1454+
.get_slot_content("jsonschema")
14491455
)
1450-
.pages[0]
1451-
.get_slot_content("jsonschema")
1452-
)
1453-
schemas.append(schema)
1454-
# generate model if not already exists
1455-
cls_name: str = schema["title"]
1456-
# If a schema_to_use is provided, we do not need to check if
1457-
# the model exists
1458-
if not param.model_to_use:
1459-
# Prefer a class already registered for this category
1460-
# IRI (e.g. a packaged model class) over compiling a
1461-
# new one. Compiling one anyway would take over the
1462-
# oold type registry entry for this category and hide
1463-
# the packaged class' typed fields/helpers.
1464-
registered_cls = oold_type_registry.get(category)
1465-
# Controllers and result wrappers (e.g.
1466-
# WikiFileController, UploadFileResult) inherit
1467-
# their category IRI from the model class they
1468-
# extend, and the registry keeps whichever class
1469-
# was defined last. Such a specialization asks for
1470-
# fields a plain page's jsondata does not carry, so
1471-
# prefer the canonical class from osw.model.entity
1472-
# over a subclass of it.
1473-
canonical_cls = getattr(model, cls_name, None)
1474-
if (
1475-
registered_cls is not None
1476-
# the schema title can also name a module
1477-
# attribute that is not a class
1478-
and isinstance(canonical_cls, type)
1479-
and registered_cls is not canonical_cls
1480-
and issubclass(registered_cls, canonical_cls)
1481-
):
1482-
_logger.debug(
1483-
f"Ignoring '{registered_cls}' registered for "
1484-
f"category '{category}': it specializes "
1485-
f"'{canonical_cls}', which is used instead."
1486-
)
1487-
registered_cls = None
1488-
if registered_cls is not None:
1489-
category_to_cls[category] = registered_cls
1490-
else:
1491-
if not hasattr(model, cls_name):
1492-
if param.autofetch_schema:
1493-
self.fetch_schema(
1494-
OSW.FetchSchemaParam(
1495-
schema_title=category,
1496-
mode="append",
1497-
offline_pages=param.offline_pages,
1498-
)
1499-
)
1500-
if not hasattr(model, cls_name):
1501-
schemas_fetched = False
1502-
_logger.error(
1503-
f"Model {cls_name} not found. Schema "
1504-
f"{category} needs to be fetched first."
1456+
schemas.append(schema)
1457+
# generate model if not already exists
1458+
cls_name: str = schema["title"]
1459+
# If a schema_to_use is provided, we do not need to check if
1460+
# the model exists
1461+
if not param.model_to_use:
1462+
# Prefer a class already registered for this category
1463+
# IRI (e.g. a packaged model class) over compiling a
1464+
# new one. Compiling one anyway would take over the
1465+
# oold type registry entry for this category and hide
1466+
# the packaged class' typed fields/helpers.
1467+
registered_cls = oold_type_registry.get(category)
1468+
# Controllers and result wrappers (e.g.
1469+
# WikiFileController, UploadFileResult) inherit
1470+
# their category IRI from the model class they
1471+
# extend, and the registry keeps whichever class
1472+
# was defined last. Such a specialization asks for
1473+
# fields a plain page's jsondata does not carry, so
1474+
# prefer the canonical class from osw.model.entity
1475+
# over a subclass of it.
1476+
canonical_cls = getattr(model, cls_name, None)
1477+
if (
1478+
registered_cls is not None
1479+
# the schema title can also name a module
1480+
# attribute that is not a class
1481+
and isinstance(canonical_cls, type)
1482+
and registered_cls is not canonical_cls
1483+
and issubclass(registered_cls, canonical_cls)
1484+
):
1485+
_logger.debug(
1486+
f"Ignoring '{registered_cls}' registered for "
1487+
f"category '{category}': it specializes "
1488+
f"'{canonical_cls}', which is used instead."
15051489
)
1490+
registered_cls = None
1491+
if registered_cls is not None:
1492+
category_to_cls[category] = registered_cls
15061493
else:
1507-
generated_cls = getattr(model, cls_name)
1508-
# The class we are about to use may have just
1509-
# claimed (or may already hold) the registry
1510-
# slot for this category. If a different class
1511-
# is registered for it, someone's registration
1512-
# was silently overwritten - do not raise, but
1513-
# make sure this does not pass silently.
1514-
conflicting_cls = oold_type_registry.get(category)
1515-
if (
1516-
conflicting_cls is not None
1517-
and conflicting_cls is not generated_cls
1518-
# the slot holder specializes
1519-
# generated_cls, e.g. a controller
1520-
# that inherits the category IRI:
1521-
# the case the guard above expects,
1522-
# not a conflict. A fetch can leave
1523-
# a non-class under the schema title.
1524-
and not (
1525-
isinstance(generated_cls, type)
1526-
and issubclass(
1527-
conflicting_cls, generated_cls
1494+
if not hasattr(model, cls_name):
1495+
if param.autofetch_schema:
1496+
self.fetch_schema(
1497+
OSW.FetchSchemaParam(
1498+
schema_title=category,
1499+
mode="append",
1500+
offline_pages=param.offline_pages,
1501+
)
15281502
)
1503+
if not hasattr(model, cls_name):
1504+
schemas_fetched = False
1505+
_logger.error(
1506+
f"Model {cls_name} not found. Schema "
1507+
f"{category} needs to be fetched first."
15291508
)
1530-
):
1531-
_logger.warning(
1532-
f"Class '{generated_cls}' generated for "
1533-
f"category '{category}' claims the oold "
1534-
f"type registry slot already held by a "
1535-
f"different class '{conflicting_cls}'."
1509+
else:
1510+
generated_cls = getattr(model, cls_name)
1511+
# The class we are about to use may have just
1512+
# claimed (or may already hold) the registry
1513+
# slot for this category. If a different class
1514+
# is registered for it, someone's registration
1515+
# was silently overwritten - do not raise, but
1516+
# make sure this does not pass silently.
1517+
conflicting_cls = oold_type_registry.get(
1518+
category
15361519
)
1537-
category_to_cls[category] = generated_cls
1520+
if (
1521+
conflicting_cls is not None
1522+
and conflicting_cls is not generated_cls
1523+
# the slot holder specializes
1524+
# generated_cls, e.g. a controller
1525+
# that inherits the category IRI:
1526+
# the case the guard above expects,
1527+
# not a conflict. A fetch can leave
1528+
# a non-class under the schema title.
1529+
and not (
1530+
isinstance(generated_cls, type)
1531+
and issubclass(
1532+
conflicting_cls, generated_cls
1533+
)
1534+
)
1535+
):
1536+
_logger.warning(
1537+
f"Class '{generated_cls}' generated for "
1538+
f"category '{category}' claims the oold "
1539+
f"type registry slot already held by a "
1540+
f"different class '{conflicting_cls}'."
1541+
)
1542+
category_to_cls[category] = generated_cls
1543+
except Exception as e:
1544+
schemas_fetched = False
1545+
_logger.error(
1546+
f"Error reading the schema of category "
1547+
f"{category} for page {page.title}: {e}"
1548+
)
1549+
continue
15381550
if not schemas_fetched:
15391551
continue
15401552
# 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)