fix(user-sync): prune by field type and decide from stored jsondata

- set list fields to [] and delete single-valued keys on --prune
- read stored jsondata via offline_pages, without an extra request
- decide protected-field removal from the stored jsondata
- a second --prune run no longer proposes the same removal

Author: LukasGold

docs/tools/user-sync.md

@@ -30,10 +30,12 @@ prompt.
 
 To also clean up existing items, add `--prune`. Without it, the sync never
 writes `employment_contract_status` and never removes anything. With it,
-`employment_contract_status` on existing items is overwritten with an empty
-value. So are `website` and `organization`, unless `--with-websites` or
-`--with-organizations` enables them. This happens in a second edit of the
-page's jsondata after the store:
+`employment_contract_status` is removed from the stored jsondata of existing
+items. `website` and `organization` are overwritten with an empty list, unless
+`--with-websites` or `--with-organizations` enables them. This happens in a
+second edit of the page's jsondata after the store. The sync decides from the
+stored jsondata, not from the loaded item, which shows the model default for
+`employment_contract_status`. A second `--prune` run therefore changes nothing:
 
 ```bash
 uv run python examples/user_sync.py --domain your-instance.example.org --prune --dry-run
@@ -62,7 +64,7 @@ only core identity plus email.
 | `--with-organizations` | Also create and link Organization items from ORCID affiliations (opt-in). |
 | `--with-extras` | Enable both websites and organizations. |
 | `--include-blocked` | Do not skip blocked MediaWiki accounts. |
-| `--prune` | Overwrite `employment_contract_status`, and `website` or `organization` when their opt-in flag is not given, on existing items with an empty value, in a second edit of the page's jsondata after the store (default off; a normal run never writes `employment_contract_status` and never removes anything). |
+| `--prune` | Remove `employment_contract_status` from the stored jsondata of existing items, and overwrite `website` or `organization` with an empty list when their opt-in flag is not given, in a second edit of the page's jsondata after the store (default off; a normal run never writes `employment_contract_status` and never removes anything). |
 
 ## What it does
 
@@ -95,9 +97,9 @@ only core identity plus email.
 | --- | --- |
 | `username`, `first_name`, `surname`, `label`, `orcid` | Always (core identity). |
 | `email` | Standard: always attempted; missing email is warned, not fatal; never removed. |
-| `website` | Opt-in (`--with-websites`); without that flag, on existing items overwritten with an empty value only with `--prune`, in a second jsondata edit after the store. |
-| `organization` | Opt-in (`--with-organizations`); without that flag, on existing items overwritten with an empty value only with `--prune`, in a second jsondata edit after the store. |
-| `employment_contract_status` | Never written without `--prune`; with `--prune`, overwritten with an empty value on existing items in a second jsondata edit after the store. |
+| `website` | Opt-in (`--with-websites`); without that flag, on existing items overwritten with an empty list only with `--prune`, in a second jsondata edit after the store. |
+| `organization` | Opt-in (`--with-organizations`); without that flag, on existing items overwritten with an empty list only with `--prune`, in a second jsondata edit after the store. |
+| `employment_contract_status` | Never written without `--prune`; with `--prune`, removed from the stored jsondata of existing items in a second jsondata edit after the store. |
 
 ## Notes
 

src/osw/tools/user_sync/config.py

@@ -113,8 +113,8 @@ def build_arg_parser() -> argparse.ArgumentParser:
         "--prune",
         dest="prune",
         action="store_true",
-        help="Remove disabled optional fields from existing items and overwrite "
-        "employment_contract_status with an empty value (data cleanup). Off by "
+        help="Overwrite disabled optional fields of existing items with an empty "
+        "list and remove employment_contract_status (data cleanup). Off by "
         "default; a normal run only adds.",
     )
     account_scope = parser.add_mutually_exclusive_group()

src/osw/tools/user_sync/existing.py

@@ -6,10 +6,13 @@
 
 from __future__ import annotations
 
+import copy
 from dataclasses import dataclass, field
-from typing import Any, Dict, List, Optional
+from typing import Any, Dict, List, Optional, Tuple
 
+from osw.core import OSW
 from osw.wiki_tools import SearchParam
+from osw.wtsite import WtSite
 
 from .config import USER_CATEGORY
 
@@ -22,10 +25,16 @@ class ExistingUsers:
     """Existing User items indexed by their MediaWiki username."""
 
     by_username: Dict[str, Any] = field(default_factory=dict)
+    # The jsondata as stored on the page, per username. The loaded entity cannot
+    # tell a stored value from a model default.
+    stored_jsondata: Dict[str, dict] = field(default_factory=dict)
 
     def get(self, username: str) -> Optional[Any]:
         return self.by_username.get(username)
 
+    def get_stored(self, username: str) -> Optional[dict]:
+        return self.stored_jsondata.get(username)
+
 
 def _as_entity_list(result: Any) -> List[Any]:
     """Normalize load_entity output (single, list, or LoadEntityResult)."""
@@ -39,6 +48,29 @@ def _as_entity_list(result: Any) -> List[Any]:
     return [result]
 
 
+def _load_with_stored(osw: Any, titles: List[str]) -> Tuple[List[Any], Dict[str, dict]]:
+    """Load the entities and read the stored jsondata of the same pages.
+
+    The pages are fetched once and handed to load_entity as offline pages, so the
+    jsondata is read without a second request. A copy is taken first, because
+    load_entity removes empty keys from the slot content in place.
+
+    Returns:
+        The entities and the stored jsondata keyed by the uuid in it.
+    """
+    pages = osw.site.get_page(WtSite.GetPageParam(titles=titles)).pages
+    offline_pages = {page.title: page for page in pages}
+    stored: Dict[str, dict] = {}
+    for page in pages:
+        jsondata = page.get_slot_content("jsondata")
+        if isinstance(jsondata, dict):
+            stored[str(jsondata.get("uuid"))] = copy.deepcopy(jsondata)
+    result = osw.load_entity(
+        OSW.LoadEntityParam(titles=titles, offline_pages=offline_pages)
+    )
+    return _as_entity_list(result), stored
+
+
 def _query_user_titles(
     osw: Any, category: str, limit: Optional[int] = None
 ) -> List[str]:
@@ -88,13 +120,16 @@ def load_existing_users(
     titles = _query_user_titles(osw, category, limit)
     if not titles:
         return ExistingUsers()
-    entities = _as_entity_list(osw.load_entity(titles))
-    by_username: Dict[str, Any] = {}
+    entities, stored = _load_with_stored(osw, titles)
+    existing = ExistingUsers()
     for entity in entities:
         username = getattr(entity, "username", None)
         if username:
-            by_username[username] = entity
-    return ExistingUsers(by_username=by_username)
+            existing.by_username[username] = entity
+            jsondata = stored.get(str(getattr(entity, "uuid", None)))
+            if jsondata is not None:
+                existing.stored_jsondata[username] = jsondata
+    return existing
 
 
 def adopt_existing_pages(
@@ -118,12 +153,15 @@ def adopt_existing_pages(
     present = [p for p in candidates if p.full_page_title not in missing]
     if not present:
         return []
-    entities = _as_entity_list(osw.load_entity([p.full_page_title for p in present]))
+    entities, stored = _load_with_stored(osw, [p.full_page_title for p in present])
     by_uuid = {str(getattr(e, "uuid", None)): e for e in entities if e is not None}
     adopted: List[str] = []
     for proposed in present:
         entity = by_uuid.get(str(proposed.uuid))
         if entity is not None:
             existing.by_username[proposed.username] = entity
+            jsondata = stored.get(str(proposed.uuid))
+            if jsondata is not None:
+                existing.stored_jsondata[proposed.username] = jsondata
             adopted.append(proposed.full_page_title)
     return adopted

src/osw/tools/user_sync/prune.py

@@ -1,23 +1,35 @@
 """Clear stored relations of existing user items (``--prune``).
 
 The store merges with the stored page, so an absent key keeps the stored value.
-The keys are therefore overwritten with an empty list in a second edit of the
-page's jsondata slot.
+The keys are therefore changed in a second edit of the page's jsondata slot.
+A key of a list or set field is overwritten with an empty list. A key of a
+single-valued field is deleted, because an empty list is not a valid value for
+it. A key that is not a field of User is deleted as well.
 """
 
 from __future__ import annotations
 
 import json
 from typing import Any, Iterable
 
+from opensemantic.base.v1 import User
+from pydantic.v1.fields import SHAPE_SINGLETON
+
 from osw.wtsite import WtPage
 
 
+def _is_multi_valued(key: str) -> bool:
+    """True if the User field is a list or a set, False for any other key."""
+    model_field = User.__fields__.get(key)
+    return model_field is not None and model_field.shape != SHAPE_SINGLETON
+
+
 def clear_jsondata_keys(osw: Any, title: str, keys: Iterable[str]) -> bool:
-    """Set each present, non-empty key in the page's jsondata slot to ``[]``.
+    """Clear each present, non-empty key in the page's jsondata slot.
 
-    The page is read with a new WtPage, which downloads the current revision and
-    bypasses the WtSite page cache. Other keys are written back unchanged.
+    A list or set field becomes ``[]``. Any other key is deleted. The page is
+    read with a new WtPage, which downloads the current revision and bypasses the
+    WtSite page cache. Other keys are written back unchanged.
     Returns True if the page was written, False if nothing changed.
     """
     page = WtPage(wtSite=osw.site, title=title)
@@ -29,7 +41,10 @@ def clear_jsondata_keys(osw: Any, title: str, keys: Iterable[str]) -> bool:
     changed = False
     for key in keys:
         if key in content and content[key] not in (None, [], "", {}):
-            content[key] = []
+            if _is_multi_valued(key):
+                content[key] = []
+            else:
+                del content[key]
             changed = True
     if not changed:
         return False

src/osw/tools/user_sync/reconcile.py

@@ -91,6 +91,18 @@ def _existing_has_iri(entity: Any, name: str) -> bool:
     return isinstance(iris, dict) and bool(iris.get(name))
 
 
+def _has_protected_value(
+    entity: Any, name: str, stored_jsondata: Optional[Dict[str, Any]]
+) -> bool:
+    """True if the protected field holds a value that pruning would remove.
+
+    Without stored jsondata the loaded entity is the only source.
+    """
+    if stored_jsondata is None:
+        return _existing_has_iri(entity, name)
+    return not _is_empty(stored_jsondata.get(name))
+
+
 def existing_fields(entity: Any) -> Dict[str, Any]:
     """Normalized comparable field view of a loaded User entity."""
     return {
@@ -151,6 +163,7 @@ def reconcile_user(
     existing: Optional[Any],
     enabled_fields: FrozenSet[str] = frozenset(),
     prune: bool = False,
+    stored_jsondata: Optional[Dict[str, Any]] = None,
 ) -> UserChange:
     if existing is None:
         return UserChange(proposed=proposed, existing=None, category=NEW)
@@ -178,10 +191,11 @@ def reconcile_user(
     if not getattr(existing, "username", None):
         diffs.append(FieldDiff("username", None, proposed.username, GAP_FILL))
         has_change = True
-    # Protected fields are stripped from existing items only when pruning.
+    # Protected fields are stripped from existing items only when pruning. The
+    # stored jsondata decides, because a loaded entity also shows model defaults.
     if prune:
         for name in PROTECTED_FIELDS:
-            if _existing_has_iri(existing, name):
+            if _has_protected_value(existing, name, stored_jsondata):
                 diffs.append(FieldDiff(name, "set", None, REMOVE))
                 has_change = True
     category = CONFLICT if has_conflict else GAP_FILL if has_change else UNCHANGED
@@ -217,7 +231,13 @@ def reconcile(
     prune: bool = False,
 ) -> ReconcilePlan:
     changes = [
-        reconcile_user(p, existing.get(p.username), enabled_fields, prune)
+        reconcile_user(
+            p,
+            existing.get(p.username),
+            enabled_fields,
+            prune,
+            existing.get_stored(p.username),
+        )
         for p in proposed_users
     ]
     return ReconcilePlan(changes=changes)

src/osw/tools/user_sync/sync.py

@@ -191,7 +191,7 @@ def _store_users(
 
 
 def _prune_users(osw: Any, resolution: Resolution, report: SyncReport) -> None:
-    """Overwrite removed fields of existing items with an empty value."""
+    """Clear removed fields of stored existing items (see clear_jsondata_keys)."""
     updated = set(report.updated)
     for resolved in resolution.resolved:
         if resolved.action != "update":

tests/tools/test_user_sync_existing.py

@@ -10,6 +10,18 @@
     adopt_existing_pages,
     load_existing_users,
 )
+from osw.wtsite import WtPage
+
+
+class FakeWtPage(WtPage):
+    """A WtPage that serves a fixed jsondata slot without a wiki."""
+
+    def __init__(self, title, jsondata=None):
+        self.title = title
+        self._jsondata = jsondata or {}
+
+    def get_slot_content(self, slot, *args, **kwargs):
+        return self._jsondata
 
 
 def _ask_result(titles, continue_offset=None):
@@ -33,6 +45,11 @@ class FakeSite:
     def __init__(self, responses):
         self._responses = list(responses)
         self.queries = []
+        self.stored = {}
+
+    def get_page(self, param):
+        pages = [FakeWtPage(t, self.stored.get(t, {"uuid": t})) for t in param.titles]
+        return SimpleNamespace(pages=pages, errors=[])
 
     def api(self, action, **params):
         assert action == "ask"
@@ -54,8 +71,10 @@ def _get_missing_page_titles(self, titles, confirm_delay_s=5):
         assert confirm_delay_s == 0
         return [t for t in titles if t in self.missing]
 
-    def load_entity(self, titles):
+    def load_entity(self, param):
+        titles = param.titles
         self.loaded = list(titles)
+        assert set(param.offline_pages) == set(titles)
         return [self._entities[t] for t in titles]
 
 
@@ -157,3 +176,28 @@ def test_adopt_skips_users_already_matched_by_username():
     existing = ExistingUsers(by_username={"Alice": SimpleNamespace(username="Alice")})
     assert adopt_existing_pages(osw, existing, [_proposed("Alice", "1")]) == []
     assert osw.loaded is None
+
+
+def test_load_records_the_stored_jsondata_per_username():
+    entities = {
+        "Item:OSW1": SimpleNamespace(username="Alice", uuid="u1"),
+        "Item:OSW2": SimpleNamespace(username="Bob", uuid="u2"),
+    }
+    osw = FakeOsw(["Item:OSW1", "Item:OSW2"], entities)
+    osw.site.stored = {
+        "Item:OSW1": {"uuid": "u1", "username": "Alice", "website": []},
+        "Item:OSW2": {"uuid": "u2", "username": "Bob"},
+    }
+    existing = load_existing_users(osw)
+    assert existing.get_stored("Alice") == osw.site.stored["Item:OSW1"]
+    assert "employment_contract_status" not in existing.get_stored("Bob")
+    assert existing.get_stored("missing") is None
+
+
+def test_adopt_records_the_stored_jsondata():
+    found = SimpleNamespace(username=None, uuid="1")
+    osw = FakeOsw([], {"Item:OSW1": found}, pages=[_ask_result([])])
+    osw.site.stored = {"Item:OSW1": {"uuid": "1", "label": [{"text": "A"}]}}
+    existing = ExistingUsers()
+    adopt_existing_pages(osw, existing, [_proposed("Alice", "1")])
+    assert existing.get_stored("Alice") == osw.site.stored["Item:OSW1"]

tests/tools/test_user_sync_reconcile.py

@@ -157,6 +157,40 @@ def test_protected_field_removed_when_pruning():
     assert change.category == GAP_FILL
 
 
+def test_protected_default_not_stored_is_not_removed():
+    # The loaded entity shows the model default IRI, the stored page has none.
+    entity = _entity()
+    entity.__iris__ = {"employment_contract_status": "Item:OSWdefault"}
+    change = reconcile_user(
+        _proposed(), entity, prune=True, stored_jsondata={"username": ORCID}
+    )
+    assert [d for d in change.diffs if d.status == REMOVE] == []
+    assert change.category == UNCHANGED
+
+
+def test_protected_field_stored_is_removed_when_pruning():
+    entity = _entity()
+    entity.__iris__ = {"employment_contract_status": "Item:OSWx"}
+    change = reconcile_user(
+        _proposed(),
+        entity,
+        prune=True,
+        stored_jsondata={"employment_contract_status": "Item:OSWx"},
+    )
+    removes = [d.name for d in change.diffs if d.status == REMOVE]
+    assert removes == ["employment_contract_status"]
+
+
+def test_reconcile_uses_the_stored_jsondata_of_the_index():
+    entity = _entity()
+    entity.__iris__ = {"employment_contract_status": "Item:OSWdefault"}
+    existing = ExistingUsers(
+        by_username={ORCID: entity}, stored_jsondata={ORCID: {"username": ORCID}}
+    )
+    plan = reconcile([_proposed()], existing, prune=True)
+    assert plan.changes[0].category == UNCHANGED
+
+
 def test_missing_label_is_gap_fill():
     change = reconcile_user(_proposed(label="Lukas Koschmieder"), _entity(label=None))
     assert change.category == GAP_FILL