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

DAO and DB defaults don't support in batch inserts #1599

Closed
smelfungus opened this issue Sep 20, 2022 · 7 comments · Fixed by #1844
Closed

DAO and DB defaults don't support in batch inserts #1599

smelfungus opened this issue Sep 20, 2022 · 7 comments · Fixed by #1844
Assignees

Comments

@smelfungus
Copy link

Hello!
What would be the correct strategy for DAO to rely on DB defaults and not force clients to define them?

object TestTable : LongIdTable(name = "test_table") {
    val seed = long("seed")
}

seed is always generated on the DB side, and we want clients to know nothing about the logic behind the seed generation nor set some fallbacks/defaults. The insert attempt results in:

Can't add a new batch because columns: seed don't have default values. DB defaults don't support in batch inserts
@smelfungus
Copy link
Author

smelfungus commented Sep 20, 2022

Or, if it is technically impossible, maybe there's a way to avoid batchInsert in flushInserts and use some sort of strategy that will allow DB defaults with no client backup? I can see that Column<T>.nullable() is one of the tricks since it impacts isDefaultable, but that would be so nice to have something like defaultable() to keep the column not-null as it really is.

@AlexeySoshin
Copy link
Contributor

Could you elaborate a bit more on that, please?
You mention DAO, but you posted just your table definition, not your entity definition.
And if you can, please also post how exactly you're making those batch inserts, so it would be easier to reproduce.

Thanks!

@smelfungus
Copy link
Author

@AlexeySoshin thank you for getting back to me!
So we have a table definition like this one:

object MyTable : LongIdTable(name = "my_table") {
    ....
    val seed = long("seed")
    ....
}

And corresponding entity of this type:

class MyEntity(
    id: EntityID<Long>
) : LongEntity(id) {
    ...
    var seed by MyTable.seed
    ...

    companion object : LongEntityClass<MyEntity>(MyTable)
}

The database defines seed as:

ALTER TABLE my_table
    ADD seed bigint DEFAULT ... NOT NULL;

When we're trying to insert a new Entity as:

MyEntity.new(...)

We're getting the following error:

org.jetbrains.exposed.sql.statements.BatchDataInconsistentException: Can't add a new batch because columns: my_table.seed don't have default values. DB defaults don't support in batch inserts

While we can apply val seed = long("seed").nullable() nullable workaround to trick isDefaultable Exposed mechanism, we're not too happy about this solution since it makes our seed nullable whereas it is not nullable by design in DB definition and requires some client-side assertions. That's why I wonder whether there's a chance to add some sort of a defaultable() column mark to explicitly tell Exposed that this column is not nullable and is always generated?

@AlexeySoshin
Copy link
Contributor

Thank you.
After looking at the code some more, I don't think this is currently supported.
Exposed expects to set values for all columns it knows about. If you remove seed column from MyTable, it will work, but from your other question I guess you still would want to read the data from that column, just not to set it.

Seems that what you'd like is to have something like @Transient from JPA:
https://www.baeldung.com/jpa-transient-ignore-field

So you would define

object TestTable : LongIdTable(name = "test_table") {
    val seed = long("seed").transient()
}

And that field will be omitted from inserts / updates

@smelfungus
Copy link
Author

@AlexeySoshin, I'm afraid the idea behind JPA's @Transient is more about dropping the annotated field from DB persistence (both reads and writes) and just using this field with some application runtime logic?

Two vital points are coming to my mind for our case:

  1. I think we eventually want to have this DB-default field updatable (support both var and val table definition, let's imagine we want to change the default value of the entity for some reason after the insert)
  2. At the same time, we don't want to make this field nullable since it breaks the definition and integrity (if the DB column is clearly defined as NOT NULL). However, I think there are cases where DB definition will allow both nullable and default expressions to be used together, so potentially we do not want to drop .nullable() support for DB-default fields.

So, long story short, we would have the Exposed to be able to define:
DB-default + nullable or not-null + mutable or read-only field. Some sort of .generated() or .defaultable()?
Maybe EntityID approach may give us some inspiration for such case? It feels pretty similar. It is also generated on the DB side with DB logic/sequences and does not require us to define client-side defaults or explicit values.

@joc-a joc-a self-assigned this Aug 17, 2023
@joc-a
Copy link
Collaborator

joc-a commented Aug 17, 2023

The database defines seed as:

ALTER TABLE my_table
    ADD seed bigint DEFAULT ... NOT NULL;

@smelfungus Just curious, how is this default being defined? Are you using any Exposed API for it or doing it through raw SQL with exec function?

@smelfungus
Copy link
Author

@joc-a, sure, we're using the database-first approach, so our database structure is defined by some external schema and migrations managed by Flyway. Respectively, this column is defined as:

ALTER TABLE xxx
    ADD seed bigint DEFAULT FLOOR(RANDOM() * yyy)::bigint NOT NULL;

The thing we were trying to achieve here is to make the Exposed respect database defaults and not make us redefine anything in the code with something like defaultable() column modifier.

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

Successfully merging a pull request may close this issue.

3 participants