Fix upsert ON CONFLICT: missing :identifier expr + numbered parameters#95
Open
AlanMcCann wants to merge 1 commit intoocean:mainfrom
Open
Fix upsert ON CONFLICT: missing :identifier expr + numbered parameters#95AlanMcCann wants to merge 1 commit intoocean:mainfrom
AlanMcCann wants to merge 1 commit intoocean:mainfrom
Conversation
Two bugs in SQL generation that cause "near ?: syntax error" on any
INSERT ... ON CONFLICT DO UPDATE query:
1. Missing :identifier expression handler. Ecto generates ON CONFLICT
UPDATE clauses using fragment expressions containing {:identifier, _,
["column_name"]} tuples. Without a matching expr/3 clause, these fall
through to the catch-all which returns bare "?", producing invalid SQL
like: SET "col" = EXCLUDED.?
2. Bare ? parameter placeholders. SQLite requires numbered positional
parameters (?1, ?2, ...) when a statement contains multiple parameter
groups (INSERT values + ON CONFLICT UPDATE). The ecto_sqlite3 adapter
already uses this pattern.
Fixes all upsert operations including Ash Framework's upsert? actions.
Contributor
WalkthroughThe LibSQL adapter's INSERT SQL generation has been refactored to use numbered positional bind parameters ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/ecto/adapters/libsql/connection.ex`:
- Around line 1184-1189: The IN with a bound list branch still emits bare "?"
placeholders; update the clause handling the pattern {:in, _, [left, {:^, _, [_,
length]}]} inside expr/3 to emit numbered placeholders like the generic {:^, [],
[ix]} clause does: generate a sequence of "?" + Integer.to_string(start_index)
through "?" + Integer.to_string(start_index + length - 1) (matching the same
numbering scheme used by expr({:^,[],[ix]},...)), join them with ", " and return
the full parenthesized placeholder list so SQLite gets ?1, ?2, ... for IN ^list.
Ensure you reference and reuse the same index base/offset logic used by
expr({:^,...}) so numbering remains consistent across other parameter groups.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f519f1b-e1b3-479f-8e81-0e161b5058ae
📒 Files selected for processing (1)
lib/ecto/adapters/libsql/connection.ex
Comment on lines
+1184
to
1189
| # Parameter placeholder - use numbered parameters (?1, ?2, ...). | ||
| # SQLite requires numbered parameters when a statement has multiple | ||
| # parameter groups (e.g., INSERT values + ON CONFLICT UPDATE). | ||
| defp expr({:^, [], [ix]}, _sources, _query) do | ||
| [?? | Integer.to_string(ix + 1)] | ||
| end |
Contributor
There was a problem hiding this comment.
Finish the numbered-placeholder pass for IN ^list.
This change only renumbers the generic {:^, ...} case. The dedicated {:in, _, [left, {:^, _, [_, length]}]} branch below still expands to bare ? placeholders, so queries that use IN ^list can still hit SQLite's numbered-parameter requirement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/ecto/adapters/libsql/connection.ex` around lines 1184 - 1189, The IN with
a bound list branch still emits bare "?" placeholders; update the clause
handling the pattern {:in, _, [left, {:^, _, [_, length]}]} inside expr/3 to
emit numbered placeholders like the generic {:^, [], [ix]} clause does: generate
a sequence of "?" + Integer.to_string(start_index) through "?" +
Integer.to_string(start_index + length - 1) (matching the same numbering scheme
used by expr({:^,[],[ix]},...)), join them with ", " and return the full
parenthesized placeholder list so SQLite gets ?1, ?2, ... for IN ^list. Ensure
you reference and reuse the same index base/offset logic used by expr({:^,...})
so numbering remains consistent across other parameter groups.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two bugs in SQL generation that cause "near ?: syntax error" on any INSERT ... ON CONFLICT DO UPDATE query:
Missing :identifier expression handler. Ecto generates ON CONFLICT UPDATE clauses using fragment expressions containing {:identifier, _, ["column_name"]} tuples. Without a matching expr/3 clause, these fall through to the catch-all which returns bare "?", producing invalid SQL like: SET "col" = EXCLUDED.?
Bare ? parameter placeholders. SQLite requires numbered positional parameters (?1, ?2, ...) when a statement contains multiple parameter groups (INSERT values + ON CONFLICT UPDATE). The ecto_sqlite3 adapter already uses this pattern.
Fixes all upsert operations including Ash Framework's upsert? actions.
Summary by CodeRabbit
Bug Fixes