NIFI-12885 Add convenience method to Record to parse DateTime objects

[knguyen1]

Summary

NIFI-12885

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

• Apache NiFi Jira issue created

Pull Request Tracking

• Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
• Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

• Pull Request based on current revision of the main branch
• Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

• Build completed using mvn clean install -P contrib-check
  • JDK 21

Licensing

• New dependencies are compatible with the Apache License 2.0 according to the License Policy
• New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

• Documentation formatting appears as expected in rendered files

[exceptionfactory]

Thanks for putting this together @knguyen1.

The general approach looks good on initial review. I will take a closer look soon.

Can you revert the import reordering changes? Thanks!

[exceptionfactory]

@knguyen1 The test failures on MacOS and Windows indicate a problem with the expectations for timestamp conversion across time zones.

It can be difficult to set proper expectations that will work regardless of local machine time zone, but it is very important to get these tests correct to avoid confusing runtime problems.

For example, converting from OffsetDateTime to Instant and then comparing the values is not an ideal approach due to time zone differences.

It may be helpful to scope down the number of timestamps in the provideTimestamps method to focus on differences. It is not necessary to exercise every variation of formatting, but it is more important to exercise different types of time fields in terms of year, month, day, hour, minute, second and time zone.

[knguyen1]

It may be helpful to scope down the number of timestamps in the provideTimestamps method to focus on differences.

@exceptionfactory Okay. I scoped it down to 2 formats: ISO_LOCAL_DATE_TIME and ISO_OFFSET_DATE_TIME and I limited the offset to +00:00. Because there were parsing failures previously, I think it's indicative of some race conditions we're not covering, which is out of scope for this change. Given that best practice is ISO format and UTC+0 for most db's anyway, I think we should be fine.

[exceptionfactory]

It may be helpful to scope down the number of timestamps in the provideTimestamps method to focus on differences.

@exceptionfactory Okay. I scoped it down to 2 formats: ISO_LOCAL_DATE_TIME and ISO_OFFSET_DATE_TIME and I limited the offset to +00:00. Because there were parsing failures previously, I think it's indicative of some race conditions we're not covering, which is out of scope for this change. Given that best practice is ISO format and UTC+0 for most db's anyway, I think we should be fine.

Thanks for making the updates, unfortunately the tests are still failing:

Error:  org.apache.nifi.serialization.record.TestMapRecord.testGettingLocalDateAndOffsetDateTime(Object, String, long, long)[1] -- Time elapsed: 0.031 s <<< FAILURE!
org.opentest4j.AssertionFailedError: expected: <1641040496789> but was: <1641072896789>

The problem is not a race condition, but the fact of the different GitHub Runners have different local time zones. Converting between UTC and local time changes the expected epoch values, so the expectations need to be written such that they work across all time zones.

[exceptionfactory]

Thanks for the recent updates @knguyen1. Taking a closer look at getOffsetDateTime() highlights a potential problem with the helper function doing too much. It seems better to throw an exception, as opposed to defaulting the time zone. This raises a wider question about the usefulness of this particular helper method, as opposed to putting the responsibility back on the caller to know the appropriate time class to use.

[knguyen1]

@exceptionfactory Hopefully we are close. I added

1. testGettingLocalDate
2. testGettingLocalDateTime
3. testGettingOffsetDateTime

in their own tests. In addition, the Record interface has an additional method getAsLocalDateTime.

Tests should now pass regardless of system default timezones. I checked against America/New_York, UTC+00, etc.

[exceptionfactory]

Thanks for working through the feedback @knguyen1, the latest version looks good, and removes the ambiguity around the legacy getAsDate() method. +1 merging