Skip to content

Eschew leniency when parsing time zones #77267

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

Merged
merged 8 commits into from
Sep 15, 2021
Merged

Eschew leniency when parsing time zones #77267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
980a28d
eschew leniency when parsing time zones
not-napoleon Sep 3, 2021
6380825
Merge branch 'master' into 76415-parsing-time-zones
not-napoleon Sep 3, 2021
c729086
remove useless comment
not-napoleon Sep 13, 2021
0948bb6
Merge branch 'master' into 76415-parsing-time-zones
not-napoleon Sep 14, 2021
574f9ee
add some more tests
not-napoleon Sep 14, 2021
f760b6e
Merge branch 'master' into 76415-parsing-time-zones
elasticmachine Sep 14, 2021
c0328ef
BWC dance
not-napoleon Sep 14, 2021
91e3007
fix typo in yaml
not-napoleon Sep 14, 2021
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
95 changes: 95 additions & 0 deletions ...rc/yamlRestTest/resources/rest-api-spec/test/search.aggregation/49_range_timezone_bug.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
setup:
Copy link
Member Author

@not-napoleon not-napoleon Sep 3, 2021

Choose a reason for hiding this comment

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

This could be in the same file as the rest of the range agg tests, but I'm refactoring that file in a different branch, and wanted to save myself a merge conflict.

- do:
indices.create:
index: test
body:
settings:
number_of_replicas: 0
mappings:
properties:
mydate:
type: date
format: "uuuu-MM-dd'T'HH:mm:ss.SSSSSSSSSZZZZZ"

- do:
cluster.health:
wait_for_status: green

- do:
index:
index: test
id: 1
body: { "mydate": "2021-08-12T01:00:00.000000000+02:00" }

- do:
indices.refresh: {}

---
"respect offsets in range bounds":
- skip:
version: " - 7.99.99"
reason: "Fixed in 7.16 (backport pending)"
- do:
search:
rest_total_hits_as_int: true
body: {
"query": {
"match_all": {}
},
"aggregations": {
"myagg": {
"date_range": {
"field": "mydate",
"ranges": [
{
"from": "2021-08-12T00:00:00.000000000+02:00",
"to": "2021-08-12T02:00:00.000000000+02:00"
}
]
}
}
}
}
- match: { hits.total: 1 }
- length: { aggregations.myagg.buckets: 1 }
- match: { aggregations.myagg.buckets.0.from_as_string: "2021-08-11T22:00:00.000000000Z" }
- match: { aggregations.myagg.buckets.0.from: 1628719200000 }
Copy link
Contributor

@pgomulka pgomulka Sep 8, 2021

Choose a reason for hiding this comment

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

I wonder if we could add human readable date in a comment to help with readability?

- match: { aggregations.myagg.buckets.0.to_as_string: "2021-08-12T00:00:00.000000000Z" }
- match: { aggregations.myagg.buckets.0.to: 1628726400000 }
- match: { aggregations.myagg.buckets.0.doc_count: 1 }

---
"offsets and timezones play nicely together":
- skip:
version: " - 7.99.99"
reason: "Fixed in 7.16 (backport pending)"
- do:
search:
rest_total_hits_as_int: true
body: {
"query": {
"match_all": {}
},
"aggregations": {
"myagg": {
"date_range": {
"time_zone": "America/New_York",
"field": "mydate",
"ranges": [
{
"from": "2021-08-12T00:00:00.000000000+02:00",
"to": "2021-08-12T02:00:00.000000000+02:00"
}
]
}
}
}
}
- match: { hits.total: 1 }
- length: { aggregations.myagg.buckets: 1 }
- match: { aggregations.myagg.buckets.0.from_as_string: "2021-08-11T18:00:00.000000000-04:00" }
- match: { aggregations.myagg.buckets.0.from: 1628719200000 }
- match: { aggregations.myagg.buckets.0.to_as_string: "2021-08-11T20:00:00.000000000-04:00" }
- match: { aggregations.myagg.buckets.0.to: 1628726400000 }
- match: { aggregations.myagg.buckets.0.doc_count: 1 }

