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

Add elasticsearch repository test #679

Closed

Conversation

puppylpg
Copy link

  • You have read the Spring Data contribution guidelines.
  • 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).
  1. Add elasticsearch repository test cases;
  2. Fix: use LocalDate rather than String type to match the annotation @Field(type = Date). The former String type is a misuse of date type and will cause the following warn:
    WARN  org.springframework.data.elasticsearch.core.mapping.SimpleElasticsearchPersistentProperty - Unsupported type 'class java.lang.String' for date property 'date'.
    
  3. Use singleton containers when testing using testcontainers;

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jan 23, 2024
@mp911de
Copy link
Member

mp911de commented Sep 9, 2024

@sothawo what do you think?

@sothawo
Copy link

sothawo commented Sep 9, 2024

I will have a look, but cannot do this before the next weekend

Copy link

@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.

I left a couple of comments

result.forEach(it -> verify(it, expectedWord, expectedDate));
}

private void verify(Conference it, String expectedWord, LocalDate expectedDate) {
Copy link

Choose a reason for hiding this comment

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

I'd inline this method

@@ -51,16 +55,16 @@ public void insertDataSample() {
// Save data sample

var documents = Arrays.asList(
Conference.builder().date("2014-11-06").name("Spring eXchange 2014 - London")
Conference.builder().date(LocalDate.parse("2014-11-06", FORMAT)).name("Spring eXchange 2014 - London")
Copy link

Choose a reason for hiding this comment

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

instead of parsing, wouldn't it be better to use

LocalDate.of(2014,11,6)

protected static final DateTimeFormatter FORMAT = DateTimeFormatter.ISO_LOCAL_DATE;

private static final ElasticsearchContainer CONTAINER = new ElasticsearchContainer(
DockerImageName.parse("docker.elastic.co/elasticsearch/elasticsearch:8.7.0")) //
Copy link

Choose a reason for hiding this comment

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

The current released version of Spring Data Elasticsearch is built against Elasticsearch 8.13.4 so. would at least use that.

@Test
void textSearch() {

var expectedDate = LocalDate.parse("2014-10-29", FORMAT);
Copy link

Choose a reason for hiding this comment

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

as above

LocalDate.of(2014, 9, 29)

protected static final DateTimeFormatter FORMAT = DateTimeFormatter.ISO_LOCAL_DATE;

private static final ElasticsearchContainer CONTAINER = new ElasticsearchContainer(
DockerImageName.parse("docker.elastic.co/elasticsearch/elasticsearch:8.7.0")) //
Copy link

Choose a reason for hiding this comment

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

see imperative code

@mp911de mp911de added the status: waiting-for-feedback We need additional information before we can continue label Oct 21, 2024
@mp911de
Copy link
Member

mp911de commented Nov 11, 2024

Closing as this pull request wasn't updated in time. If you would like us to look at this issue, please provide additional information and we will re-open the issue.

@mp911de mp911de closed this Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-feedback We need additional information before we can continue status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants