Improve error handling for epoch format parser with time zone (#22621) #23689
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
|
Thanks for the PR, after a first quick glance this already looks good, but can you add some unit tests that trigger those exceptions? |
There was a problem hiding this comment.
@sneivandt thanks, this looks good and will only need a bit of reformatting to pass our checkstyle rules before I start a CI run.
| if (bucket.getZone() != DateTimeZone.UTC) { | ||
| String format = hasMilliSecondPrecision ? "epoch_millis" : "epoch_second"; | ||
| throw new IllegalArgumentException("time_zone must be UTC for format [" + format + "]"); | ||
| } |
There was a problem hiding this comment.
nit: can you move up the else block to this line?
| FormatDateTimeFormatter formatter = new FormatDateTimeFormatter("epoch_seconds", new DateTimeFormatterBuilder().append(new Joda.EpochTimeParser(false)).toFormatter().withZone(zone), Locale.ROOT); | ||
| try { | ||
| formatter.parser().parseDateTime("1433144433655"); | ||
| fail("Expected IllegalArgumentException"); |
There was a problem hiding this comment.
Just as a note, LuceneTestCase (which we inherit from) offers a nice helper to shorten these try/catch constructs, it goes something like this:
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> ...do Something...);
assertThat(... some assertions about e...);
I see that the rest of this test also still uses try/catch for most exception tests. This is okay as it is, I just wanted to mention it.
| @@ -314,6 +314,28 @@ public void testForInvalidDatesInEpochMillis() { | |||
| } | |||
| } | |||
|
|
|||
| public void testForInvalidTimeZoneWithEpochSeconds() { | |||
| DateTimeZone zone = DateTimeZone.forOffsetHours(1); | |||
| FormatDateTimeFormatter formatter = new FormatDateTimeFormatter("epoch_seconds", new DateTimeFormatterBuilder().append(new Joda.EpochTimeParser(false)).toFormatter().withZone(zone), Locale.ROOT); | |||
There was a problem hiding this comment.
The length of this line will trigger our checkstyle rules once we run the tests, we were at 140 characters (still applies to this test class) and recently moved to 100. Can you shorten this before I kick of the first CI run?
|
@elasticmachine test this please |
cbuescher
changed the title
|
thanks a lot, I changed the title slightly and will merge this. |
Change the error response when using a non UTF timezone for range queries with epoch_millis or epoch_second formats to an illegal argument exception. The goal is to provide a better explanation of why the query has failed. The current behavior is to respond with a parse exception. Closes #22621
* master: (2923 commits) Add 5.3.0 version for packaging tests Require explicit query in _delete_by_query API (elastic#23632) Adds boolean similarity to Elasticsearch (elastic#23637) Revert "Skip 5.4 bwc test for new name for now" Improve error handling for epoch format parser with time zone (elastic#23689) Docs: fix health response test Docs: Clean up response test in getting_started Validate top-level keys when parsing mget requests Reflect cross-cluster search in "dedicated" terminology (elastic#23771) Fix serialization for plugin info Update contributing docs for line-length change Remove line-length violations in SmokeTestClientIT Reenable smoke test client tests on JDK 9 Build: Use GradleBuild task for invoking 5.x checkout build (elastic#23770) Introduce translog generation rolling Modify permissions dialog for plugins FuzzyQueryBuilder should error when parsing array of values (elastic#23762) Remove obsolete index setting `index.version.minimum_compatible`. (elastic#23593) Upgrade to Lucene 6.5.0 (elastic#23750) Fix docs for plugin install via proxy on Windows ...
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
clintongormley
added
:Search Foundations/Mapping
Change the error response when using a non UTC timezone for range queries with epoch_millis or epoch_second formats to an illegal argument exception. The goal is to provide a better explanation of why the query has failed. The current behavior is to respond with a parse exception.