Skip to content
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
6 changes: 6 additions & 0 deletions docs/changelog/112650.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 112650
summary: Handle ArithmeticException(long overflow) in dateFieldMapper
area: Search
type: bug
issues:
- 112483
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this patch solves issue #112483 though - wouldn't IllegalArgumentException still produce 500? One would have to catch it in DateFieldMapper and convert it to ElasticsearchParseException to make it 4xx if I understand it correctly. Which however raises the question - if we'd have to catch it there, does it really need to be also converted in toLongMillis (not a rhetorical question, I don't have a predetermined answer to it)?

Copy link
Author

@pulak777 pulak777 Sep 9, 2024

Choose a reason for hiding this comment

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

I totally agree with this point. Now I think, the only use case of this patch can be providing better error message to the user (given that we catch it in DateFieldMapper as well, and convert to ElasticsearchParseException to make it 4xx)

Copy link
Member

Choose a reason for hiding this comment

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

IllegalArgumentException does normally get converted to a 400 error here: https://github.com/elastic/elasticsearch/blob/main/server/src/main/java/org/elasticsearch/ExceptionsHelper.java#L64 . Any reason why this one specifically would lead to a 500 instead?

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't aware of this and assumed IllegalArgumentException returns 500 based on previous comment. As that's not the case, I think the patch should work without additonal modification

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah maybe I was confusing it with another exception.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have a test that verifies this? Don't just believe me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't verify the full circle here because the original issue did not contain reproducing example and I am not sure how to reproduce it from scratch. But this suggests @javanna is correct:

} else if (t instanceof IllegalArgumentException) {
so while I'd be more happy if we had full cycle test I think this is ok.

Copy link
Member

Choose a reason for hiding this comment

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

++ sounds good.

Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,30 @@ public static long toLong(Instant instant) {
return instant.getEpochSecond() * 1_000_000_000 + instant.getNano();
}

/**
* Convert a java time instant to a long value which is stored in lucene,
* the long value resembles the milliseconds since the epoch
*
* @param instant the instant to convert
* @return the total milliseconds as a single long
*/
public static long toLongMillis(Instant instant) {
try {
return instant.toEpochMilli();
} catch (ArithmeticException e) {
if (instant.isAfter(Instant.now())) {
throw new IllegalArgumentException(
"date[" + instant + "] is too far in the future to fit in a long milliseconds variable"
);
}
else {
throw new IllegalArgumentException(
"date[" + instant + "] is too far in the past to fit in a long milliseconds variable"
);
}
}
}

/**
* Returns an instant that is with valid nanosecond resolution. If
* the parameter is before the valid nanosecond range then this returns
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import java.util.function.LongSupplier;

import static org.elasticsearch.common.time.DateUtils.toLong;
import static org.elasticsearch.common.time.DateUtils.toLongMillis;

/** A {@link FieldMapper} for dates. */
public final class DateFieldMapper extends FieldMapper {
Expand All @@ -89,7 +90,7 @@ public enum Resolution {
MILLISECONDS(CONTENT_TYPE, NumericType.DATE, DateMillisDocValuesField::new) {
@Override
public long convert(Instant instant) {
return instant.toEpochMilli();
return toLongMillis(instant);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import static org.elasticsearch.common.time.DateUtils.clampToNanosRange;
import static org.elasticsearch.common.time.DateUtils.toInstant;
import static org.elasticsearch.common.time.DateUtils.toLong;
import static org.elasticsearch.common.time.DateUtils.toLongMillis;
import static org.elasticsearch.common.time.DateUtils.toMilliSeconds;
import static org.elasticsearch.common.time.DateUtils.toNanoSeconds;
import static org.hamcrest.Matchers.containsString;
Expand Down Expand Up @@ -53,6 +54,28 @@ public void testInstantToLongMax() {
assertThat(e.getMessage(), containsString("is after"));
}

public void testInstantToLongMillis() {
assertThat(toLongMillis(Instant.EPOCH), is(0L));

Instant instant = createRandomInstant();
long timeSinceEpochInMillis = instant.toEpochMilli();
assertThat(toLongMillis(instant), is(timeSinceEpochInMillis));
}

public void testInstantToLongMillisMin() {
/* negative millisecond value of this instant exceeds the maximum value a java long variable can store */
Instant tooEarlyInstant = Instant.ofEpochSecond(-9223372036854776L);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> toLongMillis(tooEarlyInstant));
assertThat(e.getMessage(), containsString("too far in the past"));
}

public void testInstantToLongMillisMax() {
/* millisecond value of this instant exceeds the maximum value a java long variable can store */
Instant tooLateInstant = Instant.ofEpochSecond(9223372036854776L);
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> toLongMillis(tooLateInstant));
assertThat(e.getMessage(), containsString("too far in the future"));
}

public void testLongToInstant() {
assertThat(toInstant(0), is(Instant.EPOCH));
assertThat(toInstant(1), is(Instant.EPOCH.plusNanos(1)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public static Iterable<Object[]> parameters() {
suppliers,
"ToDatetimeFromDateNanosEvaluator[field=" + read + "]",
DataType.DATETIME,
i -> DateUtils.toMilliSeconds(DateUtils.toLong(i)),
DateUtils::toLongMillis,
emptyList()
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public static Iterable<Object[]> parameters() {
TestCaseSupplier.forUnaryBoolean(suppliers, evaluatorName.apply("Boolean"), DataType.LONG, b -> b ? 1L : 0L, List.of());

// datetimes
TestCaseSupplier.forUnaryDatetime(suppliers, read, DataType.LONG, Instant::toEpochMilli, List.of());
TestCaseSupplier.forUnaryDatetime(suppliers, read, DataType.LONG, DateUtils::toLongMillis, List.of());
TestCaseSupplier.forUnaryDateNanos(suppliers, read, DataType.LONG, DateUtils::toLong, List.of());
// random strings that don't look like a long
TestCaseSupplier.forUnaryStrings(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public static Iterable<Object[]> parameters() {
suppliers,
"ToStringFromDatetimeEvaluator[field=" + read + "]",
DataType.KEYWORD,
i -> new BytesRef(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.formatMillis(i.toEpochMilli())),
i -> new BytesRef(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.formatMillis(DateUtils.toLongMillis(i))),
List.of()
);
TestCaseSupplier.forUnaryDateNanos(
Expand Down