kysely-org / kysely

A type-safe typescript SQL query builder
https://kysely.dev
MIT License
10.8k stars 275 forks source link

Update statements with joins don't seem to work #192

Open bduff9 opened 2 years ago

bduff9 commented 2 years ago

Hello! Just switched from Prisma and loving the library so far. Just ran into an issue where I'm hoping there's a workaround or I'm doing something wrong.

Basically, I have an existing MySQL statement that reads:

update OverallMV O join OverallMV O2 on O.Rank = O2.Rank and O.UserID <> O2.UserID set O.Tied = true

So I recreated it in Kysely as:

await trx.updateTable("OverallMV as O").innerJoin(
      "OverallMV as O2",
      (join) =>
        join
          .onRef("O.Rank", "=", "O2.Rank")
          .onRef("O.UserID", "<>", "O2.UserID"),
    ).set({ Tied: 1 }).execute();

However, this fails with a ER_PARSE_ERROR since it puts the set before the join:

update `OverallMV` as `O` set `Tied` = 1 inner join `OverallMV` as `O2` on `O`.`Rank` = `O2`.`Rank` and `O`.`UserID` <> `O2`.`UserID`

Any ideas here?

Side note, I'm wondering if this use case was never tested as I would also expect to choose which table to update in the set. So more like .set({ "O.Tied": 1 }), which does not currently work but .set({ Tied: 1 }) does.

igalklebanov commented 2 years ago

Hey, MySQL style UPDATE...JOIN...SET syntax is currently not supported.

The innerJoin method you've invoked is part of UPDATE...SET...FROM...JOIN... syntax.

You can use a subquery for now.. something like this should work:

db.updateTable('OverallMV as O1')
  .set({ Tied: 1 })
  .whereExists((eb) =>
    eb
      .selectFrom('OverallMV as O2')
      .whereRef('O1.Rank', '=', 'O2.Rank')
      .whereRef('O1.UserID', '<>', 'O2.UserID')
      .select(['O2.Rank', 'O2.UserId'])
  )
  .execute()

Let me know if that did the trick for you..

bduff9 commented 2 years ago

@igalklebanov Unfortunately that won't work. Using what you wrote gives the following error:

{
  code: 'ER_UPDATE_TABLE_USED',
  errno: 1093,
  sqlState: 'HY000',
  sqlMessage: "You can't specify target table 'O' for update in FROM clause",
  sql: 'update `OverallMV` as `O` set `Tied` = 1 where exists (select 1 as `Found` from `OverallMV` as `O2` where `O`.`Rank` = `O2`.`Rank` and `O`.`UserID` <> `O2`.`UserID`)'
}

Doing a quick search on SO says to use inner join to fix: https://stackoverflow.com/a/45498

There is a possibility to try to nest the where exists deeper which I will try in a bit but would love to have the update join option.

igalklebanov commented 2 years ago

Damn, how about a WITH...UPDATE...SET...WHERE ?

koskimas commented 2 years ago
.set({ "O.Tied": 1 })

This would only work on MySQL AFAIK, so I haven't supported that syntax.

koskimas commented 2 years ago
"You can't specify target table 'O' for update in FROM clause"

Yeah, MySQL has that weird limitation. This should work:

db.updateTable('OverallMV as O1')
  .set({ Tied: 1 })
  .whereExists((eb) =>
    eb.selectFrom((eb) => eb
      .selectFrom('OverallMV as O2')
      .whereRef('O1.Rank', '=', 'O2.Rank')
      .whereRef('O1.UserID', '<>', 'O2.UserID')
      .select(['O2.Rank', 'O2.UserId'])
      .as('O3')
    ).selectAll('O3')
  )
  .execute()
bduff9 commented 2 years ago

Damn, how about a WITH...UPDATE...SET...WHERE ?

@igalklebanov This doesn't work since the WITH is read only.

bduff9 commented 2 years ago
.set({ "O.Tied": 1 })

This would only work on MySQL AFAIK, so I haven't supported that syntax.

@koskimas I hear you, however, part of why I love Kysely so far and am trying to convince my team to convert is its simplicity in doing SQL operations. Your blog post calls out how it implements things that might not be available on the specific DB you are working on in the section on on conflict:

However, unlike knex, Kysely doesn’t try to add an abstraction layer on top of SQL. With Kysely, “what you see is what you get”.

The workaround you posted I was also able to generate yesterday to work, however, it is very ugly and difficult to understand what is happening for devs reading this code:

