Fixing onConflict case with existing column used

zio · Nov 24, 2024 · 85b978d · 85b978d
1 parent 8e43a94
commit 85b978d
Show file tree

Hide file tree

Showing 4 changed files with 37 additions and 3 deletions.
diff --git a/quill-engine/src/main/scala/io/getquill/sql/idiom/OnConflictSupport.scala b/quill-engine/src/main/scala/io/getquill/sql/idiom/OnConflictSupport.scala
@@ -22,8 +22,29 @@ trait OnConflictSupport {
 
     val customAstTokenizer =
       Tokenizer.withFallback[Ast](self.astTokenizer(_, strategy, idiomContext)) {
+        case Property(_: OnConflict.Excluded, value) => stmt"EXCLUDED.${value.token}"
+
+        // At first glance it might be hard to understand why this is doing `case OnConflict.Existing(a) => stmt""`
+        // but consider that this is a situation where multiple aliases are used in multiple update clauses e.g. the `tt` in the below example
+        // wouldn't even exist as a variable because in the query produced (also below) it would not even exist
+        // i.e. since the table will be aliased as the first `existing table` variable i.e. `t`.
+        // The second one `tt` wouldn't even exist.
+        // ins.onConflictUpdate(_.i, _.s)(
+        //    (t, e) => t.l -> foo(t.l, e.l), (tt, ee) => tt.l -> bar(tt.l, ee.l)
+        //  )
+        //                                                                                                                            This doesn't exist!!
+        //                                                                                                                                    v
+        // > INSERT INTO TestEntity AS t (s,i,l,o,b) VALUES (?, ?, ?, ?, ?) ON CONFLICT (i,s) DO UPDATE SET l = foo(t.l, EXCLUDED.l), l = bar(tt.l, EXCLUDED.l)
+        //
+        // See the "cols target - update + infix" example for more detail
+        case Property(_: OnConflict.Existing, value) => stmt"${value.token}"
+        // As a backup if we have a standalone `OnConflict.Existing` or `OnConflict.Excluded` we just use
+        // the `EXCLUDED` keyword directly or the empty string `` for the `Existing` case.
+        // we can't have just the below two cases otherwise properties with the empty one `` would render as `.column`
+        // which would be invalid SQL.
         case _: OnConflict.Excluded => stmt"EXCLUDED"
-        case OnConflict.Existing(a) => stmt"${a.token}"
+        case _: OnConflict.Existing => stmt""
+        // Use the above action tokenizer for the `Action` case
         case a: Action =>
           self.actionTokenizer(customEntityTokenizer)(actionAstTokenizer, strategy, idiomContext).token(a)
       }

diff --git a/quill-sql-test/src/test/scala/io/getquill/context/sql/idiom/OnConflictSpec.scala b/quill-sql-test/src/test/scala/io/getquill/context/sql/idiom/OnConflictSpec.scala
@@ -40,6 +40,14 @@ trait OnConflictSpec extends Spec {
   def `cols target - update`(ins: Quoted[Insert[TestEntity]]) = quote {
     ins.onConflictUpdate(_.i, _.s)((t, e) => t.l -> (t.l + e.l) / 2, _.s -> _.s)
   }
+  // In this situation the `tt` variable that the "existing" row is pointing to (in the second clause) wouldn't
+  // even be created so clearly the variable name itself must be dropped and only the column reference should be used
+  def `cols target - update + infix`(ins: Quoted[Insert[TestEntity]]) = quote {
+    ins.onConflictUpdate(_.i, _.s)(
+      (t, e) => t.l -> sql"foo(${t.l}, ${e.l})".as[Long],
+      (tt, ee) => tt.l -> sql"bar(${tt.l}, ${ee.l})".as[Long]
+    )
+  }
   def insBatch = quote(liftQuery(List(e, TestEntity("s2", 1, 2L, Some(1), true))))
 
   def `no target - ignore batch` = quote {

diff --git a/quill-sql-test/src/test/scala/io/getquill/context/sql/idiom/PostgresDialectSpec.scala b/quill-sql-test/src/test/scala/io/getquill/context/sql/idiom/PostgresDialectSpec.scala
@@ -60,7 +60,12 @@ class PostgresDialectSpec extends OnConflictSpec {
     }
     "cols target - update" in {
       ctx.run(`cols target - update`(i)).string mustEqual
-        "INSERT INTO TestEntity AS t (s,i,l,o,b) VALUES (?, ?, ?, ?, ?) ON CONFLICT (i,s) DO UPDATE SET l = ((t.l + EXCLUDED.l) / 2), s = EXCLUDED.s"
+        "INSERT INTO TestEntity AS t (s,i,l,o,b) VALUES (?, ?, ?, ?, ?) ON CONFLICT (i,s) DO UPDATE SET l = ((l + EXCLUDED.l) / 2), s = EXCLUDED.s"
+    }
+    "cols target - update + infix" in {
+      println(ctx.run(`cols target - update + infix`(i)).string)
+      ctx.run(`cols target - update + infix`(i)).string mustEqual
+        "INSERT INTO TestEntity AS t (s,i,l,o,b) VALUES (?, ?, ?, ?, ?) ON CONFLICT (i,s) DO UPDATE SET l = foo(l, EXCLUDED.l), l = bar(l, EXCLUDED.l)"
     }
   }
 }
diff --git a/quill-sql-test/src/test/scala/io/getquill/context/sql/idiom/SqliteDialectSpec.scala b/quill-sql-test/src/test/scala/io/getquill/context/sql/idiom/SqliteDialectSpec.scala
@@ -51,7 +51,7 @@ class SqliteDialectSpec extends OnConflictSpec {
     }
     "cols target - update" in {
       ctx.run(`cols target - update`(i)).string mustEqual
-        "INSERT INTO TestEntity AS t (s,i,l,o,b) VALUES (?, ?, ?, ?, ?) ON CONFLICT (i,s) DO UPDATE SET l = ((t.l + EXCLUDED.l) / 2), s = EXCLUDED.s"
+        "INSERT INTO TestEntity AS t (s,i,l,o,b) VALUES (?, ?, ?, ?, ?) ON CONFLICT (i,s) DO UPDATE SET l = ((l + EXCLUDED.l) / 2), s = EXCLUDED.s"
     }
   }
 }