Conversation
- Add await to all async DB methods: createNode, getNode, updateNode, deleteNode, createEdge, getEdge, deleteEdge, export, import, close, mergeNode, mergeEdge, createPropertyIndex, listIndexes, dropIndex - Make transaction callbacks async with await on inner calls - Await NodeQuery terminal methods: exec(), first(), count(), exists() - Await TraversalQuery terminal methods: toArray(), toPaths(), etc. - Make beforeEach/afterEach/it() callbacks async where needed - Transform expect(() => asyncMethod()).toThrow() to rejects.toThrow() - Handle Array.from/map with async callbacks using Promise.all - Convert forEach(async ...) to await Promise.all(arr.map(async ...)) - PatternQuery.exec/first/count remain synchronous (no await) - traverse() sync validation unchanged Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: michaeloboyle <61171+michaeloboyle@users.noreply.github.com>
Agent-Logs-Url: https://github.com/michaeloboyle/sqlite-graph/sessions/c2e30f86-7c89-4c8c-a70d-c9ff39267e19 Co-authored-by: michaeloboyle <61171+michaeloboyle@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates the public GraphDatabase API and query execution methods from synchronous returns to async Promises to enable browser/WASM compatibility where synchronous file I/O is unavailable.
Changes:
- Converted core database CRUD/transaction/export/import/index APIs to
asyncand introduced an asyncGraphDatabase.create(...)factory. - Updated query terminal methods (
NodeQuery.exec/first/count/exists,TraversalQuery.toArray/toPaths/shortestPath/paths/allPaths) to be async. - Migrated unit/integration tests and helper scripts to the async API.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 26 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/Database.ts | Converts GraphDatabase public API to async; adds create(...), internal sync helper(s), and async index/export/import operations. |
| src/query/NodeQuery.ts | Makes query terminal methods async (exec/first/count/exists). |
| src/query/TraversalQuery.ts | Makes traversal terminal methods async (toArray/toPaths/shortestPath/paths/allPaths). |
| src/index.ts | Updates public exports to reflect the async API surface. |
| tests/unit/Database.test.ts | Updates core GraphDatabase tests for async CRUD/transaction/export/import behavior. |
| tests/unit/Database-merge.test.ts | Updates merge/index-management tests to async behavior. |
| tests/unit/NodeQuery.test.ts | Updates NodeQuery tests for async terminal methods and execution. |
| tests/unit/NodeQuery-both-direction.test.ts | Updates bidirectional relationship query tests for async execution. |
| tests/unit/PatternQuery.test.ts | Updates PatternQuery tests to the async database/query execution model. |
| tests/unit/Transaction.test.ts | Updates transaction tests for async transaction callback support. |
| tests/unit/TraversalQuery.test.ts | Updates traversal tests for async terminal traversal methods. |
| tests/unit/TraversalQuery-paths.test.ts | Updates paths() wrapper tests for async traversal terminal methods. |
| tests/unit/concurrency.test.ts | Updates concurrency-related tests to the async database API. |
| tests/integration/job-pipeline.test.ts | Migrates end-to-end job pipeline flows to async database/query/traversal calls. |
| tests/integration/graph-operations.test.ts | Migrates complex graph operation workflows to async database/query/traversal calls. |
| scripts/transform_tests.py | Automation script used to transform tests to async patterns. |
| scripts/transform_tests_v2.py | Updated/alternative automation script for async test transformations. |
Comments suppressed due to low confidence (2)
tests/unit/Database-merge.test.ts:196
expect(async () => { ... }).not.toThrow()does not work for async code because the function returns a Promise (rejections won’t be caught bytoThrow). To assert “does not reject”, useawait expect(db.mergeNode(...)).resolves...(or simplyawait db.mergeNode(...)and let the test fail on rejection).
it('should not throw when only one node matches', async () => {
await db.createNode('Company', { name: 'TechCorp', industry: 'SaaS' });
expect(async () => {
await db.mergeNode(
'Company',
{ name: 'TechCorp' },
{ name: 'TechCorp', size: 'Large' }
);
}).not.toThrow();
src/core/Database.ts:973
createPropertyIndex()interpolatesnodeType,property, andindexNamedirectly into SQL (including a quoted string literal inWHERE type = '${nodeType}').validateNodeType()only checks for non-empty strings (and schema membership is optional), so this is vulnerable to SQL injection / malformed identifiers. Consider restrictingnodeType/propertyto a safe identifier regex, escaping quotes for the string literal, and/or using parameters where possible (e.g.,WHERE type = ?).
async createPropertyIndex(nodeType: string, property: string, unique = false): Promise<void> {
const indexName = `idx_merge_${nodeType}_${property}`;
const uniqueClause = unique ? 'UNIQUE' : '';
// Note: SQLite doesn't allow parameters in partial index WHERE clauses
// Must use string concatenation (safe here as nodeType is validated)
const sql = `
CREATE ${uniqueClause} INDEX IF NOT EXISTS ${indexName}
ON nodes(type, json_extract(properties, '$.${property}'))
WHERE type = '${nodeType}'
`;
this.db.prepare(sql).run();
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Tests all traversal methods, path finding, and cycle detection | ||
| */ | ||
| describe('TraversalQuery', () => { | ||
| describe('TraversalQuery', async () => { |
There was a problem hiding this comment.
Jest describe callbacks must be synchronous; marking this (and nested describe blocks in this file) as async returns a Promise that Jest does not await and may reject with “Returning a Promise from describe is not supported”. Remove async from describe and keep async only in beforeEach/afterEach/it callbacks.
| import { GraphDatabase } from '../../src/core/Database'; | ||
|
|
||
| describe('TraversalQuery - paths() wrapper', () => { | ||
| describe('TraversalQuery - paths() wrapper', async () => { |
There was a problem hiding this comment.
Jest describe callbacks must be synchronous; marking this as async returns a Promise that Jest does not await and can break test registration. Remove async from describe blocks and keep async only in hooks/tests.
| describe('TraversalQuery - paths() wrapper', async () => { | |
| describe('TraversalQuery - paths() wrapper', () => { |
| import * as fs from 'fs'; | ||
|
|
||
| describe('TransactionContext', () => { | ||
| describe('TransactionContext', async () => { |
There was a problem hiding this comment.
Jest describe callbacks must be synchronous; using async here returns a Promise that Jest does not await and can break the suite. Remove async from describe blocks and keep async only in hooks/tests.
| import { PatternError } from '../../src/types/pattern'; | ||
|
|
||
| describe('PatternQuery', () => { | ||
| describe('PatternQuery', async () => { |
There was a problem hiding this comment.
Jest describe callbacks must be synchronous; using async here returns a Promise that Jest does not await and can break the suite. Remove async from describe blocks and keep async only in hooks/tests.
| import { Node } from '../../src/types'; | ||
|
|
||
| describe('NodeQuery', () => { | ||
| describe('NodeQuery', async () => { |
There was a problem hiding this comment.
Jest describe callbacks must be synchronous; using async here returns a Promise that Jest does not await and can break the suite. Remove async from describe blocks and keep async only in hooks/tests.
| describe('NodeQuery', async () => { | |
| describe('NodeQuery', () => { |
| """Check if position is directly inside expect(...) as the direct argument (not inside a nested arrow fn).""" | ||
| pre = content[max(0, pos - 80):pos] | ||
| # Check if preceded by expect( (direct arg) or expect(() => (lambda wrapper) | ||
| return bool(re.search(r'expect\s*\(\s*$', pre)) or bool(re.search(r'expect\s*\(\s*\(\s*\)\s*=>\s*$', pre)) |
There was a problem hiding this comment.
This return statement is missing a closing parenthesis, making the script invalid Python (it will raise a SyntaxError and can’t be run). Add the missing ) (and consider simplifying the boolean expression for readability).
| return bool(re.search(r'expect\s*\(\s*$', pre)) or bool(re.search(r'expect\s*\(\s*\(\s*\)\s*=>\s*$', pre)) | |
| return ( | |
| bool(re.search(r'expect\s*\(\s*$', pre)) | |
| or bool(re.search(r'expect\s*\(\s*\(\s*\)\s*=>\s*$', pre)) | |
| ) |
| async dropIndex(indexName: string): Promise<void> { | ||
| this.db.prepare(`DROP INDEX IF EXISTS ${indexName}`).run(); | ||
| } |
There was a problem hiding this comment.
dropIndex() interpolates indexName directly into the DROP INDEX statement. Since identifiers can’t be parameterized, this should validate/sanitize indexName (e.g., allow only [A-Za-z0-9_]+ and/or enforce an idx_merge_ prefix) to avoid SQL injection via this public API.
| const results = await this.exec(); | ||
| this.limitValue = original; | ||
| return results.length > 0 ? results[0] : null; |
There was a problem hiding this comment.
If exec() throws, first() won’t restore this.limitValue, leaving the query builder mutated for subsequent calls. Wrap the await this.exec() in a try/finally so limitValue is always restored.
| const results = await this.exec(); | |
| this.limitValue = original; | |
| return results.length > 0 ? results[0] : null; | |
| try { | |
| const results = await this.exec(); | |
| return results.length > 0 ? results[0] : null; | |
| } finally { | |
| this.limitValue = original; | |
| } |
| const jobCompanies = db2.traverse(importedJobs[0].id) | ||
| .out('POSTED_BY') | ||
| .toArray(); | ||
| expect(jobCompanies).toHaveLength(1); |
There was a problem hiding this comment.
toArray() is now async; this call is missing await, so jobCompanies will be a Promise and the length assertion will fail. Update this to await db2.traverse(...).out(...).toArray().
| const nodeCount = 100; | ||
| const nodes = Array.from({ length: nodeCount }, (_, i) => | ||
| db.createNode('Node', { index: i, value: Math.random() }) | ||
| const nodes = await Promise.all(Array.from({ length: nodeCount }, async (_, i) => await db.createNode('Node', { index: i, value: Math.random() }) |
There was a problem hiding this comment.
This Promise.all(Array.from(...)) call is missing a closing )) for the Array.from callback / Promise.all invocation, making the test file invalid TypeScript. Close the parentheses so the array of node creation Promises is built correctly.
| const nodes = await Promise.all(Array.from({ length: nodeCount }, async (_, i) => await db.createNode('Node', { index: i, value: Math.random() }) | |
| const nodes = await Promise.all( | |
| Array.from({ length: nodeCount }, async (_, i) => | |
| await db.createNode('Node', { index: i, value: Math.random() }) | |
| ) |
Converts all
GraphDatabasepublic methods from synchronous to async-returning Promises, enabling browser WASM compatibility where synchronous file I/O is unavailable.API Changes
Migration example
src/core/Database.tsstatic async create(path, options?)factory method_getNodeSync()helper for internal sync node lookups (avoids async chains inside CRUD methods that need to validate node existence)createNode,getNode,updateNode,deleteNode,createEdge,getEdge,deleteEdge,transaction,export,import,close,mergeNode,mergeEdge,createPropertyIndex,listIndexes,dropIndextraverse()andnodes()remain synchronous — they return builder objects with no I/O;traverse()still validates node existence via directdb.prepare()calltransaction()callback now acceptsT | Promise<T>, enabling async operations inside transactionspattern()(Record<string, unknown>→Record<string, GraphEntity>)src/query/NodeQuery.tsexec(),first(),count(),exists()→ asyncsrc/query/TraversalQuery.tstoArray(),toPaths(),shortestPath(),paths(),allPaths()→ asyncgetNode,getNeighbors) remain synchronousTests (11 files)
beforeEach/afterEachand test callbacks made asyncexpect(() => ...).toThrow()→await expect(...).rejects.toThrow()where neededasync (ctx) => { ... }