Look for existing instances in all spaces when saving - #142
Merged
apdavison merged 1 commit intoSep 15, 2026
Merged
Conversation
save() restricted its existence query to the space it was about to write to, so a locally-constructed object whose counterpart already lives in a different space was not recognised, and a duplicate was created. This happened most often with recursive=True, where new child objects inherit the parent's target space. The restriction was intended as an optimization, but benchmarking against the pre-production KG showed no measurable benefit: Person queries took the same time whether or not they were restricted to a space, and File queries timed out after ~50 s even when restricted to a space containing no files. save() now runs a single existence query across all spaces, which also returns the space of each matching instance. If one of the matches is in the target space, that instance is used, and matches in other spaces are not treated as duplicates. If the only match is in another space, it is used and updated in the space where it lives, with a warning, rather than a duplicate being created in the requested space. Multiple matches, none of them in the target space, raise an exception unless ignore_duplicates=True. This resolves the long-standing TODO about existing objects in a different space. To let save() choose between matches, the query part of exists() is split into private methods (_exists_without_query, _query_matching_instances, _check_for_duplicates, _use_matching_instance), shared by exists() and save(). The signature and behaviour of exists(), including in_spaces, are unchanged. When exists() binds to a match found by the query, the object's space is set to the space of that instance. Existence queries request EXISTENCE_QUERY_SIZE (2) results. If the response reports a larger total, all matches are retrieved in a second query, so that an instance in the target space is not missed. In exists(), a ConnectionError other than RemoteDisconnected was swallowed, leaving the result undefined and raising a NameError. It is now re-raised. MockKGClient now returns the space of matching instances under the name used by the query API. New tests cover children and top-level objects found in another space, preferring the target space, duplicates in the target space and in other spaces, retrieving all matches, and connection errors.
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
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 #136
Currently,
save()restricts its existence query to the space it is about to write to, so a locally-constructed object whose counterpart already lives in a different space is not recognised, and a duplicate is created. This happens most often withrecursive=True, where new child objects inherit the parent's target space.The restriction was intended as an optimization, but benchmarking against the pre-production KG showed no measurable benefit: Person queries took the same time whether or not they were restricted to a space, and File queries timed out after ~50 s even when restricted to a space containing no files.
With the changes in this PR,
save()now runs a single existence query across all spaces, which also returns the space of each matching instance.ignore_duplicates=True. This resolves the long-standing TODO about existing objects in a different space.To let
save()choose between matches, the query part ofexists()is split into private methods (_exists_without_query(),_query_matching_instances(),_check_for_duplicates(),_use_matching_instance()), shared byexists()andsave(). The signature and behaviour ofexists(), includingin_spaces, are unchanged. Whenexists()binds to a match found by the query, the object's space is set to the space of that instance.Existence queries request
EXISTENCE_QUERY_SIZE(2) results for efficiency, since it is rare to have more than one duplicate. If the response reports a larger total, all matches are retrieved in a second query, so that an instance in the target space is not missed.Related bug fix: In
exists(), aConnectionErrorother thanRemoteDisconnectedwas swallowed, leaving the result undefined and raising aNameError. TheConnectionErroris now re-raised.Tests:
MockKGClientnow returns the space of matching instances under the name used by the query API. New tests cover children and top-level objects found in another space, preferring the target space, duplicates in the target space and in other spaces, retrieving all matches, and connection errors.