feat: triple store support for @cap-js/sqlite - #49
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: feat: Triple Store Support for
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
| @@ -0,0 +1,42 @@ | |||
| import SQLiteService from '@cap-js/sqlite' | |||
| import TripleStore from './triplestore.js' | |||
| import cds from '@sap/cds' | |||
| const empty = await cds.run(query) | ||
| expect(empty).property('length').eq(0) | ||
|
|
||
| const res = await cds.run(`CALL SPARQL_EXECUTE('LOAD <${cds.root}${file}.gz> INTO GRAPH <${graph}>','', ?, ?)`) |
![hyperspace-pr-bot[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR introduces several substantive bugs: the factory getter mutates a shared object on every call risking double-wrapping; both SPARQL_EXECUTE and LOAD regex matches are used without null-checking, causing potential destructuring crashes on malformed input; the SPARQL column-name parser is fragile and the extracted names are interpolated into SQL without sanitization (SQL injection risk); and the 'accept: json' string passed to the sparql_table UDF is not a valid media type. The overall feature concept is sound but the implementation needs hardening before it is production-ready.
PR Bot Information
Version: 1.29.18
- File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened - Correlation ID:
6d268ff0-92ac-11f1-88e4-9dde3ffb9494
| get factory() { | ||
| const factory = super.factory | ||
| factory._create = factory.create | ||
| factory.create = async (tenant) => { | ||
| const dbc = await factory._create(tenant) | ||
| dbc.function('sparql_table', { deterministic: true }, (query) => this._tripleStore.query(query, 'accept: json').RESPONSE) | ||
| return dbc | ||
| } | ||
| return factory | ||
| } |
There was a problem hiding this comment.
Bug: Mutating the shared factory object on every get factory call causes race conditions and double-wrapping.
super.factory likely returns the same object reference each time. Every call to this getter overwrites factory._create with whatever factory.create currently is, then wraps it again. On the second call, factory._create becomes the already-wrapped function, so the real original is lost and each connection creation wraps another layer. Store the original once using a guard flag or wrap inside init() instead.
| get factory() { | |
| const factory = super.factory | |
| factory._create = factory.create | |
| factory.create = async (tenant) => { | |
| const dbc = await factory._create(tenant) | |
| dbc.function('sparql_table', { deterministic: true }, (query) => this._tripleStore.query(query, 'accept: json').RESPONSE) | |
| return dbc | |
| } | |
| return factory | |
| } | |
| get factory() { | |
| const factory = super.factory | |
| if (!factory._create) { | |
| factory._create = factory.create | |
| factory.create = async (tenant) => { | |
| const dbc = await factory._create(tenant) | |
| dbc.function('sparql_table', { deterministic: true }, (query) => this._tripleStore.query(query, 'accept: json').RESPONSE) | |
| return dbc | |
| } | |
| } | |
| return factory | |
| } |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| onPlainSQL(req, next) { | ||
| const { query } = req | ||
| if (/^\s*CALL SPARQL_EXECUTE/i.test(query)) { | ||
| const [_, sparql, headers] = /SPARQL_EXECUTE\s*\(\s*'((?:[^']|'')*)'\s*,\s*'((?:[^']|'')*)'\s*,\s*\?\s*,\s*\?\s*\)/.exec(query) |
There was a problem hiding this comment.
Bug: The regex .exec(query) on line 25 can return null if the CALL SPARQL_EXECUTE(...) pattern does not match (e.g. malformed query), causing a destructuring TypeError at runtime.
The outer if only checks for CALL SPARQL_EXECUTE but the inner regex is more restrictive (requires two quoted string arguments). A malformed but matching prefix would crash the handler. The result should be checked before destructuring.
| const [_, sparql, headers] = /SPARQL_EXECUTE\s*\(\s*'((?:[^']|'')*)'\s*,\s*'((?:[^']|'')*)'\s*,\s*\?\s*,\s*\?\s*\)/.exec(query) | |
| const match = /SPARQL_EXECUTE\s*\(\s*'((?:[^']|'')*)'\s*,\s*'((?:[^']|'')*)'\s*,\s*\?\s*,\s*\?\s*\)/.exec(query) | |
| if (!match) return super.onPlainSQL(req, next) | |
| const [_, sparql, headers] = match |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| .replace(/accept:/i, '').trim() // strip HTTP header formatting | ||
|
|
||
| // oxigraph does not support LOAD queries | ||
| if (/^\w*LOAD/i.test(query)) { |
There was a problem hiding this comment.
Bug: The regex /^\w*LOAD/i incorrectly matches queries that are not LOAD statements. \w* matches zero or more word characters, so strings like "SELECT LOAD ..." or "SELECTLOAD" would also match. SPARQL LOAD is always the first keyword; the pattern should anchor to optional whitespace only.
Consider using /^\s*LOAD\b/i to match only actual LOAD queries.
| if (/^\w*LOAD/i.test(query)) { | |
| if (/^\s*LOAD\b/i.test(query)) { |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
|
|
||
| // oxigraph does not support LOAD queries | ||
| if (/^\w*LOAD/i.test(query)) { | ||
| const [_, file, graph] = /LOAD <([^>]*)> INTO GRAPH <([^>]*)>/.exec(query) |
There was a problem hiding this comment.
Bug: .exec(query) can return null for a LOAD query that doesn't match the expected LOAD <uri> INTO GRAPH <uri> pattern (e.g. LOAD <uri> without INTO GRAPH). Destructuring null throws a TypeError.
Should check the result before destructuring.
| const [_, file, graph] = /LOAD <([^>]*)> INTO GRAPH <([^>]*)>/.exec(query) | |
| const match = /LOAD <([^>]*)> INTO GRAPH <([^>]*)>/i.exec(query) | |
| if (!match) throw new Error(`Unsupported LOAD syntax: ${query}`) | |
| const [_, file, graph] = match |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| query(query, headers) { | ||
| this._ready() | ||
|
|
||
| const accept = (headers?.split('\r\n') | ||
| .find(header => /accept:/i.test(header)) ?? 'accept:application/sparql-results+json') | ||
| .replace(/accept:/i, '').trim() // strip HTTP header formatting | ||
|
|
||
| // oxigraph does not support LOAD queries | ||
| if (/^\w*LOAD/i.test(query)) { | ||
| const [_, file, graph] = /LOAD <([^>]*)> INTO GRAPH <([^>]*)>/.exec(query) | ||
| return this.load(file, graph) | ||
| } | ||
| const RESPONSE = super.query(query, { use_default_graph_as_union: true, results_format: accept }) | ||
| return { RESPONSE } | ||
| } |
There was a problem hiding this comment.
Bug: query() can return a Promise (when the query is a LOAD) or a plain object { RESPONSE } (for regular queries), but callers in SQLiteService.js treat the return value as a synchronous object with a .RESPONSE property. In the sparql_table SQLite user function the return of this._tripleStore.query(...) is used directly as .RESPONSE, which will be a Promise object, not the actual result string, causing the SQL function to silently return garbage.
The query method should be made async and the load path should be awaited, or the two code paths should be separated so the SQLite scalar function callback can handle this correctly.
| query(query, headers) { | |
| this._ready() | |
| const accept = (headers?.split('\r\n') | |
| .find(header => /accept:/i.test(header)) ?? 'accept:application/sparql-results+json') | |
| .replace(/accept:/i, '').trim() // strip HTTP header formatting | |
| // oxigraph does not support LOAD queries | |
| if (/^\w*LOAD/i.test(query)) { | |
| const [_, file, graph] = /LOAD <([^>]*)> INTO GRAPH <([^>]*)>/.exec(query) | |
| return this.load(file, graph) | |
| } | |
| const RESPONSE = super.query(query, { use_default_graph_as_union: true, results_format: accept }) | |
| return { RESPONSE } | |
| } | |
| async query(query, headers) { | |
| this._ready() | |
| const accept = (headers?.split('\r\n') | |
| .find(header => /accept:/i.test(header)) ?? 'accept:application/sparql-results+json') | |
| .replace(/accept:/i, '').trim() // strip HTTP header formatting | |
| // oxigraph does not support LOAD queries | |
| if (/^\s*LOAD\b/i.test(query)) { | |
| const match = /LOAD <([^>]*)> INTO GRAPH <([^>]*)>/i.exec(query) | |
| if (!match) throw new Error(`Unsupported LOAD syntax: ${query}`) | |
| const [_, file, graph] = match | |
| return this.load(file, graph) | |
| } | |
| const RESPONSE = super.query(query, { use_default_graph_as_union: true, results_format: accept }) | |
| return { RESPONSE } | |
| } |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| factory._create = factory.create | ||
| factory.create = async (tenant) => { | ||
| const dbc = await factory._create(tenant) | ||
| dbc.function('sparql_table', { deterministic: true }, (query) => this._tripleStore.query(query, 'accept: json').RESPONSE) |
There was a problem hiding this comment.
Bug: The SQLite user-defined function callback is synchronous, but this._tripleStore.query(...) is async (returns a Promise when handling LOAD) and even for regular SELECT queries the .RESPONSE value is accessed directly without awaiting. SQLite's better-sqlite3 (used by @cap-js/sqlite) does not support async user-defined functions; an async callback will silently return undefined to SQLite instead of the query result.
The sparql_table function must only be used for SELECT-type SPARQL queries (not LOAD), and the synchronous super.query() path must be kept synchronous. Confirm that oxigraph's Store.query() for SELECT is indeed synchronous and document this expectation explicitly.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| const split = query.val.split('?').slice(1).map(m => m.trim().split(/\s+/)) | ||
| const last = split.findIndex(s => s.length > 1) | ||
| const cols = split.slice(0, last + 1).map(c => c[0]) |
There was a problem hiding this comment.
Logic Error: The sparql_table SQL function builder computes last as the index of the first element in split whose length > 1, then takes cols as split.slice(0, last + 1). This logic assumes that all column-variable entries appear before any entry with more than one token — but that heuristic is fragile and breaks when the SPARQL projection contains AS aliases or when the variable list is formatted differently. Additionally, split.slice(1) discards the portion before the first ?, which silently drops any prefix text.
Consider parsing the SELECT projection variables with a proper regex (e.g. query.val.match(/\?\w+/g)) instead of relying on whitespace splitting around ?.
| const split = query.val.split('?').slice(1).map(m => m.trim().split(/\s+/)) | |
| const last = split.findIndex(s => s.length > 1) | |
| const cols = split.slice(0, last + 1).map(c => c[0]) | |
| const cols = (query.val.match(/\?\w+/g) ?? []).map(v => v.slice(1)) |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| const split = query.val.split('?').slice(1).map(m => m.trim().split(/\s+/)) | ||
| const last = split.findIndex(s => s.length > 1) | ||
| const cols = split.slice(0, last + 1).map(c => c[0]) | ||
| return `(SELECT ${cols.map(c => `value->>'$.${c}.value' as "${c}"`)} FROM json_each(sparql_table(${query})->'$.results.bindings'))` |
There was a problem hiding this comment.
Security: The column names extracted from the SPARQL query string are interpolated directly into the SQL fragment without any sanitization. A SPARQL variable name like ?"; DROP TABLE Employees; -- would inject arbitrary SQL into the generated SELECT statement.
The extracted column names should be validated to contain only word characters (/^\w+$/) before being used in the SQL template.
| return `(SELECT ${cols.map(c => `value->>'$.${c}.value' as "${c}"`)} FROM json_each(sparql_table(${query})->'$.results.bindings'))` | |
| if (cols.some(c => !/^\w+$/.test(c))) throw new Error(`Invalid SPARQL variable name in: ${query.val}`) | |
| return `(SELECT ${cols.map(c => `value->>'$.${c}.value' as "${c}"`)} FROM json_each(sparql_table(${query})->'$.results.bindings'))` |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
| } | ||
| }, | ||
| "sql": { | ||
| "[development]": { |
There was a problem hiding this comment.
Why would this be limited to the development profile?
2 participants
This PR implements the
SPARQL_EXECUTEprocedure andSPARQL_TABLEtable function of SAP HANA into the@cap-js/sqlitedatabase service.This enables developers to trigger
sparqlqueries against their developmentSQLite3database the same way as they can with theirSAP HANA cloudinstance with thetriple storefeature enabled.Bridging the gap between the metadata stored inside the knowledge graphs and actual data stored in the database. Enabling
AIqueries to directly map semantical concepts to exact stored values.The model can produce metadata from the
enumdefinition. Mapping the exact same semantical meaning that people get from thecdsfile. Allowing the AI to use the semantical terms inside their query. While still applying the correct technical values.Resulting in a mixed query that would look something like: