reposilite-playground / exposed-upsert

Upsert DSL extension for Exposed, Kotlin SQL framework
The Unlicense
34 stars 3 forks source link

Insert body breaks the order of fields #6

Closed HoffiMuc closed 1 year ago

HoffiMuc commented 2 years ago

a) repository link at bottom of page https://androidrepo.com/repo/dzikoysk-exposed-upsert is still broken

b) if I have non DAO exposed Tables, is your upsert supposed to work also?

I have DSL defined tables (non IdTables)

abstract class BaseTable(name: String) : Table(name) {
    val dtoModCount = long("dto_mod_count")
    val dtoOptimisticLockId = long("dto_optimistic_lock_id")
    val dtoCreationDate = timestamp("dto_creation_date")
    val dtoCreationUser = varchar("dto_creation_user", VARCHAR_MEDIUM)
    val dtoModDate = timestamp("dto_mod_date")
    val dtoModUser = varchar("dto_mod_user", VARCHAR_MEDIUM)
}

object IssuesTable : BaseTable(name = "ISSUES") {
    val key = varchar("key", VARCHAR_SMALL)
    val project = varchar("project", VARCHAR_SMALL)
    ...
    override val primaryKey = PrimaryKey(key, name = "PK_Issue_key")
    private fun upsertCommonFields(it: InsertStatement<Number>, table: IssuesTable, dto: Issue) {
        it[table.project] = dto.project
        ...
    fun upsert(dto: Issue) {
        IssuesTable.upsert(conflictColumn = IssuesTable.key,
            insertBody = {
                it[IssuesTable.key] = dto.key
                upsertCommonFields(it, this, dto)
                updateBaseDTOmetadata(it, this, dto)
            },
            updateBody = {
                upsertCommonFields(it, this, dto)
                updateBaseDTOmetadata(it, this, dto)
            }
        )
    }

insert works, but update totally confuses column names and the values to put into them.

dzikoysk commented 2 years ago

I do not maintain "https://androidrepo.com/repo/dzikoysk-exposed-upsert", that's probably just an automated site to scrap public repositories from GitHub.

To make upsert work, you have to fulfill these requirements:

Does your methods have a custom logic/optional fields? I mean the logic behind:

upsertCommonFields(it, this, dto)
updateBaseDTOmetadata(it, this, dto)
HoffiMuc commented 2 years ago

both of these methods to just have it[table.xxx] = dto.xxx statements.

except for the primary key field itself, I want to insert/upsert ALL fields of the table.

So I came up with these helper methods to assure that a) I only have to write them once b) they are the same and in the same order

only differene is the primary key itself, which is part of the insert, but obviously not of the update
insert fails if I move the it[IssuesTable.key] = dto.key to the common method.

but the actual update statement that I see in the logs is not "off by 1" it is totally different.

dzikoysk commented 2 years ago

The problem with Exposed is that its internals are so... quite messy, not gonna like. It has a really strict policy about indexes, so if one of methods have extra entries, then it fails. I never tried to upsert table that extends another table, so there is a chance that it could be the root of the problem, as your insert & upsert body looks fine I think.

HoffiMuc commented 2 years ago

