Review issues: add test with empty distinctOn, update query so it doe…

…s not generate wrong SQL with empty distinctOn
JetBrains · Oct 29, 2024 · 4f41be1 · 4f41be1
1 parent 7cc0529
commit 4f41be1
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 9 deletions.
diff --git a/exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/Query.kt b/exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/Query.kt
@@ -103,6 +103,8 @@ open class Query(override var set: FieldSet, where: Op<Boolean>?) : AbstractQuer
      * @return The current `Query` instance with the `DISTINCT ON` clause applied.
      */
     fun withDistinctOn(vararg columns: Column<*>): Query = apply {
+        if (columns.isEmpty()) return@apply
+
         require(!distinct) { "DISTINCT ON cannot be used with the DISTINCT modifier. Only one of them should be applied." }
         distinctOn = (distinctOn ?: emptyList()) + columns
     }
@@ -118,10 +120,11 @@ open class Query(override var set: FieldSet, where: Op<Boolean>?) : AbstractQuer
      * @return The current `Query` instance with the `DISTINCT ON` clause and reordering applied.
      */
     fun withDistinctOn(vararg columns: Pair<Column<*>, SortOrder>): Query = apply {
+        if (columns.isEmpty()) return@apply
+
         require(!distinct) { "DISTINCT ON cannot be used with the DISTINCT modifier. Only one of them should be applied." }
-        @Suppress("SpreadOperator")
-        withDistinctOn(*columns.map { it.first }.toTypedArray())
-        return orderBy(*columns)
+        withDistinctOn(columns = columns.map { it.first }.toTypedArray())
+        return orderBy(order = columns)
     }
 
     @Deprecated(
@@ -218,9 +221,11 @@ open class Query(override var set: FieldSet, where: Op<Boolean>?) : AbstractQuer
                 if (distinct) {
                     append("DISTINCT ")
                 }
-                distinctOn?.let { columns ->
-                    columns.appendTo(prefix = "DISTINCT ON (", postfix = ") ") { +"${it.table.tableName}.${it.name}" }
-                }
+                distinctOn
+                    ?.takeIf { it.isNotEmpty() }
+                    ?.let { columns ->
+                        columns.appendTo(prefix = "DISTINCT ON (", postfix = ") ") { append(it) }
+                    }
                 set.realFields.appendTo { +it }
             }
             if (set.source != Table.Dual || currentDialect.supportsDualTableConcept) {

diff --git a/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/dml/DistinctOnTests.kt b/exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/dml/DistinctOnTests.kt
@@ -1,14 +1,14 @@
 package org.jetbrains.exposed.sql.tests.shared.dml
 
 import org.jetbrains.exposed.dao.id.IntIdTable
-import org.jetbrains.exposed.sql.SortOrder
-import org.jetbrains.exposed.sql.batchInsert
-import org.jetbrains.exposed.sql.selectAll
+import org.jetbrains.exposed.sql.*
 import org.jetbrains.exposed.sql.tests.DatabaseTestsBase
 import org.jetbrains.exposed.sql.tests.TestDB
 import org.jetbrains.exposed.sql.tests.shared.assertEqualLists
+import org.jetbrains.exposed.sql.tests.shared.assertEquals
 import org.jetbrains.exposed.sql.tests.shared.expectException
 import org.junit.Test
+import kotlin.test.assertNull
 
 class DistinctOnTests : DatabaseTestsBase() {
 
@@ -79,4 +79,27 @@ class DistinctOnTests : DatabaseTestsBase() {
             }
         }
     }
+
+    @Test
+    fun testEmptyDistinctOn() {
+        val tester = object : IntIdTable("distinct_function_test") {
+            val value = integer("value1")
+        }
+
+        withTables(excludeSettings = TestDB.ALL - distinctOnSupportedDb, tester) {
+            addLogger(StdOutSqlLogger)
+            // Empty list of columns should not cause exception
+            tester.insert {
+                it[value] = 1
+            }
+
+            val query = tester.selectAll()
+                .withDistinctOn(columns = emptyArray<Column<*>>())
+            assertNull(query.distinctOn)
+
+            val value = query
+                .first()[tester.value]
+            assertEquals(1, value)
+        }
+    }
 }