source_filter and fields fixes.

Original Pull Request #1819 
Closes #1817

Author: sothawo

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`.

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

src/main/java/org/springframework/data/elasticsearch/core/RequestFactory.java

@@ -681,6 +681,7 @@ private List<MultiGetRequest.Item> 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<String> 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

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<String> getFields();
 

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);
 

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<SampleEntity> 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<String, Object> 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();

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<Entity> 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<MultiGetItem<Entity>> entities = operations.multiGet(query, Entity.class);