Skip to content

fix(core): prefer a registered class over a generated one - #166

Open
LukasGold wants to merge 2 commits into
mainfrom
fix/load-entity-registered-class
Open

fix(core): prefer a registered class over a generated one#166
LukasGold wants to merge 2 commits into
mainfrom
fix/load-entity-registered-class

Conversation

@LukasGold

Copy link
Copy Markdown
Contributor

Closes #138

Problem

load_entity decided whether to compile a class by asking hasattr(model, cls_name), where cls_name is schema["title"]. That is keyed by class name and only ever looks in osw.model.entity, so a packaged class already registered for the category IRI (e.g. opensemantic.base.v1.Database) was invisible. A new class got compiled, took over the oold type registry slot for that category, and the packaged typed fields and helpers were lost. For oold range fields the reference then resolves to None with no error.

Confirmed while writing the tests: importing osw.core alone already registers Database under Category:OSW51ad0d17... in oold.model.v1._types, even though osw.model.entity never exposes Database by name. That mismatch is the bug.

Changes

  • load_entity now looks the category IRI up in the oold type registry first and reuses a registered class instead of compiling a replacement.
  • Construction resolves through a category_to_cls map for both the single-schema and the multiple-inheritance base list, instead of getattr(model, schema["title"]).
  • When a generated class would claim a registry slot already held by a different class, this is now logged via the existing _logger rather than passing silently. It warns rather than raising, so existing setups keep working.
  • param.model_to_use still takes precedence and its branch is unchanged.

Registry API

The issue proposed oold.model._types and oold.model.v1._types. Against the installed oold 0.16.2 only the v1 registry is relevant: osw model classes descend from pydantic.v1.BaseModel / oold.model.v1.LinkedBaseModel, and entries are written by LinkedBaseModelMetaClass.__new__ when a subclass is defined. oold.static.resolve_type is not a usable accessor here, since it still requires the caller to pass the private dict and adds controller-preference logic osw never uses.

Out of scope, deliberately

The issue notes that the jsonschema slot no longer needs fetching when a class is already registered, and asks to keep that separate since schemas still feeds the multiple-inheritance base list. This PR keeps the fetch unconditional and does not take that optimization.

Tests

tests/test_load_entity_registered_class.py, fully offline:

  • a registered packaged class is preferred: type(entity) is Database, and no new Database is compiled into osw.model.entity
  • fallback unchanged: a category with nothing registered still gets the generated class
  • the conflict case logs the warning and still constructs the entity

The first and third fail on unpatched code; the second passes there, since it guards behaviour that is genuinely unchanged. Full unit suite: 194 passed, 1 skipped.

- 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
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Release preview

Merging this PR would release v2.3.2 (current: v2.3.1).

Changelog preview (truncated)
## v2.3.2 (2026-09-04)

### Bug Fixes

- **core**: Ignore registered subclasses of the canonical model class
  ([`07e7e99`](https://github.com/OpenSemanticLab/osw-python/commit/07e7e998bdb6265b6ed3734ad0c8869a892355dc))

- **core**: Prefer a registered class over a generated one
  ([`f4c7e01`](https://github.com/OpenSemanticLab/osw-python/commit/f4c7e01bee60c708de865a39ce0bd540cc5adb11))

Preview via python-semantic-release and conventional commits.

- controllers and result wrappers inherit the category IRI they extend
- oold's registry keeps whichever of them was defined last
- UploadFileResult thus replaced WikiFile and broke file up/download
- prefer osw.model.entity's class when the registered one specializes it
@LukasGold

Copy link
Copy Markdown
Contributor Author

Integration tests caught a regression in this PR: test_file_upload_download, test_live_upload_download_via_instance and test_live_upload_with_target_fpt all failed with AttributeError: 'NoneType' object has no attribute 'cast', behind this logged error:

ERROR osw.core:core.py:1330 Error creating entity from page File:TestTargetFptUpload….txt:
UploadFileResult.__init__() missing 1 required positional argument: 'source'

Cause. Preferring the registered class unconditionally is not safe, because osw's own classes reuse a category IRI. WikiFileController(model.WikiFile, RemoteFileController) and UploadFileResult(FileResult, WikiFileController) both inherit WikiFile's IRI, and oold registers by registry[iri] = cls (oold/model/v1/__init__.py:145), so the last class defined wins. oold only diverts controllers to a separate registry when oold.model.BaseController is in the MRO, which osw's controllers do not use, so they land in _types and displace the model class. Verified offline:

category IRI : Category:OSW11a53cdfbdc24524bf8ac435cbf65d9d
registered   : <class 'osw.express.UploadFileResult'>
canonical    : <class 'opensemantic.core.v1._model.WikiFile'>
is subclass  : True

load_entity then tried to build an UploadFileResult from a plain page's jsondata, which has no source.

Fix (07e7e99): ignore a registered class that is a strict subclass of the class osw.model.entity exposes for the category, and use the canonical one. Inverting to model-first would have regressed this issue's actual point, since #138 is about preferring a packaged class over a generated one; a packaged class is not a subclass of the generated one, so it is still preferred. In the case above the canonical class is itself the packaged opensemantic.core one.

New offline regression test asserts a registered specialization requiring an extra field is skipped. Without the fix it fails with the same shape as CI: Error creating entity from page …: 1 validation error for SubclassTestController.

Local: 195 passed, 1 skipped. Integration needs a re-run here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

load_entity regenerates a class for a category that a packaged class already registers, and overwrites it in the type lookup

1 participant