Skip to content
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

Date fields - timezone problem #149

Closed
euronymous opened this issue Feb 11, 2016 · 77 comments
Closed

Date fields - timezone problem #149

euronymous opened this issue Feb 11, 2016 · 77 comments

Comments

@euronymous
Copy link

euronymous commented Feb 11, 2016

Hi,

I've spend the last hour for reverse engineering of that case...

Lets look at the file:
/loopback-connector-mysql/lib/mysql.js:353
val = new Date(val.toString().replace(/GMT.*$/, 'GMT'));
Why you don't give a sh about timezone?! This is at least weird.

Here is an example. I have a date from MySQL:
Mon Jun 01 2015 02:01:01 GMT+0200 (CEST)

So lets try:

        console.log(val);
        console.log(val.toString().replace(/GMT.*$/, 'GMT'));
        val = new Date(val.toString().replace(/GMT.*$/, 'GMT'));
        console.log(val);

And the result is:

        Mon Jun 01 2015 02:01:01 GMT+0200 (CEST)
        Mon Jun 01 2015 02:01:01 GMT
        Mon Jun 01 2015 04:01:01 GMT+0200 (CEST)

And at last the strongloop API response:
"2015-06-01T02:01:01.000Z"

Kind Regards,
Krzysiek

@bbito
Copy link
Contributor

bbito commented Feb 17, 2016

@euronymous I had an issue with non UTC dates in MySQL and found the dateString option useful for a workaround. It may help in your case as well - see issue 120 and daddybh's dateString option: #120

bbito added a commit to bbito/node-mysql that referenced this issue Mar 7, 2016
Adding checks to allow true OR "DATE" as dateStrings values "DATE" should only effect DATE fields whereas true retains original behavior, effecting DATE, DATETIME and TIMESTAMP fields. See: mysqljs#605 , loopbackio/loopback-connector-mysql#120 , loopbackio/loopback-connector-mysql#149
@DaGaMs
Copy link
Contributor

DaGaMs commented Jul 25, 2016

I also just ran into this problem when writing some tests where I entered data into a mysql database directly, then fetched it with loopback. My understanding is that DATETIME fields are "timezone-free". Most libraries, including e.g. mysqljs interpret DATETIME objects in the local timezone by default. Loopback however turns them into UTC by default, which is a bit confusing.

@bbito
Copy link
Contributor

bbito commented Jul 25, 2016

@DaGaMs , Since this is a JavaScript/node system, I think this is the correct behavior for DATETIME and TIMESTAMP columns since JavaScript serializes its Date object into ISO 8601 'which is always 24 characters long: YYYY-MM-DDTHH:mm:ss.sssZ. The timezone is always zero UTC offset, as denoted by the suffix "Z".' See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Date/toJSON

Are you saying that if you retrieve the same DATETIME field with mysqljs (which is what this loopback connector uses) and serialize that javascript Date object with .toJSON() you get a different result than you see when you retrieve it as JSON through Loopback?

@superkhau
Copy link
Contributor

Are you saying that if you retrieve the same DATETIME field with mysqljs (which is what this loopback connector uses) and serialize that javascript Date object with .toJSON() you get a different result than you see when you retrieve is as JSON through Loopback?

Good question. @DaGaMs Can you provide a sample repo so we can clone and verify the issue on our machines locally?

@bbito
Copy link
Contributor

bbito commented Jul 26, 2016

@DaGaMs , The way I see it, you have a Date in JavaScript as in:
var nowDate = new Date(); -> Mon Jul 25 2016 17:05:03 GMT-0700 (Pacific Standard Time).

If you want to send that to a REST API like Loopback you are going to turn it into JSON:
nowDate.toJSON(); -> 2016-07-26T00:05:03.861Z

If you retrieve it from Loopback and make a new JavaScript Date object it will be the same date you put in:
var backFromJSON = new Date('2016-07-26T00:05:03.861Z'); ->Mon Jul 25 2016 17:05:03 GMT-0700 (Pacific Standard Time)

@DaGaMs
Copy link
Contributor

DaGaMs commented Jul 26, 2016

@bbito @superkhau Indeed, I get different results depending whether I get the date with mysqljs or with loopback. Here's a code snippet to explain what I did (from a mocha/chai test):

