Exposed icon indicating copy to clipboard operation
Exposed copied to clipboard

Using Transaction.exec changes all referenced columns to be nullable going forward

Open levilansing opened this issue 1 year ago • 2 comments

Feel free to tell me I'm doing it wrong, but when I try to use exposed to generate prepared statements that I execute directly on the transaction, it changes all the column types that I inserted to be nullable for all future inserts. My use case is that I want to support inserts that ignore conflicts, here's a contrived example:

object IdTests : Table() {
    val id = long("id").autoIncrement()
    val value = varchar("value", 255)

    override val primaryKey = PrimaryKey(id)
}

object IdTestService {
    fun createSchema() {
        transaction {
            SchemaUtils.create(IdTests)
        }
    }

    fun insertTest() {
        transaction {
            val statement1 = InsertStatement<Number>(IdTests).apply {
                this[IdTests.value] = "test1"
            }
            this.exec(statement1.prepareSQL(this), statement1.arguments().first())
            // INSERT INTO idtests ("value") VALUES ('test1')

            val statement2 = InsertStatement<Number>(IdTests).apply {
                this[IdTests.id] = -2
                this[IdTests.value] = "test2"
            }
            val sql = """
                ${statement2.prepareSQL(this)}
                ON CONFLICT DO NOTHING
                RETURNING ${IdTests.id.name}
            """.trimIndent()
            this.exec(sql, statement2.arguments().first(), StatementType.MULTI)
            // INSERT INTO idtests (id, "value") VALUES (-2, 'test2')

            val statement3 = InsertStatement<Number>(IdTests).apply {
                this[IdTests.value] = "test3"
            }
            this.exec(statement3.prepareSQL(this), statement3.arguments().first())
            // INSERT INTO idtests (id, "value") VALUES (NULL, 'test3')
            // ERROR: null value in column "id" of relation "idtests" violates not-null constraint
        }
    }
}

Notice the generated SQL for statement1 and statement3. In statement1, exposed knows the id column has a default value in the DB and leaves it out of the insert statement, but in statement3 (and all future insert statements), it will try to insert null because the column type has been modified permanently in the execution of statement2.

This is highly problematic for any column that uses a DB generated default, as all future inserts will try to insert NULL.

I think I've tracked it down to this recent change:

https://github.com/JetBrains/Exposed/blob/3fafec1a31f88b1ea4fda13fdf17c6dc182e5693/exposed-core/src/main/kotlin/org/jetbrains/exposed/sql/Transaction.kt#L253-L257

levilansing avatar Jun 28 '24 00:06 levilansing

@levilansing Thanks very much for bringing this use case to our attention.

exec() arguments have been typically tried and tested using stand-alone/throw-away columnTypes via manually-defined lists of pairs. The logic will need to be refactored if Exposed is also being used for SQL-string statement generation, with actually existing column objects being used to provide the arguments. I'll look into making this use case work as well.

Also, if you wouldn't mind sharing, is there a specific reason that insertIgnore() does not suffice for your use case?

bog-walk avatar Jul 02 '24 19:07 bog-walk

Thanks for looking into this @bog-walk. I'm trying to build responsibly and want to limit manual SQL as much as possible when I need something custom. I personally feel the best balance is to augment exposed generated SQL so I get the safety of exposed using my data model exactly as it is defined in one place with some flexibility to create more optimal queries for my needs. I understand now that may be contrary to the intent of exec(). I did not consider creating duplicates of my model columns for each manual query, but I did create my own duplicate of exec as a work-around.

For my actual use cases right now (w/postgres), a batch insert ignore, and a handful of update statements that return specific columns including DB generated values. I was on v 0.48, but after doing some more research now I see updateReturning is now a thing and batchInserts have an ignore option, so I will update and give those a shot! I do expect to have more use cases in the future when I get to more analytical queries, but this is all for now.

levilansing avatar Jul 05 '24 01:07 levilansing