Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
levilansing opened this issue Jun 28, 2024 · 2 comments
Assignees

Comments

@levilansing
Copy link

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:

override fun arguments(): Iterable<Iterable<Pair<IColumnType<*>, Any?>>> = listOf(
args.map { (columnType, value) ->
columnType.apply { nullable = true } to value
}
)

@bog-walk bog-walk self-assigned this Jun 30, 2024
@bog-walk
Copy link
Member

bog-walk commented Jul 2, 2024

@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?

@levilansing
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants