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

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

Open
bduff9 opened this issue Oct 11, 2022 · 17 comments
Open

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

bduff9 opened this issue Oct 11, 2022 · 17 comments
Labels
api Related to library's API enhancement New feature or request mysql Related to MySQL

Comments

@bduff9
Copy link

bduff9 commented Oct 11, 2022

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
Copy link
Member

igalklebanov commented Oct 11, 2022

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

@igalklebanov igalklebanov added mysql Related to MySQL api Related to library's API enhancement New feature or request labels Oct 11, 2022
@bduff9
Copy link
Author

bduff9 commented Oct 11, 2022

@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
Copy link
Member

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

@koskimas
Copy link
Member

koskimas commented Oct 12, 2022

.set({ "O.Tied": 1 })

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

@koskimas
Copy link
Member

koskimas commented Oct 12, 2022

"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
Copy link
Author

bduff9 commented Oct 12, 2022

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

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

@bduff9
Copy link
Author

bduff9 commented Oct 12, 2022

.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
Copy link
Member

koskimas commented Oct 12, 2022

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
Copy link
Member

koskimas commented Oct 12, 2022

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
Copy link
Member

koskimas commented Oct 12, 2022

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
Copy link
Member

igalklebanov commented Oct 12, 2022

@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
Copy link
Member

koskimas commented Oct 13, 2022

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
Copy link
Member

igalklebanov commented Oct 14, 2022

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
Copy link

CanRau commented Nov 11, 2023

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
Copy link

setvik commented Jan 26, 2024

+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

This comment was marked as spam.

@koskimas
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to library's API enhancement New feature or request mysql Related to MySQL
Projects
None yet
Development

No branches or pull requests

6 participants