Skip to content

Support advanced UPSERT features in InsertQuery#98

Open
cungminh2710 wants to merge 2 commits into
mainfrom
feat/advanced-upsert-28976301087815799
Open

Support advanced UPSERT features in InsertQuery#98
cungminh2710 wants to merge 2 commits into
mainfrom
feat/advanced-upsert-28976301087815799

Conversation

@cungminh2710
Copy link
Copy Markdown
Contributor

This PR significantly enhances Rain ORM's UPSERT capabilities, bringing it closer to feature parity with Drizzle ORM.

Key features implemented:

  • ON CONSTRAINT support: Target a specific named constraint for conflicts.
  • Conflict Target WHERE clauses: Support for partial index targets (e.g., ON CONFLICT (email) WHERE active).
  • Update WHERE clauses: Support for conditional updates (e.g., DO UPDATE SET ... WHERE status = 'active').
  • Custom Update Assignments: Explicitly set values or expressions in the update clause using .Set(col, val).
  • Dialect-Awareness: Correct mapping to ON DUPLICATE KEY UPDATE for MySQL (including VALUES(col) for excluded values) while rejecting unsupported features like ON CONSTRAINT or WHERE clauses on MySQL.
  • Generic Conflict Targets: OnConflict now accepts any schema.Expression, allowing for functional indexes or complex targets.

Internal improvements:

  • Refactored insertConflictClause to use expressions and assignments.
  • Introduced excludedColumn internal expression for clean dialect-specific rendering of excluded row values.
  • Enhanced compileContext with writeColumn vs writeQualifiedColumn to handle Postgres's restriction on qualified names in conflict target lists.

PR created automatically by Jules for task 28976301087815799 started by @cungminh2710

…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>
@google-labs-jules
Copy link
Copy Markdown
Contributor

👋 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 @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

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-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Jun 1, 2026

Greptile Summary

This PR significantly extends Rain ORM's UPSERT capabilities, adding ON CONSTRAINT, conflict-target WHERE clauses, update-clause WHERE predicates, explicit Set() assignments, and dialect-aware rejection for MySQL and SQLite.

  • query_insert.go: Refactors insertConflictClause from column lists to typed []schema.Expression targets; adds OnConstraint, TargetWhere, Set, Where, and DoUpdate builder methods; routes MySQL through its ON DUPLICATE KEY UPDATE path with correct VALUES() rendering.
  • query_compile.go: Introduces unqualified context flag and a new writeColumn / writeQualifiedColumn split; propagates the flag through all compound expression types so conflict target column lists emit bare names as PostgreSQL requires.
  • upsert_advanced_test.go: Adds end-to-end SQL-generation tests for all new features across Postgres, SQLite, and MySQL dialects.

Confidence Score: 3/5

The 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.

Important Files Changed

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
Loading

Fix All in Codex

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

Comment thread pkg/rain/query_insert.go
Comment thread pkg/rain/query_insert.go
Comment thread pkg/rain/query_insert.go
…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>
@cungminh2710
Copy link
Copy Markdown
Contributor Author

@jules 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.

	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.

		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
			}
		}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant