Skip to content

fix(core): address follow-up issues in the overwrite policy - #168

Open
LukasGold wants to merge 2 commits into
mainfrom
fix/overwrite-policy-followup
Open

fix(core): address follow-up issues in the overwrite policy#168
LukasGold wants to merge 2 commits into
mainfrom
fix/overwrite-policy-followup

Conversation

@LukasGold

@LukasGold LukasGold commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Follow-up to #163, which is now merged. Addresses the six issues listed there under "Found but not fixed", after a design pass with the maintainer, plus two defects found on the way.

Behaviour changes

A property can now be cleared by assigning an empty value.
remove_empty=True (the default) stripped ''/[]/{} from the local content before the merge, and since the merge starts from the remote content, the stored value always survived. Clearing was only possible by turning remove_empty off for the whole call.
_explicitly_set_empty() uses pydantic's __fields_set__ to keep an empty value the caller assigned on purpose, and still strips one that is merely a default. Whether it then clears depends on the overwrite setting as usual: true clears, only empty clears only an empty remote, false and keep existing do not.

Assign None, not '', for "not known" - an explicitly empty value is now written.

StoreEntityResult.skipped. Pages that existed and were left untouched by keep existing were indistinguishable from pages that were written. They are now also listed in skipped. Additive: they remain in pages, so no existing caller breaks.

StoreEntityPartialError.stored no longer counts skipped pages. It derived from result.pages, which lists kept pages too, so a page that was deliberately left untouched was reported as stored. stored now excludes them and the error carries its own skipped list. result.pages is unchanged, for callers that only care about which entities are on the wiki afterwards.

OverwriteClassParam.model reassignment re-validates per_property. Assigning a model that lacks a declared property silently dropped the setting. It now raises, and __setattr__ rolls the field back.

Non-behaviour changes

Subclass dispatch: kept, error message improved. Dispatch stays keyed on the OSW type default. Declaring policies for a class and a subclass that inherits type still raises, but the message now names both classes and explains the cause. Generated model classes always redefine type (verified: 11 of 11 subclass pairs), so this only reaches hand-written subclasses such as the file controllers. WikiFileController.put casts to model.WikiFile before storing, so the collision is not reachable through the library itself.

Dead code removed. StoreEntityParam._overwrite_per_class was {"by name": {...}, "by type": {...}}; the by name half was built and duplicate-checked but never read. It is now a flat dict keyed by OSW type.

Snapshot semantics documented. pydantic v1 shallow-copies each policy into overwrite_per_class, so mutating one afterwards does not affect the param. Confirmed as intended: the snapshot is the wanted behaviour, so this is a docstring, not a code change.

Defects found while doing the above

LocalFileController and InMemoryController were filed under the wrong category. Both declare (FileController, model.LocalFile), and FileController extends model.File, so pydantic resolved type to File's category instead of LocalFile's. Both now declare type explicitly, read from model.LocalFile rather than hardcoded. RemoteFileController, S3FileController and WikiFileController put the model class first and were already correct.

register_workflow would have cleared PrefectFlow.domain. utils/workflow.py set domain=parsed.hostname or "" and stored with overwrite=True. Harmless before, destructive under the clearing change: with PREFECT_API_URL unset it would have written "" over a stored domain. Now or None, matching the neighbouring url_path.

Tests

19 added: 14 in tests/test_overwrite_policy.py, 4 in the new tests/test_controller_type_defaults.py, 1 in tests/test_store_entity_failure.py.

Reverting the source changes fails 11 of them, one per behaviour change; the rest are characterisation coverage. test_an_empty_value_that_was_not_explicitly_set_is_still_stripped edits __fields_set__ directly, because the generated models default every field to None and None is dropped before remove_empty() is reached, so there is no other way to construct the case that separates the two code paths.

Open point for review

@SimonStier please take a look at the clearing change as it applies to src/osw/ontology.py: ImportOntologyParam builds entities from JSON-LD graph nodes and stores them with overwrite=True. Under the new behaviour, a node that carries an empty value will clear the value stored on the wiki, where previously it was stripped and the stored value survived. Whether that actually occurs depends on the source ontology, and it is arguably the correct outcome for an authoritative re-import, but you know those import paths best. Not changed in this PR.

- clear a property by assigning an empty value, honouring __fields_set__
- report pages kept by 'keep existing' in StoreEntityResult.skipped
- re-validate per_property when OverwriteClassParam.model is reassigned
- drop the unused 'by name' half of the class dispatch map
- give file controllers the OSW category of the model they wrap
@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**: Address follow-up issues in the overwrite policy
  ([`10808a0`](https://github.com/OpenSemanticLab/osw-python/commit/10808a05d1c27c5cf4053e79169d39ceeac16f99))

- **core**: Drop skipped pages from StoreEntityPartialError.stored
  ([`cf78de9`](https://github.com/OpenSemanticLab/osw-python/commit/cf78de9f253700a92b8b44761dd4baa75050695e))

Preview via python-semantic-release and conventional commits.

- 'stored' now lists only the pages that were actually written
- add 'skipped' to the error, mirroring StoreEntityResult
- 'result.pages' unchanged, still lists kept pages
@LukasGold
LukasGold requested a review from SimonStier September 3, 2026 15:23
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.

1 participant