// This is our direct connection to the database
var db = mysql.createConnection({
  host     : 'localhost',
  user     : 'test_user',
  password : 'test_password',
  database : 'testdb',
  dateStrings: false
});

// imagine a db.connect(...) somewhere here, and all the server bootstrapping

// make a request object
request = require('supertest')('http://localhost:3000');

thisDate = new Date();
db.query("INSERT INTO `File` (created, name) VALUES (?, ?, ?)", [thisDate, "testFile.txt"], (err, res) => {
  db.query("SELECT * FROM `File`", function (err, rows, fields) {
    // Get the file through the api
    request.get('/api/Files')
    .expect(200)
    .expect('Content-Type', /json/).end((err, res) => {
      var files = res.body;
      var file = files[0];

      var createdDate = new Date(file.created);
      console.log("ThisDate", thisDate);
      console.log("ThisDate toJSON", thisDate.toJSON());
      console.log("MysqlJS toJSON", rows[0].created.toJSON());
      console.log("MysqlJS", rows[0].created);
      console.log("LoopBack", file.created);
      console.log("CreatedDate toJSON", createdDate.toJSON());

      done(err);
    });
  });
});

The result looked like this:

ThisDate Tue Jul 26 2016 13:49:04 GMT+0100 (BST)
ThisDate toJSON 2016-07-26T12:49:04.039Z
MysqlJS toJSON 2016-07-26T12:49:04.000Z
MysqlJS Tue Jul 26 2016 13:49:04 GMT+0100 (BST)
LoopBack 2016-07-26T13:49:04.000Z
CreatedDate toJSON 2016-07-26T13:49:04.000Z

I'm in London, but due to summer time I'm in GMT+1. So, while mysqlJS correctly converts UTC back to local time, loopback ignores the local timezone.

@bbito
Copy link
Contributor

bbito commented Jul 26, 2016

@DaGaMs - that sure is confusing!
What does your loopback /common/models/file.json model definition look like?
..and what does the value look like in mysql?
mysql> select * from File;

@bbito
Copy link
Contributor

bbito commented Jul 26, 2016

When I PUT your ThisDate toJSON value ("2016-07-26T12:49:04.039Z") through loopback API Explorer it persists unchanged in mysql (less ms):

mysql> select mft_datetime, mft_date_time_notes from date_time_table where id=12;
+---------------------+-----------------------------------------+
| mft_datetime        | mft_date_time_notes                     |
+---------------------+-----------------------------------------+
| 2016-07-26 12:49:04 | from DaGaMs as 2016-07-26T12:49:04.039Z |
+---------------------+-----------------------------------------+

When I retrieve it through loopback: http://192.168.0.184:3000/api/DateTimeTables/12
It is still the same UTC date (less ms):

{
  "id": 12,
  "mftDateTimeNotes": "from DaGaMs as 2016-07-26T12:49:04.039Z",
  "mftDatetime": "2016-07-26T12:49:04.000Z"
}

@DaGaMs
Copy link
Contributor

DaGaMs commented Jul 26, 2016

My File.json file looks like this:

{
  "name": "File",
  "base": "PersistedModel",
  "strict": true,
  "idInjection": true,
  "options": {
    "validateUpsert": true
  },
  "mysql": {
    "schema": "testdb",
    "table": "File"
  },
  "properties": {
    "created": {
      "type": "date",
      "required": true,
      "length": null,
      "precision": null,
      "scale": null,
      "mysql": {
        "columnName": "created",
        "dataType": "datetime",
        "dataLength": null,
        "dataPrecision": null,
        "dataScale": null,
        "nullable": "N"
      },
      "_selectable": false
    },
    "path": {
      "type": "string",
      "required": true,
      "length": 4096,
      "precision": null,
      "scale": null,
      "mysql": {
        "columnName": "path",
        "dataType": "varchar",
        "dataLength": 4096,
        "dataPrecision": null,
        "dataScale": null,
        "nullable": "N"
      },
      "_selectable": false
    }
  },
  "validations": [],
  "relations": {},
  "acls": [],
  "methods": {}
}

the datasource.json file is pretty simple:

{
  "db": {
    "name": "db",
    "connector": "memory"
  },
  "mysql": {
    "host": "localhost",
    "port": 3306,
    "database": "testdb",
    "password": "test_password",
    "name": "mysql",
    "user": "test_user",
    "connector": "mysql"
  }
}

