diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/dialect/DialectResolver.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/dialect/DialectResolver.java index 0ae794bab..2dbda1b0a 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/dialect/DialectResolver.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/dialect/DialectResolver.java @@ -205,6 +205,11 @@ public class DialectResolver { this.arrayColumns = new JdbcArrayColumnsAdapter(delegate.getArraySupport()); } + @Override + public String getName() { + return delegate.getName(); + } + @Override public LimitClause limit() { return delegate.limit(); diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/AbstractJdbcAggregateTemplateIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/AbstractJdbcAggregateTemplateIntegrationTests.java index 139de48c3..ddddeb2c7 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/AbstractJdbcAggregateTemplateIntegrationTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/AbstractJdbcAggregateTemplateIntegrationTests.java @@ -189,34 +189,108 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { return entity; } + private void withSqlServerIdentityInsertOn(JdbcAggregateOperations jdbcAggregateTemplate, String tableName, + Runnable action) { + + if (jdbcAggregateTemplate.getDataAccessStrategy().getDialect() instanceof SqlServerDialect) { + jdbc.getJdbcOperations().execute("SET IDENTITY_INSERT " + tableName + " ON"); + try { + action.run(); + } finally { + jdbc.getJdbcOperations().execute("SET IDENTITY_INSERT " + tableName + " OFF"); + } + } else { + action.run(); + } + } + @Test // GH-493 void upsertInsertsWhenIdDoesNotExistAndUpdatesWhenItExists() { - assumeThat(template).isInstanceOf(JdbcAggregateTemplate.class); - JdbcAggregateTemplate jdbcTemplate = (JdbcAggregateTemplate) template; - assumeThat(jdbcTemplate.getDataAccessStrategy().getDialect().getUpsertRenderContext()).isNotNull(); + withSqlServerIdentityInsertOn(template, "with_insert_only", () -> { - if(template.getDataAccessStrategy().getDialect() instanceof SqlServerDialect) { - String tableName = "with_insert_only"; - jdbc.getJdbcOperations().execute("SET IDENTITY_INSERT " + tableName + " ON"); - } + WithInsertOnly entity = new WithInsertOnly(); + entity.id = 8888L; + entity.insertOnly = "upserted"; + template.upsert(entity); - WithInsertOnly entity = new WithInsertOnly(); - entity.id = 8888L; - entity.insertOnly = "upserted"; - template.upsert(entity); + assertThat(template.findById(8888L, WithInsertOnly.class).insertOnly).isEqualTo("upserted"); + + entity.insertOnly = "updated"; + template.upsert(entity); - assertThat(template.findById(8888L, WithInsertOnly.class).insertOnly).isEqualTo("upserted"); + assertThat(template.findById(8888L, WithInsertOnly.class).insertOnly).isEqualTo("updated"); + }); + } - entity.insertOnly = "updated"; - template.upsert(entity); + @Test // GH-493 + void upsertWhenMatchedAndUpdateAssignmentsEqualConflictKeyOnly() { - assertThat(template.findById(8888L, WithInsertOnly.class).insertOnly).isEqualTo("updated"); + long id = 8889L; + withSqlServerIdentityInsertOn(template, "with_id_only", () -> { - if(template.getDataAccessStrategy().getDialect() instanceof SqlServerDialect) { - String tableName = "with_insert_only"; - jdbc.getJdbcOperations().execute("SET IDENTITY_INSERT " + tableName + " OFF"); - } + WithIdOnly first = new WithIdOnly(); + first.id = id; + template.upsert(first); + + WithIdOnly second = new WithIdOnly(); + second.id = id; + template.upsert(second); + + assertThat(template.findById(id, WithIdOnly.class).id).isEqualTo(id); + }); + } + + @Test // GH-493 + void upsertNoOpWhenNonKeyColumnsAlreadyMatch() { + + long id = 8890L; + withSqlServerIdentityInsertOn(template, "LEGO_SET", () -> { + + LegoSet lego = new LegoSet(); + lego.id = id; + lego.name = "millennium"; + template.upsert(lego); + template.upsert(lego); + + assertThat(template.findById(id, LegoSet.class).name).isEqualTo("millennium"); + }); + } + + @Test // GH-493 + void upsertAfterDeleteInsertsAgain() { + + long id = 8891L; + withSqlServerIdentityInsertOn(template, "LEGO_SET", () -> { + + LegoSet lego = new LegoSet(); + lego.id = id; + lego.name = "first"; + template.upsert(lego); + + template.deleteById(id, LegoSet.class); + + lego.name = "second"; + template.upsert(lego); + + assertThat(template.findById(id, LegoSet.class).name).isEqualTo("second"); + }); + } + + @Test // GH-493 + void upsertExistingRowWithSameInsertOnlyValue() { + + withSqlServerIdentityInsertOn(template, "with_insert_only", () -> { + + long id = 8892L; + WithInsertOnly entity = new WithInsertOnly(); + entity.id = id; + entity.insertOnly = "unchanged"; + template.upsert(entity); + template.upsert(entity); + + assertThat(template.findById(id, WithInsertOnly.class).insertOnly).isEqualTo("unchanged"); + }); } @Test // GH-1446 diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/sql/render/UpsertRendererUnitTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/sql/render/UpsertRendererUnitTests.java index 63abaf8a3..f8e63c527 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/sql/render/UpsertRendererUnitTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/sql/render/UpsertRendererUnitTests.java @@ -18,6 +18,7 @@ package org.springframework.data.jdbc.core.sql.render; import static org.assertj.core.api.Assertions.assertThat; import org.junit.jupiter.api.Test; +import org.springframework.data.jdbc.core.dialect.JdbcOracleDialect; import org.springframework.data.relational.core.dialect.RenderContextFactory; import org.springframework.data.relational.core.sql.SQL; import org.springframework.data.relational.core.sql.StatementBuilder; @@ -124,4 +125,19 @@ class UpsertRendererUnitTests { assertThat(sql).contains("WHEN MATCHED THEN UPDATE SET"); assertThat(sql).contains("WHEN NOT MATCHED THEN INSERT"); } + + @Test // GH-493 + void oracleIdOnlyMergeOmitsWhenMatchedUpdate() { + + Table table = SQL.table("ent"); + Upsert upsert = StatementBuilder.upsert().table(table).columnValue(table.column("id").set(SQL.bindMarker("id"))) + .where(table.column("id").isEqualTo(SQL.bindMarker("id"))).build(); + + var context = new RenderContextFactory(JdbcOracleDialect.INSTANCE).createRenderContext(); + String sql = SqlRenderer.create(context).render(upsert); + + assertThat(sql).contains("MERGE INTO"); + assertThat(sql).contains("WHEN NOT MATCHED THEN INSERT"); + assertThat(sql).doesNotContain("WHEN MATCHED THEN UPDATE SET"); + } } diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/OracleUpsertRenderContext.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/OracleUpsertRenderContext.java index 316ecc587..3e218e041 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/OracleUpsertRenderContext.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/OracleUpsertRenderContext.java @@ -16,17 +16,14 @@ package org.springframework.data.relational.core.sql.render; import java.util.List; -import java.util.Set; import java.util.function.Function; -import org.springframework.data.relational.core.sql.IdentifierProcessing; import org.springframework.data.relational.core.sql.SqlIdentifier; import org.springframework.data.relational.core.sql.Table; import org.springframework.util.Assert; /** - * Oracle MERGE upsert. Uses {@code SELECT ... FROM DUAL} as the source, which Oracle requires for - * literal/bind-marker-only selects. + * Oracle MERGE upsert. Uses {@code SELECT ... FROM DUAL} for source values. * * @since 4.x */ @@ -34,53 +31,56 @@ public enum OracleUpsertRenderContext implements UpsertRenderContext { INSTANCE; - private static final String SOURCE_FROM_CLAUSE = " FROM DUAL"; - @Override - public String renderUpsert(Table table, Columns merge, Function bindMarkerFn) { + public String renderUpsert(Table table, Columns columns, Function bindMarkerFn) { - List insertColumns = merge.insertColumns(); - List conflictColumns = merge.filterColumns(); - IdentifierProcessing identifierProcessing = merge.identifierProcessing(); + Assert.notEmpty(columns.insertColumns(), "Insert columns must not be empty"); + Assert.notEmpty(columns.filterColumns(), "Filter columns must not be empty"); - Assert.notEmpty(insertColumns, "Insert columns must not be empty"); - Assert.notEmpty(conflictColumns, "Conflict columns must not be empty"); + String targetTableAlias = columns.identifierProcessing().quote(StandardSqlUpsertRenderContext.targetTableAlias); + String sourceTableAlias = columns.identifierProcessing().quote(StandardSqlUpsertRenderContext.sourceTableAlias); - Set conflictSet = Set.copyOf(conflictColumns); - String tableSql = table.getName().toSql(identifierProcessing); + String tableName = columns.tableName(table); + String insertColumnNames = String.join(", ", columns.insertColumnNames()); + String sourceSelectList = String.join(", ", + columns.insertColumns().stream().map(col -> bindMarkerFn.apply(col) + " AS " + columns.column(col)).toList()); - String sourceSelectList = String.join(", ", insertColumns.stream() - .map(col -> bindMarkerFn.apply(col) + " AS " + col.toSql(identifierProcessing)) - .toList()); + String onCondition = String.join(" AND ", columns.filterColumns().stream().map(col -> { + String colName = columns.column(col); + return "%s.%s = %s.%s".formatted(targetTableAlias, colName, sourceTableAlias, colName); + }).toList()); - String onCondition = "(" + String.join(" AND ", conflictColumns.stream() - .map(col -> "t." + col.toSql(identifierProcessing) + " = s." + col.toSql(identifierProcessing)) - .toList()) + ")"; + String insertValuesSql = String.join(", ", + columns.insertColumns().stream().map(col -> columns.column(sourceTableAlias, col)).toList()); - List updateColumns = insertColumns.stream() - .filter(col -> !conflictSet.contains(col)) - .toList(); + String insertClause = "WHEN NOT MATCHED THEN INSERT (%s) VALUES (%s)".formatted(insertColumnNames, + insertValuesSql); - String updateSetClause; + List updateColumns = columns.updateColumns(); if (updateColumns.isEmpty()) { - SqlIdentifier firstConflict = conflictColumns.get(0); - updateSetClause = "t." + firstConflict.toSql(identifierProcessing) + " = s." - + firstConflict.toSql(identifierProcessing); - } else { - updateSetClause = String.join(", ", updateColumns.stream() - .map(col -> "t." + col.toSql(identifierProcessing) + " = s." + col.toSql(identifierProcessing)) - .toList()); + // ORA-38104: columns referenced in ON cannot be updated; omit WHEN MATCHED so existing rows are left + // unchanged (same as a no-op update of key-only columns). + return "MERGE INTO %s %s USING (SELECT %s FROM DUAL) %s ON (%s) %s".formatted( // + tableName, // + targetTableAlias, // + sourceSelectList, // + sourceTableAlias, // + onCondition, // + insertClause); } - String insertColumnsSql = String.join(", ", - insertColumns.stream().map(col -> col.toSql(identifierProcessing)).toList()); - - String insertValuesSql = String.join(", ", - insertColumns.stream().map(col -> "s." + col.toSql(identifierProcessing)).toList()); - - return "MERGE INTO " + tableSql + " t USING (SELECT " + sourceSelectList + SOURCE_FROM_CLAUSE + ") s ON " - + onCondition - + " WHEN MATCHED THEN UPDATE SET " + updateSetClause - + " WHEN NOT MATCHED THEN INSERT (" + insertColumnsSql + ") VALUES (" + insertValuesSql + ")"; + String updateSetClause = String.join(", ", updateColumns.stream().map(col -> { + String colName = columns.column(col); + return "%s.%s = %s.%s".formatted(targetTableAlias, colName, sourceTableAlias, colName); + }).toList()); + + return "MERGE INTO %s %s USING (SELECT %s FROM DUAL) %s ON (%s) WHEN MATCHED THEN UPDATE SET %s %s".formatted( // + tableName, // + targetTableAlias, // + sourceSelectList, // + sourceTableAlias, // + onCondition, // + updateSetClause, // + insertClause); } } diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/PostgresUpsertRenderContext.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/PostgresUpsertRenderContext.java index 975a91644..33cd00644 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/PostgresUpsertRenderContext.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/PostgresUpsertRenderContext.java @@ -42,8 +42,16 @@ public enum PostgresUpsertRenderContext implements UpsertRenderContext { String insertColumnNames = String.join(", ", columns.insertColumnNames()); String bindMarkers = String.join(", ", columns.insertColumnBindMarkers(bindMarkerFn)); String filterColumnNames = String.join(", ", columns.filterColumnNames()); - String setValues = setValuesSnippet(columns); + if(columns.updateColumns().isEmpty()) { + return "INSERT INTO %s (%s) VALUES (%s) ON CONFLICT (%s) DO NOTHING".formatted(// + tableName, // + insertColumnNames, // + bindMarkers, // + filterColumnNames); + } + + String setValues = setValuesSnippet(columns); return "INSERT INTO %s (%s) VALUES (%s) ON CONFLICT (%s) DO UPDATE SET %s".formatted(// tableName, // insertColumnNames, // diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/StandardSqlUpsertRenderContext.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/StandardSqlUpsertRenderContext.java index b4d4f3a0e..c54d8e007 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/StandardSqlUpsertRenderContext.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/sql/render/StandardSqlUpsertRenderContext.java @@ -33,8 +33,8 @@ public enum StandardSqlUpsertRenderContext implements UpsertRenderContext { INSTANCE; - private static final String targetTableAlias = "_t"; - private static final String sourceTableAlias = "_s"; + static final String targetTableAlias = "_t"; + static final String sourceTableAlias = "_s"; @Override public String renderUpsert(Table table, Columns columns, Function bindMarkerFn) { @@ -54,26 +54,39 @@ public enum StandardSqlUpsertRenderContext implements UpsertRenderContext { return "%s.%s = %s.%s".formatted(targetTableAlias, colName, sourceTableAlias, colName); }).toList()); + String insertValuesSql = String.join(", ", + columns.insertColumns().stream().map(col -> columns.column(sourceTableAlias, col)).toList()); + + String insertClause = "WHEN NOT MATCHED THEN INSERT (%s) VALUES (%s)".formatted(insertColumnNames, + insertValuesSql); + List updateColumns = columns.updateColumns(); + if (updateColumns.isEmpty()) { + // Matched rows are left unchanged. Updating only key columns is invalid on SQL Server (identity) and Oracle + // (ORA-38104). + return "MERGE INTO %s %s USING (VALUES (%s)) AS %s (%s) ON %s %s".formatted( // + tableName, // + targetTableAlias, // + bindMarkers, // + sourceTableAlias, // + insertColumnNames, // + onCondition, // + insertClause); + } String updateSetClause = String.join(", ", updateColumns.stream().map(col -> { String colName = columns.column(col); return "%s.%s = %s.%s".formatted(targetTableAlias, colName, sourceTableAlias, colName); }).toList()); - String insertValuesSql = String.join(", ", - columns.insertColumns().stream().map(col -> columns.column(sourceTableAlias, col)).toList()); - - return "MERGE INTO %s %s USING (VALUES (%s)) AS %s (%s) ON %s WHEN MATCHED THEN UPDATE SET %s WHEN NOT MATCHED THEN INSERT (%s) VALUES (%s)" - .formatted( // - tableName, // - targetTableAlias, // - bindMarkers, // - sourceTableAlias, // - insertColumnNames, // - onCondition, // - updateSetClause, // - insertColumnNames, // - insertValuesSql); + return "MERGE INTO %s %s USING (VALUES (%s)) AS %s (%s) ON %s WHEN MATCHED THEN UPDATE SET %s %s".formatted( // + tableName, // + targetTableAlias, // + bindMarkers, // + sourceTableAlias, // + insertColumnNames, // + onCondition, // + updateSetClause, // + insertClause); } } diff --git a/spring-data-relational/src/test/java/org/springframework/data/relational/core/sql/render/UpsertRenderContextUnitTests.java b/spring-data-relational/src/test/java/org/springframework/data/relational/core/sql/render/UpsertRenderContextUnitTests.java index a0edcd6b6..b8012539b 100644 --- a/spring-data-relational/src/test/java/org/springframework/data/relational/core/sql/render/UpsertRenderContextUnitTests.java +++ b/spring-data-relational/src/test/java/org/springframework/data/relational/core/sql/render/UpsertRenderContextUnitTests.java @@ -48,6 +48,23 @@ class UpsertRenderContextUnitTests { "MERGE INTO my_table \"_t\" USING (VALUES (:id, :name)) AS \"_s\" (id, name) ON \"_t\".id = \"_s\".id WHEN MATCHED THEN UPDATE SET \"_t\".name = \"_s\".name WHEN NOT MATCHED THEN INSERT (id, name) VALUES (\"_s\".id, \"_s\".name)"); } + @Test // GH-493 + void mergeUpsertWithMultipleConflictColumnsBuildsFilterClauseWithAllColumns() { + + List insertColumns = List.of(SqlIdentifier.unquoted("tenant_id"), SqlIdentifier.unquoted("id"), + SqlIdentifier.unquoted("name")); + List conflictColumns = List.of(SqlIdentifier.unquoted("tenant_id"), SqlIdentifier.unquoted("id")); + Columns columns = new Columns(insertColumns, conflictColumns, IDENTIFIER_PROCESSING); + + String sql = StandardSqlUpsertRenderContext.INSTANCE.renderUpsert(TABLE, columns, BIND_MARKER); + + assertThat(sql).isEqualToIgnoringWhitespace( + "MERGE INTO my_table \"_t\" USING (VALUES (:tenant_id, :id, :name)) AS \"_s\" (tenant_id, id, name) " + + "ON \"_t\".tenant_id = \"_s\".tenant_id AND \"_t\".id = \"_s\".id " + + "WHEN MATCHED THEN UPDATE SET \"_t\".name = \"_s\".name " + + "WHEN NOT MATCHED THEN INSERT (tenant_id, id, name) VALUES (\"_s\".tenant_id, \"_s\".id, \"_s\".name)"); + } + @Test // GH-493 void postgresUpsertRendersInsertOnConflictDoUpdate() { @@ -58,6 +75,16 @@ class UpsertRenderContextUnitTests { "INSERT INTO my_table (id, name) VALUES (:id, :name) ON CONFLICT (id) DO UPDATE SET name = EXCLUDED.name"); } + @Test // GH-493 + void postgresUpsertRendersInsertOnConflictDoNothing() { + + String sql = PostgresUpsertRenderContext.INSTANCE.renderUpsert(TABLE, + new Columns(INSERT_COLUMNS, INSERT_COLUMNS, IDENTIFIER_PROCESSING), BIND_MARKER); + + assertThat(sql).isEqualToIgnoringWhitespace( + "INSERT INTO my_table (id, name) VALUES (:id, :name) ON CONFLICT (id, name) DO NOTHING"); + } + @Test // GH-493 void mySqlUpsertRendersOnDuplicateKeyUpdate() { @@ -85,14 +112,30 @@ class UpsertRenderContextUnitTests { String sql = OracleUpsertRenderContext.INSTANCE.renderUpsert(TABLE, new Columns(INSERT_COLUMNS, CONFLICT_COLUMNS, IDENTIFIER_PROCESSING), BIND_MARKER); - System.out.println("sql: " + sql); - assertThat(sql).startsWith("MERGE INTO"); - assertThat(sql).contains("USING (SELECT"); - assertThat(sql).contains("FROM DUAL"); - // Oracle requires ON condition in parentheses (ORA-00969 otherwise). - assertThat(sql).contains("ON (t.id = s.id)"); - assertThat(sql).contains("WHEN MATCHED THEN UPDATE SET"); - assertThat(sql).contains("WHEN NOT MATCHED THEN INSERT"); + assertThat(sql).isEqualToIgnoringWhitespace( + "MERGE INTO my_table \"_t\" USING (SELECT :id AS id, :name AS name FROM DUAL) \"_s\" ON (\"_t\".id = \"_s\".id) WHEN MATCHED THEN UPDATE SET \"_t\".name = \"_s\".name WHEN NOT MATCHED THEN INSERT (id, name) VALUES (\"_s\".id, \"_s\".name)"); + } + + @Test // GH-493 + void standardMergeIdOnlyOmitsWhenMatchedUpdate() { + + List idOnly = List.of(SqlIdentifier.unquoted("id")); + String sql = StandardSqlUpsertRenderContext.INSTANCE.renderUpsert(TABLE, new Columns(idOnly, idOnly, + IDENTIFIER_PROCESSING), BIND_MARKER); + + assertThat(sql).isEqualTo( + "MERGE INTO my_table \"_t\" USING (VALUES (:id)) AS \"_s\" (id) ON \"_t\".id = \"_s\".id WHEN NOT MATCHED THEN INSERT (id) VALUES (\"_s\".id)"); + } + + @Test // GH-493 + void oracleMergeIdOnlyOmitsWhenMatchedUpdate() { + + List idOnly = List.of(SqlIdentifier.unquoted("id")); + String sql = OracleUpsertRenderContext.INSTANCE.renderUpsert(TABLE, new Columns(idOnly, idOnly, + IDENTIFIER_PROCESSING), BIND_MARKER); + + assertThat(sql).isEqualToIgnoringWhitespace( + "MERGE INTO my_table \"_t\" USING (SELECT :id AS id FROM DUAL) \"_s\" ON (\"_t\".id = \"_s\".id) WHEN NOT MATCHED THEN INSERT (id) VALUES (\"_s\".id)"); } @Test // GH-493