4 changes: 3 additions & 1 deletion server/src/main/java/org/elasticsearch/common/time/JavaDateMathParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,9 @@ private Instant parseDateTime(String value, ZoneId timeZone, boolean roundUpIfNo
return DateFormatters.from(formatter.parse(value)).toInstant();
} else {
TemporalAccessor accessor = formatter.parse(value);
ZoneId zoneId = TemporalQueries.zone().queryFrom(accessor);
Copy link
Member Author

@not-napoleon not-napoleon Sep 3, 2021

Choose a reason for hiding this comment

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

The docs describe the zone() query as "A lenient query for the ZoneId, falling back to the ZoneOffset", and how do we feel about leniency?

Copy link
Contributor

@pgomulka pgomulka Sep 7, 2021

Choose a reason for hiding this comment

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

while this would fix the use case where a mapping expects an offset but the problem will re-highlight itself whe na mapping is using zoneId?

 public void testParseOffset() {
        // Format string from #76415
        DocValueFormat.DateTime parsesZone = new DocValueFormat.DateTime(
            DateFormatter.forPattern("uuuu-MM-dd'T'HH:mm:ss.SSSSSSSSSVV"),
            ZoneOffset.UTC,
            Resolution.MILLISECONDS
        );
        long expected = 1628719200000L;
        ZonedDateTime sample = ZonedDateTime.of(2021, 8, 12, 0, 0, 0, 0, ZoneId.ofOffset("", ZoneOffset.ofHours(2)));
        assertEquals("GUARD: wrong initial millis", expected, sample.toEpochSecond() * 1000);
        //assertEquals("GUARD: wrong initial string", "2021-08-12T00:00:00.000000000+02:00", parsesZone.format(expected));
        long actualMillis = parsesZone.parseLong(
            "2021-08-12T00:00:00.000000000CET",
            false,
            () -> { throw new UnsupportedOperationException("don't use now"); }
        );
        assertEquals(expected, actualMillis);
    }

Copy link
Contributor

@pgomulka pgomulka Sep 7, 2021

Choose a reason for hiding this comment

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

actually I just tested this and it would work..

Copy link
Member Author

@not-napoleon not-napoleon Sep 7, 2021

Choose a reason for hiding this comment

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

That's a good test though, we should add it to the suite. Do you mind if I just copy it in?

Copy link
Contributor

@pgomulka pgomulka Sep 7, 2021

Choose a reason for hiding this comment

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

I don't mind it at all :) it is a copy of your test with just patterns changed :D
problem is that it is prone to DST changes. ZoneId.ofOffset("", ZoneOffset.ofHours(2))); and 2021-08-12T00:00:00.000000000+02:00 will have to account for this

// Use the offset if provided, otherwise fall back to the zone, or null.
ZoneOffset offset = TemporalQueries.offset().queryFrom(accessor);
ZoneId zoneId = offset == null ? TemporalQueries.zoneId().queryFrom(accessor) : ZoneId.ofOffset("", offset);
if (zoneId != null) {
timeZone = zoneId;
}
Expand Down
42 changes: 42 additions & 0 deletions server/src/test/java/org/elasticsearch/search/DocValueFormatTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import java.time.ZoneId;
import java.time.ZoneOffset;
import java.time.ZonedDateTime;
import java.util.ArrayList;
import java.util.List;

Expand Down Expand Up @@ -345,4 +346,45 @@ public void testDateTimeWithTimezone() {
tokyo.parseLong(tokyo.format(millis), false, () -> { throw new UnsupportedOperationException("don't use now"); })
);
}

/**
* This is a regression test for https://github.com/elastic/elasticsearch/issues/76415
*/
public void testParseOffset() {
DocValueFormat.DateTime parsesZone = new DocValueFormat.DateTime(
DateFormatter.forPattern("uuuu-MM-dd'T'HH:mm:ss.SSSSSSSSSZZZZZ"),
ZoneOffset.UTC,
Resolution.MILLISECONDS
);
long expected = 1628719200000L;
ZonedDateTime sample = ZonedDateTime.of(2021, 8, 12, 0, 0, 0, 0, ZoneId.ofOffset("", ZoneOffset.ofHours(2)));
assertEquals("GUARD: wrong initial millis", expected, sample.toEpochSecond() * 1000);
long actualMillis = parsesZone.parseLong(
"2021-08-12T00:00:00.000000000+02:00",
false,
() -> { throw new UnsupportedOperationException("don't use now"); }
);
assertEquals(expected, actualMillis);
}

/**
* Make sure fixing 76415 doesn't break parsing zone strings
*/
public void testParseZone() {
DocValueFormat.DateTime parsesZone = new DocValueFormat.DateTime(
DateFormatter.forPattern("uuuu-MM-dd'T'HH:mm:ss.SSSSSSSSSVV"),
ZoneOffset.UTC,
Resolution.MILLISECONDS
);
long expected = 1628719200000L;
ZonedDateTime sample = ZonedDateTime.of(2021, 8, 12, 0, 0, 0, 0, ZoneId.ofOffset("", ZoneOffset.ofHours(2)));
assertEquals("GUARD: wrong initial millis", expected, sample.toEpochSecond() * 1000);
//assertEquals("GUARD: wrong initial string", "2021-08-12T00:00:00.000000000+02:00", parsesZone.format(expected));
long actualMillis = parsesZone.parseLong(
"2021-08-12T00:00:00.000000000CET",
false,
() -> { throw new UnsupportedOperationException("don't use now"); }
);
assertEquals(expected, actualMillis);
}
}