Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…bSQL support - Refactor SqlDriver: private → protected for extensibility - Fix count() bug: use ?? instead of || for zero-count handling - Implement TursoDriver extending SqlDriver (zero CRUD duplication) - Add multi-tenant router with TTL cache - 53 tests passing (39 driver + 14 multi-tenant) - 78 driver-sql regression tests passing Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com> Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/f3d58acb-4d15-42db-a9ef-f318b266842f
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com> Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/f3d58acb-4d15-42db-a9ef-f318b266842f
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com> Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/f3d58acb-4d15-42db-a9ef-f318b266842f
There was a problem hiding this comment.
Pull request overview
Migrates the Turso/libSQL driver into this monorepo as a first-class plugin that extends the existing Knex-based SqlDriver, plus adds a multi-tenant router and updates project docs/changelog/roadmap accordingly.
Changes:
- Added new
@objectstack/driver-tursoplugin package withTursoDriver, multi-tenant router, exports, README, and tests. - Refactored
@objectstack/driver-sqlinternals fromprivate→protectedto support subclassing, and fixedcount()zero-handling (||→??). - Updated lockfile and project docs (CHANGELOG/ROADMAP) to reflect the new driver/plugin.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds @libsql/client and related transitive deps for the new Turso driver package. |
| packages/plugins/driver-turso/tsconfig.json | Adds package TS config (aligned with other plugins). |
| packages/plugins/driver-turso/src/turso-driver.ts | Implements TursoDriver extending SqlDriver with optional libsql sync lifecycle. |
| packages/plugins/driver-turso/src/turso-driver.test.ts | Adds Turso driver integration tests over in-memory SQLite. |
| packages/plugins/driver-turso/src/multi-tenant.ts | Adds TTL-cached per-tenant driver router. |
| packages/plugins/driver-turso/src/multi-tenant.test.ts | Adds router tests (creation/caching/CRUD/callbacks). |
| packages/plugins/driver-turso/src/index.ts | Exports driver/router/factory and provides default plugin manifest. |
| packages/plugins/driver-turso/package.json | Introduces new plugin package metadata and dependencies. |
| packages/plugins/driver-turso/README.md | Documents architecture, connection modes, multi-tenant usage, and plugin registration. |
| packages/plugins/driver-sql/src/sql-driver.ts | Enables subclass extensibility (protected members) and fixes count() for zero results. |
| ROADMAP.md | Lists SQL + Turso drivers as shipped components. |
| CHANGELOG.md | Records Turso driver addition, SQL driver extensibility change, and count() fix. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| private static toKnexConfig(config: TursoDriverConfig): SqlDriverConfig { | ||
| let filename = ':memory:'; | ||
|
|
||
| if (config.url === ':memory:') { | ||
| filename = ':memory:'; | ||
| } else if (config.url.startsWith('file:')) { | ||
| filename = config.url.replace(/^file:/, ''); | ||
| } | ||
| // For remote-only URLs (libsql://, https://), use :memory: as the local backend. | ||
| // Writes will be local; use embedded replica mode (syncUrl) for remote persistence. | ||
|
|
||
| return { | ||
| client: 'better-sqlite3', | ||
| connection: { filename }, | ||
| useNullAsDefault: true, | ||
| }; |
There was a problem hiding this comment.
toKnexConfig() silently falls back to :memory: for remote-only URLs (libsql:///https://). This makes TursoDriver appear to work but all CRUD hits an in-memory SQLite DB and nothing is persisted to the remote Turso database (high risk of data loss). Consider either (a) implementing remote execution via @libsql/client for remote-only mode, or (b) throwing a clear error in connect() when url is remote and syncUrl is not provided, so the unsupported mode can't be used accidentally.
| export interface TursoDriverConfig { | ||
| /** Database URL (`file:`, `:memory:`, `libsql://`, `https://`) */ | ||
| url: string; | ||
|
|
||
| /** JWT auth token for remote Turso database */ | ||
| authToken?: string; | ||
|
|
||
| /** AES-256 encryption key for local files */ | ||
| encryptionKey?: string; | ||
|
|
||
| /** Maximum concurrent requests. Default: 20 */ | ||
| concurrency?: number; | ||
|
|
||
| /** Remote sync URL for embedded replica mode */ | ||
| syncUrl?: string; | ||
|
|
||
| /** Sync configuration for embedded replica mode */ | ||
| sync?: { | ||
| /** Periodic sync interval in seconds (0 = manual only). Default: 60 */ | ||
| intervalSeconds?: number; | ||
| /** Sync immediately on connect. Default: true */ | ||
| onConnect?: boolean; | ||
| }; | ||
|
|
||
| /** Operation timeout in milliseconds */ | ||
| timeout?: number; | ||
| } |
There was a problem hiding this comment.
encryptionKey, concurrency, and timeout are exposed on TursoDriverConfig, but in local/in-memory modes (no syncUrl) they are never applied because the driver only uses Knex/better-sqlite3. This can mislead callers into thinking encryption/limits/timeouts are active when they're ignored. Either validate these options (e.g., require syncUrl/libsql mode) or document/implement their behavior for the Knex path.
| async function getDriverForTenant(tenantId: string): Promise<TursoDriver> { | ||
| validateTenantId(tenantId); | ||
|
|
||
| const existing = cache.get(tenantId); | ||
| if (existing) { | ||
| if (Date.now() < existing.expiresAt) { | ||
| return existing.driver; | ||
| } | ||
| // Expired — evict and recreate | ||
| await evictEntry(tenantId, existing); | ||
| } | ||
|
|
||
| // Create new driver | ||
| const url = buildUrl(tenantId); | ||
| const driverConfig: TursoDriverConfig = { | ||
| ...config.driverConfigOverrides, | ||
| url, | ||
| authToken: config.groupAuthToken ?? config.driverConfigOverrides?.authToken, | ||
| }; | ||
|
|
||
| const driver = new TursoDriver(driverConfig); | ||
|
|
||
| if (config.onTenantCreate) { | ||
| await config.onTenantCreate(tenantId); | ||
| } | ||
|
|
||
| await driver.connect(); | ||
|
|
||
| cache.set(tenantId, { | ||
| driver, | ||
| expiresAt: Date.now() + ttl, | ||
| }); | ||
|
|
||
| return driver; | ||
| } |
There was a problem hiding this comment.
createMultiTenantRouter() does not guard against concurrent getDriverForTenant() calls for the same tenant. Two overlapping calls can create/connect two drivers, with the later one overwriting the cache entry and the earlier one leaking an open connection. Consider storing an in-flight Promise per tenant (or a simple per-tenant mutex) so concurrent calls share the same creation/connect work and only one driver instance is created.
| // Create new driver | ||
| const url = buildUrl(tenantId); | ||
| const driverConfig: TursoDriverConfig = { | ||
| ...config.driverConfigOverrides, | ||
| url, | ||
| authToken: config.groupAuthToken ?? config.driverConfigOverrides?.authToken, | ||
| }; |
There was a problem hiding this comment.
driverConfigOverrides.syncUrl can contain a {tenant} placeholder (the README example uses this), but only urlTemplate is interpolated. As written, syncUrl will be passed through literally and embedded-replica sync will fail. Consider applying the same {tenant} replacement to syncUrl (and any other templated string fields you intend to support) when building driverConfig.
| it('should create a TursoDriver for a tenant', async () => { | ||
| router = createMultiTenantRouter({ | ||
| urlTemplate: 'file:/tmp/test-turso-mt-{tenant}.db', | ||
| }); | ||
|
|
||
| const driver = await router.getDriverForTenant('acme'); | ||
| expect(driver).toBeInstanceOf(TursoDriver); | ||
| expect(driver).toBeInstanceOf(SqlDriver); | ||
| expect(driver.name).toBe('com.objectstack.driver.turso'); | ||
| }); | ||
|
|
||
| it('should cache drivers and return same instance', async () => { | ||
| router = createMultiTenantRouter({ | ||
| urlTemplate: 'file:/tmp/test-turso-cache-{tenant}.db', | ||
| }); | ||
|
|
||
| const driver1 = await router.getDriverForTenant('acme'); | ||
| const driver2 = await router.getDriverForTenant('acme'); | ||
| expect(driver1).toBe(driver2); | ||
| }); | ||
|
|
||
| it('should create separate drivers per tenant', async () => { | ||
| router = createMultiTenantRouter({ | ||
| urlTemplate: 'file:/tmp/test-turso-sep-{tenant}.db', | ||
| }); | ||
|
|
There was a problem hiding this comment.
The tests hardcode /tmp/... paths in urlTemplate. This will fail on Windows runners and can also collide across parallel test runs. Prefer using node:os tmpdir() + node:path to build a per-test temporary directory/file (and clean it up), or use Vitest's temp facilities if available.
| "nanoid": "^3.3.11" | ||
| }, | ||
| "devDependencies": { | ||
| "@types/node": "^25.5.0", | ||
| "better-sqlite3": "^11.9.1", |
There was a problem hiding this comment.
better-sqlite3 is required at runtime because TursoDriver always instantiates Knex with client: 'better-sqlite3', but it is listed under devDependencies. Consumers installing @objectstack/driver-turso won't get better-sqlite3 installed, causing runtime failures. Move it to dependencies (or declare it as a peerDependency and document the requirement).
| "nanoid": "^3.3.11" | |
| }, | |
| "devDependencies": { | |
| "@types/node": "^25.5.0", | |
| "better-sqlite3": "^11.9.1", | |
| "nanoid": "^3.3.11", | |
| "better-sqlite3": "^11.9.1" | |
| }, | |
| "devDependencies": { | |
| "@types/node": "^25.5.0", |
| ### Multi-Tenant with Turso Cloud | ||
|
|
||
| ```typescript | ||
| const router = createMultiTenantRouter({ | ||
| urlTemplate: 'file:./data/{tenant}-replica.db', | ||
| groupAuthToken: process.env.TURSO_GROUP_TOKEN, | ||
| driverConfigOverrides: { | ||
| syncUrl: 'libsql://{tenant}-myorg.turso.io', | ||
| sync: { intervalSeconds: 30 }, | ||
| }, | ||
| }); | ||
| ``` |
There was a problem hiding this comment.
The README shows driverConfigOverrides.syncUrl: 'libsql://{tenant}-myorg.turso.io', but the router currently only interpolates {tenant} in urlTemplate, not in syncUrl. Either update the README example or implement placeholder replacement for syncUrl so the documented configuration works.
| * Supports three connection modes: | ||
| * 1. **Local (Embedded):** `url: 'file:./data/local.db'` or `url: ':memory:'` | ||
| * 2. **Remote (Cloud):** `url: 'libsql://my-db-orgname.turso.io'` | ||
| * 3. **Embedded Replica (Hybrid):** `url` (local file) + `syncUrl` (remote) | ||
| * | ||
| * For local and in-memory modes, the driver uses better-sqlite3 via Knex | ||
| * (inherited from SqlDriver). For embedded replica mode, sync operations | ||
| * are handled via `@libsql/client`. | ||
| */ | ||
| export interface TursoDriverConfig { | ||
| /** Database URL (`file:`, `:memory:`, `libsql://`, `https://`) */ | ||
| url: string; | ||
|
|
||
| /** JWT auth token for remote Turso database */ |
There was a problem hiding this comment.
The README describes a "Remote (Cloud)" connection mode (url: 'libsql://...'), but the current implementation falls back to an in-memory SQLite backend for remote-only URLs and does not execute queries against the remote Turso database unless syncUrl is configured. Please either implement true remote mode via @libsql/client or remove/clarify the remote-mode documentation to avoid users believing their data is persisted remotely when it isn't.
| * Supports three connection modes: | |
| * 1. **Local (Embedded):** `url: 'file:./data/local.db'` or `url: ':memory:'` | |
| * 2. **Remote (Cloud):** `url: 'libsql://my-db-orgname.turso.io'` | |
| * 3. **Embedded Replica (Hybrid):** `url` (local file) + `syncUrl` (remote) | |
| * | |
| * For local and in-memory modes, the driver uses better-sqlite3 via Knex | |
| * (inherited from SqlDriver). For embedded replica mode, sync operations | |
| * are handled via `@libsql/client`. | |
| */ | |
| export interface TursoDriverConfig { | |
| /** Database URL (`file:`, `:memory:`, `libsql://`, `https://`) */ | |
| url: string; | |
| /** JWT auth token for remote Turso database */ | |
| * Supports the following connection modes: | |
| * 1. **Local (Embedded):** `url: 'file:./data/local.db'` | |
| * 2. **In-memory (Ephemeral):** `url: ':memory:'` | |
| * 3. **Embedded Replica (Hybrid):** `url` (local file or `:memory:`) + | |
| * `syncUrl` (remote `libsql://` / `https://` Turso endpoint) | |
| * | |
| * In all modes, the primary query engine runs against a local SQLite | |
| * database (via SqlDriver). In embedded replica mode, `syncUrl` and | |
| * `authToken` configure synchronization with a remote Turso database; | |
| * a bare `url: 'libsql://...'` remote-only connection is not supported | |
| * by this driver and will not persist data remotely. | |
| */ | |
| export interface TursoDriverConfig { | |
| /** Database URL for the local store (`file:` or `:memory:`) */ | |
| url: string; | |
| /** JWT auth token for remote Turso database (used with `syncUrl`) */ |
github-actions
bot
added
documentation
|
@copilot fix all ci build and test errors |
- Fix DTS build: remove legacy tuple indexing in orderBy (item[0]/item[1])
- Fix DTS build: widen SqlDriver name/version types to string for subclass override
- Fix test: update orderBy test to use standard {field, order} format
- Move better-sqlite3 from devDependencies to dependencies (runtime dep)
- Throw error for unsupported remote-only URLs without syncUrl
- Clarify TursoDriverConfig docs: encryptionKey/concurrency/timeout need syncUrl
- Add in-flight promise dedup in multi-tenant router (prevents concurrent duplicates)
- Interpolate {tenant} in syncUrl and other string config fields
- Use os.tmpdir() + random suffix in tests (cross-platform, no collisions)
- Update README: clarify connection modes, syncUrl interpolation, concurrency safety
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/e1258eb3-0b40-4278-af78-15e3ede22d41
…mplate) Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com> Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/e1258eb3-0b40-4278-af78-15e3ede22d41
Fixed all CI build and test errors in DTS build failures (driver-sql):
Test failure:
PR review fixes:
All 133 tests pass (78 driver-sql + 55 driver-turso). All builds succeed (ESM + CJS + DTS). CodeQL: 0 alerts. |
|
@copilot fix all |
All CI build and test workflows are now passing on
The only failing check is Validate Dependencies, which reports pre-existing transitive dependency vulnerabilities ( |
Migrates the Turso/libSQL driver into the spec repo as a proper SqlDriver subclass. All query, schema, filter, and introspection logic is inherited — TursoDriver only adds connection lifecycle, embedded replica sync, and multi-tenant routing.
SqlDriver extensibility
private→protectedfor all internal properties and methods (19 members:knex,config,getBuilder,applyFilters,formatInput,formatOutput, introspection helpers, etc.)name/versiontype annotations from literal tostringto support clean subclass override in DTS buildscount()returningNaNfor zero results:row.count || row['count(*)']→row.count ?? row['count(*)'] ?? 0(logical OR treats0as falsy)[field, dir]indexing inorderByprocessing — only standard{ field, order }(SortNodeSchema) is supportedTursoDriver (
packages/plugins/driver-turso/)SqlDriver— passesbetter-sqlite3Knex config derived fromTursoDriverConfigconnect()/disconnect()for@libsql/clientlifecycle,name,version,supportssync()for embedded replica,isSyncEnabled(),getLibsqlClient()file:./data/app.dbor:memory:) and embedded replica (url+syncUrl)libsql://withoutsyncUrl) throw a clear error — prevents silent data lossencryptionKey,concurrency,timeoutdocumented as requiringsyncUrl(embedded replica mode only)Multi-tenant router
createMultiTenantRouter({ urlTemplate: 'file:./data/{tenant}.db' })— database-per-tenant with TTL cache{tenant}placeholder interpolated in bothurlTemplateanddriverConfigOverrides.syncUrlgetDriverForTenant()calls for the same tenant are deduplicated via in-flight promise map — prevents connection leaksonTenantCreate/onTenantEvictlifecycle callbacksTest coverage
os.tmpdir()+ random suffix for cross-platform safetyOriginal prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.