Hello, Disclaimer: I'm completely new to Fineract's TZ problems, and have a pretty limited understanding of what is really going on. That being said:
FYI these kinds of issues are pesky and and, surprisingly, hard to get completely right. Googling the problem in general, one can hit posts such as e.g. https://javorszky.co.uk/2016/06/06/today-i-learned-about-mysql-and-timezones/ to get a glimpse at why this is such an issue. On a fundamental level, I am not sure I actually understand the basic design/architecture re TZ in Fineract... so the idea seems to be that a tenant has a fixed TZ? What do the REST services when they return, or receive input of, dates and times- never anything with a TZ? And how is it stored in the DB? Never with TZ? FYI I've seen this problem being tackled elsewhere in the past in... exactly the opposite way: Both in DB and API always everything in UTC only, so never any "local time" in code - and let the UI handle conversions, based on a User preference (which could be provided via a public API, and could be user or still tenant scoped). Wouldn't that be better even for Fineract, at least as a future goal, if not immediately? I'm wondering if anyone with Fineract CN experience reading this wants to chime in here about it's done there? More concretely, and short term: 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? Finally ;-) re your specific questions, see inline: On Tue, 23 Jul 2019, 22:13 Dylan Robson, <[email protected]> wrote: > Hello everyone. > > I am working on FINERACT-723 ( > https://issues.apache.org/jira/browse/FINERACT-723) regarding issues > where the tenant's time zone is sometimes not considered when calculating > dates/times. So the goal is to refactor all usages of MySQL date/time > functions to use Fineract's DateUtils class instead. > > I started by looking at MySQL docs and other resources and made a list of > 60 MySQL date/time functions. > I then wrote a script (pseudocode below) to grep recursively, ignoring > case, outputting the list of files where the given function name was found. > The ".*" regex accounts for any [0, n] function arguments. > > sqlDateTimeFunctions = [ "ADDDATE", "ADDTIME", "CONVERT_TZ", "CURDATE", > "CURRENT_DATE", "CURRENT_TIME", "CURRENT_TIMESTAMP", "CURTIME", "DATE_ADD", > "DATE_FORMAT", "DATE_SUB", "DATE", "DATEDIFF", "DAY", "DAYNAME", > "DAYOFMONTH", "DAYOFWEEK", "DAYOFYEAR", "EXTRACT", "FROM_DAYS", > "FROM_UNIXTIME", "GET_FORMAT", "HOUR", "LAST_DAY", "LOCALTIME", > "LOCALTIMESTAMP", "MAKEDATE", "MAKETIME", "MICROSECOND", "MINUTE", "MONTH", > "MONTHNAME", "NOW", "PERIOD_ADD", "PERIOD_DIFF", "QUARTER", "SEC_TO_TIME", > "SECOND", "STR_TO_DATE", "SUBDATE", "SUBTIME", "SYSDATE", "TIME_FORMAT", > "TIME_TO_SEC", "TIME", "TIMEDIFF", "TIMESTAMP", "TIMESTAMPADD", > "TIMESTAMPDIFF", "TO_DAYS", "TO_SECONDS", "UNIX_TIMESTAMP", "UTC_DATE", > "UTC_TIME", "UTC_TIMESTAMP", "WEEK", "WEEKDAY", "WEEKOFYEAR", "YEAR", > "YEARWEEK" ] > for (func in sqlDateTimeFunctions) > grep -ril funcName + "(.*)" ../../fineract-FORK >> ./funcNames/funcName.txt > > Question (1): Does this approach seem reasonable to find *all* problems > where tenant TZ is not considered? > Yeah, something like this looks cool, to me. I realize there might be some false-positives for example because the > DATE() and NOW() syntax is used both in MySQL and Java. But these false > positives can be ruled out by manually inspecting each file when > refactoring. > agreed. (Perhaps a you'd like to write up on some MD in docs/ of the project, or just Wiki, an analysis of which of these SQL functions are problematic vs no problem?) (I also think some of these functions, like TIMESTAMPADD() might not be > problems because they probably don’t use the date/time from the MySQL > server, but I searched them anyways to be safe). > > From this script, I found 1360 different files total, and each file might > have more than 1 MySQL date/time function that needs to be refactored. > But it seems that many of these 1360 files are from the migrations > directory as .sql files. > It could lessen this workload if someone can help me understand these > migration files. > (I've ignored .sql files so far as I've been working through the .java > files, because I think they're more relevant). > On the Wikipedia page about schema migrations I found this: "Production > databases are usually huge, old and full of surprises. The surprises can > come from many sources: [e.g.] Corrupt data that was written by old > versions of the software and not cleaned properly". > So all those "DDL" (as in CREATE TABLE, ALTER TABLE) SQL scripts just create the Fineract DB. We don't use SQL table generation by the ORM from code. This is great because it automatically migrates DBs of previous releases. Read some introduction to Flyway if you haven't yet, and I'm sure you'll quickly understand. Basically, AFAIK there's just a special Flyway system table somewhere (I forgot what it's called) which records the current version. If it doesn't exist, on a fresh install, then all those SQL scripts run, in order. If there's already something, then Flyway will run only the scripts from the required version forward. Makes sense? This approach also means that, AFAIK (someone please jump in to correct me if I'm talking non-sense) that you can't fix existing past SQL scripts - just sayin', in case that's what you were looking at doing? But what you (anyone!) certainly can do is contribute PRs with new scripts which migrate both the structure of and data currently in existing tables to fix TZ related problems. Do you know what I mean? Not 100% sure if that answers your Q? Just ask. Question (2): Do these migration files only contain "corrupt" row values > e.g. NOW() because previous Fineract versions inserted that row using NOW() > as a value? > i.e. once a given service layer is refactored to not use the MySQL NOW() > function, will future migration files no longer contain the NOW() function? > That depends on what future contributors put into future migration files, no? (Just to clear, they are hand written.) What I guess you could do, eventually, is grep for and abort if newer files contain keywords that shouldn't be used anymore. It's probably not the first problem to solve though. Thanks and regards > --Dylan >
