Support advanced UPSERT features in InsertQuery#98
Conversation
…auses) - Updated InsertQuery.OnConflict to accept schema.Expression targets. - Added OnConstraint, TargetWhere, Where, and Set methods to InsertConflictBuilder. - Implemented dialect-aware rendering for EXCLUDED (Postgres/SQLite) and VALUES (MySQL). - Supported optional targets for ON CONFLICT DO NOTHING on Postgres/SQLite. - Added comprehensive tests for new UPSERT capabilities. 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. |
Greptile SummaryThis PR significantly extends Rain ORM's UPSERT capabilities, adding
Confidence Score: 3/5The conflict-clause compiler has two correctness gaps that produce invalid SQL at runtime for reachable input combinations without returning any error. Two distinct code paths produce silently broken SQL: NullCheckExpr inside a TargetWhere emits a table-qualified column name that PostgreSQL rejects, and TargetWhere used without any conflict targets generates ON CONFLICT WHERE ... DO NOTHING which is syntactically invalid in both PostgreSQL and SQLite. Neither case is caught by the new test suite. pkg/rain/query_compile.go (NullCheckExpr context propagation) and pkg/rain/query_insert.go (targetWhere guard) need fixes before merging.
|
| Filename | Overview |
|---|---|
| pkg/rain/query_insert.go | Significantly extends UPSERT support with OnConstraint, TargetWhere, Set, Where, and DoUpdate; contains a bug where targetWhere without conflict targets silently generates invalid SQL instead of returning an error. |
| pkg/rain/query_compile.go | Adds unqualified/qualified column rendering and context propagation through expressions; NullCheckExpr was missed and still uses the default (qualified) context, producing invalid SQL in conflict target WHERE clauses. |
| pkg/rain/upsert_advanced_test.go | New test file covering OnConstraint, TargetWhere, Set, Where, DoUpdate, and MySQL rejection paths; does not cover TargetWhere without targets or NullCheckExpr in TargetWhere, leaving the two new bugs untested. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[writeConflictClause] --> B{conflict == nil?}
B -- yes --> Z[return nil]
B -- no --> C{action == None?}
C -- yes --> ERR1[error: action required]
C -- no --> D{dialect?}
D -- mysql --> E{targets > 0?}
E -- yes --> ERR2[error: MySQL no targets]
E -- no --> F{constraint != ''?}
F -- yes --> ERR3[error: MySQL no constraint]
F -- no --> G{targetWhere != nil?}
G -- yes --> ERR4[error: MySQL no targetWhere]
G -- no --> H{where != nil?}
H -- yes --> ERR5[error: MySQL no where]
H -- no --> I{action?}
I -- DoNothing --> J[ON DUPLICATE KEY UPDATE col=col]
I -- DoUpdateSet --> K[ON DUPLICATE KEY UPDATE ...]
D -- postgres/sqlite --> L{constraint != ''?}
L -- yes --> M{dialect != postgres?}
M -- yes --> ERR6[error: SQLite no constraint]
M -- no --> N[ON CONFLICT ON CONSTRAINT name]
L -- no --> O[ON CONFLICT]
O --> P{targets > 0?}
P -- yes --> Q[write target col list]
P -- no --> R{action == DoUpdateSet?}
R -- yes --> ERR7[error: DO UPDATE needs target]
Q --> S{targetWhere != nil?}
S -- yes --> T[write WHERE pred]
S -- no --> U{action?}
T --> U
R -- no --> U
N --> U
U -- DoNothing --> V[DO NOTHING]
U -- DoUpdateSet --> W[DO UPDATE SET ...]
W --> X{where != nil?}
X -- yes --> Y[write WHERE pred]
X -- no --> Z2[return nil]
Y --> Z2
V --> Z2
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_compile.go:329-337
**`NullCheckExpr` does not propagate the `unqualified` context**
The `NullCheckExpr` branch still calls `c.writeExpression(value.Expr)` instead of `c.writeExpressionInContext(value.Expr, context)`. Every other compound expression type was updated in this PR to forward the context, but this one was missed. As a result, using a null-check predicate inside `TargetWhere` — e.g., `.TargetWhere(users.Email.IsNull())` — emits a table-qualified name (`"users"."email" IS NULL`) inside the conflict target WHERE clause, which PostgreSQL rejects as invalid syntax.
```suggestion
case schema.NullCheckExpr:
if err := c.writeExpressionInContext(value.Expr, context); err != nil {
return err
}
```
### Issue 2 of 2
pkg/rain/query_insert.go:637-646
**`targetWhere` without any conflict targets produces invalid SQL**
When a caller chains `.OnConflict().TargetWhere(pred).DoNothing()` (no column targets, no constraint), the else-branch writes `ON CONFLICT WHERE … DO NOTHING`. Both PostgreSQL and SQLite require the WHERE clause to follow a parenthesised column list — it cannot appear after a bare `ON CONFLICT`. An explicit validation should be added so callers get a clear error instead of silently broken SQL.
```suggestion
if q.conflict.targetWhere != nil {
if len(q.conflict.targets) == 0 {
return errors.New("rain: conflict targetWhere requires at least one conflict target column")
}
ctx.writeString(" WHERE ")
oldUseLiterals := ctx.useLiterals
ctx.useLiterals = true
err := ctx.writeExpressionInContext(q.conflict.targetWhere, expressionContext{unqualified: true})
ctx.useLiterals = oldUseLiterals
if err != nil {
return err
}
}
```
Reviews (2): Last reviewed commit: "feat(rain): enhance UPSERT support (ON C..." | Re-trigger Greptile
…tom assignments) - Added support for named constraints via `OnConstraint` - Added support for partial index targets via `TargetWhere` - Added support for conditional updates via `Where` in conflict builder - Added `Set` and `DoUpdate` for explicit assignments in the update clause - Implemented dialect-aware rendering for excluded values (`EXCLUDED.` vs `VALUES()`) - Fixed table qualification in conflict target lists for PostgreSQL - Added comprehensive tests for all new capabilities across dialects Co-authored-by: cungminh2710 <8063319+cungminh2710@users.noreply.github.com>
|
@jules Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes. Issue 1 of 2pkg/rain/query_compile.go:329-337 The Issue 2 of 2pkg/rain/query_insert.go:637-646 When a caller chains |
This PR significantly enhances Rain ORM's UPSERT capabilities, bringing it closer to feature parity with Drizzle ORM.
Key features implemented:
ON CONSTRAINTsupport: Target a specific named constraint for conflicts.WHEREclauses: Support for partial index targets (e.g.,ON CONFLICT (email) WHERE active).WHEREclauses: Support for conditional updates (e.g.,DO UPDATE SET ... WHERE status = 'active')..Set(col, val).ON DUPLICATE KEY UPDATEfor MySQL (includingVALUES(col)for excluded values) while rejecting unsupported features likeON CONSTRAINTorWHEREclauses on MySQL.OnConflictnow accepts anyschema.Expression, allowing for functional indexes or complex targets.Internal improvements:
insertConflictClauseto use expressions and assignments.excludedColumninternal expression for clean dialect-specific rendering of excluded row values.compileContextwithwriteColumnvswriteQualifiedColumnto handle Postgres's restriction on qualified names in conflict target lists.PR created automatically by Jules for task 28976301087815799 started by @cungminh2710