Refine CompositeColumn: remove getRaw(composite), remove maps

[vlsi]

I refined UpdateBuilder#set methods in https://github.com/JetBrains/Exposed/pull/1277, and I noticed set(column: CompositeColumn<S>, value: S) is not very elegant.

Then it turned out that all usages of CompositeColumn,getRealColumnsWithValues are forEach {...} which end up with Any? kind o API since Map can't express <Column<T>, T> for individual entries.

In practice, ColumnValue needs two APIs: one for "creating composite value out of parts" (e.g. to construct Kotlin object out of database values) and the second one for "producing parts out of composite" (e.g. to store them into database or to write them in WHERE clause)

The current issues include:

• All usages of CompositeColumn#getRealColumnsWithValues introduce UNCHECKED_CAST
• RawResult#getRaw is ill defined for CompositeColumn. For instance, ResultRow#hasValue(c), ResultRow#getOrNull is likely to produce wrong results for CompositeColumn, and it does not really use individual column value conversion when parsing the value to composite. For instance, the current CompositeMoneyColumn#transformToValue implementation does have to basically duplicate CurrencyColumnType logic.

What do you think of the following idea?

Producing parts out of composite

Current API: abstract fun getRealColumnsWithValues(compositeValue: T): Map<Column<*>, Any?>

Suggested API:

interface ForEachColumnValue {
    fun <U> consume(column: Column<U>, value: U)
}

abstract class CompositeColumn<T> : Expression<T>() {
    abstract fun splitParts(compositeValue: T, consumer: ForEachColumnPart)

Sample usage:

UpdateBuilder

https://github.com/JetBrains/Exposed/blob/d59247283dc8c1af019e3e2a8c75c7519d1fdb6d/exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/statements/UpdateBuilder.kt#L42-L44

    open operator fun <S> set(column: CompositeColumn<S>, value: S) {
        // suggested
        column.splitParts(value, object : ForEachColumnValue {
            override fun <U> consume(column: Column<U>, value: U) {
                set(column, value)
            }
        })
        // old
        column.getRealColumnsWithValues(value).forEach { (realColumn, itsValue) ->
            @Suppress("UNCHECKED_CAST")
            set(realColumn as Column<Any?>, itsValue)
        }
    }

Table

https://github.com/JetBrains/Exposed/blob/d59247283dc8c1af019e3e2a8c75c7519d1fdb6d/exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/Table.kt#L675-L682

    fun <T> CompositeColumn<T>.default(defaultValue: T): CompositeColumn<T> = apply {
        with(this@Table) { // <-- by the way, I think this is not needed
            // new
            this@default.splitParts(defaultValue, object : ForEachColumnValue {
                override fun <U> consume(column: Column<U>, value: U) {
                    column.default(value)
                }
            })
            // old
            this@default.getRealColumnsWithValues(defaultValue).forEach {
                @Suppress("UNCHECKED_CAST")
                (it.key as Column<Any>).default(it.value as Any)
            }
        }
    }

Entity

https://github.com/JetBrains/Exposed/blob/d59247283dc8c1af019e3e2a8c75c7519d1fdb6d/exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/Entity.kt#L133-L139

    operator fun <T> CompositeColumn<T>.setValue(o: Entity<ID>, desc: KProperty<*>, value: T) {
        with(o) {
            // new
            this@setValue.splitParts(value, object: ForEachColumnValue {
                override fun <U> consume(column: Column<U>, value: U) {
                    column.setValue(o, desc, value)
                }
            })
            // old
            this@setValue.getRealColumnsWithValues(value).forEach {
                // @Suppress("UNCHECKED_CAST")
                (it.key as Column<Any?>).setValue(o, desc, it.value)
            }
        }
    }

Unfortunately, object: .. syntax is a bit verbose, however, it does make types safe for the callers (UNCHECKED_CAST no longer needed).

The implementation of splitValues would be safer too.

Here's a sample implementation for BiCompositeColumn:

abstract class BiCompositeColumn<C1, C2, T>(
    val transformFromValue: (T) -> Pair<C1, C2>
) {
    override fun splitParts(compositeValue: T, consumer: ForEachColumnValue) {
        val (v1, v2) = transformFromValue(compositeValue)
        consumer.consume(column1, v1) // <-- note that types of column1 and v1 are verified here
        consumer.consume(column2, v2)
    }

Creating composite value out of parts

1. Make "raw" value for composites non-existing. In other words, composites are pure virtual, and there's no way to tell if the money value is null without converting amount + currency to money

2. Add interface so composite implementation can "query" the needed value:

interface GetColumnValue {
    operator fun <U> get(column: Column<U>): U
}

abstract class CompositeColumn<T> : Expression<T>() {

    abstract fun restoreValueFromParts(parts: GetColumnValue): T

The implementation for BiCompositeColumn would be trivial:

    override fun restoreValueFromParts(parts: GetColumnValue): T {
        val result = transformToValue(parts[column1], parts[column2]) // <-- no "unchecked cast here" !
        require(result != null || nullable) {
            "Null value received from DB for non-nullable ${this::class.simpleName} column"
        }
        return result
    }

Sample usages:

Entity

    operator fun <T> CompositeColumn<T>.getValue(o: Entity<ID>, desc: KProperty<*>): T {
        // new
        return restoreValueFromParts(object : GetColumnValue {
            override fun <U> get(column: Column<U>): U = column.lookup()
        })
        // old
        val values = this.getRealColumns().associateWith { it.lookup() }
        return this.restoreValueFromParts(values)
    }

ResultRow

    private fun <T> getRaw(c: Expression<T>): T? {
        if (c is CompositeColumn<T>) {
            val rawParts = c.getRealColumns().associateWith { getRaw(it) }
            return c.restoreValueFromParts(rawParts) // <-- this is bug. restoreValueFromParts produces non-raw value 
        }

The better implementation would be behind the lines of

    operator fun <T> get(c: Expression<T>): T {
        if (c is CompositeColumn<T>) {
            return c.restoreValueFromParts(object: GetColumnValue {
                override fun <U> get(column: Column<U>): U = get(column) // <-- it might need to account for withDialect
            })
        }

[vlsi]

Just a side note:

interface ForEachColumnValue {
    fun <U> consume(column: Column<U>, value: U)
}

could be

interface SetColumnValue {
    operator fun <U> set(column: Column<U>, value: U)
}

Then the implementation of CompositeColumnwould be slightly easier to read:

    override fun splitParts(compositeValue: T, columns: SetColumnValue) {
        val (v1, v2) = transformFromValue(compositeValue)
        columns[column1] = v1 // <-- note that types of column1 and v1 are verified here
        columns[column2] = v2
    }

However, the usage of splitParts method might be slightly more confusing:

UpdateBuilder:

    open operator fun <S> set(column: CompositeColumn<S>, value: S) {
        column.splitParts(value, object: SetColumnValue {
            override fun <U> set(column: Column<U>, value: U) {
                this@UpdateBuilder[column] = value // <-- this@.. is needed to avoid recursive set call
            }
        })

Table

    fun <T> CompositeColumn<T>.default(defaultValue: T): CompositeColumn<T> = apply {
        splitParts(defaultValue, object: SetColumnValue {
            override fun <U> set(column: Column<U>, value: U) {
                column.default(value)
            }
        })

Frankly speaking, I like how GetColumnValue is paired with SetColumnValue, so I like that naming better than ForEachColumnValue.