From ca6ef58195fe0f73bbc955229dea75e032b3fd8c Mon Sep 17 00:00:00 2001 From: Peter-Josef Meisch Date: Fri, 25 Sep 2020 09:23:35 +0200 Subject: [PATCH] DATAES-937 - Repository queries with IN filters fail with empty input list. Original PR: #525 (cherry picked from commit 7117e5d70d7b52aa689787b2201a3f26639bbcf8) --- .../core/CriteriaQueryProcessor.java | 45 +++++++---- .../elasticsearch/core/query/Criteria.java | 78 ++++++++----------- .../AbstractElasticsearchRepository.java | 8 +- .../query/keywords/QueryKeywordsTests.java | 37 ++++++--- 4 files changed, 100 insertions(+), 68 deletions(-) diff --git a/src/main/java/org/springframework/data/elasticsearch/core/CriteriaQueryProcessor.java b/src/main/java/org/springframework/data/elasticsearch/core/CriteriaQueryProcessor.java index 7205f8033..c0b222684 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/CriteriaQueryProcessor.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/CriteriaQueryProcessor.java @@ -15,7 +15,7 @@ */ package org.springframework.data.elasticsearch.core; -import static org.elasticsearch.index.query.Operator.AND; +import static org.elasticsearch.index.query.Operator.*; import static org.elasticsearch.index.query.QueryBuilders.*; import static org.springframework.data.elasticsearch.core.query.Criteria.*; @@ -25,7 +25,9 @@ import java.util.List; import java.util.ListIterator; import org.apache.lucene.queryparser.flexible.core.util.StringUtils; -import org.elasticsearch.index.query.*; +import org.apache.lucene.queryparser.flexible.standard.QueryParserUtil; +import org.elasticsearch.index.query.BoolQueryBuilder; +import org.elasticsearch.index.query.QueryBuilder; import org.springframework.data.elasticsearch.core.query.Criteria; import org.springframework.util.Assert; @@ -39,7 +41,6 @@ import org.springframework.util.Assert; */ class CriteriaQueryProcessor { - QueryBuilder createQueryFromCriteria(Criteria criteria) { if (criteria == null) return null; @@ -104,7 +105,6 @@ class CriteriaQueryProcessor { return query; } - private QueryBuilder createQueryFragmentForCriteria(Criteria chainedCriteria) { if (chainedCriteria.getQueryCriteriaEntries().isEmpty()) return null; @@ -131,8 +131,8 @@ class CriteriaQueryProcessor { return query; } - - private QueryBuilder processCriteriaEntry(Criteria.CriteriaEntry entry,/* OperationKey key, Object value,*/ String fieldName) { + private QueryBuilder processCriteriaEntry(Criteria.CriteriaEntry entry, + /* OperationKey key, Object value,*/ String fieldName) { Object value = entry.getValue(); if (value == null) { return null; @@ -180,17 +180,15 @@ class CriteriaQueryProcessor { query = fuzzyQuery(fieldName, searchText); break; case IN: - query = boolQuery(); - collection = (Iterable) value; - for (Object item : collection) { - ((BoolQueryBuilder) query).should(queryStringQuery(item.toString()).field(fieldName)); + if (value instanceof Iterable) { + Iterable iterable = (Iterable) value; + query = queryStringQuery(orQueryString(iterable)).field(fieldName); } break; case NOT_IN: - query = boolQuery(); - collection = (Iterable) value; - for (Object item : collection) { - ((BoolQueryBuilder) query).mustNot(queryStringQuery(item.toString()).field(fieldName)); + if (value instanceof Iterable) { + Iterable iterable = (Iterable) value; + query = queryStringQuery("NOT(" + orQueryString(iterable) + ')').field(fieldName); } break; } @@ -203,4 +201,23 @@ class CriteriaQueryProcessor { } query.boost(boost); } + + private static String orQueryString(Iterable iterable) { + StringBuilder sb = new StringBuilder(); + + for (Object item : iterable) { + + if (item != null) { + + if (sb.length() > 0) { + sb.append(' '); + } + sb.append('"'); + sb.append(QueryParserUtil.escape(item.toString())); + sb.append('"'); + } + } + + return sb.toString(); + } } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/query/Criteria.java b/src/main/java/org/springframework/data/elasticsearch/core/query/Criteria.java index 6056614fb..0ba3cc5b6 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/query/Criteria.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/query/Criteria.java @@ -15,10 +15,13 @@ */ package org.springframework.data.elasticsearch.core.query; -import java.util.*; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Set; -import org.springframework.util.ObjectUtils; -import org.springframework.util.StringUtils; import org.springframework.dao.InvalidDataAccessApiUsageException; import org.springframework.data.elasticsearch.core.geo.GeoBox; import org.springframework.data.elasticsearch.core.geo.GeoPoint; @@ -26,6 +29,8 @@ import org.springframework.data.geo.Box; import org.springframework.data.geo.Distance; import org.springframework.data.geo.Point; import org.springframework.util.Assert; +import org.springframework.util.ObjectUtils; +import org.springframework.util.StringUtils; /** * Criteria is the central class when constructing queries. It follows more or less a fluent API style, which allows to @@ -39,13 +44,9 @@ public class Criteria { @Override public String toString() { - return "Criteria{" + - "field=" + field.getName() + - ", boost=" + boost + - ", negating=" + negating + - ", queryCriteria=" + ObjectUtils.nullSafeToString(queryCriteria) + - ", filterCriteria=" + ObjectUtils.nullSafeToString(filterCriteria) + - '}'; + return "Criteria{" + "field=" + field.getName() + ", boost=" + boost + ", negating=" + negating + ", queryCriteria=" + + ObjectUtils.nullSafeToString(queryCriteria) + ", filterCriteria=" + + ObjectUtils.nullSafeToString(filterCriteria) + '}'; } public static final String WILDCARD = "*"; @@ -64,8 +65,7 @@ public class Criteria { private Set filterCriteria = new LinkedHashSet<>(); - public Criteria() { - } + public Criteria() {} /** * Creates a new Criteria with provided field name @@ -305,7 +305,7 @@ public class Criteria { throw new InvalidDataAccessApiUsageException("Range [* TO *] is not allowed"); } - queryCriteria.add(new CriteriaEntry(OperationKey.BETWEEN, new Object[]{lowerBound, upperBound})); + queryCriteria.add(new CriteriaEntry(OperationKey.BETWEEN, new Object[] { lowerBound, upperBound })); return this; } @@ -376,11 +376,6 @@ public class Criteria { } private List toCollection(Object... values) { - if (values.length == 0 || (values.length > 1 && values[1] instanceof Collection)) { - throw new InvalidDataAccessApiUsageException("At least one element " - + (values.length > 0 ? ("of argument of type " + values[1].getClass().getName()) : "") - + " has to be present."); - } return Arrays.asList(values); } @@ -398,15 +393,14 @@ public class Criteria { * Creates new CriteriaEntry for {@code location WITHIN distance} * * @param location {@link org.springframework.data.elasticsearch.core.geo.GeoPoint} center coordinates - * @param distance {@link String} radius as a string (e.g. : '100km'). - * Distance unit : - * either mi/miles or km can be set + * @param distance {@link String} radius as a string (e.g. : '100km'). Distance unit : either mi/miles or km can be + * set * @return Criteria the chaind criteria with the new 'within' criteria included. */ public Criteria within(GeoPoint location, String distance) { Assert.notNull(location, "Location value for near criteria must not be null"); Assert.notNull(location, "Distance value for near criteria must not be null"); - filterCriteria.add(new CriteriaEntry(OperationKey.WITHIN, new Object[]{location, distance})); + filterCriteria.add(new CriteriaEntry(OperationKey.WITHIN, new Object[] { location, distance })); return this; } @@ -414,56 +408,54 @@ public class Criteria { * Creates new CriteriaEntry for {@code location WITHIN distance} * * @param location {@link org.springframework.data.geo.Point} center coordinates - * @param distance {@link org.springframework.data.geo.Distance} radius - * . + * @param distance {@link org.springframework.data.geo.Distance} radius . * @return Criteria the chaind criteria with the new 'within' criteria included. */ public Criteria within(Point location, Distance distance) { Assert.notNull(location, "Location value for near criteria must not be null"); Assert.notNull(location, "Distance value for near criteria must not be null"); - filterCriteria.add(new CriteriaEntry(OperationKey.WITHIN, new Object[]{location, distance})); + filterCriteria.add(new CriteriaEntry(OperationKey.WITHIN, new Object[] { location, distance })); return this; } /** * Creates new CriteriaEntry for {@code geoLocation WITHIN distance} * - * @param geoLocation {@link String} center point - * supported formats: - * lat on = > "41.2,45.1", - * geohash = > "asd9as0d" - * @param distance {@link String} radius as a string (e.g. : '100km'). - * Distance unit : - * either mi/miles or km can be set + * @param geoLocation {@link String} center point supported formats: lat on = > "41.2,45.1", geohash = > "asd9as0d" + * @param distance {@link String} radius as a string (e.g. : '100km'). Distance unit : either mi/miles or km can be + * set * @return */ public Criteria within(String geoLocation, String distance) { Assert.isTrue(!StringUtils.isEmpty(geoLocation), "geoLocation value must not be null"); - filterCriteria.add(new CriteriaEntry(OperationKey.WITHIN, new Object[]{geoLocation, distance})); + filterCriteria.add(new CriteriaEntry(OperationKey.WITHIN, new Object[] { geoLocation, distance })); return this; } /** * Creates new CriteriaEntry for {@code location GeoBox bounding box} * - * @param boundingBox {@link org.springframework.data.elasticsearch.core.geo.GeoBox} bounding box(left top corner + right bottom corner) + * @param boundingBox {@link org.springframework.data.elasticsearch.core.geo.GeoBox} bounding box(left top corner + + * right bottom corner) * @return Criteria the chaind criteria with the new 'boundingBox' criteria included. */ public Criteria boundedBy(GeoBox boundingBox) { Assert.notNull(boundingBox, "boundingBox value for boundedBy criteria must not be null"); - filterCriteria.add(new CriteriaEntry(OperationKey.BBOX, new Object[]{boundingBox})); + filterCriteria.add(new CriteriaEntry(OperationKey.BBOX, new Object[] { boundingBox })); return this; } /** * Creates new CriteriaEntry for {@code location Box bounding box} * - * @param boundingBox {@link org.springframework.data.elasticsearch.core.geo.GeoBox} bounding box(left top corner + right bottom corner) + * @param boundingBox {@link org.springframework.data.elasticsearch.core.geo.GeoBox} bounding box(left top corner + + * right bottom corner) * @return Criteria the chaind criteria with the new 'boundingBox' criteria included. */ public Criteria boundedBy(Box boundingBox) { Assert.notNull(boundingBox, "boundingBox value for boundedBy criteria must not be null"); - filterCriteria.add(new CriteriaEntry(OperationKey.BBOX, new Object[]{boundingBox.getFirst(), boundingBox.getSecond()})); + filterCriteria + .add(new CriteriaEntry(OperationKey.BBOX, new Object[] { boundingBox.getFirst(), boundingBox.getSecond() })); return this; } @@ -477,7 +469,7 @@ public class Criteria { public Criteria boundedBy(String topLeftGeohash, String bottomRightGeohash) { Assert.isTrue(!StringUtils.isEmpty(topLeftGeohash), "topLeftGeohash must not be empty"); Assert.isTrue(!StringUtils.isEmpty(bottomRightGeohash), "bottomRightGeohash must not be empty"); - filterCriteria.add(new CriteriaEntry(OperationKey.BBOX, new Object[]{topLeftGeohash, bottomRightGeohash})); + filterCriteria.add(new CriteriaEntry(OperationKey.BBOX, new Object[] { topLeftGeohash, bottomRightGeohash })); return this; } @@ -491,14 +483,15 @@ public class Criteria { public Criteria boundedBy(GeoPoint topLeftPoint, GeoPoint bottomRightPoint) { Assert.notNull(topLeftPoint, "topLeftPoint must not be null"); Assert.notNull(bottomRightPoint, "bottomRightPoint must not be null"); - filterCriteria.add(new CriteriaEntry(OperationKey.BBOX, new Object[]{topLeftPoint, bottomRightPoint})); + filterCriteria.add(new CriteriaEntry(OperationKey.BBOX, new Object[] { topLeftPoint, bottomRightPoint })); return this; } public Criteria boundedBy(Point topLeftPoint, Point bottomRightPoint) { Assert.notNull(topLeftPoint, "topLeftPoint must not be null"); Assert.notNull(bottomRightPoint, "bottomRightPoint must not be null"); - filterCriteria.add(new CriteriaEntry(OperationKey.BBOX, new Object[]{GeoPoint.fromPoint(topLeftPoint), GeoPoint.fromPoint(bottomRightPoint)})); + filterCriteria.add(new CriteriaEntry(OperationKey.BBOX, + new Object[] { GeoPoint.fromPoint(topLeftPoint), GeoPoint.fromPoint(bottomRightPoint) })); return this; } @@ -611,10 +604,7 @@ public class Criteria { @Override public String toString() { - return "CriteriaEntry{" + - "key=" + key + - ", value=" + value + - '}'; + return "CriteriaEntry{" + "key=" + key + ", value=" + value + '}'; } } } diff --git a/src/main/java/org/springframework/data/elasticsearch/repository/support/AbstractElasticsearchRepository.java b/src/main/java/org/springframework/data/elasticsearch/repository/support/AbstractElasticsearchRepository.java index 9cb0c1ab1..92be2f86d 100644 --- a/src/main/java/org/springframework/data/elasticsearch/repository/support/AbstractElasticsearchRepository.java +++ b/src/main/java/org/springframework/data/elasticsearch/repository/support/AbstractElasticsearchRepository.java @@ -155,7 +155,13 @@ public abstract class AbstractElasticsearchRepository implements Elastics Assert.notNull(ids, "ids can't be null."); - SearchQuery query = new NativeSearchQueryBuilder().withIds(stringIdsRepresentation(ids)).build(); + List stringIds = stringIdsRepresentation(ids); + + if (stringIds.isEmpty()) { + return Collections.emptyList(); + } + + SearchQuery query = new NativeSearchQueryBuilder().withIds(stringIds).build(); return elasticsearchOperations.multiGet(query, getEntityClass()); } diff --git a/src/test/java/org/springframework/data/elasticsearch/repository/query/keywords/QueryKeywordsTests.java b/src/test/java/org/springframework/data/elasticsearch/repository/query/keywords/QueryKeywordsTests.java index 5fff46d7c..8458a802e 100644 --- a/src/test/java/org/springframework/data/elasticsearch/repository/query/keywords/QueryKeywordsTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/repository/query/keywords/QueryKeywordsTests.java @@ -23,6 +23,7 @@ import lombok.Getter; import lombok.NoArgsConstructor; import lombok.Setter; +import java.util.ArrayList; import java.util.Arrays; import java.util.Date; import java.util.List; @@ -61,15 +62,15 @@ abstract class QueryKeywordsTests { IndexInitializer.init(elasticsearchTemplate, Product.class); - Product product1 = Product.builder().id("1").name("Sugar").text("Cane sugar").price(1.0f).available(false) + Product product1 = Product.builder().id("1").name("Sugar").text("Cane sugar").price(1.0f).available(false) .sortName("sort5").build(); - Product product2 = Product.builder().id("2").name("Sugar").text("Cane sugar").price(1.2f).available(true) + Product product2 = Product.builder().id("2").name("Sugar").text("Cane sugar").price(1.2f).available(true) .sortName("sort4").build(); - Product product3 = Product.builder().id("3").name("Sugar").text("Beet sugar").price(1.1f).available(true) + Product product3 = Product.builder().id("3").name("Sugar").text("Beet sugar").price(1.1f).available(true) .sortName("sort3").build(); - Product product4 = Product.builder().id("4").name("Salt").text("Rock salt").price(1.9f).available(true) + Product product4 = Product.builder().id("4").name("Salt").text("Rock salt").price(1.9f).available(true) .sortName("sort2").build(); - Product product5 = Product.builder().id("5").name("Salt").text("Sea salt").price(2.1f).available(false) + Product product5 = Product.builder().id("5").name("Salt").text("Sea salt").price(2.1f).available(false) .sortName("sort1").build(); repository.saveAll(Arrays.asList(product1, product2, product3, product4, product5)); @@ -165,7 +166,7 @@ abstract class QueryKeywordsTests { @Test // DATAES-615 public void shouldSupportSortOnStandardFieldWithCriteria() { - List sortedIds = repository.findAllByNameOrderByText("Salt").stream() // + List sortedIds = repository.findAllByNameOrderByText("Salt").stream() // .map(it -> it.id).collect(Collectors.toList()); assertThat(sortedIds).containsExactly("4", "5"); @@ -174,7 +175,7 @@ abstract class QueryKeywordsTests { @Test // DATAES-615 public void shouldSupportSortOnFieldWithCustomFieldNameWithCriteria() { - List sortedIds = repository.findAllByNameOrderBySortName("Sugar").stream() // + List sortedIds = repository.findAllByNameOrderBySortName("Sugar").stream() // .map(it -> it.id).collect(Collectors.toList()); assertThat(sortedIds).containsExactly("3", "2", "1"); @@ -182,7 +183,7 @@ abstract class QueryKeywordsTests { @Test // DATAES-615 public void shouldSupportSortOnStandardFieldWithoutCriteria() { - List sortedIds = repository.findAllByOrderByText().stream() // + List sortedIds = repository.findAllByOrderByText().stream() // .map(it -> it.text).collect(Collectors.toList()); assertThat(sortedIds).containsExactly("Beet sugar", "Cane sugar", "Cane sugar", "Rock salt", "Sea salt"); @@ -191,12 +192,28 @@ abstract class QueryKeywordsTests { @Test // DATAES-615 public void shouldSupportSortOnFieldWithCustomFieldNameWithoutCriteria() { - List sortedIds = repository.findAllByOrderBySortName().stream() // + List sortedIds = repository.findAllByOrderBySortName().stream() // .map(it -> it.id).collect(Collectors.toList()); assertThat(sortedIds).containsExactly("5", "4", "3", "2", "1"); } + @Test // DATAES-937 + public void shouldReturnEmptyListOnFindByIdWithEmptyInputList() { + + Iterable products = repository.findAllById(new ArrayList<>()); + + assertThat(products).isEmpty(); + } + + @Test // DATAES-937 + public void shouldReturnEmptyListOnDerivedMethodWithEmptyInputList() { + + Iterable products = repository.findAllByNameIn(new ArrayList<>()); + + assertThat(products).isEmpty(); + } + /** * @author Mohsin Husen * @author Artur Konczak @@ -279,6 +296,8 @@ abstract class QueryKeywordsTests { List findAllByOrderByText(); List findAllByOrderBySortName(); + + List findAllByNameIn(List names); } }