romeerez/orchid-orm

onConflict().merge() doesn't collect primary keys

Closed this issue · 3 comments

According to the documentation:

When no onConflict argument provided, it will automatically collect all table columns that have unique index and use them as a conflict target.

I haven't checked if it actually collects columns with unique index, but it seems to be ignoring primary keys. If that's intended for some reason, I think it should be mentioned in the documentation. If not, that's apparently a bug.

class UserTable extends BaseTable {
  readonly table = "user"

  columns = this.setColumns(t => ({
    name: t.varchar().primaryKey(),
    age: t.integer(),
  }))
}

await db.$query`
  create table "user" (
    name varchar not null primary key,
    age integer not null
  );
`

await db.user.createMany([{ name: "Alice", age: 25 }])
await db.user.createMany([{ name: "Alice", age: 26 }]).onConflict().merge()

generates:

(1.4ms) INSERT INTO "user"("name", "age") VALUES ($1, $2) RETURNING * ['Alice', 25]
(0.7ms) INSERT INTO "user"("name", "age") VALUES ($1, $2) ON CONFLICT () DO UPDATE SET "name" = excluded."name", "age" = excluded."age" RETURNING * ['Alice', 26]
Error: syntax error at or near ")"
  • orchid-orm 1.24.6

The reason is that the primary key is typically autogenerated, it's either a serial or uuid, and can't cause conflicts.
However, other columns may be unique and may be conflicted.

For example, the user table has an autogenerated uuid pkey and a unique email column. In this case, "on conflict" will be for email.

But if the user also has a unique username, it's a problem because ON CONFLICT does not support multiple independently unique columns, it won't work.

ON CONFLICT DO NOTHING doesn't require unique columns, so in the case of onConflict().ignore() it's okay to leave it empty.

But for onConflict().merge() ORM has to make guesses and cannot always be correct, thus it is better to require columns set for merge. And need to redesign it somehow so that on the type level it's optional for ignore but required for merge.

Thinking of changing it to:

// columns are optional for ignore.
db.table.createMany(...).onConflictIgnore()

// onConfict() will require columns list explicitly
db.table.createMany(...).onConflict(['name']).merge()

// merge only one column
db.table.createMany(...).onConflict(['name']).merge('someColumn')

// and let it be a separate `set` for concrete values
db.table.createMany(...).onConflict(['name']).set({ someColumn: 123 })

I released a large update that includes a change for onConflict, here are updated docs. Now onConflict is type-safe: accepts only unique columns or pkeys, and also can accept a constraint name.

Check out new breaking changes document, the changes are mostly related to unique columns and are inspired by this issue, thanks for the inspiration.

I further updated onConflict, see a summary in the breaking changes.

Added new ability to merge all except some

onConflict(...).merge({ except: ['some', 'columns'] })