Skip to content

Commit 9ceb926

Browse files
Copilothotlong
andcommitted
refactor: address code review - extract DDL helper, add identifier validation, add local mode test
- Extract buildCreateTableSQL() shared helper to eliminate code duplication - Add assertSafeIdentifier() for SQL injection prevention in DDL - Clean up Map pattern in plugin.ts (remove non-null assertion) - Add local mode syncSchemasBatch test for TursoDriver Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com> Agent-Logs-Url: https://github.com/objectstack-ai/spec/sessions/8dd81704-9ca5-4d57-ad08-e1c15692b189
1 parent dee8e25 commit 9ceb926

3 files changed

Lines changed: 90 additions & 37 deletions

File tree

packages/objectql/src/plugin.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -283,10 +283,12 @@ export class ObjectQLPlugin implements Plugin {
283283

284284
const tableName = obj.tableName || obj.name;
285285

286-
if (!driverGroups.has(driver)) {
287-
driverGroups.set(driver, []);
286+
let group = driverGroups.get(driver);
287+
if (!group) {
288+
group = [];
289+
driverGroups.set(driver, group);
288290
}
289-
driverGroups.get(driver)!.push({ obj, tableName });
291+
group.push({ obj, tableName });
290292
}
291293

292294
// Process each driver group

packages/plugins/driver-turso/src/remote-transport.ts

Lines changed: 54 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,13 @@ const DEFAULT_ID_LENGTH = 16;
2525
*/
2626
const BUILTIN_COLUMNS = new Set(['id', 'created_at', 'updated_at']);
2727

28+
/**
29+
* Pattern for valid SQL identifiers (table and column names).
30+
* Prevents SQL injection in DDL statements where parameterized queries
31+
* are not supported (e.g. PRAGMA, CREATE TABLE, ALTER TABLE).
32+
*/
33+
const SAFE_IDENTIFIER = /^[a-zA-Z_][a-zA-Z0-9_]*$/;
34+
2835
/**
2936
* Remote transport that executes all queries via @libsql/client.
3037
*
@@ -338,6 +345,7 @@ export class RemoteTransport {
338345

339346
const objectDef = schema as { name: string; fields?: Record<string, any> };
340347
const tableName = object;
348+
this.assertSafeIdentifier(tableName);
341349

342350
// Check if table exists
343351
const checkResult = await this.client!.execute({
@@ -347,21 +355,7 @@ export class RemoteTransport {
347355
const exists = checkResult.rows.length > 0;
348356

349357
if (!exists) {
350-
// Build CREATE TABLE
351-
let sql = `CREATE TABLE "${tableName}" ("id" TEXT PRIMARY KEY, "created_at" TEXT DEFAULT (datetime('now')), "updated_at" TEXT DEFAULT (datetime('now'))`;
352-
353-
if (objectDef.fields) {
354-
for (const [name, field] of Object.entries(objectDef.fields)) {
355-
if (BUILTIN_COLUMNS.has(name)) continue;
356-
const type = (field as any).type || 'string';
357-
if (type === 'formula') continue; // Virtual — no column
358-
const colType = this.mapFieldTypeToSQL(field);
359-
sql += `, "${name}" ${colType}`;
360-
}
361-
}
362-
363-
sql += ')';
364-
await this.client!.execute(sql);
358+
await this.client!.execute(this.buildCreateTableSQL(tableName, objectDef));
365359
} else {
366360
// ALTER TABLE — add missing columns
367361
if (objectDef.fields) {
@@ -372,12 +366,12 @@ export class RemoteTransport {
372366
const existingColumns = new Set(columnsResult.rows.map((r: any) => r.name));
373367

374368
for (const [name, field] of Object.entries(objectDef.fields)) {
375-
if (!existingColumns.has(name)) {
376-
const type = (field as any).type || 'string';
377-
if (type === 'formula') continue; // Virtual — no column
378-
const colType = this.mapFieldTypeToSQL(field);
379-
await this.client!.execute(`ALTER TABLE "${tableName}" ADD COLUMN "${name}" ${colType}`);
380-
}
369+
if (existingColumns.has(name)) continue;
370+
const type = (field as any).type || 'string';
371+
if (type === 'formula') continue; // Virtual — no column
372+
this.assertSafeIdentifier(name);
373+
const colType = this.mapFieldTypeToSQL(field);
374+
await this.client!.execute(`ALTER TABLE "${tableName}" ADD COLUMN "${name}" ${colType}`);
381375
}
382376
}
383377
}
@@ -398,6 +392,11 @@ export class RemoteTransport {
398392
this.ensureClient();
399393
if (schemas.length === 0) return;
400394

395+
// Validate all identifiers up-front
396+
for (const s of schemas) {
397+
this.assertSafeIdentifier(s.object);
398+
}
399+
401400
// Phase 1: introspect all tables in one batch
402401
const introspectStmts: InStatement[] = schemas.map((s) => ({
403402
sql: `SELECT name FROM sqlite_master WHERE type='table' AND name=?`,
@@ -422,19 +421,7 @@ export class RemoteTransport {
422421

423422
for (const { object, schema } of newSchemas) {
424423
const objectDef = schema as { name: string; fields?: Record<string, any> };
425-
let sql = `CREATE TABLE "${object}" ("id" TEXT PRIMARY KEY, "created_at" TEXT DEFAULT (datetime('now')), "updated_at" TEXT DEFAULT (datetime('now'))`;
426-
427-
if (objectDef.fields) {
428-
for (const [name, field] of Object.entries(objectDef.fields)) {
429-
if (BUILTIN_COLUMNS.has(name)) continue;
430-
const type = (field as any).type || 'string';
431-
if (type === 'formula') continue;
432-
const colType = this.mapFieldTypeToSQL(field);
433-
sql += `, "${name}" ${colType}`;
434-
}
435-
}
436-
sql += ')';
437-
ddlStatements.push(sql);
424+
ddlStatements.push(this.buildCreateTableSQL(object, objectDef));
438425
}
439426

440427
// Phase 2b: for existing tables, introspect columns in one batch
@@ -456,6 +443,7 @@ export class RemoteTransport {
456443
if (existingColumns.has(name)) continue;
457444
const type = (field as any).type || 'string';
458445
if (type === 'formula') continue;
446+
this.assertSafeIdentifier(name);
459447
const colType = this.mapFieldTypeToSQL(field);
460448
ddlStatements.push(`ALTER TABLE "${object}" ADD COLUMN "${name}" ${colType}`);
461449
}
@@ -484,6 +472,38 @@ export class RemoteTransport {
484472
return this.client;
485473
}
486474

475+
/**
476+
* Validate that a string is a safe SQL identifier.
477+
* Prevents injection in DDL where parameterized queries are unsupported.
478+
*/
479+
private assertSafeIdentifier(name: string): void {
480+
if (!SAFE_IDENTIFIER.test(name)) {
481+
throw new Error(`RemoteTransport: unsafe identifier rejected: "${name}"`);
482+
}
483+
}
484+
485+
/**
486+
* Build a CREATE TABLE SQL string for the given object definition.
487+
* Shared by syncSchema() and syncSchemasBatch() to avoid duplication.
488+
*/
489+
private buildCreateTableSQL(tableName: string, objectDef: { fields?: Record<string, any> }): string {
490+
let sql = `CREATE TABLE "${tableName}" ("id" TEXT PRIMARY KEY, "created_at" TEXT DEFAULT (datetime('now')), "updated_at" TEXT DEFAULT (datetime('now'))`;
491+
492+
if (objectDef.fields) {
493+
for (const [name, field] of Object.entries(objectDef.fields)) {
494+
if (BUILTIN_COLUMNS.has(name)) continue;
495+
const type = (field as any).type || 'string';
496+
if (type === 'formula') continue; // Virtual — no column
497+
this.assertSafeIdentifier(name);
498+
const colType = this.mapFieldTypeToSQL(field);
499+
sql += `, "${name}" ${colType}`;
500+
}
501+
}
502+
503+
sql += ')';
504+
return sql;
505+
}
506+
487507
/**
488508
* Map ObjectStack field types to SQLite column types for DDL.
489509
*/

packages/plugins/driver-turso/src/turso-driver.test.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -227,6 +227,37 @@ describe('TursoDriver (SQLite Integration)', () => {
227227
expect(created.price).toBe(9.99);
228228
});
229229

230+
it('should batch-sync multiple schemas in local mode (sequential fallback)', async () => {
231+
await driver.syncSchemasBatch([
232+
{
233+
object: 'local_orders',
234+
schema: {
235+
name: 'local_orders',
236+
fields: {
237+
product: { type: 'string' },
238+
quantity: { type: 'integer' },
239+
},
240+
},
241+
},
242+
{
243+
object: 'local_invoices',
244+
schema: {
245+
name: 'local_invoices',
246+
fields: {
247+
amount: { type: 'float' },
248+
},
249+
},
250+
},
251+
]);
252+
253+
// Verify tables were created
254+
const order = await driver.create('local_orders', { product: 'Gadget', quantity: 3 });
255+
expect(order.product).toBe('Gadget');
256+
257+
const invoice = await driver.create('local_invoices', { amount: 42.0 });
258+
expect(invoice.amount).toBe(42.0);
259+
});
260+
230261
// ── Raw Execution ────────────────────────────────────────────────────────
231262

232263
it('should execute raw SQL', async () => {

0 commit comments

Comments
 (0)