From 5d3f8e52d49f69519293384011c02f13314a6778 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Tue, 8 Aug 2017 15:33:05 +0300 Subject: [PATCH 1/3] Epoch millis and second formats accept float implicitly The coerce parameter is implicity true for the epoch millis DateFormater. It is not defined for other date formaters. This extends the current "coerce" from numbers to strings for all dates. See: #14641 --- .../org/elasticsearch/common/joda/Joda.java | 6 +++-- .../deps/joda/SimpleJodaTests.java | 20 ++++++++++++++ .../index/mapper/DateFieldMapperTests.java | 27 +++++++++++++++++++ .../index/mapper/RangeFieldMapperTests.java | 5 ++++ .../aggregations/bucket/DateRangeIT.java | 25 +++++++++-------- 5 files changed, 70 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/joda/Joda.java b/core/src/main/java/org/elasticsearch/common/joda/Joda.java index c9eaa9ab3aa2e..d6acfb3871825 100644 --- a/core/src/main/java/org/elasticsearch/common/joda/Joda.java +++ b/core/src/main/java/org/elasticsearch/common/joda/Joda.java @@ -42,6 +42,7 @@ import java.io.IOException; import java.io.Writer; +import java.math.BigDecimal; import java.util.Locale; public class Joda { @@ -331,7 +332,8 @@ public int estimateParsedLength() { @Override public int parseInto(DateTimeParserBucket bucket, String text, int position) { boolean isPositive = text.startsWith("-") == false; - boolean isTooLong = text.length() > estimateParsedLength(); + int firstDotIndex = text.indexOf((int)'.'); + boolean isTooLong = (firstDotIndex == -1 ? text.length() : firstDotIndex) > estimateParsedLength(); if (bucket.getZone() != DateTimeZone.UTC) { String format = hasMilliSecondPrecision ? "epoch_millis" : "epoch_second"; @@ -342,7 +344,7 @@ public int parseInto(DateTimeParserBucket bucket, String text, int position) { int factor = hasMilliSecondPrecision ? 1 : 1000; try { - long millis = Long.valueOf(text) * factor; + long millis = new BigDecimal(text).longValue() * factor; DateTime dt = new DateTime(millis, DateTimeZone.UTC); bucket.saveField(DateTimeFieldType.year(), dt.getYear()); bucket.saveField(DateTimeFieldType.monthOfYear(), dt.getMonthOfYear()); diff --git a/core/src/test/java/org/elasticsearch/deps/joda/SimpleJodaTests.java b/core/src/test/java/org/elasticsearch/deps/joda/SimpleJodaTests.java index b99dd8e835384..a40b808806b9e 100644 --- a/core/src/test/java/org/elasticsearch/deps/joda/SimpleJodaTests.java +++ b/core/src/test/java/org/elasticsearch/deps/joda/SimpleJodaTests.java @@ -262,6 +262,26 @@ public void testThatEpochsCanBeParsed() { } } + public void testThatFloatEpochsCanBeParsed() { + + long millisFromEpoch = randomNonNegativeLong(); + + String epochFloatValue = String.format(Locale.US, "%d.%d", millisFromEpoch, randomNonNegativeLong()); + FormatDateTimeFormatter formatter = Joda.forPattern("epoch_millis"); + + DateTime dateTime = formatter.parser().parseDateTime(epochFloatValue); + + assertEquals(dateTime.getMillis(), millisFromEpoch); + + + epochFloatValue = String.format(Locale.US, "%d.%d", millisFromEpoch / 1000, randomNonNegativeLong()); + formatter = Joda.forPattern("epoch_second"); + + dateTime = formatter.parser().parseDateTime(epochFloatValue); + + assertEquals(dateTime.getMillis(), millisFromEpoch / 1000 * 1000); + } + public void testThatNegativeEpochsCanBeParsed() { // problem: negative epochs can be arbitrary in size... boolean parseMilliSeconds = randomBoolean(); diff --git a/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java index 747e4b498da95..0d0064d9c0f53 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java @@ -35,6 +35,7 @@ import java.io.IOException; import java.util.Collection; +import java.util.Locale; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.notNullValue; @@ -214,6 +215,32 @@ public void testChangeFormat() throws IOException { assertEquals(1457654400000L, pointField.numericValue().longValue()); } + public void testFloatEpochFormat() throws IOException { + String mapping = XContentFactory.jsonBuilder().startObject().startObject("type") + .startObject("properties").startObject("field").field("type", "date") + .field("format", "epoch_millis").endObject().endObject() + .endObject().endObject().string(); + + DocumentMapper mapper = parser.parse("type", new CompressedXContent(mapping)); + + assertEquals(mapping, mapper.mappingSource().toString()); + + long millisFromEpoch = randomNonNegativeLong(); + String epochFloatValue = String.format(Locale.US, "%d.%d", millisFromEpoch, randomNonNegativeLong()); + + ParsedDocument doc = mapper.parse(SourceToParse.source("test", "type", "1", XContentFactory.jsonBuilder() + .startObject() + .field("field", epochFloatValue) + .endObject() + .bytes(), + XContentType.JSON)); + + IndexableField[] fields = doc.rootDoc().getFields("field"); + assertEquals(2, fields.length); + IndexableField pointField = fields[0]; + assertEquals(millisFromEpoch, pointField.numericValue().longValue()); + } + public void testChangeLocale() throws IOException { String mapping = XContentFactory.jsonBuilder().startObject().startObject("type") .startObject("properties").startObject("field").field("type", "date").field("locale", "fr").endObject().endObject() diff --git a/core/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java index b8aff721a6851..7f1f2457eceb0 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java @@ -245,6 +245,11 @@ public void doTestCoerce(String type) throws IOException { IndexableField pointField = fields[1]; assertEquals(2, pointField.fieldType().pointDimensionCount()); + // date_range ignores the coerce parameter and epoch_millis date format truncates floats (see issue: #14641) + if (type.equals("date_range")) { + return; + } + mapping = XContentFactory.jsonBuilder().startObject().startObject("type") .startObject("properties").startObject("field").field("type", type).field("coerce", false).endObject().endObject() .endObject().endObject(); diff --git a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java index d54d8cd10f036..f47e59640073d 100644 --- a/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java +++ b/core/src/test/java/org/elasticsearch/search/aggregations/bucket/DateRangeIT.java @@ -1014,17 +1014,20 @@ public void testRangeWithFormatNumericValue() throws Exception { assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); - // however, e-notation should and fractional parts provided as string - // should be parsed and error if not compatible - Exception e = expectThrows(Exception.class, () -> client().prepareSearch(indexName).setSize(0) - .addAggregation(dateRange("date_range").field("date").addRange("1.0e3", "3.0e3").addRange("3.0e3", "4.0e3")).get()); - assertThat(e.getCause(), instanceOf(ElasticsearchParseException.class)); - assertEquals("failed to parse date field [1.0e3] with format [epoch_second]", e.getCause().getMessage()); - - e = expectThrows(Exception.class, () -> client().prepareSearch(indexName).setSize(0) - .addAggregation(dateRange("date_range").field("date").addRange("1000.123", "3000.8").addRange("3000.8", "4000.3")).get()); - assertThat(e.getCause(), instanceOf(ElasticsearchParseException.class)); - assertEquals("failed to parse date field [1000.123] with format [epoch_second]", e.getCause().getMessage()); + // also e-notation and floats provided as string also be truncated (see: #14641) + searchResponse = client().prepareSearch(indexName).setSize(0) + .addAggregation(dateRange("date_range").field("date").addRange("1.0e3", "3.0e3").addRange("3.0e3", "4.0e3")).get(); + assertThat(searchResponse.getHits().getTotalHits(), equalTo(3L)); + buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); + assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); + + searchResponse = client().prepareSearch(indexName).setSize(0) + .addAggregation(dateRange("date_range").field("date").addRange("1000.123", "3000.8").addRange("3000.8", "4000.3")).get(); + assertThat(searchResponse.getHits().getTotalHits(), equalTo(3L)); + buckets = checkBuckets(searchResponse.getAggregations().get("date_range"), "date_range", 2); + assertBucket(buckets.get(0), 2L, "1000-3000", 1000000L, 3000000L); + assertBucket(buckets.get(1), 1L, "3000-4000", 3000000L, 4000000L); // using different format should work when to/from is compatible with // format in aggregation From daac500b2b8f3f497dc467f29dfada82ba6f20ea Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 10 Aug 2017 11:03:42 +0300 Subject: [PATCH 2/3] Better float timestamp truncation tests --- .../deps/joda/SimpleJodaTests.java | 32 ++++++++----------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/deps/joda/SimpleJodaTests.java b/core/src/test/java/org/elasticsearch/deps/joda/SimpleJodaTests.java index a40b808806b9e..257ebef9a9477 100644 --- a/core/src/test/java/org/elasticsearch/deps/joda/SimpleJodaTests.java +++ b/core/src/test/java/org/elasticsearch/deps/joda/SimpleJodaTests.java @@ -260,26 +260,10 @@ public void testThatEpochsCanBeParsed() { } else { assertThat(dateTime.getMillisOfSecond(), is(0)); } - } - - public void testThatFloatEpochsCanBeParsed() { - - long millisFromEpoch = randomNonNegativeLong(); - - String epochFloatValue = String.format(Locale.US, "%d.%d", millisFromEpoch, randomNonNegativeLong()); - FormatDateTimeFormatter formatter = Joda.forPattern("epoch_millis"); - - DateTime dateTime = formatter.parser().parseDateTime(epochFloatValue); - - assertEquals(dateTime.getMillis(), millisFromEpoch); - - epochFloatValue = String.format(Locale.US, "%d.%d", millisFromEpoch / 1000, randomNonNegativeLong()); - formatter = Joda.forPattern("epoch_second"); - - dateTime = formatter.parser().parseDateTime(epochFloatValue); - - assertEquals(dateTime.getMillis(), millisFromEpoch / 1000 * 1000); + // test floats get truncated + String epochFloatValue = String.format(Locale.US, "%d.%d", dateTime.getMillis() / (parseMilliSeconds ? 1L : 1000L), randomNonNegativeLong()); + assertThat(formatter.parser().parseDateTime(epochFloatValue).getMillis(), is(dateTime.getMillis())); } public void testThatNegativeEpochsCanBeParsed() { @@ -301,16 +285,26 @@ public void testThatNegativeEpochsCanBeParsed() { assertThat(dateTime.getSecondOfMinute(), is(20)); } + // test floats get truncated + String epochFloatValue = String.format(Locale.US, "%d.%d", dateTime.getMillis() / (parseMilliSeconds ? 1L : 1000L), randomNonNegativeLong()); + assertThat(formatter.parser().parseDateTime(epochFloatValue).getMillis(), is(dateTime.getMillis())); + // every negative epoch must be parsed, no matter if exact the size or bigger if (parseMilliSeconds) { formatter.parser().parseDateTime("-100000000"); formatter.parser().parseDateTime("-999999999999"); formatter.parser().parseDateTime("-1234567890123"); formatter.parser().parseDateTime("-1234567890123456789"); + + formatter.parser().parseDateTime("-1234567890123.9999"); + formatter.parser().parseDateTime("-1234567890123456789.9999"); } else { formatter.parser().parseDateTime("-100000000"); formatter.parser().parseDateTime("-1234567890"); formatter.parser().parseDateTime("-1234567890123456"); + + formatter.parser().parseDateTime("-1234567890.9999"); + formatter.parser().parseDateTime("-1234567890123456.9999"); } } From 5d8229f9a4f4f01a0ad8feedb2d27f55ee4a4a29 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 10 Aug 2017 15:33:20 +0300 Subject: [PATCH 3/3] Date Field Mapper and Integ tests improvments --- .../org/elasticsearch/common/joda/Joda.java | 2 +- .../index/mapper/DateFieldMapperTests.java | 6 +-- .../index/mapper/RangeFieldMapperTests.java | 37 ++++++++----------- .../search/query/SearchQueryIT.java | 10 +++-- 4 files changed, 27 insertions(+), 28 deletions(-) diff --git a/core/src/main/java/org/elasticsearch/common/joda/Joda.java b/core/src/main/java/org/elasticsearch/common/joda/Joda.java index d6acfb3871825..832043af63e63 100644 --- a/core/src/main/java/org/elasticsearch/common/joda/Joda.java +++ b/core/src/main/java/org/elasticsearch/common/joda/Joda.java @@ -332,7 +332,7 @@ public int estimateParsedLength() { @Override public int parseInto(DateTimeParserBucket bucket, String text, int position) { boolean isPositive = text.startsWith("-") == false; - int firstDotIndex = text.indexOf((int)'.'); + int firstDotIndex = text.indexOf('.'); boolean isTooLong = (firstDotIndex == -1 ? text.length() : firstDotIndex) > estimateParsedLength(); if (bucket.getZone() != DateTimeZone.UTC) { diff --git a/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java index 0d0064d9c0f53..b728c44cc650b 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/DateFieldMapperTests.java @@ -225,8 +225,8 @@ public void testFloatEpochFormat() throws IOException { assertEquals(mapping, mapper.mappingSource().toString()); - long millisFromEpoch = randomNonNegativeLong(); - String epochFloatValue = String.format(Locale.US, "%d.%d", millisFromEpoch, randomNonNegativeLong()); + double epochFloatMillisFromEpoch = (randomDouble() * 2 - 1) * 1000000; + String epochFloatValue = String.format(Locale.US, "%f", epochFloatMillisFromEpoch); ParsedDocument doc = mapper.parse(SourceToParse.source("test", "type", "1", XContentFactory.jsonBuilder() .startObject() @@ -238,7 +238,7 @@ public void testFloatEpochFormat() throws IOException { IndexableField[] fields = doc.rootDoc().getFields("field"); assertEquals(2, fields.length); IndexableField pointField = fields[0]; - assertEquals(millisFromEpoch, pointField.numericValue().longValue()); + assertEquals((long)epochFloatMillisFromEpoch, pointField.numericValue().longValue()); } public void testChangeLocale() throws IOException { diff --git a/core/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java b/core/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java index 7f1f2457eceb0..7bae878b92459 100644 --- a/core/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java +++ b/core/src/test/java/org/elasticsearch/index/mapper/RangeFieldMapperTests.java @@ -246,28 +246,23 @@ public void doTestCoerce(String type) throws IOException { assertEquals(2, pointField.fieldType().pointDimensionCount()); // date_range ignores the coerce parameter and epoch_millis date format truncates floats (see issue: #14641) - if (type.equals("date_range")) { - return; + if (type.equals("date_range") == false) { + + mapping = XContentFactory.jsonBuilder().startObject().startObject("type").startObject("properties").startObject("field") + .field("type", type).field("coerce", false).endObject().endObject().endObject().endObject(); + DocumentMapper mapper2 = parser.parse("type", new CompressedXContent(mapping.string())); + + assertEquals(mapping.string(), mapper2.mappingSource().toString()); + + ThrowingRunnable runnable = () -> mapper2 + .parse(SourceToParse.source( + "test", "type", "1", XContentFactory.jsonBuilder().startObject().startObject("field") + .field(getFromField(), "5.2").field(getToField(), "10").endObject().endObject().bytes(), + XContentType.JSON)); + MapperParsingException e = expectThrows(MapperParsingException.class, runnable); + assertThat(e.getCause().getMessage(), anyOf(containsString("passed as String"), containsString("failed to parse date"), + containsString("is not an IP string literal"))); } - - mapping = XContentFactory.jsonBuilder().startObject().startObject("type") - .startObject("properties").startObject("field").field("type", type).field("coerce", false).endObject().endObject() - .endObject().endObject(); - DocumentMapper mapper2 = parser.parse("type", new CompressedXContent(mapping.string())); - - assertEquals(mapping.string(), mapper2.mappingSource().toString()); - - ThrowingRunnable runnable = () -> mapper2.parse(SourceToParse.source("test", "type", "1", XContentFactory.jsonBuilder() - .startObject() - .startObject("field") - .field(getFromField(), "5.2") - .field(getToField(), "10") - .endObject() - .endObject().bytes(), - XContentType.JSON)); - MapperParsingException e = expectThrows(MapperParsingException.class, runnable); - assertThat(e.getCause().getMessage(), anyOf(containsString("passed as String"), - containsString("failed to parse date"), containsString("is not an IP string literal"))); } @Override diff --git a/core/src/test/java/org/elasticsearch/search/query/SearchQueryIT.java b/core/src/test/java/org/elasticsearch/search/query/SearchQueryIT.java index 90523f0051dc0..06f6e20d3836f 100644 --- a/core/src/test/java/org/elasticsearch/search/query/SearchQueryIT.java +++ b/core/src/test/java/org/elasticsearch/search/query/SearchQueryIT.java @@ -1699,11 +1699,15 @@ public void testDateProvidedAsNumber() throws ExecutionException, InterruptedExc assertAcked(client().admin().indices().preparePutMapping("test").setType("type").setSource("field", "type=date,format=epoch_millis").get()); indexRandom(true, client().prepareIndex("test", "type", "1").setSource("field", -1000000000001L), client().prepareIndex("test", "type", "2").setSource("field", -1000000000000L), - client().prepareIndex("test", "type", "3").setSource("field", -999999999999L)); + client().prepareIndex("test", "type", "3").setSource("field", -999999999999L), + client().prepareIndex("test", "type", "4").setSource("field", -1000000000001.0123456789), + client().prepareIndex("test", "type", "5").setSource("field", -1000000000000.0123456789), + client().prepareIndex("test", "type", "6").setSource("field", -999999999999.0123456789)); - assertHitCount(client().prepareSearch("test").setSize(0).setQuery(rangeQuery("field").lte(-1000000000000L)).get(), 2); - assertHitCount(client().prepareSearch("test").setSize(0).setQuery(rangeQuery("field").lte(-999999999999L)).get(), 3); + assertHitCount(client().prepareSearch("test").setSize(0).setQuery(rangeQuery("field").lte(-1000000000000L)).get(), 4); + assertHitCount(client().prepareSearch("test").setSize(0).setQuery(rangeQuery("field").lte(-999999999999L)).get(), 6); + } public void testRangeQueryWithTimeZone() throws Exception {