From 10ae1e2666bb945552f823a7f71ed536e6bcd4ad Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Wed, 15 Mar 2023 16:36:18 +0100 Subject: [PATCH] Apply custom converter for Collection-like values in queries. We now apply converters only for Collection-like values and no longer to Iterable types. Closes #1452 --- .../data/jdbc/core/convert/QueryMapper.java | 58 ++++++++++--------- .../data/r2dbc/query/QueryMapper.java | 52 ++++++++--------- .../r2dbc/query/QueryMapperUnitTests.java | 49 +++++++++++++++- .../conversion/BasicRelationalConverter.java | 28 +++++++++ 4 files changed, 130 insertions(+), 57 deletions(-) diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/QueryMapper.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/QueryMapper.java index 4eade2f0..5abdf229 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/QueryMapper.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/QueryMapper.java @@ -41,7 +41,6 @@ import org.springframework.data.relational.core.query.CriteriaDefinition; import org.springframework.data.relational.core.query.CriteriaDefinition.Comparator; import org.springframework.data.relational.core.query.ValueFunction; import org.springframework.data.relational.core.sql.*; -import org.springframework.data.util.TypeInformation; import org.springframework.data.util.Pair; import org.springframework.data.util.TypeInformation; import org.springframework.jdbc.core.namedparam.MapSqlParameterSource; @@ -276,17 +275,18 @@ public class QueryMapper { Column column = table.column(propertyField.getMappedColumnName()); Object mappedValue; SQLType sqlType; + Comparator comparator = criteria.getComparator(); - if (criteria.getValue() instanceof JdbcValue settableValue) { + if (criteria.getValue()instanceof JdbcValue settableValue) { - mappedValue = convertValue(settableValue.getValue(), propertyField.getTypeHint()); + mappedValue = convertValue(comparator, settableValue.getValue(), propertyField.getTypeHint()); sqlType = getTypeHint(mappedValue, actualType.getType(), settableValue); } else if (criteria.getValue() instanceof ValueFunction) { ValueFunction valueFunction = (ValueFunction) criteria.getValue(); - Object value = valueFunction.apply(getEscaper(criteria.getComparator())); + Object value = valueFunction.apply(getEscaper(comparator)); - mappedValue = convertValue(value, propertyField.getTypeHint()); + mappedValue = convertValue(comparator, value, propertyField.getTypeHint()); sqlType = propertyField.getSqlType(); } else if (propertyField instanceof MetadataBackedField // @@ -296,17 +296,15 @@ public class QueryMapper { RelationalPersistentProperty property = ((MetadataBackedField) propertyField).property; JdbcValue jdbcValue = convertToJdbcValue(property, criteria.getValue()); mappedValue = jdbcValue.getValue(); - sqlType = jdbcValue.getJdbcType() != null ? jdbcValue.getJdbcType() - : propertyField.getSqlType(); + sqlType = jdbcValue.getJdbcType() != null ? jdbcValue.getJdbcType() : propertyField.getSqlType(); } else { - mappedValue = convertValue(criteria.getValue(), propertyField.getTypeHint()); + mappedValue = convertValue(comparator, criteria.getValue(), propertyField.getTypeHint()); sqlType = propertyField.getSqlType(); } - return createCondition(column, mappedValue, sqlType, parameterSource, criteria.getComparator(), - criteria.isIgnoreCase()); + return createCondition(column, mappedValue, sqlType, parameterSource, comparator, criteria.isIgnoreCase()); } /** @@ -428,6 +426,23 @@ public class QueryMapper { return Escaper.DEFAULT; } + @Nullable + private Object convertValue(Comparator comparator, @Nullable Object value, TypeInformation typeHint) { + + if (Comparator.IN.equals(comparator) && value instanceof Collection collection && !collection.isEmpty()) { + + Collection mapped = new ArrayList<>(collection.size()); + + for (Object o : collection) { + mapped.add(convertValue(o, typeHint)); + } + + return mapped; + } + + return convertValue(value, typeHint); + } + @Nullable protected Object convertValue(@Nullable Object value, TypeInformation typeInformation) { @@ -450,19 +465,6 @@ public class QueryMapper { return Pair.of(first, second); } - if (value instanceof Iterable) { - - List mapped = new ArrayList<>(); - - for (Object o : (Iterable) value) { - - mapped.add(convertValue(o, typeInformation.getActualType() != null ? typeInformation.getRequiredActualType() - : TypeInformation.OBJECT)); - } - - return mapped; - } - if (value.getClass().isArray() && (TypeInformation.OBJECT.equals(typeInformation) || typeInformation.isCollectionLike())) { return value; @@ -476,7 +478,7 @@ public class QueryMapper { } private Condition createCondition(Column column, @Nullable Object mappedValue, SQLType sqlType, - MapSqlParameterSource parameterSource, Comparator comparator, boolean ignoreCase) { + MapSqlParameterSource parameterSource, Comparator comparator, boolean ignoreCase) { if (comparator.equals(Comparator.IS_NULL)) { return column.isNull(); @@ -614,12 +616,12 @@ public class QueryMapper { } private Expression bind(@Nullable Object mappedValue, SQLType sqlType, MapSqlParameterSource parameterSource, - String name) { + String name) { return bind(mappedValue, sqlType, parameterSource, name, false); } - private Expression bind(@Nullable Object mappedValue, SQLType sqlType, MapSqlParameterSource parameterSource, String name, - boolean ignoreCase) { + private Expression bind(@Nullable Object mappedValue, SQLType sqlType, MapSqlParameterSource parameterSource, + String name, boolean ignoreCase) { String uniqueName = getUniqueName(parameterSource, name); @@ -671,7 +673,7 @@ public class QueryMapper { /** * Returns the key to be used in the mapped document eventually. * - * @return the key to be used in the mapped document eventually. + * @return the key to be used in the mapped document eventually. */ public SqlIdentifier getMappedColumnName() { return this.name; diff --git a/spring-data-r2dbc/src/main/java/org/springframework/data/r2dbc/query/QueryMapper.java b/spring-data-r2dbc/src/main/java/org/springframework/data/r2dbc/query/QueryMapper.java index 12eb979c..d25c3a36 100644 --- a/spring-data-r2dbc/src/main/java/org/springframework/data/r2dbc/query/QueryMapper.java +++ b/spring-data-r2dbc/src/main/java/org/springframework/data/r2dbc/query/QueryMapper.java @@ -38,7 +38,6 @@ import org.springframework.data.relational.core.query.CriteriaDefinition; import org.springframework.data.relational.core.query.CriteriaDefinition.Comparator; import org.springframework.data.relational.core.query.ValueFunction; import org.springframework.data.relational.core.sql.*; -import org.springframework.data.util.TypeInformation; import org.springframework.data.util.Pair; import org.springframework.data.util.TypeInformation; import org.springframework.lang.Nullable; @@ -348,26 +347,27 @@ public class QueryMapper { Object mappedValue; Class typeHint; + Comparator comparator = criteria.getComparator(); if (criteria.getValue() instanceof Parameter) { Parameter parameter = (Parameter) criteria.getValue(); - mappedValue = convertValue(parameter.getValue(), propertyField.getTypeHint()); + mappedValue = convertValue(comparator, parameter.getValue(), propertyField.getTypeHint()); typeHint = getTypeHint(mappedValue, actualType.getType(), parameter); } else if (criteria.getValue() instanceof ValueFunction) { ValueFunction valueFunction = (ValueFunction) criteria.getValue(); - Object value = valueFunction.apply(getEscaper(criteria.getComparator())); + Object value = valueFunction.apply(getEscaper(comparator)); - mappedValue = convertValue(value, propertyField.getTypeHint()); + mappedValue = convertValue(comparator, value, propertyField.getTypeHint()); typeHint = actualType.getType(); } else { - mappedValue = convertValue(criteria.getValue(), propertyField.getTypeHint()); + mappedValue = convertValue(comparator, criteria.getValue(), propertyField.getTypeHint()); typeHint = actualType.getType(); } - return createCondition(column, mappedValue, typeHint, bindings, criteria.getComparator(), criteria.isIgnoreCase()); + return createCondition(column, mappedValue, typeHint, bindings, comparator, criteria.isIgnoreCase()); } private Escaper getEscaper(Comparator comparator) { @@ -395,6 +395,23 @@ public class QueryMapper { return Parameter.from(convertValue(value.getValue(), TypeInformation.OBJECT)); } + @Nullable + private Object convertValue(Comparator comparator, @Nullable Object value, TypeInformation typeHint) { + + if (Comparator.IN.equals(comparator) && value instanceof Collection collection && !collection.isEmpty()) { + + Collection mapped = new ArrayList<>(collection.size()); + + for (Object o : collection) { + mapped.add(convertValue(o, typeHint)); + } + + return mapped; + } + + return convertValue(value, typeHint); + } + @Nullable protected Object convertValue(@Nullable Object value, TypeInformation typeInformation) { @@ -407,33 +424,14 @@ public class QueryMapper { Pair pair = (Pair) value; Object first = convertValue(pair.getFirst(), - typeInformation.getActualType() != null ? typeInformation.getRequiredActualType() - : TypeInformation.OBJECT); + typeInformation.getActualType() != null ? typeInformation.getRequiredActualType() : TypeInformation.OBJECT); Object second = convertValue(pair.getSecond(), - typeInformation.getActualType() != null ? typeInformation.getRequiredActualType() - : TypeInformation.OBJECT); + typeInformation.getActualType() != null ? typeInformation.getRequiredActualType() : TypeInformation.OBJECT); return Pair.of(first, second); } - if (value instanceof Iterable) { - - List mapped = new ArrayList<>(); - - for (Object o : (Iterable) value) { - mapped.add(convertValue(o, typeInformation.getActualType() != null ? typeInformation.getRequiredActualType() - : TypeInformation.OBJECT)); - } - - return mapped; - } - - if (value.getClass().isArray() - && (TypeInformation.OBJECT.equals(typeInformation) || typeInformation.isCollectionLike())) { - return value; - } - return this.converter.writeValue(value, typeInformation); } diff --git a/spring-data-r2dbc/src/test/java/org/springframework/data/r2dbc/query/QueryMapperUnitTests.java b/spring-data-r2dbc/src/test/java/org/springframework/data/r2dbc/query/QueryMapperUnitTests.java index e0e78ff7..b283569d 100644 --- a/spring-data-r2dbc/src/test/java/org/springframework/data/r2dbc/query/QueryMapperUnitTests.java +++ b/spring-data-r2dbc/src/test/java/org/springframework/data/r2dbc/query/QueryMapperUnitTests.java @@ -22,7 +22,7 @@ import static org.springframework.data.domain.Sort.Order.*; import java.util.Collections; import org.junit.jupiter.api.Test; - +import org.springframework.core.convert.converter.Converter; import org.springframework.data.domain.Sort; import org.springframework.data.r2dbc.convert.MappingR2dbcConverter; import org.springframework.data.r2dbc.convert.R2dbcConverter; @@ -39,6 +39,8 @@ import org.springframework.data.relational.core.sql.Table; import org.springframework.r2dbc.core.Parameter; import org.springframework.r2dbc.core.binding.BindMarkersFactory; import org.springframework.r2dbc.core.binding.BindTarget; +import org.testcontainers.shaded.com.fasterxml.jackson.databind.JsonNode; +import org.testcontainers.shaded.com.fasterxml.jackson.databind.node.TextNode; /** * Unit tests for {@link QueryMapper}. @@ -53,7 +55,8 @@ class QueryMapperUnitTests { QueryMapper createMapper(R2dbcDialect dialect) { - R2dbcCustomConversions conversions = R2dbcCustomConversions.of(dialect); + R2dbcCustomConversions conversions = R2dbcCustomConversions.of(dialect, JsonNodeToStringConverter.INSTANCE, + StringToJsonNodeConverter.INSTANCE); R2dbcMappingContext context = new R2dbcMappingContext(); context.setSimpleTypeHolder(conversions.getSimpleTypeHolder()); @@ -463,6 +466,28 @@ class QueryMapperUnitTests { assertThat(bindings.getBindings().iterator().next().getValue()).isEqualTo((byte) 1); } + @Test // gh-1452 + void shouldMapJsonNodeToString() { + + Criteria criteria = Criteria.where("jsonNode").is(new TextNode("foo")); + + BoundCondition bindings = map(criteria); + + assertThat(bindings.getCondition()).hasToString("person.json_node = ?[$1]"); + assertThat(bindings.getBindings().iterator().next().getValue()).isEqualTo("foo"); + } + + @Test // gh-1452 + void shouldMapJsonNodeListToString() { + + Criteria criteria = Criteria.where("jsonNode").in(new TextNode("foo"), new TextNode("bar")); + + BoundCondition bindings = map(criteria); + + assertThat(bindings.getCondition()).hasToString("person.json_node IN (?[$1], ?[$2])"); + assertThat(bindings.getBindings().iterator().next().getValue()).isEqualTo("foo"); + } + private BoundCondition map(Criteria criteria) { BindMarkersFactory markers = BindMarkersFactory.indexed("$", 1); @@ -478,9 +503,29 @@ class QueryMapperUnitTests { MyEnum enumValue; boolean state; + + JsonNode jsonNode; } enum MyEnum { ONE, TWO, } + + enum JsonNodeToStringConverter implements Converter { + INSTANCE; + + @Override + public String convert(JsonNode source) { + return source.asText(); + } + } + + enum StringToJsonNodeConverter implements Converter { + INSTANCE; + + @Override + public JsonNode convert(String source) { + return new TextNode(source); + } + } } diff --git a/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/BasicRelationalConverter.java b/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/BasicRelationalConverter.java index 04ceb0ff..91a862ea 100644 --- a/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/BasicRelationalConverter.java +++ b/spring-data-relational/src/main/java/org/springframework/data/relational/core/conversion/BasicRelationalConverter.java @@ -15,6 +15,8 @@ */ package org.springframework.data.relational.core.conversion; +import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.List; import java.util.Optional; @@ -171,6 +173,32 @@ public class BasicRelationalConverter implements RelationalConverter { return getPotentiallyConvertedSimpleWrite(value); } + // TODO: We should add conversion support for arrays, however, + // these should consider multi-dimensional arrays as well. + if (value.getClass().isArray() && (TypeInformation.OBJECT.equals(type) || type.isCollectionLike())) { + return value; + } + + if (value instanceof Collection) { + + List mapped = new ArrayList<>(); + + TypeInformation component = TypeInformation.OBJECT; + if (type.isCollectionLike() && type.getActualType() != null) { + component = type.getRequiredComponentType(); + } + + for (Object o : (Iterable) value) { + mapped.add(writeValue(o, component)); + } + + if (type.getType().isInstance(mapped) || !type.isCollectionLike()) { + return mapped; + } + + return conversionService.convert(mapped, type.getType()); + } + RelationalPersistentEntity persistentEntity = context.getPersistentEntity(value.getClass()); if (persistentEntity != null) {