From 79fdc449b873b317cc6d9544285e870c11a4d240 Mon Sep 17 00:00:00 2001 From: Peter-Josef Meisch Date: Mon, 24 Aug 2020 07:02:43 +0200 Subject: [PATCH] DATAES-912 - Derived Query with "In" Keyword does not work on Text field. Original PR: #510 --- .../elasticsearch-repository-queries.adoc | 17 ++++-- .../core/CriteriaQueryProcessor.java | 50 ++++++++++++++--- .../core/ElasticsearchRestTemplate.java | 10 ++-- .../MappingElasticsearchConverter.java | 23 ++++++-- .../data/elasticsearch/core/query/Field.java | 14 +++++ .../elasticsearch/core/query/SimpleField.java | 16 +++++- .../core/ElasticsearchPartQueryTests.java | 48 +++++++++++------ .../CustomMethodRepositoryBaseTests.java | 54 +++++++++++++++++-- 8 files changed, 190 insertions(+), 42 deletions(-) diff --git a/src/main/asciidoc/reference/elasticsearch-repository-queries.adoc b/src/main/asciidoc/reference/elasticsearch-repository-queries.adoc index c17844fb3..7b35a79a0 100644 --- a/src/main/asciidoc/reference/elasticsearch-repository-queries.adoc +++ b/src/main/asciidoc/reference/elasticsearch-repository-queries.adoc @@ -48,7 +48,9 @@ A list of supported keywords for Elasticsearch is shown below. |=== | Keyword | Sample -| Elasticsearch Query String| `And` +| Elasticsearch Query String + +| `And` | `findByNameAndPrice` | `{ "query" : { "bool" : { @@ -201,7 +203,7 @@ A list of supported keywords for Elasticsearch is shown below. } }}` -| `In` +| `In` (when annotated as FieldType.Keyword) | `findByNameIn(Collectionnames)` | `{ "query" : { "bool" : { @@ -215,7 +217,12 @@ A list of supported keywords for Elasticsearch is shown below. } }}` -| `NotIn` + +| `In` +| `findByNameIn(Collectionnames)` +| `{ "query": {"bool": {"must": [{"query_string":{"query": "\"?\" \"?\"", "fields": ["name"]}}]}}}` + +| `NotIn` (when annotated as FieldType.Keyword) | `findByNameNotIn(Collectionnames)` | `{ "query" : { "bool" : { @@ -229,6 +236,10 @@ A list of supported keywords for Elasticsearch is shown below. } }}` +| `NotIn` +| `findByNameNotIn(Collectionnames)` +| `{"query": {"bool": {"must": [{"query_string": {"query": "NOT(\"?\" \"?\")", "fields": ["name"]}}]}}}` + | `Near` | `findByStoreNear` | `Not Supported Yet !` 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 f465f2201..958caddde 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/CriteriaQueryProcessor.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/CriteriaQueryProcessor.java @@ -26,7 +26,9 @@ import java.util.List; 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.annotations.FieldType; import org.springframework.data.elasticsearch.core.query.Criteria; +import org.springframework.data.elasticsearch.core.query.Field; import org.springframework.lang.Nullable; import org.springframework.util.Assert; @@ -128,23 +130,24 @@ class CriteriaQueryProcessor { @Nullable private QueryBuilder queryForEntries(Criteria criteria) { - if (criteria.getField() == null || criteria.getQueryCriteriaEntries().isEmpty()) + Field field = criteria.getField(); + + if (field == null || criteria.getQueryCriteriaEntries().isEmpty()) return null; - String fieldName = criteria.getField().getName(); - + String fieldName = field.getName(); Assert.notNull(fieldName, "Unknown field"); Iterator it = criteria.getQueryCriteriaEntries().iterator(); QueryBuilder query; if (criteria.getQueryCriteriaEntries().size() == 1) { - query = queryFor(it.next(), fieldName); + query = queryFor(it.next(), field); } else { query = boolQuery(); while (it.hasNext()) { Criteria.CriteriaEntry entry = it.next(); - ((BoolQueryBuilder) query).must(queryFor(entry, fieldName)); + ((BoolQueryBuilder) query).must(queryFor(entry, field)); } } @@ -153,7 +156,11 @@ class CriteriaQueryProcessor { } @Nullable - private QueryBuilder queryFor(Criteria.CriteriaEntry entry, String fieldName) { + private QueryBuilder queryFor(Criteria.CriteriaEntry entry, Field field) { + + String fieldName = field.getName(); + boolean isKeywordField = FieldType.Keyword == field.getFieldType(); + OperationKey key = entry.getKey(); if (key == OperationKey.EXISTS) { @@ -209,13 +216,21 @@ class CriteriaQueryProcessor { case IN: if (value instanceof Iterable) { Iterable iterable = (Iterable) value; - query = boolQuery().must(termsQuery(fieldName, toStringList(iterable))); + if (isKeywordField) { + query = boolQuery().must(termsQuery(fieldName, toStringList(iterable))); + } else { + query = queryStringQuery(orQueryString(iterable)).field(fieldName); + } } break; case NOT_IN: if (value instanceof Iterable) { Iterable iterable = (Iterable) value; - query = boolQuery().mustNot(termsQuery(fieldName, toStringList(iterable))); + if (isKeywordField) { + query = boolQuery().mustNot(termsQuery(fieldName, toStringList(iterable))); + } else { + query = queryStringQuery("NOT(" + orQueryString(iterable) + ')').field(fieldName); + } } break; } @@ -230,6 +245,25 @@ class CriteriaQueryProcessor { return list; } + 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(); + } + private void addBoost(@Nullable QueryBuilder query, float boost) { if (query == null || Float.isNaN(boost)) { diff --git a/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchRestTemplate.java b/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchRestTemplate.java index ebd98a497..cd3358761 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchRestTemplate.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/ElasticsearchRestTemplate.java @@ -92,8 +92,8 @@ public class ElasticsearchRestTemplate extends AbstractElasticsearchTemplate { private static final Logger LOGGER = LoggerFactory.getLogger(ElasticsearchRestTemplate.class); - private RestHighLevelClient client; - private ElasticsearchExceptionTranslator exceptionTranslator; + private final RestHighLevelClient client; + private final ElasticsearchExceptionTranslator exceptionTranslator; // region Initialization public ElasticsearchRestTemplate(RestHighLevelClient client) { @@ -241,11 +241,11 @@ public class ElasticsearchRestTemplate extends AbstractElasticsearchTemplate { IndexCoordinates index) { maybeCallbackBeforeConvertWithQueries(queries, index); BulkRequest bulkRequest = requestFactory.bulkRequest(queries, bulkOptions, index); - List indexedObjectInformations = checkForBulkOperationFailure( + List indexedObjectInformationList = checkForBulkOperationFailure( execute(client -> client.bulk(bulkRequest, RequestOptions.DEFAULT))); - updateIndexedObjectsWithQueries(queries, indexedObjectInformations); + updateIndexedObjectsWithQueries(queries, indexedObjectInformationList); maybeCallbackAfterSaveWithQueries(queries, index); - return indexedObjectInformations; + return indexedObjectInformationList; } // endregion diff --git a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java index deda4c3fe..5de116b78 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java @@ -50,6 +50,7 @@ import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersiste import org.springframework.data.elasticsearch.core.mapping.ElasticsearchPersistentPropertyConverter; import org.springframework.data.elasticsearch.core.query.Criteria; import org.springframework.data.elasticsearch.core.query.CriteriaQuery; +import org.springframework.data.elasticsearch.core.query.Field; import org.springframework.data.elasticsearch.core.query.SeqNoPrimaryTerm; import org.springframework.data.mapping.PersistentPropertyAccessor; import org.springframework.data.mapping.context.MappingContext; @@ -94,7 +95,7 @@ public class MappingElasticsearchConverter private final ElasticsearchTypeMapper typeMapper; - private ConcurrentHashMap propertyWarnings = new ConcurrentHashMap<>(); + private final ConcurrentHashMap propertyWarnings = new ConcurrentHashMap<>(); public MappingElasticsearchConverter( MappingContext, ElasticsearchPersistentProperty> mappingContext) { @@ -579,13 +580,13 @@ public class MappingElasticsearchConverter } if (property.isEntity() || !isSimpleType(value)) { - return writeEntity(value, property, typeHint); + return writeEntity(value, property); } return value; } - private Object writeEntity(Object value, ElasticsearchPersistentProperty property, TypeInformation typeHint) { + private Object writeEntity(Object value, ElasticsearchPersistentProperty property) { Document target = Document.create(); writeEntity(mappingContext.getRequiredPersistentEntity(value.getClass()), value, target, @@ -769,11 +770,17 @@ public class MappingElasticsearchConverter } private void updateCriteria(Criteria criteria, ElasticsearchPersistentEntity persistentEntity) { - String name = criteria.getField().getName(); + Field field = criteria.getField(); + + if (field == null) { + return; + } + + String name = field.getName(); ElasticsearchPersistentProperty property = persistentEntity.getPersistentProperty(name); if (property != null && property.getName().equals(name)) { - criteria.getField().setName(property.getFieldName()); + field.setName(property.getFieldName()); if (property.hasPropertyConverter()) { ElasticsearchPersistentPropertyConverter propertyConverter = property.getPropertyConverter(); @@ -789,6 +796,12 @@ public class MappingElasticsearchConverter } }); } + + org.springframework.data.elasticsearch.annotations.Field fieldAnnotation = property.findAnnotation(org.springframework.data.elasticsearch.annotations.Field.class); + + if (fieldAnnotation != null) { + field.setFieldType(fieldAnnotation.type()); + } } for (Criteria subCriteria : criteria.getSubCriteria()) { diff --git a/src/main/java/org/springframework/data/elasticsearch/core/query/Field.java b/src/main/java/org/springframework/data/elasticsearch/core/query/Field.java index aa80c12dc..92969a11d 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/query/Field.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/query/Field.java @@ -15,6 +15,9 @@ */ package org.springframework.data.elasticsearch.core.query; +import org.springframework.data.elasticsearch.annotations.FieldType; +import org.springframework.lang.Nullable; + /** * Defines a Field that can be used within a Criteria. * @@ -27,4 +30,15 @@ public interface Field { void setName(String name); String getName(); + + /** + * @param fieldType sets the field's type + */ + void setFieldType(FieldType fieldType); + + /** + * @return The annotated FieldType of the field + */ + @Nullable + FieldType getFieldType(); } diff --git a/src/main/java/org/springframework/data/elasticsearch/core/query/SimpleField.java b/src/main/java/org/springframework/data/elasticsearch/core/query/SimpleField.java index f9fed6a69..1091bdb78 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/query/SimpleField.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/query/SimpleField.java @@ -15,10 +15,12 @@ */ package org.springframework.data.elasticsearch.core.query; +import org.springframework.data.elasticsearch.annotations.FieldType; +import org.springframework.lang.Nullable; import org.springframework.util.Assert; /** - * The most trivial implementation of a Field. The {@link #name} is updateable, so it may be changed during query + * The most trivial implementation of a Field. The {@link #name} is updatable, so it may be changed during query * preparation by the {@link org.springframework.data.elasticsearch.core.convert.MappingElasticsearchConverter}. * * @author Rizwan Idrees @@ -28,6 +30,7 @@ import org.springframework.util.Assert; public class SimpleField implements Field { private String name; + @Nullable private FieldType fieldType; public SimpleField(String name) { @@ -49,6 +52,17 @@ public class SimpleField implements Field { return name; } + @Override + public void setFieldType(FieldType fieldType) { + this.fieldType = fieldType; + } + + @Nullable + @Override + public FieldType getFieldType() { + return fieldType; + } + @Override public String toString() { return getName(); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchPartQueryTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchPartQueryTests.java index 70691f434..7fde9d1fc 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchPartQueryTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchPartQueryTests.java @@ -197,14 +197,22 @@ class ElasticsearchPartQueryTests { String query = getQueryBuilder(methodName, parameterClasses, parameters); - String expected = "{\"query\": {" + // - " \"bool\" : {" + // - " \"must\" : [" + // - " {\"bool\" : {\"must\" : [{\"terms\" : {\"name\" : [\"" + names.get(0) + "\", \"" + names.get(1) - + "\"]}}]}}" + // - " ]" + // - " }" + // - "}}"; // + String expected = "{\n" + // + " \"query\": {\n" + // + " \"bool\": {\n" + // + " \"must\": [\n" + // + " {\n" + // + " \"query_string\": {\n" + // + " \"query\": \"\\\"Title\\\" \\\"Title2\\\"\",\n" + // + " \"fields\": [\n" + // + " \"name^1.0\"\n" + // + " ]\n" + // + " }\n" + // + " }\n" + // + " ]\n" + // + " }\n" + // + " }\n" + // + "}\n"; // assertEquals(expected, query, false); } @@ -220,14 +228,22 @@ class ElasticsearchPartQueryTests { String query = getQueryBuilder(methodName, parameterClasses, parameters); - String expected = "{\"query\": {" + // - " \"bool\" : {" + // - " \"must\" : [" + // - " {\"bool\" : {\"must_not\" : [{\"terms\" : {\"name\" : [\"" + names.get(0) + "\", \"" + names.get(1) - + "\"]}}]}}" + // - " ]" + // - " }" + // - "}}"; // + String expected = "{\n" + // + " \"query\": {\n" + // + " \"bool\": {\n" + // + " \"must\": [\n" + // + " {\n" + // + " \"query_string\": {\n" + // + " \"query\": \"NOT(\\\"Title\\\" \\\"Title2\\\")\",\n" + // + " \"fields\": [\n" + // + " \"name^1.0\"\n" + // + " ]\n" + // + " }\n" + // + " }\n" + // + " ]\n" + // + " }\n" + // + " }\n" + // + "}\n"; // assertEquals(expected, query, false); } diff --git a/src/test/java/org/springframework/data/elasticsearch/repositories/custommethod/CustomMethodRepositoryBaseTests.java b/src/test/java/org/springframework/data/elasticsearch/repositories/custommethod/CustomMethodRepositoryBaseTests.java index d2c2b3f71..780626d78 100644 --- a/src/test/java/org/springframework/data/elasticsearch/repositories/custommethod/CustomMethodRepositoryBaseTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/repositories/custommethod/CustomMethodRepositoryBaseTests.java @@ -360,7 +360,7 @@ public abstract class CustomMethodRepositoryBaseTests { } @Test // DATAES-647 - public void shouldHandleManyValuesQueryingIn() { + public void shouldHandleManyKeywordValuesQueryingIn() { // given String documentId1 = nextIdAsString(); @@ -378,7 +378,8 @@ public abstract class CustomMethodRepositoryBaseTests { List keywords = new ArrayList<>(); keywords.add("foo"); - for (int i = 0; i < 1025; i++) { + // limit for normal query clauses is 1024, for keywords we change to terms queries + for (int i = 0; i < 1200; i++) { keywords.add(nextIdAsString()); } @@ -391,7 +392,7 @@ public abstract class CustomMethodRepositoryBaseTests { } @Test // DATAES-647 - public void shouldHandleManyValuesQueryingNotIn() { + public void shouldHandleManyKeywordValuesQueryingNotIn() { // given String documentId1 = nextIdAsString(); @@ -409,7 +410,8 @@ public abstract class CustomMethodRepositoryBaseTests { List keywords = new ArrayList<>(); keywords.add("foo"); - for (int i = 0; i < 1025; i++) { + // limit for normal query clauses is 1024, for keywords we change to terms queries + for (int i = 0; i < 1200; i++) { keywords.add(nextIdAsString()); } @@ -421,6 +423,46 @@ public abstract class CustomMethodRepositoryBaseTests { assertThat(list.get(0).getId()).isEqualTo(documentId2); } + @Test // DATAES-912 + void shouldHandleTextFieldQueryingIn() { + String documentId1 = nextIdAsString(); + SampleEntity sampleEntity1 = new SampleEntity(); + sampleEntity1.setId(documentId1); + sampleEntity1.setMessage("foo"); + repository.save(sampleEntity1); + + String documentId2 = nextIdAsString(); + SampleEntity sampleEntity2 = new SampleEntity(); + sampleEntity2.setId(documentId2); + sampleEntity2.setMessage("bar"); + repository.save(sampleEntity2); + + List list = repository.findByMessageIn(Arrays.asList("Foo", "Bar")); + + assertThat(list).hasSize(2); + assertThat(list.stream().map(SampleEntity::getId)).containsExactlyInAnyOrder(documentId1, documentId2); + } + + @Test // DATAES-912 + void shouldHandleTextFieldQueryingNotIn() { + String documentId1 = nextIdAsString(); + SampleEntity sampleEntity1 = new SampleEntity(); + sampleEntity1.setId(documentId1); + sampleEntity1.setMessage("foo"); + repository.save(sampleEntity1); + + String documentId2 = nextIdAsString(); + SampleEntity sampleEntity2 = new SampleEntity(); + sampleEntity2.setId(documentId2); + sampleEntity2.setMessage("bar"); + repository.save(sampleEntity2); + + List list = repository.findByMessageNotIn(Arrays.asList("Boo", "Bar")); + + assertThat(list).hasSize(1); + assertThat(list.get(0).getId()).isEqualTo(documentId1); + } + @Test public void shouldExecuteCustomMethodForTrue() { @@ -1622,6 +1664,10 @@ public abstract class CustomMethodRepositoryBaseTests { List findByKeywordNotIn(List keywords); + List findByMessageIn(List keywords); + + List findByMessageNotIn(List keywords); + Page findByIdNotIn(List ids, Pageable pageable); Page findByAvailableTrue(Pageable pageable);