DATAES630 - Remove GetResultMapper and friends from core package. #331

sothawo · 2019-10-13T16:19:52Z

This is a draft PR not ready to be merged.

@mp911de I have moved the ResultMapper stuff into MappingElasticsearchConverter and cleaned up some stuff.
Can you check if this what you thought about the rewriting of the mapping? There still is quite some cleanup to be done, refactoring, poilshing etc, but I'd like to have your feedback before going on.

mp911de

I very much like the direction where the PR is heading to. I added a few comments where I spotted things we need to address.

I'm also wondering regarding the current setup for search result mapping. @Score is a read-only feature and @ScriptedField seems also only useful during reads.

What if we consider in a future version projection of search results in the sense that query(…) methods are able to return an interface and domain objects are no longer required to use @Score.

Something along the lines of:

interface PersonSerarchHit {
    String getFirstName(); // maps to firstName field
    @Score
    float getScore(); // maps to synthetic score field
}

List<PersonSerarchHit> hits = template.query(query, Person.class, PersonSerarchHit.class);

@ScriptedField("foo") seems to correspond with @Value("#root.foo") so we might want to deprecate @ScriptedField once we can provide proper SpEL support.

mp911de · 2019-10-15T11:34:54Z

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


 	private static final Logger LOGGER = LoggerFactory.getLogger(AbstractElasticsearchTemplate.class);

-	protected ElasticsearchConverter elasticsearchConverter;
+	protected MappingElasticsearchConverter elasticsearchConverter;


Let's stick with ElasticSearchConverter (the interface) here and gradually explore which functionality we require that we might need to add to the converter interface.

mp911de · 2019-10-15T11:36:50Z

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


