Dataes 631 consolidate query objects #340

sothawo · 2019-11-02T21:25:15Z

Draft PR.

mp911de

The PR isn't addressing shortcomings of SearchQuery and its coupling to the driver types. Also, we miss the mapping and conversion of values in CriteriaQuery. A query mapper should apply mapping of criteria expression (property/property path and value) against the domain model. A transition of the final, mapped criteria should happen in a different place.

Looking at SearchQuery I strongly feel we should remove that interface entirely and keep NativeSearchQuery as NativeSearchQuery honestly expresses what it is.

On Query, we should remove the generics at setPageable and addSort. A revision of Query should be done in its own ticket, but that is something I spotted while going over Query that method names are not consistent, and in some places, we offer a fluent API possibility, and in others, we don't.

I would suggest doing the following in the scope of this ticket:

Remove SearchQuery interface
Move the code from the created query mappers into a SearchRequestFactory that takes all sorts of queries and translates these into a SearchRequest.
Optionally: A Query mapper that takes a CriteriaQuery and maps and converts values against the domain model (map property names to field names, apply value conversion by considering CustomConversions)

mp911de · 2019-11-08T10:09:23Z

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

@@ -339,34 +330,20 @@
 	 * @param clazz
 	 * @return
 	 */
-	<T> long count(CriteriaQuery query, Class<T> clazz);
+	<T> long count(Query query, @Nullable Class<T> clazz);


Query currently carries types and indices. Typically, we keep these table-like coordinates on method level so a Query object can be applied to various (tables/collections/)indices. I wonder whether it would make sense to extract indices/types into a type that holds both information. We could then add these coordinates to count(…) via count(Query, Class<?>, Index...). That way, we separate the actual criteria from execution-specific targets.

Ideally, we can expose count with the following overloads:

long count(Query query, Class<?> entityClass);

long count(Query query, Index... indexes);

long count(Query query, @Nullable Class<?> entityClass, Index... indexes);

We should also drop the generic type T as there's no outbound usage of T.

mp911de · 2019-11-08T10:17:58Z

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

