-
Notifications
You must be signed in to change notification settings - Fork 183
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor date, timestamp and datetime data types handling #257
Conversation
ping @ssh24 |
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
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? |
a118455
to
1bb8e05
Compare
@slnode test please |
test/datetypes.notz.test.js
Outdated
|
||
var localTZ = getTimeZone(); | ||
|
||
describe('MySQL DATE, DATETTIME, TIMESTAMP types on server with local TZ', function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move these two test cases all under one test file. For example in ./test/datatypes.test.js/.
test/datetypes.notz.test.js
Outdated
}); | ||
}); | ||
|
||
it('should create table', function(done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't have a test case for it. And instead of creating a table like this, create a loopback model directly and automigrate the model to the database. More on automigrate here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I create table via SQL to have a test field with DEFAULT CURRENT_TIMESTAMP:
timestampDefaultField timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP,
.
for this test case:
it('timestampField shoud equal DEFAULT CURRENT_TIMESTAMP field', function(done) {
DateModel.findOne({where: {id: 1}}, function(err, found) {
assert.ok(!err);
assert.equal(found.timestampField.toISOString(), found.timestampDefaultField.toISOString());
done();
});
});
It is needed to be sure that time set on server side is the same as time we pass from new Date();
I know how to use auto migration but don't know how to create field with such defaults. I can do auto migrate and then query ALTER TABLE CHANGE timestampDefaultField timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP
. Is it acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and, sure, I'll move it to setup section instead of test case
test/datetypes.notz.test.js
Outdated
}); | ||
}); | ||
|
||
function setup(done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need any of these setup AFAIK. All the models/tables should be on the default database the test runs against.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I unified setup procedures for both test cases
Done. @ssh24, please review. |
@darknos Looks like loopback does not support @raymondfeng Could you correct me if I am wrong? |
Js Date type is mapped to mysql datetime, date or timestamp. I use loopack on tens of clients project and in most cases in their databases were these three types. loopback has to support all of them. |
id: {type: Number, id: 1, generated: true}, | ||
datetimeField: {type: Date, dataType: 'datetime', null: false}, | ||
timestampField: {type: Date, dataType: 'timestamp', null: false}, | ||
timestampDefaultField: {type: Date, dataType: 'timestamp', null: false}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darknos You would need to change the dataType
in that case here then. LoopBack does not support timestamp
as an official data type. We support date
which is the JavaScript date object. More info on docs.
Same for datetime
as a data type.
Sorry totally ignore my comment. I thought for some reason you were invoking the loopback model with dataType timestamp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Type mappings:
LoopBack to MySQL types
Date -> DATETIME
MySQL to LoopBack types
DATE,TIMESTAMP,DATETIME -> Date
I just want to make this mapping correct. Try my simple and clear test cases with current version of connector to see what is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darknos Code LGTM. I did some small cleanups. Please fix the rest of the code with the suggested changes. Make sure to pull the latest before committing any changes.
}, | ||
}, function(err, found) { | ||
assert.ok(!err); | ||
assert.ok(found); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could assert for something more than just found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.equal(found.id, 1);
should be enough - we have to be sure that param of each type is working and found proper recored.
}, | ||
}, function(err, found) { | ||
assert.ok(!err); | ||
assert.ok(found); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Assert more than just found
}, | ||
}, function(err, found) { | ||
assert.ok(!err); | ||
assert.ok(found); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Assert more than just found
lib/mysql.js
Outdated
} else if (prop.dataType == 'timestamp' || (prop.mysql && prop.mysql.dataType == 'timestamp')) { | ||
var tz = this.client.config.connectionConfig.timezone; | ||
return dateToMysql(val, tz); | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the else. Just return dateToMysql(val);
is good.
lib/mysql.js
Outdated
} | ||
|
||
var m = tz.match(/([\+\-\s])(\d\d):?(\d\d)?/); | ||
if (m) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One liner for this should be fine: if (m) return (m[1] == '-' ? -1 : 1) * (parseInt(m[2], 10) + ((m[3] ? parseInt(m[3], 10) : 0) / 60)) * 60;
lib/mysql.js
Outdated
} else { | ||
tz = convertTimezone(tz); | ||
|
||
if (tz !== false && tz !== 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change this to: if (tz !== false && tz !== 0) dt.setTime(dt.getTime() + (tz * 60000));
Done. @ssh24 please review |
test/date_types.test.js
Outdated
}); | ||
}); | ||
|
||
it('should find model instance by date field passing string', function(done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darknos Test is failing locally for this. Why is this a valid test case to test for a dateField
of type string when it expects a Date object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All "where" parameters are passed normalize function of "juggler".
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L1146
If property is one of type Date it does new Date(arg)
https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L1540
for DATE type test case will pass for timezones like my one (+xxxx):
"2016-12-22" -> "Thu Dec 22 2016 02:00:00 GMT+0200 (EET)"
but
will fail for (-xxxx) timezones:
"2016-12-22" -> "Thu Dec 21 2016 22:00:00 GMT-0200"
I'm going to remove this test case but I'd suggest to mean it in documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a question how to pass where parameter to DATE field in different timezone via REST interface.
f.e. server is in GMT-6000 timezone and client is in GMT+2000 timezone. on client side we do:
var filter = {where:{dateFiled:new Date(2016,11,22)}};
it will pass as {"where":{"dateFiled":"2016-12-21T22:00:00.000Z"}}
and dao will convert it to date Wed Dec 21 2016 16:00:00 GMT-0600 (CST)
and pass to SQL as 2016-12-21
and we got wrong result. :(
test/date_types.test.js
Outdated
@@ -202,6 +217,18 @@ describe('MySQL DATE, DATETTIME, TIMESTAMP types on server with non local TZ (+0 | |||
}); | |||
}); | |||
|
|||
it('should find model instance by date field passing string', function(done) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same reason here. Test is failing locally for this. Why is this a valid test case to test for a dateField
of type string when it expects a Date object?
It is a question how to pass where parameter to DATE field in different timezones via REST interface. f.e. server is in
it will pass via HTTP as |
@darknos Not sure what you mean
What is the wrong result? |
I wish I had time right now to really look at the implications of this PR, but unfortunately I do not right now. I do want to comment that I'm concerned about forcing the mysql config: |
Pretend we have field dateField with value "2016-12-22" stored in database and I want to get records with this value. I'll do
but it will execute SQL If server is in +0200 timezone and I'll do
the SQL will be If server is in -0500 timezone |
this PR fixes timezone handling and makes difference between handling DATETIME and TIMESTAMP as described in MySQL documentation. I found only one way to fix it - make only one conversion from mysql strings to js Date object and do it in the loopback connector but not on node-mysql side. Probably there are another ways to fix it but I'd ask to keep test/date_types.test.js I made for it and even extend it if you have any ideas about other test cases. I guess you can get ideas of new test cases from #149 |
I don't think there should be a difference in the way this connector handles DATETIME vs TIMESTAMP: |
Still not sure what is the problem.
Why should it execute SQL with dateField's date equal to 21? I am assuming the server is in GMT, and you are accessing it from somewhere GMT-x timezone? |
There is a simple way to see that the difference is present. It is described in my test cases.
Just runt test/date_types.test.js with DEBUG=mysql to see mysql data packets. |
this is connected only to WHERE not to INSERT or UPDATE this is not issue but I'd suggest to describe it as "notice" in documentation. |
We work with date time as below and I expect such behavior. let date = new Date();
let dateString = date.toISOString();
DateModel.create({
id: 1.
datetimeField: date,
}, function(err, res) {
DateModel.findOne({where:{dateField: date}, function(err, result) {
//result.id == 1;
}
//or
DateModel.findOne({where:{dateField: dateString}, function(err, result) {
//result.id == 1;
}
} where do you suggest to specify explicit timezone? |
I'm not sure that understand 'bizarre behavior'. In my TZ:
one hour difference is Daylight Saving Time difference
|
I suggest that if you initialise a MySQL datasource with |
@DaGaMs So, do you just suggest to change this //if datatype is timestamp and zimezone is not local - convert to proper timezone
if (tz !== 'local' && (prop.dataType == 'timestamp' || (prop.mysql && prop.mysql.dataType == 'timestamp'))) {
dateString += ' ' + tz;
} to this //if timezone is not local - convert to proper timezone
if (tz !== 'local') {
dateString += ' ' + tz;
} ? |
yes. And correspondingly also make sure that whatever gets written to the db is converted to the tz of the connection |
sure:
to
|
all tests are passed. |
I had test failures with your branch, strangely. Here is a branch where all tests are passing for me: |
Should discussion about this (loopback-connector-mysql date / time zone issues) continue here or at #149? |
I think @darknos should open up a PR with the legacyBehaviour flag mentioned above and we can reference this issue and #149 to that PR for the fixup and release ASAP so you and @DaGaMs can get their fixes right away with an immediate publish. cc @ssh24 @kjdelisle |
Hey @superkhau , I don't have a 'fix' ready to go. My feeling is that this (#257) goes in the opposite direction from what was discussed at #149 where I was suggesting removing all the code converting dates to UTC and just handing off and receiving js Date objects to/from the underlying mysql module. I do have a fork with that stuff removed that works fine in my limited testing, but I don't have any legacy fallback or the docs for upgrading. This (#257) goes completely the other way and forces the underlying module to produce String values for all date types. |
@bbito I hear your concern. Maybe the simplest route is to revert #257 and continue the discussion at #149. My feeling is we need to do:
as suggested by @bajtos at #149 Then in a subsequent PR (after continuing some discussion at #149):
Thoughts? |
@superkhau That sound best to me! I think what we came up with at #149 was a solid plan. |
@superkhau I'm pretty sure that with the updated code (after @darknos and my discussion yesterday) the "compatibility setting" would simply be to make |
@bbito I'm not sure I agree any more with the idea of leaving all date casting to javascript. In that case, a change in the timezone setting of the server that the loopback instance runs on would change the interpretation of the data. I prefer the more "explicit" setting in the datasource.json file, so it's totally clear that all dates that go in and come out of the database are in a specific timezone (and UTC is a very sensible default ;-) ) |
@superkhau, @ssh24 , @DaGaMs I added new PR: #265 and I'll wait for your decision. |
I completely agree and this was the crux of #149 (comment). Any fix must honor |
…)" This reverts commit f2f0dac.
Reverting this PR as it is a breaking change for other users. Please follow #149 as we try to come up with a cleaner solution. |
got it. after all discussions I see that the best way is to keep own fork of the connector. :( |
Resolve conflicts following revert of loopbackio#257 via loopbackio#266
* Tests for datetime types (Kevin Delisle) * Add new type DateString to fromColumnValue (Buck Bito) * Remove String manipulations of Date objects (Buck Bito) * Properties with mysql custom "columnName" don't get autoupdated (#273) (Sergey Nosenko) * Revert PR #257 (#266) (Sakib Hasan) * Fix async.each in migration (#262) (Benjamin Schuster-Boeckler) * refactor date, timestamp and datetime data types handling (#257) (Sergey Nosenko) * Fix too many connection error (#261) (Sakib Hasan)
This is a continue of #170.
I propose more correct way (from my point of view) of handling DATE, TIMESTAMP and DATETIME types.
Related issues
#170
Checklist
guide
connected to https://github.com/strongloop-internal/scrum-apex/issues/173