From e104a1df5f1f994665bea4b6a0dd48e30467dfdc Mon Sep 17 00:00:00 2001 From: Georg Semmler Date: Fri, 15 Dec 2023 14:59:32 +0100 Subject: [PATCH] Fix #3872 This commit changes some `QueryFragment` impls that previously assumed that the provided primary key consist only of a single column. The new implementation allows composite keys as well. In addition I also added two tests to cover these cases as well. --- .../query_builder/query_fragment_impls.rs | 87 +++++++++++++++---- diesel_tests/tests/insert.rs | 66 ++++++++++++++ 2 files changed, 137 insertions(+), 16 deletions(-) diff --git a/diesel/src/mysql/query_builder/query_fragment_impls.rs b/diesel/src/mysql/query_builder/query_fragment_impls.rs index 0b4276aa8213..ea1a5f20709e 100644 --- a/diesel/src/mysql/query_builder/query_fragment_impls.rs +++ b/diesel/src/mysql/query_builder/query_fragment_impls.rs @@ -76,17 +76,11 @@ impl QueryFragment for D where T: Table + StaticQueryFragment, T::Component: QueryFragment, - T::PrimaryKey: Column, + T::PrimaryKey: DoNothingHelper, { fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Mysql>) -> QueryResult<()> { out.push_sql(" UPDATE "); - T::STATIC_COMPONENT.walk_ast(out.reborrow())?; - out.push_sql("."); - out.push_identifier(::NAME)?; - out.push_sql(" = "); - T::STATIC_COMPONENT.walk_ast(out.reborrow())?; - out.push_sql("."); - out.push_identifier(::NAME)?; + T::PrimaryKey::walk_ast::(out.reborrow())?; Ok(()) } } @@ -95,20 +89,14 @@ impl QueryFragment where T: QueryFragment, Tab: Table + StaticQueryFragment, + Tab::PrimaryKey: DoNothingHelper, Tab::Component: QueryFragment, - Tab::PrimaryKey: Column, { fn walk_ast<'b>(&'b self, mut out: AstPass<'_, 'b, Mysql>) -> QueryResult<()> { out.unsafe_to_cache_prepared(); out.push_sql(" UPDATE "); if self.changeset.is_noop(out.backend())? { - Tab::STATIC_COMPONENT.walk_ast(out.reborrow())?; - out.push_sql("."); - out.push_identifier(::NAME)?; - out.push_sql(" = "); - Tab::STATIC_COMPONENT.walk_ast(out.reborrow())?; - out.push_sql("."); - out.push_identifier(::NAME)?; + Tab::PrimaryKey::walk_ast::(out.reborrow())?; } else { self.changeset.walk_ast(out.reborrow())?; } @@ -159,3 +147,70 @@ where self.0.walk_ast(out) } } + +trait DoNothingHelper { + fn walk_ast<'b, T>(out: AstPass<'_, 'b, Mysql>) -> QueryResult<()> + where + T: StaticQueryFragment, + T::Component: QueryFragment; +} + +impl DoNothingHelper for C +where + C: Column, +{ + fn walk_ast<'b, T>(mut out: AstPass<'_, 'b, Mysql>) -> QueryResult<()> + where + T: StaticQueryFragment, + T::Component: QueryFragment, + { + T::STATIC_COMPONENT.walk_ast(out.reborrow())?; + out.push_sql("."); + out.push_identifier(C::NAME)?; + out.push_sql(" = "); + T::STATIC_COMPONENT.walk_ast(out.reborrow())?; + out.push_sql("."); + out.push_identifier(C::NAME)?; + Ok(()) + } +} + +macro_rules! do_nothing_for_composite_keys { + ($( + $Tuple:tt { + $(($idx:tt) -> $T:ident, $ST:ident, $TT:ident,)+ + } + )+) => { + $( + impl<$($T,)*> DoNothingHelper for ($($T,)*) + where $($T: Column,)* + { + fn walk_ast<'b, Table>(mut out: AstPass<'_, 'b, Mysql>) -> QueryResult<()> + where + Table: StaticQueryFragment, + Table::Component: QueryFragment, + { + let mut first = true; + $( + #[allow(unused_assignments)] + if first { + first = false; + } else { + out.push_sql(", "); + } + Table::STATIC_COMPONENT.walk_ast(out.reborrow())?; + out.push_sql("."); + out.push_identifier($T::NAME)?; + out.push_sql(" = "); + Table::STATIC_COMPONENT.walk_ast(out.reborrow())?; + out.push_sql("."); + out.push_identifier($T::NAME)?; + )* + Ok(()) + } + } + )* + } +} + +diesel_derives::__diesel_for_each_tuple!(do_nothing_for_composite_keys); diff --git a/diesel_tests/tests/insert.rs b/diesel_tests/tests/insert.rs index 410d6174f156..3abae4a16693 100644 --- a/diesel_tests/tests/insert.rs +++ b/diesel_tests/tests/insert.rs @@ -865,3 +865,69 @@ fn mixed_defaultable_insert() { assert_eq!(Ok(expected_data), actual_data); } + +// regression test for https://github.com/diesel-rs/diesel/issues/3872 +#[test] +fn upsert_with_composite_primary_key_do_nothing() { + table! { + users (id, name) { + id -> Integer, + name -> Text, + hair_color -> Nullable, + } + } + + let conn = &mut connection_with_sean_and_tess_in_users_table(); + + diesel::insert_into(users::table) + .values((users::id.eq(1), users::name.eq("John"))) + .on_conflict_do_nothing() + .execute(conn) + .unwrap(); + let users = users::table + .select(users::name) + .load::(conn) + .unwrap(); + + assert_eq!(users[0], "Sean"); + assert_eq!(users[1], "Tess"); +} + +// regression test for https://github.com/diesel-rs/diesel/issues/3872 +#[test] +fn upsert_with_composite_primary_key_do_update() { + table! { + users (id, name) { + id -> Integer, + name -> Text, + hair_color -> Nullable, + } + } + + let conn = &mut connection_with_sean_and_tess_in_users_table(); + + #[cfg(feature = "mysql")] + diesel::insert_into(users::table) + .values((users::id.eq(1), users::name.eq("John"))) + .on_conflict(diesel::dsl::DuplicatedKeys) + .do_update() + .set(users::name.eq("Jane")) + .execute(conn) + .unwrap(); + + #[cfg(not(feature = "mysql"))] + diesel::insert_into(users::table) + .values((users::id.eq(1), users::name.eq("John"))) + .on_conflict(users::id) + .do_update() + .set(users::name.eq("Jane")) + .execute(conn) + .unwrap(); + let users = users::table + .select(users::name) + .load::(conn) + .unwrap(); + + assert_eq!(users[0], "Jane"); + assert_eq!(users[1], "Tess"); +}