Upsert with where clause Postgres implementation is wrong

[bowenyang007]

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 (#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" 

[weiznich]

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:

diesel/diesel/src/query_builder/upsert/on_conflict_clause.rs

Lines 51 to 66 in 269e8b2

	impl<DB, Values, Target, Action> QueryFragment<DB> for OnConflictValues<Values, Target, Action>
	where
	DB: Backend,
	DB::OnConflictClause: sql_dialect::on_conflict_clause::SupportsOnConflictClause,
	Values: QueryFragment<DB>,
	Target: QueryFragment<DB>,
	Action: QueryFragment<DB>,
	{
	fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, DB>) -> QueryResult<()> {
	self.values.walk_ast(out.reborrow())?;
	out.push_sql(" ON CONFLICT");
	self.target.walk_ast(out.reborrow())?;
	self.action.walk_ast(out.reborrow())?;
	Ok(())
	}
	}

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.

[weiznich]

This report does not describe an actual bug, but it's a feature request as it seek to add new functionality. The WHERE clause in this position is valid and shouldn't be changed as outlined in #3368.
Contributions are welcome to add the necessary query dsl interface to support this feature.