Skip to content

add routing support for repository operations #1906

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
frnandu opened this issue Aug 24, 2021 · 17 comments
Closed

add routing support for repository operations #1906

frnandu opened this issue Aug 24, 2021 · 17 comments
Assignees

Comments

@frnandu
Copy link
Contributor

frnandu commented Aug 24, 2021

It seems that when using ReactiveElasticsearchRepository, findBy(...) methods don't use the @routing annotation on the entity when building the SearchRequest.
For performance sensitive applications, this means we can't use repositories and are stuck with manually building query and using ReactiveElasticsearchTemplate.
Would be nice to have support for routing on the repository level.
Any ideas going around about this?
I can try to implement it if there aren't any arguments against it...

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Aug 24, 2021
@sothawo
Copy link
Collaborator

sothawo commented Aug 26, 2021

ReactiveElasticsearchOperations support the @Routingannotation since version 4.2 and therefore it should work with repositories as well. Can provide a minimal project reproducing the problem?

@sothawo sothawo added the status: waiting-for-feedback We need additional information before we can continue label Aug 26, 2021
@spring-projects-issues
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Sep 2, 2021
@frnandu
Copy link
Contributor Author

frnandu commented Sep 6, 2021

ok, I found the method withRouting(RoutingResolver routingResolver) in ReactiveElasticsearchOperations, is this what you're referring to when you say that it support @routing annotation? I tough the @routing annotation was used only on save and delete operations, and what I initially was mentioning was the findBy(...) methods. If setting this routingResolver works for findBy(...) methods would mean that you have to configure a separate ReactiveElasticsearchTemplate for each repository (since each entity repository needs a different routingResolving field). If you have loads of different entities and repositories, this starts to be a little inconvenient. Would be nice to be able to parameterize the findBy(...) search operations with maybe a findBy(...)WithRouting(...) kind of way.
Any toughts?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Sep 6, 2021
@sothawo sothawo added the type: bug A general bug label Sep 6, 2021
@sothawo
Copy link
Collaborator

sothawo commented Sep 6, 2021

I checked the code and found that Spring Data Elasticsearch supports the routing when creating, updating and deleting entities. But not in search requests. This should have been done in #1218 but it was missed there. Thanks for finding this.

This not only is an error in the reactive code, but in the imperative as well.

As for the withRouting method: This would not help, as the RoutingResolver currently is not used anyway.

@sothawo sothawo removed the status: waiting-for-triage An issue we've not yet triaged label Sep 6, 2021
@sothawo sothawo self-assigned this Sep 10, 2021
@sothawo
Copy link
Collaborator

sothawo commented Sep 10, 2021

I just started to get deeper into this and just am wondering how the @Routing annotation on the entity would help for a repository search.

The @Routing annotation either defines a property of an entity instance which should be used for the routing value or it is a SpEL expression that resolves to a routing for an entity. But a repository search method does not have an entity instance to extract a routing value from.

So how should this be used in the repository context? Can you provide a concrete example of how this should work?

@sothawo sothawo added status: waiting-for-feedback We need additional information before we can continue and removed type: bug A general bug status: feedback-provided Feedback has been provided labels Sep 10, 2021
@frnandu
Copy link
Contributor Author

frnandu commented Sep 15, 2021

The way I see it, when you need to for example findById (which accepts only ID for entity) and routing is something required on elastic mapping or mandatory by your business logic, the only way to pass routing value for whatever property you use for routing is to use something like findByIdWithRouting(ID id, String routing).
Currently in my project I'm using temporarily something along the lines of:

public interface RoutingAwareReactiveElasticsearchRepository<T, ID> extends ReactiveElasticsearchRepository<T,ID>  {
   Mono<T> findByIdWithRouting(ID id, String routing);
   Flux<T> findByMustPropertiesWithRouting(Map<String,Object> properties, String routing);
}

with following implementation:

public class RoutingAwareReactiveElasticsearchRepositoryImpl<T, ID> extends SimpleReactiveElasticsearchRepository<T, ID> implements RoutingAwareReactiveElasticsearchRepository<T, ID> {

private final ElasticsearchEntityInformation<T, ID> entityInformation;
private final ReactiveElasticsearchOperations operations;
private final ReactiveIndexOperations indexOperations;

public RoutingAwareReactiveElasticsearchRepositoryImpl(ElasticsearchEntityInformation<T, ID> entityInformation, ReactiveElasticsearchOperations operations) {
    super(entityInformation, operations);
    this.entityInformation = entityInformation;
    this.operations = operations;
    this.indexOperations = operations.indexOps(entityInformation.getJavaType());
}

@Override
public Mono<T> findByIdWithRouting(ID id, String routing) {
    Assert.notNull(id, "Id must not be null!");

    Class<T> entityType = entityInformation.getJavaType();
    ReactiveElasticsearchTemplate reactiveElasticsearchTemplate = (ReactiveElasticsearchTemplate) operations;

    IndexCoordinates index = reactiveElasticsearchTemplate.getIndexCoordinatesFor(entityType);

    ReadDocumentCallback<T> callback = reactiveElasticsearchTemplate.new ReadDocumentCallback<>(
            reactiveElasticsearchTemplate.getElasticsearchConverter(), entityType, index);

    return Mono.defer(() ->
                    reactiveElasticsearchTemplate.doGet(
                            reactiveElasticsearchTemplate.requestFactory.getRequest(
                                    reactiveElasticsearchTemplate.getElasticsearchConverter().convertId(id), routing, index)))


            .flatMap(response -> callback.toEntity(DocumentAdapters.from(response)));

}

@Override
public Flux<T> findByMustPropertiesWithRouting(Map<String, Object> properties, String routing) {
    BoolQueryBuilder boolQueryBuilder = QueryBuilders.boolQuery();
    for (String key : properties.keySet()) {
        boolQueryBuilder.must(QueryBuilders.termQuery(key, properties.get(key)));
    }
    Query query = new NativeSearchQueryBuilder()
            .withQuery(boolQueryBuilder)
            .withRoute(routing)
            .withSearchType(SearchType.DEFAULT)
            .build();
    ReactiveElasticsearchTemplate reactiveElasticsearchTemplate = (ReactiveElasticsearchTemplate) operations;
    return reactiveElasticsearchTemplate.search(query, entityInformation.getJavaType())
            .map(searchHit -> searchHit.getContent());
}

}

