fix: EXPOSED-811 argument "where" in "batchUpsert" have no way to use it #2529
Conversation
obabichevjb
force-pushed
the
obabichev/exposed-811-batch-upsert-excluded
branch
from
abf80f1 to
de2f7f9
Compare
| * @sample org.jetbrains.exposed.v1.sql.tests.shared.dml.UpsertTests.testUpsertWithManualUpdateUsingInsertValues | ||
| * @sample org.jetbrains.exposed.v1.tests.shared.dml.UpsertTests.testUpsertWithManualUpdateUsingInsertValues |
There was a problem hiding this comment.
A side question: Does this actually resolve for you in the IDE?
I know the package is correct, but it still shows 'Unresolved error' for me.
There was a problem hiding this comment.
yep, Idea resolves this class, and I can navigate to the class from that place.
Before I fixed it, it was underlined with a yellow line (like a warning), that's why I noticed the mistake
There was a problem hiding this comment.
I might ask you to help me debug my IDE next week then, because it still has a yellow line even with the correct package and the KDocs still shows 'Unresolved' when hovered over for me.
exposed-core/src/main/kotlin/org/jetbrains/exposed/v1/core/statements/UpsertStatement.kt
Outdated
Show resolved
Hide resolved
| // SQL seems correct, but the statement fails on POSTGRESNG | ||
| withTables(excludeSettings = TestDB.ALL_MYSQL_LIKE + upsertViaMergeDB + TestDB.POSTGRESQLNG, tester) { |
There was a problem hiding this comment.
Probably a bug with batchUpsert() | BatchInsertStatement that should be logged.
The test passes in the following cases:
- If the second use of
testerBatchUpsert()is replaced with 2 identical but separateupsert()operations. - If the
whereblock is omitted.
It's probably because of an equality issue with the 2 last InsertValue instances in the statement. Confirmed by adding another integer column and replacing the insertValue() in where with it, made tests pass.
There was a problem hiding this comment.
I have a feeling that it crashes on the driver side in the case if batch insert inserted/updated less rows than amount of statements.
It crashes even if I replace where condition with the where = { Op.FALSE } colndition, and works well with the condition where = { Op.FALSE }. So it's not relates to existing of where condition inside statement, and it's not related to the usage of insertValue(column)
| /** Builder object for creating SQL expressions in UpsertStatement */ | ||
| object UpsertSqlExpressionBuilder : ISqlExpressionBuilder by SqlExpressionBuilder { | ||
| @OptIn(InternalApi::class) | ||
| fun <T> insertValue(column: Column<T>): ExpressionWithColumnType<T> = InsertValue(column, column.columnType) | ||
| } |
There was a problem hiding this comment.
I originally made UpsertBuilder because I thought keywords like EXCLUDED | NEW could only be used in upsert statements in a single clause.
Now that I know it's possible to use following WHERE, I tried to see if maybe they could be used elsewhere.
Because if they can, then it'd probably be better to replace both these with a single declaration directly inside SqlExpressionBuilder .
I can't find any uses of EXCLUDED outside of upsert, but I found NEW (for MySQL) in triggers with SET. Not sure if that's enough to validate, but if you find any other potential future uses of insertValue(), it might be worth considering adding directly to the base builder instead of making a new object just for this.
But if there's no potential for extension, this works fine.
There was a problem hiding this comment.
As I can see in Postgres the key word EXCLUDED is used only for insert statements. Haven't checked other databases yet.
obabichevjb
force-pushed
the
obabichev/exposed-811-batch-upsert-excluded
branch
from
de2f7f9 to
e51ebb1
Compare
Description
Update
whereparameter in upsert statement to allow useinsertValue(column)also in thewherecondition.Type of Change
Please mark the relevant options with an "X":
Updates/remove existing public API methods:
Affected databases:
Related Issues
EXPOSED-811 argument "where" in "batchUpsert" have no way to use it