Conversation
- 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
Release previewNo version bump from the current commits (stays at Changelog preview (truncated)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
- keep main's verify_write field beside the flattened _overwrite_per_class - keep both skipped_pages and edited_titles in store_entity
- a page kept by 'keep existing' must not enter the write verification - it stays in pages and skipped when a neighbour is reported missing - fails if edited_titles is filled outside the kept_existing guard
|
@SimonStier this is the open point from the PR description, restated with the What changesWith this PR a property can be cleared by assigning an empty value. Before, Lines 1621 to 1629 in e6acace "Assigned on purpose" means present in pydantic's Lines 150 to 187 in e6acace Why the ontology import is affected
osw-python/src/osw/ontology.py Lines 762 to 775 in e6acace Passing a key to the constructor puts it in osw-python/src/osw/ontology.py Lines 828 to 834 in e6acace So a node that carries What is not an opt-out
The overwrite setting does control it. The decision
Not changed in this PR. Option 1 needs no code change. If you prefer 2 or 3, the change belongs in Related: #190 |
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 turningremove_emptyoff 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:trueclears,only emptyclears only an empty remote,falseandkeep existingdo not.Assign
None, not'', for "not known" - an explicitly empty value is now written.StoreEntityResult.skipped. Pages that existed and were left untouched bykeep existingwere indistinguishable from pages that were written. They are now also listed inskipped. Additive: they remain inpages, so no existing caller breaks.StoreEntityPartialError.storedno longer counts skipped pages. It derived fromresult.pages, which lists kept pages too, so a page that was deliberately left untouched was reported as stored.storednow excludes them and the error carries its ownskippedlist.result.pagesis unchanged, for callers that only care about which entities are on the wiki afterwards.OverwriteClassParam.modelreassignment re-validatesper_property. Assigning amodelthat 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
typedefault. Declaring policies for a class and a subclass that inheritstypestill raises, but the message now names both classes and explains the cause. Generated model classes always redefinetype(verified: 11 of 11 subclass pairs), so this only reaches hand-written subclasses such as the file controllers.WikiFileController.putcasts tomodel.WikiFilebefore storing, so the collision is not reachable through the library itself.Dead code removed.
StoreEntityParam._overwrite_per_classwas{"by name": {...}, "by type": {...}}; theby namehalf 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
LocalFileControllerandInMemoryControllerwere filed under the wrong category. Both declare(FileController, model.LocalFile), andFileControllerextendsmodel.File, so pydantic resolvedtypetoFile's category instead ofLocalFile's. Both now declaretypeexplicitly, read frommodel.LocalFilerather than hardcoded.RemoteFileController,S3FileControllerandWikiFileControllerput the model class first and were already correct.register_workflowwould have clearedPrefectFlow.domain.utils/workflow.pysetdomain=parsed.hostname or ""and stored withoverwrite=True. Harmless before, destructive under the clearing change: withPREFECT_API_URLunset it would have written""over a stored domain. Nowor None, matching the neighbouringurl_path.Tests
20 added: 15 in
tests/test_overwrite_policy.py, 4 in the newtests/test_controller_type_defaults.py, 1 intests/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_strippededits__fields_set__directly, because the generated models default every field toNoneandNoneis dropped beforeremove_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:ImportOntologyParambuilds entities from JSON-LD graph nodes and stores them withoverwrite=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.Tracked issues
Closes #190 (an empty value
cannot clear a property; pages kept by
keep existingare reported as stored).Closes #191 (unread
by namelookup; silent drop onmodelreassignment; undocumented snapshotsemantics).
main(v2.5.1) is merged into this branch as of af682e1. e6acace adds the 15th test intests/test_overwrite_policy.py, which covers a combination that first became possible with that merge: a page kept bykeep existingin the same call as a page the write verification reports as missing. Two conflicts insrc/osw/core.py, both resolved by keeping both sides: main'sverify_writefield beside the flattened
_overwrite_per_class, and main'sedited_titlesbeside
skipped_pages. A page kept bykeep existingnever entersedited_titles, so the write verification added on main does not query it.