This for now works for my scenario, but if it could be done automatically for findBy...WithRouting methods without need for special interface and impl, would be great.
What's your toughs, any better ideas?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 15, 2021
@sothawo
Copy link
Collaborator

sothawo commented Sep 15, 2021

Ok, I see the point.

The bad news: It's not possible just by using a method name like findByIdWithRouting. These methods names are
parsed in the common Spring Data code which is independent of the different stores like Elasticsearch or MongoDB or
JPA and so these names can only use the prefixes or keywords that are common to these. There's no point to add store
specific things like withRouting

The good news: it's not as complicated as in the code you wrote.

You need to implement custom repository fragments, let me show this in an example. In my test application a have a
Person entity class with a String id field.

We start with an interface definition for a routing repository:

RoutingRepository

public interface RoutingRepository<T, ID> {
	Mono<T> findByIdWithRouting(ID id, String routing);
}

This is just a plain interface defining the desired methods, not extending anything, no spring or spring data stuff
here.

The next is an abstract implementation of this interface:

AbstractRoutingRepository

public abstract class AbstractRoutingRepository<T, ID> implements RoutingRepository<T, ID> {

	private final ReactiveElasticsearchOperations operations;
	private final Class<T> clazz;

	protected AbstractRoutingRepository(ReactiveElasticsearchOperations operations, Class<T> clazz) {
		this.operations = operations;
		this.clazz = clazz;
	}

	@Override
	public Mono<T> findByIdWithRouting(ID id, String routing) {

		var stringId = operations.getElasticsearchConverter().convertId(id);
		return operations.withRouting(RoutingResolver.just(routing))
			.get(stringId, clazz);
	}
}

Classes extending will provide an ReactiveElasticsearchOperations and the entity class in the constructor, we'll
see later how this is done. Again, no Spring stuff here.

In the implementation of the findByIdWithRouting method we first convert the ID to a String. Then we call the
get() method of the ReactiveElasticsearchOperations instance but before that, we set the routing for the
operations to use the fix value routing which was passed into the method.

So this is what you need to implement: call the methods of the operations like get or query and set the
routing like shown.

The next step is to define a concrete interface:

PersonCustomRepository

public interface PersonCustomRepository extends RoutingRepository<Person, String> {
}

Just a typed version of the interface. The repository we use in our program will extend from this and Spring Data
will try into instantiate this. Therefore we need to provide an implementation:

PersonCustomRepositoryImpl

public class PersonCustomRepositoryImpl extends AbstractRoutingRepository<Person, String> implements PersonCustomRepository {

	public PersonCustomRepositoryImpl(ReactiveElasticsearchOperations operations) {
		super(operations, Person.class);
	}

}

This implementation will be instantiated by Spring Data Elasticsearch and will have the
ReactiveElasticsearchOperations injected in its constructor. We pass that together with the entity class to the
base class.

And now the final repository interface:

PersonRepository

public interface PersonRepository extends ReactiveElasticsearchRepository<Person, Long>, 
	PersonCustomRepository {
}

This is a normal Spring Data Elasticsearch repository but also extending our custom interface. You'll have this
injected into your service or controller classes like any Spring Data repository.

So you will have to do the implementation by yourself, but you can use the methods of the
ReactiveElasticsearchOperations interface in combination with ReactiveElasticsearchOperations.withRouting (RoutingResolver) like shown in the example.

Hope that helps.

@sothawo sothawo added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Sep 15, 2021
@frnandu
Copy link
Contributor Author

frnandu commented Sep 16, 2021

Your suggested code would require 1 class + 1 interface PER business entity, instead of only 1 interface per entity.

