Fill in empty properties when an object is recognized through the save cache - #135
Open
apdavison wants to merge 3 commits into
Open
Fill in empty properties when an object is recognized through the save cache#135apdavison wants to merge 3 commits into
apdavison wants to merge 3 commits into
Conversation
…e cache exists() has three ways of finding an object that already exists in the KG, but only two of them filled in the properties that were left empty locally. The save cache branch took the cached object's remote_data without doing so, which made every property that is set in the KG but absent from the object look like a deliberate deletion to modified_data(), so the next call to save() set it to null. Metadata-harvesting scripts, which typically build a fresh object for each role a person holds, were losing people's contact information, affiliations and ORCIDs the second time a person was saved in a single run. Assigning remote_data directly also left the two objects sharing a single dict, as the comment there suspected; the back-fill gives each its own. The mock client used in the tests can now serve seeded instances, answer existence queries against them, and record what gets written, so a save can be followed end to end.
Review follow-up to the previous commit; no change in behaviour. The "@type" fallback was unreachable: to_jsonld always emits @type with include_empty_properties either way, normalize_data preserves it, and _update_empty_properties copies it in from the KG document, so every source of remote_data already carries one. Worse, it would have papered over a genuine class mismatch that _deserialize_data is right to reject, so it is gone rather than kept as belt-and-braces. The guard on _raw_remote_data was likewise a no-op: this branch is only reached when the object has no http id, and _raw_remote_data is only ever assigned when it does, so it is always None here. The comment on the remote_data assertion overstated what the code delivers. The two objects get separate top-level dicts, so neither can rewrite the other's record, but nested values are shared by reference; that is safe only because remote_data is written a key at a time or replaced wholesale, never mutated in place, so the comment now says so. RecordingMockClient duplicated what MockKGClient already does now that it serves seeded instances and records writes, so the test uses the shared fixture instead and there is one less mock to keep in step. MockKGClient._match_instances now returns None for any query shape it cannot honour faithfully - a filter on a nested node, an operator other than EQUALS or CONTAINS, or a filter on @id - so those fall through to NotImplementedError as the rest of the mock expects. Previously it ignored the operator and any nested filters, which would have returned a plausible but wrong match set and let a test pass for the wrong reason. CONTAINS is now matched as a substring rather than equality, and the ordering trap is documented: the hard-coded name branches are still consulted first, so a test seeding an instance whose name they special-case will get the canned response.
Used black. No change in behaviour.
apdavison
force-pushed
the
fix-exists-cache-empty-properties
branch
from
September 1, 2026 13:16
b8302cf to
d948846
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #134.
KGObject.exists()has three ways of finding an object that already exists in the KG, but only two of them fill in the properties that were left empty locally. The branch that recognizes an object from the save cache took the cached object'sremote_datawithout doing so.That breaks the invariant
save()depends on:_update_empty_properties()distinguishes "never set" from "deliberately set toNone" by diffing againstremote_data, so populatingremote_datawhile leaving a property empty makes that property look like a deliberate deletion.modified_data()then reports it as changed, andsave()sends it to the KG asnull.The trigger is simply saving the same object twice in one session from separately-constructed objects. Metadata-harvesting scripts do this routinely — they build a fresh
Personfor each role someone holds and each project they appear in — and were silently erasing people'scontactInformation,affiliation,alternateNameanddigitalIdentifieron the second save.The fix calls
_update_empty_properties()in that branch too, so all three paths throughexists()behave the same way. Assigningremote_datadirectly also left the two objects sharing a single dict, as the# copy or update needed?comment there suspected; the back-fill gives each its own equal but independent copy.Tests in
test/test_base.pycover the back-fill, the fact that values set locally still win and are still written, and a save that must send nothing;test/test_openminds_core.pyhas a regression test for the person-saved-twice case. All run offline against the mock client, which can now serve seeded instances, answer existence queries against them, and record what gets written.