Repository: SeaQL/sea-query (language: rust)

Issue:
`OnConflict::values` and `OnConflict::update_columns` gets overridden by one another
<!--

Welcome! Thanks for reporting bugs!

Do you want to ask a question? Are you looking for support?
Please ask us on
- Discord: https://discord.com/invite/uCPdDXzbdv
- GitHub Discussion

-->

## Description

<!-- describe the bug -->
Since `OnConflict` uses the builder pattern, it is expected that a call to `values` then `update_columns`, or vice-versa, should create a query with both features, but only the last one gets applied to the query.

## Steps to Reproduce

1. Build an `InsertStatement` with an `OnConflict` clause
2. Add actions using `OnConflict::values([(Column::Col1, false.into())])` and `OnConflict::update_columns([Column::Col2])`
3. Build and check the generated query

### Expected Behavior

<!-- what is expected to happen? -->
PostgreSQL:
   `... DO UPDATE SET "col1" = false, "col2" = "excluded"."col2"`

### Actual Behavior

<!-- what actually happened? -->
PostgreSQL:
   `... DO UPDATE SET "col2" = "excluded"."col2"`

Only `OnConflict::update_columns` gets applied since it was used last

### Reproduces How Often

<!-- is it always reproducible? -->
Everytime

## Versions
0.24+ (when `OnConflict` was introduced)

<!-- you can get this information from copy and pasting the output of `cargo tree | grep sea-` from the console. Also, please include the database and OS that you are running. -->

## Additional Information

<!-- please provide a minimal reproducible example, if possible. Any other additional information that might be helpful. -->



Discussion / hints:

