Implement InsertQuery.Ignore() and improve targetless DoNothing()#157
Implement InsertQuery.Ignore() and improve targetless DoNothing()#157cungminh2710 wants to merge 1 commit into
Conversation
…ing() Implemented .Ignore() for InsertQuery to support dialect-native ignore semantics: - MySQL: INSERT IGNORE - SQLite: INSERT OR IGNORE - PostgreSQL: Returns error (use .OnConflict().DoNothing() instead) Improved targetless .OnConflict().DoNothing() support: - PostgreSQL: ON CONFLICT DO NOTHING - SQLite: Renders as INSERT OR IGNORE - MySQL: Renders as INSERT IGNORE (replaces brittle no-op update) Cleaned up technical debt by removing mysqlConflictNoopColumn helper. Added comprehensive tests for all dialects. Co-authored-by: cungminh2710 <8063319+cungminh2710@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Greptile SummaryThis PR adds an
Confidence Score: 3/5The The switch from pkg/rain/query_insert.go — specifically the MySQL
|
| Filename | Overview |
|---|---|
| pkg/rain/query_insert.go | Adds useIgnore field, Ignore() method, and refactors writeInsertInto/writeConflictClause. The MySQL OnConflict().DoNothing() path now emits INSERT IGNORE instead of the no-op ON DUPLICATE KEY UPDATE idiom, silently broadening error suppression beyond duplicate-key violations. |
| pkg/rain/query_insert_test.go | Adds unit tests for Ignore(), targetless DoNothing() for PostgreSQL and SQLite, and updated MySQL DoNothing() SQL expectation. Coverage is adequate for the happy paths but lacks tests for non-duplicate-key error propagation on MySQL. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[InsertQuery.compile] --> B{selectQuery?}
B -- yes --> C[writeSelectSQL]
B -- no --> D[writeValuesSQL]
C --> E[writeInsertInto]
D --> E
E --> F{renderIgnore?}
F -- useIgnore=true OR\nMySQL+DoNothing OR\nSQLite+targetless+DoNothing --> G{dialect}
G -- mysql --> H[INSERT IGNORE INTO]
G -- sqlite --> I[INSERT OR IGNORE INTO]
G -- postgres & useIgnore --> J[error: use OnConflict DoNothing]
G -- default & useIgnore --> K[error: not implemented]
F -- false --> L[INSERT INTO]
H & I & L --> M[writeTableName]
M --> N[writeConflictClause]
N --> O{dialect}
O -- mysql+DoNothing --> P[return nil\nhandled by INSERT IGNORE]
O -- mysql+DoUpdateSet --> Q[ON DUPLICATE KEY UPDATE ...]
O -- postgres+targetless+DoNothing --> R[ON CONFLICT DO NOTHING]
O -- sqlite+targetless+DoNothing --> S[return nil\nhandled by INSERT OR IGNORE]
O -- postgres/sqlite+targeted --> T[ON CONFLICT col DO NOTHING/UPDATE]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[InsertQuery.compile] --> B{selectQuery?}
B -- yes --> C[writeSelectSQL]
B -- no --> D[writeValuesSQL]
C --> E[writeInsertInto]
D --> E
E --> F{renderIgnore?}
F -- useIgnore=true OR\nMySQL+DoNothing OR\nSQLite+targetless+DoNothing --> G{dialect}
G -- mysql --> H[INSERT IGNORE INTO]
G -- sqlite --> I[INSERT OR IGNORE INTO]
G -- postgres & useIgnore --> J[error: use OnConflict DoNothing]
G -- default & useIgnore --> K[error: not implemented]
F -- false --> L[INSERT INTO]
H & I & L --> M[writeTableName]
M --> N[writeConflictClause]
N --> O{dialect}
O -- mysql+DoNothing --> P[return nil\nhandled by INSERT IGNORE]
O -- mysql+DoUpdateSet --> Q[ON DUPLICATE KEY UPDATE ...]
O -- postgres+targetless+DoNothing --> R[ON CONFLICT DO NOTHING]
O -- sqlite+targetless+DoNothing --> S[return nil\nhandled by INSERT OR IGNORE]
O -- postgres/sqlite+targeted --> T[ON CONFLICT col DO NOTHING/UPDATE]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
pkg/rain/query_insert.go:836-839
**MySQL `INSERT IGNORE` silences more errors than the old no-op idiom**
The old `ON DUPLICATE KEY UPDATE id = id` approach surfaces every error _except_ a duplicate-key violation. `INSERT IGNORE` converts **all** MySQL errors — data truncation, out-of-range values, FK violations, generated-column failures, etc. — into warnings and silently discards the affected row. Any existing caller of `OnConflict().DoNothing()` on MySQL that relied on non-duplicate-key errors being propagated will now silently swallow those errors without any indication that something went wrong. The PR description does not call out this behavioral difference, and there are no tests that verify non-duplicate-key errors still surface.
### Issue 2 of 2
pkg/rain/query_insert.go:745-776
**`Ignore()` + `OnConflict().DoUpdateSet()` combination is not validated on MySQL**
When `useIgnore` is `true` and the conflict action is `insertConflictActionDoUpdateSet`, `writeInsertInto` writes `INSERT IGNORE INTO`, then `writeConflictClause` appends `ON DUPLICATE KEY UPDATE ...`. The resulting SQL — `INSERT IGNORE INTO … ON DUPLICATE KEY UPDATE col = ?` — is syntactically valid in MySQL but semantically contradictory: the `ON DUPLICATE KEY UPDATE` clause specifically handles duplicate-key conflicts, while `INSERT IGNORE` silently discards all other errors. A user who calls `.Ignore().OnConflict().DoUpdateSet(col)` likely expects an error, not silently contradictory SQL. Consider adding a validation that `useIgnore` and `DoUpdateSet` are mutually exclusive for MySQL.
Reviews (1): Last reviewed commit: "feat(rain): add Ignore() to InsertQuery ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if q.conflict.action == insertConflictActionDoNothing { | ||
| noopColumn, err := mysqlConflictNoopColumn(q.table) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| ctx.writeString(" ON DUPLICATE KEY UPDATE ") | ||
| ctx.writeQuotedIdentifier(noopColumn.Name) | ||
| ctx.writeString(" = ") | ||
| ctx.writeQuotedIdentifier(noopColumn.Name) | ||
| // Handled by INSERT IGNORE in writeInsertInto | ||
| return nil | ||
| } |
There was a problem hiding this comment.
INSERT IGNORE silences more errors than the old no-op idiom
The old ON DUPLICATE KEY UPDATE id = id approach surfaces every error except a duplicate-key violation. INSERT IGNORE converts all MySQL errors — data truncation, out-of-range values, FK violations, generated-column failures, etc. — into warnings and silently discards the affected row. Any existing caller of OnConflict().DoNothing() on MySQL that relied on non-duplicate-key errors being propagated will now silently swallow those errors without any indication that something went wrong. The PR description does not call out this behavioral difference, and there are no tests that verify non-duplicate-key errors still surface.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/query_insert.go
Line: 836-839
Comment:
**MySQL `INSERT IGNORE` silences more errors than the old no-op idiom**
The old `ON DUPLICATE KEY UPDATE id = id` approach surfaces every error _except_ a duplicate-key violation. `INSERT IGNORE` converts **all** MySQL errors — data truncation, out-of-range values, FK violations, generated-column failures, etc. — into warnings and silently discards the affected row. Any existing caller of `OnConflict().DoNothing()` on MySQL that relied on non-duplicate-key errors being propagated will now silently swallow those errors without any indication that something went wrong. The PR description does not call out this behavioral difference, and there are no tests that verify non-duplicate-key errors still surface.
How can I resolve this? If you propose a fix, please make it concise.
| func (q *InsertQuery) writeInsertInto(ctx *compileContext) error { | ||
| dialectName := ctx.dialect.Name() | ||
|
|
||
| renderIgnore := q.useIgnore | ||
| if q.conflict != nil && q.conflict.action == insertConflictActionDoNothing { | ||
| if dialectName == "mysql" { | ||
| renderIgnore = true | ||
| } else if dialectName == "sqlite" && len(q.conflict.columns) == 0 && q.conflict.constraint == "" { | ||
| renderIgnore = true | ||
| } | ||
| } | ||
|
|
||
| if renderIgnore { | ||
| switch dialectName { | ||
| case "mysql": | ||
| ctx.writeString("INSERT IGNORE INTO ") | ||
| case "sqlite": | ||
| ctx.writeString("INSERT OR IGNORE INTO ") | ||
| case "postgres": | ||
| if q.useIgnore { | ||
| return errors.New("rain: .Ignore() is not supported for PostgreSQL; use .OnConflict().DoNothing() instead") | ||
| } | ||
| ctx.writeString("INSERT INTO ") | ||
| default: | ||
| if q.useIgnore { | ||
| return fmt.Errorf("rain: .Ignore() is not implemented for %s dialect", dialectName) | ||
| } | ||
| ctx.writeString("INSERT INTO ") | ||
| } | ||
| } else { | ||
| ctx.writeString("INSERT INTO ") | ||
| } |
There was a problem hiding this comment.
Ignore() + OnConflict().DoUpdateSet() combination is not validated on MySQL
When useIgnore is true and the conflict action is insertConflictActionDoUpdateSet, writeInsertInto writes INSERT IGNORE INTO, then writeConflictClause appends ON DUPLICATE KEY UPDATE .... The resulting SQL — INSERT IGNORE INTO … ON DUPLICATE KEY UPDATE col = ? — is syntactically valid in MySQL but semantically contradictory: the ON DUPLICATE KEY UPDATE clause specifically handles duplicate-key conflicts, while INSERT IGNORE silently discards all other errors. A user who calls .Ignore().OnConflict().DoUpdateSet(col) likely expects an error, not silently contradictory SQL. Consider adding a validation that useIgnore and DoUpdateSet are mutually exclusive for MySQL.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/query_insert.go
Line: 745-776
Comment:
**`Ignore()` + `OnConflict().DoUpdateSet()` combination is not validated on MySQL**
When `useIgnore` is `true` and the conflict action is `insertConflictActionDoUpdateSet`, `writeInsertInto` writes `INSERT IGNORE INTO`, then `writeConflictClause` appends `ON DUPLICATE KEY UPDATE ...`. The resulting SQL — `INSERT IGNORE INTO … ON DUPLICATE KEY UPDATE col = ?` — is syntactically valid in MySQL but semantically contradictory: the `ON DUPLICATE KEY UPDATE` clause specifically handles duplicate-key conflicts, while `INSERT IGNORE` silently discards all other errors. A user who calls `.Ignore().OnConflict().DoUpdateSet(col)` likely expects an error, not silently contradictory SQL. Consider adding a validation that `useIgnore` and `DoUpdateSet` are mutually exclusive for MySQL.
How can I resolve this? If you propose a fix, please make it concise.
1 participant
This PR adds the
.Ignore()method toInsertQuery, bringing Rain ORM closer to feature parity with Drizzle ORM. It also improves the implementation of targetlessON CONFLICT DO NOTHINGacross all supported dialects.Key changes:
Ignore()method toInsertQuery.Ignore()with MySQL'sINSERT IGNOREand SQLite'sINSERT OR IGNORE..Ignore(), guiding users toward the idiomatic.OnConflict().DoNothing().writeConflictClauseto allow targetlessDoNothing()for PostgreSQL and SQLite.OnConflict().DoNothing()to utilizeINSERT IGNORElogic, providing better performance and cleaner SQL than the previous no-op assignment hack.mysqlConflictNoopColumnhelper.PR created automatically by Jules for task 9814084095486085146 started by @cungminh2710