feat: Oxigraph 0.5.7 + RDF 1.2 reifiers + Rust model query engine + JS pipeline elimination + SHACL sh:in#803
Open
feat: Oxigraph 0.5.7 + RDF 1.2 reifiers + Rust model query engine + JS pipeline elimination + SHACL sh:in#803
Conversation
- buildPaginationSubquery(): generates an inner SELECT DISTINCT ?source subquery with ORDER BY, LIMIT, OFFSET so the database constrains the result set before fetching all links for those sources - buildSPARQLCountQuery(): separate COUNT(DISTINCT ?source) query for getting totalCount without hydrating instances - hasJsOnlyWhereFilters() guards: when JS-only filters (author, timestamp, comparison operators on literal properties) are present, pagination stays in JS to avoid incorrect result capping - buildSPARQLOrderLimitOffset() remains empty (backward compat) - real pagination is handled by the subquery pattern in buildSPARQLQuery() - Updated existing tests to verify SPARQL-level pagination behavior
- Added deepQuery?: boolean to the Query type in types.ts - instancesFromQueryResult() now skips evaluateCustomGettersForInstance() unless query.deepQuery === true (inverted default) - Single-instance get() continues to evaluate getters by default via getData() which is unaffected by the deepQuery guard - Added Ad4mModel.evaluateGetters() static method for on-demand batch getter resolution on visible items - Added deepQuery() method to ModelQueryBuilder for fluent API usage - instancesFromQueryResult() detects SPARQL-level pagination and skips redundant JS-level Array.slice() when SPARQL already constrained results - Updated existing test to reflect ORDER BY/LIMIT/OFFSET now in SPARQL
- Added #proxyCache (Map<string, WeakRef<PerspectiveProxy>>) to PerspectiveClient - byUUID() now checks cache first, returns existing proxy for same UUID - WeakRef allows GC of unreferenced proxies (no memory leak) - Added es2021 to tsconfig lib for WeakRef support
22 tests covering: - WS-1: SPARQL pagination subquery generation, LIMIT/OFFSET inclusion, ORDER BY behavior, JS-only filter fallback, count query, and instancesFromQueryResult pagination bypass - WS-2: Getter skipping on default collection queries, deepQuery opt-in, single-instance get() getter evaluation, evaluateGetters() batch resolution, and ModelQueryBuilder deepQuery() method - WS-7: Proxy cache hit (same reference), cache miss (different UUIDs), and re-fetch after cache eviction
- paginate() now issues 2 SPARQL queries (data + count), update test expectation
- count() now uses buildSPARQLCountQuery, update mock to return [{count: N}]
- Both tests now correctly verify the optimised query paths
…files - WS-1 pagination tests → query-sparql.test.ts - WS-2 deepQuery/getter tests → Ad4mModel.test.ts - WS-7 proxy cache tests → PerspectiveClient.test.ts (new) - Delete monolithic architectural-optimisation.test.ts - 412/414 tests passing (1 pre-existing failure in Ad4mClient.test.ts)
Code comments now describe what the code does rather than referencing internal planning workstream identifiers.
1. Test: use offset:10 in pagination test to prove no double-slicing (was offset:0) 2. Evaluate getter-backed where fields before JS filtering — when query.where references getter properties, evaluate those specific getters even without deepQuery so the JS post-filter compares against real values 3. Sync _snapshot after evaluateGetters() to prevent isDirty() from flagging getter-only mutations, which would cause save() to write read-only fields 4. Bind order-only properties in pagination subquery — add OPTIONAL joins for order properties not already in the WHERE clause so ORDER BY is deterministic 5. Remove unused tsconfig-browser.json (es2018/commonjs, not referenced), remove WeakRef proxy cache from PerspectiveClient (simplifies to fresh proxy per call), update PerspectiveClient tests accordingly
- Add waitForCalls() helper that polls mock call count with timeout instead of asserting immediately after async mutation - Fix removeListener test: after removeListener + proxy cache, the callback should NOT fire (was asserting 1, now correctly asserts 0) - Apply waitForCalls to all listener/subscription assertions that depend on async WebSocket event delivery
- update() now replaces the cached proxy with the updated one - remove() evicts the UUID from cache so next byUUID re-fetches - Fixes integration-tests-js failure where byUUID returned stale cached proxy after perspectiveUpdate mutation - Added tests for cache update and eviction behaviour
…ift) The lockfile had 5329 lines of diff from pnpm resolving newer transitive deps locally. No package.json changes require lockfile updates — our changes only add to source files.
The proxy cache in PerspectiveClient.byUUID() caused integration test failures: tests that call update() then byUUID() expect fresh state, but cached proxies returned stale handles. The cache semantics are incompatible with the current test suite and would need deeper integration with the subscription system to stay in sync. Reverted: proxy cache Map, update/remove cache invalidation, test file. Kept: waitForCalls helper for flaky WebSocket subscription tests.
…Client Add a proxy cache (Map<uuid, PerspectiveProxy>) to PerspectiveClient as the single source of truth for proxy instances. Every code path that creates or returns a PerspectiveProxy now goes through getOrCreateProxy(), which: - Returns cached proxy on hit, updating its handle in-place via updateHandle() - Creates a new proxy on miss (registers listeners only once in constructor) Changes: - PerspectiveProxy: add updateHandle(handle) for in-place field updates - PerspectiveClient: add #proxyCache and private getOrCreateProxy() - all(), byUUID(), add(), update(): route through getOrCreateProxy() - byUUID(): cache-first with server fallback (zero network on cache hit) - remove(): evict from cache before mutation - Subscription handlers (added/updated/removed): update/populate/evict cache - No duplicate listener registrations: constructor only called on cache miss Tests: - Ad4mClient.test.ts: add cache hit, in-place update, eviction, all() tests - Ad4mClient.test.ts: fix removeListener test (toBeCalledTimes(0) with cache) - New PerspectiveClient.test.ts: 8 focused unit tests with mock Apollo client
…eries The WS-2 deepQuery opt-in incorrectly skipped relation getter evaluation when deepQuery was false. This broke @HasMany/@hasone type-conformance filtering — targets were returned unfiltered because the auto-generated conformance getter never ran. Fix: split getter evaluation into two phases: - Property getters: only when deepQuery=true or where references them - Relation getters: always (they enforce type correctness, not hydration) Fixes integration-tests-js failures: - 'should filter results in findAll() by type' (got 2 instead of 1) - 'paginate() and paginateSubscribe()' (timeout from broken filtering)
paginateSubscribe() was baking LIMIT/OFFSET into the subscription's SPARQL query. With SPARQL-level pagination (WS-1), this meant the subscription only monitored a fixed page — adding items beyond the page boundary didn't change the result set, so the callback never fired. Fix: subscribe to the full (unpaginated) result set so any change is detected. Pagination is still applied in processResults via the paginationQuery passed to instancesFromQueryResult.
Subscription queries use unpaginated SPARQL to detect all changes, but processResults passes paginationQuery with limit/offset. instancesFromQueryResult must always slice — SPARQL pagination is a performance optimisation (less data over the wire), but JS slicing is the correctness guarantee that works regardless of the query source.
…, test improvements) - waitForCalls now throws on timeout instead of returning silently - Fixed test title: 'remove() evicts proxy from cache (new reference on next fetch)' - Added computeTotalCount flag to instancesFromQueryResult to skip COUNT query for callers that discard it (findAll). findAllAndCount, paginate, and count pass true; findAll passes false (default) - Extracted parseSparqlCount() helper into query-sparql.ts, used in both instancesFromQueryResult and count() to eliminate duplication - Cache eviction on update(): PerspectiveClient.update() now evicts stale proxy before creating fresh one; perspectiveUpdated subscription does same - Added UUID mismatch guard in PerspectiveProxy.updateHandle() — throws if incoming handle UUID differs from the proxy's bound UUID - Updated PerspectiveClient.test.ts to match new evict-and-recreate semantics
- Upgrade oxigraph 0.4 → 0.5.7 with rdf-12 + rocksdb features
- Replace named graph storage (GRAPH ?g { s p o }) with RDF 1.2 reifiers
(direct triple in default graph + rdf:reifies <<( s p o )>> for metadata)
- Each link now stored as 8 quads: triple + reifier + author + timestamp +
source/predicate/target on reifier + status
- Update SparqlEvaluator API (QueryOptions → SparqlEvaluator, Subject →
NamedOrBlankNode, Query::parse → SparqlEvaluator for validation)
- Add migrate_named_graphs_to_reifiers() for data migration on startup
- Update SDK query generation: buildSPARQLQuery, buildSPARQLGetDataQuery,
query-sparql-batch.ts, hydration.ts all use reifier patterns
- Add PREFIX rdf: to all generated SPARQL queries
- 59 Rust tests + 36 SDK query-sparql tests + 153 Ad4mModel tests pass
is_sparql_query() did not recognize queries starting with PREFIX or BASE declarations, causing subscribe_and_query() to fall through to the Prolog path and return false instead of SPARQL result arrays. Rust fix: case-insensitive matching + PREFIX/BASE recognition. JS fix: remove PREFIX declarations from generated model queries, use full IRIs (<http://www.w3.org/1999/02/22-rdf-syntax-ns#reifies>) as belt-and- suspenders approach.
…iltering Add a new perspectiveModelQuery GraphQL endpoint that moves ORM query execution (SPARQL generation, hydration, filtering, sorting, pagination) from TypeScript into Rust for better performance and consistency. Key changes: - New model_query.rs (~1500 lines): Rust model query engine with SPARQL generation, typed literal parsing, property/relation hydration, where filtering, sorting, reverse relation resolution, and includes support - Updated Ad4mModel.ts: executeModelQuery() bridges TS to Rust endpoint with prototype-chain-aware instance construction - Updated ModelQueryBuilder.ts: routes queries through executeModelQuery - Updated PerspectiveClient/Proxy: new modelQuery() method - GraphQL resolver: perspectiveModelQuery wired to Rust engine - 15 Rust unit tests, 424 TS unit tests passing Benchmark results vs feat/sparql-1.2 (first-call, 115k links): Full page load: 3304ms -> 1546ms (-53%) Message.findAll(limit:10): 2128ms -> 345ms (-84%) Message.findAll(paginate): 2151ms -> 354ms (-84%) Channel.findAll(): 638ms -> 502ms (-21%) Variance (stddev): 45-80x lower across all operations Known: count() needs SPARQL COUNT optimization (currently does full query). Nested includes with get() still use TS-side relation resolution.
Two optimizations that eliminate the remaining performance gaps:
1. COUNT fast path (count() was 484ms, now 5.7ms — 98.8% faster)
- When all where conditions are SPARQL-pushable (equality, boolean),
runs SELECT COUNT(DISTINCT ?source) directly instead of hydrating
all instances and counting in memory
- Extracted shared query pattern builder for both COUNT and full queries
- Added all_where_pushable() to detect when fast path is safe
2. Rust-side include resolution (eliminates per-relation GraphQL round-trips)
- TS enriches shape metadata with target class shapes for included
relations before sending to Rust
- New resolve_includes_recursive() in Rust handles forward (hasMany/
hasOne) and reverse (belongsToOne/belongsToMany) relations
- Recursive: nested includes like { subgroups: { include: { items: true } } }
resolve entirely in-process
- New jsonToModelInstance() in TS recursively constructs proper class
instances from the fully-hydrated JSON response
Benchmark results vs prior commit (115k links):
Channel.count(): 483.8ms → 5.7ms (-98.8%)
Consistency: stddev improved across all operations
Remove methods that lost their callers after Subject.ts deletion: - getSubjectProxy() (deprecated wrapper) - getAllSubjectProxies() (deprecated wrapper) - getPropertyValueViaSparql() (only caller was Subject.ts) - getRelationValuesViaSparql() (only caller was Subject.ts) - batchCheckSubjectInstances() (only caller was getRelationValuesViaSparql) - filterInstancesSequential() (only caller was getRelationValuesViaSparql) - getSubjectClassMetadataFromSDNA() (merged into getSubjectClassMetadata) -265 lines, all 393 tests pass.
…ode cleanup Phase 11: Remove usePerspectives link bus (Flux side) Phase 12: PerspectiveProxy SPARQL optimization (isSubjectInstance, findInstancesByMetadata, findClassByProperties) Phase 13: Dead hydration code deletion (hydrateFromLinks, hydrateRelations, assignValuesToInstance, hydratePropertyValue) Phase 14: Rust-side model subscription endpoint (perspectiveModelSubscribe) - model_subscribe_and_query() in perspective_instance.rs - build_model_trigger_predicates() for SPARQL trigger generation - check_subscribed_queries() updated for model subscriptions - ModelSubscriptionParams struct for subscription metadata - GraphQL mutation resolver Phase 14 TS: ModelQueryBuilder uses modelSubscribe instead of queryToSPARQL + pooledSubscribe - subscribe(), countSubscribe(), paginateSubscribe() all route through Rust - PerspectiveClient.modelSubscribe() + PerspectiveProxy.modelSubscribe() - Ad4mModel.prepareModelQueryParams() + parseModelResult() helpers Dead code cleanup: - Delete subscription-pool.ts (211 lines) — zero production callers - Delete query-sparql-batch.ts (305 lines) — zero production callers - Delete subscription-delta.test.ts (207 lines) — tested deleted code - hydration.ts: 907 → 304 lines (-603 lines) Tests: 371 passed (12 suites), build clean, no circular deps
Delete buildSPARQLCountQuery (77 lines), groupSPARQLResults (23 lines), parseSparqlCount (12 lines) — zero production callers after Phase 14. Remove corresponding tests. query-sparql.ts: 637 → 510 lines.
- Run cargo fmt on perspective_instance.rs and model_query.rs - Remove unused Subject import in SubjectRepository.ts (fixes Flux CI build: 'Subject is not exported by @coasys/ad4m')
- Delete tests/js/tests/model/model-prolog.test.ts (imports deleted generatePrologFacts function) - Fix Rust evaluate_getters: property getters should not decode literal values through parse_literal_value (returns raw string like base branch) - Fix Rust parse_shape_from_json: don't skip getter-only relations with empty predicate (e.g. @hasmany({ getter: ... })) - Add CI debug output for Message.all() test failure diagnosis
… messages - In executeModelQuery and prepareModelQueryParams, use compileWhereClause() to incorporate relation-level where conditions into conformance getter SPARQL. Previously, where clauses on @HasMany/@hasone were only applied via TS-side post-filtering (evaluateCustomGettersForInstance), which was removed in the Rust migration. Now the where conditions are compiled directly into the getter SPARQL sent to Rust. - In subscribe() and countSubscribe(), skip delayed #init# messages that contain stale initial data. These messages (sent at 10s/15s/20s/25s delays) could overwrite real subscription updates, causing subscription callbacks to appear to never fire on link removal. - Remove CI-DEBUG logging from flag link test (now passing).
…terability check
Three fixes for integration test failures:
1. Rust hydrate_one: exclude properties with getters from pred_to_prop HashMap.
When two relations share the same predicate (e.g. entries and messages both
using flux://entry_type), the HashMap overwrites one. Now getter-having
properties are excluded from link-based hydration since evaluate_getters()
handles them separately.
2. Restore TS-side evaluateCustomGettersForInstance() in executeModelQuery.
The base branch called this after the Rust pipeline for relation conformance
filtering. Our branch removed it, relying solely on Rust-side evaluate_getters.
Adding it back as a fallback ensures conformance + JS post-filtering works
for relations with non-SPARQL-filterable where clauses.
3. Add allSparqlFilterable check for relation where-clause compilation.
Properties without resolveLanguage or with resolveLanguage='literal' store
values as signed JSON expressions (literal:json:{...}), not plain
literal:string:X IRIs. SPARQL exact-match fails for these. Now the
where-clause is only compiled to SPARQL when all target properties use
a non-literal resolveLanguage (plain URIs).
Without OPTIONS handler, Chrome CORS preflight fails with 405, blocking all browser fetch() requests to the executor. - Add warp::options() route returning 204 No Content - Add Access-Control-Allow-Methods and Access-Control-Allow-Headers headers to both non-TLS and TLS server blocks
evaluate_getters was skipping collection properties that appeared in the include map, but resolve_forward_include needs the raw target IDs that the getter populates. Without those IDs, include resolution has nothing to hydrate and the relation comes back empty. Remove the skip so getters always run first, then resolve_forward_include replaces the raw IDs with hydrated objects.
… test 1. Ad4mModel.timestamp getter was removed in the dead-code cleanup but is still used by integration tests (and potentially downstream). It returns createdAt (epoch ms) — needed for where-clause lt/gt/lte/gte comparisons which expect numeric values. 2. paginateSubscribe test: waitForCondition now checks for totalCount === 11 instead of just lastResult !== null, avoiding a race where the subscription callback fires with stale data before the SPARQL store reflects the new record.
HexaField
force-pushed
the
feat/sparql-1.2-cleanup
branch
from
5b01ef0 to
4ee6cef
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
core/src/model/Ad4mModel.ts (1)
856-907: ⚡ Quick winConformance getter pre-computation logic is duplicated.
The code block at lines 856–907 is nearly identical to lines 979–1043 in
executeModelQuery. Both compute conformance getters with where-clause SPARQL compilation.Consider extracting a shared helper (e.g.,
precomputeConformanceGetters(metadata, allRelMeta)) to eliminate duplication and ensure both code paths stay in sync.♻️ Proposed refactor: extract shared helper
// Add near the top of the file, after enrichShapeForIncludes /** * Pre-compute conformance getters for relations with target classes. * Sets `rel.getter` on metadata.relations entries for Rust-side evaluation. * `@internal` */ function precomputeConformanceGetters( metadata: ModelMetadata, allRelMeta: Record<string, RelationMetadataEntry>, ): void { for (const [relName, relMeta] of Object.entries(metadata.relations)) { const rel = relMeta as any; if (rel.getter || rel.direction === 'reverse' || rel.filter === false) continue; const meta = allRelMeta[relName]; if (!meta?.target) continue; try { const TargetClass = meta.target(); const filter = buildConformanceFilter(meta.predicate, TargetClass); let whereConditions: string[] = []; if (rel.where) { try { const targetMetadata = (TargetClass as any).getModelMetadata?.() ?? null; let allSparqlFilterable = true; if (targetMetadata) { for (const propName of Object.keys(rel.where)) { if (['id', 'author', 'timestamp'].includes(propName)) continue; const propMeta = targetMetadata.properties[propName]; if (propMeta && (!propMeta.resolveLanguage || propMeta.resolveLanguage === 'literal')) { allSparqlFilterable = false; break; } } } else { allSparqlFilterable = false; } if (allSparqlFilterable) { whereConditions = compileWhereClause(rel.where, targetMetadata); } } catch (_) {} } if (filter) { let getter = filter.getter; if (whereConditions.length > 0) { getter = getter.replace(/ \}$/, ` ${whereConditions.join(' ')} }`); } rel.getter = getter; } else if (whereConditions.length > 0) { const escapedPred = meta.predicate.replace(/[<>"{}|\\^`\u0000-\u0020]/g, ''); rel.getter = `SELECT ?target WHERE { <Base> <${escapedPred}> ?target . ${whereConditions.join(' ')} }`; } } catch (_) {} } }Then replace both occurrences with:
precomputeConformanceGetters(metadata, allRelMeta);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/model/Ad4mModel.ts` around lines 856 - 907, The conformance-getter computation is duplicated in the top-level pre-compute block and inside executeModelQuery; extract that logic into a shared helper (e.g., precomputeConformanceGetters) and call it from both places to keep behavior in sync. Move the loop over metadata.relations and its inner logic (uses of allRelMeta, meta.target(), buildConformanceFilter, compileWhereClause, rel.where handling, getter construction and escaping) into precomputeConformanceGetters(metadata, allRelMeta) and replace both duplicated blocks with a single call to that helper; preserve the same guards (rel.getter, rel.direction === 'reverse', rel.filter === false) and error swallowing. Ensure the helper returns void and mutates metadata.relations entries (setting rel.getter) exactly as the original code did.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/model/ModelQueryBuilder.ts`:
- Around line 649-661: The current processResults builds two separate queries (a
countQuery with limit:0 and a paginatedQuery) and calls (ctor as
any).executeModelQuery twice, causing double load and possible snapshot
mismatch; instead combine into a single query with count: true (e.g. include
count: true, limit: pageSize, offset: pageSize * (pageNumber - 1)) and call
(ctor as any).executeModelQuery once, then destructure { results, totalCount }
from that single response and pass to callback; apply the same change to the
other identical block referenced (lines ~680-684) so paginateSubscribe uses the
single { count: true, limit, offset } executor call.
- Around line 663-666: The paginateSubscribe() handler currently calls
processResults() for every event from
this.perspective.client.subscribeToQueryUpdates(subscriptionId, ...), causing
late isInit messages to replay stale data; update the callback to accept the
event (e.g., rawResult) and return early when rawResult?.isInit is true before
invoking processResults(), mirroring the guards used in subscribe() and
countSubscribe() so the explicit initial fetch isn't followed by an extra
callback.
In `@core/src/perspectives/PerspectiveProxy.ts`:
- Around line 1468-1470: createSubject() currently returns the raw expression
address (exprAddr) for non-batch calls which breaks callers expecting a
proxy/object of type T (e.g., code that calls setProperties(newInstance, data)
and getSubjectData(newInstance)); change createSubject() so that when B is
undefined (non-batch) it returns a proxy-compatible object instance instead of
just exprAddr — for example, after creating the expression use the same proxy
construction path you use elsewhere (via Ad4mModel or the existing subject
factory) to build and return the T instance that wraps exprAddr, or
alternatively if you intend to return raw addresses change the public signature
to Promise<string> and update callers; focus edits on the createSubject() return
path and the code that currently returns exprAddr so callers like
getSubjectData() work without runtime errors.
- Around line 2027-2049: getInstanceClasses currently returns any class that
matches a single required sh:minCount path; change it to require all required
triples and honor fixed-target constructor constraints by reusing the same
requiredTriples semantics used in isSubjectInstance() and
findInstancesByMetadata(). Specifically, in getInstanceClasses(baseExpression)
locate or reuse the helper logic that builds requiredTriples (paths + any fixed
target checks) from a class's shape, then change the SPARQL generation to
require every required path (e.g., via multiple EXISTS/VALUES or a GROUP BY
HAVING count equal to requiredTriples.length) and include checks for
fixed-target constructor values where present so a class is only returned when
all its required predicates/values are satisfied for <baseExpression>. Update
getInstanceClasses to call or mirror the same helper used by
isSubjectInstance()/findInstancesByMetadata() to ensure consistent semantics.
- Around line 1995-2003: The sh:in payload is being read from a literal:string:
URI without URI-decoding, which breaks values with spaces/quotes/% and won't
round-trip with addNamedOption that writes encoded literals; in getClassShape
(in PerspectiveProxy.ts) update the parsing logic for row.shIn to first strip
the literal:string: prefix, then URI-decode the remaining string (e.g.,
decodeURIComponent or equivalent) before calling JSON.parse, and repeat the same
fix in the other parsing location noted (around the block at ~2103-2108); ensure
you still swallow parse errors but only after decoding and parsing.
In `@core/src/shacl/SHACLShape.ts`:
- Around line 554-560: fromLinks() currently ignores the persisted enum values
emitted by toLinks(), so when reconstructing properties you must detect links
with predicate "sh://in" and restore prop.in from the link target; in the
fromLinks / link-processing logic (the method named fromLinks and the code that
builds prop objects) add a branch that when link.predicate === "sh://in" strips
the "literal:string:" prefix from link.target, JSON.parse(...) the remainder
into an array, and assign that array to the corresponding property’s prop.in
(with a safe try/catch or fallback to an empty array if parsing fails).
In `@rust-executor/src/perspectives/model_query.rs`:
- Around line 1648-1652: The current chain on the getter variable does a blind
.replace("Base", ...) which mangles any occurrence of "Base" inside IRIs or
identifiers; stop the global replacement and only substitute the exact tokens
(e.g. replace "?source" and "<Base>" only) or perform a token-aware replacement
(e.g. use a regex that matches Base only when wrapped in angle brackets or as a
standalone token) in the code in model_query.rs where getter is transformed so
that only the placeholder "<Base>" and "?source" are replaced rather than every
"Base" substring.
- Around line 261-268: The predicates() helper currently only clones property
predicates and thus drops getter-only relations; update ModelShape::predicates
to include predicates from properties, relations and flags, and for relations
where predicate is empty but a getter exists, extract and include the getter's
declared dependency predicates (e.g., from relation.getter.dependencies or
relation.getter.deps) so modelSubscribe() receives the actual link dependencies;
ensure you reference ModelShape::properties, ModelShape::relations,
ModelShape::flags and the relation.getter dependency field when collecting
predicates.
- Around line 1726-1744: The getter branch stores the first binding as a raw
string (prop.name insertion) which causes type mismatches for matches_where()
and sort_instances(); update the branch that handles first_val → val to decode
literal strings into proper JSON Value types the same way hydrated properties
are decoded (use the same literal-decoding logic/function used for hydrated
properties) before inserting into instance (instead of always inserting
Value::String). Ensure the decoded Value is inserted under prop.name so
downstream matches_where() and sort_instances() operate on correct types.
In `@rust-executor/src/perspectives/perspective_instance.rs`:
- Around line 4172-4219: build_model_trigger_predicates currently stops using
the store-backed shape if any predicates were found in the client‑provided
shape_json, which misses predicates defined only in the stored shape; change the
logic so after parsing shape_json it always merges (union) the predicates from
super::model_query::load_shape_from_store(&self.sparql_store, class_name) into
the predicates vector (or set) rather than using the store only when
predicates.is_empty(); ensure you deduplicate entries when combining results so
both sources contribute triggers.
- Around line 4118-4125: When reusing an existing subscription (the branch
guarded by existing_subscription), also refresh the cached trigger metadata on
the stored subscription entry in addition to updating q.last_result: acquire the
same subscribed_queries lock (as done), get_mut(&existing_id) and update
q.trigger_sparql and q.predicate_set with the freshly recomputed values (and
q.shape_json if applicable) so the subscription uses the new predicate set when
shape_json was previously None; ensure the updates happen inside the same
mutex-protected block where q.last_result is set so the stored entry is fully
refreshed before releasing the lock.
---
Nitpick comments:
In `@core/src/model/Ad4mModel.ts`:
- Around line 856-907: The conformance-getter computation is duplicated in the
top-level pre-compute block and inside executeModelQuery; extract that logic
into a shared helper (e.g., precomputeConformanceGetters) and call it from both
places to keep behavior in sync. Move the loop over metadata.relations and its
inner logic (uses of allRelMeta, meta.target(), buildConformanceFilter,
compileWhereClause, rel.where handling, getter construction and escaping) into
precomputeConformanceGetters(metadata, allRelMeta) and replace both duplicated
blocks with a single call to that helper; preserve the same guards (rel.getter,
rel.direction === 'reverse', rel.filter === false) and error swallowing. Ensure
the helper returns void and mutates metadata.relations entries (setting
rel.getter) exactly as the original code did.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 368138eb-0dc3-4dae-82ad-d8422a848e11
📒 Files selected for processing (27)
ad4m-hooks/helpers/src/factory/SubjectRepository.tscore/src/model/Ad4mModel.test.tscore/src/model/Ad4mModel.tscore/src/model/ModelQueryBuilder.tscore/src/model/Subject.tscore/src/model/decorators.tscore/src/model/hydration-batch.tscore/src/model/hydration-batching.test.tscore/src/model/hydration.tscore/src/model/index.tscore/src/model/prolog-facts.tscore/src/model/query-prolog.tscore/src/model/query-sparql-batch.tscore/src/model/query-sparql.test.tscore/src/model/query-sparql.tscore/src/model/shacl-gen.tscore/src/model/subscription-delta.test.tscore/src/model/subscription-pool.tscore/src/perspectives/PerspectiveClient.tscore/src/perspectives/PerspectiveProxy.tscore/src/shacl/SHACLShape.tsrust-executor/src/graphql/mod.rsrust-executor/src/graphql/mutation_resolvers.rsrust-executor/src/perspectives/model_query.rsrust-executor/src/perspectives/perspective_instance.rstests/js/tests/model/model-prolog.test.tstests/js/tests/prolog-and-literals.test.ts
💤 Files with no reviewable changes (12)
- ad4m-hooks/helpers/src/factory/SubjectRepository.ts
- core/src/model/index.ts
- tests/js/tests/model/model-prolog.test.ts
- core/src/model/subscription-delta.test.ts
- core/src/model/query-sparql-batch.ts
- core/src/model/hydration-batching.test.ts
- core/src/model/Subject.ts
- core/src/model/subscription-pool.ts
- core/src/model/hydration-batch.ts
- core/src/model/query-prolog.ts
- core/src/model/query-sparql.ts
- core/src/model/prolog-facts.ts
Comment on lines
+1468
to
+1470
| // Return the expression address directly — callers should use Ad4mModel or | ||
| // getSubjectData() to interact with the created instance. | ||
| return exprAddr as unknown as B extends undefined ? T : string; |
Contributor
There was a problem hiding this comment.
createSubject() now violates its own return contract.
This method still promises T for non-batch calls, but Lines 1468-1470 always return the raw expression address. ad4m-hooks/helpers/src/factory/SubjectRepository.ts:42-72 still does setProperties(newInstance, data) and getSubjectData(newInstance) on that return value, so this becomes a runtime break instead of a typed migration. Either keep returning a proxy-compatible object here or migrate the remaining callers and narrow the public type to Promise<string>.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/perspectives/PerspectiveProxy.ts` around lines 1468 - 1470,
createSubject() currently returns the raw expression address (exprAddr) for
non-batch calls which breaks callers expecting a proxy/object of type T (e.g.,
code that calls setProperties(newInstance, data) and
getSubjectData(newInstance)); change createSubject() so that when B is undefined
(non-batch) it returns a proxy-compatible object instance instead of just
exprAddr — for example, after creating the expression use the same proxy
construction path you use elsewhere (via Ad4mModel or the existing subject
factory) to build and return the T instance that wraps exprAddr, or
alternatively if you intend to return raw addresses change the public signature
to Promise<string> and update callers; focus edits on the createSubject() return
path and the code that currently returns exprAddr so callers like
getSubjectData() work without runtime errors.
Comment on lines
+1995
to
+2003
| // Parse sh:in values if present | ||
| let options: Array<{ value: string; label?: string }> | undefined; | ||
| if (row.shIn) { | ||
| try { | ||
| let raw = row.shIn; | ||
| // Strip literal: prefix if present | ||
| if (raw.startsWith('literal:string:')) raw = raw.substring('literal:string:'.length); | ||
| options = JSON.parse(raw); | ||
| } catch { /* ignore parse errors */ } |
Contributor
There was a problem hiding this comment.
Encode and decode sh:in payloads consistently.
addNamedOption() writes raw JSON into a literal:string: URI, while getClassShape() reads it back without URI-decoding. Labels or values containing spaces, quotes, or % will either produce an invalid literal URI or fail to round-trip cleanly. Use the normal literal encoder on write and decode before JSON.parse() on read.
Suggested fix
if (row.shIn) {
try {
let raw = row.shIn;
- // Strip literal: prefix if present
- if (raw.startsWith('literal:string:')) raw = raw.substring('literal:string:'.length);
+ if (raw.startsWith('literal:string:')) {
+ raw = decodeURIComponent(raw.substring('literal:string:'.length));
+ }
options = JSON.parse(raw);
} catch { /* ignore parse errors */ }
} await this.add(new Link({
source: propShapeId,
predicate: "sh://in",
- target: `literal:string:${JSON.stringify(updated)}`
+ target: Literal.from(JSON.stringify(updated)).toUrl()
}));Also applies to: 2103-2108
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/perspectives/PerspectiveProxy.ts` around lines 1995 - 2003, The
sh:in payload is being read from a literal:string: URI without URI-decoding,
which breaks values with spaces/quotes/% and won't round-trip with
addNamedOption that writes encoded literals; in getClassShape (in
PerspectiveProxy.ts) update the parsing logic for row.shIn to first strip the
literal:string: prefix, then URI-decode the remaining string (e.g.,
decodeURIComponent or equivalent) before calling JSON.parse, and repeat the same
fix in the other parsing location noted (around the block at ~2103-2108); ensure
you still swallow parse errors but only after decoding and parsing.
Comment on lines
+2027
to
+2049
| async getInstanceClasses(baseExpression: string): Promise<string[]> { | ||
| const safeId = baseExpression.replace(/['"\\]/g, ''); | ||
| // Get all registered classes and their required conformance properties | ||
| const results = await this.querySparql( | ||
| `SELECT DISTINCT ?class WHERE { | ||
| ?class <rdf://type> <ad4m://SubjectClass> . | ||
| ?class <ad4m://shape> ?shape . | ||
| ?shape <sh://property> ?propShape . | ||
| ?propShape <sh://minCount> ?mc . | ||
| FILTER(?mc >= 1) | ||
| ?propShape <sh://path> ?path . | ||
| <${safeId}> ?path ?_val . | ||
| }` | ||
| ); | ||
| if (!Array.isArray(results)) return []; | ||
| return results.map((row: any) => { | ||
| const uri: string = row.class || ''; | ||
| const hashIdx = uri.lastIndexOf('#'); | ||
| if (hashIdx >= 0) return uri.substring(hashIdx + 1); | ||
| const slashIdx = uri.lastIndexOf('/'); | ||
| if (slashIdx >= 0) return uri.substring(slashIdx + 1); | ||
| return uri; | ||
| }).filter(Boolean); |
Contributor
There was a problem hiding this comment.
getInstanceClasses() matches classes on any one required property.
This query only checks whether the instance has a sh:minCount >= 1 path from the shape. It does not require all required triples, and it ignores fixed-target constructor constraints, so classes that share a single required predicate will be reported incorrectly. Reuse the same requiredTriples semantics as isSubjectInstance() / findInstancesByMetadata() here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/perspectives/PerspectiveProxy.ts` around lines 2027 - 2049,
getInstanceClasses currently returns any class that matches a single required
sh:minCount path; change it to require all required triples and honor
fixed-target constructor constraints by reusing the same requiredTriples
semantics used in isSubjectInstance() and findInstancesByMetadata().
Specifically, in getInstanceClasses(baseExpression) locate or reuse the helper
logic that builds requiredTriples (paths + any fixed target checks) from a
class's shape, then change the SPARQL generation to require every required path
(e.g., via multiple EXISTS/VALUES or a GROUP BY HAVING count equal to
requiredTriples.length) and include checks for fixed-target constructor values
where present so a class is only returned when all its required
predicates/values are satisfied for <baseExpression>. Update getInstanceClasses
to call or mirror the same helper used by
isSubjectInstance()/findInstancesByMetadata() to ensure consistent semantics.
Comment on lines
+261
to
+268
| impl ModelShape { | ||
| /// Returns all predicate IRIs declared in this shape (properties + relations + flags). | ||
| pub fn predicates(&self) -> Vec<String> { | ||
| self.properties | ||
| .iter() | ||
| .map(|p| p.predicate.clone()) | ||
| .collect() | ||
| } |
Contributor
There was a problem hiding this comment.
predicates() drops getter-only subscription dependencies.
parse_shape_from_json() now accepts relations with a getter but no predicate at Line 744. This helper still just clones p.predicate, so those relations contribute "" and the trigger builder cannot watch the links the getter actually depends on. modelSubscribe() will stay stale for read-only getter-backed relations unless dependency extraction is added here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-executor/src/perspectives/model_query.rs` around lines 261 - 268, The
predicates() helper currently only clones property predicates and thus drops
getter-only relations; update ModelShape::predicates to include predicates from
properties, relations and flags, and for relations where predicate is empty but
a getter exists, extract and include the getter's declared dependency predicates
(e.g., from relation.getter.dependencies or relation.getter.deps) so
modelSubscribe() receives the actual link dependencies; ensure you reference
ModelShape::properties, ModelShape::relations, ModelShape::flags and the
relation.getter dependency field when collecting predicates.
Comment on lines
+1726
to
+1744
| } else { | ||
| // Property getter → scalar value (first binding's first value) | ||
| if let Some(first_row) = results.first() { | ||
| if let Some(obj_row) = first_row.as_object() { | ||
| if let Some(first_val) = obj_row.values().next() { | ||
| let val = first_val | ||
| .get("value") | ||
| .and_then(|s| s.as_str()) | ||
| .or_else(|| first_val.as_str()); | ||
| if let Some(s) = val { | ||
| if !s.is_empty() && s != "None" { | ||
| if let Some(obj) = instance.as_object_mut() { | ||
| // Property getters return raw values | ||
| // (e.g. literal URLs). Don't decode them | ||
| // — the TS side handles resolution if needed. | ||
| obj.insert( | ||
| prop.name.clone(), | ||
| Value::String(s.to_string()), | ||
| ); |
Contributor
There was a problem hiding this comment.
Normalize getter-backed property values before filtering and sorting.
This branch stores the first SELECT binding as a raw string. If a getter returns literal:number:*, literal:boolean:*, or an ISO timestamp, matches_where() and sort_instances() will see the wrong type because getter evaluation happens before those steps. Getter-backed properties need the same literal decoding as hydrated properties.
Suggested fix
if let Some(obj) = instance.as_object_mut() {
- // Property getters return raw values
- // (e.g. literal URLs). Don't decode them
- // — the TS side handles resolution if needed.
obj.insert(
prop.name.clone(),
- Value::String(s.to_string()),
+ parse_literal_value(s),
);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-executor/src/perspectives/model_query.rs` around lines 1726 - 1744, The
getter branch stores the first binding as a raw string (prop.name insertion)
which causes type mismatches for matches_where() and sort_instances(); update
the branch that handles first_val → val to decode literal strings into proper
JSON Value types the same way hydrated properties are decoded (use the same
literal-decoding logic/function used for hydrated properties) before inserting
into instance (instead of always inserting Value::String). Ensure the decoded
Value is inserted under prop.name so downstream matches_where() and
sort_instances() operate on correct types.
- ModelQueryBuilder: combine count+paginate into single query with count:true in paginateSubscribe (was 2 calls per refresh) - ModelQueryBuilder: add isInit guard in paginateSubscribe callback (consistent with subscribe/countSubscribe) - PerspectiveProxy: URI-decode sh:in payload in getClassShape for robustness with special characters - SHACLShape: restore sh:in values in fromLinks() so enum metadata round-trips through save/load cycles - model_query.rs: remove dangerous global 'Base' replacement that could mangle getter SPARQL containing 'Base' substrings - model_query.rs: extend predicates() to include relation predicates (not just properties) for subscription trigger matching - perspective_instance.rs: refresh trigger query/predicates when reusing existing subscription (not just last_result) - perspective_instance.rs: always merge store-backed shape predicates instead of fallback-only, so partial shapes get full trigger coverage Intentionally not changed: - createSubject return type: deliberate migration to return address - getInstanceClasses: would need complex GROUP BY/HAVING SPARQL - getter value normalization: intentional raw-value design per comment
- SHACLShape: sh:in round-trip via toLinks/fromLinks (5 tests) - ModelQueryBuilder: paginateSubscribe single query + isInit guard (3 tests) - PerspectiveClient: getClassShape URI-decode for sh:in values (6 tests)
HexaField
changed the title
jhweir
force-pushed
the
feat/sparql-1.2-cleanup
branch
from
b8956ee to
633fa59
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/src/perspectives/PerspectiveProxy.ts (2)
1794-1799:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMatch rows to the exact shape URI, not a suffix.
endsWith(\${className}Shape`)aliases suffix-related classes, so rows fromMyRecipeShapealso get attached toRecipe. That can makefindClassByProperties()` resolve the wrong class when names share a suffix.Suggested fix
for (const r of results) { if (r.predicate === 'sh://property' || r.predicate === 'sh://collection') { - // Match shape source to class (e.g., "recipe://RecipeShape" -> "Recipe") + // Match the exact shape URI discovered in the first pass for (const [className, shape] of classShapes) { - if (r.shape.endsWith(`${className}Shape`)) { + if (r.shape === shape.shapeUri) { const dotIdx = r.target.lastIndexOf('.'); if (dotIdx < 0) continue; const name = r.target.substring(dotIdx + 1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/perspectives/PerspectiveProxy.ts` around lines 1794 - 1799, The loop in PerspectiveProxy.ts currently matches shapes using r.shape.endsWith(`${className}Shape`), which can incorrectly alias similarly suffixed classes; change the match to compare the full/unique shape identifier instead (e.g., replace the endsWith check with an exact equality against the shape object from classShapes, such as r.shape === shape.uri or r.shape === shape.id), so rows are attached only when the shape URI exactly matches the class shape and avoid wrong resolutions in findClassByProperties().
1473-1484:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop deriving these return types from the
subjectClassinput.When calling
getSubjectData("Recipe", id), the generic is inferred asT=string, yielding a return type ofPromise<string>. However, both code paths returnJSON.parse(...), which always returns an object. The same issue affectsgetAllSubjectInstances("Recipe"), which is typed asPromise<string[]>but returns an array of hydrated objects. Add function overloads to match actual behavior:async getSubjectData(subjectClass: string, exprAddr: string): Promise<any>; async getSubjectData<T extends object>(subjectClass: T, exprAddr: string): Promise<T>;This restores type safety for both calling patterns and applies to
getAllSubjectInstancesas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/perspectives/PerspectiveProxy.ts` around lines 1473 - 1484, The return types are incorrectly inferred from the subjectClass parameter (e.g., calling getSubjectData("Recipe", id) makes T=string), so add explicit overloads for getSubjectData and getAllSubjectInstances: one overload accepting subjectClass: string that returns Promise<any> (or Promise<any[]> for getAllSubjectInstances) and a generic overload accepting subjectClass: T extends object that returns Promise<T> (or Promise<T[]>), then keep the existing implementation body as the implementation signature; this ensures the string-class call returns parsed object(s) and object-class call preserves<T> typing without deriving T from a string input.
♻️ Duplicate comments (3)
core/src/perspectives/PerspectiveProxy.ts (3)
2029-2051:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRequire full class conformance in
getInstanceClasses().This query returns a class as soon as the instance has one
sh:minCount >= 1path. Classes that share a single required predicate will be reported even when the rest of the required paths, or any fixed-target constructor triples, do not match. ReusegetSubjectClassMetadata()/requiredTripleshere so conformance stays aligned withisSubjectInstance()andfindInstancesByMetadata().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/perspectives/PerspectiveProxy.ts` around lines 2029 - 2051, The current getInstanceClasses() only checks one required predicate and can falsely include classes; instead, query candidate classes as before but then for each candidate class URI call getSubjectClassMetadata(classUri) and use the returned requiredTriples (and any constructor/fixed-target info) to verify full conformance for the given baseExpression (safeId); only include the class when all requiredTriples (and constructor triples) are present—you can reuse isSubjectInstance() or run the same triple-existence checks that isSubjectInstance()/findInstancesByMetadata() use so conformance logic stays aligned with those functions.
2105-2110:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSerialize
sh:inthroughLiteralinstead of embedding raw JSON.This writes unescaped JSON into a
literal:string:URL. Any option containing spaces, quotes,%, or non-ASCII characters can produce an invalid literal URI or fail to round-trip throughgetClassShape().Suggested fix
await this.add(new Link({ source: propShapeId, predicate: "sh://in", - target: `literal:string:${JSON.stringify(updated)}` + target: Literal.from(JSON.stringify(updated)).toUrl() }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/perspectives/PerspectiveProxy.ts` around lines 2105 - 2110, The code adds an sh:in link by embedding raw JSON into a literal:string: URI which can produce invalid URIs; instead serialize the options through the project's Literal serializer. Replace the target construction inside the add(new Link(...)) call so it uses the existing Literal utility/class (e.g. construct a Literal from updated and call its URI/serialization method) rather than JSON.stringify; keep the same predicate "sh://in" and the same Link/add usage so getClassShape() round-trips correctly.
1424-1470:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftFix
createSubject()'s public return contract.Non-batch calls now always return
exprAddr, butPromise<B extends undefined ? T : string>still advertises an instance whenbatchIdis omitted. That turns this migration into a runtime break for typed callers that still expect a hydrated/proxy object. Either rebuild and return the instance here or narrow the public API toPromise<string>and migrate callers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/perspectives/PerspectiveProxy.ts` around lines 1424 - 1470, The current createSubject (method createSubject) advertises returning a hydrated instance (Promise<B extends undefined ? T : string>) but always returns exprAddr, breaking callers that expect a proxy; either change the public signature to Promise<string> or actually build and return the subject proxy when batchId is undefined. To fix: if you choose to return a proxy, after calling `#client.createSubject` use the existing lookup/creation helpers (e.g. findClassByProperties and whatever routine creates subject proxies / getSubjectData or Ad4mModel’s proxy factory) to construct and return the hydrated proxy for exprAddr when batchId is not set; otherwise update the method signature and all callers to return/expect string and keep returning exprAddr (ensure error message and batch handling with `#handle.uuid` and `#client.createSubject` remain unchanged).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rust-executor/src/perspectives/perspective_instance.rs`:
- Around line 229-233: The variable-predicate regex (var_pred) in
perspective_instance.rs currently requires a trailing dot so it misses triples
that end before a closing brace; update the regex used to detect variable
predicates (the var_pred Regex::new(...) expression) so it also matches when the
triple ends without a dot by allowing either a trailing dot OR end-of-statement
markers such as end-of-string or a closing brace/parenthesis; keep the rest of
the logic (returning an empty HashSet when is_match(query) is true) unchanged.
---
Outside diff comments:
In `@core/src/perspectives/PerspectiveProxy.ts`:
- Around line 1794-1799: The loop in PerspectiveProxy.ts currently matches
shapes using r.shape.endsWith(`${className}Shape`), which can incorrectly alias
similarly suffixed classes; change the match to compare the full/unique shape
identifier instead (e.g., replace the endsWith check with an exact equality
against the shape object from classShapes, such as r.shape === shape.uri or
r.shape === shape.id), so rows are attached only when the shape URI exactly
matches the class shape and avoid wrong resolutions in findClassByProperties().
- Around line 1473-1484: The return types are incorrectly inferred from the
subjectClass parameter (e.g., calling getSubjectData("Recipe", id) makes
T=string), so add explicit overloads for getSubjectData and
getAllSubjectInstances: one overload accepting subjectClass: string that returns
Promise<any> (or Promise<any[]> for getAllSubjectInstances) and a generic
overload accepting subjectClass: T extends object that returns Promise<T> (or
Promise<T[]>), then keep the existing implementation body as the implementation
signature; this ensures the string-class call returns parsed object(s) and
object-class call preserves<T> typing without deriving T from a string input.
---
Duplicate comments:
In `@core/src/perspectives/PerspectiveProxy.ts`:
- Around line 2029-2051: The current getInstanceClasses() only checks one
required predicate and can falsely include classes; instead, query candidate
classes as before but then for each candidate class URI call
getSubjectClassMetadata(classUri) and use the returned requiredTriples (and any
constructor/fixed-target info) to verify full conformance for the given
baseExpression (safeId); only include the class when all requiredTriples (and
constructor triples) are present—you can reuse isSubjectInstance() or run the
same triple-existence checks that isSubjectInstance()/findInstancesByMetadata()
use so conformance logic stays aligned with those functions.
- Around line 2105-2110: The code adds an sh:in link by embedding raw JSON into
a literal:string: URI which can produce invalid URIs; instead serialize the
options through the project's Literal serializer. Replace the target
construction inside the add(new Link(...)) call so it uses the existing Literal
utility/class (e.g. construct a Literal from updated and call its
URI/serialization method) rather than JSON.stringify; keep the same predicate
"sh://in" and the same Link/add usage so getClassShape() round-trips correctly.
- Around line 1424-1470: The current createSubject (method createSubject)
advertises returning a hydrated instance (Promise<B extends undefined ? T :
string>) but always returns exprAddr, breaking callers that expect a proxy;
either change the public signature to Promise<string> or actually build and
return the subject proxy when batchId is undefined. To fix: if you choose to
return a proxy, after calling `#client.createSubject` use the existing
lookup/creation helpers (e.g. findClassByProperties and whatever routine creates
subject proxies / getSubjectData or Ad4mModel’s proxy factory) to construct and
return the hydrated proxy for exprAddr when batchId is not set; otherwise update
the method signature and all callers to return/expect string and keep returning
exprAddr (ensure error message and batch handling with `#handle.uuid` and
`#client.createSubject` remain unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e7e64b8a-230e-43e4-9c37-a450cb2196b5
📒 Files selected for processing (10)
core/src/model/Ad4mModel.test.tscore/src/model/Ad4mModel.tscore/src/model/ModelQueryBuilder.tscore/src/model/types.tscore/src/perspectives/PerspectiveClient.test.tscore/src/perspectives/PerspectiveProxy.tscore/src/shacl/SHACLShape.test.tscore/src/shacl/SHACLShape.tsrust-executor/src/perspectives/model_query.rsrust-executor/src/perspectives/perspective_instance.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/model/ModelQueryBuilder.ts
- core/src/model/Ad4mModel.ts
Comment on lines
+229
to
+233
| let var_pred = | ||
| regex::Regex::new(r"(?:\?\w+|<[^>]+>)\s+\?\w+\s+(?:\?\w+|<[^>]+>)\s*\.").unwrap(); | ||
| if var_pred.is_match(query) { | ||
| return HashSet::new(); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
import re
old = re.compile(r"(?:\?\w+|<[^>]+>)\s+\?\w+\s+(?:\?\w+|<[^>]+>)\s*\.")
new = re.compile(r"(?:\?\w+|<[^>]+>)\s+\?\w+\s+(?:\?\w+|<[^>]+>)\s*(?:\.|\}|$)")
query = "SELECT ?s WHERE { ?s <test://fixed> ?o1 . ?s ?p ?o2 }"
print("old_matches =", bool(old.search(query))) # expected: False
print("new_matches =", bool(new.search(query))) # expected: True
PYRepository: coasys/ad4m
Length of output: 94
🏁 Script executed:
cat -n rust-executor/src/perspectives/perspective_instance.rs | sed -n '225,240p'Repository: coasys/ad4m
Length of output: 894
🏁 Script executed:
# Search for usage of var_pred to understand the impact
rg "var_pred" rust-executor/src/perspectives/perspective_instance.rs -A 5 -B 2Repository: coasys/ad4m
Length of output: 895
🏁 Script executed:
# Look for tests related to this regex or function
fd -t f "\.rs$" rust-executor/src/perspectives/ | xargs grep -l "var_pred\|variable.*predicate" | head -5Repository: coasys/ad4m
Length of output: 110
🏁 Script executed:
# Search for tests related to this function or predicate detection
rg -i "test.*predicate\|predicate.*test" rust-executor/src/perspectives/perspective_instance.rs -A 3 -B 1 | head -50Repository: coasys/ad4m
Length of output: 37
🏁 Script executed:
# Look for test file for this module
fd -t f "*.rs" rust-executor | xargs grep -l "perspective_instance\|extract.*predicate" | grep testRepository: coasys/ad4m
Length of output: 339
🏁 Script executed:
# Search for SPARQL test queries to see example patterns
rg "SELECT.*WHERE.*\{" rust-executor/src/perspectives/perspective_instance.rs -A 1 | head -30Repository: coasys/ad4m
Length of output: 944
🏁 Script executed:
# Look for the full context of the extract_predicates_from_sparql function
rg -B 20 "let var_pred" rust-executor/src/perspectives/perspective_instance.rs | head -40Repository: coasys/ad4m
Length of output: 1059
🏁 Script executed:
# Search for test cases that specifically test variable predicates
rg "extract_predicates.*\?" rust-executor/src/perspectives/perspective_instance.rs -A 2 | head -50Repository: coasys/ad4m
Length of output: 196
🏁 Script executed:
# Check if there are any test cases without trailing dots
rg "WHERE.*\{.*\?" rust-executor/src/perspectives/perspective_instance.rs | grep -v "\.$" | head -10Repository: coasys/ad4m
Length of output: 470
🏁 Script executed:
# Get the full test function to understand the context
rg -B 5 "extract_predicates_from_sparql.*\?" rust-executor/src/perspectives/perspective_instance.rs -A 8 | head -80Repository: coasys/ad4m
Length of output: 572
🏁 Script executed:
# Check if there are integration tests or other test files
find rust-executor -type f -name "*.rs" | xargs grep -l "extract_predicates_from_sparql"Repository: coasys/ad4m
Length of output: 110
🏁 Script executed:
# Verify the specific test case without trailing dot
python3 - <<'EOF'
import re
# Current regex with trailing dot requirement
current = re.compile(r"(?:\?\w+|<[^>]+>)\s+\?\w+\s+(?:\?\w+|<[^>]+>)\s*\.")
# Proposed fix with optional dot/brace/end
proposed = re.compile(r"(?:\?\w+|<[^>]+>)\s+\?\w+\s+(?:\?\w+|<[^>]+>)\s*(?:\.|\}|$)")
test_query = "SELECT ?s ?p ?o WHERE { ?s ?p ?o }"
print("Test query:", test_query)
print("Current regex matches:", bool(current.search(test_query)))
print("Proposed regex matches:", bool(proposed.search(test_query)))
print()
# Test with trailing dot (should match both)
test_with_dot = "SELECT ?s ?p ?o WHERE { ?s ?p ?o . }"
print("Query with dot:", test_with_dot)
print("Current regex matches:", bool(current.search(test_with_dot)))
print("Proposed regex matches:", bool(proposed.search(test_with_dot)))
EOFRepository: coasys/ad4m
Length of output: 271
🏁 Script executed:
# Run the actual test to see if it passes or fails
cd rust-executor && cargo test test_extract_predicates_variable_predicate -- --nocapture 2>&1 | head -100Repository: coasys/ad4m
Length of output: 4740
🏁 Script executed:
# Let me also check the complete test function and surrounding context to understand the logic
rg -A 15 "fn test_extract_predicates_variable_predicate" rust-executor/src/perspectives/perspective_instance.rsRepository: coasys/ad4m
Length of output: 722
🏁 Script executed:
# Verify the actual control flow - what happens when var_pred.is_match returns false?
rg -A 10 "if var_pred.is_match" rust-executor/src/perspectives/perspective_instance.rsRepository: coasys/ad4m
Length of output: 512
🏁 Script executed:
# Check if there are any real queries in the codebase that use variable predicates without trailing dots
rg "WHERE.*\{\s*\?.*\?" rust-executor/src/perspectives/perspective_instance.rs | grep -v "\.$" | head -20Repository: coasys/ad4m
Length of output: 400
🏁 Script executed:
# Double-check: verify the second regex to ensure fixed predicates are correctly extracted
python3 - <<'EOF'
import re
# The second regex for fixed predicates
fixed_pred = re.compile(r"(?:\?\w+|<[^>]+>)\s+(<[^>]+>)\s+(?:\?\w+|<[^>]+>)")
# Test query WITH variable predicate but no dot
var_pred_no_dot = "SELECT ?s ?p ?o WHERE { ?s ?p ?o }"
print("Query with var predicate (no dot):", var_pred_no_dot)
print("Fixed predicate regex matches:", bool(fixed_pred.search(var_pred_no_dot)))
print()
# Test query with mixed: var predicate + fixed predicate
mixed = "SELECT ?s ?p ?o WHERE { ?s ?p ?o <test://fixed> ?x ?y }"
print("Mixed query:", mixed)
print("Fixed predicate regex matches:", bool(fixed_pred.search(mixed)))
print()
# Query like in test_extract_predicates_variable_predicate
test_query = "SELECT ?s ?p ?o WHERE { ?s ?p ?o }"
print("Test query:", test_query)
print("Fixed predicate regex matches:", bool(fixed_pred.search(test_query)))
EOFRepository: coasys/ad4m
Length of output: 353
🏁 Script executed:
# Check the full flow of logic in extract_predicates_from_sparql to understand impact
rg -A 30 "fn extract_predicates_from_sparql" rust-executor/src/perspectives/perspective_instance.rs | head -50Repository: coasys/ad4m
Length of output: 1330
Variable-predicate detection misses valid SPARQL when the triple has no trailing dot.
Line 230 only matches ?s ?p ?o . with a trailing dot. If a variable-predicate triple ends before } (without dot), the pattern won't match. This causes incorrect behavior in mixed queries: a query with both a variable predicate (no dot) and fixed predicates would incorrectly narrow to the fixed predicates instead of forcing a full re-check.
♻️ Suggested fix
- let var_pred =
- regex::Regex::new(r"(?:\?\w+|<[^>]+>)\s+\?\w+\s+(?:\?\w+|<[^>]+>)\s*\.").unwrap();
+ let var_pred = regex::Regex::new(
+ r"(?:\?\w+|<[^>]+>)\s+\?\w+\s+(?:\?\w+|<[^>]+>)\s*(?:\.|\}|$)"
+ ).unwrap();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let var_pred = | |
| regex::Regex::new(r"(?:\?\w+|<[^>]+>)\s+\?\w+\s+(?:\?\w+|<[^>]+>)\s*\.").unwrap(); | |
| if var_pred.is_match(query) { | |
| return HashSet::new(); | |
| } | |
| let var_pred = regex::Regex::new( | |
| r"(?:\?\w+|<[^>]+>)\s+\?\w+\s+(?:\?\w+|<[^>]+>)\s*(?:\.|\}|$)" | |
| ).unwrap(); | |
| if var_pred.is_match(query) { | |
| return HashSet::new(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rust-executor/src/perspectives/perspective_instance.rs` around lines 229 -
233, The variable-predicate regex (var_pred) in perspective_instance.rs
currently requires a trailing dot so it misses triples that end before a closing
brace; update the regex used to detect variable predicates (the var_pred
Regex::new(...) expression) so it also matches when the triple ends without a
dot by allowing either a trailing dot OR end-of-statement markers such as
end-of-string or a closing brace/parenthesis; keep the rest of the logic
(returning an empty HashSet when is_match(query) is true) unchanged.
# Conflicts: # benchmarks/.gitignore # benchmarks/README.md # benchmarks/package.json # benchmarks/src/client.ts # benchmarks/src/utils.ts # benchmarks/tsconfig.json
…or_actions fallback Creates a fully functional Ad4mModel subclass from a SHACLShape at runtime, without needing the original TypeScript class definition. - Scalar properties (maxCount=1) registered via setPropertyRegistryEntry - Collections (no maxCount or >1) registered as hasMany via setRelationRegistryEntry - Flag properties (sh:hasValue) registered as type-discrimination entries so the SPARQL query builder emits the correct fixed-value triple - Backward-compat: old Flux shapes that predate sh:hasValue persistence carry the flag value in constructor_actions; this is recovered via a fallback map - Properties without a name field are skipped - Model decorator applied so generateSHACL/generateSDNA work on the dynamic class 13 new unit tests added, all 135 Ad4mModel tests pass.
…fixed include keys - Add IncludeProjection interface and isIncludeProjection guard to types.ts; update IncludeMap union to include IncludeProjection - In executeModelQuery, split $-prefixed keys out of queryInput.include into queryInput.projections; enrich with targetShape via relMeta.target() when the relation declares a target class - $ prefix is the required discriminator — no silent value-type dispatch - Add ProjectionInput struct and projections field to Rust ModelQueryInput - Implement resolve_projections() using grouped VALUES SPARQL (O(P) queries, not O(N×P)); per-parent truncation in Rust for correct limit semantics - Add build_projection_where_patterns() for target-property where filtering - Add build_projection_order_clause() with correct single ORDER BY keyword - Call resolve_projections after instance finalisation in execute_model_query_inner - Fix: global LIMIT removed from list-projection SPARQL (was capping total rows, not per-parent rows); truncation now applied per-parent in Rust - Fix: ORDER BY keyword emitted once, not per term - 10 new TS tests in Ad4mModel.test.ts; 5 new Rust unit tests in model_query.rs
- Add `includeAll?: boolean` to the Query type in types.ts. When true,
executeModelQuery expands it to a full IncludeMap covering every declared
forward relation before building queryInput — zero Rust changes required.
Explicit `include` entries take precedence over the expansion.
- HasMany: emit console.warn when `through` is omitted and no `getter`
is present, naming the class and field. Behaviour is otherwise unchanged
(still defaults to ad4m://has_child). Add { through: 'ad4m://has_child' }
to silence the warning, or use a semantic predicate for targeted queries.
# Conflicts: # core/src/model/Ad4mModel.test.ts
HexaField
force-pushed
the
feat/sparql-1.2-cleanup
branch
from
a2b36f0 to
21b4594
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
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.
Summary
This PR combines the full SPARQL 1.2 migration for the AD4M SDK and executor — originally split across
feat/sparql-1.2(#801, now closed) andfeat/sparql-1.2-cleanup(#803). Together they deliver: Oxigraph 0.5.7 with RDF 1.2 reifier storage, a complete Rust-side model query engine replacing the JS pipeline, SHACLsh:insupport, server-side model subscriptions, and the elimination of all Prolog code andeval()calls from the SDK.Companion: Flux coasys/flux#591
Part 1: Oxigraph 0.5.7 + RDF 1.2 Reifier Storage + Rust Model Query Engine
(Originally #801)
45 files changed, +5,311 / −5,312 lines
Oxigraph 0.5.7 + RDF 1.2 Reifier Storage
Rust Executor
rdf-12feature:SparqlEvaluatorAPI,NamedOrBlankNodetype changesmigrate_named_graphs_to_reifiers()) — idempotent, runs on startupsparql_storeunit tests passSDK Query Generation
buildSPARQLQuery()— generates?_reifier rdf:reifies <<( ?source ?predicate ?target )>>instead ofGRAPH ?linkGraph { ... }query-sparql-batch.ts— batch include queries use reifier patternsPREFIX rdf: <http://www.w3.org/1999/02/22-rdf-syntax-ns#>Other SPARQL / SDK Fixes
ORDER BYfrom SPARQL pagination — eliminates unnecessary sort overhead when the caller doesn't need orderingsubscribeQueryroutesPREFIX/BASEqueries correctlyWeakRef+ proper cache invalidationdeepQueryopt-in for collectionsRust-Side Model Query Endpoint
New:
model_query.rs(~2,100 lines)Replaces the TS pipeline (SPARQL build → GraphQL round-trip → JS hydrate → JS filter/sort/paginate) with a single
perspectiveModelQueryGraphQL endpoint:parse_literal_value()extracts data from signed JSON envelopes, yielding proper strings/numbers/booleansequals,notEquals,gt,gte,lt,lte,like,in,notIn,betweenSELECT COUNT(DISTINCT ?source)when all where conditions are SPARQL-pushabledeserialize_order_flexhandles both[[\"k\",\"v\"]](tuple) and{\"k\":\"v\"}(object) order formatsTypeScript SDK
Ad4mModel.ts:executeModelQuery()bridges TS → Rust;enrichShapeForIncludes()walks include tree with target class shapes;jsonToModelInstance()recursively constructs class instances from hydrated JSONModelQueryBuilder.ts: RoutesexecuteSparqlQuery(),count(),paginate()throughexecuteModelQuery()PerspectiveClient.ts/PerspectiveProxy.ts: NewmodelQuery()methodkind,maxCount,directionPart 2: JS Query Pipeline Elimination + SHACL sh:in + Server-Side Subscriptions
(Originally #803)
24 files changed, 1,141 insertions, 4,528 deletions (net −3,387 lines)
After this part, the SDK has zero Prolog code, zero JS-side query processing, and zero
eval()calls. All model queries and subscriptions route through Rust. SHACL shapes now support enum constraints (sh:in) with new PerspectiveProxy APIs for runtime class introspection.JS Query Pipeline → Rust
Ad4mModel.getData()callsexecuteModelQuery()instead of building SPARQL client-side and hydrating in JSsubscribe,countSubscribe,paginateSubscribe) useexecuteModelQuery()instead ofprocessSparqlResult()→groupSPARQLResults()→instancesFromQueryResult()@Property({ getter })and relation conformance getters) moved tomodel_query.rs— eliminates N ×querySparqlround-trips per instanceDeleted Files (−1,806 lines)
Subject.tseval()security riskquery-prolog.tsprolog-facts.tshydration-batch.tshydration-batching.test.tssubscription-pool.tsperspectiveModelSubscribequery-sparql-batch.tssubscription-delta.test.tsDead Code Removed from Existing Files
Ad4mModel.ts:instancesFromQueryResult()(~285 lines),assignValuesToInstance(),countQueryToSPARQL(),get timestamp(),get baseExpression()— total −402 linesAd4mModel.test.ts: −649 lines of tests for deleted methodsModelQueryBuilder.ts: Prolog branches,processSparqlResult(), JS subscription pipeline all removedhydration.ts: 907 → 304 linesquery-sparql.ts: 637 → 510 linesindex.ts: 4 barrel exports removedSHACL
sh:in(Enum/Named Options)SHACLPropertyShape.in:Array<{ value: string; label?: string }>— stored as<propShape> <sh://in> literal:string:[...]@Property({ options: [...] })decorator field maps tosh:inbuildSHACL()emitssh:infrom decorator metadataNew PerspectiveProxy APIs
listRegisteredClasses()getClassShape(className)sh:inoptionsgetInstanceClasses(baseExpression)getNamedOptions(className)sh:invalues grouped by propertyaddNamedOption(className, prop, value, label?)sh:inoptionmodelSubscribe(className, queryJson, shapeJson?)Subject.ts Removal
Subject.ts(~150 lines) — Prolog-based dynamic proxy witheval()security riskcreateSubject()returns expression address directlyDead/Deprecated PerspectiveProxy Method Cleanup
getSubjectProxy()getAllSubjectProxies()getPropertyValueViaSparql()getRelationValuesViaSparql()batchCheckSubjectInstances()filterInstancesSequential()getSubjectClassMetadataFromSDNA()getSubjectClassMetadata()PerspectiveProxy SPARQL Optimization
isSubjectInstance()findInstancesByMetadata()findClassByProperties()Server-Side Model Subscriptions
New
perspectiveModelSubscribeGraphQL mutation — Rust handles the full subscription lifecycle:className+queryJson+ optionalshapeJsonmodel_query()and pushes results via existingperspectiveQuerySubscriptionchannelModelQueryBuilder.subscribe()/countSubscribe()/paginateSubscribe()rewritten to use this endpointRust Changes
model_query.rs: +193 lines —getterfield onShapeProperty,evaluate_getters(),load_shape_from_store()public API,ModelShape::predicates()perspective_instance.rs: +194 lines —ModelSubscriptionParams,model_subscribe_and_query(),build_model_trigger_predicates(), modifiedcheck_subscribed_queries()mutation_resolvers.rs: +27 lines —perspective_model_subscriberesolverTest Results
sparql_store)model_query)Benchmark Results
All benchmarks against 115,171 links (real Flux channel data), 10 iterations.
Raw SPARQL (Oxigraph 0.5.7 vs 0.4)
Custom Getters (Rust endpoint)
Rust Endpoint vs TS Pipeline
Expected Additional Impact from Pipeline Elimination
isSubjectInstance(): ~5-20× faster — single SPARQL ASK replaces fetching all linksfindClassByProperties(): ~Nx faster (N = number of classes) — single SPARQL UNION replaces N sequential queriesKnown Limitations
Conversation.get(include: subgroups)runs sub-queries sequentially — potential for parallel optimization.Summary by CodeRabbit
Release Notes
New Features
deepQuerymode for evaluating computed properties during collection hydrationBug Fixes
Refactor
Subjectabstraction and legacy Prolog execution layerChores