Fix bug when formatting epoch dates (#73955) (#74139)

not-napoleon · web-flow · commit 2e585a85ef1f · 2021-06-15T13:15:33.000-04:00
Date based aggregations accept a timezone, which gets applied to both the bucketing logic and the formatter. This is usually what you want, but in the case of date formats where a timezone doesn't make any sense, it can create problems. In particular, our formatting logic and our parsing logic were doing different things for epoch_second and epoch_millis formats with time zones. This led to a problem on composite where we'd return an after key for the last bucket that would parse to a time before the last bucket, so instead of correctly returning an empty response to indicate the end of the aggregation, we'd keep returning the same last page of data.
diff --git a/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/230_composite.yml b/rest-api-spec/src/yamlRestTest/resources/rest-api-spec/test/search.aggregation/230_composite.yml
@@ -941,6 +941,76 @@ setup:
   - match: { aggregations.test.buckets.0.key.date: "2017-10-21T00:00:00.000-02:00" }
   - match: { aggregations.test.buckets.0.doc_count: 2 }
 
+---
+"date_histogram with time_zone epoch format":
+# Same as above, but with a different format.  We had a bug about this specifically:
+# https://github.com/elastic/elasticsearch/issues/68963
+  - skip:
+      version: " - 7.6.0"
+      reason: Fixed in 7.6.0
+  - do:
+      index:
+        index:   test
+        id:      7
+        body:    { "date": "2017-10-22T01:00:00" }
+        refresh: true
+  - do:
+      search:
+        index: test
+        body:
+          aggregations:
+            test:
+              composite:
+                sources: [
+                {
+                  "date": {
+                    "date_histogram": {
+                      "field": "date",
+                      "calendar_interval": "1d",
+                      "time_zone": "-02:00",
+                      "format": "epoch_second"
+                    }
+                  }
+                }
+              ]
+
+  - match: { hits.total.value: 7 }
+  - match: { hits.total.relation: eq }
+  - length: { aggregations.test.buckets: 2 }
+  - match: { aggregations.test.buckets.0.key.date: "1508464800" }
+  - match: { aggregations.test.buckets.0.doc_count: 1 }
+  - match: { aggregations.test.buckets.1.key.date: "1508551200" }
+  - match: { aggregations.test.buckets.1.doc_count: 2 }
+
+  - do:
+      search:
+        index: test
+        body:
+          aggregations:
+            test:
+              composite:
+                after: {
+                  date: "1508464800"
+                }
+                sources: [
+                {
+                  "date": {
+                    "date_histogram": {
+                      "field": "date",
+                      "calendar_interval": "1d",
+                      "time_zone": "-02:00",
+                      "format": "epoch_second"
+                    }
+                  }
+                }
+            ]
+
+  - match: { hits.total.value: 7 }
+  - match: { hits.total.relation: eq }
+  - length: { aggregations.test.buckets: 1 }
+  - match: { aggregations.test.buckets.0.key.date: "1508551200" }
+  - match: { aggregations.test.buckets.0.doc_count: 2 }
+
 ---
 "date_histogram on date_nanos":
   - skip:
diff --git a/server/src/main/java/org/elasticsearch/search/DocValueFormat.java b/server/src/main/java/org/elasticsearch/search/DocValueFormat.java
@@ -232,9 +232,9 @@ public DateTime(DateFormatter formatter, ZoneId timeZone, DateFieldMapper.Resolu
         }
 
         private DateTime(DateFormatter formatter, ZoneId timeZone, DateFieldMapper.Resolution resolution, boolean formatSortValues) {
-            this.formatter = formatter;
             this.timeZone = Objects.requireNonNull(timeZone);
-            this.parser = formatter.toDateMathParser();
+            this.formatter = formatter.withZone(timeZone);
+            this.parser = this.formatter.toDateMathParser();
             this.resolution = resolution;
             this.formatSortValues = formatSortValues;
         }
diff --git a/server/src/test/java/org/elasticsearch/search/DocValueFormatTests.java b/server/src/test/java/org/elasticsearch/search/DocValueFormatTests.java
@@ -20,6 +20,7 @@
 import org.elasticsearch.index.mapper.DateFieldMapper.Resolution;
 import org.elasticsearch.test.ESTestCase;
 
+import java.time.ZoneId;
 import java.time.ZoneOffset;
 import java.util.ArrayList;
 import java.util.List;
@@ -245,4 +246,103 @@ public void testBadIp() {
         assertThat(e.getMessage(), containsString("mapping"));
         assertThat(e.getMessage(), containsString("IP address"));
     }
+
+    /**
+     * <p>Test that if we format a datetime using the `epoch_second` format, we can then parse the result
+     * back into the same value we started with, for all timezones</p>
+     *
+     * <p>"Why would you put a timezone on epoch_seconds? it doesn't make sense" you might be asking.
+     * I was.  The key to remember here is that we use the same time zone parameter for date histogram
+     * bucket generation and formatting.  So, while asking for (e.g.) epoch_seconds in New York time
+     * is nonsensical, asking for day-buckets in New York time with the keys formatted in epoch_seconds
+     * is pretty standard.</p>
+     *
+     * <p>This test validates that if someone does this in composite, we can then parse the after key
+     * we generated into the correct value.  Parsing also happens on missing values.</p>
+     */
+    public void testParseEpochSecondsTimezone() {
+        ZoneId zone = randomZone();
+        DocValueFormat.DateTime formatter = new DocValueFormat.DateTime(
+            DateFormatter.forPattern("epoch_second"),
+            zone,
+            Resolution.MILLISECONDS
+        );
+        long millis = randomNonNegativeLong();
+        // Convert to seconds
+        millis -= (millis % 1000);
+        assertEquals(
+            "failed formatting for tz " + zone,
+            millis,
+            formatter.parseLong(formatter.format(millis), false, () -> { throw new UnsupportedOperationException("don't use now"); })
+        );
+    }
+
+    public void testParseEpochMillisTimezone() {
+        ZoneId zone = randomZone();
+        DocValueFormat.DateTime formatter = new DocValueFormat.DateTime(
+            DateFormatter.forPattern("epoch_millis"),
+            zone,
+            Resolution.MILLISECONDS
+        );
+        long millis = randomNonNegativeLong();
+        assertEquals(
+            "failed formatting for tz " + zone,
+            millis,
+            formatter.parseLong(formatter.format(millis), false, () -> { throw new UnsupportedOperationException("don't use now"); })
+        );
+    }
+
+
+    public void testDateHMSTimezone() {
+        DocValueFormat.DateTime tokyo = new DocValueFormat.DateTime(
+            DateFormatter.forPattern("date_hour_minute_second"),
+            ZoneOffset.ofHours(9),
+            Resolution.MILLISECONDS
+        );
+        DocValueFormat.DateTime utc = new DocValueFormat.DateTime(
+            DateFormatter.forPattern("date_hour_minute_second"),
+            ZoneOffset.UTC,
+            Resolution.MILLISECONDS
+        );
+        long millis = 1622567918000L;
+        assertEquals("2021-06-01T17:18:38", utc.format(millis));
+        assertEquals("2021-06-02T02:18:38", tokyo.format(millis));
+        assertEquals(
+            "couldn't parse UTC",
+            millis,
+            utc.parseLong(utc.format(millis), false, () -> { throw new UnsupportedOperationException("don't use now"); })
+        );
+        assertEquals(
+            "couldn't parse Tokyo",
+            millis,
+            tokyo.parseLong(tokyo.format(millis), false, () -> { throw new UnsupportedOperationException("don't use now"); })
+        );
+    }
+
+    public void testDateTimeWithTimezone() {
+
+        DocValueFormat.DateTime tokyo = new DocValueFormat.DateTime(
+            DateFormatter.forPattern("basic_date_time_no_millis"),
+            ZoneOffset.ofHours(9),
+            Resolution.MILLISECONDS
+        );
+        DocValueFormat.DateTime utc = new DocValueFormat.DateTime(
+            DateFormatter.forPattern("basic_date_time_no_millis"),
+            ZoneOffset.UTC,
+            Resolution.MILLISECONDS
+        );
+        long millis = 1622567918000L;
+        assertEquals("20210601T171838Z", utc.format(millis));
+        assertEquals("20210602T021838+09:00", tokyo.format(millis));
+        assertEquals(
+            "couldn't parse UTC",
+            millis,
+            utc.parseLong(utc.format(millis), false, () -> { throw new UnsupportedOperationException("don't use now"); })
+        );
+        assertEquals(
+            "couldn't parse Tokyo",
+            millis,
+            tokyo.parseLong(tokyo.format(millis), false, () -> { throw new UnsupportedOperationException("don't use now"); })
+        );
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -232,9 +232,9 @@ public DateTime(DateFormatter formatter, ZoneId timeZone, DateFieldMapper.Resolu`
`232`	`232`	`}`
`233`	`233`
`234`	`234`	`private DateTime(DateFormatter formatter, ZoneId timeZone, DateFieldMapper.Resolution resolution, boolean formatSortValues) {`
`235`		`- this.formatter = formatter;`
`236`	`235`	`this.timeZone = Objects.requireNonNull(timeZone);`
`237`		`- this.parser = formatter.toDateMathParser();`
	`236`	`+ this.formatter = formatter.withZone(timeZone);`
	`237`	`+ this.parser = this.formatter.toDateMathParser();`
`238`	`238`	`this.resolution = resolution;`
`239`	`239`	`this.formatSortValues = formatSortValues;`
`240`	`240`	`}`