From fb8ef0f6e05f8f5ed9eb39e536a827b3cb73fad6 Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Wed, 15 Nov 2023 10:05:46 +0100 Subject: [PATCH] Support for Id generation in Oracle using quoted identifiers. The latest Oracle JDBC driver properly supports returning of generated ids, both in batches and for quoted identifiers. This allows us to now support this feature. Closes #1666 Original pull request: #1667 --- .../IdGeneratingBatchInsertStrategy.java | 6 ++-- .../convert/IdGeneratingInsertStrategy.java | 6 ++-- ...JdbcAggregateTemplateIntegrationTests.java | 29 ++----------------- ...gregateTemplateSchemaIntegrationTests.java | 1 - ...ractJdbcRepositoryLookUpStrategyTests.java | 2 +- .../jdbc/testing/TestDatabaseFeatures.java | 12 +------- ...gregateTemplateIntegrationTests-oracle.sql | 4 +-- ...eTemplateSchemaIntegrationTests-oracle.sql | 3 +- .../relational/core/dialect/IdGeneration.java | 14 +++++++++ .../core/dialect/OracleDialect.java | 6 ++++ 10 files changed, 35 insertions(+), 48 deletions(-) diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/IdGeneratingBatchInsertStrategy.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/IdGeneratingBatchInsertStrategy.java index b6903cca..7422b195 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/IdGeneratingBatchInsertStrategy.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/IdGeneratingBatchInsertStrategy.java @@ -64,7 +64,7 @@ class IdGeneratingBatchInsertStrategy implements BatchInsertStrategy { IdGeneration idGeneration = dialect.getIdGeneration(); if (idGeneration.driverRequiresKeyColumnNames()) { - String[] keyColumnNames = getKeyColumnNames(); + String[] keyColumnNames = getKeyColumnNames(idGeneration); if (keyColumnNames.length == 0) { batchJdbcOperations.batchUpdate(sql, sqlParameterSources, holder); } else { @@ -91,10 +91,10 @@ class IdGeneratingBatchInsertStrategy implements BatchInsertStrategy { return ids; } - private String[] getKeyColumnNames() { + private String[] getKeyColumnNames(IdGeneration idGeneration) { return Optional.ofNullable(idColumn) - .map(idColumn -> new String[] { idColumn.getReference() }) + .map(idColumn -> new String[] {idGeneration.getKeyColumnName( idColumn) }) .orElse(new String[0]); } } diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/IdGeneratingInsertStrategy.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/IdGeneratingInsertStrategy.java index cf936478..82baf8f4 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/IdGeneratingInsertStrategy.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/IdGeneratingInsertStrategy.java @@ -58,7 +58,7 @@ class IdGeneratingInsertStrategy implements InsertStrategy { if (idGeneration.driverRequiresKeyColumnNames()) { - String[] keyColumnNames = getKeyColumnNames(); + String[] keyColumnNames = getKeyColumnNames(idGeneration); if (keyColumnNames.length == 0) { jdbcOperations.update(sql, sqlParameterSource, holder); } else { @@ -84,8 +84,8 @@ class IdGeneratingInsertStrategy implements InsertStrategy { } } - private String[] getKeyColumnNames() { - return Optional.ofNullable(idColumn).map(idColumn -> new String[] { idColumn.getReference() }) + private String[] getKeyColumnNames(IdGeneration idGeneration) { + return Optional.ofNullable(idColumn).map(idColumn -> new String[] { idGeneration.getKeyColumnName(idColumn) }) .orElse(new String[0]); } } diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java index 7b7959a9..0ccea568 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateIntegrationTests.java @@ -198,7 +198,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithReferencedEntityById() { template.save(legoSet); @@ -219,7 +218,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadManyEntitiesWithReferencedEntity() { template.save(legoSet); @@ -232,7 +230,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-101 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadManyEntitiesWithReferencedEntitySorted() { template.save(createLegoSet("Lava")); @@ -247,7 +244,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-101 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadManyEntitiesWithReferencedEntitySortedAndPaged() { template.save(createLegoSet("Lava")); @@ -262,7 +258,7 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // GH-821 - @EnabledOnFeature({ SUPPORTS_QUOTED_IDS, SUPPORTS_NULL_PRECEDENCE }) + @EnabledOnFeature(SUPPORTS_NULL_PRECEDENCE) void saveAndLoadManyEntitiesWithReferencedEntitySortedWithNullPrecedence() { template.save(createLegoSet(null)); @@ -279,7 +275,6 @@ class JdbcAggregateTemplateIntegrationTests { @Test // - @EnabledOnFeature({ SUPPORTS_QUOTED_IDS}) void findByNonPropertySortFails() { assertThatThrownBy(() -> template.findAll(LegoSet.class, @@ -289,7 +284,6 @@ class JdbcAggregateTemplateIntegrationTests { @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadManyEntitiesByIdWithReferencedEntity() { template.save(legoSet); @@ -301,7 +295,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithReferencedNullEntity() { legoSet.setManual(null); @@ -314,7 +307,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndDeleteAnEntityWithReferencedEntity() { template.save(legoSet); @@ -329,7 +321,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndDeleteAllWithReferencedEntity() { template.save(legoSet); @@ -345,7 +336,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // GH-537 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndDeleteAllByAggregateRootsWithReferencedEntity() { LegoSet legoSet1 = template.save(legoSet); @@ -362,7 +352,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // GH-537 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndDeleteAllByIdsWithReferencedEntity() { LegoSet legoSet1 = template.save(legoSet); @@ -423,7 +412,7 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature({ SUPPORTS_QUOTED_IDS, SUPPORTS_GENERATED_IDS_IN_REFERENCED_ENTITIES }) + @EnabledOnFeature(SUPPORTS_GENERATED_IDS_IN_REFERENCED_ENTITIES) void updateReferencedEntityFromNull() { legoSet.setManual(null); @@ -442,7 +431,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void updateReferencedEntityToNull() { template.save(legoSet); @@ -473,7 +461,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void replaceReferencedEntity() { template.save(legoSet); @@ -494,7 +481,7 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature({ SUPPORTS_QUOTED_IDS, TestDatabaseFeatures.Feature.SUPPORTS_GENERATED_IDS_IN_REFERENCED_ENTITIES }) + @EnabledOnFeature(TestDatabaseFeatures.Feature.SUPPORTS_GENERATED_IDS_IN_REFERENCED_ENTITIES) void changeReferencedEntity() { template.save(legoSet); @@ -509,7 +496,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-266 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void oneToOneChildWithoutId() { OneToOneParent parent = new OneToOneParent(); @@ -526,7 +512,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-266 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void oneToOneNullChildWithoutId() { OneToOneParent parent = new OneToOneParent(); @@ -542,7 +527,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-266 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void oneToOneNullAttributes() { OneToOneParent parent = new OneToOneParent(); @@ -558,7 +542,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-125 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithSecondaryReferenceNull() { template.save(legoSet); @@ -571,7 +554,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-125 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithSecondaryReferenceNotNull() { legoSet.alternativeInstructions = new Manual(); @@ -593,7 +575,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-276 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithListOfElementsWithoutId() { ListParent entity = new ListParent(); @@ -612,7 +593,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // GH-498 DATAJDBC-273 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithListOfElementsInConstructor() { ElementNoId element = new ElementNoId(); @@ -752,7 +732,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-340 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadLongChain() { Chain4 chain4 = new Chain4(); @@ -781,7 +760,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-359 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadLongChainWithoutIds() { NoIdChain4 chain4 = new NoIdChain4(); @@ -1012,7 +990,6 @@ class JdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-462 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void resavingAnUnversionedEntity() { LegoSet legoSet = new LegoSet(); diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateSchemaIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateSchemaIntegrationTests.java index 794ff95c..0373eac3 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateSchemaIntegrationTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/JdbcAggregateTemplateSchemaIntegrationTests.java @@ -55,7 +55,6 @@ public class JdbcAggregateTemplateSchemaIntegrationTests { @Autowired NamedParameterJdbcOperations jdbcTemplate; @Test - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) public void insertFindUpdateDelete() { DummyEntity entity = new DummyEntity(); diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/AbstractJdbcRepositoryLookUpStrategyTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/AbstractJdbcRepositoryLookUpStrategyTests.java index a114b32b..a24a9870 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/AbstractJdbcRepositoryLookUpStrategyTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/AbstractJdbcRepositoryLookUpStrategyTests.java @@ -30,7 +30,7 @@ import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; /** * Base class to test @EnableJdbcRepositories(queryLookupStrategy = ...) - * + * * @author Diego Krupitza * @since 2.4 */ diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/testing/TestDatabaseFeatures.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/testing/TestDatabaseFeatures.java index c2d3e087..6afab79a 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/testing/TestDatabaseFeatures.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/testing/TestDatabaseFeatures.java @@ -40,7 +40,7 @@ public class TestDatabaseFeatures { String productName = jdbcTemplate.execute( (ConnectionCallback) c -> c.getMetaData().getDatabaseProductName().toLowerCase(Locale.ENGLISH)); - database = Arrays.stream(Database.values()).filter(db -> db.matches(productName)).findFirst().get(); + database = Arrays.stream(Database.values()).filter(db -> db.matches(productName)).findFirst().orElseThrow(); } /** @@ -50,15 +50,6 @@ public class TestDatabaseFeatures { assumeThat(database).isNotIn(Database.Oracle, Database.SqlServer); } - /** - * Oracles JDBC driver seems to have a bug that makes it impossible to acquire generated keys when the column is - * quoted. See - * https://stackoverflow.com/questions/62263576/how-to-get-the-generated-key-for-a-column-with-lowercase-characters-from-oracle - */ - private void supportsQuotedIds() { - assumeThat(database).isNotEqualTo(Database.Oracle); - } - /** * Microsoft SqlServer does not allow explicitly setting ids in columns where the value gets generated by the * database. Such columns therefore must not be used in referenced entities, since we do a delete and insert, which @@ -115,7 +106,6 @@ public class TestDatabaseFeatures { public enum Feature { SUPPORTS_MULTIDIMENSIONAL_ARRAYS(TestDatabaseFeatures::supportsMultiDimensionalArrays), // - SUPPORTS_QUOTED_IDS(TestDatabaseFeatures::supportsQuotedIds), // SUPPORTS_HUGE_NUMBERS(TestDatabaseFeatures::supportsHugeNumbers), // SUPPORTS_ARRAYS(TestDatabaseFeatures::supportsArrays), // SUPPORTS_GENERATED_IDS_IN_REFERENCED_ENTITIES(TestDatabaseFeatures::supportsGeneratedIdsInReferencedEntities), // diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.core/JdbcAggregateTemplateIntegrationTests-oracle.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.core/JdbcAggregateTemplateIntegrationTests-oracle.sql index 155a98b1..8155dc61 100644 --- a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.core/JdbcAggregateTemplateIntegrationTests-oracle.sql +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.core/JdbcAggregateTemplateIntegrationTests-oracle.sql @@ -43,7 +43,7 @@ CREATE TABLE MANUAL ( "id2" NUMBER GENERATED by default on null as IDENTITY PRIMARY KEY, LEGO_SET NUMBER, - ALTERNATIVE NUMBER, + "alternative" NUMBER, CONTENT VARCHAR(2000) ); @@ -59,7 +59,7 @@ CREATE TABLE ONE_TO_ONE_PARENT CREATE TABLE Child_No_Id ( ONE_TO_ONE_PARENT INTEGER PRIMARY KEY, - "content" VARCHAR(30) + CONTENT VARCHAR(30) ); CREATE TABLE LIST_PARENT diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.core/JdbcAggregateTemplateSchemaIntegrationTests-oracle.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.core/JdbcAggregateTemplateSchemaIntegrationTests-oracle.sql index b93ef418..9e93e500 100644 --- a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.core/JdbcAggregateTemplateSchemaIntegrationTests-oracle.sql +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.core/JdbcAggregateTemplateSchemaIntegrationTests-oracle.sql @@ -1,8 +1,9 @@ DROP USER OTHER CASCADE; - CREATE USER OTHER; +ALTER USER OTHER QUOTA UNLIMITED ON USERS; + CREATE TABLE OTHER.DUMMY_ENTITY ( ID NUMBER GENERATED by default on null as IDENTITY PRIMARY KEY, diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/dialect/IdGeneration.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/dialect/IdGeneration.java index 3b61ca25..30cf7132 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/dialect/IdGeneration.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/dialect/IdGeneration.java @@ -18,6 +18,8 @@ package org.springframework.data.relational.core.dialect; import java.sql.Connection; import java.sql.PreparedStatement; +import org.springframework.data.relational.core.sql.SqlIdentifier; + /** * Describes how obtaining generated ids after an insert works for a given JDBC driver. * @@ -45,6 +47,18 @@ public interface IdGeneration { return false; } + /** + * Provides for a given id {@link SqlIdentifier} the String that is to be used for registering interest in the + * generated value of that column. + * + * @param id {@link SqlIdentifier} representing a column for which a generated value is to be obtained. + * @return a String representing that column in the way expected by the JDBC driver. + * @since 3.3 + */ + default String getKeyColumnName(SqlIdentifier id) { + return id.getReference(); + } + /** * Does the driver support id generation for batch operations. *

diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/dialect/OracleDialect.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/dialect/OracleDialect.java index cb47d04f..83c5cd05 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/dialect/OracleDialect.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/dialect/OracleDialect.java @@ -18,6 +18,7 @@ package org.springframework.data.relational.core.dialect; import org.springframework.core.convert.converter.Converter; import org.springframework.data.convert.ReadingConverter; import org.springframework.data.convert.WritingConverter; +import org.springframework.data.relational.core.sql.SqlIdentifier; import java.util.Collection; import java.util.Collections; @@ -42,6 +43,11 @@ public class OracleDialect extends AnsiDialect { public boolean driverRequiresKeyColumnNames() { return true; } + + @Override + public String getKeyColumnName(SqlIdentifier id) { + return id.toSql(INSTANCE.getIdentifierProcessing()); + } }; protected OracleDialect() {}