#1218 - Add routing parameter to ElasticsearchOperations. #562

sothawo · 2020-11-30T21:25:33Z

The MappingContext now has a RoutingAccessorFactory.

The @Document annotation has a routing parameter (I call that the routing specification) that will be passed together with the bean to the RoutingAccessorFactory which then can produce a RoutingAccessor for the bean.

The default implementation first checks if the routing specification is the name of an PersistentProperty for the bean. If so, the value of that property will be used for routing. If not, the routing specification will be evaluated as a SpEL expression with the bean as root object, allowing to define some bean that will resolve to a routing value.

If this is not enough, a user can set a custom RoutingAccessorFactory in the ConfigurationSupport.

Where a routing is needed, but no bean/entity available (get/delete by id), I introduced additional methods taking a routing parameter.

sothawo · 2020-12-23T06:47:14Z

@mp911de after our last call I reworked this.

The configuration of the routing is not an parameter in @Document anymore, but has been moved into a new @Routing annotation.

The value of this annotation is not a fixed routing value. This would not make sense. @Document defines the index for an entity class. When this index has multiple shards, using a fixed routing value for all entities would store all entities in the same shard, having the same effect as only having one shard in the index.

Resolving the routing for an entity is done by a RoutingResolver, the default implementation does this:

if the @Routing value is the name of a property, the value of this property is used as routing value
otherwise it is evaluated a SpEL expression against the root context where the entity can be referenced with "#entity"

ElasticsearchOperations has no more overloads for get or delete, but has two methods named withRouting(), one taking a String for a fixed routing value, the other a RoutingResolver.

sothawo · 2021-01-08T06:31:27Z

@mp911de if you find the time to have a look…

mp911de

This looks much cleaner now, good work. I left a few comments, mostly about the design of the routing resolver. Resolution means in that context to obtain the value from the context. I think splitting out more methods to make the routing more null-safe makes a lot of sense. Also, take a look at the design of TypeAliasAccessor and Alias to represent the absence of a defined routing key.

src/main/asciidoc/reference/elasticsearch-routing.adoc

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

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

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

.../java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java

mp911de · 2021-01-08T08:17:49Z

.../java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java

+	 * @since 4.2
+	 */
+	@Nullable
+	String getRouting();


Should return the evaluated expression (when using SpEL) or the routing string (when using literals).

No. this is the value of the @Routing annotation that is defined on the entity class. Do you mean this should return a SpEL expression? To evaluate the expression this would need the entity, that's what resolveRoutingis for.

I removed this method, the persistent entity now only has the resolveRouting() method, that now contains the complete logic.

mp911de · 2021-01-08T08:20:59Z

src/main/java/org/springframework/data/elasticsearch/core/routing/DefaultRoutingResolver.java

