Skip to content

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

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

LukasGold wants to merge 4 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

20 added: 15 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.

Tracked issues

Closes #190 (an empty value
cannot clear a property; pages kept by keep existing are reported as stored).

Closes #191 (unread by name lookup; silent drop on model reassignment; undocumented snapshot
semantics).

main (v2.5.1) is merged into this branch as of af682e1. e6acace adds the 15th test in tests/test_overwrite_policy.py, which covers a combination that first became possible with that merge: a page kept by keep existing in the same call as a page the write verification reports as missing. Two conflicts in
src/osw/core.py, both resolved by keeping both sides: main's verify_write
field beside the flattened _overwrite_per_class, and main's edited_titles
beside skipped_pages. A page kept by keep existing never enters
edited_titles, so the write verification added on main does not query it.

- 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

No version bump from the current commits (stays at v2.5.1). Use conventional commit types (feat, fix, ...) to trigger a release.

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
@LukasGold

LukasGold commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

@SimonStier this is the open point from the PR description, restated with the
exact code path so it can be decided without reading the whole diff.

What changes

With this PR a property can be cleared by assigning an empty value. Before, remove_empty=True
(the default) stripped "", [] and {} from the local content before the merge, and since the
merge starts from the remote content, the stored value always survived. Now an empty value that
the caller assigned on purpose survives the strip and reaches the merge:

osw-python/src/osw/core.py

Lines 1621 to 1629 in e6acace

if param.remove_empty:
# an empty value the caller assigned on purpose is how a property gets
# cleared, so it has to survive remove_empty() and reach the merge
# below, where step c) can let it win over the remote value
explicitly_set_empty = _explicitly_set_empty(
param.entity, local_content["jsondata"]
)
remove_empty(local_content["jsondata"])
local_content["jsondata"].update(explicitly_set_empty)

"Assigned on purpose" means present in pydantic's __fields_set__:

osw-python/src/osw/core.py

Lines 150 to 187 in e6acace

def _explicitly_set_empty(entity: OswBaseModel, jsondata: dict) -> dict:
"""Returns the top-level jsondata entries the caller set to an empty value.
remove_empty() cannot tell an empty value that was assigned on purpose from
one that is merely a default, so it strips both. Assigning an empty value is
how a property gets cleared, though: the merge in _apply_overwrite_policy()
starts from the remote content, so a local key that was stripped leaves the
remote value in place and the property can never be emptied. pydantic records
the fields that were explicitly supplied in __fields_set__, which is enough to
keep those and drop the rest.
Parameters
----------
entity:
The entity the jsondata was serialized from
jsondata:
The serialized entity, before remove_empty() has been applied
Returns
-------
result:
The subset of jsondata that is empty but was explicitly set
"""
fields_set = getattr(entity, "__fields_set__", None)
if not fields_set:
return {}
fields = getattr(entity, "__fields__", {})
keys = set()
for name in fields_set:
# json() is called without by_alias, so the key is the field name. Accept
# the alias too, so that a field declared with one (model.PrefectFlow
# names 'schema_' after 'schema') is matched either way.
keys.add(name)
if name in fields:
keys.add(fields[name].alias)
return {
key: value for key, value in jsondata.items() if key in keys and is_empty(value)
}

Why the ontology import is affected

_create_entities() builds every entity as cls(**node) straight from a JSON-LD graph node:

if "owl:Class" in node["rdf_type"]:
e = self.import_config.base_class(**node)
if "owl:ObjectProperty" in node["rdf_type"]:
e = model.ObjectProperty(**node)
if "owl:DatatypeProperty" in node["rdf_type"]:
e = model.DataProperty(**node)
if "owl:AnnotationProperty" in node["rdf_type"]:
e = model.AnnotationProperty(**node)
if "owl:NamedIndividual" in node["rdf_type"]:
e = model.OwlIndividual(**node)

Passing a key to the constructor puts it in __fields_set__. Every key a node carries therefore
counts as explicitly set, including one whose value is empty. The entities are then stored with
overwrite=True:

self.osw.store_entity(
OSW.StoreEntityParam(
entities=param.entities,
overwrite=True,
change_id=self.import_config.change_id,
)
)

So a node that carries "description": "" or "label": [] will erase that property on the wiki
page. Before this PR the same node left the stored value untouched. Whether any node carries an
empty value depends on the ontology and on the JSON-LD conversion. _create_entities() does not
remove empty entries from node before constructing the entity.

What is not an opt-out

remove_empty does not switch this off. With True the explicitly set empty values are kept and
written. With False every empty value is kept and written, which affects more properties, not
fewer.

The overwrite setting does control it. true clears, only empty clears only when the stored
value is empty too, false and keep existing do not clear. The import passes overwrite=True.

The decision

  1. Accept it. For an authoritative re-import, writing the source state including its empty values
    is arguably correct.
  2. Strip empty entries from node before cls(**node) in _create_entities(). The values then
    count as unset, and the previous behaviour returns for the import path only.
  3. Lower the overwrite setting for the import, or protect single properties with per_property.

Not changed in this PR. Option 1 needs no code change. If you prefer 2 or 3, the change belongs in
src/osw/ontology.py and I can add it here.

Related: #190

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

Labels

enhancement New feature or request

Projects

None yet

1 participant