Skip to content

NIFI-14572 Fix time offset issue in DateTimeAdapter#unmarshal#11129

Open
superdachuan wants to merge 3 commits intoapache:mainfrom
superdachuan:NIFI-14572
Open

NIFI-14572 Fix time offset issue in DateTimeAdapter#unmarshal#11129
superdachuan wants to merge 3 commits intoapache:mainfrom
superdachuan:NIFI-14572

Conversation

@superdachuan
Copy link
Copy Markdown

Summary

NIFI-14572

This PR fixes a time offset issue in DateTimeAdapter#unmarshal. The current implementation uses ZonedDateTime, which can lead to ambiguous parsing of time zone abbreviations such as CST, resulting in incorrect offsets. This change aligns the implementation with TimestampAdapter by replacing ZonedDateTime with LocalDateTime and converting it using the system default time zone, avoiding such ambiguity. Although this issue may appear related to NIFI-14581, they are not the same problem and have different root causes: NIFI-14581 is caused by inconsistencies between the time zone ID returned by TimezoneAdapter and the server's default locale, whereas this PR addresses ambiguity in parsing time zone abbreviations. Therefore, this change focuses specifically on fixing the time offset issue in DateTimeAdapter and does not address NIFI-14581.

Tracking

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

Issue Tracking

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 contains commits signed with a registered key indicating Verified status

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 ./mvnw clean install -P contrib-check
    • JDK 21
    • JDK 25

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

@superdachuan superdachuan force-pushed the NIFI-14572 branch 2 times, most recently from 53f1a63 to d28fbdf Compare April 13, 2026 04:13
Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this issue @superdachuan.

The functional approach looks good, aligning with TimestampAdapter as described.

I noted several recommendations on the test class, which could use some refinement to ensure expected behavior.

@superdachuan
Copy link
Copy Markdown
Author

@exceptionfactory Thanks for the review and helpful suggestions. I updated the tests accordingly.

My original intent in combining marshal and unmarshal was to verify round-trip consistency, since the adapter output includes a time zone and I initially wanted to avoid relying on a single fixed string. I have kept that intent as explicit round-trip tests, and also added more direct assertions for multiple time zones.

I had originally avoided temporarily switching the default time zone because I was concerned about affecting other tests. After reviewing similar patterns in the codebase, I updated this test to verify behavior across multiple time zones directly.
One detail here is that DateTimeAdapter initializes its DateTimeFormatter during class loading with ZoneId.systemDefault(), so changing the default time zone later does not update the formatter. For that reason, the multi-time-zone assertions reload DateTimeAdapter with a dedicated ClassLoader instead of reusing the default adapter instance from this test class.

Thanks again.

Copy link
Copy Markdown
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @superdachuan. Unfortunately, the new testing approach goes too far in the direction of complexity, and implementing a custom ClassLoader is far more than needed for testing this class. Please revert the change and pursue a simpler approach.

@superdachuan
Copy link
Copy Markdown
Author

Thanks for the feedback @exceptionfactory . I reverted the custom ClassLoader-based test change and simplified the approach.

In the updated revision, I removed withZone(ZoneId.systemDefault()) from the static DateTimeFormatter in DateTimeAdapter. In this adapter, that formatter-level zone was not necessary for the existing marshal/unmarshal behavior, so removing it does not change the intended time zone handling. It does, however, avoid binding the formatter to the JVM default time zone at class initialization time.

With that change in place, the tests can switch the default time zone directly without relying on class reloading or a custom ClassLoader.

Align DateTimeAdapter#unmarshal with TimestampAdapter by replacing
ZonedDateTime with LocalDateTime to avoid timezone ambiguity (e.g., CST)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants