From 2be10dd5fdebb81eaeb43c0e8d56addabc61442d 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 | 3 ++ .../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, 37 insertions(+), 47 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 0c592a90..04853af0 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 @@ -67,7 +67,7 @@ class IdGeneratingBatchInsertStrategy implements BatchInsertStrategy { IdGeneration idGeneration = dialect.getIdGeneration(); if (idGeneration.driverRequiresKeyColumnNames()) { - String[] keyColumnNames = getKeyColumnNames(); + String[] keyColumnNames = getKeyColumnNames(idGeneration); if (keyColumnNames.length == 0) { jdbcOperations.batchUpdate(sql, sqlParameterSources, holder); } else { @@ -94,10 +94,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/AbstractJdbcAggregateTemplateIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/AbstractJdbcAggregateTemplateIntegrationTests.java index b823314b..afbc4c73 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 @@ -286,7 +286,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithReferencedEntityById() { template.save(legoSet); @@ -304,7 +303,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadManyEntitiesWithReferencedEntity() { template.save(legoSet); @@ -317,7 +315,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-101 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadManyEntitiesWithReferencedEntitySorted() { template.save(createLegoSet("Lava")); @@ -332,7 +329,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-101 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadManyEntitiesWithReferencedEntitySortedAndPaged() { template.save(createLegoSet("Lava")); @@ -347,7 +343,7 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // GH-821 - @EnabledOnFeature({ SUPPORTS_QUOTED_IDS, SUPPORTS_NULL_PRECEDENCE }) + @EnabledOnFeature(SUPPORTS_NULL_PRECEDENCE) void saveAndLoadManyEntitiesWithReferencedEntitySortedWithNullPrecedence() { template.save(createLegoSet(null)); @@ -363,7 +359,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // - @EnabledOnFeature({ SUPPORTS_QUOTED_IDS }) void findByNonPropertySortFails() { assertThatThrownBy(() -> template.findAll(LegoSet.class, Sort.by("somethingNotExistant"))) @@ -371,7 +366,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadManyEntitiesByIdWithReferencedEntity() { template.save(legoSet); @@ -383,7 +377,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithReferencedNullEntity() { legoSet.manual = null; @@ -396,7 +389,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndDeleteAnEntityWithReferencedEntity() { template.save(legoSet); @@ -408,7 +400,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndDeleteAllWithReferencedEntity() { template.save(legoSet); @@ -420,7 +411,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // GH-537 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndDeleteAllByAggregateRootsWithReferencedEntity() { LegoSet legoSet1 = template.save(legoSet); @@ -434,7 +424,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // GH-537 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndDeleteAllByIdsWithReferencedEntity() { LegoSet legoSet1 = template.save(legoSet); @@ -494,7 +483,7 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature({ SUPPORTS_QUOTED_IDS, SUPPORTS_GENERATED_IDS_IN_REFERENCED_ENTITIES }) + @EnabledOnFeature(SUPPORTS_GENERATED_IDS_IN_REFERENCED_ENTITIES) void updateReferencedEntityFromNull() { legoSet.manual = (null); @@ -513,7 +502,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void updateReferencedEntityToNull() { template.save(legoSet); @@ -541,7 +529,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-112 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void replaceReferencedEntity() { template.save(legoSet); @@ -559,7 +546,7 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @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); @@ -574,7 +561,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-266 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void oneToOneChildWithoutId() { OneToOneParent parent = new OneToOneParent(); @@ -591,7 +577,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-266 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void oneToOneNullChildWithoutId() { OneToOneParent parent = new OneToOneParent(); @@ -607,7 +592,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-266 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void oneToOneNullAttributes() { OneToOneParent parent = new OneToOneParent(); @@ -623,7 +607,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-125 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithSecondaryReferenceNull() { template.save(legoSet); @@ -636,7 +619,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-125 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithSecondaryReferenceNotNull() { legoSet.alternativeInstructions = new Manual(); @@ -655,7 +637,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-276 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithListOfElementsWithoutId() { ListParent entity = new ListParent(); @@ -674,7 +655,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // GH-498 DATAJDBC-273 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadAnEntityWithListOfElementsInConstructor() { ElementNoId element = new ElementNoId(); @@ -815,7 +795,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-340 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadLongChain() { Chain4 chain4 = new Chain4(); @@ -844,7 +823,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @Test // DATAJDBC-359 - @EnabledOnFeature(SUPPORTS_QUOTED_IDS) void saveAndLoadLongChainWithoutIds() { NoIdChain4 chain4 = new NoIdChain4(); @@ -1084,7 +1062,6 @@ abstract class AbstractJdbcAggregateTemplateIntegrationTests { } @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 38996c0a..76cde45e 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 @@ -46,7 +46,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..8d181cd2 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 @@ -24,6 +24,8 @@ import java.util.stream.Collectors; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.data.annotation.Id; import org.springframework.data.jdbc.repository.query.Query; +import org.springframework.data.jdbc.testing.DatabaseType; +import org.springframework.data.jdbc.testing.EnabledOnDatabase; import org.springframework.data.relational.core.mapping.RelationalMappingContext; import org.springframework.data.repository.CrudRepository; import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; @@ -34,6 +36,7 @@ import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; * @author Diego Krupitza * @since 2.4 */ +@EnabledOnDatabase(DatabaseType.HSQL) abstract class AbstractJdbcRepositoryLookUpStrategyTests { @Autowired protected OnesRepository onesRepository; 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 28ccde4a..6f1700df 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 @@ -51,7 +51,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) ); @@ -67,7 +67,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 86af1213..66b42ecc 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 @@ -17,6 +17,7 @@ package org.springframework.data.relational.core.dialect; import org.springframework.core.convert.converter.Converter; import org.springframework.data.convert.WritingConverter; +import org.springframework.data.relational.core.sql.SqlIdentifier; import java.util.Collection; @@ -40,6 +41,11 @@ public class OracleDialect extends AnsiDialect { public boolean driverRequiresKeyColumnNames() { return true; } + + @Override + public String getKeyColumnName(SqlIdentifier id) { + return id.toSql(INSTANCE.getIdentifierProcessing()); + } }; protected OracleDialect() {}