Skip to content

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

Merged
merged 2 commits into from
Sep 24, 2022

Conversation

puppylpg
Copy link
Contributor

  • You have read the Spring Data contribution guidelines.
  • There is a ticket in the bug tracker for the project in our [issue tracker](https://github.
    com/spring-projects/spring-data-elasticsearch/issues)
    . Add the issue number to the Closes #issue-number line below
  • 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).

Closes #2304

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Sep 22, 2022
Copy link
Collaborator

@sothawo sothawo left a 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.

@@ -508,7 +529,7 @@ ElasticsearchPersistentEntity<?> getRequiredPersistentEntity(Class<?> clazz) {
}

@Nullable
private String getEntityId(Object entity) {
public String getEntityId(Object entity) {

Copy link
Collaborator

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.

Copy link
Contributor Author

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.

@sothawo sothawo merged commit 7e904cd into spring-projects:4.4.x Sep 24, 2022
@sothawo sothawo linked an issue Sep 24, 2022 that may be closed by this pull request
@sothawo
Copy link
Collaborator

sothawo commented Sep 24, 2022

Sorry, had to revert the merge. I missed that it was targeting the wrong branch.
We do not add new features in published versions, only on the main branch, so this should be merged into the main branch.

Copy link
Contributor Author

@puppylpg puppylpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes are accepted

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants