[jira] [Commented] (FINERACT-826) Migrate to java.time from Joda API

["Avik Ganguly (Jira)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/FINERACT-826?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17411205#comment-17411205 ] 

Avik Ganguly commented on FINERACT-826:
---------------------------------------

[~ptuomola] [~Percy Ashu] [~awasum]  [~vorburger] :

FINERACT-826
actualDisbursementDate.toDate() got replaced by
java.util.Date.from(actualDisbursementDate.atStartOfDay(ZoneId.systemDefault()).toInstant());

Before FINERACT-1112
actualDisbursementDate = T
Server Timezone : +05:30Z, Tenant Timezone : +6Z
ZoneId zone = ZoneId.systemDefault(); //+05:30Z
ZonedDateTime dateTime = actualDisbursementDate.atStartOfDay(zone); //Midnight +5:30Z
Instant instant = dateTime.toInstant(); // T-1 Midnight + 18:30 0Z
Date dt = Date.from(instant); // T-1 Midnight + 18:30 + 5:30 = T 00:00 +05:30Z

FINERACT-1112
If tenant timezone is ahead of server timezone, then :-

Server Timezone : +05:30Z, Tenant Timezone : +6Z
actualDisbursementDate = T
ZoneId zone = DateUtils.getDateTimeZoneOfTenant(); //+6Z // Previously ZoneId.systemDefault()
ZonedDateTime dateTime = actualDisbursementDate.atStartOfDay(zone); //Midnight +6Z
Instant instant = dateTime.toInstant(); // T-1 Midnight + 18 0Z
Date dt = Date.from(instant); // T-1 Midnight + 18 + 5:30 = T-1 23:30 +05:30Z

If tenant timezone and server timezones are different, then this creates a T-1 issue for all txn dates populated by application.
{quote}Hi - in all the places where this currently calls ZoneId.systemDefault(), should we not be retrieving the appropriate tenant timezone instead? I know that's not what the previous code does, but wouldn't that be the right behaviour for any logic that is related to a specific tenant - i.e. we should be using the tenant's timezone for any dates?
{quote}
{quote}actualDisbursementDate.toDate() got replaced by
java.util.Date.from(actualDisbursementDate.atStartOfDay(ZoneId.systemDefault()).toInstant());
{quote}
This part looks mostly correct as this is the most popular way ([https://stackoverflow.com/questions/22929237/convert-java-time-localdate-into-java-util-date-type/22929420]) to convert time.date to util.date. Note that the popular answer is questioned in it's comments section because atStartOfDay changes the time but overall the answer fixes the question.

Problem is with using DateUtils.getDateTimeZoneOfTenant() instead of ZoneId.systemDefault() as this deviates from the solution provided for Fineract-826. A tenant is already passing the correct tenant-specific date in the API. There is no need to mess with the date by applying tenant timezone all over again.

We have reverted DateUtils.getDateTimeZoneOfTenant() to ZoneId.systemDefault() wherever date was being parsed with atStartOfDay logic and so far SIT is successful. Please approve the corresponding PR if you approve of this logic reversal.

> Migrate to java.time from Joda API
> ----------------------------------
>
>                 Key: FINERACT-826
>                 URL: https://issues.apache.org/jira/browse/FINERACT-826
>             Project: Apache Fineract
>          Issue Type: Bug
>    Affects Versions: 1.4.0
>            Reporter: Michael Vorburger
>            Assignee: Percy Ashu
>            Priority: Major
>              Labels: technical
>             Fix For: 1.5.0
>
>          Time Spent: 1h 40m
>  Remaining Estimate: 0h
>
> [http://mail-archives.apache.org/mod_mbox/fineract-dev/201907.mbox/%3cCALiX4ib1d32ZSKZN87KureoSBwsCJMo9DHGNoQbWYDyUi-CTUQ@mail.gmail.com%3e,] and the original previous and the follow up replies on that thread Subject "Questions about my approach to find problems with tenant time zones, and about the instance values of SQL migration files.", related to FINERACT-723? I think, point out that {{org.apache.fineract.infrastructure.core.service.DateUtils}} could do with a closer look and fixes, specifically:
> {quote}That DateUtils class is interesting.. do we really need to mix the old java.util.Date API and the org.joda.time Time API ? FYI Joda Time has been integrated into Java 8 as java.time. So if someone was up for taking up work related to cleaning that up, that could be useful IMHO (so converge everything to java.time, and eventually remove the Joda dependency, and introduce an enforced check via Checkstle or SpotBugs to forbid java.util.Date).
>   
>  The null management in DateUtils is pretty interesting as well. So is this per tenant TZ guaranteed to always be in the DB? What does the DB schema say - required, never NULL? Does the Java code ensure it's never null? I'm asking these questions because something like that getLocalDateTimeOfTenant(), despite its name, seems to be actually be written to EITHER give us a "local" time based on the tenant, or, if that's null, it just falls back to what l suspect (not verified) is dependent on the OS / JVM TZ - that's scary. Perhaps it's not a problem in reality, if every tenant is guaranteed to always have a TZ, but if someone could double check this, and if so remove those null check and system dependent fallbacks, then IMHO that would make that utility code clearer.
>   
>  BTW line 45 in DateUtils seems completely useless, agreed? Would you like to raise a PR proposing to ditch that?
> {quote}



--
This message was sent by Atlassian Jira
(v8.3.4#803005)