Adding support for fractional seconds #274
Conversation
|
Can one of the admins verify this patch? |
2 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
@joostdebruijn Thank you, could you rebase your code? I will trigger CI test for you then. |
|
@jannyHou Thanks for having a first look at it! I gave you permissions to edit my fork. I'm not very familiar with doing rebases, so I will appreciate your help with that. |
|
@joostdebruijn I have rebased the branch for you. Please sign the Contributor License Agreement before we can move on with this PR. @slnode test please |
There was a problem hiding this comment.
@joostdebruijn You only need to sign the CLA once per one loopback repository. You had to resign on this PR because you closed #270.
Left some comments on the PR, please take a look.
| @@ -412,7 +412,7 @@ MySQL.prototype.fromColumnValue = function(prop, val) { | |||
| // MySQL allows, unless NO_ZERO_DATE is set, dummy date/time entries | |||
| // new Date() will return Invalid Date for those, so we need to handle | |||
| // those separate. | |||
| if (!val || val == '0000-00-00 00:00:00' || val == '0000-00-00') { | |||
| if (!val || /^0{4}(-00){2}( (00:){2}0{2}(\.0{1,6}){0,1}){0,1}$/.test(val)) { | |||
There was a problem hiding this comment.
Not sure what the end of the regex means
{0,1}$/.test(val)
There was a problem hiding this comment.
This regex checks if val is one of the following values:
0000-00-00
0000-00-00 00:00:00
0000-00-00 00:00:00.0
0000-00-00 00:00:00.00
0000-00-00 00:00:00.000
0000-00-00 00:00:00.0000
0000-00-00 00:00:00.00000
0000-00-00 00:00:00.000000
You can see it working over here: https://regex101.com/r/IkM0wc/2
There was a problem hiding this comment.
Yes, I get what the regex does. I am confused about the end part of the regex.
.test(val)
There was a problem hiding this comment.
Ah, I get it! The test() method checks if the string specified in the function argument matches the regex. It will return true or false. See also this article on MDN.
There was a problem hiding this comment.
That makes much more sense now.
| break; | ||
| case 'time': | ||
| case 'datetime': | ||
| case 'timestamp': |
There was a problem hiding this comment.
Also should we only add precision fractional second for timestamp values? According to MySQL documentation
A DATETIME or TIMESTAMP value can include a trailing fractional seconds part in up to microseconds (6 digits) precision
There was a problem hiding this comment.
I think I don't understand your comment. As the MySQL documentation said: TIME, DATETIME and TIMESTAMP types can have a precision. The DATE-type (of course) not.
This function checks the columntype and for columns with a type of time, datetime and timestamp and a precision is defined, the columnType will modified (see line 915), for example to DATETIME(3). This will allow to save values like 2017-05-07 12:34:56.789. When the column is of the type DATE, the function returns the colomnType without any modification.
| @@ -17,6 +17,7 @@ describe('MySQL datetime handling', function() { | |||
| married: Boolean, | |||
| dob: {type: 'DateString'}, | |||
| createdAt: {type: Date, default: Date}, | |||
| lastLogon: {type: Date, precision: 3, default: Date}, | |||
There was a problem hiding this comment.
hi @joostdebruijn from the login on line#900, a property with date type won't execute dateOptions() which appends precision config, can you take a look?
There was a problem hiding this comment.
Are you sure? I ran the tests again and dateOptions() is hit twice for my specific test. The first time for property createdAt (skipping the if at migration.js:914), resulting in a DATETIME. The second time for the property lastLogon (hitting the if at migration.js:914) and resulting in a DATETIME(3).
Please note that the Loopback-type 'Date' maps on 'DATETIME' by default, see documentation and migration.js:725. The method dateOptions() handles the columnType and not the type as in Loopback-type.
There was a problem hiding this comment.
ah I see, it's executed after dt = columnType(p, 'DATETIME');. Thanks.
Description
In MySQL 5.6 the support for saving fractional seconds for the DATETIME datatype was added. This pull request will add support for it to the connector for the use of native Date-types. The DateString-type already supports fractional seconds.
Earlier this week I posted #270, however this a a replacement based on the changes in the 4.1-release of the connector.
Related issues
Checklist