@DaGaMs
Copy link
Contributor

DaGaMs commented Jul 26, 2016

The File table is generated during migration, so I'm not doing anything weird in mysql. describe File gives me this:

mysql> describe File;
+-------------+---------------+------+-----+---------+----------------+
| Field       | Type          | Null | Key | Default | Extra          |
+-------------+---------------+------+-----+---------+----------------+
| id          | int(11)       | NO   | PRI | NULL    | auto_increment |
| created     | datetime      | NO   |     | NULL    |                |
| path        | varchar(4096) | NO   |     | NULL    |                |
+-------------+---------------+------+-----+---------+----------------+

I ran the script again, getting these dates:

ThisDate Tue Jul 26 2016 18:09:17 GMT+0100 (BST)
LoopBack 2016-07-26T18:09:18.000Z

Now, here's what's in the database:

mysql> select * from File;
+----+---------------------+---------------------------------+
| id | created             | path                            |
+----+---------------------+---------------------------------+
|  1 | 2016-07-26 18:09:18 | /some/path/to/file/testFile.txt |
+----+---------------------+---------------------------------+

So, what seems to be happening is that when I INSERT the row into the database using the mysqlJS adapter, the date gets saved in local time. That's in line with the mysql documentation as far as I can tell. However, loopback seems to assume that whatever comes out of the database is in UTC.

Looking at the output of the console.logs earlier, the most obvious explanation for this is that loopback doesn't use toJSON but somehow converts the Date object returned by mysqlJS into a string and back into a date, dropping the timezone information...

@bbito
Copy link
Contributor

bbito commented Jul 26, 2016

Very interesting!
Although loopback checks the timezone config setting:
https://github.com/strongloop/loopback-connector-mysql/blob/master/lib/mysql.js#L114
...it doesn't seem to do anything with that except make sure it is passed on to mysqljs and just converts to UTC regardless:
https://github.com/strongloop/loopback-connector-mysql/blob/master/lib/mysql.js#L352

  if (prop.type === Date) {
    if (!val.toUTCString) {
      val = new Date(val);
    }
    return dateToMysql(val);
  }

https://github.com/strongloop/loopback-connector-mysql/blob/master/lib/mysql.js#L307

function dateToMysql(val) {
  return val.getUTCFullYear() + '-' +
    fillZeros(val.getUTCMonth() + 1) + '-' +
    fillZeros(val.getUTCDate()) + ' ' +
    fillZeros(val.getUTCHours()) + ':' +
    fillZeros(val.getUTCMinutes()) + ':' +
    fillZeros(val.getUTCSeconds());

  function fillZeros(v) {
    return v < 10 ? '0' + v : v;
  }
}

It seems that the mysqljs timezone setting is checked and passed to the connection, but dateToMysql is called to make a UTC Date regardless and I don't understand why toJSON isn't simply called if that is the intent anyway...

@bbito
Copy link
Contributor

bbito commented Jul 26, 2016

I don't know what it breaks, but if I remove that call to dateToMysql(val), then the DATETIME is stored as local (as expected).
https://github.com/bbito/loopback-connector-mysql/blob/2.x-latest/lib/mysql.js#L298
Here's the diff link:
strongloop:loopback-2.x...bbito:2.x-latest
[Edited: Was modifying Master, Now basing off 2.x-latest which matches my loopback install]

When I send the same data as above it now gets stored as:

mysql> select mft_datetime, mft_date_time_notes from date_time_table where id=13;
+---------------------+-----------------------------------------+
| mft_datetime        | mft_date_time_notes                     |
+---------------------+-----------------------------------------+
| 2016-07-26 05:49:04 | from DaGaMs as 2016-07-26T12:49:04.039Z |
+---------------------+-----------------------------------------+

The return through loopback is still the same as well.

@bbito
Copy link
Contributor

bbito commented Jul 26, 2016

I don't understand why Loopback-Connector isn't just handing off a date object to mysqljs... That seems to provide the correct behavior in the case of my forked branch... As @DaGaMs found, the current behavior disregards the timezone option and stores as UTC which will cause problems when storing or accessing DATETIME or TIMESTAMP values with other connectors that DO respect the 'local' option for timezone (which is default).

@bbito
Copy link
Contributor

bbito commented Jul 26, 2016

