Chages#2
Conversation
Signed-off-by: Lwazi Ciniso Dlamini <teamallghostanon@gmail.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcbdbc3f72
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const rows = await db.query<any>(`PRAGMA table_info(${table});`); | ||
| return new Set(rows.map((r: any) => r.name)); | ||
| case DBType.MySQL: | ||
| const cols = await db.query<any>(`SHOW COLUMNS FROM \`${table}\`;`); |
There was a problem hiding this comment.
autoMigrate calls getExistingColumns before deciding whether to create a table, but the MySQL branch runs SHOW COLUMNS FROM directly; on a brand-new model table this statement errors instead of returning an empty set, so migration aborts before reaching CREATE TABLE. This breaks first-time auto-migration on MySQL databases.
Useful? React with 👍 / 👎.
| if (dialect === "mysql") { | ||
| return `ALTER TABLE DROP INDEX \`${indexName}\``; | ||
| } |
There was a problem hiding this comment.
The MySQL drop-index SQL is missing the table identifier (ALTER TABLE DROP INDEX ...), which is invalid syntax in MySQL. Whenever autoMigrate tries to remove an obsolete index, the migration will fail at runtime instead of converging schema state.
Useful? React with 👍 / 👎.
| const whereParts: string[] = ["id = ?"]; | ||
| const queryParams = [...keys.map((k) => (entity as any)[k]), id]; | ||
|
|
There was a problem hiding this comment.
In _update, the SET clause is built from entityWithTimestamps, but parameter values are read from the original entity. When updatedAt is auto-injected, its placeholder receives undefined, so timestamp columns are not updated correctly (and can violate NOT NULL constraints) despite being included in the SQL.
Useful? React with 👍 / 👎.
| const lockColIdx = keys.findIndex( | ||
| (k) => k === this.optimisticLockField, | ||
| ); | ||
| if (lockColIdx !== -1) { | ||
| queryParams[lockColIdx] = newLockVal; |
There was a problem hiding this comment.
The optimistic-lock field is assigned a new value only if it was already present in the user-provided update keys; otherwise keys/setClause omit it and the row version never changes. In that common path, concurrent updates with the same prior version can both succeed, defeating optimistic locking conflict detection.
Useful? React with 👍 / 👎.
1 participant
No description provided.