Remove String manipulations of Date objects #271

bbito · 2017-04-25T22:18:37Z

Description

This is replacing PR #268 after rebase went badly.

Pull Request requested by @kjdelisle regarding #149 following my proposed solution "A" at #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 #265 from @darknos .

Previously timezone values specified in datasources.json had no effect on the underlying mysqljs/mysql module because Loopback was converting the Dates to Strings before handing off to the driver module.
With this change, timezone values specified in datasources.json provide the expected behavior only if "legacyUtcDateProcessing" is explicitly set to false in datasources.json, e.g.
"legacyUtcDateProcessing": false

This PR was made under conditions spelled out in the related issue thread #149 that I have limited time and expertise as a contributor and do not plan to personally address the test creation and documentation outlined in the action plan from @bajtos as pertains to this correction of long-standing undocumented forced coercion of Date types to UTC.

Please label: breaking-change, bug as per #149

Related issues

Date fields - timezone problem #149

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

slnode · 2017-04-25T22:18:39Z

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

slnode · 2017-04-25T22:18:39Z

Can one of the admins verify this patch?

slnode · 2017-04-25T22:18:39Z

Can one of the admins verify this patch?

slnode · 2017-04-25T22:18:39Z

Can one of the admins verify this patch?

kjdelisle · 2017-04-25T22:42:29Z

@slnode test please

kjdelisle · 2017-04-27T19:25:50Z

@slnode test please

ssh24 · 2017-04-27T19:52:41Z

@bbito Could we add some test cases?

kjdelisle · 2017-04-27T19:53:20Z

@ssh24 As previously discussed, we'll be making the tests for this instead.

kjdelisle · 2017-04-27T19:54:32Z

@bbito Would I be able to begin working on this PR? I want to make sure everything's sorted out before I begin modifying this branch (adding tests)

bbito · 2017-04-27T20:03:15Z

@kjdelisle Hell's yeah! Let me know if you need anything from my end, I'm mostly on email, but sometimes doing client care for ~1 hour at a time and not checking.

darknos · 2017-04-27T20:07:51Z

@kjdelisle @bbito You can use test cases from #257 as starting point.

bbito · 2017-04-27T20:14:03Z

@darknos Thanks! I owe it to you that movement is finally happening on this bug that's been wasting my time since last year - Thank You!

@kjdelisle

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

To sync with loopback-datasource-juggler #1356 which introduces new Type: DateString

kjdelisle · 2017-05-02T19:34:11Z

@slnode test please

0candy · 2017-05-02T20:09:19Z

test/datetime.test.js

@@ -0,0 +1,93 @@
+// Copyright IBM Corp. 2013,2016. All Rights Reserved.
+// Node module: loopback-datasource-juggler


Wrong module name. And copyright year.

Oh, yuck. Fixing

jannyHou · 2017-05-02T20:18:32Z

test/datetime.test.js

+      createdAt: new Date(),
+    }).then(function(inst) {
+      inst.should.not.eql(null);
+      inst.dob.toString().should.eql(d.toString());


I might be wrong, not sure does create return the same data in cb as find returns. According to the next test, you want to test the data returned by find :)

ssh24

LGTM

jannyHou · 2017-05-02T20:21:46Z

test/datetime.test.js

+var fmt = require('util').format;
+var should = require('./init.js');
+
+var db, Order, Person;


Yeah Order doesn't seem like its being used.

jannyHou · 2017-05-02T20:25:07Z

test/datetime.test.js

+    name: String,
+    gender: String,
+    married: Boolean,
+    dob: DateString,


I think after DateString get registered to a model, user don't have to specify this type by requiring the file in datasource directly, you can define it by doc: {type: 'datestring'} then model builder knows its type right?

jannyHou

three minor comments, LGTM

kjdelisle · 2017-05-02T22:14:19Z

@slnode test please

kjdelisle · 2017-05-02T22:24:07Z

@bbito Merged. :)

bbito · 2017-05-02T22:41:15Z

@kjdelisle Hey Great News! Thanks for taking that on!

This was referenced Apr 25, 2017

Remove String manipulations of Date objects #268

Closed

Date fields - timezone problem #149

Closed

kjdelisle self-assigned this Apr 25, 2017

joostdebruijn mentioned this pull request Apr 26, 2017

Add support for fractional seconds. #270

Closed

2 tasks

kjdelisle mentioned this pull request Apr 27, 2017

Add DateString type loopbackio/loopback-datasource-juggler#1356

Merged

3 tasks