@superkhau do you know why that conversion to UTC date string is being performed via dateToMysql(val)? Unfortunately a fix will throw off any server-side date comparisons since all existing commits through loopback will be UTC regardless of the mysql timezone setting including the default of 'local'.

@bbito
Copy link
Contributor

bbito commented Jul 26, 2016

@DaGaMs - Could you try re-running your test with my patch?
EDIT: now based off 2.x-latest branch which is correct for my loopback install v2.2.1
https://github.com/bbito/loopback-connector-mysql/blob/2.x-latest/lib/mysql.js
I also made a testing branch with logging within the toColumnValue and fromColumnValue functions:
https://github.com/bbito/loopback-connector-mysql/blob/2.x-latest-bbito-testing/lib/mysql.js
-Thanks!

@superkhau
Copy link
Contributor

@bbito I do not know off the the top of my head. @bajtos or @raymondfeng Do you know?

@bbito
Copy link
Contributor

bbito commented Jul 28, 2016

@superkhau what I have found is that the current code (v2.2.1) always commits UTC to mysql. The change I made as I understand it just allows the JS Date object to be passed to the driver. Without this change, neither the default timezone setting of 'local' nor any other setting for timezone effects the committed DATETIME field. With the change I am finding the the default 'local' setting is honored and that changing the setting to 'UTC' changes the commit to UTC.

@bbito
Copy link
Contributor

bbito commented Jul 28, 2016

I'm only working with the Explorer right now, but in my exploration so far I'm always finding that val is already a Date object when it gets to toColumnValue or fromColumnValue, and allowing that object to travel on from either function unchanged seems to fix the issue, but I don't know if there are situations where it might not be a Date object....

@bajtos
Copy link
Member

bajtos commented Jul 28, 2016

I don't have a good enough understanding of this problem, but based on the discussion above, this looks like a bug to me.

One more thing to consider: let's say the database is configured to use British Summer Time when storing DateTime values. And let's say a REST client sends a date in a different timezone, e.g. Mon Jul 25 2016 17:05:03 GMT-0700. What should happen? I would expect that the date gets converted to BST and stored to the database in that timezone. Later, when returning a response with data from the database, the value should be converted to ISO string in GMT. Is it a reasonable expectation?

@DaGaMs
Copy link
Contributor

DaGaMs commented Jul 28, 2016

Just to make things complicated, there's another thing to consider: according to the MySQL docs:

MySQL converts TIMESTAMP values from the current time zone to UTC for storage, and back from UTC to the current time zone for retrieval. (This does not occur for other types such as DATETIME.)

So potentially DATETIME and TIMESTAMP values have to be treated differently?

I'm afraid it will be difficult to fix this issue due to the regressions it might cause - currently, all code that enters dates/times through the api will store data as UTC in the database, but code that writes to the database directly needs to ensure that values are UTC or otherwise loopback will mess up the timezone. For now, I've resorted to the making all DATETIME entries in the database UTC.

@bbito
Copy link
Contributor

bbito commented Jul 28, 2016

One more thing to consider: let's say the database is configured to use British Summer Time when storing DateTime values. And let's say a REST client sends a date in a different timezone, e.g. Mon Jul 25 2016 17:05:03 GMT-0700. What should happen? I would expect that the date gets converted to BST and stored to the database in that timezone. Later, when returning a response with data from the database, the value should be converted to ISO string in GMT. Is it a reasonable expectation?

@bajtos I think this is over-thinking the problem. I'm sure there are many challenges in working with time-based data in systems where clients are in multiple time zones, but I don't think it is the responsibility of Loopback to figure those out. The bug as I see it is that Loopback is neither honoring the timezone setting of the mysql connection nor documenting its current behavior that all DATETIME commits will be stored as UTC. The current behavior gets the same value out of mysql that goes in as long as Loopback is the only interface working with that database. Problems arise as @DaGaMs demonstrated when another interface that does respect the timezone setting is also manipulating the db. I think the fix does not involve any "converted to ISO string in GMT" but simply taking loopback-connector-mysql out of the way of the mysqljs/mysql driver by only passing unaltered JS Date objects to mysqljs/mysql for commit and only serializing the JS Date object provided by mysqljs/mysql from queries with toJSON or equivalent when constructing the API payload. Unfortunately, as @DaGaMs mentioned, a fix that honors the connection's timezone setting will cause issues with server-side code performing time comparisons in existing Loopback systems. Perhaps it needs to be documented that Loopback forces commits as UTC unless a setting flag e.g. enableMysqlTimezone is set which then provides the behavior found in my patch.

