Add DateString type #1356

kjdelisle · 2017-04-26T00:20:00Z

Description

New type that preserves string input as a string, but ensures that
the string is a valid Date.

Additionally, provides a .toDate function to provide the Date
object representation of the string.

NOTE: This PR has changed from its original intent, so if you feel confused, you should know that you're not crazy. :)

Linked Issues

loopbackio/loopback-connector-mysql#149

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide
Write the Doc PR that describes this feature's usage with examples!

kjdelisle · 2017-04-26T00:25:11Z

I know there's been quite a bit of spitballing regarding this MySQL date issue, but it got me thinking about the idea of "DATE" type fields in databases, and how there's easily an argument for not wanting timezones to interfere with their use. If you were attempting to insert a birthday into a database, you wouldn't want the timezone offset to push that day forwards or backwards, right?

This change would give users the ability to fall back on a string representation that still acts as a Date, and is still validated by the JS Date constructor, but without being converted to UTC. It would be on a per-property basis.

~~(I know, I know, another config flag 🤢 )~~
"Good news, everyone!" /farnsworth
No config flag with this alternate approach.

ssh24 · 2017-04-26T20:49:38Z

lib/model-builder.js

@@ -526,10 +527,13 @@ ModelBuilder.prototype.define = function defineClass(className, properties, sett
 };

 // DataType for Date
