-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Support partially update document by entity. #2305
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR. I have commented some minor things, I will fix them with a Polishing commit after merging.
src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchIntegrationTests.java
Show resolved
Hide resolved
src/test/java/org/springframework/data/elasticsearch/core/ElasticsearchIntegrationTests.java
Show resolved
Hide resolved
@@ -508,7 +529,7 @@ ElasticsearchPersistentEntity<?> getRequiredPersistentEntity(Class<?> clazz) { | |||
} | |||
|
|||
@Nullable | |||
private String getEntityId(Object entity) { | |||
public String getEntityId(Object entity) { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change is not necessary for this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's true this modifycation is unrelated with this PR. However,
before this PR, I update an entity using spring-data-elasticsearch in this way:
- to get
_id
: ElasticsearchRestTemplate#getEntityId - to get
_routing
: ElasticsearchRestTemplate#getEntityRouting - to get
_source
: elasticsearchConverter.mapObject(entity)
(Then combine id/routing/source to a UpdateQuery.)
The latter two methods have public access while the first one is private
thus must be copied from ElasticsearchRestTemplate
to my class manually to use. If it was public
, this copy won't happen.
If this PR is merged, seems that there is no need to access getEntityId
in outer class again. However, getRouting(entity)
is similar to this method in the same class and be marked as public
, which is convenient to use. I think it may be better to be accessible for this method too.
Anyway, I'll not modify this line to public
in new PR, but leave it for your consideration.
src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java
Show resolved
Hide resolved
src/main/java/org/springframework/data/elasticsearch/core/AbstractElasticsearchTemplate.java
Show resolved
Hide resolved
Sorry, had to revert the merge. I missed that it was targeting the wrong branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes are accepted
com/spring-projects/spring-data-elasticsearch/issues). Add the issue number to the Closes #issue-number line below
Closes #2304