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

Insert body breaks the order of fields #6

Closed
HoffiMuc opened this issue Dec 6, 2021 · 11 comments
Closed

Insert body breaks the order of fields #6

HoffiMuc opened this issue Dec 6, 2021 · 11 comments
Labels
bug Something isn't working

Comments

@HoffiMuc
Copy link

HoffiMuc commented Dec 6, 2021

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
Copy link
Member

dzikoysk commented Dec 6, 2021

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:

  • Remember to keep the same order of fields in insert & upsert body
  • Default values are not supported

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

upsertCommonFields(it, this, dto)
updateBaseDTOmetadata(it, this, dto)

@HoffiMuc
Copy link
Author

HoffiMuc commented Dec 6, 2021

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
Copy link
Member

dzikoysk commented Dec 6, 2021

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
Copy link
Author

HoffiMuc commented Dec 6, 2021

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
Copy link
Member

dzikoysk commented Dec 6, 2021

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 dzikoysk added the bug Something isn't working label Dec 6, 2021
@dzikoysk dzikoysk changed the title usage in non DAO fails Insert body breaks the order of fields Dec 6, 2021
@dzikoysk
Copy link
Member

dzikoysk commented Dec 6, 2021

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

@HoffiMuc
Copy link
Author

HoffiMuc commented Dec 6, 2021

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
Copy link
Member

dzikoysk commented Dec 6, 2021

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:
JetBrains/Exposed#167 (comment)
I didn't test it, but that's built-in function, so there is a chance it could cover your case.

@HoffiMuc
Copy link
Author

HoffiMuc commented Dec 6, 2021

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
Copy link

prsltd commented Jul 8, 2022

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
Copy link
Member

dzikoysk commented Jul 8, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants