From 4c6894464e1c54d9310ca27654f1da622a25517f Mon Sep 17 00:00:00 2001 From: Jens Schauder Date: Wed, 26 May 2021 08:04:33 +0200 Subject: [PATCH] Fix derived queries with boolean literals. `IsTrue` and `IsFalse` queries no longer use a literal in the query, but a bind parameter. This allows Spring Data JDBC or the JDBC driver to convert the passed boolean value to whatever is required in the database. For Oracle converter where added to support storing and loading booleans as NUMBER(1,0) where 0 is false and everything else is true. Closes #908 Original pull request #983 --- .../config/AbstractJdbcConfiguration.java | 2 +- .../jdbc/repository/query/QueryMapper.java | 14 ++- .../JdbcRepositoryIntegrationTests.java | 17 ++++ ...ractJdbcConfigurationIntegrationTests.java | 86 +++++++++++++++++-- .../query/PartTreeJdbcQueryUnitTests.java | 5 +- .../JdbcRepositoryIntegrationTests-db2.sql | 3 +- .../JdbcRepositoryIntegrationTests-h2.sql | 3 +- .../JdbcRepositoryIntegrationTests-hsql.sql | 3 +- ...JdbcRepositoryIntegrationTests-mariadb.sql | 3 +- .../JdbcRepositoryIntegrationTests-mssql.sql | 3 +- .../JdbcRepositoryIntegrationTests-mysql.sql | 9 +- .../JdbcRepositoryIntegrationTests-oracle.sql | 3 +- ...dbcRepositoryIntegrationTests-postgres.sql | 3 +- .../core/dialect/OracleDialect.java | 26 +++++- 14 files changed, 156 insertions(+), 24 deletions(-) diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/config/AbstractJdbcConfiguration.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/config/AbstractJdbcConfiguration.java index be6398b3..7aca5b75 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/config/AbstractJdbcConfiguration.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/config/AbstractJdbcConfiguration.java @@ -133,7 +133,7 @@ public class AbstractJdbcConfiguration implements ApplicationContextAware { } } - private List userConverters() { + protected List userConverters() { return Collections.emptyList(); } diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/QueryMapper.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/QueryMapper.java index f8ee2d7b..328e0882 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/QueryMapper.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/repository/query/QueryMapper.java @@ -402,11 +402,15 @@ class QueryMapper { } if (comparator == Comparator.IS_TRUE) { - return column.isEqualTo(SQL.literalOf(true)); + + Expression bind = bindBoolean(column, parameterSource, true); + return column.isEqualTo(bind); } if (comparator == Comparator.IS_FALSE) { - return column.isEqualTo(SQL.literalOf(false)); + + Expression bind = bindBoolean(column, parameterSource, false); + return column.isEqualTo(bind); } Expression columnExpression = column; @@ -495,6 +499,12 @@ class QueryMapper { } } + private Expression bindBoolean(Column column, MapSqlParameterSource parameterSource, boolean value) { + + Object converted = converter.writeValue(value, ClassTypeInformation.OBJECT); + return bind(converted, Types.BIT, parameterSource, column.getName().getReference()); + } + Field createPropertyField(@Nullable RelationalPersistentEntity entity, SqlIdentifier key) { return entity == null ? new Field(key) : new MetadataBackedField(key, entity, mappingContext, converter); } diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryIntegrationTests.java index 4b878d6c..41a4fce5 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryIntegrationTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/JdbcRepositoryIntegrationTests.java @@ -501,6 +501,19 @@ public class JdbcRepositoryIntegrationTests { repository.updateWithIntervalCalculation(23L, LocalDateTime.now()); } + @Test // #908 + void derivedQueryWithBooleanLiteralFindsCorrectValues() { + + repository.save(createDummyEntity()); + DummyEntity entity = createDummyEntity(); + entity.flag = true; + entity = repository.save(entity); + + List result = repository.findByFlagTrue(); + + assertThat(result).extracting(e -> e.idProp).containsExactly(entity.idProp); + } + private Instant createDummyBeforeAndAfterNow() { Instant now = Instant.now(); @@ -570,6 +583,8 @@ public class JdbcRepositoryIntegrationTests { @Modifying @Query("UPDATE dummy_entity SET point_in_time = :start - interval '30 minutes' WHERE id_prop = :id") void updateWithIntervalCalculation(@Param("id") Long id, @Param("start") LocalDateTime start); + + List findByFlagTrue(); } @Configuration @@ -616,10 +631,12 @@ public class JdbcRepositoryIntegrationTests { @Data @NoArgsConstructor static class DummyEntity { + String name; Instant pointInTime; OffsetDateTime offsetDateTime; @Id private Long idProp; + boolean flag; public DummyEntity(String name) { this.name = name; diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/config/AbstractJdbcConfigurationIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/config/AbstractJdbcConfigurationIntegrationTests.java index 1ec72619..e2cad273 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/config/AbstractJdbcConfigurationIntegrationTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/config/AbstractJdbcConfigurationIntegrationTests.java @@ -15,12 +15,13 @@ */ package org.springframework.data.jdbc.repository.config; +import static java.util.Arrays.*; import static org.assertj.core.api.Assertions.*; import static org.mockito.Mockito.*; -import java.util.Arrays; -import java.util.Collections; +import java.util.Collection; import java.util.List; +import java.util.Optional; import java.util.function.Consumer; import org.junit.jupiter.api.Test; @@ -29,6 +30,7 @@ import org.springframework.context.annotation.AnnotationConfigApplicationContext import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.core.convert.converter.Converter; +import org.springframework.data.convert.ReadingConverter; import org.springframework.data.convert.WritingConverter; import org.springframework.data.jdbc.core.JdbcAggregateTemplate; import org.springframework.data.jdbc.core.convert.DataAccessStrategy; @@ -36,7 +38,9 @@ import org.springframework.data.jdbc.core.convert.JdbcConverter; import org.springframework.data.jdbc.core.convert.JdbcCustomConversions; import org.springframework.data.jdbc.core.mapping.JdbcMappingContext; import org.springframework.data.relational.core.dialect.Dialect; -import org.springframework.data.relational.core.dialect.HsqlDbDialect; +import org.springframework.data.relational.core.dialect.LimitClause; +import org.springframework.data.relational.core.dialect.LockClause; +import org.springframework.data.relational.core.sql.render.SelectRenderContext; import org.springframework.jdbc.core.JdbcOperations; import org.springframework.jdbc.core.namedparam.NamedParameterJdbcOperations; import org.springframework.jdbc.core.namedparam.NamedParameterJdbcTemplate; @@ -53,7 +57,7 @@ public class AbstractJdbcConfigurationIntegrationTests { assertApplicationContext(context -> { - List> expectedBeanTypes = Arrays.asList(DataAccessStrategy.class, // + List> expectedBeanTypes = asList(DataAccessStrategy.class, // JdbcMappingContext.class, // JdbcConverter.class, // JdbcCustomConversions.class, // @@ -70,11 +74,26 @@ public class AbstractJdbcConfigurationIntegrationTests { void registersSimpleTypesFromCustomConversions() { assertApplicationContext(context -> { + JdbcMappingContext mappingContext = context.getBean(JdbcMappingContext.class); assertThat( // mappingContext.getPersistentEntity(AbstractJdbcConfigurationUnderTest.Blah.class) // ).describedAs("Blah should not be an entity, since there is a WritingConversion configured for it") // .isNull(); + + }, AbstractJdbcConfigurationUnderTest.class, Infrastructure.class); + } + + @Test // #908 + void userProvidedConversionsOverwriteDialectSpecificConversions() { + + assertApplicationContext(applicationContext -> { + + Optional> customWriteTarget = applicationContext.getBean(JdbcCustomConversions.class) + .getCustomWriteTarget(Boolean.class); + + assertThat(customWriteTarget).contains(String.class); + }, AbstractJdbcConfigurationUnderTest.class, Infrastructure.class); } @@ -106,12 +125,12 @@ public class AbstractJdbcConfigurationIntegrationTests { @Override @Bean public Dialect jdbcDialect(NamedParameterJdbcOperations operations) { - return HsqlDbDialect.INSTANCE; + return new DummyDialect(); } @Override - public JdbcCustomConversions jdbcCustomConversions() { - return new JdbcCustomConversions(Collections.singletonList(Blah2BlubbConverter.INSTANCE)); + protected List userConverters() { + return asList(Blah2BlubbConverter.INSTANCE, BooleanToYnConverter.INSTANCE); } @WritingConverter @@ -127,6 +146,59 @@ public class AbstractJdbcConfigurationIntegrationTests { private static class Blah {} private static class Blubb {} + + private static class DummyDialect implements Dialect { + @Override + public LimitClause limit() { + return null; + } + + @Override + public LockClause lock() { + return null; + } + + @Override + public SelectRenderContext getSelectContext() { + return null; + } + + @Override + public Collection getConverters() { + return asList(BooleanToNumberConverter.INSTANCE, NumberToBooleanConverter.INSTANCE); + } + } + + @WritingConverter + enum BooleanToNumberConverter implements Converter { + INSTANCE; + + @Override + public Number convert(Boolean source) { + return source ? 1 : 0; + } + } + + @ReadingConverter + enum NumberToBooleanConverter implements Converter { + INSTANCE; + + @Override + public Boolean convert(Number source) { + return source.intValue() == 0; + } + } + + @WritingConverter + enum BooleanToYnConverter implements Converter { + INSTANCE; + + @Override + public String convert(Boolean source) { + return source ? "Y" : "N"; + } + } + } } diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/query/PartTreeJdbcQueryUnitTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/query/PartTreeJdbcQueryUnitTests.java index cd24ec39..3416ae0b 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/query/PartTreeJdbcQueryUnitTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/query/PartTreeJdbcQueryUnitTests.java @@ -27,6 +27,7 @@ import java.util.Date; import java.util.List; import java.util.Properties; +import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.junit.jupiter.MockitoExtension; @@ -457,7 +458,7 @@ public class PartTreeJdbcQueryUnitTests { RelationalParametersParameterAccessor accessor = getAccessor(queryMethod, new Object[0]); ParametrizedQuery query = jdbcQuery.createQuery(accessor, returnedType); - assertThat(query.getQuery()).isEqualTo(BASE_SELECT + " WHERE " + TABLE + ".\"ACTIVE\" = TRUE"); + assertThat(query.getQuery()).isEqualTo(BASE_SELECT + " WHERE " + TABLE + ".\"ACTIVE\" = :active"); } @Test // DATAJDBC-318 @@ -468,7 +469,7 @@ public class PartTreeJdbcQueryUnitTests { RelationalParametersParameterAccessor accessor = getAccessor(queryMethod, new Object[0]); ParametrizedQuery query = jdbcQuery.createQuery(accessor, returnedType); - assertThat(query.getQuery()).isEqualTo(BASE_SELECT + " WHERE " + TABLE + ".\"ACTIVE\" = FALSE"); + assertThat(query.getQuery()).isEqualTo(BASE_SELECT + " WHERE " + TABLE + ".\"ACTIVE\" = :active"); } @Test // DATAJDBC-318 diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-db2.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-db2.sql index daa41534..d41d8acc 100644 --- a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-db2.sql +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-db2.sql @@ -5,5 +5,6 @@ CREATE TABLE dummy_entity id_Prop BIGINT GENERATED BY DEFAULT AS IDENTITY ( START WITH 1 ) PRIMARY KEY, NAME VARCHAR(100), POINT_IN_TIME TIMESTAMP, - OFFSET_DATE_TIME TIMESTAMP -- with time zone is only supported with z/OS + OFFSET_DATE_TIME TIMESTAMP, -- with time zone is only supported with z/OS + FLAG BOOLEAN ); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-h2.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-h2.sql index b9b31016..5a3f1654 100644 --- a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-h2.sql +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-h2.sql @@ -3,5 +3,6 @@ CREATE TABLE dummy_entity id_Prop BIGINT GENERATED BY DEFAULT AS IDENTITY ( START WITH 1 ) PRIMARY KEY, NAME VARCHAR(100), POINT_IN_TIME TIMESTAMP, - OFFSET_DATE_TIME TIMESTAMP WITH TIME ZONE + OFFSET_DATE_TIME TIMESTAMP WITH TIME ZONE, + FLAG BOOLEAN ); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-hsql.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-hsql.sql index b9b31016..5a3f1654 100644 --- a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-hsql.sql +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-hsql.sql @@ -3,5 +3,6 @@ CREATE TABLE dummy_entity id_Prop BIGINT GENERATED BY DEFAULT AS IDENTITY ( START WITH 1 ) PRIMARY KEY, NAME VARCHAR(100), POINT_IN_TIME TIMESTAMP, - OFFSET_DATE_TIME TIMESTAMP WITH TIME ZONE + OFFSET_DATE_TIME TIMESTAMP WITH TIME ZONE, + FLAG BOOLEAN ); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mariadb.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mariadb.sql index f9b08644..663446bd 100644 --- a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mariadb.sql +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mariadb.sql @@ -3,5 +3,6 @@ CREATE TABLE dummy_entity id_Prop BIGINT AUTO_INCREMENT PRIMARY KEY, NAME VARCHAR(100), POINT_IN_TIME TIMESTAMP(3), - OFFSET_DATE_TIME TIMESTAMP(3) + OFFSET_DATE_TIME TIMESTAMP(3), + FLAG BOOLEAN ); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mssql.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mssql.sql index c71942b4..f18b9da5 100644 --- a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mssql.sql +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mssql.sql @@ -4,5 +4,6 @@ CREATE TABLE dummy_entity id_Prop BIGINT IDENTITY PRIMARY KEY, NAME VARCHAR(100), POINT_IN_TIME DATETIME, - OFFSET_DATE_TIME DATETIMEOFFSET + OFFSET_DATE_TIME DATETIMEOFFSET, + FLAG BIT ); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mysql.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mysql.sql index 9c4085b2..60e23ca6 100644 --- a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mysql.sql +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-mysql.sql @@ -1,9 +1,10 @@ SET SQL_MODE='ALLOW_INVALID_DATES'; -CREATE TABLE dummy_entity +CREATE TABLE DUMMY_ENTITY ( - id_Prop BIGINT AUTO_INCREMENT PRIMARY KEY, + ID_PROP BIGINT AUTO_INCREMENT PRIMARY KEY, NAME VARCHAR(100), - POINT_IN_TIME TIMESTAMP(3) default null, - OFFSET_DATE_TIME TIMESTAMP(3) default null + POINT_IN_TIME TIMESTAMP(3) DEFAULT NULL, + OFFSET_DATE_TIME TIMESTAMP(3) DEFAULT NULL, + FLAG BIT(1) ); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-oracle.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-oracle.sql index a3d83134..5a92e2a2 100644 --- a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-oracle.sql +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-oracle.sql @@ -5,5 +5,6 @@ CREATE TABLE DUMMY_ENTITY ID_PROP NUMBER GENERATED BY DEFAULT ON NULL AS IDENTITY PRIMARY KEY, NAME VARCHAR2(100), POINT_IN_TIME TIMESTAMP, - OFFSET_DATE_TIME TIMESTAMP WITH TIME ZONE + OFFSET_DATE_TIME TIMESTAMP WITH TIME ZONE, + FLAG NUMBER(1,0) ); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-postgres.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-postgres.sql index 5e670bfe..05f4908e 100644 --- a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-postgres.sql +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/JdbcRepositoryIntegrationTests-postgres.sql @@ -4,5 +4,6 @@ CREATE TABLE dummy_entity id_Prop SERIAL PRIMARY KEY, NAME VARCHAR(100), POINT_IN_TIME TIMESTAMP, - OFFSET_DATE_TIME TIMESTAMP WITH TIME ZONE + OFFSET_DATE_TIME TIMESTAMP WITH TIME ZONE, + FLAG BOOLEAN ); 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 76f0c72c..90f7d466 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 @@ -15,9 +15,15 @@ */ 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 java.util.Collection; import java.util.Collections; +import static java.util.Arrays.*; + /** * An SQL dialect for Oracle. * @@ -47,7 +53,25 @@ public class OracleDialect extends AnsiDialect { @Override public Collection getConverters() { - return Collections.singletonList(TimestampAtUtcToOffsetDateTimeConverter.INSTANCE); + return asList(TimestampAtUtcToOffsetDateTimeConverter.INSTANCE, NumberToBooleanConverter.INSTANCE, BooleanToIntegerConverter.INSTANCE); } + @ReadingConverter + enum NumberToBooleanConverter implements Converter { + INSTANCE; + + @Override + public Boolean convert(Number number) { + return number.intValue() != 0; + } + } + @WritingConverter + enum BooleanToIntegerConverter implements Converter { + INSTANCE; + + @Override + public Integer convert(Boolean bool) { + return bool ? 1 : 0; + } + } }