diff --git a/pom.xml b/pom.xml index 18d77da98f..7db87ddf3d 100644 --- a/pom.xml +++ b/pom.xml @@ -4,7 +4,7 @@ org.springframework.data spring-data-relational-parent - 4.1.0-SNAPSHOT + 4.1.x-GH-2276-SNAPSHOT pom Spring Data Relational Parent diff --git a/spring-data-jdbc-distribution/pom.xml b/spring-data-jdbc-distribution/pom.xml index 824e70541b..0f3538b8c1 100644 --- a/spring-data-jdbc-distribution/pom.xml +++ b/spring-data-jdbc-distribution/pom.xml @@ -14,7 +14,7 @@ org.springframework.data spring-data-relational-parent - 4.1.0-SNAPSHOT + 4.1.x-GH-2276-SNAPSHOT ../pom.xml diff --git a/spring-data-jdbc/pom.xml b/spring-data-jdbc/pom.xml index 87567d6220..0b8bc0e8a5 100644 --- a/spring-data-jdbc/pom.xml +++ b/spring-data-jdbc/pom.xml @@ -6,7 +6,7 @@ 4.0.0 spring-data-jdbc - 4.1.0-SNAPSHOT + 4.1.x-GH-2276-SNAPSHOT Spring Data JDBC Spring Data module for JDBC repositories. @@ -15,7 +15,7 @@ org.springframework.data spring-data-relational-parent - 4.1.0-SNAPSHOT + 4.1.x-GH-2276-SNAPSHOT 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 69c77295e1..8a64101dc3 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 @@ -22,14 +22,15 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Function; import java.util.regex.Pattern; import org.jspecify.annotations.Nullable; - import org.springframework.data.core.PropertyPath; import org.springframework.data.core.PropertyReferenceException; import org.springframework.data.core.TypeInformation; import org.springframework.data.domain.Sort; +import org.springframework.data.jdbc.core.mapping.AggregateReference; import org.springframework.data.jdbc.core.mapping.JdbcValue; import org.springframework.data.jdbc.support.JdbcUtil; import org.springframework.data.mapping.MappingException; @@ -41,12 +42,28 @@ 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.relational.core.sql.Aliased; +import org.springframework.data.relational.core.sql.AndCondition; +import org.springframework.data.relational.core.sql.AsteriskFromTable; +import org.springframework.data.relational.core.sql.Column; +import org.springframework.data.relational.core.sql.Condition; +import org.springframework.data.relational.core.sql.Conditions; +import org.springframework.data.relational.core.sql.Expression; +import org.springframework.data.relational.core.sql.Expressions; +import org.springframework.data.relational.core.sql.Functions; +import org.springframework.data.relational.core.sql.OrderByField; +import org.springframework.data.relational.core.sql.OrCondition; +import org.springframework.data.relational.core.sql.SQL; +import org.springframework.data.relational.core.sql.SimpleFunction; +import org.springframework.data.relational.core.sql.SqlIdentifier; +import org.springframework.data.relational.core.sql.Table; +import org.springframework.data.relational.core.sql.TableLike; import org.springframework.data.relational.domain.SqlSort; import org.springframework.data.util.Pair; import org.springframework.jdbc.core.namedparam.MapSqlParameterSource; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; +import org.springframework.util.CollectionUtils; /** * Maps {@link CriteriaDefinition} and {@link Sort} objects considering mapping metadata and dialect-specific @@ -303,42 +320,11 @@ private Condition mapCondition(CriteriaDefinition criteria, MapSqlParameterSourc // 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 = Conditions.nest(mapCondition(cw, parameterSource, table, entity, true)); - condition = condition == null ? c : condition.or(c); - } - - if (condition == null) { - return Comparator.IN.equals(comparator) ? Conditions.unrestricted().not() : Conditions.unrestricted(); - } + && value instanceof Collection collection) { - return condition; + return expandInCollectionComparison(comparator, collection, + element -> mapCondition(new ListElementCriteria(criteria, element), parameterSource, table, entity, true)); } PersistentPropertyPath path = ((MetadataBackedField) propertyField).getPath(); @@ -347,9 +333,35 @@ public Object getValue() { .getRequiredPersistentEntity(propertyField.getRequiredProperty()); Condition condition = mapEmbeddedObjectCondition(criteria, parameterSource, table, embeddedEntity, embedded); + if (!embedded && condition instanceof OrCondition) { + return Conditions.nest(condition); + } return embedded || !(condition instanceof AndCondition) ? condition : Conditions.nest(condition); } + // AggregateReference (and similar associations) to a composite identifier: expand IN/NOT_IN like embedded + if (propertyField instanceof MetadataBackedField metadataBackedField && metadataBackedField.property != null) { + + RelationalPersistentProperty associationProperty = metadataBackedField.property; + + if (Association.isAssociation(associationProperty)) { + + Association association = Association.from(associationProperty, converter); + + if (association.isComplexIdentifier() // + && (Comparator.IN.equals(comparator) || Comparator.NOT_IN.equals(comparator)) + && value instanceof Collection collection) { + + RelationalPersistentEntity identifierEntity = association.getRequiredTargetIdentifierEntity(); + + return expandInCollectionComparison(comparator, collection, + element -> mapEmbeddedObjectCondition( + new ListElementCriteria(criteria, unwrapAssociationCriteriaValue(element)), parameterSource, table, + identifierEntity, true)); + } + } + } + TypeInformation actualType = propertyField.getTypeHint().getRequiredActualType(); Column column = table.column(propertyField.getMappedColumnName()); Object mappedValue; @@ -384,6 +396,29 @@ public Object getValue() { return createCondition(column, mappedValue, sqlType, parameterSource, comparator, criteria.isIgnoreCase()); } + /** + * Expands {@link Comparator#IN}/{@link Comparator#NOT_IN} over a collection to a disjunction of nested conditions, + * one per element (used for embedded types and composite association identifiers). + *

+ * IN: (col = v AND …) OR (…) per element. + * NOT_IN: AND over tuple negations; each negation is (col != v OR …), i.e. NOT (c1 = v1 AND c2 = v2) ≡ (c1 != v1 OR c2 != v2). + */ + @SuppressWarnings("NullAway") + private Condition expandInCollectionComparison(Comparator comparator, Collection collection, + Function nestedConditionFactory) { + + if (CollectionUtils.isEmpty(collection)) { + return Comparator.IN.equals(comparator) ? Conditions.unrestricted().not() : Conditions.unrestricted(); + } + + Condition condition = null; + for (Object element : collection) { + Condition next = Conditions.nest(nestedConditionFactory.apply(element)); + condition = condition == null ? next : (Comparator.IN.equals(comparator) ? condition.or(next) : condition.and(next)); + } + return condition; + } + /** * Converts values while taking specific value types like arrays, {@link Iterable}, or {@link Pair}. * @@ -469,6 +504,15 @@ private JdbcValue getWriteValue(RelationalPersistentProperty property, Object va ); } + private static Object unwrapAssociationCriteriaValue(Object value) { + + if (value instanceof AggregateReference aggregateReference) { + return aggregateReference.getId(); + } + + return value; + } + private Condition mapEmbeddedObjectCondition(CriteriaDefinition criteria, MapSqlParameterSource parameterSource, Table table, RelationalPersistentEntity embeddedEntity, boolean embedded) { @@ -477,30 +521,22 @@ private Condition mapEmbeddedObjectCondition(CriteriaDefinition criteria, MapSql PersistentPropertyAccessor embeddedAccessor = embeddedEntity.getPropertyAccessor(criteria.getValue()); - Condition condition = Conditions.unrestricted(); - for (RelationalPersistentProperty embeddedProperty : embeddedEntity) { - - Object propertyValue = embeddedAccessor.getProperty(embeddedProperty); + Comparator tupleComparator = criteria.getComparator(); + Assert.notNull(tupleComparator, "Comparator must not be null"); - CriteriaWrapper cw = new CriteriaWrapper(criteria) { + boolean negateTuple = Comparator.NEQ.equals(tupleComparator); - @Override - public SqlIdentifier getColumn() { - return SqlIdentifier.unquoted(embeddedProperty.getName()); - } + Condition condition = null; + for (RelationalPersistentProperty embeddedProperty : embeddedEntity) { - @Nullable - @Override - public Object getValue() { - return propertyValue; - } - }; + Object propertyValue = embeddedAccessor.getProperty(embeddedProperty); + CriteriaDefinition cw = new EmbeddedPropertyCriteria(criteria, embeddedProperty, propertyValue); Condition mapped = mapCondition(cw, parameterSource, table, embeddedEntity, embedded); - condition = condition.and(mapped); + condition = condition == null ? mapped : (negateTuple ? condition.or(mapped) : condition.and(mapped)); } - return condition; + return condition != null ? condition : Conditions.unrestricted(); } @Nullable @@ -742,6 +778,66 @@ private static String getUniqueName(MapSqlParameterSource parameterSource, Strin return uniqueName; } + /** + * {@link CriteriaDefinition} view of one element when expanding {@code IN}/{@code NOT IN} over embedded or composite + * identifier values. + */ + private static final class ListElementCriteria extends CriteriaWrapper { + + // private final SqlIdentifier column; + // private final Comparator comparator; + private final Object elementValue; + + ListElementCriteria(CriteriaDefinition delegate, Object elementValue) { + super(delegate); + // this.column = column; + // this.comparator = comparator; + this.elementValue = elementValue; + } + + @Override + public @Nullable Comparator getComparator() { + Comparator elementComparator = getDelegate().getComparator(); + return Comparator.IN.equals(elementComparator) ? Comparator.EQ : Comparator.NEQ; + } + + @Override + public @Nullable SqlIdentifier getColumn() { + return getDelegate().getColumn(); + } + + @Override + public @Nullable Object getValue() { + return this.elementValue; + } + } + + /** + * {@link CriteriaDefinition} for a single property of an embedded object, delegating flags to the outer criteria. + */ + private static final class EmbeddedPropertyCriteria extends CriteriaWrapper { + + private final SqlIdentifier propertyColumn; + private final @Nullable Object propertyValue; + + EmbeddedPropertyCriteria(CriteriaDefinition delegate, RelationalPersistentProperty embeddedProperty, + @Nullable Object propertyValue) { + super(delegate); + this.propertyColumn = SqlIdentifier.unquoted(embeddedProperty.getName()); + this.propertyValue = propertyValue; + } + + @Override + public SqlIdentifier getColumn() { + return this.propertyColumn; + } + + @Override + public @Nullable Object getValue() { + return this.propertyValue; + } + } + abstract static class CriteriaWrapper implements CriteriaDefinition { private final CriteriaDefinition delegate; @@ -750,6 +846,10 @@ public CriteriaWrapper(CriteriaDefinition delegate) { this.delegate = delegate; } + protected CriteriaDefinition getDelegate() { + return delegate; + } + @Nullable @Override public Comparator getComparator() { @@ -760,6 +860,7 @@ public Comparator getComparator() { public boolean isIgnoreCase() { return delegate.isIgnoreCase(); } + @Override public boolean isGroup() { return false; @@ -776,7 +877,6 @@ public SqlIdentifier getColumn() { return null; } - @Nullable @Override public Object getValue() { 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 8d51df3f9d..aecce963bd 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 @@ -236,7 +236,7 @@ void upsertInsertsWhenIdDoesNotExistAndUpdatesWhenItExists() { void upsertUpdatesExistingWithNullValues() { long id = 8891L; - withSqlServerIdentityInsertOn(template, List.of("LEGO_SET", "MANUAL"), () -> { + withSqlServerIdentityInsertOn(template, List.of("LEGO_SET"), () -> { LegoSet lego = new LegoSet(); lego.id = id; @@ -245,7 +245,11 @@ void upsertUpdatesExistingWithNullValues() { Manual manual = new Manual(); manual.id = 42L; manual.content = "Accelerates to 99% of light speed; Destroys almost everything. See https://what-if.xkcd.com/1/"; - lego.manual = manual; + + // Only one table with identity insert on at the time guard + if (!(template.getDataAccessStrategy().getDialect() instanceof SqlServerDialect)) { + lego.manual = manual; + } template.upsert(lego); @@ -255,8 +259,12 @@ void upsertUpdatesExistingWithNullValues() { LegoSet loaded = template.findById(id, LegoSet.class); assertThat(loaded.name).isEqualTo(null); - assertThat(loaded.manual).isNotNull(); - assertThat(loaded.manual.content).isEqualTo(manual.content); + + if (!(template.getDataAccessStrategy().getDialect() instanceof SqlServerDialect)) { + assertThat(loaded.manual).isNotNull(); + assertThat(loaded.manual.content).isEqualTo(manual.content); + } + }); } 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 da67573aaf..41b82dd11f 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 @@ -32,6 +32,7 @@ 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.AggregateReference; import org.springframework.data.jdbc.core.mapping.JdbcMappingContext; import org.springframework.data.relational.core.mapping.Column; import org.springframework.data.relational.core.mapping.Embedded; @@ -191,7 +192,7 @@ void shouldMapCompositeIdCriteria() { criteria = Criteria.where("id").not(new CompositeId(1, "a")).or("foo").is("bar"); assertThat(map(criteria, WithCompositeId.class)).hasToString( - "(withcompositeid.\"TENANT\" != ?[:tenant3] AND withcompositeid.\"NAME\" != ?[:name4]) OR withcompositeid.foo = ?[:foo5]"); + "(withcompositeid.\"TENANT\" != ?[:tenant3] OR withcompositeid.\"NAME\" != ?[:name4]) OR withcompositeid.foo = ?[:foo5]"); } @Test // DATAJDBC-318 @@ -361,16 +362,60 @@ 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])"); + .hasToString("(withcompositeid.\"TENANT\" != ?[:tenant] OR 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])"); + "(withcompositeid.\"TENANT\" != ?[:tenant2] OR withcompositeid.\"NAME\" != ?[:name3]) AND (withcompositeid.\"TENANT\" != ?[:tenant4] OR withcompositeid.\"NAME\" != ?[:name5])"); criteria = Criteria.where("id").notIn(); assertThat(map(criteria, WithCompositeId.class)).hasToString("1 = 1"); } + @Test // GH-2276 + void shouldMapInForAggregateReferenceWithCompositeId() { + + MapSqlParameterSource parameters = new MapSqlParameterSource(); + + Criteria criteria = Criteria.where("referredRoot").in(AggregateReference.to(new TargetCompositeId(1, "a")), + AggregateReference.to(new TargetCompositeId(2, "b"))); + + Condition condition = mapper.getMappedObject(parameters, criteria, Table.create("dependantroot"), + context.getRequiredPersistentEntity(DependantRoot.class)); + + assertThat(condition).hasToString( + "(dependantroot.\"TENANT\" = ?[:tenant] AND dependantroot.\"NAME\" = ?[:name]) OR (dependantroot.\"TENANT\" = ?[:tenant2] AND dependantroot.\"NAME\" = ?[:name3])"); + } + + @Test // GH-2276 + void shouldMapInForBareCompositeIdWhenPropertyIsAggregateReference() { + + MapSqlParameterSource parameters = new MapSqlParameterSource(); + + Criteria criteria = Criteria.where("referredRoot").in(List.of(new TargetCompositeId(1, "a"), new TargetCompositeId(2, "b"))); + + Condition condition = mapper.getMappedObject(parameters, criteria, Table.create("dependantroot"), + context.getRequiredPersistentEntity(DependantRoot.class)); + + assertThat(condition).hasToString( + "(dependantroot.\"TENANT\" = ?[:tenant] AND dependantroot.\"NAME\" = ?[:name]) OR (dependantroot.\"TENANT\" = ?[:tenant2] AND dependantroot.\"NAME\" = ?[:name3])"); + } + + @Test // GH-2276 + void shouldMapNotInForAggregateReferenceWithCompositeId() { + + MapSqlParameterSource parameters = new MapSqlParameterSource(); + + Criteria criteria = Criteria.where("referredRoot").notIn(AggregateReference.to(new TargetCompositeId(1, "a")), + AggregateReference.to(new TargetCompositeId(2, "b"))); + + Condition condition = mapper.getMappedObject(parameters, criteria, Table.create("dependantroot"), + context.getRequiredPersistentEntity(DependantRoot.class)); + + assertThat(condition).hasToString( + "(dependantroot.\"TENANT\" != ?[:tenant] OR dependantroot.\"NAME\" != ?[:name]) AND (dependantroot.\"TENANT\" != ?[:tenant2] OR dependantroot.\"NAME\" != ?[:name3])"); + } + @Test void shouldMapIsNotInWithCollectionToStringConverter() { @@ -599,6 +644,18 @@ private record CompositeId(int tenant, String name) { private record WithCompositeId(@Id CompositeId id) { } + private record TargetCompositeId(int tenant, String name) { + } + + private static class ReferencedRoot { + @Id TargetCompositeId id; + } + + private static class DependantRoot { + + AggregateReference referredRoot; + } + static class WithEmbeddable { @Embedded.Nullable(prefix = "home_") Address home; diff --git a/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/CompositeIdJdbcRepositoryIntegrationTests.java b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/CompositeIdJdbcRepositoryIntegrationTests.java new file mode 100644 index 0000000000..01c795e533 --- /dev/null +++ b/spring-data-jdbc/src/test/java/org/springframework/data/jdbc/repository/CompositeIdJdbcRepositoryIntegrationTests.java @@ -0,0 +1,87 @@ +/* + * Copyright 2026-present the original author or authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.data.jdbc.repository; + +import static org.assertj.core.api.Assertions.*; + +import java.util.Collection; +import java.util.List; + +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.context.annotation.ComponentScan; +import org.springframework.context.annotation.Configuration; +import org.springframework.context.annotation.FilterType; +import org.springframework.context.annotation.Import; +import org.springframework.data.annotation.Id; +import org.springframework.data.jdbc.core.JdbcAggregateOperations; +import org.springframework.data.jdbc.repository.config.EnableJdbcRepositories; +import org.springframework.data.jdbc.testing.IntegrationTest; +import org.springframework.data.jdbc.testing.TestConfiguration; +import org.springframework.data.relational.core.mapping.Column; +import org.springframework.data.relational.core.mapping.Embedded; +import org.springframework.data.relational.core.mapping.Table; +import org.springframework.data.repository.CrudRepository; + +/** + * Integration tests for JDBC repositories with a composite {@link Id}. + *

+ * + * @author Christoph Strobl + */ +@IntegrationTest +class CompositeIdJdbcRepositoryIntegrationTests { + + @Autowired WithCompositeIdRepository repository; + + @Autowired JdbcAggregateOperations jdbcAggregateOperations; + + @Test // GH-2276 + void findAllByCompositePkNotInLooksRowsUpCorrectly() { + + this.jdbcAggregateOperations.insert(new WithCompositeId(new CompositeId(42, "HBAR"), "Walter")); + this.jdbcAggregateOperations.insert(new WithCompositeId(new CompositeId(23, "2PI"), "Jesse")); + this.jdbcAggregateOperations.insert(new WithCompositeId(new CompositeId(42, "2PI"), "Extra")); + + List rows = this.repository.findAllByPkNotIn( + List.of(new CompositeId(42, "HBAR"), new CompositeId(23, "2PI"))); + + assertThat(rows).singleElement() // + .satisfies(row -> { + assertThat(row.name()).isEqualTo("Extra"); + assertThat(row.pk()).isEqualTo(new CompositeId(42, "2PI")); + }); + } + + @Configuration + @Import(TestConfiguration.class) + @EnableJdbcRepositories(considerNestedRepositories = true, + includeFilters = @ComponentScan.Filter(value = WithCompositeIdRepository.class, type = FilterType.ASSIGNABLE_TYPE)) + static class Config { + } + + interface WithCompositeIdRepository extends CrudRepository { + + List findAllByPkNotIn(Collection ids); + } + + @Table("with_composite_id") + record WithCompositeId(@Id @Embedded.Nullable CompositeId pk, @Column("NAME") String name) { + } + + record CompositeId(@Column("col_one") Integer one, @Column("col_two") String two) { + } +} diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-db2.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-db2.sql new file mode 100644 index 0000000000..111a069e28 --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-db2.sql @@ -0,0 +1,9 @@ +DROP TABLE "with_composite_id"; + +CREATE TABLE "with_composite_id" +( + "col_one" INTEGER NOT NULL, + "col_two" VARCHAR(255) NOT NULL, + "NAME" VARCHAR(255), + PRIMARY KEY ("col_one", "col_two") +); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-h2.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-h2.sql new file mode 100644 index 0000000000..be10ae5174 --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-h2.sql @@ -0,0 +1,9 @@ +DROP TABLE IF EXISTS "with_composite_id"; + +CREATE TABLE "with_composite_id" +( + "col_one" INT NOT NULL, + "col_two" VARCHAR(255) NOT NULL, + "NAME" VARCHAR(255), + PRIMARY KEY ("col_one", "col_two") +); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-hsql.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-hsql.sql new file mode 100644 index 0000000000..0f5b9a931d --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-hsql.sql @@ -0,0 +1,9 @@ +DROP TABLE "with_composite_id" IF EXISTS; + +CREATE TABLE "with_composite_id" +( + "col_one" INT NOT NULL, + "col_two" VARCHAR(255) NOT NULL, + "NAME" VARCHAR(255), + PRIMARY KEY ("col_one", "col_two") +); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-mariadb.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-mariadb.sql new file mode 100644 index 0000000000..ca666b4006 --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-mariadb.sql @@ -0,0 +1,9 @@ +DROP TABLE IF EXISTS with_composite_id; + +CREATE TABLE with_composite_id +( + col_one INT NOT NULL, + col_two VARCHAR(255) NOT NULL, + NAME VARCHAR(255), + PRIMARY KEY (col_one, col_two) +); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-mssql.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-mssql.sql new file mode 100644 index 0000000000..1be1b21083 --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-mssql.sql @@ -0,0 +1,9 @@ +DROP TABLE IF EXISTS with_composite_id; + +CREATE TABLE with_composite_id +( + col_one INT NOT NULL, + col_two VARCHAR(255) NOT NULL, + [NAME] VARCHAR(255), + PRIMARY KEY (col_one, col_two) +); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-mysql.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-mysql.sql new file mode 100644 index 0000000000..ca666b4006 --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-mysql.sql @@ -0,0 +1,9 @@ +DROP TABLE IF EXISTS with_composite_id; + +CREATE TABLE with_composite_id +( + col_one INT NOT NULL, + col_two VARCHAR(255) NOT NULL, + NAME VARCHAR(255), + PRIMARY KEY (col_one, col_two) +); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-oracle.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-oracle.sql new file mode 100644 index 0000000000..1686fc31cc --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-oracle.sql @@ -0,0 +1,9 @@ +DROP TABLE IF EXISTS "with_composite_id"; + +CREATE TABLE "with_composite_id" +( + "col_one" NUMBER NOT NULL, + "col_two" VARCHAR2(255) NOT NULL, + "NAME" VARCHAR2(255), + PRIMARY KEY ("col_one", "col_two") +); diff --git a/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-postgres.sql b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-postgres.sql new file mode 100644 index 0000000000..ef6d4a69f6 --- /dev/null +++ b/spring-data-jdbc/src/test/resources/org.springframework.data.jdbc.repository/CompositeIdJdbcRepositoryIntegrationTests-postgres.sql @@ -0,0 +1,9 @@ +DROP TABLE IF EXISTS "with_composite_id"; + +CREATE TABLE "with_composite_id" +( + "col_one" INTEGER NOT NULL, + "col_two" VARCHAR(255) NOT NULL, + "NAME" VARCHAR(255), + PRIMARY KEY ("col_one", "col_two") +); diff --git a/spring-data-r2dbc/pom.xml b/spring-data-r2dbc/pom.xml index 98a4e11e60..dadbee42a7 100644 --- a/spring-data-r2dbc/pom.xml +++ b/spring-data-r2dbc/pom.xml @@ -6,7 +6,7 @@ 4.0.0 spring-data-r2dbc - 4.1.0-SNAPSHOT + 4.1.x-GH-2276-SNAPSHOT Spring Data R2DBC Spring Data module for R2DBC @@ -15,7 +15,7 @@ org.springframework.data spring-data-relational-parent - 4.1.0-SNAPSHOT + 4.1.x-GH-2276-SNAPSHOT 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 e1b5465924..687eb28f90 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 @@ -20,6 +20,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.function.Function; import java.util.regex.Pattern; import org.jspecify.annotations.Nullable; @@ -51,6 +52,7 @@ import org.springframework.r2dbc.core.binding.MutableBindings; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; +import org.springframework.util.CollectionUtils; /** * Maps {@link CriteriaDefinition} and {@link Sort} objects considering mapping metadata and dialect-specific @@ -61,6 +63,7 @@ * @author Manousos Mathioudakis * @author Jens Schauder * @author Yan Qiang + * @author Christoph Strobl */ public class QueryMapper { @@ -368,10 +371,7 @@ private Condition mapCondition(CriteriaDefinition criteria, MutableBindings bind if ((Comparator.IN.equals(comparator) || Comparator.NOT_IN.equals(comparator)) && value instanceof Collection collection) { - Condition condition = null; - - for (Object o : collection) { - + return expandInCollectionComparison(comparator, collection, element -> { CriteriaWrapper cw = new CriteriaWrapper(criteria) { @Override @@ -382,7 +382,7 @@ private Condition mapCondition(CriteriaDefinition criteria, MutableBindings bind @Nullable @Override public Object getValue() { - return o; + return element; } @Override @@ -390,16 +390,8 @@ public Object getValue() { return criteriaColumn; } }; - - Condition c = Conditions.nest(mapCondition(cw, bindings, table, entity, true)); - condition = condition == null ? c : condition.or(c); - } - - if (condition == null) { - return Comparator.IN.equals(comparator) ? Conditions.unrestricted().not() : Conditions.unrestricted(); - } - - return condition; + return mapCondition(cw, bindings, table, entity, true); + }); } RelationalPersistentEntity embeddedEntity = mappingContext @@ -407,7 +399,10 @@ public Object getValue() { PersistentPropertyAccessor propertyAccessor = getEmbeddedPropertyAccessor(value, embeddedEntity, propertyField); - Condition condition = Conditions.unrestricted(); + Assert.notNull(comparator, "Comparator must not be null"); + boolean negateTuple = Comparator.NEQ.equals(comparator); + + Condition condition = null; for (RelationalPersistentProperty embeddedProperty : embeddedEntity) { @@ -428,9 +423,13 @@ public Object getValue() { }; Condition mapped = mapCondition(cw, bindings, table, embeddedEntity, true); - condition = condition.and(mapped); + condition = condition == null ? mapped : (negateTuple ? condition.or(mapped) : condition.and(mapped)); } + condition = condition != null ? condition : Conditions.unrestricted(); + if (!embedded && condition instanceof OrCondition) { + return Conditions.nest(condition); + } return embedded || !(condition instanceof AndCondition) ? condition : Conditions.nest(condition); } @@ -461,6 +460,29 @@ public Object getValue() { return createCondition(column, mappedValue, typeHint, bindings, comparator, criteria.isIgnoreCase()); } + /** + * Expands {@link Comparator#IN}/{@link Comparator#NOT_IN} over a collection to a disjunction of nested conditions, + * one per element (used for embedded types and composite association identifiers). + *

+ * IN: (col = v AND …) OR (…) per element. + * NOT_IN: AND over tuple negations; each negation is (col != v OR …), i.e. NOT (c1 = v1 AND c2 = v2) ≡ (c1 != v1 OR c2 != v2). + */ + @SuppressWarnings("NullAway") + private Condition expandInCollectionComparison(Comparator comparator, Collection collection, + Function nestedConditionFactory) { + + if (CollectionUtils.isEmpty(collection)) { + return Comparator.IN.equals(comparator) ? Conditions.unrestricted().not() : Conditions.unrestricted(); + } + + Condition condition = null; + for (Object element : collection) { + Condition next = Conditions.nest(nestedConditionFactory.apply(element)); + condition = condition == null ? next : (Comparator.IN.equals(comparator) ? condition.or(next) : condition.and(next)); + } + return condition; + } + static PersistentPropertyAccessor getEmbeddedPropertyAccessor(@Nullable Object value, RelationalPersistentEntity embeddedEntity, Field propertyField) { 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 da883024e9..57afd11d52 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 @@ -225,7 +225,7 @@ void shouldMapCompositeIdCriteria() { criteria = Criteria.where("id").not(new CompositeId(1, "a")).or("foo").is("bar"); assertThat(map(criteria, WithCompositeId.class).getCondition()).hasToString( - "(withcompositeid.tenant != ?[$1] AND withcompositeid.name != ?[$2]) OR withcompositeid.foo = ?[$3]"); + "(withcompositeid.tenant != ?[$1] OR withcompositeid.name != ?[$2]) OR withcompositeid.foo = ?[$3]"); } @Test // gh-300 @@ -402,11 +402,11 @@ void shouldMapNotInComposite() { Criteria criteria = Criteria.where("id").notIn(new CompositeId(1, "a")); assertThat(map(criteria, WithCompositeId.class).getCondition()) - .hasToString("(withcompositeid.tenant != ?[$1] AND withcompositeid.name != ?[$2])"); + .hasToString("(withcompositeid.tenant != ?[$1] OR withcompositeid.name != ?[$2])"); criteria = Criteria.where("id").notIn(new CompositeId(1, "a"), new CompositeId(2, "b")); assertThat(map(criteria, WithCompositeId.class).getCondition()).hasToString( - "(withcompositeid.tenant != ?[$1] AND withcompositeid.name != ?[$2]) OR (withcompositeid.tenant != ?[$3] AND withcompositeid.name != ?[$4])"); + "(withcompositeid.tenant != ?[$1] OR withcompositeid.name != ?[$2]) AND (withcompositeid.tenant != ?[$3] OR withcompositeid.name != ?[$4])"); criteria = Criteria.where("id").notIn(); assertThat(map(criteria, WithCompositeId.class).getCondition()).hasToString("1 = 1"); diff --git a/spring-data-r2dbc/src/test/java/org/springframework/data/r2dbc/repository/CompositeIdRepositoryIntegrationTests.java b/spring-data-r2dbc/src/test/java/org/springframework/data/r2dbc/repository/CompositeIdRepositoryIntegrationTests.java index f9716e6c85..8232589708 100644 --- a/spring-data-r2dbc/src/test/java/org/springframework/data/r2dbc/repository/CompositeIdRepositoryIntegrationTests.java +++ b/spring-data-r2dbc/src/test/java/org/springframework/data/r2dbc/repository/CompositeIdRepositoryIntegrationTests.java @@ -23,6 +23,7 @@ import reactor.test.StepVerifier; import java.util.ArrayList; +import java.util.Collection; import java.util.List; import java.util.Optional; import java.util.concurrent.atomic.AtomicInteger; @@ -61,6 +62,7 @@ * * @author Jens Schauder * @author Mark Paluch + * @author Christoph Strobl */ @ExtendWith(SpringExtension.class) public class CompositeIdRepositoryIntegrationTests { @@ -233,8 +235,27 @@ void emptyDeleteAllDoesNotDeleteItems() { .verifyComplete(); } + @Test // GH-2276 + void findAllByCompositePkNotInLooksRowsUpCorrectly() { + + this.jdbc.execute("INSERT INTO with_composite_id VALUES (42, '2PI','Extra')"); + + repository.findAllByPkNotIn(List.of(new CompositeId(42, "HBAR"), new CompositeId(23, "2PI"))) // + .collectList() // + .as(StepVerifier::create) // + .assertNext(rows -> assertThat(rows).singleElement() // + .satisfies(row -> { + assertThat(row.name()).isEqualTo("Extra"); + assertThat(row.pk()).isEqualTo(new CompositeId(42, "2PI")); + })) // + .verifyComplete(); + } + interface WithCompositeIdRepository extends ReactiveCrudRepository { + Flux findByName(String name); + + Flux findAllByPkNotIn(Collection ids); } @Table("with_composite_id") diff --git a/spring-data-relational/pom.xml b/spring-data-relational/pom.xml index 652ad2853d..1067a29223 100644 --- a/spring-data-relational/pom.xml +++ b/spring-data-relational/pom.xml @@ -6,7 +6,7 @@ 4.0.0 spring-data-relational - 4.1.0-SNAPSHOT + 4.1.x-GH-2276-SNAPSHOT Spring Data Relational Spring Data Relational support @@ -14,7 +14,7 @@ org.springframework.data spring-data-relational-parent - 4.1.0-SNAPSHOT + 4.1.x-GH-2276-SNAPSHOT