+ * @author Peter-Josef Meisch
+ * @since 4.0
+ */
+public abstract class AbstractQueryMapper<R> {


We probably don't want a generic type as the query mapper should always return a concrete type that can be used with the store. There's probably not much value in sharing an AbstractQueryMapper across the Rest and the Transport Client implementation.

sothawo · 2019-11-12T20:21:05Z

@mp911de: next iteration:

I moved the IndexCoordinates from being an inner class in EntityOperations out and changed it so it also can handle multiple index names and type names.

The count() methods from ElasticsearchOperations now have additionally the possibility to use IndexCoordinates instead of implicitly having the index and type extracted from the entity or from the Query.

There now is a SearchRequestFactory which is used - at the moment only - for the count() implementations. This factory takes the index info from the passed-in IndexCoordinates, if these are not set, from the entity. It does not use the index/type values from the Query object, index names and types will be removed from Queryonce all the queries are rewritten to use the SearchRequestFactoryand theIndexCoordinates`.

What I have not started to implement is the QueryMapper, part of this mapping is already contained for example in the prepareSort method of the SearchRequestFactory, for this I still need to pass the Entity class down into this method. That will be changed later.

SearchQueryinterface is removed as well as the not needed EsClient interface.

Am I going in the right direction?

mp911de

The change is definitively going in the right direction. I left a few comments about some high-level issues. Great work!

mp911de · 2019-11-13T09:51:40Z

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

-	 * @return
-	 */
-	<T> long count(CriteriaQuery query);
+	default long count(Query query, @Nullable Class<?> clazz) {


Ideally, we can make the entity class non-nullable (if we manage getting rid of the index coordinates inside of Query).

mp911de · 2019-11-13T09:54:15Z

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

+ * Value object encapsulating index name(s) and index type(s).
+ *
+ * @author Mark Paluch
+ * @author Christoph Strobl  @author Peter-Josef Meisch


Nit: Line break, @since

mp911de · 2019-11-13T09:55:07Z

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

+	private final String[] indexNames;
+	private final String[] typeNames;
+
+	public IndexCoordinates(String[] indexNames) {


Let's make constructors private so creation works only using static factory methods and mutators (of(…), withIndex(…), withType(…)).

mp911de · 2019-11-13T09:55:43Z

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

+ * @author Peter-Josef Meisch
+ * @since 4.0
+ */
+public class SearchRequestFactory {


Let's keep that type package-protected.

mp911de · 2019-11-13T09:56:51Z

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

+		}
+	}
+
+	private Pair<QueryBuilder, QueryBuilder> getQueryAndFilter(Query query) {


Let's resolve the need for a Pair by splitting that method into two methods.

mp911de · 2019-11-13T09:58:19Z

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

+	}
+
+	@Nullable
+	private String[] retrieveIndexNameFromPersistentEntity(@Nullable Class clazz) {


The class should be non-nullable and we should use getRequiredPersistentEntity(…) to avoid NPEs.

This method is called when we do not have index coord specified, so having a persistent entity is a requirement. Same for retrieveTypeFromPersistentEntity.

sothawo · 2019-11-25T20:21:17Z

@mp911de

I'm making progress. The work is not complete yet. What I did up to now:

refactor creation of different requests/requestbuilders to RequestFactory
only use the Query class in ElasticsearchOperations instead of a special method for every subclass (not complete yet)
introduce IndexCoordinates to get away from index name(s)/type(s) being passed in the Queryobjects - types are deprecated in ES anyway
I nearly removed all elasticsearch classes from the ElasticsearchOperations API (not complete yet)
I removed all the Facet related classes that were deprecated since 2016 (DATAES-180)
consolidated logic from ElasticsearchTemplate and ElasticsearchRestTemplate into AbstractElasticsearchTemplate and into default methods of ElasticsearchOperations

I'm not yet content with the many calls of IndexCoordinates.of() in the tests, need some better approach there.

If you find the time please have a look.
Do you know why the travis build for this PR fails?

mp911de · 2019-11-25T20:33:57Z

The default Travis environment does not provide Java 8 anymore. Only the older Travis environments come with Java 8.

We're currently in the process of enabling Spring Data builds (the build itself, using Spring Data on Java9+ works for quite some time) on Java 11. There's a branch and build that work on Java 11 with mvn verify.

sothawo · 2019-11-27T22:03:30Z

I think it's time to merge this

mp911de · 2019-11-28T10:28:36Z

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

+		this.indexNames = indexNames;
+	}
+
+	public IndexCoordinates withTypes(String... typeNames) {


Let's follow the immutable approach and create a new instance of IndexCoordinates here so IndexCoordinates can be used as a fully immutable object.

mp911de

Looks pretty decent. I second your opinion that we should merge that PR to not lose more focus. I left a few comments around IndexCoordinates as the class was used previously internally. Now that it's public API, we should make it safe to use.

Feel free to merge this PR afterward. Great work!

mp911de · 2019-11-28T10:29:47Z

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

+	private String[] typeNames = new String[0];
+
+	public static IndexCoordinates of(String... indexNames) {
+		Assert.notNull(indexNames,"indexNames must not be null");


Let's also ensure that indexNames contains at least one element so we can get rid of nullability in getIndexName()

mp911de · 2019-11-28T10:30:03Z

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

+		return indexNames;
+	}
+
+	public String getTypeName() {


Nit: @Nullable

mp911de · 2019-11-28T10:30:47Z

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

+		return indexNames != null && indexNames.length > 0 ? indexNames[0] : null;
+	}
+
+	public String[] getIndexNames() {


Not sure, we might want to return a copy of the array to prevent mutation (e.g getIndexNames()[0] = "foo")

sothawo force-pushed the DATAES-631_-_Consolidate_query_objects branch from efd592e to b23cfae Compare November 3, 2019 06:48

sothawo mentioned this pull request Nov 3, 2019

DATAES-680 - ReactiveElasticsearchTemplate-should-use-the-count-API. #341

Merged

sothawo force-pushed the DATAES-631_-_Consolidate_query_objects branch 2 times, most recently from e0224eb to ee19280 Compare November 5, 2019 10:06

mp911de requested changes Nov 8, 2019

View reviewed changes

sothawo force-pushed the DATAES-631_-_Consolidate_query_objects branch 3 times, most recently from 94290ab to 856ef16 Compare November 12, 2019 20:11

sothawo requested a review from mp911de November 13, 2019 07:59

mp911de reviewed Nov 13, 2019

View reviewed changes

sothawo force-pushed the DATAES-631_-_Consolidate_query_objects branch 2 times, most recently from 1a3333d to d1d09af Compare November 18, 2019 21:55

sothawo force-pushed the DATAES-631_-_Consolidate_query_objects branch 2 times, most recently from 19da170 to f5f4a33 Compare November 24, 2019 15:31

sothawo force-pushed the DATAES-631_-_Consolidate_query_objects branch 2 times, most recently from b738155 to f5814df Compare November 27, 2019 21:57

sothawo marked this pull request as ready for review November 27, 2019 22:02

sothawo requested a review from mp911de November 27, 2019 22:03

mp911de reviewed Nov 28, 2019

View reviewed changes

mp911de approved these changes Nov 28, 2019

View reviewed changes

DATAES-631 - Consolidate query objects.

3e8db01

sothawo force-pushed the DATAES-631_-_Consolidate_query_objects branch from f5814df to 3e8db01 Compare November 28, 2019 22:36

sothawo merged commit 2cd1817 into spring-projects:master Nov 28, 2019

sothawo deleted the DATAES-631_-_Consolidate_query_objects branch November 29, 2019 14:58

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

Consolidate query objects [DATAES-631] #1204

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dataes 631 consolidate query objects #340

Dataes 631 consolidate query objects #340

sothawo commented Nov 2, 2019

mp911de left a comment

mp911de Nov 8, 2019

mp911de Nov 8, 2019

sothawo commented Nov 12, 2019

mp911de left a comment

mp911de Nov 13, 2019

mp911de Nov 13, 2019

mp911de Nov 13, 2019

mp911de Nov 13, 2019

mp911de Nov 13, 2019

mp911de Nov 13, 2019

sothawo commented Nov 25, 2019 •

edited

Loading

mp911de commented Nov 25, 2019

sothawo commented Nov 27, 2019

mp911de Nov 28, 2019

mp911de left a comment

mp911de Nov 28, 2019

mp911de Nov 28, 2019

mp911de Nov 28, 2019

Dataes 631 consolidate query objects #340

Dataes 631 consolidate query objects #340

Conversation

sothawo commented Nov 2, 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

sothawo commented Nov 12, 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

sothawo commented Nov 25, 2019 • edited Loading

mp911de commented Nov 25, 2019

sothawo commented Nov 27, 2019

Choose a reason for hiding this comment

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

sothawo commented Nov 25, 2019 •

edited

Loading