Feature: De-fork sparql-client, vanilla 3.2.2 + goo-owned bolt-ons (Ship 1) - #194
Merged
Merged
Conversation
…into goo Stop depending on the ncbo/sparql-client hard fork; pin vanilla sparql-client 3.2.2 and carry the NCBO-specific behavior as goo-owned bolt-ons. Re-homed: - form-urlencoded query transport (Client#make_post_request) - multiple-FROM + nested-UNION serialization (Ext::QuerySerialization) - union-with-bind DSL as a goo QueryElement (Ext::UnionWithBind, QueryBuilder) - xsd:string forcing, empty-binding tolerance, INSERT/INSERT DATA toggle (Ext::VirtuosoCompat) - Redis read-through caching (Goo::SPARQL::Cache + query/update overrides, invalidate-after-write) Removes the fork-era query-logging wiring (vanilla lacks it; goo-owned logging is reintroduced separately). Offline characterization tests pin the generated SPARQL as byte-identical across the swap. Re-implemented onto ncbo's minitest-6 base (not merged from the alexskr POC fork).
…parity, §3 tests)
Implements the parity-pure gates from docs/sparql-defork-review.md (D13 Ship 1):
- D3/H-1: match the fork's cache scope — only JSON SELECT/ASK results (Solutions or
the ASK boolean) are cached; CONSTRUCT/DESCRIBE graph results are not (the fork wrote
the cache solely from the RESULT_JSON branch of parse_response).
- D8/M-2: restore the fork's raise ("Unsupported cacheable query") when caching is on
and an update has no graph (incl. plain string updates) — fail loudly before the
write rather than silently leave stale cache.
- D5/§6: make use_cache authoritative regardless of add_redis_backend vs
add_sparql_backend order (set_sparql_cache at the end of add_sparql_backend), and add
the OP_USE_CACHE env opt-in (goo default remains OFF).
- M-5 parity restore: reinstate the fork's inner DEL rescue in cache invalidation —
a failed DEL warns and moves on instead of silently entering the sleep(5)×3 retry
loop (the port had dropped the fork's log line and its two-layer rescue).
- M-1: reword the invalidate-after-write comments — narrows, not eliminates, the
cache-aside race.
Tests (review §3): new test/test_cache_unit.rb — 17 backend-free unit tests covering
key format, store/get round-trip, inert-when-off, reload/bypass, stale-entry eviction
(the allkeys-lru fail-safe), the 50MB guard, cache scope (T-7), invalidation mechanics
+ DEL-failure path, Marshal round-trip, empty-binding tolerance, xsd:string forcing,
form-urlencoded transport, the D8 raise, UnionWithBind contract, and use_cache
authority (T-24). Plus T-12 backend-matrix locks in the query characterization
(graphdb BIND branch, allegrograph FILTER branch).
…/B parity Carried over from the POC branch so the review + A/B parity record ship with the de-fork PR (resolves review nit N-1).
goo's Gemfile pin only governs goo's own bundle. Consumers (OLD, ontologies_api, ncbo_cron, annotator, recommender) resolve sparql-client through goo.gemspec, which was unversioned; once they drop their fork Gemfile pins they would pick up the latest rubygems release (3.3.0 today), while the de-fork bolt-ons copy and prepend 3.2.2 internals (QuerySerialization#to_s, make_post_request, parse_json_value; review M-3 / section 7.5). Pin the gemspec dependency to 3.2.2 so every consumer resolves the reviewed version; bump deliberately with a re-review of the ext/ prepends.
Member
Author
|
Local AllegroGraph verification of the merge preview (this branch merged into current development 752fb6b), against AG in Docker + clean Redis: 216 runs, 3216 assertions, 0 failures, 0 errors, 2 skips (backend-specific skips). Together with the 4store run recorded in the description and the green 4-backend CI matrix, all pre-merge verification is complete. |
This was referenced Jul 28, 2026
mdorf
changed the title
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
2 participants
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.
What this does
goo used to get its SPARQL client from an NCBO fork of the
sparql-clientgem, with our custom features edited directly into the gem's source. This PR switches goo to the standardsparql-client3.2.2 from rubygems and moves every NCBO customization into goo itself, where it is tested and maintained like any other goo code.Why
The fork was expensive to carry. Its diff against upstream was mostly whitespace, which buried the real changes, so every upstream security or bug fix meant a risky manual merge, and one past merge silently lost a feature. With the customizations living in goo, upgrading the gem becomes a normal dependency bump.
What moved into goo
lib/goo/sparql/cache.rb. Same Redis key format and same caching scope as before, so existing cache contents stay valid. Two ordering bugs were fixed in the move: results are now cached exactly as returned, and invalidation happens after a write commits instead of before.lib/goo/sparql/ext/query_extensions.rb, along with support for multiple FROM clauses and nested UNIONs.lib/goo/sparql/ext/virtuoso_compat.rb.Goo.use_cacheis now respected regardless of configuration order, andOP_USE_CACHE=truecan enable caching from the environment. goo's default remains off.goo.gemspecnow pinssparql-client = 3.2.2, so consuming apps resolve exactly the version goo was tested against.How we know behavior did not change
docs/sparql-defork-ab-record.md.development.Action needed in consuming repos
Each downstream repo (ontologies_linked_data, ncbo_annotator, ncbo_cron, ncbo_ontology_recommender, ontologies_api) pins the fork in its own Gemfile. Those pins must be removed when picking up this goo version; otherwise bundler keeps installing the fork and its built-in caching runs on top of goo's.
Not in this PR (planned follow-ups)
feature/sparql-resilience: circuit breakers so a Redis or triple store outage fails fast instead of hanging requests. Opt-in via env flag.feature/sparql-observability: reworked query logging, cache hit-rate, and query-count reporting. Opt-in.Design and full review notes:
docs/sparql-client-defork-proposal.mdanddocs/sparql-defork-review.md.Work by @alexskr; review, verification, and shipping by @mdorf.