What I was trying to anchieve (but maybe not as clean code since I'm not familiar with internals of spring-data or spring-data/elasticsearch) is to not have the need for passing a Class clazz on the implementation, since it seemed to me that from inside SimpleReactiveElasticsearchRepository you could get which entity class is this repository created for from the ElasticsearchEntityInformation<T, ID> entityInformation;

Maybe possible to merge both options, and have cleaner code (like yours) on the implementation but without the need for passing Class clazz , thus avoiding the need for 1 additional impl class for each entity??
That would nice.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 16, 2021
@frnandu
Copy link
Contributor Author

frnandu commented Sep 16, 2021

...actually 1 class + 2 interfaces PER business entity...

@sothawo
Copy link
Collaborator

sothawo commented Sep 17, 2021

Yes, what I showed was customization of single repositories. But you can as well change the base implementation merging your approach with my custom implementation (I only show one of the methods):

RoutingAwareReactiveElasticsearchRepository

@NoRepositoryBean
public interface RoutingAwareReactiveElasticsearchRepository<T, ID> extends ReactiveElasticsearchRepository<T, ID> {
	Mono<T> findByIdWithRouting(ID id, String routing);
}

Please mark the @NoRepositoryBean annotation on this interface.

RoutingAwareReactiveElasticsearchRepositoryImpl

public class RoutingAwareReactiveElasticsearchRepositoryImpl<T, ID> extends SimpleReactiveElasticsearchRepository<T, ID>
     implements RoutingAwareReactiveElasticsearchRepository<T, ID> {

	private final ElasticsearchEntityInformation<T, ID> entityInformation;
	private final ReactiveElasticsearchOperations operations;
	private final ReactiveIndexOperations indexOperations;

	public RoutingAwareReactiveElasticsearchRepositoryImpl(ElasticsearchEntityInformation<T, ID> entityInformation, ReactiveElasticsearchOperations operations) {
		super(entityInformation, operations);
		this.entityInformation = entityInformation;
		this.operations = operations;
		this.indexOperations = operations.indexOps(entityInformation.getJavaType());
	}

	@Override
	public Mono<T> findByIdWithRouting(ID id, String routing) {
		var stringId = operations.getElasticsearchConverter().convertId(id);
		return operations.withRouting(RoutingResolver.just(routing)).get(stringId, entityInformation.getJavaType());
	}
}

no need to copy all that callback stuff, use withRouting().

PersonRepository

public interface PersonRepository extends RoutingAwareReactiveElasticsearchRepository<Person, Long> {
}

You need to adapt the interface your repositories extend

Configuration

@SpringBootApplication(exclude = ElasticsearchDataAutoConfiguration.class)
@EnableReactiveElasticsearchRepositories(repositoryBaseClass = RoutingAwareReactiveElasticsearchRepositoryImpl.class)
public class SpringdataElasticTestApplication {
//...
}

Set the repository base class in the @EnableReactiveElasticsearchRepositories annotation.

And for the other methods just build a Query and then call the appropriates search call on the operations.withRouting().

@sothawo sothawo added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Sep 17, 2021
@frnandu
Copy link
Contributor Author

frnandu commented Sep 17, 2021

That's perfect, thank you.
Could these RoutingAwareReactiveElasticsearchRepository* be added to spring-data-elasticsearch?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Sep 17, 2021
@sothawo
Copy link
Collaborator

sothawo commented Sep 17, 2021

For the findByIdWithRouting method it might make sense to add, but findByMustPropertiesWithRouting not. This is a very specialized method that assumes that all the fields that are passed in are stored as analyzed Strings. What about keywords, dates in different formats or numeric values?

And even more problematic: If you have a property defined like this:

@Field(name="foo-bar", type = FieldType.text)
private String fooBar

where the property name and the field name in Elasticsearch differ, the caller of the method would need to remember this when passing in these names in the parameter map's keys. For date fields the caller would need to manually convert the date property to a String value in the format that was defined on the property. These conversions are normally done transparently for the user.

So the user would anyway need a custom implementation.

@sothawo sothawo added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Sep 17, 2021
@spring-projects-issues
Copy link

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Sep 24, 2021
@spring-projects-issues
Copy link

Closing due to lack of requested feedback. If you would like us to look at this issue, please provide the requested information and we will re-open the issue.

@spring-projects-issues spring-projects-issues removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Oct 1, 2021
@neeraj7483
Copy link

I checked the code and found that Spring Data Elasticsearch supports the routing when creating, updating and deleting entities. But not in search requests. This should have been done in #1218 but it was missed there. Thanks for finding this.

This not only is an error in the reactive code, but in the imperative as well.

As for the withRouting method: This would not help, as the RoutingResolver currently is not used anyway.

@sothawo Sorry to open this again but is there a way around it, I am using ReactiveElasticsearchOperations's searchForPage and count methods and using withrouting method and it is not doing anything as you have mentioned and I was looking at the code and routingResolver is straight-up ignored. I am using spring-data-elasticsearch version 4.2.7 (comes with spring boot 2.5.8).

@sothawo
Copy link
Collaborator

sothawo commented Feb 15, 2022

This issue was about routing support in repositories, you are using operations, that's something different. Please create a new issue and provide a compilable, runnable example that show your issue.

@neeraj7483
Copy link

@sothawo I have created a new issue 2087. Can you please take a look at it and direct me to the right path if I am doing something wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants