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

fix: EXPOSED-602 Column name that includes table name is aliased with upserts #2287

Merged
merged 1 commit into from
Oct 29, 2024

Conversation

bog-walk
Copy link
Member

Description

Summary of the change:
Allows valid MERGE syntax to be generated again when using upsert() with update columns that include the table name in their name.

Detailed description:

  • Why: PR feat: EXPOSED-436 Allow using insert values on update with upsert() #2172 changed the way update values that copy insert values are processed internally. Since then (version 0.54.0), any update-insert value is parsed like a regular expression. For dialects that use MERGE, this means the value is automatically aliased with virtual S alias and any potential presence of the table name in the update expression is also aliased with origin T. This leads to invalid identifier syntax if the expression holds a column name that itself includes the table name.

  • What: String replacement now only happens if the table name is located in a prefix identifier position, before '.'


Type of Change

Please mark the relevant options with an "X":

  • Bug fix

Affected databases:

  • Oracle
  • SqlServer
  • H2

Checklist

  • Unit tests are in place
  • The build is green (including the Detekt check)

Related Issues

EXPOSED-602

… upserts

Dialects that use MERGE for upserts automatically have their update values aliased.
Since version 0.54.0, update values that should be the same as insert values are
wrapped as a special expression. This means they are processed like any expression
and any potential inclusion of the table name is replaced with alias 'T'. This
causes invalid identifiers if the actual column to update uses an identifier
that includes the table name. Restricting the case for string replacement
fixes this.
@@ -749,6 +749,24 @@ class UpsertTests : DatabaseTestsBase() {
}
}

@Test
fun testUpsertWhenColumnNameIncludesTableName() {
val tester = object : Table("my_table") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: looks like, if the table created with IntIdTable the error on the master is the same. It confuses a bit, at the first time I thought that the problem is in the custom id column, but looks like it's not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, it fails if IntIdTable is used instead, also with an invalid identifier exception, but not quite the same.

It fails because S.ID, despite being correct, is not declared anywhere in:
USING (VALUES (1, 'Hello')) S(MY_TABLE_ID, MY_TABLE_VALUE).
The type of single-table MERGE used for upserts relies on a derived source table above, so key columns used in the ON clause need to be defined first in this table, which doesn't happen if auto-incrementing (because no value is set).
So the test passes if a manual value is also assigned to the id column in upsert():
USING (VALUES (1, 1, 'Hello')) S(ID, MY_TABLE_ID, MY_TABLE_VALUE).

It's a limitation I noticed when upsert via merge was first implemented and I kept wondering if a user would ever come forward with it as an issue, but so far nothing.

@bog-walk bog-walk merged commit c5c5c7b into main Oct 29, 2024
5 checks passed
@bog-walk bog-walk deleted the bog-walk/fix-merge-alias branch October 29, 2024 16:52
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 this pull request may close these issues.

2 participants