-
Notifications
You must be signed in to change notification settings - Fork 3.4k
feat: Add examples for updating documents using ElasticsearchTemplate #700
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
feat: Add examples for updating documents using ElasticsearchTemplate #700
Conversation
Signed-off-by: OmniCoder77 <rishabh_2024bcse025@nitsri.ac.in> Signed-off-by: OmniCoder77 <rishabhsaraswat17@gmail.com>
5443941
to
d1bef67
Compare
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.
This pull request illustrates why I don't like AI: It generates the wrong thing just faster. In contrast, a human consistently repeats their mistakes or evolves throughout a coding task by learning from their doings.
One might argue: But wait, this code uses API exposed by the framework, what's wrong with it? Actually, every single aspect of it. Contributions to both files are super-inconsistent, API isn't used as it should be, reading the code causes a maximal level of confusion and it is hard to understand what and why some things are done.
.search(initialQuery, Conference.class) | ||
.next() | ||
.flatMap(searchHit -> { | ||
assert searchHit != null; |
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.
assert
?
@@ -74,6 +76,8 @@ public ClientConfiguration clientConfiguration() { | |||
} | |||
|
|||
@Autowired ReactiveElasticsearchOperations operations; | |||
@Autowired |
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.
Why is this injecting the Template object? We have already ReactiveElasticsearchOperations
at hand.
@@ -73,6 +77,8 @@ public ClientConfiguration clientConfiguration() { | |||
} | |||
|
|||
@Autowired ElasticsearchOperations operations; | |||
@Autowired |
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.
Why is this injecting the Template object? We have already ElasticsearchOperations
at hand.
|
||
var initialQuery = new CriteriaQuery(new Criteria("name").is(conferenceName)); | ||
SearchHit<Conference> searchHit = template.searchOne(initialQuery, Conference.class); | ||
assertThat(searchHit).isNotNull(); |
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.
Asserting an intermediate state quickly creates the impression that we're testing something that shouldn't be tested. When setting up a state, we assume it is correct and expect the given state, otherwise, the entire arrangement is just waste.
String conferenceId = searchHit.getId(); | ||
|
||
int originalKeywordsCount = conference.getKeywords().size(); | ||
assertThat(conference.getKeywords()).doesNotContain(newKeyword); |
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.
Same as above, the need for asserting initial state hints at a potential problem with creating the state and distracts from the thing we actually want to explain here.
|
||
Conference updatedConference = template.get(conferenceId, Conference.class); | ||
assertThat(updatedConference).isNotNull(); | ||
assertThat(updatedConference.getKeywords()).contains(newKeyword); |
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.
contains
and hasSize
can be combined.
return template.save(conference) | ||
.then(template.get(conferenceId, Conference.class)) | ||
.map(updatedConference -> { | ||
assert updatedConference != null; |
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.
Why don't we use AssertJ here?
} | ||
|
||
@Test | ||
void shouldUpdateConferenceKeywords() { |
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.
We use spaces and not tabs.
Closes #698
Goal
This pull request addresses the feature request to add examples for updating an existing document, a core use case that was previously missing from the samples.
As suggested in the issue, this implementation uses
ElasticsearchTemplate
(ElasticsearchOperations
) to perform the update, filling a specific gap in the existing examples and demonstrating a template-based approach instead of a repository-based one.Implementation Details
Imperative Example:
shouldUpdateConferenceKeywords()
, has been added toElasticsearchOperationsTest.java
.Conference
object using aCriteriaQuery
, modifies its keyword list, saves the object back usingtemplate.save()
, and verifies that the update was successful by re-fetching the document and asserting on its contents.Reactive Example:
shouldUpdateConferenceKeywordsReactive()
, has been added toReactiveElasticsearchOperationsTest.java
.ReactiveElasticsearchOperations
) andStepVerifier
to test the entire non-blocking flow of finding, modifying, saving, and verifying the document.Code Style:
@author
tags have been added to the class-level Javadoc of the modified files to conform to the project's existing style.