Add the filter parts of a CriteriaQuery to the query, not as post-fil…

…ter. Original Pull Request #2906 Closes #2857
spring-projects · May 4, 2024 · dc5bf5a · dc5bf5a
1 parent 1d89054
commit dc5bf5a
Show file tree

Hide file tree

Showing 6 changed files with 77 additions and 59 deletions.
diff --git a/src/main/java/org/springframework/data/elasticsearch/client/elc/CriteriaFilterProcessor.java b/src/main/java/org/springframework/data/elasticsearch/client/elc/CriteriaFilterProcessor.java
@@ -70,9 +70,13 @@ public static Optional<Query> createQuery(Criteria criteria) {
 		for (Criteria chainedCriteria : criteria.getCriteriaChain()) {
 
 			if (chainedCriteria.isOr()) {
-				BoolQuery.Builder boolQueryBuilder = QueryBuilders.bool();
-				queriesForEntries(chainedCriteria).forEach(boolQueryBuilder::should);
-				filterQueries.add(new Query(boolQueryBuilder.build()));
+				Collection<? extends Query> queriesForEntries = queriesForEntries(chainedCriteria);
+
+				if (!queriesForEntries.isEmpty()) {
+					BoolQuery.Builder boolQueryBuilder = QueryBuilders.bool();
+					queriesForEntries.forEach(boolQueryBuilder::should);
+					filterQueries.add(new Query(boolQueryBuilder.build()));
+				}
 			} else if (chainedCriteria.isNegating()) {
 
 				Assert.notNull(criteria.getField(), "criteria must have a field");

diff --git a/src/main/java/org/springframework/data/elasticsearch/client/elc/CriteriaQueryProcessor.java b/src/main/java/org/springframework/data/elasticsearch/client/elc/CriteriaQueryProcessor.java
@@ -29,6 +29,7 @@
 import java.util.ArrayList;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Objects;
 
 import org.springframework.data.elasticsearch.annotations.FieldType;
 import org.springframework.data.elasticsearch.core.query.Criteria;
@@ -115,11 +116,18 @@ public static Query createQuery(Criteria criteria) {
 			}
 		}
 
+		var filterQuery = CriteriaFilterProcessor.createQuery(criteria);
 		if (shouldQueries.isEmpty() && mustNotQueries.isEmpty() && mustQueries.isEmpty()) {
-			return null;
+
+			if (filterQuery.isEmpty()) {
+				return null;
+			}
+
+			// we need something to add the filter to
+			mustQueries.add(Query.of(qb -> qb.matchAll(m -> m)));
 		}
 
-		Query query = new Query.Builder().bool(boolQueryBuilder -> {
+        return new Query.Builder().bool(boolQueryBuilder -> {
 
 			if (!shouldQueries.isEmpty()) {
 				boolQueryBuilder.should(shouldQueries);
@@ -133,10 +141,10 @@ public static Query createQuery(Criteria criteria) {
 				boolQueryBuilder.must(mustQueries);
 			}
 
+			filterQuery.ifPresent(boolQueryBuilder::filter);
+
 			return boolQueryBuilder;
 		}).build();
-
-		return query;
 	}
 
 	@Nullable
@@ -238,7 +246,7 @@ private static Query.Builder queryFor(Criteria.CriteriaEntry entry, Field field,
 				queryBuilder.queryString(queryStringQuery(fieldName, '*' + searchText, true, boost));
 				break;
 			case EXPRESSION:
-				queryBuilder.queryString(queryStringQuery(fieldName, value.toString(), boost));
+				queryBuilder.queryString(queryStringQuery(fieldName, Objects.requireNonNull(value).toString(), boost));
 				break;
 			case LESS:
 				queryBuilder //
@@ -270,6 +278,7 @@ private static Query.Builder queryFor(Criteria.CriteriaEntry entry, Field field,
 				break;
 			case BETWEEN:
 				Object[] ranges = (Object[]) value;
+				Assert.notNull(value, "value for a between condition must not be null");
 				queryBuilder //
 						.range(rb -> {
 							rb.field(fieldName);
@@ -293,10 +302,10 @@ private static Query.Builder queryFor(Criteria.CriteriaEntry entry, Field field,
 								.boost(boost)); //
 				break;
 			case MATCHES:
-				queryBuilder.match(matchQuery(fieldName, value.toString(), Operator.Or, boost));
+				queryBuilder.match(matchQuery(fieldName, Objects.requireNonNull(value).toString(), Operator.Or, boost));
 				break;
 			case MATCHES_ALL:
-				queryBuilder.match(matchQuery(fieldName, value.toString(), Operator.And, boost));
+				queryBuilder.match(matchQuery(fieldName, Objects.requireNonNull(value).toString(), Operator.And, boost));
 
 				break;
 			case IN:
@@ -345,7 +354,7 @@ private static Query.Builder queryFor(Criteria.CriteriaEntry entry, Field field,
 				queryBuilder //
 						.regexp(rb -> rb //
 								.field(fieldName) //
-								.value(value.toString()) //
+								.value(Objects.requireNonNull(value).toString()) //
 								.boost(boost)); //
 				break;
 			case HAS_CHILD:

diff --git a/src/main/java/org/springframework/data/elasticsearch/client/elc/RequestConverter.java b/src/main/java/org/springframework/data/elasticsearch/client/elc/RequestConverter.java
@@ -1210,7 +1210,7 @@ public <T> SearchRequest searchRequest(Query query, @Nullable String routing, @N
 			builder.routing(routing);
 		}
 
-		addFilter(query, builder);
+		addPostFilter(query, builder);
 
 		return builder.build();
 	}
@@ -1758,22 +1758,18 @@ co.elastic.clients.elasticsearch._types.query_dsl.Query getQuery(@Nullable Query
 		return getEsQuery(query, (q) -> elasticsearchConverter.updateQuery(q, clazz));
 	}
 
-	private void addFilter(Query query, SearchRequest.Builder builder) {
+	@SuppressWarnings("StatementWithEmptyBody")
+	private void addPostFilter(Query query, SearchRequest.Builder builder) {
 
-		if (query instanceof CriteriaQuery) {
-			CriteriaFilterProcessor.createQuery(((CriteriaQuery) query).getCriteria()).ifPresent(builder::postFilter);
-		} else // noinspection StatementWithEmptyBody
-		if (query instanceof StringQuery) {
-			// no filter for StringQuery
-		} else if (query instanceof NativeQuery nativeQuery) {
+		// we only need to handle NativeQuery here. filter from a CriteriaQuery are added into the query and not as post
+		// filter anymore, StringQuery do not have post filters
+		if (query instanceof NativeQuery nativeQuery) {
 
 			if (nativeQuery.getFilter() != null) {
 				builder.postFilter(nativeQuery.getFilter());
 			} else if (nativeQuery.getSpringDataQuery() != null) {
-				addFilter(nativeQuery.getSpringDataQuery(), builder);
+				addPostFilter(nativeQuery.getSpringDataQuery(), builder);
 			}
-		} else {
-			throw new IllegalArgumentException("unhandled Query implementation " + query.getClass().getName());
 		}
 	}
 

diff --git a/...t/java/org/springframework/data/elasticsearch/core/query/NativeQueryIntegrationTests.java b/...t/java/org/springframework/data/elasticsearch/core/query/NativeQueryIntegrationTests.java
@@ -17,6 +17,11 @@
 
 import static org.assertj.core.api.Assertions.*;
 
+import co.elastic.clients.elasticsearch._types.GeoHashPrecision;
+import co.elastic.clients.elasticsearch._types.aggregations.Aggregation;
+
+import java.util.List;
+
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Order;
@@ -26,6 +31,7 @@
 import org.springframework.data.elasticsearch.annotations.Document;
 import org.springframework.data.elasticsearch.annotations.Field;
 import org.springframework.data.elasticsearch.annotations.FieldType;
+import org.springframework.data.elasticsearch.client.elc.ElasticsearchAggregation;
 import org.springframework.data.elasticsearch.client.elc.NativeQuery;
 import org.springframework.data.elasticsearch.core.ElasticsearchOperations;
 import org.springframework.data.elasticsearch.core.geo.GeoPoint;
@@ -98,6 +104,40 @@ void shouldBeAbleToUseCriteriaQueryWithFilterArgumentsInANativeQuery() {
 		assertThat(searchHits.getSearchHit(0).getId()).isEqualTo(entity1.getId());
 	}
 
+	@Test // #2857
+	@DisplayName("should apply CriteriaQuery with filter arguments in a NativeQuery to aggregations")
+	void shouldBeAbleToUseCriteriaQueryWithFilterArgumentsInANativeQueryToAggregations() {
+		var entity1 = new SampleEntity();
+		entity1.setId("60");
+		var location1 = new GeoPoint(60.0, 60.0);
+		entity1.setLocation(location1);
+		entity1.setText("60");
+		var entity2 = new SampleEntity();
+		entity2.setId("70");
+		entity2.setText("70");
+		var location70 = new GeoPoint(70.0, 70.0);
+		entity2.setLocation(location70);
+		operations.save(entity1, entity2);
+
+		var criteriaQuery = new CriteriaQuery(Criteria.where("location").within(location1, "10km"));
+		var nativeQuery = NativeQuery.builder()
+				.withQuery(criteriaQuery)
+				.withAggregation("geohashgrid", Aggregation.of(ab -> ab
+						.geohashGrid(ghg -> ghg
+								.field("location")
+								.precision(GeoHashPrecision.of(ghp -> ghp.distance("10000km"))))))
+				.build();
+
+		var searchHits = operations.search(nativeQuery, SampleEntity.class);
+
+		assertThat(searchHits.getTotalHits()).isEqualTo(1);
+		assertThat(searchHits.getSearchHit(0).getId()).isEqualTo(entity1.getId());
+		assertThat(searchHits.getAggregations()).isNotNull();
+		// noinspection unchecked
+		var aggregations = (List<ElasticsearchAggregation>) searchHits.getAggregations().aggregations();
+		assertThat(aggregations).hasSize(1);
+	}
+
 	@Test // #2391
 	@DisplayName("should be able to use StringQuery in a NativeQuery")
 	void shouldBeAbleToUseStringQueryInANativeQuery() {

diff --git a/...framework/data/elasticsearch/repository/query/keywords/QueryKeywordsIntegrationTests.java b/...framework/data/elasticsearch/repository/query/keywords/QueryKeywordsIntegrationTests.java
@@ -79,47 +79,27 @@ void cleanup() {
 	@Test
 	public void shouldSupportAND() {
 
-		// given
-
-		// when
-
-		// then
 		assertThat(repository.findByNameAndText("Sugar", "Cane sugar")).hasSize(2);
 		assertThat(repository.findByNameAndPrice("Sugar", 1.1f)).hasSize(1);
 	}
 
 	@Test
 	public void shouldSupportOR() {
 
-		// given
-
-		// when
-
-		// then
 		assertThat(repository.findByNameOrPrice("Sugar", 1.9f)).hasSize(4);
 		assertThat(repository.findByNameOrText("Salt", "Beet sugar")).hasSize(3);
 	}
 
 	@Test
 	public void shouldSupportTrueAndFalse() {
 
-		// given
-
-		// when
-
-		// then
 		assertThat(repository.findByAvailableTrue()).hasSize(3);
 		assertThat(repository.findByAvailableFalse()).hasSize(4);
 	}
 
 	@Test
 	public void shouldSupportInAndNotInAndNot() {
 
-		// given
-
-		// when
-
-		// then
 		assertThat(repository.findByPriceIn(Arrays.asList(1.2f, 1.1f))).hasSize(2);
 		assertThat(repository.findByPriceNotIn(Arrays.asList(1.2f, 1.1f))).hasSize(5);
 		assertThat(repository.findByPriceNot(1.2f)).hasSize(6);
@@ -128,33 +108,18 @@ public void shouldSupportInAndNotInAndNot() {
 	@Test // DATAES-171
 	public void shouldWorkWithNotIn() {
 
-		// given
-
-		// when
-
-		// then
 		assertThat(repository.findByIdNotIn(Arrays.asList("2", "3"))).hasSize(5);
 	}
 
 	@Test
 	public void shouldSupportBetween() {
 
-		// given
-
-		// when
-
-		// then
 		assertThat(repository.findByPriceBetween(1.0f, 2.0f)).hasSize(4);
 	}
 
 	@Test
 	public void shouldSupportLessThanAndGreaterThan() {
 
-		// given
-
-		// when
-
-		// then
 		assertThat(repository.findByPriceLessThan(1.1f)).hasSize(1);
 		assertThat(repository.findByPriceLessThanEqual(1.1f)).hasSize(2);
 

diff --git a/src/test/java/org/springframework/data/elasticsearch/utils/IndexNameProvider.java b/src/test/java/org/springframework/data/elasticsearch/utils/IndexNameProvider.java
@@ -16,7 +16,7 @@
 package org.springframework.data.elasticsearch.utils;
 
 /**
- * Class providing an index name with a prefix and an index number
+ * Class providing an index name with a prefix, an index number and a random 6-digit number.
  *
  * @author Peter-Josef Meisch
  */
@@ -35,7 +35,7 @@ public IndexNameProvider(String prefix) {
 	}
 
 	public void increment() {
-		indexName = prefix + '-' + ++idx;
+		indexName = prefix + '-' + ++idx + '-' + sixDigits();
 	}
 
 	public String indexName() {
@@ -48,4 +48,8 @@ public String indexName() {
 	public String getPrefix() {
 		return prefix;
 	}
+
+	private String sixDigits() {
+		return String.valueOf((int) (100000 + Math.random() * 900000));
+	}
 }