Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EXPOSED-372 UpsertStatement.resultedValues contains incorrect value #2075

Merged
merged 4 commits into from
May 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions exposed-core/api/exposed-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -2993,6 +2993,7 @@ public class org/jetbrains/exposed/sql/statements/InsertStatement : org/jetbrain
public final fun getOrNull (Lorg/jetbrains/exposed/sql/Column;)Ljava/lang/Object;
public final fun getResultedValues ()Ljava/util/List;
public final fun getTable ()Lorg/jetbrains/exposed/sql/Table;
protected fun isColumnValuePreferredFromResultSet (Lorg/jetbrains/exposed/sql/Column;Ljava/lang/Object;)Z
public final fun isIgnore ()Z
public fun prepareSQL (Lorg/jetbrains/exposed/sql/Transaction;Z)Ljava/lang/String;
public fun prepared (Lorg/jetbrains/exposed/sql/Transaction;Ljava/lang/String;)Lorg/jetbrains/exposed/sql/statements/api/PreparedStatementApi;
Expand Down Expand Up @@ -3169,6 +3170,7 @@ public class org/jetbrains/exposed/sql/statements/UpsertStatement : org/jetbrain
public final fun getOnUpdate ()Ljava/util/List;
public final fun getOnUpdateExclude ()Ljava/util/List;
public final fun getWhere ()Lorg/jetbrains/exposed/sql/Op;
protected fun isColumnValuePreferredFromResultSet (Lorg/jetbrains/exposed/sql/Column;Ljava/lang/Object;)Z
public fun prepareSQL (Lorg/jetbrains/exposed/sql/Transaction;Z)Ljava/lang/String;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,10 @@ fun <T : Table> T.updateReturning(
*
* **Note:** Vendors that do not support this operation directly implement the standard MERGE USING command.
*
* **Note:** Currently, the `upsert()` function might return an incorrect auto-generated ID (such as a UUID) if it performs an update.
* In this case, it returns a new auto-generated ID instead of the ID of the updated row.
* Postgres should not be affected by this issue as it implicitly returns the IDs of updated rows.
*
* @param keys (optional) Columns to include in the condition that determines a unique constraint match.
* If no columns are provided, primary keys will be used. If the table does not have any primary keys, the first unique index will be attempted.
* @param onUpdate List of pairs of specific columns to update and the expressions to update them with.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,22 +100,22 @@ open class InsertStatement<Key : Any>(
}
pairs.forEach { (col, value) ->
if (value != DefaultValueMarker) {
if (col.columnType.isAutoInc || value is NextVal<*>) {
if (isColumnValuePreferredFromResultSet(col, value)) {
map.getOrPut(col) { value }
} else {
map[col] = value
}
}
}

// pairs.filter{ it.second != DefaultValueMarker }.forEach { (col, value) ->
// map.getOrPut(col){ value }
// }
}
@Suppress("UNCHECKED_CAST")
return autoGeneratedKeys.map { ResultRow.createAndFillValues(it as Map<Expression<*>, Any?>) }
}

protected open fun isColumnValuePreferredFromResultSet(column: Column<*>, value: Any?): Boolean {
return column.columnType.isAutoInc || value is NextVal<*>
}

@Suppress("NestedBlockDepth")
protected open fun valuesAndDefaults(values: Map<Column<*>, Any?> = this.values): Map<Column<*>, Any?> {
val result = values.toMutableMap()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,15 @@ open class UpsertStatement<Key : Any>(
builder.args
} ?: emptyList()
}

override fun isColumnValuePreferredFromResultSet(column: Column<*>, value: Any?): Boolean {
return isEntityIdClientSideGeneratedUUID(column) ||
super.isColumnValuePreferredFromResultSet(column, value)
}

private fun isEntityIdClientSideGeneratedUUID(column: Column<*>) =
(column.columnType as? EntityIDColumnType<*>)
?.idColumn
?.takeIf { it.columnType is UUIDColumnType }
?.defaultValueFun != null
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package org.jetbrains.exposed.sql.tests.shared.dml

import org.jetbrains.exposed.dao.id.IntIdTable
import org.jetbrains.exposed.dao.id.UUIDTable
import org.jetbrains.exposed.exceptions.ExposedSQLException
import org.jetbrains.exposed.exceptions.UnsupportedByDialectException
import org.jetbrains.exposed.sql.*
Expand Down Expand Up @@ -626,6 +627,34 @@ class UpsertTests : DatabaseTestsBase() {
}
}

@Test
fun testUpsertWithUUIDPrimaryKey() {
val tester = object : UUIDTable("upsert_test", "id") {
val key = integer("test_key").uniqueIndex()
val value = text("test_value")
}

// At present, only Postgres returns the correct UUID directly from the result set.
// For other databases incorrect ID is returned from the 'upsert' command.
withTables(excludeSettings = TestDB.entries - listOf(TestDB.POSTGRESQL, TestDB.POSTGRESQLNG), tester) { testDb ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the future, there are some available sets for common TestDB collections in its companion object, if you ever need them.

val insertId = tester.insertAndGetId {
it[key] = 1
it[value] = "one"
}

val upsertId = tester.upsert(
keys = arrayOf(tester.key),
onUpdateExclude = listOf(tester.id),
) {
it[key] = 1
it[value] = "two"
}.resultedValues!!.first()[tester.id]

assertEquals(insertId, upsertId)
assertEquals("two", tester.selectAll().where { tester.id eq insertId }.first()[tester.value])
}
}

private object AutoIncTable : Table("auto_inc_table") {
val id = integer("id").autoIncrement()
val name = varchar("name", 64)
Expand Down
Loading