From 7cdb2b53c878d8ec6e48dc2f8993861307f0d824 Mon Sep 17 00:00:00 2001 From: Peter-Josef Meisch Date: Sat, 4 May 2024 09:44:33 +0200 Subject: [PATCH] Add the filter parts of a CriteriaQuery to the query, not as post-filter. Closes #2857 --- .../client/elc/CriteriaFilterProcessor.java | 10 +++-- .../client/elc/CriteriaQueryProcessor.java | 25 ++++++++---- .../client/elc/RequestConverter.java | 18 ++++----- .../query/NativeQueryIntegrationTests.java | 40 +++++++++++++++++++ .../QueryKeywordsIntegrationTests.java | 35 ---------------- .../utils/IndexNameProvider.java | 8 +++- 6 files changed, 77 insertions(+), 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 index 8d56aebc4..140f87ac5 100644 --- 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 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 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 index b18f6d405..435ba9a70 100644 --- 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 index 50efe0823..c0fa6d932 100644 --- 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 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/src/test/java/org/springframework/data/elasticsearch/core/query/NativeQueryIntegrationTests.java b/src/test/java/org/springframework/data/elasticsearch/core/query/NativeQueryIntegrationTests.java index 579b22ffe..a03bc9348 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/query/NativeQueryIntegrationTests.java +++ b/src/test/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) searchHits.getAggregations().aggregations(); + assertThat(aggregations).hasSize(1); + } + @Test // #2391 @DisplayName("should be able to use StringQuery in a NativeQuery") void shouldBeAbleToUseStringQueryInANativeQuery() { diff --git a/src/test/java/org/springframework/data/elasticsearch/repository/query/keywords/QueryKeywordsIntegrationTests.java b/src/test/java/org/springframework/data/elasticsearch/repository/query/keywords/QueryKeywordsIntegrationTests.java index 1bb15c09e..757484e73 100644 --- a/src/test/java/org/springframework/data/elasticsearch/repository/query/keywords/QueryKeywordsIntegrationTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/repository/query/keywords/QueryKeywordsIntegrationTests.java @@ -79,11 +79,6 @@ 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); } @@ -91,11 +86,6 @@ public void shouldSupportAND() { @Test public void shouldSupportOR() { - // given - - // when - - // then assertThat(repository.findByNameOrPrice("Sugar", 1.9f)).hasSize(4); assertThat(repository.findByNameOrText("Salt", "Beet sugar")).hasSize(3); } @@ -103,11 +93,6 @@ public void shouldSupportOR() { @Test public void shouldSupportTrueAndFalse() { - // given - - // when - - // then assertThat(repository.findByAvailableTrue()).hasSize(3); assertThat(repository.findByAvailableFalse()).hasSize(4); } @@ -115,11 +100,6 @@ public void shouldSupportTrueAndFalse() { @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 index 0b19964af..5a4820edd 100644 --- 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)); + } }