Repository: diesel-rs/diesel (language: rust)

Issue:
Upsert with where clause Postgres implementation is wrong
## Problem Description

Using `on_conflict(...).with_target(...)` is supposed to limit the rows updated during an upsert, but the implementation is slightly wrong. As a result no row is actually being limited. (code here: https://github.com/diesel-rs/diesel/blob/v2.0.0/diesel/src/query_builder/upsert/on_conflict_target_decorations.rs#L19)

Note, the current implementation actually solves a different problem around index predicate but I don't believe that it was the intention of the original issue (https://github.com/diesel-rs/diesel/issues/2430). 

## Details
For example, looking at the test file (https://github.com/diesel-rs/diesel/blob/master/diesel_tests/tests/debug/mod.rs#L158), we would potentially get back an sql statement that doesn't work as intended. 
```
INSERT INTO "users" ("name", "hair_color") VALUES ($1, $2), ($3, $4) ON CONFLICT ("hair_color") WHERE "users"."hair_color" = $5 DO NOTHING
-- binds ...
```

The `WHERE` clause should actually be moved to the end, otherwise it does nothing. 

For example, according to the postgres documentation (https://www.postgresql.org/docs/current/sql-insert.html), `WHERE` needs to happen after `DO UPDATE SET...`. 
```
-- Don't update existing distributors based in a certain ZIP code
INSERT INTO distributors AS d (did, dname) VALUES (8, 'Anvil Distribution')
    ON CONFLICT (did) DO UPDATE
    SET dname = EXCLUDED.dname || ' (formerly ' || d.dname || ')'
    WHERE d.zipcode <> '21201';
```


## Repro
I tested in a postgresql DB with a trivial example: 
```
create table test(
    name text primary key,
    version int    
);

insert into test values ('a', 5)

-- this works as intended, row is not updated
insert into test ("name", "version") values ('a', 3) on conflict ("name") 
do update set "version" = excluded."version" where test."version" < 1;


-- moving where right after on conflict is wrong. row will be updated regardless what the condition is
insert into test ("name", "version") values ('a', 3) on conflict ("name") where test."version" < 1
do update set "version" = excluded."version" 




Discussion / hints:
Thanks for the bug report. To clarify that: This was correct with the 1.4 release? Or was this always broken?

The relevant impl is here:

https://github.com/diesel-rs/diesel/blob/269e8b242eb2664c6fc98972de092a060d96be28/diesel/src/query_builder/upsert/on_conflict_clause.rs#L51-L66

The problematic field there is `target`. Unfortunately does the `target` field contain much more information than only the `WHERE` clause. So just moving it to the end won't work. What could work is splitting this impl in such a way that there is a concrete one for `Target: UndecoratedConflictTarget` and one for `Target = DecoratedConflictTarget`. Given that we could modify the `QueryFragment` impl for the second case in such a way that we don't call `QueryFragment` for the inner type, but only the specific fields. This would allow to reorder the specific clauses.

That all written: I would happily accept a PR fixing this as long as it includes a working test. Hopefully the information above gives some indication where to start looking.

Fix patch:
diff --git a/diesel/src/pg/query_builder/query_fragment_impls.rs b/diesel/src/pg/query_builder/query_fragment_impls.rs
index e90191242a79..47035742b874 100644
--- a/diesel/src/pg/query_builder/query_fragment_impls.rs
+++ b/diesel/src/pg/query_builder/query_fragment_impls.rs
@@ -5,6 +5,8 @@ use crate::pg::Pg;
 use crate::query_builder::locking_clause::{
     ForKeyShare, ForNoKeyUpdate, ForShare, ForUpdate, NoModifier, NoWait, SkipLocked,
 };
+use crate::query_builder::upsert::on_conflict_actions::DoUpdate;
+use crate::query_builder::upsert::on_conflict_clause::OnConflictValues;
 use crate::query_builder::upsert::on_conflict_target_decorations::DecoratedConflictTarget;
 use crate::query_builder::{AstPass, QueryFragment};
 use crate::result::QueryResult;
@@ -98,15 +100,20 @@ where
     }
 }
 
-impl<T, U> QueryFragment<Pg, crate::pg::backend::PgOnConflictClaues>
-    for DecoratedConflictTarget<T, U>
+impl<Values, Target, Action, Where> QueryFragment<Pg, crate::pg::backend::PgOnConflictClaues>
+    for OnConflictValues<Values, DecoratedConflictTarget<Target, Where>, DoUpdate<Action>>
 where
-    T: QueryFragment<Pg>,
-    U: QueryFragment<Pg>,
+    Values: QueryFragment<Pg>,
+    Target: QueryFragment<Pg>,
+    DoUpdate<Action>: QueryFragment<Pg>,
+    Where: QueryFragment<Pg>,
 {
     fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Pg>) -> QueryResult<()> {
-        self.target.walk_ast(out.reborrow())?;
-        self.where_clause.walk_ast(out.reborrow())?;
+        self.values.walk_ast(out.reborrow())?;
+        out.push_sql(" ON CONFLICT");
+        self.target.target.walk_ast(out.reborrow())?;
+        self.action.walk_ast(out.reborrow())?;
+        self.target.where_clause.walk_ast(out.reborrow())?;
         Ok(())
     }
 }
diff --git a/diesel/src/query_builder/upsert/on_conflict_clause.rs b/diesel/src/query_builder/upsert/on_conflict_clause.rs
index f8527348c47b..a154285cb340 100644
--- a/diesel/src/query_builder/upsert/on_conflict_clause.rs
+++ b/diesel/src/query_builder/upsert/on_conflict_clause.rs
@@ -9,9 +9,9 @@ use crate::result::QueryResult;
 #[doc(hidden)]
 #[derive(Debug, Clone, Copy)]
 pub struct OnConflictValues<Values, Target, Action> {
-    values: Values,
-    target: Target,
-    action: Action,
+    pub(crate) values: Values,
+    pub(crate) target: Target,
+    pub(crate) action: Action,
 }
 
 impl<Values, Target, Action> QueryId for OnConflictValues<Values, Target, Action> {
@@ -53,7 +53,7 @@ where
     DB: Backend,
     DB::OnConflictClause: sql_dialect::on_conflict_clause::SupportsOnConflictClause,
     Values: QueryFragment<DB>,
-    Target: QueryFragment<DB>,
+    Target: QueryFragment<DB> + super::on_conflict_target_decorations::UndecoratedConflictTarget,
     Action: QueryFragment<DB>,
 {
     fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, DB>) -> QueryResult<()> {
@@ -64,3 +64,19 @@ where
         Ok(())
     }
 }
+
+impl<DB, Values, Target, Action, Where> QueryFragment<DB>
+    for OnConflictValues<
+        Values,
+        super::on_conflict_target_decorations::DecoratedConflictTarget<Target, Where>,
+        Action,
+    >
+where
+    DB: Backend,
+    DB::OnConflictClause: sql_dialect::on_conflict_clause::SupportsOnConflictClause,
+    Self: QueryFragment<DB, DB::OnConflictClause>,
+{
+    fn walk_ast<'b>(&'b self, pass: AstPass<'_, 'b, DB>) -> QueryResult<()> {
+        <Self as QueryFragment<DB, DB::OnConflictClause>>::walk_ast(self, pass)
+    }
+}
diff --git a/diesel/src/query_builder/upsert/on_conflict_target_decorations.rs b/diesel/src/query_builder/upsert/on_conflict_target_decorations.rs
index 015abb203e35..1bd3d92eac61 100644
--- a/diesel/src/query_builder/upsert/on_conflict_target_decorations.rs
+++ b/diesel/src/query_builder/upsert/on_conflict_target_decorations.rs
@@ -1,8 +1,6 @@
-use crate::backend::Backend;
 use crate::expression::Expression;
 use crate::query_builder::upsert::on_conflict_target::{ConflictTarget, NoConflictTarget};
 use crate::query_builder::where_clause::{NoWhereClause, WhereAnd, WhereClause};
-use crate::query_builder::{AstPass, QueryFragment, QueryResult};
 use crate::sql_types::BoolOrNullableBool;
 
 pub trait UndecoratedConflictTarget {}
@@ -56,13 +54,3 @@ where
         }
     }
 }
-
-impl<DB, T, U> QueryFragment<DB> for DecoratedConflictTarget<T, U>
-where
-    DB: Backend,
-    Self: QueryFragment<DB, DB::OnConflictClause>,
-{
-    fn walk_ast<'b>(&'b self, pass: AstPass<'_, 'b, DB>) -> QueryResult<()> {
-        <Self as QueryFragment<DB, DB::OnConflictClause>>::walk_ast(self, pass)
-    }
-}


Test patch:
diff --git a/diesel_compile_tests/tests/fail/invalid_upsert.rs b/diesel_compile_tests/tests/fail/invalid_upsert.rs
new file mode 100644
index 000000000000..5c8b36bd4f42
--- /dev/null
+++ b/diesel_compile_tests/tests/fail/invalid_upsert.rs
@@ -0,0 +1,35 @@
+extern crate diesel;
+
+use diesel::prelude::*;
+
+table! {
+    users {
+        id -> Integer,
+        name -> Text,
+        hair_color -> Nullable<Text>,
+    }
+}
+
+fn main() {
+    let conn = &mut PgConnection::establish("...").unwrap();
+
+    // this query is valid
+    diesel::insert_into(users::table)
+        .values(&[(users::id.eq(1), users::name.eq("Sean"))])
+        .on_conflict(users::id)
+        .filter_target(users::hair_color.eq("black"))
+        .do_update()
+        .set(users::hair_color.eq("red"))
+        .execute(conn)
+        .unwrap();
+
+    // This is invalid as filter_target is only valid for
+    // do update queries
+    diesel::insert_into(users::table)
+        .values(&[(users::id.eq(1), users::name.eq("Sean"))])
+        .on_conflict(users::id)
+        .filter_target(users::hair_color.eq("black"))
+        .do_nothing()
+        .execute(conn)
+        .unwrap();
+}
diff --git a/diesel_compile_tests/tests/fail/invalid_upsert.stderr b/diesel_compile_tests/tests/fail/invalid_upsert.stderr
new file mode 100644
index 000000000000..12ce92e5d8a7
--- /dev/null
+++ b/diesel_compile_tests/tests/fail/invalid_upsert.stderr
@@ -0,0 +1,16 @@
+error[E0275]: overflow evaluating the requirement `_: Sized`
+    --> tests/fail/invalid_upsert.rs:33:10
+     |
+33   |         .execute(conn)
+     |          ^^^^^^^
+     |
+     = help: consider increasing the recursion limit by adding a `#![recursion_limit = "256"]` attribute to your crate (`$CRATE`)
+     = note: required because of the requirements on the impl of `QueryFragment<_>` for `diesel::query_builder::upsert::on_conflict_clause::OnConflictValues<diesel::query_builder::insert_statement::batch_insert::BatchInsert<Vec<diesel::query_builder::insert_statement::ValuesClause<(ColumnInsertValue<columns::id, &diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>, ColumnInsertValue<columns::name, &diesel::expression::bound::Bound<diesel::sql_types::Text, &str>>), users::table>>, users::table, [diesel::query_builder::insert_statement::ValuesClause<(ColumnInsertValue<columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>, ColumnInsertValue<columns::name, diesel::expression::bound::Bound<diesel::sql_types::Text, &str>>), users::table>; 1], true>, diesel::query_builder::upsert::on_conflict_target_decorations::DecoratedConflictTarget<diesel::query_builder::upsert::on_conflict_target::ConflictTarget<columns::id>, diesel::query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<columns::hair_color, diesel::expression::bound::Bound<diesel::sql_types::Nullable<diesel::sql_types::Text>, &str>>>>>, diesel::query_builder::upsert::on_conflict_actions::DoNothing>`
+     = note: 127 redundant requirements hidden
+     = note: required because of the requirements on the impl of `QueryFragment<_>` for `InsertStatement<users::table, diesel::query_builder::upsert::on_conflict_clause::OnConflictValues<diesel::query_builder::insert_statement::batch_insert::BatchInsert<Vec<diesel::query_builder::insert_statement::ValuesClause<(ColumnInsertValue<columns::id, &diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>, ColumnInsertValue<columns::name, &diesel::expression::bound::Bound<diesel::sql_types::Text, &str>>), users::table>>, users::table, [diesel::query_builder::insert_statement::ValuesClause<(ColumnInsertValue<columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>, ColumnInsertValue<columns::name, diesel::expression::bound::Bound<diesel::sql_types::Text, &str>>), users::table>; 1], true>, diesel::query_builder::upsert::on_conflict_target_decorations::DecoratedConflictTarget<diesel::query_builder::upsert::on_conflict_target::ConflictTarget<columns::id>, diesel::query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<columns::hair_color, diesel::expression::bound::Bound<diesel::sql_types::Nullable<diesel::sql_types::Text>, &str>>>>>, diesel::query_builder::upsert::on_conflict_actions::DoNothing>>`
+     = note: required because of the requirements on the impl of `ExecuteDsl<_, _>` for `InsertStatement<users::table, diesel::query_builder::upsert::on_conflict_clause::OnConflictValues<diesel::query_builder::insert_statement::batch_insert::BatchInsert<Vec<diesel::query_builder::insert_statement::ValuesClause<(ColumnInsertValue<columns::id, &diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>, ColumnInsertValue<columns::name, &diesel::expression::bound::Bound<diesel::sql_types::Text, &str>>), users::table>>, users::table, [diesel::query_builder::insert_statement::ValuesClause<(ColumnInsertValue<columns::id, diesel::expression::bound::Bound<diesel::sql_types::Integer, i32>>, ColumnInsertValue<columns::name, diesel::expression::bound::Bound<diesel::sql_types::Text, &str>>), users::table>; 1], true>, diesel::query_builder::upsert::on_conflict_target_decorations::DecoratedConflictTarget<diesel::query_builder::upsert::on_conflict_target::ConflictTarget<columns::id>, diesel::query_builder::where_clause::WhereClause<diesel::expression::grouped::Grouped<diesel::expression::operators::Eq<columns::hair_color, diesel::expression::bound::Bound<diesel::sql_types::Nullable<diesel::sql_types::Text>, &str>>>>>, diesel::query_builder::upsert::on_conflict_actions::DoNothing>>`
+note: required by a bound in `diesel::RunQueryDsl::execute`
+    --> $DIESEL/src/query_dsl/mod.rs
+     |
+     |         Self: methods::ExecuteDsl<Conn>,
+     |               ^^^^^^^^^^^^^^^^^^^^^^^^^ required by this bound in `diesel::RunQueryDsl::execute`
diff --git a/diesel_tests/tests/debug/mod.rs b/diesel_tests/tests/debug/mod.rs
index 50e822f8c928..a57a303b5403 100644
--- a/diesel_tests/tests/debug/mod.rs
+++ b/diesel_tests/tests/debug/mod.rs
@@ -149,13 +149,14 @@ fn test_upsert() {
         .values(&values)
         .on_conflict(hair_color)
         .filter_target(hair_color.eq("black"))
-        .do_nothing();
+        .do_update()
+        .set(hair_color.eq("red"));
     let upsert_single_where_sql_display =
         debug_query::<TestBackend, _>(&upsert_command_single_where).to_string();
 
     assert_eq!(
         upsert_single_where_sql_display,
-        r#"INSERT INTO "users" ("name", "hair_color") VALUES ($1, $2), ($3, $4) ON CONFLICT ("hair_color") WHERE ("users"."hair_color" = $5) DO NOTHING -- binds: ["Sean", Some("black"), "Tess", None, "black"]"#
+        r#"INSERT INTO "users" ("name", "hair_color") VALUES ($1, $2), ($3, $4) ON CONFLICT ("hair_color") DO UPDATE SET "hair_color" = $5 WHERE ("users"."hair_color" = $6) -- binds: ["Sean", Some("black"), "Tess", None, "red", "black"]"#
     );
 
     let upsert_command_second_where = insert_into(users)
@@ -163,13 +164,14 @@ fn test_upsert() {
         .on_conflict(hair_color)
         .filter_target(hair_color.eq("black"))
         .filter_target(name.eq("Sean"))
-        .do_nothing();
+        .do_update()
+        .set(hair_color.eq("red"));
 
     let upsert_second_where_sql_display =
         debug_query::<TestBackend, _>(&upsert_command_second_where).to_string();
 
     assert_eq!(
         upsert_second_where_sql_display,
-        r#"INSERT INTO "users" ("name", "hair_color") VALUES ($1, $2), ($3, $4) ON CONFLICT ("hair_color") WHERE (("users"."hair_color" = $5) AND ("users"."name" = $6)) DO NOTHING -- binds: ["Sean", Some("black"), "Tess", None, "black", "Sean"]"#
+        r#"INSERT INTO "users" ("name", "hair_color") VALUES ($1, $2), ($3, $4) ON CONFLICT ("hair_color") DO UPDATE SET "hair_color" = $5 WHERE (("users"."hair_color" = $6) AND ("users"."name" = $7)) -- binds: ["Sean", Some("black"), "Tess", None, "red", "black", "Sean"]"#
     );
 }
diff --git a/diesel_tests/tests/update.rs b/diesel_tests/tests/update.rs
index 229ae6130c0d..30ac16c42959 100644
--- a/diesel_tests/tests/update.rs
+++ b/diesel_tests/tests/update.rs
@@ -308,6 +308,50 @@ fn upsert_with_sql_literal_for_target() {
     assert_eq!(Ok(expected_data), data);
 }
 
+// regression test for https://github.com/diesel-rs/diesel/issues/3330
+#[test]
+#[cfg(feature = "postgres")]
+fn upsert_with_target_filter() {
+    #[derive(AsChangeset)]
+    #[diesel(table_name = users)]
+    struct Changes<'a> {
+        hair_color: Option<&'a str>,
+    }
+
+    let conn = &mut connection_with_sean_and_tess_in_users_table();
+
+    let users = [User::new(1, "Sean"), User::new(2, "Tess")];
+    let query = insert_into(users::table)
+        .values(&users)
+        .on_conflict(users::id)
+        .filter_target(users::name.eq("Sean"))
+        .do_update()
+        .set(&Changes {
+            hair_color: Some("red"),
+        });
+
+    let upsert_single_where_sql_display = debug_query::<TestBackend, _>(&query).to_string();
+
+    assert_eq!(
+        upsert_single_where_sql_display,
+        r#"INSERT INTO "users" ("id", "name", "hair_color") VALUES ($1, $2, DEFAULT), ($3, $4, DEFAULT) ON CONFLICT ("id") DO UPDATE SET "hair_color" = $5 WHERE ("users"."name" = $6) -- binds: [1, "Sean", 2, "Tess", "red", "Sean"]"#
+    );
+
+    let result = query.execute(conn);
+
+    assert_eq!(result, Ok(1));
+    let users = users::table
+        .order_by(users::id)
+        .load::<(i32, String, Option<String>)>(conn)
+        .unwrap();
+
+    assert_eq!(
+        users[0],
+        (1, String::from("Sean"), Some(String::from("red")))
+    );
+    assert_eq!(users[1], (2, String::from("Tess"), None));
+}
+
 #[test]
 #[cfg(feature = "postgres")]
 fn update_array_index_expression() {