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.
feat: triple store support for
@cap-js/sqlite#49New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Uh oh!
There was an error while loading. Please reload this page.
feat: triple store support for
@cap-js/sqlite#49Changes from all commits
809ca02File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
Check warning on line 3 in lib/knowledge-graph/SQLiteService.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.RESPONSEvalue is accessed directly without awaiting. SQLite'sbetter-sqlite3(used by@cap-js/sqlite) does not support async user-defined functions; an async callback will silently returnundefinedto SQLite instead of the query result.The
sparql_tablefunction must only be used for SELECT-type SPARQL queries (not LOAD), and the synchronoussuper.query()path must be kept synchronous. Confirm thatoxigraph'sStore.query()for SELECT is indeed synchronous and document this expectation explicitly.Please provide feedback on the review comment by checking the appropriate box:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Mutating the shared
factoryobject on everyget factorycall causes race conditions and double-wrapping.super.factorylikely returns the same object reference each time. Every call to this getter overwritesfactory._createwith whateverfactory.createcurrently is, then wraps it again. On the second call,factory._createbecomes 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 insideinit()instead.Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
Check warning on line 25 in lib/knowledge-graph/SQLiteService.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The regex
.exec(query)on line 25 can returnnullif theCALL SPARQL_EXECUTE(...)pattern does not match (e.g. malformed query), causing a destructuring TypeError at runtime.The outer
ifonly checks forCALL SPARQL_EXECUTEbut 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.Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
Check warning on line 34 in lib/knowledge-graph/SQLiteService.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic Error: The
sparql_tableSQL function builder computeslastas the index of the first element insplitwhose length > 1, then takescolsassplit.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
SELECTprojection variables with a proper regex (e.g.query.val.match(/\?\w+/g)) instead of relying on whitespace splitting around?.Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 generatedSELECTstatement.The extracted column names should be validated to contain only word characters (
/^\w+$/) before being used in the SQL template.Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: The regex
/^\w*LOAD/iincorrectly matches queries that are notLOADstatements.\w*matches zero or more word characters, so strings like"SELECT LOAD ..."or"SELECTLOAD"would also match. SPARQLLOADis always the first keyword; the pattern should anchor to optional whitespace only.Consider using
/^\s*LOAD\b/ito match only actual LOAD queries.Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
Check warning on line 44 in lib/knowledge-graph/triplestore.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug:
.exec(query)can returnnullfor a LOAD query that doesn't match the expectedLOAD <uri> INTO GRAPH <uri>pattern (e.g.LOAD <uri>withoutINTO GRAPH). Destructuringnullthrows a TypeError.Should check the result before destructuring.
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug:
query()can return a Promise (when the query is a LOAD) or a plain object{ RESPONSE }(for regular queries), but callers inSQLiteService.jstreat the return value as a synchronous object with a.RESPONSEproperty. In thesparql_tableSQLite user function the return ofthis._tripleStore.query(...)is used directly as.RESPONSE, which will be aPromiseobject, not the actual result string, causing the SQL function to silently return garbage.The
querymethod should be madeasyncand theloadpath should be awaited, or the two code paths should be separated so the SQLite scalar function callback can handle this correctly.Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would this be limited to the development profile?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh oh!
There was an error while loading. Please reload this page.