Github user dsmiley commented on a diff in the pull request:
https://github.com/apache/lucene-solr/pull/428#discussion_r207724539
--- Diff:
solr/core/src/java/org/apache/solr/update/processor/ParseDateFieldUpdateProcessorFactory.java
---
@@ -172,4 +181,34 @@ public void init(NamedList args) {
return (null == type) || type instanceof DateValueFieldType;
};
}
+
+ public static void validateFormatter(DateTimeFormatter formatter) {
+ // check it's valid via round-trip
+ try {
+ parseInstant(formatter, formatter.format(Instant.now()));
+ } catch (Exception e) {
+ throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
+ "Bad or unsupported pattern: " +
formatter.toFormat().toString(), e);
+ }
+ }
+
+ private static Instant parseInstant(DateTimeFormatter formatter, String
dateStr) {
+ final TemporalAccessor temporalAccessor = formatter.parse(dateStr);
+ // parsed successfully. But is it a full instant or just to the day?
+ if (temporalAccessor.isSupported(ChronoField.INSTANT_SECONDS)) { //
has time
+ // has offset time
+ if(temporalAccessor.isSupported(ChronoField.OFFSET_SECONDS)) {
--- End diff --
I'm feeling pretty confident about the following:
```
private static Instant parseInstant(DateTimeFormatter formatter, String
dateStr) {
final TemporalAccessor temporal = formatter.parse(dateStr);
// Get Date; mandatory
LocalDate date = temporal.query(TemporalQueries.localDate());//mandatory
if (date == null) {
throw new SolrException(SolrException.ErrorCode.SERVER_ERROR,
"Date (year, month, day) is mandatory: " +
formatter.toFormat().toString());
}
// Get Time; optional
LocalTime time = temporal.query(TemporalQueries.localTime());
if (time == null) {
time = LocalTime.MIN;
}
final LocalDateTime localDateTime = LocalDateTime.of(date, time);
// Get Zone Offset; optional
ZoneOffset offset = temporal.query(TemporalQueries.offset());
if (offset == null) {
// no Zone offset; get Zone ID
ZoneId zoneId = temporal.query(TemporalQueries.zone());
if (zoneId == null) {
zoneId = formatter.getZone();
if (zoneId == null) {
zoneId = ZoneOffset.UTC;
}
}
return localDateTime.atZone(zoneId).toInstant();
} else {
return localDateTime.toInstant(offset);
}
}
```
This dispenses with trying to use INSTANT_SECONDS which would be a
short-cut/optimization but we can't trust that due to my previous comment.
What's needed in the tests is an input string that actually provides an
interesting timezone and/or offset, plus a default timezone of something other
than UTC to show that we ignore that and use the zone or offset provided in the
value. I don't see any test that does that; the tests all provide values of
referring to `Z` or zeroes (equivalent) -- which doesn't exercise the
defaulting logic. An example value taken from SOLR-12561's patch is: `Fri Oct
7 05:14:15 AKDT 2005` (Alaska).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
