Skip to content
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

DATAES-407 - Polishing. #278

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@xhaggi
Copy link
Collaborator

commented Apr 25, 2019

  • introduce an AbstractElasticsearchTemplate
  • unify code between ElasticsearchTemplate and ElasticseachRestTemplate
  • improve assertion for index name and index type
  • extract methods to reduce dupe code
  • add missing generics
  • reuse ObjectMapper in ElasticseachRestTemplate
  • fix sort by score in ElasticseachRestTemplate
  • organize imports + formatter
  • cleanup code
  • remove unused methods
  • fix tests not defining index types

@mp911de @christophstrobl @akonczak Would you mind taking a look at this polishing commit

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our JIRA.
  • You use the code formatters provided here and have them applied to your changes. Don’t submit any formatting related changes.
  • You submit test cases (unit or integration tests) that back your changes.
  • You added yourself as author in the headers of the classes you touched. Amend the date range in the Apache license header if needed. For new types, add the license header (copy from another file and set the current year only).

@xhaggi xhaggi force-pushed the xhaggi:polishing/DATAES-407 branch from 03897a2 to ab4bdf7 Apr 25, 2019

@mp911de

This comment has been minimized.

Copy link
Member

commented Apr 28, 2019

Would you mind rebasing the PR before review? Would it also make sense to consider the reactive template into polishing or do you think that's rather something for a separate round of cleanups (i.e. aligning ElasticsearchRestTemplate towards the reactive one)?

DATAES-407 - Polishing.
* introduce an AbstractElasticsearchTemplate
* unify code between ElasticsearchTemplate and ElasticseachRestTemplate
* improve assertion for index name and index type
* extract methods to reduce dupe code
* add missing generics
* reuse ObjectMapper in ElasticseachRestTemplate
* fix sort by score in ElasticseachRestTemplate
* organize imports + formatter
* cleanup code
* remove unused methods
* fix tests not defining index types

@xhaggi xhaggi force-pushed the xhaggi:polishing/DATAES-407 branch from ab4bdf7 to 47e21f7 Apr 29, 2019

@xhaggi

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 29, 2019

Would you mind rebasing the PR before review?

Sure and already done.

Would it also make sense to consider the reactive template into polishing or do you think that's rather >something for a separate round of cleanups (i.e. aligning ElasticsearchRestTemplate towards the >reactive one)?

IMO we should do it in a separate round, with a dedicated Jira issue for that.

*/
public abstract class AbstractElasticsearchTemplate implements ElasticsearchOperations {

protected static String[] toArray(List<String> values) {

This comment has been minimized.

Copy link
@mp911de

mp911de Apr 29, 2019

Member

Let's try to not expose protected utility methods as API. How about introducing a package-private utility class for Spring Data ES to ES API conversion. Alternatively, how about default methods on Query to retrieve fields, indices, types as array?

return values.toArray(new String[values.size()]);
}

protected static MoreLikeThisQueryBuilder.Item[] toArray(MoreLikeThisQueryBuilder.Item... values) {

This comment has been minimized.

Copy link
@mp911de

mp911de Apr 29, 2019

Member

We should find a better place for this utility method.

return getElasticsearchConverter().getMappingContext().getRequiredPersistentEntity(type);
}

protected List<String> defaultIfEmpty(List<String> strs, Supplier<String> defaultString) {

This comment has been minimized.

Copy link
@mp911de

mp911de Apr 29, 2019

Member

We should find a better place for this utility method.

return CollectionUtils.isEmpty(strs) ? Arrays.asList(defaultString.get()) : strs;
}

protected String defaultIfEmpty(String str, Supplier<String> defaultString) {

This comment has been minimized.

Copy link
@mp911de

mp911de Apr 29, 2019

Member

We should find a better place for this utility method.

return StringUtils.isEmpty(str) ? defaultString.get() : str;
}

protected List<String> extractIds(SearchResponse response) {

This comment has been minimized.

Copy link
@mp911de

mp911de Apr 29, 2019

Member

We should find a better place for this utility method.

return ids;
}

protected <T> Map<String, String> getDefaultSettings(ElasticsearchPersistentEntity<T> persistentEntity) {

This comment has been minimized.

Copy link
@mp911de

mp911de Apr 29, 2019

Member

How about introducing a package-private EntityOperations API that exposes common functionality for entities? All these methods including how to get/set an Id would be subject to moving to such a utility.

See MongoDB for further reference.

@mp911de mp911de force-pushed the spring-projects:master branch from 01cda35 to b6fa4c8 May 6, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.