Keep milliseconds when converting date objects to a SQL-friendly format

[lafave]

Currently timestamps cannot be stored w/ millisecond precision. See: https://github.com/balderdashy/sails-postgresql/issues/105, https://github.com/balderdashy/waterline/issues/624, both of which were closed due to the actual issue not residing in those modules.

I don't see an issue opened in waterline-sequel regarding this but I believe that this is the module responsible for the bug.

toUTCString() drops milliseconds whereas toISOString() doesn't. I've switched to using toISOString on the UTC date (couldn't find another way to get at it aside from manually building one piece at a time -- open to suggestions).

It's worth noting that toUTCString() and toISOString() produce different outputs. Postgres, anyways, is cool with either. Although seemingly unlikely, it's possible that other database won't be okay with an ISO8601-formatted timestamp (the output of toISOString()).

[dylanpyle]

var

[PatriotBob]

This bit me earlier when I had to track down why ordering by createdAt wasn't always accurate. I'm going to have to deploy a local fork until this gets merged.

I think toISOString() already ensures that a date is in UTC. Have a look at MDN's polyfill. Creating a new date using from UTC and then calling toISOString() like what you're doing would effectively apply the timezone offset twice. We should be able to just replace toUTCString() with toISOString().

[dmarcelino]

Hi @lafave, @PatriotBob, is this change compatible with mysql, cassandra, oracledb and postgresql? The adapters for these dbs also use waterline-sequel.

[PatriotBob]

@dmarcelino I have only tested this on postgresql at the moment. While I absolutely understand the need to verify this works on all currently supported dbs, truncating milliseconds from the timestamp seems to be an aggressive solution. If we can't make a universal change due to differences in the in the dbs, shouldn't this method be moved to the adapters?

I know sails-postgresql has the exact same method. The entire file looks fairly similar, but I wasn't ever able to get their util.toSqlDate to even be called. So perhaps rather than risking breaking changes on waterline-sequel we can delegate converting dates to strings to the adapter that support it.

[dmarcelino]

@PatriotBob, that's precisely my concern, we can't break other adapters. If it's something specific to the each adapter than yes, each adapter should handle it. I'm not (yet) familiar with the code of the SQL adapters but we'll need to ensure compatibility.

Another option not discussed yet is to make utils.toSqlDate return different outputs based on some config, assuming not all adapters support milliseconds.

[tobalsgithub]

So what's the best workaround in the meantime? Here's what we're doing but it feels pretty hacky.

In our sails bootstrap function...

var sequelUtils = require('../node_modules/sails-postgresql/node_modules/waterline-sequel/sequel/lib/utils');

  // https://github.com/balderdashy/waterline-sequel/pull/24
  sequelUtils.toSqlDate = function (date) {
    var utc = new Date(
      date.getUTCFullYear(),
      date.getUTCMonth(),
      date.getUTCDate(),
      date.getUTCHours(),
      date.getUTCMinutes(),
      date.getUTCSeconds(),
      date.getUTCMilliseconds()
    );
    return utc.toISOString();
  };

Essentially we're just overwriting the function definition with the pull request version. We're using sails, so this may not work as well in an app that's not using sails.

Any better ideas out there?

[kevinburkeshyp]

Fork the lib? that's what we did.

[tobalsgithub]

@kevinburkeshyp Yeah, thought about it. Wanted to continue getting updates automatically if possible. May end up forking if the hack causes issues. Thanks.

[dmarcelino]

@lafave, can you merge master into your branch so it runs the unit and integration tests? That would tell us if this change breaks sails-mysql or sails-postgresql (assuming the tests cover dates).

[lafave]

@dmarcelino done

[dmarcelino]

So, unit tests (travis) pass and the integration tests (circleci) don't show any new error which is encouraging. @devinivy, @tjwebb, what do you think of this change?

[tobalsgithub]

After playing with this a bit, I noticed that my timezones were off. With the proposed change, we're taking a date object generated from new Date(), creating a new date from that using all the getUTC functions, which actually pushes the newly created date off by however far your current timezone is off from UTC, then we're returning toISOString().

For example, I'm in MDT (GMT -0600).

sails> var d = new Date();
undefined
sails> d
Wed Apr 08 2015 10:33:05 GMT-0600 (MDT)
sails> d.toUTCString();
'Wed, 08 Apr 2015 16:33:05 GMT'
sails> d.toISOString();
'2015-04-08T16:33:05.224Z'
sails> var x = new Date(d.getUTCFullYear(), d.getUTCMonth(), d.getUTCDate(), d.getUTCHours(), d.getUTCMinutes(), d.getUTCSeconds(), d.getUTCMilliseconds());
undefined
sails> x
Wed Apr 08 2015 16:33:05 GMT-0600 (MDT)
sails> x.toUTCString();
'Wed, 08 Apr 2015 22:33:05 GMT'
sails> x.toISOString();
'2015-04-08T22:33:05.224Z'
sails> 

Notice how x is now off by 6 hours.

Is there a reason we need to create a new date based on the one passed in? Why can't we just return date.toISOString()?

utils.toSqlDate = function (date) {
    return date.toISOString();
  };

[devinivy]

@tobalsgithub I see no reason why not. And good catch! I think that the ISO strings are completely fine, given that Date understands them just as well as the UTC strings. My concern would be that databases could start to be filled with times in different formats if they store the string representation. Perhaps timestamp can have two sizes. Is that too confusing?

{
    timeMs: {
        type: 'timestamp',
        size: 'short' // default, stores up to the second
    },
    timeSec: {
        type: 'timestamp',
        size: 'long' // stores ms
    }
}

[tobalsgithub]

I'm not super familiar with this stuff, just sort of happened upon this yesterday, so I'll ask a noob question if you don't mind.

Doesn't this toSqlDate function get called for any Date object that's getting inserted into the DB? I believe there's a check for _.isDate that is the gatekeeper for this function getting called. So how is it possible that two different dates could be stored with different lengths?

[tobalsgithub]

@devinivy, not sure I understood your question about the size of the date strings. Can you elaborate? Would be happy to help with the PR if I can understand your concern more fully.

[devinivy]

I mean, if this PR were to go through and someone were to upgrade, then their db would include timestamps of two different formats. We can preserve backwards compatibility while supporting this feature by keying the date format (whether or not it includes milliseconds) on the attribute's size option. Am I communicating that well enough, or is it still confusing?

[tobalsgithub]

Sorry for the delay. I was on vacation. Yeah, this makes sense now. Not sure I like having to specify the size, but seems like it might be necessary. So, somehow we'd have to pass the size parameter through to the toSqlDate function. Is that what you're thinking?

[devinivy]

Yeah, it's just one idea. The adapter could also expose the "sizes" as constants.

var Mysql = require('sails-mysql');

// ...
  size: Mysql.TIME_MILLISECONDS
// ...

[tobalsgithub]

Interesting. I'd have to dig into this a lot more to understand how it all fits together. Currently we're just doing this in our config/bootstrap.js file:

var sequelUtils = require('../node_modules/sails-postgresql/node_modules/waterline-sequel/sequel/lib/utils');

// https://github.com/balderdashy/waterline-sequel/pull/24
sequelUtils.toSqlDate = function (date) {
  return date.toISOString();
};

But we don't care about backwards compatibility in this case.

[joshuamarquez]

for sails-mysql@0.11.5 I had to do this:

  const sequelUtils = require('../node_modules/sails-mysql/lib/utils');

  // https://github.com/balderdashy/waterline-sequel/pull/24
  sequelUtils.toSqlDate = function (date) {
    date = date.getFullYear() + '-' +
      ('00' + (date.getMonth()+1)).slice(-2) + '-' +
      ('00' + date.getDate()).slice(-2) + ' ' +
      ('00' + date.getHours()).slice(-2) + ':' +
      ('00' + date.getMinutes()).slice(-2) + ':' +
      ('00' + date.getSeconds()).slice(-2) + '.' +
      ('000' + date.getMilliseconds()).slice(-3);

    return date;
  };