From ca3689412c54abb12eab079485e4a27eaac6fa9b Mon Sep 17 00:00:00 2001 From: Christoph Strobl Date: Thu, 12 Feb 2026 15:21:08 +0100 Subject: [PATCH] Port changes to composite id handling from R2DBC to JDBC QueryMapper. Also Criteria.where(...).notIn() with an empty collection should render to 1 = 1 (DATAJDBC-604). See: #2225 --- .../data/jdbc/core/convert/QueryMapper.java | 106 +++++++++++++++++- .../core/convert/SqlParametersFactory.java | 4 +- .../core/convert/QueryMapperUnitTests.java | 44 ++++++++ .../data/r2dbc/query/QueryMapper.java | 2 +- .../r2dbc/query/QueryMapperUnitTests.java | 2 +- 5 files changed, 153 insertions(+), 5 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 cf8adcdf1..0e0924fd5 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 @@ -55,6 +55,7 @@ import org.springframework.util.ClassUtils; * @author Jens Schauder * @author Yan Qiang * @author Mikhail Fedorov + * @author Christoph Strobl * @since 3.0 */ public class QueryMapper { @@ -277,9 +278,50 @@ public class QueryMapper { Assert.notNull(criteriaColumn, "Column must not be null"); Field propertyField = createPropertyField(entity, criteriaColumn, this.mappingContext); + Comparator comparator = criteria.getComparator(); + Object value = criteria.getValue(); // Single embedded entity if (propertyField.isEmbedded()) { + + // IN/NOT_IN with collection of composite/embedded values: expand to (… AND …) OR (…) + if ((Comparator.IN.equals(comparator) || Comparator.NOT_IN.equals(comparator)) + && value instanceof Collection collection) { + + Condition condition = null; + + for (Object o : collection) { + + CriteriaWrapper cw = new CriteriaWrapper(criteria) { + + @Override + public @Nullable Comparator getComparator() { + return Comparator.IN.equals(comparator) ? Comparator.EQ : Comparator.NEQ; + } + + @Nullable + @Override + public Object getValue() { + return o; + } + + @Override + public @Nullable SqlIdentifier getColumn() { + return criteriaColumn; + } + }; + + Condition c = mapCondition(cw, parameterSource, table, entity); + condition = condition == null ? c : condition.or(c); + } + + if (condition == null) { + return Comparator.IN.equals(comparator) ? Conditions.unrestricted().not() : Conditions.unrestricted(); + } + + return condition; + } + PersistentPropertyPath path = ((MetadataBackedField) propertyField).getPath(); Assert.state(path != null, "Path must not be null"); @@ -291,7 +333,6 @@ public class QueryMapper { Column column = table.column(propertyField.getMappedColumnName()); Object mappedValue; SQLType sqlType; - Comparator comparator = criteria.getComparator(); Assert.notNull(comparator, "Comparator must not be null"); @@ -680,6 +721,69 @@ public class QueryMapper { return uniqueName; } + abstract static class CriteriaWrapper implements CriteriaDefinition { + + private final CriteriaDefinition delegate; + + public CriteriaWrapper(CriteriaDefinition delegate) { + this.delegate = delegate; + } + + @Nullable + @Override + public Comparator getComparator() { + return delegate.getComparator(); + } + + @Override + public boolean isIgnoreCase() { + return delegate.isIgnoreCase(); + } + @Override + public boolean isGroup() { + return false; + } + + @Override + public List getGroup() { + return List.of(); + } + + @Nullable + @Override + public SqlIdentifier getColumn() { + return null; + } + + + @Nullable + @Override + public Object getValue() { + return null; + } + + @Nullable + @Override + public CriteriaDefinition getPrevious() { + return null; + } + + @Override + public boolean hasPrevious() { + return false; + } + + @Override + public boolean isEmpty() { + return false; + } + + @Override + public Combinator getCombinator() { + throw new UnsupportedOperationException("No combinator for AbstractCriteria"); + } + } + /** * Value object to represent a field and its meta-information. */ diff --git a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlParametersFactory.java b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlParametersFactory.java index 6aeb9abcb..9ea45999a 100644 --- a/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlParametersFactory.java +++ b/spring-data-jdbc/src/main/java/org/springframework/data/jdbc/core/convert/SqlParametersFactory.java @@ -154,7 +154,7 @@ public class SqlParametersFactory { ParameterSourceHolder holder = new ParameterSourceHolder(); BiFunction valueExtractor = getIdMapper(complexId); - List<@Nullable Object> parameterValues = new ArrayList<>(ids instanceof Collection c ? c.size() : 16); + List parameterValues = new ArrayList<>(ids instanceof Collection c ? c.size() : 16); if (complexId == null || columns.size() == 1) { @@ -164,7 +164,7 @@ public class SqlParametersFactory { } else { for (Object id : ids) { - List<@Nullable Object> tuple = new ArrayList<>(columns.size()); + List tuple = new ArrayList<>(columns.size()); appendIdentifier(holder, columns, id, valueExtractor, tuple); parameterValues.add(tuple.toArray(new Object[0])); } diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/QueryMapperUnitTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/QueryMapperUnitTests.java index 41a2531ac..d0984bbdb 100644 --- a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/QueryMapperUnitTests.java +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/core/convert/QueryMapperUnitTests.java @@ -29,6 +29,7 @@ import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; import org.springframework.core.convert.converter.Converter; +import org.springframework.data.annotation.Id; import org.springframework.data.domain.Sort; import org.springframework.data.jdbc.core.mapping.JdbcMappingContext; import org.springframework.data.relational.core.mapping.Column; @@ -47,6 +48,7 @@ import org.springframework.jdbc.core.namedparam.MapSqlParameterSource; * @author Mark Paluch * @author Jens Schauder * @author Mikhail Fedorov + * @author Christoph Strobl */ public class QueryMapperUnitTests { @@ -322,6 +324,36 @@ public class QueryMapperUnitTests { assertThat(condition).hasToString("person.\"NAME\" NOT IN (?[:name], ?[:name1], ?[:name2])"); } + @Test // GH-2208 + void shouldMapInComposite() { + + Criteria criteria = Criteria.where("id").in(new CompositeId(1, "a")); + assertThat(map(criteria, WithCompositeId.class)) + .hasToString("(withcompositeid.\"TENANT\" = ?[:tenant] AND withcompositeid.\"NAME\" = ?[:name])"); + + criteria = Criteria.where("id").in(new CompositeId(1, "a"), new CompositeId(2, "b")); + assertThat(map(criteria, WithCompositeId.class)).hasToString( + "(withcompositeid.\"TENANT\" = ?[:tenant2] AND withcompositeid.\"NAME\" = ?[:name3]) OR (withcompositeid.\"TENANT\" = ?[:tenant4] AND withcompositeid.\"NAME\" = ?[:name5])"); + + criteria = Criteria.where("id").in(); + assertThat(map(criteria, WithCompositeId.class)).hasToString("1 = 0"); + } + + @Test // GH-2208 + void shouldMapNotInComposite() { + + Criteria criteria = Criteria.where("id").notIn(new CompositeId(1, "a")); + assertThat(map(criteria, WithCompositeId.class)) + .hasToString("(withcompositeid.\"TENANT\" != ?[:tenant] AND withcompositeid.\"NAME\" != ?[:name])"); + + criteria = Criteria.where("id").notIn(new CompositeId(1, "a"), new CompositeId(2, "b")); + assertThat(map(criteria, WithCompositeId.class)).hasToString( + "(withcompositeid.\"TENANT\" != ?[:tenant2] AND withcompositeid.\"NAME\" != ?[:name3]) OR (withcompositeid.\"TENANT\" != ?[:tenant4] AND withcompositeid.\"NAME\" != ?[:name5])"); + + criteria = Criteria.where("id").notIn(); + assertThat(map(criteria, WithCompositeId.class)).hasToString("1 = 1"); + } + @Test void shouldMapIsNotInWithCollectionToStringConverter() { @@ -464,12 +496,24 @@ public class QueryMapperUnitTests { context.getRequiredPersistentEntity(Person.class)); } + private Condition map(Criteria criteria, Class entityType) { + + return mapper.getMappedObject(parameterSource, criteria, Table.create("withcompositeid"), + context.getRequiredPersistentEntity(entityType)); + } + static class Person { String name; @Column("another_name") String alternative; } + record CompositeId(int tenant, String name) { + } + + record WithCompositeId(@Id CompositeId id) { + } + enum CollectionToStringConverter implements Converter, String> { INSTANCE; 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 b22fa9882..40cd51cea 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 @@ -396,7 +396,7 @@ public class QueryMapper { } if (condition == null) { - return Conditions.unrestricted().not(); + return Comparator.IN.equals(comparator) ? Conditions.unrestricted().not() : Conditions.unrestricted(); } return condition; 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 ab6482db9..ae5f20693 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 @@ -395,7 +395,7 @@ class QueryMapperUnitTests { "(withcompositeid.tenant != ?[$1] AND withcompositeid.name != ?[$2]) OR (withcompositeid.tenant != ?[$3] AND withcompositeid.name != ?[$4])"); criteria = Criteria.where("id").notIn(); - assertThat(map(criteria, WithCompositeId.class).getCondition()).hasToString("1 = 0"); + assertThat(map(criteria, WithCompositeId.class).getCondition()).hasToString("1 = 1"); } @Test