+	 */
+	@Override
+	@Nullable
+	public String getRouting(@Nullable Object bean, @Nullable String routing) {


This method is either called without or with a bean for two well-defined cases.

I'd suggest to have two distinct methods getRouting(Class) and getRouting(Class, Object). Handing in the routing argument leaks the fact that routing can be defined on the type already. Maybe it would even make sense to have getRouting(IndexCoordinates) as all these calls to routingResolver.getRouting(null, null) are dealing with null in the first place.

I now have getRouting() for the fixed-value case and getRouting(object bean). So it's no longer possible dot use a lambda for routingResolver in operations.withRouting(routingResolver), but with RoutingResolver.just(fixedValue) this is ok.

src/main/java/org/springframework/data/elasticsearch/core/routing/package-info.java

src/main/java/org/springframework/data/elasticsearch/core/routing/RoutingResolver.java

sothawo · 2021-01-09T22:29:00Z

@mp911de 👋 ready for another round…

mp911de

I added a few minor things to consider. I'm still not decided whether SpEL resolution inside the PersistentEntity is the best approach vs. external annotation lookup. Let's stick with the current design and see how it goes.

src/main/asciidoc/reference/elasticsearch-join-types.adoc

src/main/java/org/springframework/data/elasticsearch/core/routing/RoutingResolver.java

src/main/java/org/springframework/data/elasticsearch/core/routing/DefaultRoutingResolver.java

mp911de · 2021-01-11T08:18:52Z

.../java/org/springframework/data/elasticsearch/core/mapping/ElasticsearchPersistentEntity.java

+	 * @return routing value, may be {@literal null}
+	 */
+	@Nullable
+	String resolveRouting(Object bean);


Actually, bean should be of type T. I think there's a similar constrain when obtaining PersistentPropertyAccessor. Interestingly, isNew(…) accepts Object.

Given the way how this method is used, it's fine to keep Object here and add a type assertion inside the implementation.

assert to what type?

That bean is instance of T (the entity type).

eaudet · 2021-01-13T04:51:49Z

Waiting for this new feature. If I can help (testing) would be happy to. Thanks! In the meantime, is there a workaround? Should I implement a custom repository?

sothawo · 2021-01-17T18:04:46Z

@mp911de care to have another look?

mp911de · 2021-01-18T08:54:41Z

...org/springframework/data/elasticsearch/core/mapping/SimpleElasticsearchPersistentEntity.java

+
+			return expression.getValue(context, String.class);
+		} catch (EvaluationException e) {
+			LOGGER.warn("could not resolve", e);


How about throwing an exception instead of silently falling back to null?

mp911de

Great work, this looks pretty decent.

sothawo requested a review from mp911de November 30, 2020 21:25

sothawo force-pushed the DATAES-644-routing branch from 7e2a1c1 to 8b23548 Compare December 2, 2020 20:57

sothawo marked this pull request as draft December 5, 2020 21:56

sothawo force-pushed the DATAES-644-routing branch 4 times, most recently from af3e97d to 652a7df Compare December 9, 2020 21:40

sothawo force-pushed the DATAES-644-routing branch 4 times, most recently from b768007 to 9b6fbad Compare December 23, 2020 06:33

sothawo force-pushed the DATAES-644-routing branch from 9b6fbad to 3c87483 Compare December 29, 2020 23:30

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

Add routing parameter to ElasticsearchOperations [DATAES-644] #1218

Closed

sothawo changed the title ~~DATAES-644 - Add routing parameter to ElasticsearchOperations.~~ #1218 - Add routing parameter to ElasticsearchOperations. Jan 2, 2021

sothawo linked an issue Jan 2, 2021 that may be closed by this pull request

Add routing parameter to ElasticsearchOperations [DATAES-644] #1218

Closed

sothawo force-pushed the DATAES-644-routing branch from 3c87483 to 379e973 Compare January 2, 2021 18:04

sothawo marked this pull request as ready for review January 2, 2021 18:04

mp911de requested changes Jan 8, 2021

View reviewed changes

sothawo force-pushed the DATAES-644-routing branch from 379e973 to 64690cf Compare January 9, 2021 22:19

sothawo force-pushed the DATAES-644-routing branch 2 times, most recently from d531056 to 4a990e9 Compare January 10, 2021 19:03

mp911de reviewed Jan 11, 2021

View reviewed changes

sothawo force-pushed the DATAES-644-routing branch 2 times, most recently from a0286a0 to 07bbe65 Compare January 12, 2021 18:01

eaudet mentioned this pull request Jan 13, 2021

Define alias for document [DATAES-192] #767

Closed

sothawo force-pushed the DATAES-644-routing branch from 07bbe65 to 9cc3300 Compare January 13, 2021 07:42

sothawo force-pushed the DATAES-644-routing branch 3 times, most recently from 3986468 to 5eeeef2 Compare January 17, 2021 18:03

sothawo force-pushed the DATAES-644-routing branch from 5eeeef2 to 75af58d Compare January 17, 2021 22:57

mp911de reviewed Jan 18, 2021

View reviewed changes

mp911de approved these changes Jan 18, 2021

View reviewed changes

Add routing parameter to ElasticsearchOperations.

9882d84

sothawo force-pushed the DATAES-644-routing branch from 75af58d to 9882d84 Compare January 18, 2021 22:52

sothawo merged commit 89d6ae7 into spring-projects:master Jan 18, 2021

sothawo deleted the DATAES-644-routing branch January 18, 2021 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#1218 - Add routing parameter to ElasticsearchOperations. #562

#1218 - Add routing parameter to ElasticsearchOperations. #562

sothawo commented Nov 30, 2020 •

edited

Loading

sothawo commented Dec 23, 2020

sothawo commented Jan 8, 2021

mp911de left a comment

mp911de Jan 8, 2021

sothawo Jan 9, 2021 •

edited

Loading

sothawo Jan 9, 2021

mp911de Jan 8, 2021

sothawo Jan 9, 2021

sothawo commented Jan 9, 2021

mp911de left a comment

mp911de Jan 11, 2021

sothawo Jan 12, 2021

mp911de Jan 18, 2021

eaudet commented Jan 13, 2021 •

edited

Loading

sothawo commented Jan 17, 2021

mp911de Jan 18, 2021

mp911de left a comment

#1218 - Add routing parameter to ElasticsearchOperations. #562

#1218 - Add routing parameter to ElasticsearchOperations. #562

Conversation

sothawo commented Nov 30, 2020 • edited Loading

sothawo commented Dec 23, 2020

sothawo commented Jan 8, 2021

mp911de left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sothawo Jan 9, 2021 • edited Loading

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 Jan 9, 2021

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

eaudet commented Jan 13, 2021 • edited Loading

sothawo commented Jan 17, 2021

Choose a reason for hiding this comment

mp911de left a comment

Choose a reason for hiding this comment

sothawo commented Nov 30, 2020 •

edited

Loading

sothawo Jan 9, 2021 •

edited

Loading

eaudet commented Jan 13, 2021 •

edited

Loading