-
Notifications
You must be signed in to change notification settings - Fork 1
Resolving database upgrade issues with multiple database drivers #965
New 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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -274,12 +274,18 @@ export class ObjectQLPlugin implements Plugin { | |
| continue; | ||
| } | ||
|
|
||
| // Use the physical table name (e.g., 'sys_user') for DDL operations | ||
| // instead of the FQN (e.g., 'sys__user'). ObjectSchema.create() | ||
| // auto-derives tableName as {namespace}_{name}. | ||
| const tableName = obj.tableName || obj.name; | ||
|
|
||
| try { | ||
| await driver.syncSchema(obj.name, obj); | ||
| await driver.syncSchema(tableName, obj); | ||
| synced++; | ||
|
Comment on lines
+277
to
284
|
||
| } catch (e: unknown) { | ||
| ctx.logger.warn('Failed to sync schema for object', { | ||
| object: obj.name, | ||
| tableName, | ||
| driver: driver.name, | ||
| error: e instanceof Error ? e.message : String(e), | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -314,8 +314,14 @@ export class SchemaRegistry { | |||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||
| * Get object by name (FQN or short name with fallback scan). | ||||||||||||||||||||||||||||||
| * For compatibility, tries exact match first, then scans for suffix match. | ||||||||||||||||||||||||||||||
| * Get object by name (FQN, short name, or physical table name). | ||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||
| * Resolution order: | ||||||||||||||||||||||||||||||
| * 1. Exact FQN match (e.g., 'crm__account') | ||||||||||||||||||||||||||||||
| * 2. Short name fallback (e.g., 'account' → 'crm__account') | ||||||||||||||||||||||||||||||
| * 3. Physical table name match (e.g., 'sys_user' → 'sys__user') | ||||||||||||||||||||||||||||||
| * ObjectSchema.create() auto-derives tableName as {namespace}_{name}, | ||||||||||||||||||||||||||||||
| * which uses a single underscore — different from the FQN double underscore. | ||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||
| static getObject(name: string): ServiceObject | undefined { | ||||||||||||||||||||||||||||||
| // Direct FQN lookup | ||||||||||||||||||||||||||||||
|
|
@@ -331,6 +337,15 @@ export class SchemaRegistry { | |||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Fallback: match by physical table name (e.g., 'sys_user' → FQN 'sys__user') | ||||||||||||||||||||||||||||||
| // This bridges the gap between protocol names (SystemObjectName) and FQN. | ||||||||||||||||||||||||||||||
| for (const fqn of this.objectContributors.keys()) { | ||||||||||||||||||||||||||||||
| const resolved = this.resolveObject(fqn); | ||||||||||||||||||||||||||||||
| if (resolved?.tableName === name) { | ||||||||||||||||||||||||||||||
| return resolved; | ||||||||||||||||||||||||||||||
|
Comment on lines
+342
to
+345
|
||||||||||||||||||||||||||||||
| for (const fqn of this.objectContributors.keys()) { | |
| const resolved = this.resolveObject(fqn); | |
| if (resolved?.tableName === name) { | |
| return resolved; | |
| // | |
| // To avoid repeatedly merging objects on every iteration, we derive the | |
| // physical table name directly from the FQN using the convention: | |
| // tableName = `${namespace}_${name}` | |
| // and only resolve the object once we find a match. | |
| for (const fqn of this.objectContributors.keys()) { | |
| const { namespace, shortName } = parseFQN(fqn); | |
| const tableName = `${namespace}_${shortName}`; | |
| if (tableName === name) { | |
| return this.resolveObject(fqn); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -995,12 +995,22 @@ export class SqlDriver implements IDataDriver { | |
| // ── Database helpers ──────────────────────────────────────────────────────── | ||
|
|
||
| protected async ensureDatabaseExists() { | ||
| if (!this.isPostgres) return; | ||
| // SQLite auto-creates database files — no need to check | ||
| if (this.isSqlite) return; | ||
|
|
||
| // Only PostgreSQL and MySQL support programmatic database creation | ||
| if (!this.isPostgres && !this.isMysql) return; | ||
|
|
||
| try { | ||
| await this.knex.raw('SELECT 1'); | ||
| } catch (e: any) { | ||
| if (e.code === '3D000') { | ||
| // PostgreSQL: '3D000' = database does not exist | ||
| // MySQL: 'ER_BAD_DB_ERROR' (errno 1049) = unknown database | ||
| if ( | ||
| e.code === '3D000' || | ||
| e.code === 'ER_BAD_DB_ERROR' || | ||
| e.errno === 1049 | ||
| ) { | ||
| await this.createDatabase(); | ||
| } else { | ||
|
Comment on lines
997
to
1015
|
||
| throw e; | ||
|
|
@@ -1014,19 +1024,40 @@ export class SqlDriver implements IDataDriver { | |
| let dbName = ''; | ||
| const adminConfig = { ...config }; | ||
|
|
||
| if (typeof connection === 'string') { | ||
| const url = new URL(connection); | ||
| dbName = url.pathname.slice(1); | ||
| url.pathname = '/postgres'; | ||
| adminConfig.connection = url.toString(); | ||
| if (this.isPostgres) { | ||
| // PostgreSQL: connect to the 'postgres' maintenance database | ||
| if (typeof connection === 'string') { | ||
| const url = new URL(connection); | ||
| dbName = url.pathname.slice(1); | ||
| url.pathname = '/postgres'; | ||
| adminConfig.connection = url.toString(); | ||
| } else { | ||
| dbName = connection.database; | ||
| adminConfig.connection = { ...connection, database: 'postgres' }; | ||
| } | ||
| } else if (this.isMysql) { | ||
| // MySQL: connect without specifying a database | ||
| if (typeof connection === 'string') { | ||
| const url = new URL(connection); | ||
| dbName = url.pathname.slice(1); | ||
| url.pathname = '/'; | ||
| adminConfig.connection = url.toString(); | ||
| } else { | ||
| dbName = connection.database; | ||
| const { database: _db, ...rest } = connection; | ||
| adminConfig.connection = rest; | ||
| } | ||
| } else { | ||
| dbName = connection.database; | ||
| adminConfig.connection = { ...connection, database: 'postgres' }; | ||
| return; // Unsupported dialect for auto-creation | ||
| } | ||
|
|
||
| const adminKnex = knex(adminConfig); | ||
| try { | ||
| await adminKnex.raw(`CREATE DATABASE "${dbName}"`); | ||
| if (this.isPostgres) { | ||
| await adminKnex.raw(`CREATE DATABASE "${dbName}"`); | ||
| } else if (this.isMysql) { | ||
| await adminKnex.raw(`CREATE DATABASE IF NOT EXISTS \`${dbName}\``); | ||
| } | ||
|
Comment on lines
1054
to
+1060
|
||
| } finally { | ||
| await adminKnex.destroy(); | ||
| } | ||
|
|
||
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.
Changing
resolveObjectName()to returnschema.tableNamemakes the engine’s canonicalobjectidentifier become the physical table name for objects that definetableName. This affects more than driver I/O: hook/middleware matching usescontext.objectstring equality, and action keys are built from the object name, so existing code registering hooks/actions against the FQN (e.g.sys__user) will no longer match when operations run assys_user. Consider keeping FQN as the canonical engine object identifier (for hooks/actions/registry) and passingtableNameonly at the driver boundary (or carrying both values in OperationContext) to avoid a breaking semantic change.