Parse composite patterns using ClassicFormat.parseObject backport(#40100) #40503
Conversation
|
Pinging @elastic/es-core-infra |
Java-time fails parsing composite patterns when first pattern matches only the prefix of the input. It expects pattern in longest to shortest order. Because of this constructing just one DateTimeFormatter with appendOptional is not sufficient. Parsers have to be iterated and if the parsing fails, the next one in order should be used. In order to not degrade performance parsing should not be throw exceptions on failure. Format.parseObject was used as it only returns null when parsing failed and allows to check if full input was read. closes elastic#39916
| return (TemporalAccessor) object; | ||
| } | ||
| } | ||
| throw new DateTimeParseException("Failed to parse with all enclosed parsers", input, 0); |
There was a problem hiding this comment.
Because we iterate over parsers we would not know exact failure reason for each parsers. Tests that assert about the exact reason (like position where it failed) had to be changed
danielmitterdorfer
left a comment
danielmitterdorfer
left a comment
There was a problem hiding this comment.
Thanks! Looks mostly fine but I left a few minor comments.
| try { | ||
| return doParse(input); | ||
| } catch (DateTimeParseException e) { | ||
| throw new IllegalArgumentException("failed to parse date field [" + input + "] with format [" + format + "]", e); |
There was a problem hiding this comment.
This means that this method will never throw DateTimeParseException as mentioned in the Javadoc of DateFormatter#parse. Can you please update the Javadoc there to reflect this change?
There was a problem hiding this comment.
Actually the javadoc for the interface could state RuntimeException to be absolutely correct, but it would be very imprecise.
JodaDateFormattercan throwjava.time.DateTimeExceptionas well asUnsupportedOperationExceptionandIllegalArgumentExceptionJavaDateFormatteronlyIllegalArgumentException.NullPointerExceptionis stated in the underlying doc forFormat.parseObjectbut I don't expect this to be thrown as we already check for input not being empty and provide the position of parsing.
I will refactor the javadoc to state that IllegalArgumentException is expected to be thrown when parsing fails (that applies in both cases). But I wonder if we should ommit the DateTimeException and UnsupportedOperationException (thrown only by joda)
This problem also exists in master
There was a problem hiding this comment.
I think there is no point in digging through the complete implementation of Java time and Joda time in order to come up with a Javadoc that documents all possible exceptions but rather document the ones that a user can "reasonably" expect when a date cannot be parsed which is in our case java.time.DateTimeException from JodaDateFormatter and IllegalArgumentException (from both) so they know how to deal with them.
IMHO it is ok to document both of them on the interface. Although one of them is in fact thrown by only one of the implementations, the exception type itself is available in the JDK and thus generic enough and we should expect users to program against the interface.
| } | ||
| } catch (IllegalArgumentException | DateTimeException e) { | ||
| throw new ElasticsearchParseException("failed to parse date field [{}] in format [{}]: [{}]", e, value, format, e.getMessage()); | ||
| } catch (IllegalArgumentException | DateTimeException e) {; |
There was a problem hiding this comment.
Nit: There is an additional unnecessary semicolon at the end of the line.
| } catch (IllegalArgumentException | DateTimeException e) { | ||
| throw new ElasticsearchParseException("failed to parse date field [{}] in format [{}]: [{}]", e, value, format, e.getMessage()); | ||
| } catch (IllegalArgumentException | DateTimeException e) {; | ||
|
|
There was a problem hiding this comment.
Nit: Is there a reason for the additional empty line here?
There was a problem hiding this comment.
accident when merging
danielmitterdorfer
left a comment
danielmitterdorfer
left a comment
There was a problem hiding this comment.
Thanks for iterating. LGTM
pgomulka
force-pushed
the
6.7
branch
2 times, most recently
from
29e67c5 to
358d761
Compare
3 participants
Java-time fails parsing composite patterns when first pattern matches only the prefix of the input. It expects pattern in longest to shortest order. Because of this constructing just one DateTimeFormatter with appendOptional is not sufficient. Parsers have to be iterated and if the parsing fails, the next one in order should be used. In order to not degrade performance parsing should not be throw exceptions on failure. Format.parseObject was used as it only returns null when parsing failed and allows to check if full input was read.
closes #39916
backport #40100