Refine JPQL rendering for IN expressions using parameter binding.

We now no longer wrap bound parameters with parenthesis as leaving parenthesis away is the spec-compliant approach that also works with EclipseLink and allows to remove the nasty empty-collection to null conversion.

See #4112

Author: mp911de

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/JpqlQueryBuilder.java

@@ -1476,7 +1476,8 @@ public String render(RenderContext context) {
 			Expression predicate = this.predicate;
 			String rendered = predicate.render(context);
 
-			return (hasParenthesis(rendered) ? "%s %s %s" : "%s %s (%s)").formatted(path.render(context), operator, rendered);
+			return ((hasParenthesis(rendered) || predicate instanceof ParameterExpression) ? "%s %s %s" : "%s %s (%s)")
+					.formatted(path.render(context), operator, rendered);
 		}
 
 		@Override

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ParameterBinding.java

@@ -229,24 +229,16 @@ public static class PartTreeParameterBinding extends ParameterBinding {
 		private final boolean ignoreCase;
 		private final boolean noWildcards;
 		private final @Nullable Object value;
-		private final PersistenceProvider persistenceProvider;
 
 		public PartTreeParameterBinding(BindingIdentifier identifier, ParameterOrigin origin, Class<?> parameterType,
 				Part part, @Nullable Object value, JpqlQueryTemplates templates, EscapeCharacter escape) {
-			this(identifier, origin, parameterType, part, value, templates, escape, PersistenceProvider.GENERIC_JPA);
-		}
-
-		PartTreeParameterBinding(BindingIdentifier identifier, ParameterOrigin origin, Class<?> parameterType, Part part,
-				@Nullable Object value, JpqlQueryTemplates templates, EscapeCharacter escape,
-				PersistenceProvider persistenceProvider) {
 
 			super(identifier, origin);
 
 			this.parameterType = parameterType;
 			this.templates = templates;
 			this.escape = escape;
 			this.value = value;
-			this.persistenceProvider = persistenceProvider;
 			this.type = value == null
 					&& (Type.SIMPLE_PROPERTY.equals(part.getType()) || Type.NEGATING_SIMPLE_PROPERTY.equals(part.getType()))
 							? Type.IS_NULL
@@ -293,7 +285,7 @@ public JpqlQueryTemplates getTemplates() {
 			}
 
 			return Collection.class.isAssignableFrom(parameterType) //
-					? potentiallyIgnoreCase(ignoreCase, toCollection(value, persistenceProvider)) //
+					? potentiallyIgnoreCase(ignoreCase, toCollection(value)) //
 					: value;
 		}
 
@@ -316,26 +308,23 @@ public JpqlQueryTemplates getTemplates() {
 		/**
 		 * Returns the given argument as {@link Collection} which means it will return it as is if it's a
 		 * {@link Collection}, turn an array into an {@link ArrayList} or simply wrap any other value into a single-element
-		 * {@link Collection} by default. {@link PersistenceProvider#HIBERNATE} translates empty collections to disjunctions
-		 * to avoid syntax errors.
+		 * {@link Collection} by default.
 		 *
 		 * @param value the value to be converted to a {@link Collection}.
-		 * @param provider the persistence provider to use.
 		 * @return the object itself as a {@link Collection} or a {@link Collection} constructed from the value.
 		 */
-		private static @Nullable Collection<?> toCollection(@Nullable Object value, PersistenceProvider provider) {
+		private static @Nullable Collection<?> toCollection(@Nullable Object value) {
 
 			if (value == null) {
 				return null;
 			}
 
 			if (value instanceof Collection<?> collection) {
-				return provider == PersistenceProvider.HIBERNATE || !collection.isEmpty() ? collection : null;
+				return collection;
 			}
 
 			if (ObjectUtils.isArray(value)) {
-				Object[] array = ObjectUtils.toObjectArray(value);
-				return provider == PersistenceProvider.HIBERNATE || !ObjectUtils.isEmpty(array) ? Arrays.asList(array) : null;
+				return Arrays.asList(ObjectUtils.toObjectArray(value));
 			}
 
 			return Collections.singleton(value);

spring-data-jpa/src/main/java/org/springframework/data/jpa/repository/query/ParameterMetadataProvider.java

@@ -226,7 +226,7 @@ private <T> PartTreeParameterBinding next(Part part, Class<T> type, Parameter pa
 		/* identifier refers to bindable parameters, not _all_ parameters index */
 		MethodInvocationArgument methodParameter = ParameterOrigin.ofParameter(origin);
 		PartTreeParameterBinding binding = new PartTreeParameterBinding(bindingIdentifier,
-				methodParameter, reifiedType, part, value, templates, escape, persistenceProvider);
+				methodParameter, reifiedType, part, value, templates, escape);
 
 		// PartTreeParameterBinding is more expressive than a potential ParameterBinding for Vector.
 		bindings.add(binding);

spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/EclipseLinkNamespaceUserRepositoryTests.java

@@ -51,12 +51,6 @@ void sortByAssociationPropertyShouldUseLeftOuterJoin() {}
 	@Override
 	void sortByAssociationPropertyInPageableShouldUseLeftOuterJoin() {}
 
-	/**
-	 * Ignored until https://bugs.eclipse.org/bugs/show_bug.cgi?id=349477 is resolved.
-	 */
-	@Override
-	void findByElementCollectionAttribute() {}
-
 	/**
 	 * This test will fail once https://bugs.eclipse.org/bugs/show_bug.cgi?id=521915 is fixed.
 	 */
@@ -142,11 +136,6 @@ void findByElementVarargInAttributeIgnoreCase() {}
 	@Test // DATAJPA-1303
 	void findByElementCollectionInAttributeIgnoreCaseWithNulls() {}
 
-	@Disabled("Binding collections to IN predicates not supported")
-	@Override
-	@Test
-	void invokesQueryWithVarargsParametersCorrectly() {}
-
 	@Disabled("Named parameters in native SQL queries are not supported in EclipseLink")
 	@Override
 	@Test
@@ -237,16 +226,6 @@ public void findByFluentPredicateWithProjectionAndPageRequest() {}
 	@Test
 	public void findByFluentPredicateWithProjectionAndAll() {}
 
-	@Disabled("Binding collections to IN predicates not supported")
-	@Override
-	@Test
-	public void findByCollectionWithPageRequest() {}
-
-	@Disabled("Binding collections to IN predicates not supported")
-	@Override
-	@Test
-	public void findByCollectionWithPageable() {}
-
 	@Disabled("EclipseLink does not support records")
 	@Override
 	@Test

spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/JpaQueryCreatorTests.java

@@ -385,7 +385,7 @@ void in() {
 				.forTree(Product.class, "findProductByNameIn") //
 				.withParameters(List.of("spring", "data")) //
 				.as(QueryCreatorTester::create) //
-				.expectJpql("SELECT p FROM %s p WHERE p.name IN (?1)", DefaultJpaEntityMetadata.unqualify(Product.class)) //
+				.expectJpql("SELECT p FROM %s p WHERE p.name IN ?1", DefaultJpaEntityMetadata.unqualify(Product.class)) //
 				.expectPlaceholderValue("?1", List.of("spring", "data")) //
 				.validateQuery();
 	}
@@ -397,7 +397,7 @@ void notIn() {
 				.forTree(Product.class, "findProductByNameNotIn") //
 				.withParameters(List.of("spring", "data")) //
 				.as(QueryCreatorTester::create) //
-				.expectJpql("SELECT p FROM %s p WHERE p.name NOT IN (?1)", DefaultJpaEntityMetadata.unqualify(Product.class)) //
+				.expectJpql("SELECT p FROM %s p WHERE p.name NOT IN ?1", DefaultJpaEntityMetadata.unqualify(Product.class)) //
 				.expectPlaceholderValue("?1", List.of("spring", "data")) //
 				.validateQuery();
 	}

spring-data-jpa/src/test/java/org/springframework/data/jpa/repository/query/ParameterBindingUnitTests.java

@@ -20,14 +20,9 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Set;
-import java.util.stream.Stream;
 
 import org.junit.jupiter.api.Test;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.Arguments;
-import org.junit.jupiter.params.provider.MethodSource;
 
-import org.springframework.data.jpa.provider.PersistenceProvider;
 import org.springframework.data.jpa.repository.query.ParameterBinding.BindingIdentifier;
 import org.springframework.data.jpa.repository.query.ParameterBinding.ParameterOrigin;
 import org.springframework.data.jpa.repository.support.JpqlQueryTemplates;
@@ -42,54 +37,34 @@
  */
 class ParameterBindingUnitTests {
 
-	static ParameterBinding hibernateCollectionBinding = new ParameterBinding.PartTreeParameterBinding(
+	static ParameterBinding collectionBinding = new ParameterBinding.PartTreeParameterBinding(
 			BindingIdentifier.of("foo"), ParameterOrigin.ofParameter("foo"), Collection.class,
-			new Part("name", TestEntity.class), null, JpqlQueryTemplates.UPPER, EscapeCharacter.DEFAULT,
-			PersistenceProvider.HIBERNATE);
-
-	static ParameterBinding eclipselinkCollectionBinding = new ParameterBinding.PartTreeParameterBinding(
-			BindingIdentifier.of("foo"), ParameterOrigin.ofParameter("foo"), Collection.class,
-			new Part("name", TestEntity.class), null, JpqlQueryTemplates.UPPER, EscapeCharacter.DEFAULT,
-			PersistenceProvider.ECLIPSELINK);
+			new Part("name", TestEntity.class), null, JpqlQueryTemplates.UPPER, EscapeCharacter.DEFAULT);
 
 	@Test // GH-4112
-	void shouldPrepareEmptyListForHibernate() {
+	void shouldPrepareEmptyList() {
 
-		assertThat(hibernateCollectionBinding.prepare(List.of())).isEqualTo(List.of());
-		assertThat(hibernateCollectionBinding.prepare(new Integer[0])).isEqualTo(List.of());
+		assertThat(collectionBinding.prepare(List.of())).isEqualTo(List.of());
+		assertThat(collectionBinding.prepare(new Integer[0])).isEqualTo(List.of());
 	}
 
 	@Test // GH-4112
-	void shouldPrepareEmptyListToNullForEclipselink() {
-
-		assertThat(eclipselinkCollectionBinding.prepare(List.of())).isNull();
-		assertThat(eclipselinkCollectionBinding.prepare(new Integer[0])).isNull();
-	}
+	void shouldPrepareListToCollection() {
 
-	@ParameterizedTest // GH-4112
-	@MethodSource("bindings")
-	void shouldPrepareListToCollection(ParameterBinding binding) {
-
-		assertThat(binding.prepare(List.of("foo"))).isEqualTo(List.of("foo"));
-		assertThat(binding.prepare(new Integer[] { 1, 2, 3 })).isEqualTo(List.of(1, 2, 3));
+		assertThat(collectionBinding.prepare(List.of("foo"))).isEqualTo(List.of("foo"));
+		assertThat(collectionBinding.prepare(new Integer[] { 1, 2, 3 })).isEqualTo(List.of(1, 2, 3));
 	}
 
-	@ParameterizedTest // GH-4112
-	@MethodSource("bindings")
-	void shouldPrepareValueToSingleElementCollection(ParameterBinding binding) {
-		assertThat(binding.prepare("foo")).isEqualTo(Set.of("foo"));
+	@Test // GH-4112
+	void shouldPrepareValueToSingleElementCollection() {
+		assertThat(collectionBinding.prepare("foo")).isEqualTo(Set.of("foo"));
 	}
 
-	@ParameterizedTest // GH-4112
-	@MethodSource("bindings")
-	void shouldPrepareNull(ParameterBinding binding) {
-		assertThat(binding.prepare(null)).isNull();
+	@Test // GH-4112
+	void shouldPrepareNull() {
+		assertThat(collectionBinding.prepare(null)).isNull();
 	}
 
-	static Stream<Arguments.ArgumentSet> bindings() {
-		return Stream.of(Arguments.argumentSet("Hibernate", hibernateCollectionBinding),
-				Arguments.argumentSet("EclipseLink", eclipselinkCollectionBinding));
-	}
 
 	private record TestEntity(String name) {