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

feat!: EXPOSED-577 Allow Entity and EntityID parameters to not be Comparable #2277

Merged
merged 3 commits into from
Oct 21, 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
8 changes: 8 additions & 0 deletions documentation-website/Writerside/topics/Breaking-Changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,14 @@
* `ArrayColumnType` now supports multidimensional arrays and includes an additional generic parameter.
If it was previously used for one-dimensional arrays with the parameter `T` like `ArrayColumnType<T>`,
it should now be defined as `ArrayColumnType<T, List<T>>`. For instance, `ArrayColumnType<Int>` should now be `ArrayColumnType<Int, List<Int>>`.
* `EntityID` and `CompositeID` no longer implement `Comparable` themselves, to allow their wrapped identity values to be of a type that is not
necessarily `Comparable`, like `kotlin.uuid.Uuid`.

Any use of an entity's `id` with Kotlin comparison operators or `compareTo()` will now require that the wrapped value be used directly:
`entity1.id < entity2.id` will need to become `entity1.id.value < entity2.id.value`. Any use of an entity's `id` with an Exposed function
that is also type restricted to `Comparable` (for example, `avg()`) will also require defining a new function. In this event, please
also leave a comment on [YouTrack](https://youtrack.jetbrains.com/issue/EXPOSED-577) with a use case so the original function signature
can be potentially reassessed.

## 0.55.0
* The `DeleteStatement` property `table` is now deprecated in favor of `targetsSet`, which holds a `ColumnSet` that may be a `Table` or `Join`.
Expand Down
58 changes: 27 additions & 31 deletions exposed-core/api/exposed-core.api

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@ package org.jetbrains.exposed.dao.id
import org.jetbrains.exposed.sql.Column

/** Class representing a mapping of each composite primary key column to its stored identity value. */
class CompositeID private constructor() : Comparable<CompositeID> {
internal val values: MutableMap<Column<*>, Comparable<*>?> = HashMap()
class CompositeID private constructor() {
internal val values: MutableMap<Column<*>, Any?> = HashMap()

@Suppress("UNCHECKED_CAST")
@JvmName("setWithEntityIdValue")
operator fun <T : Comparable<T>, ID : EntityID<T>> set(column: Column<ID>, value: T) {
operator fun <T : Any, ID : EntityID<T>> set(column: Column<ID>, value: T) {
require(values.isEmpty() || values.keys.first().table == column.table) {
"CompositeID key columns must all come from the same IdTable ${values.keys.first().table.tableName}"
}
Expand All @@ -17,23 +17,23 @@ class CompositeID private constructor() : Comparable<CompositeID> {

@Suppress("UNCHECKED_CAST")
@JvmName("setWithNullableEntityIdValue")
operator fun <T : Comparable<T>, ID : EntityID<T>> set(column: Column<ID?>, value: T?) {
operator fun <T : Any, ID : EntityID<T>> set(column: Column<ID?>, value: T?) {
require(column.columnType.nullable || value != null) {
"Trying to set null to not nullable column $column"
}
values[column] = value?.let { EntityID(value, column.table as IdTable<T>) }
}

@JvmName("setWithEntityID")
operator fun <T : Comparable<T>, ID : EntityID<T>> set(column: Column<ID>, value: ID) {
operator fun <T : Any, ID : EntityID<T>> set(column: Column<ID>, value: ID) {
require(values.isEmpty() || values.keys.first().table == column.table) {
"CompositeID key columns must all come from the same IdTable ${values.keys.first().table.tableName}"
}
values[column] = value
}

@Suppress("UNCHECKED_CAST")
operator fun <T : Comparable<T>> get(column: Column<T>): T = values[column] as T
operator fun <T : Any> get(column: Column<T>): T = values[column] as T

operator fun contains(column: Column<*>): Boolean = values.contains(column)

Expand All @@ -50,19 +50,6 @@ class CompositeID private constructor() : Comparable<CompositeID> {
return values == other.values
}

override fun compareTo(other: CompositeID): Int {
val compareSize = compareValues(other.values.size, values.size)
if (compareSize != 0) return compareSize

values.entries.forEach { (column, idValue) ->
if (!other.values.containsKey(column)) return -1
compareValues(idValue, other.values[column]).let {
if (it != 0) return it
}
}
return 0
}

companion object {
operator fun invoke(body: (CompositeID) -> Unit): CompositeID {
return CompositeID().apply(body).also {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ package org.jetbrains.exposed.dao.id
* @sample org.jetbrains.exposed.sql.tests.shared.entities.EntityTestsData.YTable
* @sample org.jetbrains.exposed.sql.tests.shared.dml.InsertTests.testInsertWithPredefinedId
*/
open class EntityID<T : Comparable<T>> protected constructor(val table: IdTable<T>, id: T?) : Comparable<EntityID<T>> {
open class EntityID<T : Any> protected constructor(val table: IdTable<T>, id: T?) {
constructor(id: T, table: IdTable<T>) : this(table, id)

@Suppress("VariableNaming")
Expand Down Expand Up @@ -41,6 +41,4 @@ open class EntityID<T : Comparable<T>> protected constructor(val table: IdTable<

return other._value == _value && other.table == table
}

override fun compareTo(other: EntityID<T>): Int = value.compareTo(other.value)
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import java.util.*
/** Base class representing a producer of [EntityID] instances. */
interface EntityIDFactory {
/** Returns a new [EntityID] that holds a [value] of type [T], for the specified [table]. */
fun <T : Comparable<T>> createEntityID(value: T, table: IdTable<T>): EntityID<T>
fun <T : Any> createEntityID(value: T, table: IdTable<T>): EntityID<T>
}

/** Class responsible for locating and providing the appropriate functions to produce [EntityID] instances. */
Expand All @@ -16,37 +16,37 @@ object EntityIDFunctionProvider {
init {
factory = ServiceLoader.load(EntityIDFactory::class.java, EntityIDFactory::class.java.classLoader).firstOrNull()
?: object : EntityIDFactory {
override fun <T : Comparable<T>> createEntityID(value: T, table: IdTable<T>): EntityID<T> {
override fun <T : Any> createEntityID(value: T, table: IdTable<T>): EntityID<T> {
return EntityID(value, table)
}
}
}

/** Returns a new [EntityID] that holds a [value] of type [T], for the specified [table]. */
fun <T : Comparable<T>> createEntityID(value: T, table: IdTable<T>) = factory.createEntityID(value, table)
fun <T : Any> createEntityID(value: T, table: IdTable<T>) = factory.createEntityID(value, table)
}

/**
* Base class for an identity table, which could be referenced from other tables.
*
* @param name Table name. By default, this will be resolved from any class name with a "Table" suffix removed (if present).
*/
abstract class IdTable<T : Comparable<T>>(name: String = "") : Table(name) {
abstract class IdTable<T : Any>(name: String = "") : Table(name) {
/** The identity column of this [IdTable], for storing values of type [T] wrapped as [EntityID] instances. */
abstract val id: Column<EntityID<T>>

private val _idColumns = HashSet<Column<out Comparable<*>>>()
private val _idColumns = HashSet<Column<out Any>>()

/** All base columns that make up this [IdTable]'s identifier column. */
val idColumns: Set<Column<out Comparable<*>>>
val idColumns: Set<Column<out Any>>
get() = _idColumns.ifEmpty {
val message = "Table definition must include id columns. Please use Column.entityId() or IdTable.addIdColumn()."
exposedLogger.error(message)
error(message)
}

/** Adds a column to [idColumns] so that it can be used as a component of the [id] property. */
protected fun <S : Comparable<S>> addIdColumn(newColumn: Column<EntityID<S>>) {
protected fun <S : Any> addIdColumn(newColumn: Column<EntityID<S>>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to confirm, is Any here to indicate that S not nullable?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, that was the assumption I made about most cases. Do you think it's not an ok assumption that may lead to breaking changes?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It was non-nullable Comparable, so I expect that it should be fine to replace it with non-nullable Any. To make it weaker in the future should be easier then vice versa anyway.

if (_idColumns.isNotEmpty() && this !is CompositeIdTable) {
val message = "CompositeIdTable should be used if multiple EntityID key columns are required"
exposedLogger.error(message)
Expand All @@ -55,7 +55,7 @@ abstract class IdTable<T : Comparable<T>>(name: String = "") : Table(name) {
_idColumns.add(newColumn)
}

internal fun <S : Comparable<S>> addIdColumnInternal(newColumn: Column<EntityID<S>>) { addIdColumn(newColumn) }
internal fun <S : Any> addIdColumnInternal(newColumn: Column<EntityID<S>>) { addIdColumn(newColumn) }
}

/**
Expand Down Expand Up @@ -152,7 +152,7 @@ open class CompositeIdTable(name: String = "") : IdTable<CompositeID>(name) {
(toCompare as? EntityID<CompositeID>) ?: error("toCompare must be an EntityID<CompositeID> value")
return idColumns.map { column ->
val otherValue = if (column in toCompare.value.values) {
toCompare.value[column as Column<EntityID<Comparable<Any>>>]
toCompare.value[column as Column<EntityID<Any>>]
} else {
error("Comparison CompositeID is missing a key mapping for ${column.name}")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ class Alias<out T : Table>(val delegate: T, val alias: String) : Table() {
delegateIdColumns.map { column ->
val delegateColumn = originalColumn(column)
val otherValue = if (delegateColumn in toCompare.value.values) {
toCompare.value[delegateColumn as Column<EntityID<Comparable<Any>>>]
toCompare.value[delegateColumn as Column<EntityID<Any>>]
} else {
error("Comparison CompositeID is missing a key mapping for ${delegateColumn?.name}")
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ internal fun Column<*>.isEntityIdentifier(): Boolean {
/**
* Identity column type for storing unique [EntityID] values.
*/
class EntityIDColumnType<T : Comparable<T>>(
class EntityIDColumnType<T : Any>(
/** The underlying wrapped column storing the identity values. */
val idColumn: Column<T>
) : ColumnType<EntityID<T>>() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ class Locate<T : String?>(val expr: Expression<T>, val substring: String) : Func
/**
* Represents an SQL function that returns the minimum value of [expr] across all non-null input values, or `null` if there are no non-null values.
*/
class Min<T : Comparable<T>, in S : T?>(
class Min<T : Any, in S : T?>(
/** Returns the expression from which the minimum value is obtained. */
val expr: Expression<in S>,
columnType: IColumnType<T>
Expand All @@ -182,7 +182,7 @@ class Min<T : Comparable<T>, in S : T?>(
/**
* Represents an SQL function that returns the maximum value of [expr] across all non-null input values, or `null` if there are no non-null values.
*/
class Max<T : Comparable<T>, in S : T?>(
class Max<T : Any, in S : T?>(
Copy link
Member Author

Choose a reason for hiding this comment

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

These changes were only made because our tests had use cases like TestTable.id.max().

@e5l @obabichevjb This now leads to the question, what about other restricted functions, like Avg(), which only accepts Comparable column types?
Should it still be possible to call avg() on an entityID column like TestTable.id.avg() or does that use case make no sense?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That’s a good question. At first glance, it seems like it shouldn't be Comparable, especially for Max functions, if it’s only comparable on the database side. I can’t think of a specific example right now, but imagine something that can have Max applied to it in the database but doesn’t have a comparable representation in Kotlin.

Since we get from SQL just single value and don't compare it on client side, it could be non-comparable.

/** Returns the expression from which the maximum value is obtained. */
val expr: Expression<in S>,
columnType: IColumnType<T>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -732,7 +732,7 @@ class QueryParameter<T>(
}

/** Returns the specified [value] as a query parameter with the same type as [column]. */
fun <T : Comparable<T>> idParam(value: EntityID<T>, column: Column<EntityID<T>>): Expression<EntityID<T>> =
fun <T : Any> idParam(value: EntityID<T>, column: Column<EntityID<T>>): Expression<EntityID<T>> =
QueryParameter(value, column.columnType)

/** Returns the specified [value] as a boolean query parameter. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ fun <T : Table> T.insert(body: T.(InsertStatement<Number>) -> Unit): InsertState
* @return The generated ID for the new row.
* @sample org.jetbrains.exposed.sql.tests.shared.dml.InsertTests.testGeneratedKey04
*/
fun <Key : Comparable<Key>, T : IdTable<Key>> T.insertAndGetId(body: T.(InsertStatement<EntityID<Key>>) -> Unit): EntityID<Key> =
fun <Key : Any, T : IdTable<Key>> T.insertAndGetId(body: T.(InsertStatement<EntityID<Key>>) -> Unit): EntityID<Key> =
InsertStatement<EntityID<Key>>(this, false).run {
body(this)
execute(TransactionManager.current())
Expand Down Expand Up @@ -381,7 +381,7 @@ fun <T : Table> T.insertIgnore(body: T.(UpdateBuilder<*>) -> Unit): InsertStatem
* @return The generated ID for the new row, or `null` if none was retrieved after statement execution.
* @sample org.jetbrains.exposed.sql.tests.shared.dml.InsertTests.testInsertIgnoreAndGetId01
*/
fun <Key : Comparable<Key>, T : IdTable<Key>> T.insertIgnoreAndGetId(body: T.(UpdateBuilder<*>) -> Unit): EntityID<Key>? =
fun <Key : Any, T : IdTable<Key>> T.insertIgnoreAndGetId(body: T.(UpdateBuilder<*>) -> Unit): EntityID<Key>? =
InsertStatement<EntityID<Key>>(this, isIgnore = true).run {
body(this)
when (execute(TransactionManager.current())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ class ResultRow(
column?.isEntityIdentifier() == true && column.table is CompositeIdTable -> {
val resultID = CompositeID {
column.table.idColumns.forEach { column ->
it[column as Column<EntityID<Comparable<Any>>>] = getInternal(column, checkNullability = true).value
it[column as Column<EntityID<Any>>] = getInternal(column, checkNullability = true).value
}
}
EntityID(resultID, column.table) as T
Expand Down
Loading
Loading