-	protected String buildMapping(Class<?> clazz) {
+    protected MappingElasticsearchConverter createElasticsearchConverter() {
+        MappingElasticsearchConverter mappingElasticsearchConverter = new MappingElasticsearchConverter(new SimpleElasticsearchMappingContext());


We probably want to hold on to a reference of the mapping context ( MappingContext<? extends ElasticsearchPersistentEntity<?>, ElasticsearchPersistentProperty> mappingContext) in the instance.

keep the reference in the AbstractElasticsearchTemplate?

In the *Template classes we can laways get the MappingContext from the ElasticsearchConverterif needed

mp911de · 2019-10-15T11:44:17Z

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

@@ -1440,7 +1410,7 @@ private IndexRequest prepareIndex(IndexQuery query) {
 				} else {
 					indexRequest = new IndexRequest(indexName, type);
 				}
-				indexRequest.source(resultsMapper.getEntityMapper().mapToString(query.getObject()),
+				indexRequest.source(elasticsearchConverter.mapToString(query.getObject()),


Related: Do we already have a ticket to get rid of the mapToString(…) method? I was wondering whether we can reuse our Document API here.

removed it.

mp911de · 2019-10-15T11:45:59Z

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

@@ -36,11 +35,10 @@
 * @author Artur Konczak
 * @author Christoph Strobl
 * @author Mark Paluch
+ * @author Peter-Josef Meisch
 */
 public interface ResultsMapper extends SearchResultMapper, GetResultMapper, MultiGetResultMapper {


Can we get rid of this type and all …ResultMapper interfaces?

Did this. There are tests in ElasticsearchTemplateTests which rely on custom result mappers, especially checking the highlight result or request. I uncommented these so the builds don't fail, but added todos to them, because they have to be checked.

mp911de · 2019-10-15T11:51:55Z

.../java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java

@@ -649,7 +674,134 @@ private boolean isSimpleType(Class<?> type) {
 		return conversions.isSimpleType(type);
 	}

-	// --> OHTER STUFF
+	@Override
+	public <T> AggregatedPage<T> mapResults(SearchResponse response, Class<T> clazz, Pageable pageable) {


SearchResponse should be ideally wrapped into Document (SearchDocument). This functionality pulls in ES-specific API into the conversion layer. I'd suggest handling SearchDocument here only in an EntityReader fashion. Construction of the Page object should happen ideally in a utility that is closer to the actual template.

In several stores, we don't even provide paging on the Template API level, only through repositories.

See for reference:

EntityReader use in MongoDB

mp911de · 2019-10-15T11:52:43Z

.../java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java

+	private <T> void populateScriptFields(T result, SearchHit hit) {
+		if (hit.getFields() != null && !hit.getFields().isEmpty() && result != null) {
+			for (java.lang.reflect.Field field : result.getClass().getDeclaredFields()) {
+				ScriptedField scriptedField = field.getAnnotation(ScriptedField.class);


This is a candidate for cleanup!

this will be rewritten once we have the SearchHit<T> in place, the scripted fields should go in there

mp911de · 2019-10-15T11:54:08Z

.../java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java

+	}
+
+	@Override
+	public <T> T mapResult(GetResponse response, Class<T> clazz) {


This method should also accept only Document.

setPersistentEntityId(result, response.getId(), clazz); setPersistentEntityVersion(result, response.getVersion(), clazz);

should ideally reside in readObject(…)

mp911de · 2019-10-15T11:55:30Z

.../java/org/springframework/data/elasticsearch/core/convert/MappingElasticsearchConverter.java

+		return list;
+	}
+
+	private <T> void setPersistentEntityId(T result, String id, Class<T> clazz) {


These methods are easier to implement in the context of readEntity and ElasticsearchPersistentEntity since we already the appropriate context there.

sothawo · 2019-10-15T19:42:59Z

I now cleaned up the ResultMappers, the next thing will be cleaning up the whole bunch of map methods in the MappingElasticsearchConverter.
Lets see how much more we want to make in this ticket, because I'd like to get this restructuring of the interfaces into MappingElasticsearchConverter finished pretty soon to prevent merge conflicts or heavy merging with other PRs that work with the *Template classes. We should perhaps for the other findings create follow-up tickets.

sothawo · 2019-10-16T04:15:54Z

Another thought about what you wrote about returning the additional info for search results like the score or scripted fields- there is also the highlight info -: Couldn't we introduce a SearchResult<Entity> interface/class that wraps the currently returned entity and additionally has fields for score, highlight etc? Like the SearchDocument? The methods of ElasticsearchOperations then would return these instead of the entity itself. But how can this be handled in repositories? Should they then as well return these objects?

mp911de · 2019-10-16T07:01:48Z

No need to tackle the annotation-based aspects here, it was something that I noticed during the review. We can keep the implementations of the mappers for now in the converter, but we should address these.

Regarding the search results, we have a similar setup with GeoResults in other places. GeoResults is basically an Iterable<GeoResult<T>> where auxiliary properties are held that are returned as part of the query. I didn't think about such an arrangement before and now that you've mentioned it, it makes absolute sense to introduce SearchResults<T> that is Iterable<SearchResult<T>>.

For repositories, we can be more lenient about the API: Users can either declare a query method returning List<T> or SearchResults<T> if they require additional search-related details.

sothawo · 2019-10-20T20:39:45Z

I removed all the elasticsearch stuff from the MappingElasticsearchConverter, this is handled in the DocumentAdapters now (which I moved to a separate package). We no have the Document stuff dealing with ES, and Converter only deals with converting between entity and Document.

In the Converter I renamed the map...methods to have the target type in the name like mapToDocument or mapToEntity. I find that clearer especially when for example mapping a Documentto an entity or a SearchDocumentto a SearchResult- but might conflict with naming in Converters in other spring data modules.

I added a first small test implementation of SearchResult<T> and SearchResults<T> along with a fooSearch method in ElasticsearchOperations and in the template implementations so you might have a look what I think this could be.

We could move all the stuff like fields that are not in the entity (scripted), score, highlight and whatever else is coming from Elasticsearch into the SearchDocument and from there into the SearchResult<T> enabling the client to have all this information.
Introducing this will change almost all methods in the ElasticsearchOperations interface, but I think that will be the only way to get all this additional information back to the user.

Two questions about the SearchResults<T>which I defined as Iterable<SearchResult<T>>:

how do I create a concrete List that can be cast to this?
won't returning this conflict with the method of Iterable<T> search(QueryBuilder query); in ElasticsearchRepository?

sothawo · 2019-10-20T20:59:28Z

One more thought: I introduced SearchDocumentResponse to wrap the SearchDocuments along with information about the search itself (totalHits, maxScore etc). Might be worth a thought to put something along the SearchHits to be able to return this as well.

mp911de · 2019-10-21T07:56:36Z

In the Converter I renamed the map...methods to have the target type in the name like mapToDocument or mapToEntity. I find that clearer especially when for example mapping a Documentto an entity or a SearchDocumentto a SearchResult- but might conflict with naming in Converters in other spring data modules.

Spring Data Commons ships with EntityReader and EntityWriter interfaces to reflect the direction how entities are read and written. Let's see that we can use these at some point in time.

Re: SearchResult: Let's move that effort in the context of a separate ticket.

won't returning this conflict with the method of Iterable<T> search(QueryBuilder query); in ElasticsearchRepository?

We should ask rather whether that method should be exposed on the repository in the first place.
I'd expect mostly that application code defines methods returning SearchResult<T> with the appropriate method signature. The repository implementation needs to check for the return type and return the appropriate result type.

sothawo · 2019-10-21T19:24:49Z

I removed the tests that were using a custom EntityMapper to extract the highlights; that will be addressed in a different ticket for the SearchHit<T> class. I further removed from ElasticsearchOperations the methods taking an extra ElasticsearchConverter parameter as they are not needed anymore.
So this should be ready for a PR

mp911de

Great work, looks good to me and also this PR is sort of overripe (überreif). Sometimes changes get to the point where looking over the change makes it typically worse so it makes sense to wrap up this ticket and continue with items that popped out of this effort.

mp911de · 2019-10-25T07:41:33Z

src/main/java/org/springframework/data/elasticsearch/core/convert/ElasticsearchConverter.java

+	 * Get the configured {@link ProjectionFactory}. <br />
+	 * <strong>NOTE</strong> Should be overwritten in implementation to make use of the type cache.
+	 *
+	 * @since 3.2


sothawo requested a review from mp911de October 13, 2019 16:19

mp911de reviewed Oct 15, 2019

View reviewed changes

sothawo force-pushed the DATAES-630_-_Remove_GetResultMapper_and_friends_from_core_package branch 2 times, most recently from 8c01166 to 5f8aad1 Compare October 15, 2019 19:36

sothawo force-pushed the DATAES-630_-_Remove_GetResultMapper_and_friends_from_core_package branch from 5f8aad1 to f2d1cbb Compare October 16, 2019 04:43

sothawo force-pushed the DATAES-630_-_Remove_GetResultMapper_and_friends_from_core_package branch 3 times, most recently from 165e3f1 to 468af72 Compare October 20, 2019 18:38

sothawo force-pushed the DATAES-630_-_Remove_GetResultMapper_and_friends_from_core_package branch 3 times, most recently from 2bc27f6 to 4ce10d9 Compare October 21, 2019 19:19

sothawo marked this pull request as ready for review October 21, 2019 19:24

sothawo requested a review from mp911de October 21, 2019 19:25

mp911de approved these changes Oct 25, 2019

View reviewed changes

DATAES-630 - Remove GetResultMapper and friends from core package.

5ac7efe

sothawo force-pushed the DATAES-630_-_Remove_GetResultMapper_and_friends_from_core_package branch from 4ce10d9 to 5ac7efe Compare October 25, 2019 19:50

sothawo merged commit 4e7f1cc into spring-projects:master Oct 25, 2019

sothawo deleted the DATAES-630_-_Remove_GetResultMapper_and_friends_from_core_package branch October 25, 2019 20:06

spring-projects-issues mentioned this pull request Dec 31, 2020

Remove GetResultMapper and friends from core package [DATAES-630] #1203

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DATAES630 - Remove GetResultMapper and friends from core package. #331

DATAES630 - Remove GetResultMapper and friends from core package. #331

sothawo commented Oct 13, 2019

mp911de left a comment

mp911de Oct 15, 2019

sothawo Oct 20, 2019

mp911de Oct 15, 2019

sothawo Oct 20, 2019

sothawo Oct 21, 2019

mp911de Oct 15, 2019

sothawo Oct 20, 2019

mp911de Oct 15, 2019

sothawo Oct 15, 2019

mp911de Oct 15, 2019

mp911de Oct 15, 2019

sothawo Oct 21, 2019

mp911de Oct 15, 2019

mp911de Oct 15, 2019

sothawo commented Oct 15, 2019 •

edited

Loading

sothawo commented Oct 16, 2019 •

edited

Loading

mp911de commented Oct 16, 2019

sothawo commented Oct 20, 2019

sothawo commented Oct 20, 2019 •

edited

Loading

mp911de commented Oct 21, 2019

sothawo commented Oct 21, 2019

mp911de left a comment

mp911de Oct 25, 2019

DATAES630 - Remove GetResultMapper and friends from core package. #331

DATAES630 - Remove GetResultMapper and friends from core package. #331

Conversation

sothawo commented Oct 13, 2019

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sothawo commented Oct 15, 2019 • edited Loading

sothawo commented Oct 16, 2019 • edited Loading

mp911de commented Oct 16, 2019

sothawo commented Oct 20, 2019

sothawo commented Oct 20, 2019 • edited Loading

mp911de commented Oct 21, 2019

sothawo commented Oct 21, 2019

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sothawo commented Oct 15, 2019 •

edited

Loading

sothawo commented Oct 16, 2019 •

edited

Loading

sothawo commented Oct 20, 2019 •

edited

Loading