@bbito
Copy link
Contributor

bbito commented Jul 28, 2016

@DaGaMs

Just to make things complicated, there's another thing to consider: according to the MySQL docs:

MySQL converts TIMESTAMP values from the current time zone to UTC for storage, and back from UTC to the current time zone for retrieval. (This does not occur for other types such as DATETIME.)

So potentially DATETIME and TIMESTAMP values have to be treated differently?

Edit: 20170412: darknos has simple test that contradicts my read of TIMESTAMP vs DATETIME here:
#257 (comment)

I have confirmed with a simple MySQL command line test producing similar results

I think that is a kind of red herring. My understanding is that it is a description of the internal MySQL mechanism that does not actually affect commits or queries. If I send the same dateObject.toJSON() to both a DATETIME and a TIMESTAMP field with the timezone set to 'local' (PDT for me) and query with the MySQL Command Line Client they are identical:

mysql> select * from date_time_table where id=25;
+----+------------+---------------------+---------------------+----------+------------------------------------------------------------------------------------------------------+
| id | mft_date   | mft_datetime        | mft_timestamp       | mft_time | mft_date_time_notes
                |
+----+------------+---------------------+---------------------+----------+------------------------------------------------------------------------------------------------------+
| 25 | 2016-07-27 | 2016-07-27 01:00:00 | 2016-07-27 01:00:00 | 01:00:00 | 7/27/16 - 1:00am PDT toJSON = 2016-07-27T08:00:00.005Z, timezone: Removed, should default to 'local' |
+----+------------+---------------------+---------------------+----------+------------------------------------------------------------------------------------------------------+

Here's the table description for clarity and the Loopback behavior with my patch:

mysql> describe date_time_table;
+---------------------+--------------+------+-----+---------+----------------+
| Field               | Type         | Null | Key | Default | Extra          |
+---------------------+--------------+------+-----+---------+----------------+
| id                  | int(11)      | NO   | PRI | NULL    | auto_increment |
| mft_date            | date         | YES  |     | NULL    |                |
| mft_datetime        | datetime     | YES  |     | NULL    |                |
| mft_timestamp       | timestamp    | YES  |     | NULL    |                |
| mft_time            | time         | YES  |     | NULL    |                |
| mft_date_time_notes | varchar(255) | YES  |     | NULL    |                |
+---------------------+--------------+------+-----+---------+----------------+

Data sent through Loopback:

  {
    "id": 0,
    "mftDate": "2016-07-27",
    "mftDateTimeNotes": "7/27/16 - 1:00am PDT toJSON = 2016-07-27T08:00:00.005Z, timezone: Removed, should default to 'local'",
    "mftDatetime": "2016-07-27T08:00:00.000Z",
    "mftTime": "01:00:00",
    "mftTimestamp": "2016-07-27T08:00:00.000Z"
  }

Data returned from Loopback * Note, I am using my DATE patch for mysqljs/mysql and my Loopback model definition treats mftDate as a string in Loopback and a DATE in MySQL.

  {
    "id": 25,
    "mftDate": "2016-07-27",
    "mftDateTimeNotes": "7/27/16 - 1:00am PDT toJSON = 2016-07-27T08:00:00.005Z, timezone: Removed, should default to 'local'",
    "mftDatetime": "2016-07-27T08:00:00.000Z",
    "mftTime": "01:00:00",
    "mftTimestamp": "2016-07-27T08:00:00.000Z"
  }

Finally, here's the model definition - date-time-table.json:

