You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@fineract.apache.org by Dylan Robson <dy...@gmail.com> on 2019/07/23 20:13:01 UTC
Questions about my approach to find problems with tenant time zones,
and about the instance values of SQL migration files.
Hello everyone.
I am working on FINERACT-723 (https://issues.apache.org/jira/browse/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? 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.
(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".
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?
Thanks and regards
--Dylan
Re: Questions about my approach to find problems with tenant time
zones, and about the instance values of SQL migration files.
Posted by Victor Manuel Romero Rodriguez <vi...@hotmail.com>.
Hi,
Short history.
- Use UTC and a NTP, working with TZ in the UI/Interfaces/Jobs and using a NTP for sync the clocks at OS level.
- Avoid the use of Joda and old Java Date/Calendar. Using the Java 8+ Date class.
- Be aware of the location of the branches, digital channels (Internet banking and mobile banking) and any other providers like payment gateway, ATMs, transactional switches, market data providers, etc.
Long history:
I will give the example about the timezones in Mexico (we have 5 TZs)... we love our politicians :) hehehe. Just as comment UTC doesn't handle any Daylight Saving Time. More info at https://en.wikipedia.org/wiki/Coordinated_Universal_Time
[cid:part2.CDD0025B.1FF4E257@hotmail.com][cid:part3.D469898E.4D98A874@hotmail.com]
Our issues were when using the Connector/J version 8.0.16 for 1 tenant. Ok that is easy, just use UTC as is... but then we got the following exception.
Caused by: java.sql.SQLException: The server time zone value 'CDT' is unrecognized or represents more than one time zone. You must configure either the server or JDBC driver (via the serverTimezone configuration property) to use a more specifc time zone value if you want to utilize time zone support.
Easy to fix, just add the America/Mexico_City TZ in the URL of the DB connection.
But...later when applying payments received from the back end services (not front end at all) we notice that the fields of Date & TimeStamp type were giving the wrong date. We notice that DB was taking the SYSTEM Time Zone, fixed that using the tzdata at Linux OS level and later applying a Time Zone also in the DB.
Still we notice some timing differences... we had to set the NTP in the servers using the National Timing (all the official clocks are using that server), but when using hybrid environments (On Premises and Cloud) the NTP are differents, why?... some Cloud have applied leap smear to reduce the impact of leap seconds when using UTC.
After all this "bla bla bla" I will be working in the docker-compose to apply the best practices, but each Mifos user/developer must be aware how important are the Date/Time... later we can talk about the business impact/use cases.
Also good reading:
https://stackoverflow.com/questions/48942916/difference-between-utc-and-gmt
About NTP / Leap Seconds / Leap Smear
http://www.cenam.mx/hora_oficial/gsincronizacion.aspx -- Mexico Only
https://developers.google.com/time/smear
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/set-time.html
Regards
Victor
El 23/07/19 a las 21:29, Courage Angeh escribió:
Hi All,
I think implementing a script to search and replace the time/date functions is cool but it will be daunting and time-consuming if the migrations start throwing errors after your make these updates ( Which is quite likely :-( and the SQL migration scripts are really much).
Maybe you could consider implementing a new SQL migration script to make the necessary changes? (This will also require some time and effort but maybe less tedious compared to the previous).
Michael pretty much just summarized the concept behind flyway in like four lines (Cool..!!!) but if you need more hands-on, you can get it here; https://www.baeldung.com/database-migrations-with-flyway
I also think it will be easier for us to deal with one TZ (UTC) - it more consistent compared to managing different TZs. Then we let the UI do the conversion depending on the tenant's TZ.
Regards,
Courage.
On Tue, Jul 23, 2019 at 6:21 PM Michael Vorburger <mi...@vorburger.ch>> wrote:
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, <dy...@gmail.com>> 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
Re: Questions about my approach to find problems with tenant time
zones, and about the instance values of SQL migration files.
Posted by Courage Angeh <co...@gmail.com>.
Hi All,
I think implementing a script to search and replace the time/date functions
is cool but it will be daunting and time-consuming if the migrations start
throwing errors after your make these updates ( Which is quite likely :-(
and the SQL migration scripts are really much).
Maybe you could consider implementing a new SQL migration script to make
the necessary changes? (This will also require some time and effort but
maybe less tedious compared to the previous).
Michael pretty much just summarized the concept behind flyway in like four
lines (Cool..!!!) but if you need more hands-on, you can get it here;
https://www.baeldung.com/database-migrations-with-flyway
I also think it will be easier for us to deal with one TZ (UTC) - it more
consistent compared to managing different TZs. Then we let the UI do the
conversion depending on the tenant's TZ.
Regards,
Courage.
On Tue, Jul 23, 2019 at 6:21 PM Michael Vorburger <mi...@vorburger.ch> wrote:
> 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, <dy...@gmail.com> 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
>>
>
Re: Questions about my approach to find problems with tenant time
zones, and about the instance values of SQL migration files.
Posted by Michael Vorburger <mi...@vorburger.ch>.
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, <dy...@gmail.com> 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
>