Polishing.

Introduce factory methods to flatten predicates. Also, avoid list copies.

See #4195

Author: mp911de

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

@@ -431,19 +431,7 @@ public Predicate neq(Expression value) {
 	}
 
 	public static @Nullable Predicate or(List<Predicate> intermediate) {
-
-		Predicate predicate = null;
-
-		for (Predicate other : intermediate) {
-
-			if (predicate == null) {
-				predicate = other;
-			} else {
-				predicate = predicate.or(other);
-			}
-		}
-
-		return predicate;
+		return intermediate.isEmpty() ? null : OrPredicate.of(intermediate);
 	}
 
 	/**
@@ -712,23 +700,7 @@ public interface Predicate extends Renderable {
 		@Contract("_ -> new")
 		@CheckReturnValue
 		default Predicate or(Predicate other) {
-
-			// Flatten OR composition: Or([a,b]).or(c) -> Or([a,b,c])
-			List<Predicate> parts = new ArrayList<>();
-
-			if (this instanceof OrPredicate op) {
-				parts.addAll(op.parts());
-			} else {
-				parts.add(this);
-			}
-
-			if (other instanceof OrPredicate op) {
-				parts.addAll(op.parts());
-			} else {
-				parts.add(other);
-			}
-
-			return new OrPredicate(parts);
+			return OrPredicate.of(this, other);
 		}
 
 		/**
@@ -740,26 +712,7 @@ default Predicate or(Predicate other) {
 		@Contract("_ -> new")
 		@CheckReturnValue
 		default Predicate and(Predicate other) {
-
-			// don't like the structuring of this and the nest() thing
-			//
-			// Flatten AND composition to avoid deep nesting:
-			// And([a,b]).and(c) -> And([a,b,c])
-			List<Predicate> parts = new ArrayList<>();
-
-			if (this instanceof AndPredicate ap) {
-				parts.addAll(ap.parts());
-			} else {
-				parts.add(this);
-			}
-
-			if (other instanceof AndPredicate ap) {
-				parts.addAll(ap.parts());
-			} else {
-				parts.add(other);
-			}
-
-			return new AndPredicate(parts);
+			return AndPredicate.of(this, other);
 		}
 
 		/**
@@ -1566,20 +1519,40 @@ private static boolean hasParenthesis(String str) {
 	record AndPredicate(List<Predicate> parts) implements Predicate {
 
 		AndPredicate {
-
 			Assert.notEmpty(parts, "Predicate parts must not be empty");
+		}
 
-			// Flatten nested AND predicates to keep the internal representation normalized.
-			List<Predicate> flattened = new ArrayList<>(parts.size());
-			for (Predicate p : parts) {
-				if (p instanceof AndPredicate ap) {
-					flattened.addAll(ap.parts());
+		public static AndPredicate of(List<Predicate> predicates) {
+
+			if (predicates.size() == 1) {
+
+				Predicate predicate = predicates.get(0);
+				if (predicate instanceof AndPredicate ap) {
+					return ap;
+				}
+
+				return new AndPredicate(Collections.singletonList(predicate));
+			}
+
+			List<Predicate> flattened = new ArrayList<>(predicates.size());
+			for (Predicate p : predicates) {
+				if (p instanceof AndPredicate op) {
+					flattened.addAll(op.parts());
 				} else {
 					flattened.add(p);
 				}
 			}
 
-			parts = List.copyOf(flattened);
+			return new AndPredicate(flattened);
+		}
+
+		public static AndPredicate of(Predicate predicate, Predicate other) {
+			return of(List.of(predicate, other));
+		}
+
+		@Override
+		public Predicate and(Predicate other) {
+			return of(this, other);
 		}
 
 		@Override
@@ -1598,20 +1571,29 @@ public String toString() {
 	record OrPredicate(List<Predicate> parts) implements Predicate {
 
 		OrPredicate {
-
 			Assert.notEmpty(parts, "Predicate parts must not be empty");
+		}
+
+		public static OrPredicate of(List<Predicate> predicates) {
 
-			// Flatten nested OR predicates to keep the internal representation normalized.
-			List<Predicate> flattened = new ArrayList<>(parts.size());
-			for (Predicate p : parts) {
+			if (predicates.size() == 1) {
+				return new OrPredicate(Collections.singletonList(predicates.get(0)));
+			}
+
+			List<Predicate> flattened = new ArrayList<>(predicates.size());
+			for (Predicate p : predicates) {
 				if (p instanceof OrPredicate op) {
 					flattened.addAll(op.parts());
 				} else {
 					flattened.add(p);
 				}
 			}
 
-			parts = List.copyOf(flattened);
+			return new OrPredicate(flattened);
+		}
+
+		public static OrPredicate of(Predicate predicate, Predicate other) {
+			return of(List.of(predicate, other));
 		}
 
 		@Override
@@ -1621,6 +1603,11 @@ public String render(RenderContext context) {
 					.collect(Collectors.joining(" OR "));
 		}
 
+		@Override
+		public Predicate or(Predicate other) {
+			return of(this, other);
+		}
+
 		@Override
 		public String toString() {
 			return render(RenderContext.EMPTY);

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

@@ -181,7 +181,7 @@ void inPredicateWithNestedExpression() {
 				.isEqualTo("o.country IN (SELECT c.code FROM Country c WHERE c.active = true)");
 	}
 
-	@Test
+	@Test // GH-4195
 	void chainedAndRendersWithoutExtraParentheses() {
 
 		Entity entity = entity(LineItem.class);
@@ -196,12 +196,14 @@ void chainedAndRendersWithoutExtraParentheses() {
 		Predicate b = where(bPath).eq(literal("ex40"));
 		Predicate c = where(aPath).isNotNull();
 
-		String fragment = a.and(b).and(c).render(ctx(entity));
+		AndPredicate and = (AndPredicate) a.and(b).and(c);
+		String fragment = and.render(ctx(entity));
 
+		assertThat(and.parts()).hasSize(3);
 		assertThat(fragment).isEqualTo("p.name = 'ex30' AND p_0.name = 'ex40' AND p.name IS NOT NULL");
 	}
 
-	@Test
+	@Test // GH-4195
 	void chainedOrRendersWithoutExtraParentheses() {
 
 		Entity entity = entity(Order.class);
@@ -211,8 +213,10 @@ void chainedOrRendersWithoutExtraParentheses() {
 		Predicate b = where(id).eq(literal("2"));
 		Predicate c = where(id).eq(literal("3"));
 
-		String fragment = a.or(b).or(c).render(ctx(entity));
+		OrPredicate or = (OrPredicate) a.or(b).or(c);
+		String fragment = or.render(ctx(entity));
 
+		assertThat(or.parts()).hasSize(3);
 		assertThat(fragment).isEqualTo("o.id = '1' OR o.id = '2' OR o.id = '3'");
 	}
 
@@ -369,10 +373,6 @@ static AbstractStringAssert<?> assertThatRendered(Renderable renderable) {
 		return contextual(RenderContext.EMPTY).assertThat(renderable);
 	}
 
-	static AbstractStringAssert<?> assertThat(String actual) {
-		return Assertions.assertThat(actual);
-	}
-
 	record ContextualAssert(RenderContext context) {
 
 		public AbstractStringAssert<?> assertThat(Renderable renderable) {