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.

Sign up for GitHub

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

refactor: create AbstractAggregationRequestIT class + fail with OpenS… #421

Merged
merged 12 commits into from
Apr 3, 2023
Merged

refactor: create AbstractAggregationRequestIT class + fail with OpenS… #421

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
65ed8c3
refactor: create AbstractAggregationRequestIT class + fail with OpenS…
szczepanczykd Mar 30, 2023
d938d19
feature: upload IntegrationTests reports
szczepanczykd Mar 30, 2023
bf9b658
fix: fixed workflow file
szczepanczykd Mar 30, 2023
51e0821
fix: upload reports on failure
szczepanczykd Mar 30, 2023
11210c4
refactor: add more error details to tests
szczepanczykd Mar 30, 2023
e168d8d
feat: update github workflows with name and retention-days
szczepanczykd Mar 30, 2023
5ee1c48
test: create index with manual mappings
szczepanczykd Mar 30, 2023
986d6dc
refactor: change expDate type from Date to String
szczepanczykd Mar 31, 2023
71944df
refactor: stability check
szczepanczykd Mar 31, 2023
c8f6e5e
refactor: stability check 2
szczepanczykd Mar 31, 2023
26753a9
fix: change retention-days to 7 for uploaded artifacts
szczepanczykd Mar 31, 2023
bc4eaec
refactor: change expDate type to Long
szczepanczykd Mar 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 13 additions & 15 deletions ...rc/test/java/org/opensearch/client/opensearch/integTest/AbstractAggregationRequestIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

package org.opensearch.client.opensearch.integTest;

import com.fasterxml.jackson.annotation.JsonFormat;
import org.junit.Test;
import org.opensearch.client.opensearch._types.ErrorCause;
import org.opensearch.client.opensearch._types.OpenSearchException;
Expand All @@ -23,9 +22,9 @@
import org.opensearch.client.opensearch.core.SearchResponse;

import java.io.IOException;
import java.time.Instant;
import java.time.LocalDateTime;
import java.time.ZoneOffset;
import java.util.Date;
import java.util.List;

public abstract class AbstractAggregationRequestIT extends OpenSearchJavaClientTestCase {
Expand Down Expand Up @@ -98,23 +97,23 @@ private SearchResponse<Void> sendAggregateRequest(String index, String key, Aggr
private List<DateRangeExpression> getDateAggregationRanges() {
return List.of(
new DateRangeExpression.Builder()
.from(builder -> builder.value((double) getDatePlusDays(1).getTime()))
.from(builder -> builder.value((double) getDatePlusDays(1).toEpochMilli()))
.to(FieldDateMath.of(
builder -> builder.value((double) getDatePlusDays(3).getTime() - 1000))
builder -> builder.value((double) getDatePlusDays(3).toEpochMilli() - 1000))
)
.key("from-1-to-2-days")
.build(),
new DateRangeExpression.Builder()
.from(builder -> builder.value((double) getDatePlusDays(3).getTime()))
.from(builder -> builder.value((double) getDatePlusDays(3).toEpochMilli()))
.to(FieldDateMath.of(
builder -> builder.value((double) getDatePlusDays(5).getTime() - 1000))
builder -> builder.value((double) getDatePlusDays(5).toEpochMilli() - 1000))
)
.key("from-3-to-4-days")
.build(),
new DateRangeExpression.Builder()
.from(builder -> builder.value((double) getDatePlusDays(5).getTime()))
.from(builder -> builder.value((double) getDatePlusDays(5).toEpochMilli()))
.to(FieldDateMath.of(
builder -> builder.value((double) getDatePlusDays(7).getTime() - 1000))
builder -> builder.value((double) getDatePlusDays(7).toEpochMilli() - 1000))
)
.key("from-5-to-6-days")
.build()
Expand Down Expand Up @@ -158,11 +157,11 @@ private void createIndex(String index) throws IOException {
}

private ProductDetails createProduct(String name, int cost, int plusDays) {
return new ProductDetails(name, cost, getDatePlusDays(plusDays));
return new ProductDetails(name, cost, getDatePlusDays(plusDays).toString());
}

private Date getDatePlusDays(int plusDays) {
return java.sql.Date.from(LocalDateTime.of(2023, 2, 20, 0, 0, 0).plusDays(plusDays).toInstant(ZoneOffset.UTC));
private Instant getDatePlusDays(int plusDays) {
return LocalDateTime.of(2023, 2, 20, 0, 0, 0, 0).plusDays(plusDays).toInstant(ZoneOffset.UTC);
}

private String getExceptionDetails(String index, OpenSearchException ex) {
Expand Down Expand Up @@ -190,13 +189,12 @@ private String getErrorMetadata(ErrorCause errorCause) {
public static class ProductDetails {
private String name;
private int cost;
@JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ss.SSS")
private Date expDate;
private String expDate;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This change is not very straightforward: definitely 👍 to move off the Date, but using string to pass the dates through is not a best option I think:

  • should we use LocateDateTime / Instant instead` ?
  • should be use the timestamp value (long)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

String allows to insert date in ISO format without any transformations.

LocateDateTime and Instant are requiring:

@JsonFormat(shape = JsonFormat.Shape.STRING, pattern = "yyyy-MM-dd'T'HH:mm:ssZ", timezone = "UTC")
private Instant expDate;

build.gradle.kts:
testImplementation("com.fasterxml.jackson.datatype", "jackson-datatype-jsr310", jacksonVersion)

JacksonJsonpMapper -> findAndRegisterModules method used:

new ObjectMapper()
                .findAndRegisterModules()
                .configure(SerializationFeature.INDENT_OUTPUT, false)
                .setSerializationInclusion(JsonInclude.Include.NON_NULL)

private Long expDate; does not require anything more.
Seems it works but the minus is that the Document is created with numeric values:

  "_index": "test-date-range-aggregation",
  "_type": "_doc",
  "_id": "1",
  "_score": 1,
  "_source": {
    "name": "egg",
    "cost": 2,
    "expDate": 1676937600000
  }
}

So I'm not sure what is better than simple String ;P

Copy link
Collaborator

@reta reta Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

String allows to insert date in ISO format without any transformations.

That is what `@JsonFormat(shape = JsonFormat.Shape.STRING, pattern = ISO) is for, piratically this is how the feature should be used (yes, using string does simplify tests).

private Long expDate; does not require anything more.

That's fine, the OS is able to deal with date in different formats.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, let's stick with Long expDate.


public ProductDetails() {
}

public ProductDetails(String name, int cost, Date expDate) {
public ProductDetails(String name, int cost, String expDate) {
this.name = name;
this.cost = cost;
this.expDate = expDate;
Expand All @@ -210,7 +208,7 @@ public int getCost() {
return cost;
}

public Date getExpDate() {
public String getExpDate() {
return expDate;
}
}
Expand Down