[GitHub] [nifi] turcsanyip commented on a change in pull request #4773: NIFI-8161 NiFi EL: migration from SimpleDateFormat to DateTimeFormatter

[GitBox <gi...@apache.org>]

turcsanyip commented on a change in pull request #4773:
URL: https://github.com/apache/nifi/pull/4773#discussion_r599955124



##########
File path: nifi-commons/nifi-expression-language/src/main/java/org/apache/nifi/attribute/expression/language/evaluation/cast/DateCastEvaluator.java
##########
@@ -26,21 +26,23 @@
 import org.apache.nifi.attribute.expression.language.exception.AttributeExpressionLanguageException;
 import org.apache.nifi.attribute.expression.language.exception.AttributeExpressionLanguageParsingException;
 import org.apache.nifi.expression.AttributeExpression.ResultType;
+import org.apache.nifi.util.FormatUtils;
 
-import java.text.ParseException;
-import java.text.SimpleDateFormat;
+import java.time.Instant;
+import java.time.format.DateTimeFormatter;
+import java.time.format.DateTimeParseException;
 import java.util.Date;
-import java.util.Locale;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 public class DateCastEvaluator extends DateEvaluator {
 
     public static final String DATE_TO_STRING_FORMAT = "EEE MMM dd HH:mm:ss zzz yyyy";
+    public static final DateTimeFormatter DATE_TO_STRING_FORMATTER = FormatUtils.prepareLenientCaseInsensitiveDateTimeFormatter(DATE_TO_STRING_FORMAT);
     public static final Pattern DATE_TO_STRING_PATTERN = Pattern.compile("(?:[a-zA-Z]{3} ){2}\\d{2} \\d{2}\\:\\d{2}\\:\\d{2} (?:.*?) \\d{4}");
 
-    public static final String ALTERNATE_FORMAT_WITHOUT_MILLIS = "yyyy/MM/dd HH:mm:ss";
-    public static final String ALTERNATE_FORMAT_WITH_MILLIS = "yyyy/MM/dd HH:mm:ss.SSS";
+    public static final DateTimeFormatter ALTERNATE_FORMAT_WITHOUT_MILLIS = FormatUtils.prepareLenientCaseInsensitiveDateTimeFormatter("yyyy/MM/dd HH:mm:ss");
+    public static final DateTimeFormatter ALTERNATE_FORMAT_WITH_MILLIS = FormatUtils.prepareLenientCaseInsensitiveDateTimeFormatter("yyyy/MM/dd HH:mm:ss.SSS");

Review comment:
       Using the same constant names but changing the types would break the public API of this class and it is not backward compatible.
   It could be easily avoided via leaving the original String constants as they were and adding new ones with type DateTimeFormatter. Similar to DATE_TO_STRING_FORMAT / DATE_TO_STRING_FORMATTER above.

##########
File path: nifi-commons/nifi-utils/src/main/java/org/apache/nifi/util/FormatUtils.java
##########
@@ -425,4 +436,40 @@ public static String formatNanos(final long nanos, final boolean includeTotalNan
 
         return sb.toString();
     }
+
+    public static DateTimeFormatter prepareLenientCaseInsensitiveDateTimeFormatter(String pattern) {
+        return new DateTimeFormatterBuilder()
+                .parseLenient()
+                .parseCaseInsensitive()
+                .appendPattern(pattern)
+                .toFormatter(Locale.US);
+    }
+
+    /**
+     * Parse text to Instant - support different formats like: zoned date time, date time, date, time (similar to those supported in SimpleDateFormat)
+     * @param formatter configured formatter
+     * @param text      text which will be parsed
+     * @return parsed Instant
+     */
+    public static Instant parseInstant(DateTimeFormatter formatter, String text) {

Review comment:
       `parseToInstant()` might be a better name because the method does not parse an Instant but it parses text to Instant. Though I do not insist on it.

##########
File path: nifi-commons/nifi-utils/src/test/java/org/apache/nifi/processor/TestFormatUtils.java
##########
@@ -103,4 +111,41 @@ public void testFormatDataSize() {
 
         assertEquals(String.format("181%s9 TB", decimalFormatSymbols.getDecimalSeparator()), FormatUtils.formatDataSize(200_000_000_000_000d));
     }
+
+    @Test
+    public void testParseInstant() throws Exception {
+        TimeZone current = TimeZone.getDefault();
+        TimeZone.setDefault(TimeZone.getTimeZone("Europe/Kiev"));

Review comment:
       Recently we had some issues that occurred only in UTC+ or UTC- time zones.
   I see there is no high chance for this here but as a rule of thumb, I would always write tests with UTC, UTC+ and UTC- zones.
   In this case, it would mean 3 x 3 tests (3 time zones for `TimeZone.setDefault()` and input data from 3 time zones). The input data can easily be added (2 more lines below) and the test can be converted to a parameterized one to use different `TimeZone.setDefault()` settings.

##########
File path: nifi-commons/nifi-expression-language/src/test/java/org/apache/nifi/attribute/expression/language/TestQuery.java
##########
@@ -308,10 +311,9 @@ public void implicitDateConversion() {
 
         // the date.toString() above will end up truncating the milliseconds. So remove millis from the Date before
         // formatting it
-        final SimpleDateFormat sdf = new SimpleDateFormat("yyyy/MM/dd HH:mm:ss.SSS", Locale.US);
-        final long millis = date.getTime() % 1000L;
-        final Date roundedToNearestSecond = new Date(date.getTime() - millis);
-        final String formatted = sdf.format(roundedToNearestSecond);
+        final DateTimeFormatter dtf = DateTimeFormatter.ofPattern("yyyy/MM/dd HH:mm:ss.SSS", Locale.US);
+        final Instant roundedToNearestSecond = date.toInstant().truncatedTo(ChronoUnit.SECONDS);

Review comment:
       The variable used to have a wrong name in the original version too because it was truncated, not rounded (which is more obvious with the current implementation).
   Could be please fix it and change the name to `truncatedToSecond`?




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org