Fix patch:
diff --git a/src/backend/query_builder.rs b/src/backend/query_builder.rs
index 0acb564ca..66e0ee9bb 100644
--- a/src/backend/query_builder.rs
+++ b/src/backend/query_builder.rs
@@ -1111,27 +1111,24 @@ pub trait QueryBuilder: QuotedBuilder + EscapeBuilder + TableRefBuilder {
                 OnConflictAction::DoNothing => {
                     write!(sql, " DO NOTHING").unwrap();
                 }
-                OnConflictAction::UpdateColumns(columns) => {
+                OnConflictAction::Update(update_strats) => {
                     self.prepare_on_conflict_do_update_keywords(sql);
-                    columns.iter().fold(true, |first, col| {
+                    update_strats.iter().fold(true, |first, update_strat| {
                         if !first {
                             write!(sql, ", ").unwrap()
                         }
-                        col.prepare(sql.as_writer(), self.quote());
-                        write!(sql, " = ").unwrap();
-                        self.prepare_on_conflict_excluded_table(col, sql);
-                        false
-                    });
-                }
-                OnConflictAction::UpdateExprs(column_exprs) => {
-                    self.prepare_on_conflict_do_update_keywords(sql);
-                    column_exprs.iter().fold(true, |first, (col, expr)| {
-                        if !first {
-                            write!(sql, ", ").unwrap()
+                        match update_strat {
+                            OnConflictUpdate::Column(col) => {
+                                col.prepare(sql.as_writer(), self.quote());
+                                write!(sql, " = ").unwrap();
+                                self.prepare_on_conflict_excluded_table(col, sql);
+                            }
+                            OnConflictUpdate::Expr(col, expr) => {
+                                col.prepare(sql.as_writer(), self.quote());
+                                write!(sql, " = ").unwrap();
+                                self.prepare_simple_expr(expr, sql);
+                            }
                         }
-                        col.prepare(sql.as_writer(), self.quote());
-                        write!(sql, " = ").unwrap();
-                        self.prepare_simple_expr(expr, sql);
                         false
                     });
                 }
diff --git a/src/query/on_conflict.rs b/src/query/on_conflict.rs
index f3ba42fbd..45d950c0d 100644
--- a/src/query/on_conflict.rs
+++ b/src/query/on_conflict.rs
@@ -20,10 +20,17 @@ pub enum OnConflictTarget {
 pub enum OnConflictAction {
     /// Do nothing
     DoNothing,
+    /// Update column value of existing row
+    Update(Vec<OnConflictUpdate>),
+}
+
+/// Represents strategies to update column in ON CONFLICT (upsert) actions
+#[derive(Debug, Clone)]
+pub enum OnConflictUpdate {
     /// Update column value of existing row with inserting value
-    UpdateColumns(Vec<DynIden>),
+    Column(DynIden),
     /// Update column value of existing row with expression
-    UpdateExprs(Vec<(DynIden, SimpleExpr)>),
+    Expr(DynIden, SimpleExpr),
 }
 
 impl OnConflict {
@@ -109,9 +116,19 @@ impl OnConflict {
         C: IntoIden,
         I: IntoIterator<Item = C>,
     {
-        self.action = Some(OnConflictAction::UpdateColumns(
-            columns.into_iter().map(IntoIden::into_iden).collect(),
-        ));
+        let mut update_strats: Vec<OnConflictUpdate> = columns
+            .into_iter()
+            .map(|x| OnConflictUpdate::Column(IntoIden::into_iden(x)))
+            .collect();
+
+        match &mut self.action {
+            Some(OnConflictAction::Update(v)) => {
+                v.append(&mut update_strats);
+            }
+            Some(OnConflictAction::DoNothing) | None => {
+                self.action = Some(OnConflictAction::Update(update_strats));
+            }
+        };
         self
     }
 
@@ -154,12 +171,19 @@ impl OnConflict {
         C: IntoIden,
         I: IntoIterator<Item = (C, SimpleExpr)>,
     {
-        self.action = Some(OnConflictAction::UpdateExprs(
-            values
-                .into_iter()
-                .map(|(c, e)| (c.into_iden(), e))
-                .collect(),
-        ));
+        let mut update_exprs: Vec<OnConflictUpdate> = values
+            .into_iter()
+            .map(|(c, e)| OnConflictUpdate::Expr(c.into_iden(), e))
+            .collect();
+
+        match &mut self.action {
+            Some(OnConflictAction::Update(v)) => {
+                v.append(&mut update_exprs);
+            }
+            Some(OnConflictAction::DoNothing) | None => {
+                self.action = Some(OnConflictAction::Update(update_exprs));
+            }
+        };
         self
     }
 


Test patch:
diff --git a/tests/mysql/query.rs b/tests/mysql/query.rs
index 453cecc8e..5d6010861 100644
--- a/tests/mysql/query.rs
+++ b/tests/mysql/query.rs
@@ -1274,6 +1274,60 @@ fn insert_on_conflict_4() {
     );
 }
 
+#[test]
+#[allow(clippy::approx_constant)]
+fn insert_on_conflict_5() {
+    assert_eq!(
+        Query::insert()
+            .into_table(Glyph::Table)
+            .columns([Glyph::Aspect, Glyph::Image])
+            .values_panic([
+                "04108048005887010020060000204E0180400400".into(),
+                3.1415.into(),
+            ])
+            .on_conflict(
+                OnConflict::columns([Glyph::Id, Glyph::Aspect])
+                    .value(Glyph::Aspect, Expr::val("04108048005887010020060000204E0180400400"))
+                    .update_column(Glyph::Image)
+                    .to_owned()
+            )
+            .to_string(MysqlQueryBuilder),
+        [
+            r#"INSERT INTO `glyph` (`aspect`, `image`)"#,
+            r#"VALUES ('04108048005887010020060000204E0180400400', 3.1415)"#,
+            r#"ON DUPLICATE KEY UPDATE `aspect` = '04108048005887010020060000204E0180400400', `image` = VALUES(`image`)"#,
+        ]
+        .join(" ")
+    );
+}
+
+#[test]
+#[allow(clippy::approx_constant)]
+fn insert_on_conflict_6() {
+    assert_eq!(
+        Query::insert()
+            .into_table(Glyph::Table)
+            .columns([Glyph::Aspect, Glyph::Image])
+            .values_panic([
+                "04108048005887010020060000204E0180400400".into(),
+                3.1415.into(),
+            ])
+            .on_conflict(
+                OnConflict::columns([Glyph::Id, Glyph::Aspect])
+                    .update_column(Glyph::Aspect)
+                    .value(Glyph::Image, Expr::val(1).add(2))
+                    .to_owned()
+            )
+            .to_string(MysqlQueryBuilder),
+        [
+            r#"INSERT INTO `glyph` (`aspect`, `image`)"#,
+            r#"VALUES ('04108048005887010020060000204E0180400400', 3.1415)"#,
+            r#"ON DUPLICATE KEY UPDATE `aspect` = VALUES(`aspect`), `image` = 1 + 2"#,
+        ]
+        .join(" ")
+    );
+}
+
 #[test]
 fn update_1() {
     assert_eq!(
diff --git a/tests/postgres/query.rs b/tests/postgres/query.rs
index d756f147d..632c94505 100644
--- a/tests/postgres/query.rs
+++ b/tests/postgres/query.rs
@@ -1385,6 +1385,60 @@ fn insert_on_conflict_4() {
     );
 }
 
+#[test]
+#[allow(clippy::approx_constant)]
+fn insert_on_conflict_5() {
+    assert_eq!(
+        Query::insert()
+            .into_table(Glyph::Table)
+            .columns([Glyph::Aspect, Glyph::Image])
+            .values_panic([
+                "04108048005887010020060000204E0180400400".into(),
+                3.1415.into(),
+            ])
+            .on_conflict(
+                OnConflict::columns([Glyph::Id, Glyph::Aspect])
+                    .value(Glyph::Aspect, Expr::val("04108048005887010020060000204E0180400400"))
+                    .update_column(Glyph::Image)
+                    .to_owned()
+            )
+            .to_string(PostgresQueryBuilder),
+        [
+            r#"INSERT INTO "glyph" ("aspect", "image")"#,
+            r#"VALUES ('04108048005887010020060000204E0180400400', 3.1415)"#,
+            r#"ON CONFLICT ("id", "aspect") DO UPDATE SET "aspect" = '04108048005887010020060000204E0180400400', "image" = "excluded"."image""#,
+        ]
+        .join(" ")
+    );
+}
+
+#[test]
+#[allow(clippy::approx_constant)]
+fn insert_on_conflict_6() {
+    assert_eq!(
+        Query::insert()
+            .into_table(Glyph::Table)
+            .columns([Glyph::Aspect, Glyph::Image])
+            .values_panic([
+                "04108048005887010020060000204E0180400400".into(),
+                3.1415.into(),
+            ])
+            .on_conflict(
+                OnConflict::columns([Glyph::Id, Glyph::Aspect])
+                    .update_column(Glyph::Aspect)
+                    .value(Glyph::Image, Expr::val(1).add(2))
+                    .to_owned()
+            )
+            .to_string(PostgresQueryBuilder),
+        [
+            r#"INSERT INTO "glyph" ("aspect", "image")"#,
+            r#"VALUES ('04108048005887010020060000204E0180400400', 3.1415)"#,
+            r#"ON CONFLICT ("id", "aspect") DO UPDATE SET "aspect" = "excluded"."aspect", "image" = 1 + 2"#,
+        ]
+        .join(" ")
+    );
+}
+
 #[test]
 #[allow(clippy::approx_constant)]
 fn insert_returning_all_columns() {
diff --git a/tests/sqlite/query.rs b/tests/sqlite/query.rs
index 541d6c22c..56ede6765 100644
--- a/tests/sqlite/query.rs
+++ b/tests/sqlite/query.rs
@@ -1301,6 +1301,60 @@ fn insert_on_conflict_4() {
     );
 }
 
+#[test]
+#[allow(clippy::approx_constant)]
+fn insert_on_conflict_5() {
+    assert_eq!(
+        Query::insert()
+            .into_table(Glyph::Table)
+            .columns([Glyph::Aspect, Glyph::Image])
+            .values_panic([
+                "04108048005887010020060000204E0180400400".into(),
+                3.1415.into(),
+            ])
+            .on_conflict(
+                OnConflict::columns([Glyph::Id, Glyph::Aspect])
+                    .value(Glyph::Aspect, Expr::val("04108048005887010020060000204E0180400400"))
+                    .update_column(Glyph::Image)
+                    .to_owned()
+            )
+            .to_string(SqliteQueryBuilder),
+        [
+            r#"INSERT INTO "glyph" ("aspect", "image")"#,
+            r#"VALUES ('04108048005887010020060000204E0180400400', 3.1415)"#,
+            r#"ON CONFLICT ("id", "aspect") DO UPDATE SET "aspect" = '04108048005887010020060000204E0180400400', "image" = "excluded"."image""#,
+        ]
+        .join(" ")
+    );
+}
+
+#[test]
+#[allow(clippy::approx_constant)]
+fn insert_on_conflict_6() {
+    assert_eq!(
+        Query::insert()
+            .into_table(Glyph::Table)
+            .columns([Glyph::Aspect, Glyph::Image])
+            .values_panic([
+                "04108048005887010020060000204E0180400400".into(),
+                3.1415.into(),
+            ])
+            .on_conflict(
+                OnConflict::columns([Glyph::Id, Glyph::Aspect])
+                    .update_column(Glyph::Aspect)
+                    .value(Glyph::Image, Expr::val(1).add(2))
+                    .to_owned()
+            )
+            .to_string(SqliteQueryBuilder),
+        [
+            r#"INSERT INTO "glyph" ("aspect", "image")"#,
+            r#"VALUES ('04108048005887010020060000204E0180400400', 3.1415)"#,
+            r#"ON CONFLICT ("id", "aspect") DO UPDATE SET "aspect" = "excluded"."aspect", "image" = 1 + 2"#,
+        ]
+        .join(" ")
+    );
+}
+
 #[test]
 #[allow(clippy::approx_constant)]
 fn insert_returning_all_columns() {