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 47035742b874..e90191242a79 100644
--- a/diesel/src/pg/query_builder/query_fragment_impls.rs
+++ b/diesel/src/pg/query_builder/query_fragment_impls.rs
@@ -5,8 +5,6 @@ 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;
@@ -100,20 +98,15 @@ where
     }
 }
 
-impl<Values, Target, Action, Where> QueryFragment<Pg, crate::pg::backend::PgOnConflictClaues>
-    for OnConflictValues<Values, DecoratedConflictTarget<Target, Where>, DoUpdate<Action>>
+impl<T, U> QueryFragment<Pg, crate::pg::backend::PgOnConflictClaues>
+    for DecoratedConflictTarget<T, U>
 where
-    Values: QueryFragment<Pg>,
-    Target: QueryFragment<Pg>,
-    DoUpdate<Action>: QueryFragment<Pg>,
-    Where: QueryFragment<Pg>,
+    T: QueryFragment<Pg>,
+    U: QueryFragment<Pg>,
 {
     fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Pg>) -> QueryResult<()> {
-        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())?;
+        self.target.walk_ast(out.reborrow())?;
+        self.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 a154285cb340..f8527348c47b 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> {
-    pub(crate) values: Values,
-    pub(crate) target: Target,
-    pub(crate) action: Action,
+    values: Values,
+    target: Target,
+    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> + super::on_conflict_target_decorations::UndecoratedConflictTarget,
+    Target: QueryFragment<DB>,
     Action: QueryFragment<DB>,
 {
     fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, DB>) -> QueryResult<()> {
@@ -64,19 +64,3 @@ 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 1bd3d92eac61..015abb203e35 100644
--- a/diesel/src/query_builder/upsert/on_conflict_target_decorations.rs
+++ b/diesel/src/query_builder/upsert/on_conflict_target_decorations.rs
@@ -1,6 +1,8 @@
+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 {}
@@ -54,3 +56,13 @@ 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
deleted file mode 100644
index 5c8b36bd4f42..000000000000
--- a/diesel_compile_tests/tests/fail/invalid_upsert.rs
+++ /dev/null
@@ -1,35 +0,0 @@
-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
deleted file mode 100644
index 12ce92e5d8a7..000000000000
--- a/diesel_compile_tests/tests/fail/invalid_upsert.stderr
+++ /dev/null
@@ -1,16 +0,0 @@
-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 a57a303b5403..50e822f8c928 100644
--- a/diesel_tests/tests/debug/mod.rs
+++ b/diesel_tests/tests/debug/mod.rs
@@ -149,14 +149,13 @@ fn test_upsert() {
         .values(&values)
         .on_conflict(hair_color)
         .filter_target(hair_color.eq("black"))
-        .do_update()
-        .set(hair_color.eq("red"));
+        .do_nothing();
     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") DO UPDATE SET "hair_color" = $5 WHERE ("users"."hair_color" = $6) -- binds: ["Sean", Some("black"), "Tess", None, "red", "black"]"#
+        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"]"#
     );
 
     let upsert_command_second_where = insert_into(users)
@@ -164,14 +163,13 @@ fn test_upsert() {
         .on_conflict(hair_color)
         .filter_target(hair_color.eq("black"))
         .filter_target(name.eq("Sean"))
-        .do_update()
-        .set(hair_color.eq("red"));
+        .do_nothing();
 
     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") DO UPDATE SET "hair_color" = $5 WHERE (("users"."hair_color" = $6) AND ("users"."name" = $7)) -- binds: ["Sean", Some("black"), "Tess", None, "red", "black", "Sean"]"#
+        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"]"#
     );
 }
diff --git a/diesel_tests/tests/update.rs b/diesel_tests/tests/update.rs
index 30ac16c42959..229ae6130c0d 100644
--- a/diesel_tests/tests/update.rs
+++ b/diesel_tests/tests/update.rs
@@ -308,50 +308,6 @@ 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() {