await trx.updateTable("OverallMV as O").set({ Tied: 1 }).whereExists((qb) =>
      qb.selectFrom((qb) =>
        qb.selectFrom("OverallMV as O2")
          .select([sql.literal(1).as("Found")])
          .whereRef("O.Rank", "=", "O2.Rank")
          .whereRef("O.UserID", "<>", "O2.UserID").as("INNER")
      ).select("INNER.Found")
    ).execute();

I would hope that updates with joins is something that is considered to be supported as it is not uncommon on MySQL due to not being able to use where exists over the same table being updated.

Thanks for your help in the meantime, I appreciate the quick response!

koskimas commented 2 years ago

It's ok to add custom methods that don't exist on other dialects. People don't call them accidentally (well at least not as easily). But the set method is used on all dialects and if you'd get autocompletion for table.column fields, people would definitely try to use them on all dialects.

koskimas commented 2 years ago

I think what @igalklebanov suggested regarding WITH is this:

trx
  .with('someGoodName', (qb) => qb
    .selectFrom("OverallMV as O")
    .innerJoin("OverallMV as O2", (join) => join
      .onRef("O.Rank", "=", "O2.Rank")
      .onRef("O.UserID", "<>", "O2.UserID")
    )
    .select('ID')
  )
  .updateTable("OverallMV")
  .whereExists((eb) => eb.selectFrom("someGoodName").select("ID"))
  .set({ Tied: 1 })
koskimas commented 2 years ago

I'll definitely consider supporting joins like you suggested (and I have considered it), but it can be very difficult because of the other way of using joins in update statements (in a from clause).

igalklebanov commented 2 years ago

@koskimas following Kysely's philosophy of what you invoke is what you get... we could simply support this by checking if consumer invoked .from(...) - she didn't? that's UPDATE...JOIN...SET, she did? that's UPDATE...SET...FROM...JOIN.

Wdyt?

koskimas commented 2 years ago

That wouldn't work with the types. Correct me if I'm wrong, but on MySQL you can update all joined tables? So we would need to append to UT of UpdateQueryBuilder<DB, UT, TB, O> in "before from joins" and to TB in "after from joins". We'd need to add a new type argument just to indicate if from has been called.

igalklebanov commented 2 years ago

You're right.

Types are too complicated and its surprising to consumers (join and set switch places in compiled sql because of a from existing or not).

I think expanding .updateTable(...) as follows:

  1. Add an overload that accepts an array of table references to support update table t0, t1, t2, ... syntax.
db.updateTable(['person', 'pet'])
  .set({ 
    'person.status': 'Cat lady', 
    'pet.owner_id': uuid,
  })
  .where('person.id', '=', uuid)
  .where('pet.owner_id', 'is', null)
  .where('pet.species', '=', 'cat')
  .execute()
  1. Add an overload that accepts a 2nd argument - A builder factory with join methods, to support update table t0 inner join t1 ... left join t2 ... syntax.
db.updateTable('person', (jb) => jb.innerJoin('pet', 'pet.owner_id', 'person.id'))
  .set({
    'person.status': 'Proud dog parent',
    'pet.status': 'Happy pup',
  })
  .where('pet.species', '=', 'dog')
  .execute()

Would be aligned with consumer's expectations of affecting the query's main clause, and will make "downstream" types consistent and simpler.

CanRau commented 1 year ago

Very surprising that this doesn't work but I get the complications, Typescript can be a curse sometimes 😅

Any updates on @igalklebanov's suggestion? I'm specifically interested in

db.updateTable('person', (jb) => jb.innerJoin('pet', 'pet.owner_id', 'person.id'))

and think this looks like a good option 👍🏼

Would be amazing if this could be considered if it works like that.

setvik commented 9 months ago

+1 for @igalklebanov 's suggestion to support an array of table references:

db.updateTable(['person', 'pet'])
  .set({ 
    'person.status': 'Cat lady', 
    'pet.owner_id': uuid,
  })
  .where('person.id', '=', uuid)
  .where('pet.owner_id', 'is', null)
  .where('pet.species', '=', 'cat')
  .execute()
devnomic commented 4 months ago

I currently need something like this, update with join, surprised that kysely still doesn't support it without workaround using different sql query.

koskimas commented 4 months ago

I'm finally adding support for update t1, t2 set t1.foo = ?, t2.bar = ? where t1.x = t2.y queries here.