Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG]: onConflictDoNothing doesn't handle WHERE clause correctly (for partial indexes/unique constraints) #1628

Closed
aseemk opened this issue Dec 10, 2023 · 1 comment · May be fixed by dennis-adrian/glitter-nextjs#87 or YuraGB/project_be#24
Labels
bug Something isn't working

Comments

@aseemk
Copy link

aseemk commented Dec 10, 2023

What version of drizzle-orm are you using?

0.28.6

What version of drizzle-kit are you using?

No response

Describe the Bug

I have a users table with email and deleted_at columns. And I have a UNIQUE INDEX ON (email) WHERE deleted_at IS NULL, meaning there can only be one active/non-deleted user record per email at a time.

If I try to write a Drizzle query to try and insert while checking for this:

const [user] = await db
  .insert(users)
  .values({
    email,
    ...data,
  })
  .onConflictDoNothing({
    target: [users.email],
    where: isNull(users.deletedAt),
  })
  .returning();

Drizzle outputs/executes the following invalid SQL (e.g. Postgres rejects this query):

insert into "users" (...)
values (...)
on conflict ("email") do nothing
where "users"."deleted_at" is null

The correct SQL would be:

insert into "users" (...)
values (...)
on conflict ("email") where "users"."deleted_at" is null
do nothing

I see this change was made in pull #651 in response to issue #525, which was a valid request for "on conflict do update" queries. But this isn't valid for "on conflict do nothing" queries. No WHERE clause is allowed following DO NOTHING:

https://www.postgresql.org/docs/current/sql-insert.html
https://www.sqlite.org/images/syntax/upsert-clause.gif

I see that issue #1302 has been filed as a feature request to support partial indexes with "on conflict do update" queries. I wanted to file this bug report as I think #651 is both a regression for "on conflict do insert" queries, and clearly incorrect to offer a where clause in the onConflictDoNothing config and not use it to support partial indexes.

I think this is an easy fix/revert for onConflictDoInsert and I'd be happy to PR that. But how would you like to handle onConflictDoUpdate? I agree with #1302 that Drizzle probably needs to provide two separate where configs now, to support both partial indexes and selective updates. Maybe the following?

.onConflictDoUpdate({
  target: IndexColumn | IndexColumn[],
  targetWhere?: SQL,
  set: ...,
  setWhere?: SQL,
})

That'd be a breaking API change, so perhaps you continue to accept where as a proxy for setWhere for now.

@aseemk aseemk added the bug Something isn't working label Dec 10, 2023
@aseemk
Copy link
Author

aseemk commented Dec 10, 2023

I quickly made this change in a fork to unblock ourselves:

main...hiportola:drizzle-orm:portola/fix-on-conflict-do-nothing-where

It both fixes the bug for insert queries, and separates out targetWhere and setWhere for update queries (preserving but deprecating where as a proxy for setWhere for back-compat).

I haven't yet done all the work to submit a PR, nor updated any other drivers (or their tests) besides pg-core, etc. (will try to do some of this later), but wanted to share this quickly both if it helps anyone else and if you all have any immediate feedback. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
1 participant