well the inherited table columns are the same for every/any class, they just get added to each table as columns (don't know if there is something special here ... tried it, and it worked ... not intended to have any "meaning" more than I am lazy typing, so these columns get added to any table I deal with ...

more seams like the index of the insertStatement is iterated over in kind of a HashSet type of manner ... so the indexes are totally different ...

INSERT INTO issues (
assignee_name, beginn_change_time_cf13521, created, creator_name, db_id, description, dto_creation_date, dto_creation_user, dto_mod_count, dto_mod_date, dto_mod_user, dto_optimistic_lock_id, endchangetime_cf13522, impact_cf14420, issuetype_name, `key`, loesungsbeschreibung_cf13523, priority_name, progress_progress, project, reporter_name, resolution_name, resolutiondate, resubmissiondate_cf14621, `self`, status_name, summary, timeestimate, timetracking_remainingestimateseconds, updated, vf_system_cf14520
) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON DUPLICATE KEY UPDATE
project=?, db_id=?, `self`=?, assignee_name=?, created=?, creator_name=?, description=?, issuetype_name=?, progress_progress=?, priority_name=?, reporter_name=?, resolution_name=?, resolutiondate=?, status_name=?, summary=?, timeestimate=?, timetracking_remainingestimateseconds=?, updated=?, beginn_change_time_cf13521=?, endchangetime_cf13522=?, loesungsbeschreibung_cf13523=?, impact_cf14420=?, vf_system_cf14520=?, resubmissiondate_cf14621=?, dto_creation_date=?, dto_creation_user=?, dto_mod_count=?, dto_mod_date=?, dto_mod_user=?, dto_optimistic_lock_id=?

the ones in the update are as they appear in the insertbody/updatebody
but the ones in insert are alphabetically, it seems

dzikoysk commented 2 years ago

I wouldn't be surprised if they'd sort this alphabetically and we can't do much about it if so :/ I think that the best way to go for you is just to move to 2 queries (select to find id and then create/update), that's what I did in my project to:

https://github.com/dzikoysk/reposilite/blob/dec6e2f5edc3a45682c18de48a12378f363c7002/reposilite-backend/src/main/kotlin/com/reposilite/token/infrastructure/SqlAccessTokenRepository.kt#L85

dzikoysk commented 2 years ago

Btw, could you try to sort your fields in insert & update alphabetically body? I'm just curious if it would work

HoffiMuc commented 2 years ago

well, so that means:

if you're using DAO approach, there seems to be some "magic" in IdTable that your upserts work

but this "magic" doesn't happen in normal DSL Tables ...

As for ordering the fields ... just tried it ... and it fails because:

in the insert, you need to specify the primary key field,
but in the update you're not allowed to.

so the ordering "explodes" at the alphabetical position where your PK joins the concert.

dzikoysk commented 2 years ago

To be honest I always used it with DSL and never with DAO. Your case is something new, because it's not always sorting these fields automatically 🤔. Like I said, I probably won't be able to fix this as this is something about Exposed's internals that are full of dirty magic, so I'd suggest to just move on from this solution and safely cover it with these 2 queries if possible.

Also, did you try to use the replace method, that's something that has been added lately: https://github.com/JetBrains/Exposed/issues/167#issuecomment-922145082 I didn't test it, but that's built-in function, so there is a chance it could cover your case.

HoffiMuc commented 2 years ago

no blame on you at all ! Nothing you can do on it (at least not without considerable effort, which is definitely not worth it)

but was worth a lucky shot for me to try it :) (will have a look at replace

thanx for you super fast response and help!

prsltd commented 2 years ago

I've run into the field ordering issue, and think I might have solved it.

My understanding is

Assuming :

Table.upsert( conflictIndex = ... insertBody = { it[B] = 2 it[C] = 3 it[A] = 1 it[D] = 4 }, updateBody = { it[B] = 2 it[C] = 3 it[A] = 1 it[D] = 4 })

Exposed sorts the fields into alphabetical order. When the SQL is generated for the insertBody, these are correctly output as follows

INSERT INTO tablename ("A","B","C","D") VALUES(1,2,3,4)

When it comes to the updateBody, the field names are not sorted and are output in the order they are defined , but the values are output in the sorted order leading to corruption in the data :

ON CONFLICT ON CONSTRAINT unique_constraint DO UPDATE SET B=1, C=2, A=3, D=4

I added the following to doUpdateSet :

append(" DO UPDATE SET ") updateValues.entries .filter {it.key !in indexColumns } .sortedBy {it.key.name} // This appears to fix the issue .joinTo(this) { (column, value) -> ....

This now correctly generates :

ON CONFLICT ON CONSTRAINT unique_constraint DO UPDATE SET A=1, B=2, C=3, D=4

I'm not sure if this fixes the whole problem, but in my case it appears to work.

dzikoysk commented 2 years ago

Do you want to try to submit PR? :) We have some integration tests on CI against all db targets, so we could verify if it works.