Skip to content

Commit 90fa310

Browse files
committed
fix(core): prefer a registered class over a generated one
- load_entity looked up classes by name in osw.model.entity only - a packaged class registered for the category IRI was invisible - the generated class then took over the oold type registry slot - warn instead of silently replacing a foreign registration - closes #138
1 parent 23a1db1 commit 90fa310

2 files changed

Lines changed: 282 additions & 16 deletions

File tree

‎src/osw/core.py‎

Lines changed: 52 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
set_resolver,
3232
)
3333
from oold.generator import Generator
34+
from oold.model.v1 import _types as oold_type_registry
3435
from oold.utils.codegen import OOLDJsonSchemaParser
3536
from opensemantic.v1 import OswBaseModel
3637
from pydantic import PydanticDeprecatedSince20
@@ -1244,6 +1245,10 @@ def load_entity(
12441245
entity = None
12451246
schemas = []
12461247
schemas_fetched = True
1248+
# maps a category (page title, e.g. "Category:OSW...") to the
1249+
# class to use for it, either an already registered class (e.g. a
1250+
# packaged model class) or a freshly compiled one
1251+
category_to_cls: Dict[str, Type[model.Entity]] = {}
12471252
jsondata = page.get_slot_content("jsondata")
12481253
if param.remove_empty:
12491254
remove_empty(jsondata)
@@ -1265,21 +1270,50 @@ def load_entity(
12651270
# If a schema_to_use is provided, we do not need to check if
12661271
# the model exists
12671272
if not param.model_to_use:
1268-
if not hasattr(model, cls_name):
1269-
if param.autofetch_schema:
1270-
self.fetch_schema(
1271-
OSW.FetchSchemaParam(
1272-
schema_title=category,
1273-
mode="append",
1274-
offline_pages=param.offline_pages,
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+
if registered_cls is not None:
1280+
category_to_cls[category] = registered_cls
1281+
else:
1282+
if not hasattr(model, cls_name):
1283+
if param.autofetch_schema:
1284+
self.fetch_schema(
1285+
OSW.FetchSchemaParam(
1286+
schema_title=category,
1287+
mode="append",
1288+
offline_pages=param.offline_pages,
1289+
)
12751290
)
1291+
if not hasattr(model, cls_name):
1292+
schemas_fetched = False
1293+
_logger.error(
1294+
f"Model {cls_name} not found. Schema "
1295+
f"{category} needs to be fetched first."
12761296
)
1277-
if not hasattr(model, cls_name):
1278-
schemas_fetched = False
1279-
_logger.error(
1280-
f"Model {cls_name} not found. Schema {category} "
1281-
f"needs to be fetched first."
1282-
)
1297+
else:
1298+
generated_cls = getattr(model, cls_name)
1299+
# The class we are about to use may have just
1300+
# claimed (or may already hold) the registry
1301+
# slot for this category. If a different class
1302+
# is registered for it, someone's registration
1303+
# was silently overwritten - do not raise, but
1304+
# make sure this does not pass silently.
1305+
conflicting_cls = oold_type_registry.get(category)
1306+
if (
1307+
conflicting_cls is not None
1308+
and conflicting_cls is not generated_cls
1309+
):
1310+
_logger.warning(
1311+
f"Class '{generated_cls}' generated for "
1312+
f"category '{category}' claims the oold "
1313+
f"type registry slot already held by a "
1314+
f"different class '{conflicting_cls}'."
1315+
)
1316+
category_to_cls[category] = generated_cls
12831317
if not schemas_fetched:
12841318
continue
12851319

@@ -1291,13 +1325,15 @@ def load_entity(
12911325
_logger.error("no schema defined")
12921326

12931327
elif len(schemas) == 1:
1294-
cls: Type[model.Entity] = getattr(model, schemas[0]["title"])
1328+
# category_to_cls is fully populated for every category
1329+
# that reached this point (see the loop above)
1330+
cls: Type[model.Entity] = category_to_cls[jsondata["type"][0]]
12951331
entity: model.Entity = cls(**jsondata)
12961332

12971333
else:
12981334
bases = []
1299-
for schema in schemas:
1300-
bases.append(getattr(model, schema["title"]))
1335+
for category in jsondata["type"]:
1336+
bases.append(category_to_cls[category])
13011337
cls = create_model("Test", __base__=tuple(bases))
13021338
entity: model.Entity = cls(**jsondata)
13031339
except Exception as e:
Lines changed: 230 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,230 @@
1+
"""Unit tests for OSW.load_entity() preferring an already registered class.
2+
3+
Regression guard for #138: load_entity() used to decide whether to compile a
4+
class for a category by checking ``hasattr(model, cls_name)``, keyed by class
5+
name and only looking inside ``osw.model.entity``. This missed classes from
6+
packaged modules (e.g. ``opensemantic.base.v1._model.Database``) that are
7+
already registered for that category IRI in oold's type registry
8+
(``oold.model.v1._types``), causing load_entity() to silently compile and use
9+
a different, incomplete class instead.
10+
11+
These tests run fully offline: WtSite.get_page() is fed pages through
12+
``offline_pages`` (see ``osw.wtsite.WtSite.GetPageParam``), so no network or
13+
wiki credentials are required.
14+
"""
15+
16+
import json
17+
import threading
18+
import uuid as uuid_module
19+
from typing import Any, Dict, Union
20+
21+
from oold.model.v1 import _types as oold_type_registry
22+
from opensemantic.base.v1 import Database
23+
24+
import osw.model.entity as model
25+
from osw.core import OSW
26+
from osw.utils.wiki import remove_empty
27+
from osw.wtsite import WtPage, WtSite
28+
29+
30+
class OfflineWtPage(WtPage):
31+
"""A WtPage that pretends to exist without touching a wiki."""
32+
33+
def __init__(self, wtSite: Any = None, title: str = None):
34+
self.wtSite = wtSite
35+
self.title = title
36+
self.exists = True
37+
self._original_content = ""
38+
self.changed: bool = False
39+
self._dict = []
40+
self._slots: Dict[str, Union[str, dict]] = {"main": ""}
41+
self._slots_changed: Dict[str, bool] = {"main": False}
42+
self._content_model: Dict[str, str] = {"main": "wikitext"}
43+
44+
45+
class _FakeConnection:
46+
"""Just enough of a requests session for WtSite._clear_cookies()."""
47+
48+
cookies = []
49+
50+
51+
class _FakeMwSite:
52+
connection = _FakeConnection()
53+
54+
55+
def make_offline_wtsite() -> WtSite:
56+
"""A WtSite that never touches the network (bypasses __init__)."""
57+
ws = WtSite.__new__(WtSite)
58+
ws._page_cache = {}
59+
ws._cache_enabled = False
60+
ws._session_lock = threading.RLock()
61+
ws._site = _FakeMwSite()
62+
return ws
63+
64+
65+
def make_page_for_entity(entity) -> OfflineWtPage:
66+
"""Build an offline page whose jsondata slot holds the serialized entity."""
67+
page = OfflineWtPage(title=f"Item:{OSW.get_osw_id(entity.uuid)}")
68+
jsondata = json.loads(entity.json(exclude_none=True))
69+
remove_empty(jsondata)
70+
page.set_slot_content("jsondata", jsondata)
71+
return page
72+
73+
74+
def make_schema_page(category: str, cls_name: str) -> OfflineWtPage:
75+
"""Build an offline page holding the jsonschema slot for a category."""
76+
page = OfflineWtPage(title=category)
77+
page.set_slot_content("jsonschema", {"title": cls_name})
78+
return page
79+
80+
81+
def make_isolated_cls(name: str, base=model.Item):
82+
"""Build a model.Item subclass registered under its own private category IRI.
83+
84+
Overriding schema_extra's title/uuid makes get_cls_iri() derive a fresh
85+
"Category:OSW<uuid>" IRI for this class alone, so defining it cannot
86+
clobber the registration of any real category (e.g. "Category:Item").
87+
"""
88+
namespace = {
89+
"Config": type(
90+
"Config",
91+
(base.Config,),
92+
{
93+
"schema_extra": {
94+
**base.Config.schema_extra,
95+
"title": name,
96+
"uuid": str(uuid_module.uuid4()),
97+
}
98+
},
99+
),
100+
"__qualname__": name,
101+
}
102+
return type(base)(name, (base,), namespace)
103+
104+
105+
def test_load_entity_prefers_registered_class_over_generated_one():
106+
"""A class already registered for the category IRI is used as-is, and no
107+
replacement class is compiled into osw.model.entity for it."""
108+
assert not hasattr(model, "Database")
109+
110+
db = Database(name="TestDb", label=[model.Label(text="Test Db")])
111+
category = db.type[0]
112+
entity_page = make_page_for_entity(db)
113+
schema_page = make_schema_page(category, "Database")
114+
115+
osw_obj = OSW(site=make_offline_wtsite())
116+
117+
result = osw_obj.load_entity(
118+
OSW.LoadEntityParam(
119+
titles=[entity_page.title],
120+
autofetch_schema=True,
121+
offline_pages={
122+
entity_page.title: entity_page,
123+
category: schema_page,
124+
},
125+
)
126+
)
127+
128+
entity = result.entities[0]
129+
assert type(entity) is Database
130+
# the packaged class was used directly, nothing was compiled
131+
assert not hasattr(model, "Database")
132+
133+
134+
def test_load_entity_falls_back_to_generated_class_when_nothing_registered():
135+
"""A category with nothing registered in oold's type registry still gets
136+
the class already present in osw.model.entity, exactly like before."""
137+
category = "Category:OSWFakeCategoryNotRegistered00000000000000"
138+
cls_name = "FakeGeneratedClass"
139+
assert oold_type_registry.get(category) is None
140+
141+
fake_cls = make_isolated_cls(cls_name)
142+
setattr(model, cls_name, fake_cls)
143+
try:
144+
entity_page = OfflineWtPage(title="Item:OSWFakeEntity0000000000000000000000000")
145+
jsondata = {
146+
"type": [category],
147+
"uuid": "00000000-0000-0000-0000-000000000000",
148+
"name": "x",
149+
"label": [{"text": "x"}],
150+
}
151+
remove_empty(jsondata)
152+
entity_page.set_slot_content("jsondata", jsondata)
153+
schema_page = make_schema_page(category, cls_name)
154+
155+
osw_obj = OSW(site=make_offline_wtsite())
156+
157+
result = osw_obj.load_entity(
158+
OSW.LoadEntityParam(
159+
titles=[entity_page.title],
160+
autofetch_schema=True,
161+
offline_pages={
162+
entity_page.title: entity_page,
163+
category: schema_page,
164+
},
165+
)
166+
)
167+
168+
entity = result.entities[0]
169+
assert type(entity) is fake_cls
170+
finally:
171+
delattr(model, cls_name)
172+
173+
174+
def test_load_entity_warns_on_registry_conflict(monkeypatch, caplog):
175+
"""If the class about to be used for a category differs from whatever is
176+
now registered for that IRI, load_entity() logs a warning instead of
177+
silently letting the mismatch pass."""
178+
category = "Category:OSWConflictTest000000000000000000000000"
179+
cls_name = "ConflictGeneratedClass"
180+
assert oold_type_registry.get(category) is None
181+
182+
generated_cls = make_isolated_cls(cls_name)
183+
other_cls = make_isolated_cls("OtherRegisteredClass")
184+
185+
def fake_fetch_schema(self, fetchSchemaParam=None):
186+
# Simulate fetch_schema() compiling a class and importing it into
187+
# osw.model.entity, while a *different* class ends up holding the
188+
# oold registry slot for the same category.
189+
setattr(model, cls_name, generated_cls)
190+
oold_type_registry[category] = other_cls
191+
192+
monkeypatch.setattr(OSW, "fetch_schema", fake_fetch_schema)
193+
194+
try:
195+
entity_page = OfflineWtPage(
196+
title="Item:OSWConflictEntity00000000000000000000000000"
197+
)
198+
jsondata = {
199+
"type": [category],
200+
"uuid": "11111111-1111-1111-1111-111111111111",
201+
"name": "x",
202+
"label": [{"text": "x"}],
203+
}
204+
remove_empty(jsondata)
205+
entity_page.set_slot_content("jsondata", jsondata)
206+
schema_page = make_schema_page(category, cls_name)
207+
208+
osw_obj = OSW(site=make_offline_wtsite())
209+
210+
result = osw_obj.load_entity(
211+
OSW.LoadEntityParam(
212+
titles=[entity_page.title],
213+
autofetch_schema=True,
214+
offline_pages={
215+
entity_page.title: entity_page,
216+
category: schema_page,
217+
},
218+
)
219+
)
220+
221+
entity = result.entities[0]
222+
assert type(entity) is generated_cls
223+
assert any(
224+
"claims the oold type registry slot" in record.message
225+
for record in caplog.records
226+
)
227+
finally:
228+
if hasattr(model, cls_name):
229+
delattr(model, cls_name)
230+
oold_type_registry.pop(category, None)

0 commit comments

Comments
 (0)