-function DateType(arg) {
+function DateType(arg, props) {


What was the reasoning behind have this function both in dao and modelBuilder?

kjdelisle · 2017-04-26T22:04:41Z

Still a work in progress.

bajtos · 2017-04-27T12:35:07Z

lib/date-string.js

+    throw new Error('Input must be a string');
+  }
+  // Preserve the reference for return via .toDate
+  this.date = new Date(value);


This allows users to include the time in the value too, e.g. 2015-01-01T10:00:00.0Z.

If we want to make this new type for dates only, then I am proposing to accept a single format yyyy-MM-dd only.

I was calling it DateString to reflect that it's like a JS Date (which is actually a Date and Time), but in string form.

It's mostly meant to allow the passing of unmodified date/time strings to an underlying driver, which in MySQL's case just happens to be helpful for DATE.

bajtos · 2017-04-27T12:45:11Z

lib/date-string.js

+ * Return the equivalent Date object for this DateString.
+ * @returns {Date} A JavaScript Date object
+ */
+DateString.prototype.toDate = function() {


IMO, this function needs to be parameterized by the timezone we want to get the value in. Otherwise you can end up with the same problem when somebody calls toLocaleString as you are trying to fix: if you were attempting to insert a birthday into a database, you wouldn't want the timezone offset to push that day forwards or backwards, right?

$ node > new Date('2017-04-27').toLocaleString(undefined, {timeZone:'America/Toronto'}) '4/26/2017, 8:00:00 PM'

The snippet above shows what happens to value 2017-04-27 when it's displayed via toLocaleString() by a computer located in Toronto (think of new DateString('2017-04-27').toDate().toLocaleString()). Because my timezone is Europe/Prague, I had to explicitly tell toLocaleString to use a different one.

I think it may be better to not convert DateTime values to Date at all and let each connector to decide how to represent a date value.

If we avoid parsing the string into a Date object (at least internal to this structure) we then get saddled with the responsibility of validating the date string ourselves, don't we? I mostly added the .toDate function so that if anyone wants to get at the underlying date object, they can do so. In hindsight, it's probably an unnecessary gesture, since they can just say {instance}.date to reference it.

bajtos · 2017-04-27T12:47:37Z

@kjdelisle While I think it makes sense to have a special type representing values containing a date only, and then let each database connector to store them using a database-specific format, I personally don't think it's a fix for loopbackio/loopback-connector-mysql#149.

bajtos · 2017-04-27T12:50:08Z

BTW you should probably add at least toJSON() method to customize the way how a DateTime value is serialized to JSON payload (think of HTTP responses).

You may want to consider adding inspect() and toString() too. The former is used by util.inspect used by console.log.

kjdelisle · 2017-04-27T14:26:56Z

While I think it makes sense to have a special type representing values containing a date only, and then let each database connector to store them using a database-specific format, I personally don't think it's a fix for loopbackio/loopback-connector-mysql#149.

It's not meant to fix the issue entirely by itself; it allows bypassing the Date issue on the forward pass.
This PR also helps with handling the problem on the reverse pass.

kjdelisle · 2017-04-27T14:53:05Z

lib/date-string.js

+
+DateString.prototype.toJSON = function() {
+  return this.value;
+};


@bajtos Not sure about the best way to handle this currently. If I don't override this function with the implementation you see here, then the values that get passed along the chain end up feeding a stringified JSON object to the datasource layer (which explodes fantastically). This override does the trick, but it means no one can actually get the JSON representation of this object with this method (unless they util.inspect it).

I don't understand. Can you show some examples of what do you mean?

Here is my expectation:

expect(JSON.stringify({ when: new DateString('2017-04-28') })) .to.equal('{"when": "2017-04-28T00:00:00Z"}');

Please add some unit-tests to cover .toJSON and .toString methods.

bbito · 2017-04-27T15:20:05Z

Is the anticipated use case for DateString any type of Date - i.e. for MySQL DATE, DATETIME & TIMESTAMP or just DATE types?

kjdelisle · 2017-04-27T15:27:32Z

@bbito I'm hoping that this will be viable for any sort of datetime format that you don't want modified by LoopBack/JavaScript. It will still carry the backing Date object underneath if you really want the Date object that corresponds with it, but the default behaviour will just be validating that the string was any sort of valid date(time).

bbito · 2017-04-27T15:44:54Z

@kjdelisle

viable for any sort of datetime format

That's tough because the timeless DATE is so different from the time-bearing types. What I get in my test app after pulling this branch is a Midnight UTC representation of the MySQL DATE which I think is the only predictable js Date version of a timeless DATE, that really facilitates server-side comparisons of other DATE types, but it doesn't facilitate comparisons betwixt DATE and time-bearing Dates. I'm concerned with how that documentation story is told...

BTW, I pulled last night and I'm able to GET /DateTimeTables/{id}, but a GET /DateTimeTables/ crashes the app and a POST hands the whole DateString object to mysqljs/mysql and lands "0000-00-00" in MySQL, here's the SQL value out of mysqljs/mysql:
'{\\"date\\":\\"2017-04-18T00:00:00.000Z\\",\\"value\\":\\"2017-04-18\\"}\',

I know WIP, but I thought it may be useful info for you...

kjdelisle · 2017-04-27T15:58:44Z

That's tough because the timeless DATE is so different from the time-bearing types. What I get in my test app after pulling this branch is a Midnight UTC representation of the MySQL DATE which I think is the only predictable js Date version of a timeless DATE, that really facilitates server-side comparisons of other DATE types, but it doesn't facilitate comparisons betwixt DATE and time-bearing Dates. I'm concerned with how that documentation story is told...

With the current iteration that I've pushed onto the PR branch, you could still compare the internal representations of the DateString objects if you really need to see a UTC-based comparison.

var d1 = new DateString('2016-01-01');
var d2 = new DateString('2016-01-01T16:00:00.000Z');
d1.toString();
// '2016-01-01'
d2.toString();
// '2016-01-01T16:00:00.000Z'
d1._date.toString();
// 'Thu Dec 31 2015 19:00:00 GMT-0500 (EST)'
d2._date.toString();
// 'Fri Jan 01 2016 11:00:00 GMT-0500 (EST)'

In this case, d1._date is still the result of running new Date('2016-01-01') along with all of the behaviour you would expect from a JS Date. But if you're only concerned with getting your date (or datetime) to the database through LoopBack, and the only validation you want is "Is this string a date?" then this should handle it for you.

kjdelisle · 2017-04-27T16:03:31Z

Currently struggling with this warning on app start, though:
Swagger: skipping unknown type "DateString".
~~Need to find out where we handle that.~~
Need to handle this in loopback-swagger. Deeper down the rabbit hole we go!

bbito · 2017-04-27T16:26:48Z

POST is working (sending the String only) after 5650d82, but GET without id filter still crashes

bbito · 2017-04-27T17:09:57Z

@kjdelisle it is Zero dates in MySQL that are crashing GET they are currently transformed to null in connector at: https://github.com/bbito/loopback-connector-mysql/blob/06d610a3cfc8f8d9f6046b2ddd9ed8b33e971d30/lib/mysql.js#L412
Checking that clued me in that when I rebuilt my hosed rebase for loopbackio/loopback-connector-mysql#271 I skipped a commit that would have made that read:

        if (!val || val == '0000-00-00 00:00:00' || val == '0000-00-00') {
          val = null;
        }

I can push that, or if that Zero date handling will move to juggler or another module, I'll leave it alone.

kjdelisle · 2017-04-27T17:58:52Z

@bbito I think that will only apply to objects of type "Date"; this change will make them "DateString" instead.

bbito · 2017-04-27T18:06:33Z

@kjdelisle Got it - this fixes my Zero date GET crashes, should I push it?

      case 'Date':
      case 'DateString':

        // 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') {
          val = null;
        }

kjdelisle · 2017-04-27T18:18:35Z

@bbito Go for it

bbito · 2017-04-27T18:34:52Z

Pushed loopbackio/loopback-connector-mysql@883e61f

jannyHou · 2017-04-27T20:45:15Z

test/date-string.test.js

+      var date = new DateString(theDate);
+      date.value.should.eql(theDate);
+      var d = new Date(theDate);
+      date._date.toString().should.eql(d.toString());


just curious: is there any difference if we compare with getTime() instead of toString() here?

getTime() gives you the time in milliseconds; toString() is the full datetime stamp that a Date object will generate.

jannyHou

LGTM

bajtos · 2017-04-28T07:53:50Z

was calling it DateString to reflect that it's like a JS Date (which is actually a Date and Time), but in string form.

In that case let's call it DateTimeString please. I am pretty sure I won't be the only person confused by the name DateString, thinking it contains only the date part.

kjdelisle · 2017-04-28T14:50:22Z

@bajtos

In that case let's call it DateTimeString please. I am pretty sure I won't be the only person confused by the name DateString, thinking it contains only the date part.

I'm not sure why we would call it DateTime string if it's just the String representation of a JS Date object (literally, Date-String). Originally, I was going to call it DateAsString, but that seemed wordy and clumsy.

bbito · 2017-04-28T16:08:35Z

I like DateAsString!
I feel primary use case is for providing reassurance that timeless DATE types will not go through undocumented transforms into time-bearing types that can force it across a day boundary. I feel DateAsString conveys that!

New type that preserves string input as a string, but ensures that the string is a valid Date. Additionally, provides a .toDate function to provide the Date object representation of the string.

kjdelisle · 2017-05-01T21:22:24Z

@slnode test please

crandmck · 2017-05-02T20:00:06Z

@kjdelisle To add JSdocs (apidocs.strongloop.com), you need to add lib/date-string.js to docs.json.

Then just be aware that the API doc will not be published until this module is published to npm.

kjdelisle added #review needs-priority and removed #review labels Apr 26, 2017

kjdelisle requested review from bajtos and raymondfeng April 26, 2017 00:20

kjdelisle self-assigned this Apr 26, 2017

kjdelisle added apex in-progress and removed needs-priority labels Apr 26, 2017

kjdelisle mentioned this pull request Apr 26, 2017

Date fields - timezone problem loopbackio/loopback-connector-mysql#149

Closed

ssh24 reviewed Apr 26, 2017

View reviewed changes

kjdelisle force-pushed the datetype-allow-strings branch from 3f91003 to eba167a Compare April 26, 2017 21:41

kjdelisle changed the title ~~Allow Date type to use Strings~~ Add DateString type Apr 26, 2017

bajtos reviewed Apr 27, 2017

View reviewed changes

kjdelisle force-pushed the datetype-allow-strings branch from eba167a to 709837b Compare April 27, 2017 14:43

kjdelisle commented Apr 27, 2017

View reviewed changes

kjdelisle force-pushed the datetype-allow-strings branch from 709837b to 5650d82 Compare April 27, 2017 15:46

kjdelisle mentioned this pull request Apr 27, 2017

type-registry: add DateString type strongloop/loopback-swagger#85

Merged

2 tasks

jannyHou reviewed Apr 27, 2017

View reviewed changes

jannyHou approved these changes Apr 27, 2017

View reviewed changes

kjdelisle force-pushed the datetype-allow-strings branch 4 times, most recently from 84a432c to 6938ce3 Compare May 1, 2017 16:02

kjdelisle mentioned this pull request May 1, 2017

types: add DateString type loopbackio/loopback.io#373

Merged

Add DateString type

5e80837

New type that preserves string input as a string, but ensures that the string is a valid Date. Additionally, provides a .toDate function to provide the Date object representation of the string.

kjdelisle force-pushed the datetype-allow-strings branch from 6938ce3 to 5e80837 Compare May 1, 2017 16:29

kjdelisle added this to the Sprint 35 - Apex milestone May 1, 2017

kjdelisle merged commit 156b638 into master May 2, 2017

kjdelisle deleted the datetype-allow-strings branch May 2, 2017 18:48

kjdelisle removed the in-progress label May 2, 2017

charlie-s mentioned this pull request Oct 8, 2018

DateString returning unexpected format via API #1636

Closed

This was referenced Nov 2, 2018

Date format returned via REST API does not match model definition loopbackio/loopback-next#1948

Closed

Add support for DATE type (values with a date part but no time part) loopbackio/loopback-next#1966

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DateString type #1356

Add DateString type #1356

kjdelisle commented Apr 26, 2017 •

edited

Loading

kjdelisle commented Apr 26, 2017 •

edited

Loading

ssh24 Apr 26, 2017

kjdelisle commented Apr 26, 2017

bajtos Apr 27, 2017

kjdelisle Apr 27, 2017

bajtos Apr 27, 2017

kjdelisle Apr 27, 2017

bajtos commented Apr 27, 2017

bajtos commented Apr 27, 2017

kjdelisle commented Apr 27, 2017

kjdelisle Apr 27, 2017 •

edited

Loading

bajtos Apr 28, 2017

bajtos Apr 28, 2017

bbito commented Apr 27, 2017

kjdelisle commented Apr 27, 2017

bbito commented Apr 27, 2017

kjdelisle commented Apr 27, 2017

kjdelisle commented Apr 27, 2017 •

edited

Loading

bbito commented Apr 27, 2017 •

edited

Loading

bbito commented Apr 27, 2017 •

edited

Loading

kjdelisle commented Apr 27, 2017

bbito commented Apr 27, 2017

kjdelisle commented Apr 27, 2017

bbito commented Apr 27, 2017

jannyHou Apr 27, 2017

kjdelisle Apr 27, 2017

jannyHou left a comment

bajtos commented Apr 28, 2017

kjdelisle commented Apr 28, 2017

bbito commented Apr 28, 2017

kjdelisle commented May 1, 2017

crandmck commented May 2, 2017

Add DateString type #1356

Add DateString type #1356

Conversation

kjdelisle commented Apr 26, 2017 • edited Loading

Description

Linked Issues

Checklist

kjdelisle commented Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

kjdelisle commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Apr 27, 2017

bajtos commented Apr 27, 2017

kjdelisle commented Apr 27, 2017

kjdelisle Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbito commented Apr 27, 2017

kjdelisle commented Apr 27, 2017

bbito commented Apr 27, 2017

kjdelisle commented Apr 27, 2017

kjdelisle commented Apr 27, 2017 • edited Loading

bbito commented Apr 27, 2017 • edited Loading

bbito commented Apr 27, 2017 • edited Loading

kjdelisle commented Apr 27, 2017

bbito commented Apr 27, 2017

kjdelisle commented Apr 27, 2017

bbito commented Apr 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

bajtos commented Apr 28, 2017

kjdelisle commented Apr 28, 2017

bbito commented Apr 28, 2017

kjdelisle commented May 1, 2017

crandmck commented May 2, 2017

kjdelisle commented Apr 26, 2017 •

edited

Loading

kjdelisle commented Apr 26, 2017 •

edited

Loading

kjdelisle Apr 27, 2017 •

edited

Loading

kjdelisle commented Apr 27, 2017 •

edited

Loading

bbito commented Apr 27, 2017 •

edited

Loading

bbito commented Apr 27, 2017 •

edited

Loading