kjdelisle added the bug label Apr 27, 2017

kjdelisle force-pushed the bb-fresh-master branch from 883e61f to 120514d Compare April 28, 2017 20:13

Add new type DateString to fromColumnValue

cee6303

To sync with loopback-datasource-juggler #1356 which introduces new Type: DateString

kjdelisle force-pushed the bb-fresh-master branch from 120514d to dad0ec3 Compare May 1, 2017 23:39

0candy suggested changes May 2, 2017

View reviewed changes

kjdelisle force-pushed the bb-fresh-master branch from dad0ec3 to faafc7c Compare May 2, 2017 20:13

jannyHou reviewed May 2, 2017

View reviewed changes

ssh24 approved these changes May 2, 2017

View reviewed changes

jannyHou reviewed May 2, 2017

View reviewed changes

kjdelisle force-pushed the bb-fresh-master branch from faafc7c to 02646b3 Compare May 2, 2017 20:22

jannyHou reviewed May 2, 2017

View reviewed changes

jannyHou approved these changes May 2, 2017

View reviewed changes

kjdelisle force-pushed the bb-fresh-master branch from 02646b3 to 8779a4e Compare May 2, 2017 20:29

0candy approved these changes May 2, 2017

View reviewed changes

Tests for datetime types

31a7728

kjdelisle force-pushed the bb-fresh-master branch from 8779a4e to 31a7728 Compare May 2, 2017 21:46

kjdelisle merged commit 3fa5306 into loopbackio:master May 2, 2017

ssh24 mentioned this pull request May 5, 2017

legacyDateTypeProcessing flag for datetime processing #265

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove String manipulations of Date objects #271

Remove String manipulations of Date objects #271

bbito commented Apr 25, 2017 •

edited

Loading

slnode commented Apr 25, 2017

slnode commented Apr 25, 2017

slnode commented Apr 25, 2017

slnode commented Apr 25, 2017

kjdelisle commented Apr 25, 2017

kjdelisle commented Apr 27, 2017

ssh24 commented Apr 27, 2017

kjdelisle commented Apr 27, 2017

kjdelisle commented Apr 27, 2017 •

edited

Loading

bbito commented Apr 27, 2017

darknos commented Apr 27, 2017

bbito commented Apr 27, 2017

kjdelisle commented May 2, 2017

0candy May 2, 2017

kjdelisle May 2, 2017

jannyHou May 2, 2017

kjdelisle May 2, 2017 •

edited

Loading

ssh24 left a comment

jannyHou May 2, 2017

0candy May 2, 2017

jannyHou May 2, 2017

jannyHou left a comment

kjdelisle commented May 2, 2017

kjdelisle commented May 2, 2017

bbito commented May 2, 2017

		@@ -0,0 +1,93 @@
		// Copyright IBM Corp. 2013,2016. All Rights Reserved.
		// Node module: loopback-datasource-juggler

Remove String manipulations of Date objects #271

Remove String manipulations of Date objects #271

Conversation

bbito commented Apr 25, 2017 • edited Loading

Description

Related issues

Checklist

slnode commented Apr 25, 2017

slnode commented Apr 25, 2017

slnode commented Apr 25, 2017

slnode commented Apr 25, 2017

kjdelisle commented Apr 25, 2017

kjdelisle commented Apr 27, 2017

ssh24 commented Apr 27, 2017

kjdelisle commented Apr 27, 2017

kjdelisle commented Apr 27, 2017 • edited Loading

bbito commented Apr 27, 2017

darknos commented Apr 27, 2017

bbito commented Apr 27, 2017

kjdelisle commented May 2, 2017

0candy May 2, 2017

Choose a reason for hiding this comment

kjdelisle May 2, 2017

Choose a reason for hiding this comment

jannyHou May 2, 2017

Choose a reason for hiding this comment

kjdelisle May 2, 2017 • edited Loading

Choose a reason for hiding this comment

ssh24 left a comment

Choose a reason for hiding this comment

jannyHou May 2, 2017

Choose a reason for hiding this comment

0candy May 2, 2017

Choose a reason for hiding this comment

jannyHou May 2, 2017

Choose a reason for hiding this comment

jannyHou left a comment

Choose a reason for hiding this comment

kjdelisle commented May 2, 2017

kjdelisle commented May 2, 2017

bbito commented May 2, 2017

bbito commented Apr 25, 2017 •

edited

Loading

kjdelisle commented Apr 27, 2017 •

edited

Loading

kjdelisle May 2, 2017 •

edited

Loading