triples_update: atomic asymmetric updates, blank-node identity, and pooled connections - #8
Open
EHoffm wants to merge 5 commits into
Open
Conversation
IRI._sanitize rejected any http(s) URL containing a port (e.g.
http://host:8991/path): it counted the scheme's own colon plus the authority
port colon as "too many" via `raw.count(":") > 1`. This also broke reads,
since query(convert_bindings=True) / triples_get convert an xsd:anyURI literal
to an IRI, so any stored ported URL made the read itself raise.
Fix: after confirming the scheme, strip it, split off the authority (up to the
first '/', '#', or '?'), remove a trailing ':<digits>' port, and only then
reject a stray ':' in the authority or remainder. Ports are accepted; the
previously-rejected ":"-for-"#" malformations still raise. All existing
tests/test_iri.py cases pass.
Found by kapps_semantic_middleware, whose Service ontology stores middleware
endpoint URLs (svc:address, svc:endpoint) as xsd:anyURI host:port values.
See CHANGELOG.md.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
triples_update previously raised unless old_triples and new_triples had equal length, limiting it to 1:1 replacement. It already builds a single DELETE ... INSERT ... WHERE SPARQL transaction, which handles additions, removals, and unequal-size replacements equally, so the length restriction is removed and the existence pre-check is skipped for empty old_triples (a pure insert has nothing to check). The single-transaction atomicity is required for SHACL correctness: replacing a cardinality-constrained property (e.g. a possession handover under a "possessed by exactly one resource" shape) must apply removal + insertion together so the intermediate property-absent state is never validated. Enables kapps_ogm.OGM.commit to add/remove/replace atomically. See CHANGELOG.md. Requested by kapps_semantic_middleware. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
A BNode passed in both old_triples and new_triples is the same node, but the DELETE and INSERT patterns were built from two separate blank-node -> variable maps, so it rendered as ?oldbn1 and ?newbn1 — and every new-side variable was bound by BIND(BNODE()), minting a replacement. Updating one property of an existing blank node therefore unlinked it and orphaned every triple the caller had not listed in old_triples, silently and with a success return. Use one shared map across both pattern sets, and emit BIND(BNODE()) only for blank nodes exclusive to new_triples. Pure additions, pure removals, unequal-length replacements, the IRI-only path and the single-transaction atomicity are all unchanged. tests/test_triple_update_query.py asserts on the generated SPARQL and needs no live repository. Verified red before / green after: the two identity tests fail on the previous implementation, the two control tests pass on both. Refs JaFeKl#6 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
_make_request dispatched through the module-level requests helpers (getattr(requests, method)(...)), which build a Session, use it for one request, and close it. Closing discards the urllib3 connection pool, so every call paid a fresh TCP connect and a full TLS handshake. Against a remote HTTPS endpoint that dominated the cost of a call: a trivial ASK measured ~17ms, only ~4ms of which was the request. The kapps_semantic_middleware suite issues 129 requests for a single integration test, so the waste compounded — 158s for 146 tests, of which 87-92% was HTTP wall time. The client now holds a persistent session per thread. Per thread rather than one shared session because a requests.Session mutates its cookie jar on every response and is not thread-safe, and one client is commonly driven from several threads at once — a web framework serving requests while the embedding code queries. urllib3's pools are thread-safe; the session around them is not. Each thread pays one handshake, then reuses its own pool. close() releases the pools for every thread that used the client. No signature or response-handling change. Per-request cost ~17ms -> ~4ms. This repo's own suite 12.3s -> 6.6s; the kapps_semantic_middleware suite 158s -> 53s, tests unchanged. The regression tests count urllib3 connection creations rather than timing anything, so they are deterministic and do not flake on a fast or slow network. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
65157c5 widened triples_update from 1:1 replacement to general atomic add/remove/replace, but nothing in this suite held that open: the only existing update test is symmetric 2-in/2-out, so reinstating if len(old_triples) != len(new_triples): raise InvalidInputError(...) would have passed here and broken only downstream, in kapps_ogm, whose OGM.commit feeds a set-difference diff straight into this method. Two tests, both directions -- more added than removed and the reverse -- asserting the resulting graph state rather than just the return value. Verified they fail (8/8 across the named-graph parametrisations) with the length guard temporarily restored. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the GraphDB client and query builder to support more correct and efficient triple updates: triples_update becomes a general atomic add/remove/replace operation, preserves blank-node identity across DELETE/INSERT, and the HTTP layer now reuses connections via per-thread sessions.
Changes:
- Generalize
triples_updateto allow asymmetric old/new triple lists while keeping a single atomic SPARQL update. - Fix blank-node identity in
triples_updateby sharing one BNode→variable map across DELETE and INSERT, minting only new-side-only BNodes. - Reuse HTTP connections via a persistent
requests.Sessionper thread, and add tests + changelog entries for the behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_triple_update_query.py | Adds query-construction tests pinning blank-node identity behavior without needing a live GraphDB. |
| tests/test_graph_manipulation.py | Adds live tests asserting asymmetric add/remove behavior for triples_update. |
| tests/test_connection_reuse.py | Adds tests asserting connection pooling behavior and per-thread session isolation. |
| graph_db_interface/utils/iri.py | Fixes IRI sanitization to allow URLs with authority ports while still rejecting stray colons. |
| graph_db_interface/queries/triple_multi.py | Implements asymmetric atomic updates and shared blank-node identity logic in query generation. |
| graph_db_interface/graph_db.py | Switches HTTP requests to per-thread persistent sessions and introduces close() for pool cleanup. |
| CHANGELOG.md | Documents the behavioral changes and fixes in an Unreleased changelog. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+281
to
+285
| def close(self) -> None: | ||
| """Release the HTTP connection pools held for every thread that used this client.""" | ||
| with self._sessions_lock: | ||
| sessions, self._sessions = self._sessions, [] | ||
| for session in sessions: |
Comment on lines
+41
to
+47
| for _ in range(3): | ||
| db.query("ASK { ?s ?p ?o }") | ||
|
|
||
| assert count_new_connections == [], ( | ||
| f"expected the warm connection pool to be reused, but " | ||
| f"{len(count_new_connections)} new connection(s) were opened: {count_new_connections}" | ||
| ) |
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.
Merges
fix/bnode-identity-in-triples-updateintomain, as requested by #7. Clean fast-forward —mainis an ancestor, 0 behind / 5 ahead.Closes #6. Addresses the merge half of #7; its release/version-bump criterion stays open until a release is cut.
What's in it
34dac9e65157c5triples_updateto atomic add/remove/replace (unequal lengths)e659da1triples_updatec90c940da08373triples_updatecontract with testsPlease note the last two are beyond what #7 described — that ticket's table lists only the first three, written before these landed.
c90c940is an unrelated performance fix._make_requestdispatched through the module-levelrequestshelpers, which build aSession, use it for one request and close it; closing discards the urllib3 pool, so every call paid a fresh TCP connect and full TLS handshake. Against a remote HTTPS endpoint that was ~13ms of every 17ms. It is now a persistent session per thread — not one shared session, becauserequests.Sessionmutates its cookie jar on every response and is not thread-safe, and this client is commonly driven from server threads while the embedding code queries.close()releases the pools for every thread that used the client. No signature or response-handling change.Tests
Per this repo's no-tests-no-merge rule, each behavioural change is pinned:
tests/test_graph_manipulation.py) — two live tests, both directions, asserting resulting graph state rather than the return value. Verified they fail 8/8 across the named-graph parametrisations with the old length guard temporarily restored. This was the gap: the only pre-existing update test was symmetric 2-in/2-out, so reinstating the guard would have passed here and broken only downstream inkapps_ogm.tests/test_triple_update_query.py) — four query-construction tests covering retained, minted, mixed, and no-blank-node cases. They stubqueryand assert on SPARQL text, so they need no live GraphDB.tests/test_connection_reuse.py) — three tests counting urllib3 connection creations and asserting per-thread session isolation. They count connections rather than timing anything, so they cannot flake on a fast or slow network.Full suite: 107 passed (was 96 before this branch), 9.16s — down from ~18s, since the pooling fix speeds up the suite's own live calls.
Downstream
kapps_ogmcurrently pins its dependency directly at this branch so development can continue;SAWeindel/kapps_ogm#22tracks reverting that pin once a release exists. (#7 cites#23for this — that issue does not exist, the correct one is #22.)🤖 Generated with Claude Code