{
  "name": "DateTimeTable",
  "base": "PersistedModel",
  "idInjection": false,
  "options": {
    "validateUpsert": true
  },
  "mysql": {
    "schema": "mft_date_time_tests",
    "table": "date_time_table"
  },
  "properties": {
    "id": {
      "type": "number",
      "id": true,
      "required": true,
      "length": null,
      "precision": 10,
      "scale": 0,
      "mysql": {
        "columnName": "id",
        "dataType": "int",
        "dataLength": null,
        "dataPrecision": 10,
        "dataScale": 0,
        "nullable": "N"
      },
      "_selectable": false
    },
    "mftDate": {
      "type": "string",
      "required": false,
      "length": null,
      "precision": null,
      "scale": null,
      "mysql": {
        "columnName": "mft_date",
        "dataType": "date",
        "dataLength": null,
        "dataPrecision": null,
        "dataScale": null,
        "nullable": "Y"
      },
      "_selectable": true
    },
    "mftDateTimeNotes": {
      "type": "string",
      "required": false,
      "length": 255,
      "precision": null,
      "scale": null,
      "mysql": {
        "columnName": "mft_date_time_notes",
        "dataType": "varchar",
        "dataLength": 255,
        "dataPrecision": null,
        "dataScale": null,
        "nullable": "Y"
      },
      "_selectable": true
    },
    "mftDatetime": {
      "type": "date",
      "required": false,
      "length": null,
      "precision": null,
      "scale": null,
      "mysql": {
        "columnName": "mft_datetime",
        "dataType": "datetime",
        "dataLength": null,
        "dataPrecision": null,
        "dataScale": null,
        "nullable": "Y"
      },
      "_selectable": true
    },
    "mftTime": {
      "type": "string",
      "required": false,
      "length": null,
      "precision": null,
      "scale": null,
      "mysql": {
        "columnName": "mft_time",
        "dataType": "time",
        "dataLength": null,
        "dataPrecision": null,
        "dataScale": null,
        "nullable": "Y"
      },
      "_selectable": true
    },
    "mftTimestamp": {
      "type": "date",
      "required": false,
      "length": null,
      "precision": null,
      "scale": null,
      "mysql": {
        "columnName": "mft_timestamp",
        "dataType": "timestamp",
        "dataLength": null,
        "dataPrecision": null,
        "dataScale": null,
        "nullable": "Y"
      },
      "_selectable": true
    }
  },
  "validations": [],
  "relations": {},
  "acls": [],
  "methods": {}
}

@DaGaMs
Copy link
Contributor

DaGaMs commented Jul 28, 2016

@bbito Ah, I see. It appears that I misinterpreted the mysql docs there.

@bajtos
Copy link
Member

bajtos commented Jul 29, 2016

The bug as I see it is that Loopback is neither honoring the timezone setting of the mysql connection nor documenting its current behavior that all DATETIME commits will be stored as UTC. The current behavior gets the same value out of mysql that goes in as long as Loopback is the only interface working with that database. Problems arise as @DaGaMs demonstrated when another interface that does respect the timezone setting is also manipulating the db.

That would explain why this problem was not reported earlier, as it affects only a small set of LoopBack users.

I think the fix does not involve any "converted to ISO string in GMT" but simply taking loopback-connector-mysql out of the way of the mysqljs/mysql driver by only passing unaltered JS Date objects to mysqljs/mysql for commit and only serializing the JS Date object provided by mysqljs/mysql from queries with toJSON or equivalent when constructing the API payload.

This sounds like a very reasonable solution to me.

I see one potential caveat: how to handle SQL query parameters, e.g. when converting a LoopBack request /find?filter={ where: { start: { gt: '2016-01-01T9:00:00Z' }}} to an SQL query SELECT ... WHERE START >= ?. Does the mysql driver handle timezones for parameterized queries?

Unfortunately, as @DaGaMs mentioned, a fix that honors the connection's timezone setting will cause issues with server-side code performing time comparisons in existing Loopback systems. Perhaps it needs to be documented that Loopback forces commits as UTC unless a setting flag e.g. enableMysqlTimezone is set which then provides the behavior found in my patch.

I agree that the fix proposed here is a breaking change that must be handled carefully.

I am proposing the following approach:

  • Change the default behaviour of this connector as described above
  • Add a compatibility flag allowing users to opt into old behaviour - this will simplify upgrade from the current version of the connector
  • Provide documentation on how to upgrade data stored in MySQL from the UTC format to the new format, to allow users to eventually switch off the compat flag.
  • Release the changes as a new major version of the connector.

@raymondfeng @superkhau @0candy @loay thoughts?
/cc @Amir-61 @jannyHou

bbito added a commit to bbito/loopback-connector-mysql that referenced this issue Apr 25, 2017
This commit contains all previous work after rebase went badly
RE: Pull Request requested by @kjdelisle regarding loopbackio#149 following my
proposed solution "A" at loopbackio#149 (comment).
As mentioned in the post linked above, this change allows the
underlying mysqljs/mysql module to handle Date object serialization
and removes the forced conversion of Dates to UTC Strings.
An opt-out fallback to forced coercion to UTC is included,
which was modeled after loopbackio#265 from @darknos .

