JetBrains / JetBrains/Exposed

Refine CompositeColumn: remove getRaw(composite), remove maps

Open
#1,278 1 comment 0 reactions 0 assignees View on GitHub
Dominant language
Kotlin
Stars
9.3k
Forks
798
Avg merge
4d 2h
Merged PRs (30d)
26

Description

I refined `UpdateBuilder#set` methods in https://github.com/JetBrains/Exposed/pull/1277, and I noticed `set(column: CompositeColumn, 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 `, 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, Any?>`

Suggested API:

```kotlin
interface ForEachColumnValue {
fun consume(column: Column, value: U)
}

abstract class CompositeColumn : Expression() {
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

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

`Table`

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

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

```kotlin
operator fun CompositeColumn.setValue(o: Entity, desc: KProperty<*>, value: T) {
with(o) {
// new
this@setValue.splitParts(value, object: ForEachColumnValue {
override fun consume(column: Column, value: U) {
column.setValue(o, desc, value)
}
})
// old
this@setValue.getRealColumnsWithValues(value).forEach {
// @Suppress("UNCHECKED_CAST")
(it.key as Column).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`:

```kotlin
abstract class BiCompositeColumn(
val transformFromValue: (T) -> Pair
) {
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:

```kotlin
interface GetColumnValue {
operator fun get(column: Column): U
}

abstract class CompositeColumn : Expression() {

abstract fun restoreValueFromParts(parts: GetColumnValue): T
```

The implementation for `BiCompositeColumn` would be trivial:

```kotlin
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`

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

`ResultRow`

```kotlin
private fun getRaw(c: Expression): T? {
if (c is CompositeColumn) {
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

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

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.