Skip to content

fix(core): never write unparseable entity.py from fetch_schema - #167

Open
LukasGold wants to merge 2 commits into
mainfrom
fix/fetch-schema-validate-before-write
Open

fix(core): never write unparseable entity.py from fetch_schema#167
LukasGold wants to merge 2 commits into
mainfrom
fix/fetch-schema-validate-before-write

Conversation

@LukasGold

@LukasGold LukasGold commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Fixes #125

OSW._fetch_schema could write a syntactically invalid src/osw/model/entity.py and then importlib.reload() it, raising SyntaxError and breaking every later import osw.core in that process. Because the file is written unconditionally, the corruption persisted on disk even when the call was not final and no reload surfaced it.

Root cause

datamodel-code-generator marks "no default" with a bare UNDEFINED = object() sentinel guarded by is checks. oold.utils.json_tools.merge_deep deep-copies schema dicts during allOf composition, which clones the sentinel into a look-alike object and defeats the identity guard, so the generator reprs it into source:

risk_assessment: RiskAssessmentProcess | None = Field(
    default_factory=lambda :RiskAssessmentProcess.parse_obj(<object object at 0x...>),
    options={...},
)

oold already works around this in Generator.generate(), but osw never benefits from it: _fetch_schema calls datamodel_code_generator.generate() directly instead of oold's wrapper, and reimplements its own post-processing without the sentinel cleanup.

Changes

Repair the known bad output

  • remove_unserializable_default_sentinels() strips the repr'd sentinel, called alongside the existing post-processing regexes so it covers every path reaching the write.
  • The pattern is widened relative to oold's. oold matches everything up to the first ) after the address, which is the paren belonging to parse_obj(, so on the shape reported in this issue it produces Field(default=None), options={...}) and still does not parse. Matching the factory expression itself repairs both the bare and the call-wrapped shape.

Never leave a broken entity.py behind

  • ensure_valid_python_source() runs ast.parse before the file is opened for writing. On failure it logs and raises with the path, line number and offending source line, so the previous valid entity.py is left in place.
  • ast.parse only proves syntax, so reload_module_or_restore() covers the second half: if the generated model imports badly (undefined name, error raised while a class body executes), the previous file content is written back, the module is reloaded again so it also works in memory, and the original error is re-raised.

Stop hiding the warning signs

  • The black/isort failure is logged at warning level instead of except Exception: pass. Behaviour is unchanged otherwise: formatting stays optional.

Notes

  • The guards are deliberately belt-and-braces: any future sentinel shape the regex does not repair now fails loudly at generation time rather than corrupting entity.py.
  • Remaining gap, left alone on purpose: a non-final call still writes without an import check, because intermediate content is not expected to be importable yet, which is why the existing code defers the reload to final. Such a write is still syntax-checked.
  • Out of scope: _fetch_schema writing into the installed package tree at all, which is fetch_schema rewrites src/osw/model/entity.py as a side effect of ordinary read/write calls #141.

Tests

tests/test_fetch_schema_write_safety.py, 9 tests, fully offline, never invoking the real fetch_schema:

  • the bare sentinel and the parse_obj-wrapped sentinel from the issue both become valid Python, with trailing Field kwargs preserved
  • a legitimate default_factory=lambda: uuid4() is untouched
  • the validator accepts valid source and raises with path, line number and source line on invalid source
  • a validation failure leaves an existing target file untouched
  • content that parses but raises on import is rolled back, and the module is usable again afterwards
  • a successful reload keeps the new content
  • a failed first-ever write, with nothing to roll back to, still raises

200 passed, 1 skipped offline; ruff check clean.

- strip datamodel-code-generator's repr'd `object()` default sentinel
- widen oold's regex so a sentinel wrapped in parse_obj() also repairs
- validate generated content with ast.parse before overwriting the file
- keep the previous valid entity.py in place when generation is broken
- log black/isort failures instead of swallowing them silently
@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-03)

### Bug Fixes

- **core**: Never write unparseable entity.py from fetch_schema
  ([`2f68c91`](https://github.com/OpenSemanticLab/osw-python/commit/2f68c91f4663d0f05a0ebee3390a9287140547a3))

- **core**: Roll back entity.py when the generated model cannot import
  ([`a409dac`](https://github.com/OpenSemanticLab/osw-python/commit/a409dacba600190e9f49c327cfcfffd892280233))

Preview via python-semantic-release and conventional commits.

- ast.parse only proves syntax, not that the module imports
- restore the previous file content if importlib.reload raises
- reload again after the rollback so the module works in memory
- re-raise the original import error instead of masking it
@LukasGold LukasGold self-assigned this Sep 3, 2026
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.

fetch_schema generates syntactically invalid osw/model/entity.py (RiskAssessmentProcess default_factory -> SyntaxError)

1 participant