Old, squashed commits:
d0ea1d9
Legacy UTC date processing fallback credit: @darknos

a59dad7
Remove orphaned string functions

e8fdbdc
Incorporate @darknos expanded check for zero dates

abd4e0a
Remove DATE manipulations in from/toColumnValue
@bbito
Copy link
Contributor

bbito commented Apr 25, 2017

#268 was borked. I didn't realize @ssh24 was helping fix the commit comments while I was trying to rebase at the same time.
Replaced with #271

@darknos
Copy link
Contributor

darknos commented Apr 25, 2017

@bbito

I'm assuming that function DateType(arg) {... is in a module that receives that arg from a loopback-connector-xxx like fromColumnValue in loopback-connector-mysql - if that is the case, it looks like it is expecting a String. Are the other 'connector' modules handing off Strings instead of Date objects?

no. DateType function works when you pass something in {"where":"...."}, for example from rest interface. and fromColumnValue called AFTER DateType. If you want to remove all conversion from the connector you should rid out this one too.

try to put some console.log in DateType in dao.js and in fromColumnValue to examine how it works.

@kjdelisle
Copy link
Contributor

With regard to #271 everything looks good except for the DATE format, which I think we knew was going to be problematic. Inserting basic dates from end-to-end can fall victim to the timezone which can roll them forwards/backwards. (ex. inserting 2017-02-04 from GMT-5 will give you 2017-02-03)

I've tested the isDateString change on a branch of juggler and it seems to provide the appropriate behaviour for DATE fields (namely, no UTC-ification). Tomorrow I'll start cranking out some tests for #271 and my own branch (loopbackio/loopback-datasource-juggler#1356).

@bbito
Copy link
Contributor

bbito commented Apr 26, 2017

@kjdelisle

With regard to #271 everything looks good except for the DATE format

Wow, I'm very surprised! I was sure that #271 would break all sorts of stuff I hadn't tested for. I've just been pounding on what happens when I send and retrieve Date types through REST API and checking MySQL rows. I thought changing the output of toColumnValue from String to Date Object would break stuff I wasn't looking at...

Concerning DATEs, I plan to continue to use "dateStrings": ["DATE"] in datasources.json because I just don't think a js Date Object is a safe representation of that MySQL data type.

@kjdelisle
Copy link
Contributor

kjdelisle commented Apr 26, 2017

@bbito Ah, sorry! To clarify: "looks good" means that I was manually verifying it to make sure that it's doing what I expect in combination with the other changes in my PR (I'm not going to merge one to test the other, so I linked them locally to play around with it). It does have a few failing tests, but they look unrelated.

The loopbackio/loopback-datasource-juggler#1356 PR will allow you to pass values from your model to your database column as a raw string (but still check that they're valid "dates"), which is what I was manually verifying in conjunction with your PR.

I've been held up quite a bit today (we're running some planning and such), so I'll be getting to this late in the day.

You would still want to use "dateStrings": ["DATE"] to make sure that mysqljs doesn't convert them back into Dates on the return pass, as you say. The two of these things combined should allow for a complete use case regarding DATE types in MySQL (and hopefully, any other SQL database with a DATE-like type where timezones don't apply, but this is obviously driver-dependent as well).

@bbito
Copy link
Contributor

bbito commented Apr 26, 2017

@kjdelisle Addressing your earlier comment:

dob: { type: Date, isDateString: true } // This will not be

I find this very confusing. Currently I can do this at the DataSource level - not the field/model level as you propose, but the behavior seems fairly transparent to me from the settings below.

  • Set "dateStrings": ["DATE"] in datasources.json
    • This forces mysqljs/mysql to return a String exactly as read from MySQL for all DATE columns of the form 2017-04-18 regardless of timezone settings
  • Set Model field to be a String in model.json: "myDate": { "type": "string", ... "mysql": { "columnName": "my_date", "dataType": "date", ... }, "_selectable": true },
    • This makes sure that there is no typecasting for this DATE instance within Loopback.

I don't know what to make of { type: Date, isDateString: true } - When/where will it be a Date and when/where will it be a String?

@kjdelisle
Copy link
Contributor

