Improve error reporting when user inserts NULL into non-nullable column

* Exception message would include the relevant table and column name
* LiteralOp(...) and QueryParameter(...) would fail if user creates NULL value for non-nullable type
* Single-value QueryBuilder#registerArgument is slightly faster as it no longer creates list

fixes #1268

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

@@ -46,7 +46,7 @@ interface IColumnType {
      */
     fun valueToString(value: Any?): String = when (value) {
         null -> {
-            check(nullable) { "NULL in non-nullable column" }
+            check(nullable) { "NULL in non-nullable column with type ${sqlType()}" }
             "NULL"
         }
         DefaultValueMarker -> "DEFAULT"

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

@@ -51,34 +51,66 @@ class QueryBuilder(
     operator fun Expression<*>.unaryPlus(): QueryBuilder = append(this)
 
     /** Adds the specified [argument] as a value of the specified [column]. */
-    fun <T> registerArgument(column: Column<*>, argument: T) {
+    fun registerArgument(column: ExpressionWithColumnType<*>, argument: Any?) {
         when (argument) {
-            is Expression<*> -> append(argument)
-            DefaultValueMarker -> append(TransactionManager.current().db.dialect.dataTypeProvider.processForDefaultValue(column.dbDefaultValue!!))
-            else -> registerArgument(column.columnType, argument)
+            is Expression<*> -> {
+                require(column.columnType.nullable || argument !is LiteralOp<*> || argument.value != null) {
+                    "Column ${column.identity} does not support NULLs, so cannot register null literal $argument"
+                }
+                append(argument)
+            }
+            DefaultValueMarker -> {
+                require(column is Column<*>) {
+                    "DefaultValueMarker is supported only for Column<*>, given argument is $column"
+                }
+                append(TransactionManager.current().db.dialect.dataTypeProvider.processForDefaultValue(column.dbDefaultValue!!))
+            }
+            else -> {
+                require(column.columnType.nullable || argument != null) {
+                    "Column ${column.identity} does not support NULLs"
+                }
+                @Suppress("DEPRECATION")
+                registerArgument(column.columnType, argument)
+            }
         }
     }
 
+    private val ExpressionWithColumnType<*>.identity: String
+        get() = if (this is Column<*>) "${table.tableName}.$name" else toString()
+
     /** Adds the specified [argument] as a value of the specified [sqlType]. */
-    fun <T> registerArgument(sqlType: IColumnType, argument: T): Unit = registerArguments(sqlType, listOf(argument))
+    @Deprecated(
+        level = DeprecationLevel.WARNING,
+        message = "Prefer registerArgument(Column, ...) and registerArgument(ExpressionWithColumnType, ...) since they have better error reporting"
+    )
+    @Suppress("DeprecatedCallableAddReplaceWith")
+    fun registerArgument(sqlType: IColumnType, argument: Any?) {
+        if (!prepared) {
+            +sqlType.valueToString(argument)
+            return
+        }
+        require(argument != null || sqlType.nullable) {
+            "Can't register NULL value for non-nullable type $sqlType"
+        }
+        +"?"
+        _args += sqlType to argument
+    }
 
     /** Adds the specified sequence of [arguments] as values of the specified [sqlType]. */
+    @Deprecated(
+        message = "Replace with [SingleValueInListOp]",
+        level = DeprecationLevel.ERROR,
+        replaceWith = ReplaceWith("org.jetbrains.exposed.sql.ops.SingleValueInListOp")
+    )
     fun <T> registerArguments(sqlType: IColumnType, arguments: Iterable<T>) {
-        fun toString(value: T) = when {
-            prepared && value is String -> value
-            else -> sqlType.valueToString(value)
+        if (!prepared) {
+            arguments.appendTo { +sqlType.valueToString(it) }
+            return
+        }
+        arguments.appendTo {
+            +"?"
+            _args += sqlType to it
         }
-
-        arguments.map { it to toString(it) }
-            .sortedBy { it.second }
-            .appendTo {
-                if (prepared) {
-                    _args.add(sqlType to it.first)
-                    append("?")
-                } else {
-                    append(it.second)
-                }
-            }
     }
 
     override fun toString(): String = internalBuilder.toString()

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

@@ -1,6 +1,7 @@
 package org.jetbrains.exposed.sql
 
 import org.jetbrains.exposed.dao.id.EntityID
+import org.jetbrains.exposed.sql.ops.SingleValueInListOp
 import org.jetbrains.exposed.sql.vendors.OracleDialect
 import org.jetbrains.exposed.sql.vendors.SQLServerDialect
 import org.jetbrains.exposed.sql.vendors.currentDialect
@@ -408,35 +409,10 @@ class InListOrNotInListOp<T>(
     /** Returns `true` if the check is inverted, `false` otherwise. */
     val isInList: Boolean = true
 ) : Op<Boolean>(), ComplexExpression {
-    override fun toQueryBuilder(queryBuilder: QueryBuilder): Unit = queryBuilder {
-        list.iterator().let { i ->
-            if (!i.hasNext()) {
-                if (isInList) {
-                    +FALSE
-                } else {
-                    +TRUE
-                }
-            } else {
-                val first = i.next()
-                if (!i.hasNext()) {
-                    append(expr)
-                    when {
-                        isInList -> append(" = ")
-                        else -> append(" != ")
-                    }
-                    registerArgument(expr.columnType, first)
-                } else {
-                    append(expr)
-                    when {
-                        isInList -> append(" IN (")
-                        else -> append(" NOT IN (")
-                    }
-                    registerArguments(expr.columnType, list)
-                    append(")")
-                }
-            }
-        }
-    }
+    private val impl = SingleValueInListOp(expr, list, isInList)
+
+    override fun toQueryBuilder(queryBuilder: QueryBuilder): Unit =
+        impl.toQueryBuilder(queryBuilder)
 }
 
 // Literals
@@ -449,6 +425,11 @@ class LiteralOp<T>(
     /** Returns the value being used as a literal. */
     val value: T
 ) : ExpressionWithColumnType<T>() {
+    init {
+        require(value != null || columnType.nullable) {
+            "Can't create NULL literal for non-nullable type $columnType"
+        }
+    }
     override fun toQueryBuilder(queryBuilder: QueryBuilder): Unit = queryBuilder { +columnType.valueToString(value) }
 }
 
@@ -506,7 +487,15 @@ class QueryParameter<T>(
     /** Returns the column type of this expression. */
     val sqlType: IColumnType
 ) : Expression<T>() {
-    override fun toQueryBuilder(queryBuilder: QueryBuilder): Unit = queryBuilder { registerArgument(sqlType, value) }
+    init {
+        require(value != null || sqlType.nullable) {
+            "Can't create NULL query parameter for non-nullable type $sqlType"
+        }
+    }
+    override fun toQueryBuilder(queryBuilder: QueryBuilder): Unit = queryBuilder {
+        @Suppress("DEPRECATION")
+        registerArgument(sqlType, value)
+    }
 }
 
 /** Returns the specified [value] as a query parameter with the same type as [column]. */

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

@@ -59,12 +59,12 @@ abstract class InListOrNotInListBaseOp<V> (
     private fun QueryBuilder.registerValues(values: List<Any?>) {
         val singleColumn = columnTypes.singleOrNull()
         if (singleColumn != null)
-            registerArgument(singleColumn.columnType, values.single())
+            registerArgument(singleColumn as ExpressionWithColumnType<Any?>, values.single())
         else {
             append("(")
             columnTypes.forEachIndexed { index, columnExpression ->
                 if (index != 0) append(", ")
-                registerArgument(columnExpression.columnType, values[index])
+                registerArgument(columnExpression as ExpressionWithColumnType<Any?>, values[index])
             }
             append(")")
         }

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

@@ -3,7 +3,6 @@ package org.jetbrains.exposed.sql.statements
 
 import org.jetbrains.exposed.dao.id.EntityID
 import org.jetbrains.exposed.sql.*
-import java.util.*
 
 /**
  * @author max
@@ -13,28 +12,28 @@ abstract class UpdateBuilder<out T>(type: StatementType, targets: List<Table>) :
     protected val values: MutableMap<Column<*>, Any?> = LinkedHashMap()
 
     open operator fun <S> set(column: Column<S>, value: S) {
-        when {
-            values.containsKey(column) -> error("$column is already initialized")
-            !column.columnType.nullable && value == null -> error("Trying to set null to not nullable column $column")
-            else -> {
-                column.columnType.validateValueBeforeUpdate(value)
-                values[column] = value
-            }
+        require(column !in values) { "$column is already initialized" }
+        column.validateValueBeforeUpdate(value)
+        values[column] = value
+    }
+
+    private fun Column<*>.validateValueBeforeUpdate(value: Any?) {
+        require(columnType.nullable || value != null && !(value is LiteralOp<*> && value.value == null)) {
+            "Can't set NULL into non-nullable column ${table.tableName}.$name, column type is $columnType"
         }
+        columnType.validateValueBeforeUpdate(value)
     }
 
     @JvmName("setWithEntityIdExpression")
     operator fun <S : Comparable<S>> set(column: Column<out EntityID<S>?>, value: Expression<out S?>) {
-        require(!values.containsKey(column)) { "$column is already initialized" }
-        column.columnType.validateValueBeforeUpdate(value)
-        values[column] = value
+        @Suppress("UNCHECKED_CAST")
+        set(column as Column<Any?>, value as Any?)
     }
 
     @JvmName("setWithEntityIdValue")
     operator fun <S : Comparable<S>> set(column: Column<out EntityID<S>?>, value: S?) {
-        require(!values.containsKey(column)) { "$column is already initialized" }
-        column.columnType.validateValueBeforeUpdate(value)
-        values[column] = value
+        @Suppress("UNCHECKED_CAST")
+        set(column as Column<Any?>, value as Any?)
     }
 
     /**
@@ -47,19 +46,21 @@ abstract class UpdateBuilder<out T>(type: StatementType, targets: List<Table>) :
     open operator fun <S> set(column: Column<S>, value: Expression<out S>) = update(column, value)
 
     open operator fun <S> set(column: CompositeColumn<S>, value: S) {
-        column.getRealColumnsWithValues(value).forEach { (realColumn, itsValue) -> set(realColumn as Column<Any?>, itsValue) }
+        column.getRealColumnsWithValues(value).forEach { (realColumn, itsValue) ->
+            @Suppress("UNCHECKED_CAST")
+            set(realColumn as Column<Any?>, itsValue)
+        }
     }
 
     open fun <S> update(column: Column<S>, value: Expression<out S>) {
-        require(!values.containsKey(column)) { "$column is already initialized" }
-        column.columnType.validateValueBeforeUpdate(value)
-        values[column] = value
+        @Suppress("UNCHECKED_CAST")
+        set(column as Column<Any?>, value as Any?)
     }
 
     open fun <S> update(column: Column<S>, value: SqlExpressionBuilder.() -> Expression<out S>) {
-        require(!values.containsKey(column)) { "$column is already initialized" }
-        val expression = SqlExpressionBuilder.value()
-        column.columnType.validateValueBeforeUpdate(expression)
-        values[column] = expression
+        // Note: the implementation builds value before it verifies if the column is already initialized
+        // however it makes the implementation easier and the optimization is not that important since
+        // the exceptional case should be rare.
+        set(column, SqlExpressionBuilder.value())
     }
 }

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

@@ -62,7 +62,7 @@ internal open class MysqlFunctionProvider : FunctionProvider() {
     override fun replace(table: Table, data: List<Pair<Column<*>, Any?>>, transaction: Transaction): String {
         val builder = QueryBuilder(true)
         val columns = data.joinToString { transaction.identity(it.first) }
-        val values = builder.apply { data.appendTo { registerArgument(it.first.columnType, it.second) } }.toString()
+        val values = builder.apply { data.appendTo { registerArgument(it.first, it.second) } }.toString()
         return "REPLACE INTO ${transaction.identity(table)} ($columns) VALUES ($values)"
     }
 

exposed-tests/src/test/kotlin/org/jetbrains/exposed/sql/tests/shared/dml/InsertTests.kt

@@ -17,7 +17,9 @@ import org.jetbrains.exposed.sql.tests.shared.expectException
 import org.jetbrains.exposed.sql.vendors.MysqlDialect
 import org.junit.Test
 import java.math.BigDecimal
+import kotlin.test.assertContains
 import kotlin.test.assertEquals
+import kotlin.test.assertFailsWith
 import kotlin.test.assertNotNull
 
 class InsertTests : DatabaseTestsBase() {
@@ -201,6 +203,63 @@ class InsertTests : DatabaseTestsBase() {
         }
     }
 
+    @Test
+    fun testInsertNullIntoNonNullableColumn() {
+        val cities = object : IntIdTable("cities") {
+        }
+        val users = object : IntIdTable("users") {
+            val cityId = reference("city_id", cities)
+        }
+
+        withTables(users, cities) {
+            // This is needed so valid inserts to users to succeed
+            cities.insert {
+                it[id] = 42
+            }
+            users.insert {
+                // The assertion would try inserting null, and it ensures the insert would fail before the statement is even generated
+                it.assertInsertNullFails(cityId)
+                // This is needed for insert statement to succeed
+                it[cityId] = 42
+            }
+        }
+    }
+
+    private fun <T : Comparable<T>> UpdateBuilder<Int>.assertInsertNullFails(column: Column<EntityID<T>>) {
+        fun assertInsertNullFails(column: Column<out EntityID<*>>, block: () -> Unit) {
+            val e = assertFailsWith<IllegalArgumentException>(
+                """
+                Unfortunately, type system can't protect from inserting null here
+                since the setter is declared as set(column: Column<out EntityID<S>?>, value: S?),
+                and there's no way to tell that nullness of both arguments should match, so expecting it[${column.name}] = null
+                to fail at runtime
+                """.trimIndent()
+            ) {
+                block()
+            }
+            val message = e.toString()
+            assertContains(
+                message,
+                "${column.table.tableName}.${column.name}", ignoreCase = true,
+                "Exception message should contain table and column name"
+            )
+            assertContains(message, column.columnType.toString(), ignoreCase = true, "Exception message should contain column type")
+        }
+
+        require(!column.columnType.nullable) {
+            "Assertion works for non-nullable columns only. Given column ${column.table.tableName}.${column.name} is nullable ${column.columnType}"
+        }
+        assertInsertNullFails(column) {
+            // This is written explicitly to demonstrate that the code compiles, yet it fails in the runtime
+            // This call resolves to set(column: Column<out EntityID<S>?>, value: S?)
+            this[column] = null
+        }
+        val nullableType = EntityIDColumnType(column).apply { nullable = true }
+        assertInsertNullFails(column) {
+            this[column] = LiteralOp(nullableType, null)
+        }
+    }
+
     @Test fun testInsertWithPredefinedId() {
         val stringTable = object : IdTable<String>("stringTable") {
             override val id = varchar("id", 15).entityId()