Allow update/delete with joined tables

csirkeee commented 6 years ago

The missing feature

Right now Diesel doesn't allow for update/delete with joined tables. This seems like an explicit decision based on having

pub trait IntoUpdateTarget: HasTable

Which means the update target must be a query that only touches one table.

But, using multiple tables in update/delete is a feature that both MySQL and Postgres support. (I didn't research the other databases.) So, it would be nice if Diesel could also support it. In this feature request I'll describe how that could look like.

Example code

Update

For example, working with the tables in the test suite, this could be a valid query:

update(
    comments::table
        .inner_join(posts::table.on(posts::id.eq(comments::post_id)))
        .inner_join(users::table.on(users::id.eq(posts::user_id)))
        .filter(users::id.eq(banned_id)))
    .set(comments::text.eq("Banned user"))
    .execute(&connection)
    .unwrap();

This would translate to the following query in MySQL:

UPDATE `comments`
    INNER JOIN `posts` ON `posts`.`id` = `comments`.`post_id`
    INNER JOIN `users` ON `users`.`id` = `posts`.`user_id` 
SET 
    `text` = ?
WHERE
    `users`.`id` = ?;

And into this query on Postgres:

UPDATE "comments"
SET 
    "text" = $1
FROM "posts"
    INNER JOIN "users" ON "users"."id" = "posts"."user_id"
WHERE
    "posts"."id" = "comments"."post_id"
    AND "users"."id" = $2;

Delete

Delete is a similar case:

delete(
    comments::table
        .inner_join(posts::table.on(posts::id.eq(comments::post_id)))
        .inner_join(users::table.on(users::id.eq(posts::user_id)))
        .filter(users::id.eq(banned_id)))
    .execute(&connection)
    .unwrap();

could translate, in MySQL, to:

DELETE FROM `comments`
USING `comments`
    INNER JOIN `posts` ON `posts`.`id` = `comments`.`post_id`
    INNER JOIN `users` ON `users`.`id` = `posts`.`user_id` 
WHERE
    `users`.`id` = ?;

and in Postgres:

DELETE FROM "comments"
USING "posts"
    INNER JOIN "users" ON "users"."id" = "posts"."user_id"
WHERE
    "posts"."id" = "comments"."post_id"
    AND "users"."id" = $1;

(I've checked all SQL to be valid on SQLFiddle.)

Implementation

The main problem with the implementation is that the query builder would need to handle join clauses differently from other filters. Also, the syntax is different for the databases, I don't know how much that complicates things. (It's possible that there exists a syntax that both MySQL and Postgres accept, but I haven't found one.)

Maybe the first part could be handled by using a different syntax in rust, like calling the joins on the update query and not on the table, like this:

update(comments::table.filter(users::id.eq(banned_id)))
    .inner_join(posts::table.on(posts::id.eq(comments::post_id)))
    .inner_join(users::table.on(users::id.eq(posts::user_id)))
    .set(comments::text.eq("Banned user"))
    .execute(&connection)
    .unwrap();

Ten0 commented 4 years ago

Currently we've been using an insert_into on_conflict do_update to workaround this not being implemented.

Bajix commented 3 years ago

Will this be part of Diesel 2.0?

weiznich commented 3 years ago

@Bajix If someone submits a PR till then. I personally do not plan to work on this at least till the 2.0 release.

Bajix commented 3 years ago

Is there a timetable or write up regarding 2.0? I was confused as I had seen previously the source code already had 2.0 and yet was never released, but at the time there wasn’t much written

Sent from my iPhone

On Oct 7, 2020, at 11:28 PM, Georg Semmler notifications@github.com wrote:

@Bajix If someone submits a PR till then. I personally do not plan to work on this at least till the 2.0 release.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

weiznich commented 3 years ago

@Bajix There is no complete timetable or write up for diesel 2.0. As this is a free time project at least I do not give any ETA's other than: When it's done. Before releasing 2.0 I want to fix or at least have a look at least those issues collected in the 2.0 milestone

lermana commented 1 year ago

Hi @weiznich , thanks for all your work on diesel! It's a really great library. I thought I'd ask if there were any updates here? Trying to do a delete (just like what's in the original comment) where I want to join in another table to filter what gets deleted, but it appears I can't currently write a query like that.

I am new to Rust but at some point later in the year I'd love to help you out on this project -- really important for the Rust ecosystem.

Ten0 commented 1 year ago

Hello, There haven't been updates on this topic, it's still pending workforce. As far as I'm concerned we still use the same workaround : https://github.com/diesel-rs/diesel/issues/1478#issuecomment-562113367

I think we make a reasonably good job at tagging the issues we work on so unless proven otherwise you may generally assume that if an issue has seen no comment or mention or closing it hasn't moved.

lermana commented 1 year ago

thanks @Ten0, makes sense. I saw that comment for update -- is there a complement for delete?

Ten0 commented 1 year ago

Not that I know of - you'd have to make a select first then delete with eq_any.

lermana commented 1 year ago

yeah that's what I ended up doing

thomasmost commented 10 months ago

👀 @weiznich I'd be interested in working on this PR if you have any guidance/thoughts

For context, this is the kind of query I'm hoping to execute:


  pub fn update_from_assessments(
    connection: &mut MysqlConnection,
    emissions: &Vec<super::emission::Emission>,
  ) -> Result<(), ()> {
    let transaction_ids = emissions.iter().filter_map(|e| e.transaction_id.clone()).collect();
    // Joining on the assessed emission records, update the corresponding transactions with the assessed values
    // based on each corresponding emission record; also set assessment_pending to false
    diesel::update(
        transaction::table
          .filter(transaction::id.eq_any(transaction_ids))
      )
      .inner_join(emission::table.on(transaction::id.eq(emission::transaction_id
        .assume_not_null())))
      .set((
        transaction::represented_offsets_bill_id.eq(emission::represented_offsets_bill_id),
        transaction::merchant_name.eq(emission::name),
        transaction::merchant_org_id.eq(emission::merchant_org_id),
        transaction::category_id.eq(emission::category_id),
        transaction::emission_id.eq(emission::id.nullable()),
        transaction::assessment_pending.eq(false),
      ))
      .execute(connection)
      .is_ok()
  }

weiznich commented 10 months ago

@thomasmost We don't have a design for that yet. Without having checked anything yet I would expect that the following steps need to be done:

Implement InternalJoinDsl for UpdateStatement
Make that return a custom query dsl node that allows to provide different QueryFragment implementations for mysql and postgres
Provide these query dsl node implementations (Might require additional
Write tests
Write compile-fail tests showing that certain invariants are upheld (which ones?)
Research what to do with sqlite (it's fine to say it's not supported there)

I would expect that this is not impossible to do, but likely it's also nothing that can be done in one or two hours.

kevincox commented 1 week ago

For context, this is the kind of query I'm hoping to execute:

It would be even better if the join could happen before the update(...) call. This way it would be able to compose a join+filter that could be reused across select+update+delete. This would be particularly useful for things like access control or soft deletion where you want to limit the set of records for almost every query.

So imagine having the example look like this.

diesel::update(
        transaction::table
            .filter(transaction::id.eq_any(transaction_ids))
            .inner_join(emission::table.on(transaction::id.eq(emission::transaction_id
                .assume_not_null())))
    )
    .set(( /* ... */ ))
    .execute(connection)
    .expect(query_failed)

The idea is that you can extract these common filters. For example in my schema the data is de-normalized so you may have to make a few hops to get to the actual user and check ownership. In fact in some cases I may want to compose these ownership checks themselves. As a quick example of what this could look like:

fn user_widgets(user_id: i64) -> impl IntoUpdateTarget {
    widgets::table
        .inner_join(users::table.on(users::id.eq(widgets::user)))
        .filter(users::id.eq(user_id))
}

pub fn update_widget(
    connection: &mut MysqlConnection,
    authenticated_user: i64,
    widget_id: i64,
) {
    let rows = diesel::update(user_widgets(user_id))
        .filter(widgets::id.eq(widget_id))
        .set(( /* ... */ ))
        .execute(connection)
        .expect("query failed");
    assert_eq!(rows, 1, "widget doesn't exist or isn't owned by authenticated user");
}

Of course this is far from a ready API. IntoUpdateTarget is the current trait used for updates, but this filter should also be useful for SELECT and DELETE statements. That type should also include information that users is joined as it would often be used by the query. But duplicating the join wouldn't be the end of the world.

diesel-rs / diesel