Fixing onConflict case with existing column used (#3137)

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

Hide file tree

Showing 5 changed files with 42 additions and 9 deletions.
diff --git a/.github/workflows/site.yml b/.github/workflows/site.yml
@@ -18,11 +18,11 @@ jobs:
     if: ${{ github.event_name == 'pull_request' }}
     steps:
     - name: Git Checkout
-      uses: actions/checkout@v4.2.2
+      uses: actions/checkout@v3.3.0
       with:
         fetch-depth: '0'
     - name: Setup Scala
-      uses: actions/setup-java@v4.5.0
+      uses: actions/setup-java@v3.9.0
       with:
         distribution: temurin
         java-version: 17
@@ -39,11 +39,11 @@ jobs:
     if: ${{ ((github.event_name == 'release') && (github.event.action == 'published')) || (github.event_name == 'workflow_dispatch') }}
     steps:
     - name: Git Checkout
-      uses: actions/checkout@v4.2.2
+      uses: actions/checkout@v3.3.0
       with:
         fetch-depth: '0'
     - name: Setup Scala
-      uses: actions/setup-java@v4.5.0
+      uses: actions/setup-java@v3.9.0
       with:
         distribution: temurin
         java-version: 17
@@ -63,12 +63,12 @@ jobs:
     if: ${{ (github.event_name == 'push') || ((github.event_name == 'release') && (github.event.action == 'published')) }}
     steps:
     - name: Git Checkout
-      uses: actions/checkout@v4.2.2
+      uses: actions/checkout@v3.3.0
       with:
         ref: ${{ github.head_ref }}
         fetch-depth: '0'
     - name: Setup Scala
-      uses: actions/setup-java@v4.5.0
+      uses: actions/setup-java@v3.9.0
       with:
         distribution: temurin
         java-version: 17
@@ -82,7 +82,7 @@ jobs:
         git add README.md
         git commit -m "Update README.md" || echo "No changes to commit"
     - name: Create Pull Request
-      uses: peter-evans/create-pull-request@v7.0.5
+      uses: peter-evans/create-pull-request@v4.2.3
       with:
         body: |-
           Autogenerated changes after running the `sbt docs/generateReadme` command of the [zio-sbt-website](https://zio.dev/zio-sbt) plugin.

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
@@ -15,15 +15,32 @@ trait OnConflictSupport {
     strategy: NamingStrategy,
     idiomContext: IdiomContext
   ): Tokenizer[OnConflict] = {
+    val entityAlias = "t"
 
     val customEntityTokenizer = Tokenizer[Entity] { case Entity.Opinionated(name, _, _, renameable) =>
-      stmt"INTO ${renameable.fixedOr(name.token)(strategy.table(name).token)} AS t"
+      stmt"INTO ${renameable.fixedOr(name.token)(strategy.table(name).token)} AS ${entityAlias.token}"
     }
 
     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"${entityAlias}"`
+        // 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)
+        //
+        // So instead of the situation we use the original alias of the table as it was computed by Quill i.e. `t`.
+        // See the "cols target - update + infix" example for more detail
         case _: OnConflict.Excluded => stmt"EXCLUDED"
-        case OnConflict.Existing(a) => stmt"${a.token}"
+        case _: OnConflict.Existing => stmt"${entityAlias.token}"
         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
@@ -62,5 +62,9 @@ class PostgresDialectSpec extends OnConflictSpec {
       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"
     }
+    "cols target - update + infix" in {
+      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(t.l, EXCLUDED.l), l = bar(t.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
@@ -53,5 +53,9 @@ class SqliteDialectSpec extends OnConflictSpec {
       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"
     }
+    "cols target - update + infix" in {
+      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(t.l, EXCLUDED.l), l = bar(t.l, EXCLUDED.l)"
+    }
   }
 }