chore! EXPOSED-655 Join column default fields defaultValueFun, defautValueInDb, isDatabaseGenerated into one field #2319

obabichevjb · 2024-11-27T13:58:11Z

Description

Here is the refactoring of column default values. The main idea is to joine 3 fields of the column related to default values (defaultValueFun, defautValueInDb, isDatabaseGenerated) into one field.

I see 2 main problems with the existing variant:

potential inconsistency in the fields (it's easy to change one field and forget about other, for example when you try to change default value)
it's not always easy to answer what kind of default column is now (is it client side default value, database side or something different). Also it's not always easy to understand what every condition on these fields means (when these parameters checked for null and not null in different combinations)

The solution is the following:

Replace the mentioned fields with one field of the type ColumnDefault.

We will strongly differentiate ClientColumnDefault and DatabaseColumnDefault independently of the content they have (it could be kotlin values or expressions). If the column has ClientColumnDefault it will be used for inserts, and will not be used for DDL, and vice versa for DatabaseColumnDefault.

From another perspective column default can be ColumnDefaultExpression (contains Expression), ColumnDefaultValue(contains kotlin value), no one of them (aka database generated), or both of them (the last one is important for DAO, because even if column has database default expression we still could use kotlin value to provide the value before actual commit).

Type of Change

Updates/remove existing public API methods:

Is breaking change

Affected databases:

All

Related Issues

EXPOSED-655 Join column default fields defaultValueFun, defaultValueInDb, isDatabaseGenerated into one fielld

obabichevjb · 2024-11-27T14:46:30Z

All the interface/class/function names are discussable of course.

In terms of base interface I chose ColumnDefault as a base variant. Mostly it could have prefix Database or Client and suffix Value (about that I have the most concerns, because it means in-memory kotlin value from one side, and sound like general default value on other side) or Expression (and the longest name class is DatabaseDefaultExpressionWithValue...).

The name of field with default value inside Column called default. I thought about other variants like defaultValue, but stoped on this variant, so could be other variants.

…tValueInDb, isDatabaseGenerated into one field

obabichevjb · 2024-11-28T11:02:25Z

It also should be possible to add clientDefaultExpression() variant of the default, but it should be properly checked. I expect some problems with batch inserts, because it would be necessary to check more careful if 2 inserts could be in the same batch insert. But in terms of classes structures it should be one more class ClientColumnDefaultExpression : ClientColumnDefault, ColumnDefaultExpression.

e5l · 2024-11-29T09:05:36Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+interface ColumnDefault<T>
+
+/**
+ * Represents a client-side default value for a column.


please consider adding examples and with functions where these columns are used and references to the functions as well

e5l · 2024-11-29T09:06:28Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+ * Represents a default value for a column generated by a function.
+ */
+interface ColumnDefaultValue<T> : ColumnDefault<T> {
+    val value: () -> T


I would make a method instead cause functional fields are harder to modify without breaking change

e5l · 2024-11-29T09:06:37Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+     * @param fn The transformation function.
+     * @return The transformed column default value.
+     */
+    fun <Wrapped> transform(fn: (Unwrapped) -> Wrapped): ColumnDefault<Wrapped>


fn -> block

joc-a · 2024-11-29T09:56:07Z

documentation-website/Writerside/topics/Breaking-Changes.md

+  - `defaultValueFun != null && dbDefaultValue != null` (knows as `default()`) becomes `DatabaseColumnDefaultExpressionWithValue`
+  - `dbDefaultValue != null` (knows as `defaultExpression()`) becomes `DatabaseColumnDefaultExpression`
+  - `defaultValueFun != null` (knows as `clientDefault()`) becomes `ClientColumnDefaultValue`
+  - `isDatabaseGenerated == true` (knows as `databaseGenerated`) becomes `DatabaseGeneratedColumnDefault`


Did you mean "known" instead of "knows" here?

Suggested change

- `defaultValueFun != null && dbDefaultValue != null` (knows as `default()`) becomes `DatabaseColumnDefaultExpressionWithValue`

- `dbDefaultValue != null` (knows as `defaultExpression()`) becomes `DatabaseColumnDefaultExpression`

- `defaultValueFun != null` (knows as `clientDefault()`) becomes `ClientColumnDefaultValue`

- `isDatabaseGenerated == true` (knows as `databaseGenerated`) becomes `DatabaseGeneratedColumnDefault`

- `defaultValueFun != null && dbDefaultValue != null` (known as `default()`) becomes `DatabaseColumnDefaultExpressionWithValue`

- `dbDefaultValue != null` (known as `defaultExpression()`) becomes `DatabaseColumnDefaultExpression`

- `defaultValueFun != null` (known as `clientDefault()`) becomes `ClientColumnDefaultValue`

- `isDatabaseGenerated == true` (known as `databaseGenerated`) becomes `DatabaseGeneratedColumnDefault`

joc-a · 2024-11-29T10:04:02Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+    override fun <Wrapped> transform(fn: (T) -> Wrapped): ColumnDefault<Wrapped> =
+        ClientColumnDefaultValue { fn(value()) }


Suggested change

override fun <Wrapped> transform(fn: (T) -> Wrapped): ColumnDefault<Wrapped> =

ClientColumnDefaultValue { fn(value()) }

override fun <Wrapped> transform(wrap: (T) -> Wrapped): ColumnDefault<Wrapped> =

ClientColumnDefaultValue { wrap(value()) }

joc-a · 2024-11-29T10:04:16Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+    override fun <Wrapped> transform(fn: (T) -> Wrapped): ColumnDefault<Wrapped> =
+        DatabaseColumnDefaultExpressionWithValue(expression as Expression<Wrapped>) { fn(value()) }


Suggested change

override fun <Wrapped> transform(fn: (T) -> Wrapped): ColumnDefault<Wrapped> =

DatabaseColumnDefaultExpressionWithValue(expression as Expression<Wrapped>) { fn(value()) }

override fun <Wrapped> transform(wrap: (T) -> Wrapped): ColumnDefault<Wrapped> =

DatabaseColumnDefaultExpressionWithValue(expression as Expression<Wrapped>) { wrap(value()) }

joc-a · 2024-11-29T10:04:55Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+fun <Unwrapped, Wrapped> ColumnDefault<Unwrapped>.transform(fn: (Unwrapped) -> Wrapped): ColumnDefault<Wrapped> {
+    @Suppress("UNCHECKED_CAST")
+    return when (this) {
+        is TransformableColumnDefault<Unwrapped> -> transform(fn)
+        else -> this as ColumnDefault<Wrapped>
+    }
+}


Suggested change

fun <Unwrapped, Wrapped> ColumnDefault<Unwrapped>.transform(fn: (Unwrapped) -> Wrapped): ColumnDefault<Wrapped> {

@Suppress("UNCHECKED_CAST")

return when (this) {

is TransformableColumnDefault<Unwrapped> -> transform(fn)

else -> this as ColumnDefault<Wrapped>

}

}

fun <Unwrapped, Wrapped> ColumnDefault<Unwrapped>.transform(wrap: (Unwrapped) -> Wrapped): ColumnDefault<Wrapped> {

@Suppress("UNCHECKED_CAST")

return when (this) {

is TransformableColumnDefault<Unwrapped> -> transform(wrap)

else -> this as ColumnDefault<Wrapped>

}

}

joc-a · 2024-11-29T10:05:59Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+    /**
+     * Creates new default value based on the transformation function
+     *
+     * @param fn The transformation function.
+     * @return The transformed column default value.
+     */
+    fun <Wrapped> transform(fn: (Unwrapped) -> Wrapped): ColumnDefault<Wrapped>


Suggested change

/**

* Creates new default value based on the transformation function

*

* @param fn The transformation function.

* @return The transformed column default value.

*/

fun <Wrapped> transform(fn: (Unwrapped) -> Wrapped): ColumnDefault<Wrapped>

/**

* Creates new default value based on the transformation function

*

* @param wrap The transformation function.

* @return The transformed column default value.

*/

fun <Wrapped> transform(wrap: (Unwrapped) -> Wrapped): ColumnDefault<Wrapped>

joc-a · 2024-11-29T10:29:24Z

How about using "refactor" instead of "chore"?

bog-walk · 2024-12-05T00:12:34Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/Column.kt

-    /** Returns the function that calculates the default value for this column. */
-    var defaultValueFun: (() -> T)? = null
-    internal var dbDefaultValue: Expression<T>? = null
-
-    /** Returns the default value for this column on the database-side. */
-    fun defaultValueInDb() = dbDefaultValue
-
-    internal var isDatabaseGenerated: Boolean = false
-
-    /** Returns whether this column's value will be generated in the database. */
-    fun isDatabaseGenerated() = isDatabaseGenerated


I think it'd be great for users if we could try to go through the standard deprecation cycle where possible, using ReplaceWith options etc. If the var is difficult, it should at least be possible for the two functions.

bog-walk · 2024-12-05T00:27:30Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+ * Represents a database-side column default value generated by expression
+ * that also contains in-memory kotlin value.


Probably better for JB to capitalise Kotlin

bog-walk · 2024-12-05T00:30:49Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+/**
+ * Represents a database-side generated column default value.
+ */
+class DatabaseGeneratedColumnDefault<T> : DatabaseColumnDefault<T>


Might make the difference between this class and the other class below even more clear if the KDocs is clarified more explicitly, like Represents a column default that is a database-side value generated by a pre-defined trigger in the database.

bog-walk · 2024-12-05T00:31:41Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+/**
+ * Represents a default value for a column generated by an SQL expression.
+ */
+interface ColumnDefaultExpression<T> : ColumnDefault<T> {
+    val expression: Expression<T>
+}


Could the property for this interface and the one below also get KDocs please?

bog-walk · 2024-12-05T00:37:36Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+ */
+fun <T> Column<T>.defaultValue() = (default as? ColumnDefaultValue<T>)?.value?.invoke()


Moving forward I think it would be great if we could explicitly add what the non-obvious return type is for all these functions. Especially for any user potentially reading through the source code without using the IDE.

bog-walk · 2024-12-05T01:37:29Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+/**
+ * Gets the default value of the column if it exists.
+ *
+ * @return The default value, or null if none exists.
+ */


As per comments above, I would clarify that "default value" here specifically means the result of a Kotlin function. Or at least amend the @return to say something like: or null if the column either has no default or the default is a database-side function.

bog-walk · 2024-12-05T01:40:50Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+/**
+ * Gets the client-side default value of the column if it exists.
+ *
+ * @return The client-side default value, or null if none exists.
+ */


As per comments above, I would clarify that "default value" here specifically means the result of a Kotlin function. Or at least amend the @return to say something like: or null if the column either has no default or the default is a database-side function.

bog-walk · 2024-12-05T01:45:48Z

exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/ColumnDefault.kt

+/**
+ * Gets the client-side default value or expression if it exists.
+ *
+ * @return The client-side default value or SQL expression, or null if none exists.
+ */
+fun Column<*>.clientDefaultValueOrExpression() = (default as? ClientColumnDefault)?.let {
+    when (it) {
+        is ColumnDefaultValue<*> -> it.value()
+        is ColumnDefaultExpression<*> -> it.expression
+        else -> null
+    }
+}


Given the existence of clientDefaultValue() above, what is the value of including this function now?
From what I've drawn out of the interface hierarchy, it will not be possible currently to ever reach the second when branch. I'm assuming this is here only in the event we add a new class like ClientColumnDefaultExpression, so could this wait to be included until then?

bog-walk · 2024-12-05T01:49:09Z

exposed-dao/src/main/kotlin/org/jetbrains/exposed/dao/EntityClass.kt

-                col.defaultValueFun != null && col !in writeValues && readValues.hasValue(col)
+                col.hasClientDefaultValue() && col !in writeValues && readValues.hasValue(col)


This is an example of some of the confusion I got with the new extension functions.
Why in this case is hasClientDefaultValue() being used instead of just hasClientDefault()?
Would it fail if the latter was accidentally used instead?
Should a user be able to easily understand the difference between using the 2 or are they meant to be interchangeable in basic cases?

bog-walk · 2024-12-05T02:07:02Z

documentation-website/Writerside/topics/Breaking-Changes.md

+* The fields `defaultValueFun`, `dbDefaultValue`, and `isDatabaseGenerated` have been consolidated into a single field, 
+  `default`, of type `ColumnDefault`. The previous setup with the three separate fields often led to inconsistencies 
+  and confusion regarding their various combinations.


In my opinion, this document should only include information that is relevant to the user. It's good to justify the rationale behind our decisions, but 2 of these fields are actually internal and will mean nothing to most users.

For the list below, a user would never have used dbDefaultValue != null or isDatabaseGenerated == true in their code, so I don't think they need to be noted.
The breaking changes that should be mentioned are how:

Column.defaultValueInDb() -> Column.databaseDefaultExpression() Column.isDatabaseGenerated() -> Column.hasDatabaseDefault() // this part I'm not sure how to suggest a migration for... Column.defaultValueFun -> Column.default ???

Also, is there a migration element that is maybe missing?
What if for example the field defaultValueFun was being invoked somewhere in a user's logic like:

Column.defaultValueFun?.invoke() -> ??? // what would this become? // Column.defaultValue() or Column.clientDefaultValue() or Column.clientDefaultValueOrExpression() ? // would the user need to know the underlying ColumnDefault and cast?

Does there maybe need to something like ColumnDefault.invoke() that returns the result of invoking the Kotlin function if it exists or throws an error otherwise?

obabichevjb force-pushed the obabichev/refactor-default-values branch 4 times, most recently from d3f4e28 to d925535 Compare November 27, 2024 14:41

obabichevjb force-pushed the obabichev/refactor-default-values branch 2 times, most recently from 5d032d2 to dbbd5e0 Compare November 28, 2024 10:55

chore! EXPOSED-655 Join column default fields defaultValueFun, defaul…

751ab7c

…tValueInDb, isDatabaseGenerated into one field

obabichevjb force-pushed the obabichev/refactor-default-values branch from dbbd5e0 to 751ab7c Compare November 28, 2024 10:57

obabichevjb requested review from bog-walk, e5l and joc-a November 28, 2024 11:33

e5l approved these changes Nov 29, 2024

View reviewed changes

joc-a reviewed Nov 29, 2024

View reviewed changes

joc-a approved these changes Nov 29, 2024

View reviewed changes

bog-walk reviewed Dec 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore! EXPOSED-655 Join column default fields defaultValueFun, defautValueInDb, isDatabaseGenerated into one field #2319

chore! EXPOSED-655 Join column default fields defaultValueFun, defautValueInDb, isDatabaseGenerated into one field #2319

obabichevjb commented Nov 27, 2024

obabichevjb commented Nov 27, 2024

obabichevjb commented Nov 28, 2024

e5l Nov 29, 2024

e5l Nov 29, 2024

e5l Nov 29, 2024

joc-a Nov 29, 2024

joc-a Nov 29, 2024

joc-a Nov 29, 2024

joc-a Nov 29, 2024

joc-a Nov 29, 2024

joc-a commented Nov 29, 2024

bog-walk Dec 5, 2024

bog-walk Dec 5, 2024

bog-walk Dec 5, 2024 •

edited

Loading

bog-walk Dec 5, 2024

bog-walk Dec 5, 2024

bog-walk Dec 5, 2024 •

edited

Loading

bog-walk Dec 5, 2024 •

edited

Loading

bog-walk Dec 5, 2024 •

edited

Loading

bog-walk Dec 5, 2024

bog-walk Dec 5, 2024

		override fun <Wrapped> transform(fn: (T) -> Wrapped): ColumnDefault<Wrapped> =
		ClientColumnDefaultValue { fn(value()) }

		override fun <Wrapped> transform(fn: (T) -> Wrapped): ColumnDefault<Wrapped> =
		DatabaseColumnDefaultExpressionWithValue(expression as Expression<Wrapped>) { fn(value()) }

		* Represents a database-side column default value generated by expression
		* that also contains in-memory kotlin value.

		*/
		fun <T> Column<T>.defaultValue() = (default as? ColumnDefaultValue<T>)?.value?.invoke()

		col.defaultValueFun != null && col !in writeValues && readValues.hasValue(col)
		col.hasClientDefaultValue() && col !in writeValues && readValues.hasValue(col)

chore! EXPOSED-655 Join column default fields defaultValueFun, defautValueInDb, isDatabaseGenerated into one field #2319

Are you sure you want to change the base?

chore! EXPOSED-655 Join column default fields defaultValueFun, defautValueInDb, isDatabaseGenerated into one field #2319

Conversation

obabichevjb commented Nov 27, 2024

Description

Type of Change

Related Issues

obabichevjb commented Nov 27, 2024

obabichevjb commented Nov 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joc-a commented Nov 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bog-walk Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bog-walk Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

bog-walk Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

bog-walk Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bog-walk Dec 5, 2024 •

edited

Loading

bog-walk Dec 5, 2024 •

edited

Loading

bog-walk Dec 5, 2024 •

edited

Loading

bog-walk Dec 5, 2024 •

edited

Loading