Correctly check for errors when parsing text calendars #1035
Conversation
| override def initialValue = { | ||
| val formatter = new SimpleDateFormat(pattern, locale) | ||
| formatter.setCalendar(calendar) | ||
| formatter.setLenient(true) // TODO: should this use calendarCheckPolicy? |
There was a problem hiding this comment.
I'm pretty sure this is wrong and makes it so calendar parsing is always lenient regardless of the dfdl:calendarCheckPolicy property, but this copies the existing logic from the old tlDataFormatter so at least doesn't cause any regressions and just fixes the one bug at hand.
I've attempted to make it correctly use calendarCheckPolicy, but it broke a number of tests, including some unexpected failures that I think need more investigation. Once I can create a reproducible test I'll open a ticket, but I think a fix might be best for the next release so we have time to deal with regressions.
There was a problem hiding this comment.
Agree we should fix the one problem, and defer the strict/lenient to next point release.
The SimpleDateFormat parse() method does not set the error index on the ParsePosition if there is an error. Instead, it only sets the normal index to the last successful parse position of the input string. Daffodil currently relies on the error index, which means our error checking is incorrect. Fortunately, in most cases, this doesn't matter because our error checking also requires that we parse the entire input string, so if the index is zero it wouldn't match the string length and we would still see it as an error. The one exception to this is if the input string is zero length, in which case we do not correctly error and just create a date/time with value zero. This is fixed by correctly checking index for zero instead of inspecting the error index. This also does a little refactoring to replace the very old cache implementation with the much newer Evaluatable for creating SimpleDateFormat instances. DAFFODIL-2821
stevedlawrence
force-pushed
the
daffodil-2821-empty-dateTime
branch
from
198d01a
to
f62072f
Compare
| private lazy val dateTimeFormatterEv = { | ||
| val ev = new DateTimeFormatterEv( | ||
| calendarEv, | ||
| localeEv, |
There was a problem hiding this comment.
This change may affect/fix https://issues.apache.org/jira/browse/DAFFODIL-2823 also, as the locale is being supplied differently now.
| override def initialValue = { | ||
| val formatter = new SimpleDateFormat(pattern, locale) | ||
| formatter.setCalendar(calendar) | ||
| formatter.setLenient(true) // TODO: should this use calendarCheckPolicy? |
There was a problem hiding this comment.
Agree we should fix the one problem, and defer the strict/lenient to next point release.
The SimpleDateFormat parse() method does not set the error index on the ParsePosition if there is an error. Instead, it only sets the normal index to the last successful parse position of the input string.
Daffodil currently relies on the error index, which means our error checking is incorrect.
Fortunately, in most cases, this doesn't matter because our error checking also requires that we parse the entire input string, so if the index is zero it wouldn't match the string length and we would still see it as an error. The one exception to this is if the input string is zero length, in which case we do not correctly error and just create a date/time with value zero.
This is fixed by correctly checking index for zero instead of inspecting the error index.
This also does a little refactoring to replace the very old cache implementation with the much newer Evaluatable for creating SimpleDateFormat instances.
DAFFODIL-2821