From 24e4e46c2ab6f69905f8cf73ec125975c372ff0d Mon Sep 17 00:00:00 2001 From: Peter-Josef Meisch Date: Tue, 18 May 2021 21:44:54 +0200 Subject: [PATCH] source_filter and fields fixes --- ...elasticsearch-migration-guide-4.2-4.3.adoc | 15 ++++++ .../asciidoc/reference/migration-guides.adoc | 2 + .../elasticsearch/core/RequestFactory.java | 46 ++++++++----------- .../data/elasticsearch/core/query/Query.java | 2 +- .../core/CriteriaQueryMappingUnitTests.java | 4 +- .../core/ElasticsearchTemplateTests.java | 33 +------------ .../core/SourceFilterIntegrationTests.java | 24 +++------- 7 files changed, 48 insertions(+), 78 deletions(-) create mode 100644 src/main/asciidoc/reference/elasticsearch-migration-guide-4.2-4.3.adoc diff --git a/src/main/asciidoc/reference/elasticsearch-migration-guide-4.2-4.3.adoc b/src/main/asciidoc/reference/elasticsearch-migration-guide-4.2-4.3.adoc new file mode 100644 index 000000000..ff3467ac5 --- /dev/null +++ b/src/main/asciidoc/reference/elasticsearch-migration-guide-4.2-4.3.adoc @@ -0,0 +1,15 @@ +[[elasticsearch-migration-guide-4.2-4.3]] += Upgrading from 4.2.x to 4.3.x + +This section describes breaking changes from version 4.2.x to 4.3.x and how removed features can be replaced by new introduced features. + +[[elasticsearch-migration-guide-4.2-4.3.deprecations]] +== Deprecations + +[[elasticsearch-migration-guide-4.2-4.3.breaking-changes]] +== Breaking Changes + +=== Handling of field and sourceFilter properties of Query + +Up to version 4.2 the `fields` property of a `Query` was interpreted and added to the include list of the `sourceFilter`. This was not correct, as these are different things for Elasticsearch. This has been corrected. As a consequence code might not work anymore that relies on using `fields` to specify which fields should be returned from the document's +`_source' and should be changed to use the `sourceFilter`. diff --git a/src/main/asciidoc/reference/migration-guides.adoc b/src/main/asciidoc/reference/migration-guides.adoc index ba5d1e0f7..1fe24a41a 100644 --- a/src/main/asciidoc/reference/migration-guides.adoc +++ b/src/main/asciidoc/reference/migration-guides.adoc @@ -8,4 +8,6 @@ include::elasticsearch-migration-guide-3.2-4.0.adoc[] include::elasticsearch-migration-guide-4.0-4.1.adoc[] include::elasticsearch-migration-guide-4.1-4.2.adoc[] + +include::elasticsearch-migration-guide-4.2-4.3.adoc[] :leveloffset: -1 diff --git a/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java b/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java index 20e1d4a7b..4451ae9c0 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java @@ -681,6 +681,7 @@ private List getMultiRequestItems(Query searchQuery, Class item = item.routing(searchQuery.getRoute()); } + // note: multiGet does not have fields, need to set sourceContext to filter if (fetchSourceContext != null) { item.fetchSourceContext(fetchSourceContext); } @@ -929,11 +930,6 @@ private SearchRequest prepareSearchRequest(Query query, @Nullable Class clazz sourceBuilder.seqNoAndPrimaryTerm(true); } - if (query.getSourceFilter() != null) { - SourceFilter sourceFilter = query.getSourceFilter(); - sourceBuilder.fetchSource(sourceFilter.getIncludes(), sourceFilter.getExcludes()); - } - if (query.getPageable().isPaged()) { sourceBuilder.from((int) query.getPageable().getOffset()); sourceBuilder.size(query.getPageable().getPageSize()); @@ -942,8 +938,14 @@ private SearchRequest prepareSearchRequest(Query query, @Nullable Class clazz sourceBuilder.size(INDEX_MAX_RESULT_WINDOW); } + if (query.getSourceFilter() != null) { + sourceBuilder.fetchSource(getFetchSourceContext(query)); + SourceFilter sourceFilter = query.getSourceFilter(); + sourceBuilder.fetchSource(sourceFilter.getIncludes(), sourceFilter.getExcludes()); + } + if (!query.getFields().isEmpty()) { - sourceBuilder.fetchSource(query.getFields().toArray(new String[0]), null); + query.getFields().forEach(sourceBuilder::fetchField); } if (query.getIndicesOptions() != null) { @@ -1023,11 +1025,6 @@ private SearchRequestBuilder prepareSearchRequestBuilder(Query query, Client cli searchRequestBuilder.seqNoAndPrimaryTerm(true); } - if (query.getSourceFilter() != null) { - SourceFilter sourceFilter = query.getSourceFilter(); - searchRequestBuilder.setFetchSource(sourceFilter.getIncludes(), sourceFilter.getExcludes()); - } - if (query.getPageable().isPaged()) { searchRequestBuilder.setFrom((int) query.getPageable().getOffset()); searchRequestBuilder.setSize(query.getPageable().getPageSize()); @@ -1036,8 +1033,13 @@ private SearchRequestBuilder prepareSearchRequestBuilder(Query query, Client cli searchRequestBuilder.setSize(INDEX_MAX_RESULT_WINDOW); } + if (query.getSourceFilter() != null) { + SourceFilter sourceFilter = query.getSourceFilter(); + searchRequestBuilder.setFetchSource(sourceFilter.getIncludes(), sourceFilter.getExcludes()); + } + if (!query.getFields().isEmpty()) { - searchRequestBuilder.setFetchSource(query.getFields().toArray(new String[0]), null); + query.getFields().forEach(searchRequestBuilder::addFetchField); } if (query.getIndicesOptions() != null) { @@ -1599,24 +1601,16 @@ public static WriteRequest.RefreshPolicy toElasticsearchRefreshPolicy(RefreshPol } } + @Nullable private FetchSourceContext getFetchSourceContext(Query searchQuery) { - FetchSourceContext fetchSourceContext = null; - SourceFilter sourceFilter = searchQuery.getSourceFilter(); - if (!isEmpty(searchQuery.getFields())) { - if (sourceFilter == null) { - sourceFilter = new FetchSourceFilter(toArray(searchQuery.getFields()), null); - } else { - ArrayList arrayList = new ArrayList<>(); - Collections.addAll(arrayList, sourceFilter.getIncludes()); - sourceFilter = new FetchSourceFilter(toArray(arrayList), null); - } + SourceFilter sourceFilter = searchQuery.getSourceFilter(); - fetchSourceContext = new FetchSourceContext(true, sourceFilter.getIncludes(), sourceFilter.getExcludes()); - } else if (sourceFilter != null) { - fetchSourceContext = new FetchSourceContext(true, sourceFilter.getIncludes(), sourceFilter.getExcludes()); + if (sourceFilter != null) { + return new FetchSourceContext(true, sourceFilter.getIncludes(), sourceFilter.getExcludes()); } - return fetchSourceContext; + + return null; } // endregion diff --git a/src/main/java/org/springframework/data/elasticsearch/core/query/Query.java b/src/main/java/org/springframework/data/elasticsearch/core/query/Query.java index 1f255ffb1..ae96d335e 100644 --- a/src/main/java/org/springframework/data/elasticsearch/core/query/Query.java +++ b/src/main/java/org/springframework/data/elasticsearch/core/query/Query.java @@ -99,7 +99,7 @@ static Query findAll() { /** * Get fields to be returned as part of search request * - * @return + * @return maybe empty, never null */ List getFields(); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/CriteriaQueryMappingUnitTests.java b/src/test/java/org/springframework/data/elasticsearch/core/CriteriaQueryMappingUnitTests.java index e27eedead..294e99866 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/CriteriaQueryMappingUnitTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/CriteriaQueryMappingUnitTests.java @@ -44,7 +44,7 @@ import org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchMappingContext; import org.springframework.data.elasticsearch.core.query.Criteria; import org.springframework.data.elasticsearch.core.query.CriteriaQuery; -import org.springframework.data.elasticsearch.core.query.FetchSourceFilter; +import org.springframework.data.elasticsearch.core.query.FetchSourceFilterBuilder; import org.springframework.data.elasticsearch.core.query.Query; import org.springframework.data.elasticsearch.core.query.SourceFilter; import org.springframework.lang.Nullable; @@ -430,7 +430,7 @@ void shouldMapNamesInSourceFieldsAndSourceFilters() { Query query = Query.findAll(); // Note: we don't care if these filters make sense here, this test is only about name mapping query.addFields("firstName", "lastName"); - query.addSourceFilter(new FetchSourceFilter(new String[] { "firstName" }, new String[] { "lastName" })); + query.addSourceFilter(new FetchSourceFilterBuilder().withIncludes("firstName").withExcludes("lastName").build()); mappingElasticsearchConverter.updateQuery(query, Person.class); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java index 902bd90d6..1e9b4a27a 100755 --- a/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchTemplateTests.java @@ -994,35 +994,6 @@ public void shouldDeleteGivenCriteriaQuery() { assertThat(sampleEntities).isEmpty(); } - @Test - public void shouldReturnSpecifiedFields() { - - // given - String documentId = nextIdAsString(); - String message = "some test message"; - String type = "some type"; - SampleEntity sampleEntity = SampleEntity.builder().id(documentId).message(message).type(type) - .version(System.currentTimeMillis()).location(new GeoPoint(1.2, 3.4)).build(); - - IndexQuery indexQuery = getIndexQuery(sampleEntity); - - operations.index(indexQuery, index); - indexOperations.refresh(); - NativeSearchQuery searchQuery = new NativeSearchQueryBuilder().withQuery(matchAllQuery()).withFields("message") - .build(); - - // when - SearchHits searchHits = operations.search(searchQuery, SampleEntity.class, index); - - // then - assertThat(searchHits).isNotNull(); - assertThat(searchHits.getTotalHits()).isEqualTo(1); - final SampleEntity actual = searchHits.getSearchHit(0).getContent(); - assertThat(actual.message).isEqualTo(message); - assertThat(actual.getType()).isNull(); - assertThat(actual.getLocation()).isNull(); - } - @Test public void shouldReturnFieldsBasedOnSourceFilter() { @@ -2666,7 +2637,7 @@ public void shouldRespectSourceFilterWithScanAndScrollForGivenSearchQuery() { indexOperations.refresh(); // then - SourceFilter sourceFilter = new FetchSourceFilter(new String[] { "id" }, new String[] {}); + SourceFilter sourceFilter = new FetchSourceFilterBuilder().withIncludes("id").build(); NativeSearchQuery searchQuery = new NativeSearchQueryBuilder().withQuery(matchAllQuery()) .withPageable(PageRequest.of(0, 10)).withSourceFilter(sourceFilter).build(); @@ -3631,7 +3602,7 @@ void shouldSetScriptedFieldsOnImmutableObjects() { Map params = new HashMap<>(); params.put("factor", 2); NativeSearchQuery searchQuery = new NativeSearchQueryBuilder().withQuery(matchAllQuery()) - .withSourceFilter(new FetchSourceFilter(new String[] { "*" }, new String[] {})) + .withSourceFilter(new FetchSourceFilterBuilder().withIncludes("*").build()) .withScriptField(new ScriptField("scriptedRate", new Script(ScriptType.INLINE, "expression", "doc['rate'] * factor", params))) .build(); diff --git a/src/test/java/org/springframework/data/elasticsearch/core/SourceFilterIntegrationTests.java b/src/test/java/org/springframework/data/elasticsearch/core/SourceFilterIntegrationTests.java index 0bd7f4a18..762ce9deb 100644 --- a/src/test/java/org/springframework/data/elasticsearch/core/SourceFilterIntegrationTests.java +++ b/src/test/java/org/springframework/data/elasticsearch/core/SourceFilterIntegrationTests.java @@ -29,6 +29,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.core.query.FetchSourceFilterBuilder; import org.springframework.data.elasticsearch.core.query.NativeSearchQueryBuilder; import org.springframework.data.elasticsearch.core.query.Query; import org.springframework.data.elasticsearch.core.query.SourceFilter; @@ -66,28 +67,15 @@ void tearDown() { indexOps.delete(); } - @Test // #1659 - @DisplayName("should only return requested fields on search") - void shouldOnlyReturnRequestedFieldsOnSearch() { - - Query query = Query.findAll(); - query.addFields("field2"); - - SearchHits searchHits = operations.search(query, Entity.class); - - assertThat(searchHits).hasSize(1); - Entity entity = searchHits.getSearchHit(0).getContent(); - assertThat(entity.getField1()).isNull(); - assertThat(entity.getField2()).isEqualTo("two"); - assertThat(entity.getField3()).isNull(); - } - @Test // #1659, #1678 @DisplayName("should only return requested fields on multiget") void shouldOnlyReturnRequestedFieldsOnGMultiGet() { - Query query = new NativeSearchQueryBuilder().withIds(Collections.singleton("42")).build(); - query.addFields("field2"); + // multiget has no fields, need sourcefilter here + Query query = new NativeSearchQueryBuilder() // + .withIds(Collections.singleton("42")) // + .withSourceFilter(new FetchSourceFilterBuilder().withIncludes("field2").build()) // + .build(); // List> entities = operations.multiGet(query, Entity.class);