@bbito
Technically, you can set the datatype of the field on the Model as a string, and then set a connector-specific mapping behaviour (This is the workaround from further up the thread):

"myDate": {
      "type": "string",
      "required": false,
      "length": null,
      "precision": null,
      "scale": null,
      "mysql": {
        "columnName": "my_date",
        "dataType": "date",
        "dataLength": null,
        "dataPrecision": null,
        "dataScale": null,
        "nullable": "Y"
      },
      "_selectable": true
    },

But this means that you lose validation of the date at LoopBack level; nothing's stopping you from sending in a non-Date string!
This new feature would let you set the type to "Date" (as far as LoopBack is concerned) but it would actually be a String that was validated to ensure it was a representation of a Date.

This way, if you wanted to insert 2015-01-01 into your DATE column, it won't first be made into a Date object. As a result, the timezone setting (which shouldn't be affecting a DATE) won't modify your inserted value to something like 2014-12-31T19:00:00.000Z which would end up getting truncated to 2014-12-31. The dateStrings option will handle the pass-back to make sure you receive a string on the reverse pass.

@darknos
Copy link
Contributor

darknos commented Apr 26, 2017

I'm agree with @bbito but notice that dataType should be on upper level for correct migrations:

"myDate": {
      "type": "string",
      "dataType": "date",
...
}

@bbito
Copy link
Contributor

bbito commented Apr 26, 2017

@kjdelisle

This new feature would let you set the type to "Date" (as far as LoopBack is concerned) but it would actually be a String that was validated to ensure it was a representation of a Date.

I like the sound of this feature, but I'm suspicious of how SELECTs through mysqljs/mysql will pull the DATE String from MySQL. The only way I know to be sure I'll get the same DATE whether I'm in -12:00 or +14:00 is by passing "dateStrings": ["DATE"] to mysqljs/mysql.

@kjdelisle
Copy link
Contributor

kjdelisle commented Apr 26, 2017

What I'm saying is that the isDateString property is complementary to the use of dateStrings. isDateString keeps your value the same on the way into the database from LB, and dateStrings keeps it the same on the way back out. You'd use both at the same time.

@bbito
Copy link
Contributor

bbito commented Apr 26, 2017

@kjdelisle Thanks for the clarification, that squashes my concern about the data! Great!
I do still find the attribute names confusing though. I would always assume type: Date is referring to a js Date Object.

Instead of the 2 attributes: { type: Date, isDateString: true } could the type be something new? e.g.: { type: DateAsString }

@kjdelisle
Copy link
Contributor

If I did that, I'd have to create a new Type throughout the model-builder and dao classes. Doing that might be the "nicer" way, but I'm not really convinced that we gain anything by doing that (or lose anything by not doing that).

@bbito
Copy link
Contributor

bbito commented Apr 26, 2017

@kjdelisle While I'll admit I was hoping you'd say "I've been itching to create a new Type!" I understand if it's not a good use of resources. I think the gain is that Loopback has cool tools like ARC > Discover Models where it would be nice to select DateAsString from the dropdown and be done with the config rather than having to go into the generated model.json files and add isDateString: true.

@kjdelisle
Copy link
Contributor

Giving it some thought, I do want this to be an easier process for others, and having it as a separate type would be a heck of a lot easier. Let's see how that goes.

kjdelisle pushed a commit to bbito/loopback-connector-mysql that referenced this issue Apr 28, 2017
This commit contains all previous work after rebase went badly
RE: Pull Request requested by @kjdelisle regarding loopbackio#149 following my
proposed solution "A" at loopbackio#149 (comment).
As mentioned in the post linked above, this change allows the
underlying mysqljs/mysql module to handle Date object serialization
and removes the forced conversion of Dates to UTC Strings.
An opt-out fallback to forced coercion to UTC is included,
which was modeled after loopbackio#265 from @darknos .

Old, squashed commits:
d0ea1d9
Legacy UTC date processing fallback credit: @darknos

a59dad7
Remove orphaned string functions

e8fdbdc
Incorporate @darknos expanded check for zero dates

abd4e0a
Remove DATE manipulations in from/toColumnValue
@kjdelisle kjdelisle added this to the Sprint 35 - Apex milestone May 1, 2017
@kjdelisle
Copy link
Contributor

All the relevant PRs are merged, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants