Provide new dateStrings option: "DATE" #1481

bbito · 2016-07-21T01:24:30Z

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: #605 , loopbackio/loopback-connector-mysql#120 , loopbackio/loopback-connector-mysql#149

bbito · 2016-07-21T02:01:52Z

My apologies, I didn't mean to leave that big comment block in there! I'll clean that up in 12 hours.

bbito · 2016-07-21T15:43:01Z

This hopes to be a low-impact method to satisfy users wishing to handle only DATE fields as strings while retaining the default behavior for TIMESTAMP and DATETIME fields and providing backwards compatibility with existing code. Another and perhaps more efficient approach would be to add yet another property to connection.config with boolean options, e.g. connection.config.dateOnlyStrings.

dougwilson

Please add tests for the added functionality :) !

dougwilson · 2016-09-26T17:35:35Z

lib/protocol/packets/RowDataPacket.js

@@ -61,7 +61,7 @@ function typeCast(field, parser, timeZone, supportBigNumbers, bigNumberStrings,
    case Types.DATETIME2:
    case Types.NEWDATE:
      var dateString = parser.parseLengthCodedString();
-      if (dateStrings) {
+      if (dateStrings === true || field.type === Types.DATE && dateStrings.toString().toLowerCase() === "date") {


I'm not sure if this is backwards-compatible, because code today with dateStrings: 1 will no longer work after this change, since we allowed any truthy value before to mean date strings.

Thanks for the feedback, I'll look at supporting any truthy value - except dateStrings.toString().toLowerCase() === "date" - for backward compatible behavior and also look to adding the requested tests.
I may be back to ask for help on setting up the test.

Sounds good! You may want to rebase your changes to the current master as well, as I just saw the GitHub is saying the PR is not mergable due to conflicts.

@dougwilson , I was hoping to find an existing test for the current dateStrings connection option to either add to or use as a base, but I'm not finding it. Does such a test exist?

I'm not sure.

RE: Requested tests, @dougwilson could you take a quick look at the more exhaustive tests on my branch: https://github.com/bbito/node-mysql/blob/test-datestrings-options/test/integration/connection/test-datestrings-options.js and let me know if I should merge that into this PR?

Hi @bbito sorry I haven't had the time to revisit this PR just yet. As for additional tests, I'm all for it :) If you are making tests just as general additions to test the functionality that already exists, that is awesome! It would be nice to have it as a separate PR, ideally, but you can add it to this PR if you like.

As it stands the new test file will fail without the rest of the PR, so I'll merge it for now and if the PR is rejected and you want the test for the current code just let me know and I'll strip out the dateStrings: 'date' stuff and submit it as a separate PR.

bbito · 2016-09-29T17:33:38Z

I feel like I've got a good PR now and an additional improvement for ths, more backwards compatible code is that as in the pre-feature code, there is a simple evaluation for truthiness of dateStrings and with this PR, now the additional checks only affect projects with a truthy setting for dateStrings. My earlier attempt would impact performance of both truthy and non-truthy or unset dateStrings configurations.

So thanks for prodding for that change!
Now for testing, @dougwilson - is there an example you might point me to that performs a similar test for different connection options?

dougwilson · 2016-09-29T23:24:30Z

I can't tell what is in the PR anymore, as it seems like the GitHub changes tab is not working and showing a while lot of other changes: https://github.com/mysqljs/mysql/pull/1481/files

bbito · 2016-09-29T23:33:49Z

Wow, I wouldn't doubt that I messed up the rebase (my first one!), but I think I would have noticed it was now showing 33 files changed instead of 2! I'll see if I can figure out what went haywire!

dougwilson · 2016-09-29T23:38:45Z

is there an example you might point me to that performs a similar test for different connection options?

All you need to do is create the connection with option you are trying to test in the test. THe current main test that is supposed to be testing all the type casting from the database to JavaScript is https://github.com/mysqljs/mysql/blob/c19c867d032a89263dc8717a84596275b3e4dfa5/test/integration/connection/test-type-casting.js

bbito · 2016-09-29T23:41:59Z

Thanks @dougwilson , I'll look a that existing test.
I'm a bit stumped about the status of the PR... If I look at my repo it looks as I'd expect with 13 commits and 2 files changed: master...bbito:master

dougwilson · 2016-09-30T00:05:07Z

Weird. I have a comment I want to make, but not sure if it'll get lost or not. Can you do like a squash rebase on top of the current master, perhaps? The following commands may get you out of the hole:

$ git fetch origin
$ git reset --hard origin/master
$ git checkout HEAD@{1} -- .
$ git commit -a
# validate the current commit contains all your changes
$ git push -f

bbito · 2016-09-30T00:10:11Z

Thanks for the git help - I'll try that now

dougwilson · 2016-09-30T00:12:20Z

Sorry, I forgot you're a fork :) The first two should be the name of your remote instead of origin. By default, this is usually upstream:

$ git fetch upstream
$ git reset --hard upstream/master

bbito · 2016-09-30T00:27:17Z

Okay, commits squashed down to 1 and back to 2 files changed as hoped - thanks.
Coding is in addition to full-time+ job, so it may take a day or two to get the test going.

Add 'date' as dateStrings value option. The connection option dateStrings: 'date' will only affect DATE fields whereas any truthy value (except 'date') retains original behavior, effecting DATE, DATETIME and TIMESTAMP fields. See: mysqljs#605 , loopbackio/loopback-connector-mysql#120 , loopbackio/loopback-connector-mysql#149

This test confirms that DATETIME and TIMESTAMP columns are still exhibiting the default behavior, accepting and returning javascript date objects while DATE columns will accept and return a string value.

bbito · 2016-10-02T14:54:49Z

Test added, please let me know if there are any additional tasks to sew up this PR.

bbito · 2016-10-03T14:05:01Z

Hi @dougwilson , I have a new test that I haven't pushed yet (test-datestrings-options.js), that loops through various truthy, falsy and 'date'-ish values for dateStrings:. Should I delete test-datestrings-option-date.js and add test-datestrings-options.js?

This test loops through various truthy, falsy and 'date'-ish values for connection option dateStrings: and runs corresponding typecasting checks.

bbito · 2016-10-03T15:37:28Z

@dougwilson I stuck the test-datestrings-options.js test onto a branch: https://github.com/bbito/node-mysql/blob/test-datestrings-options/test/integration/connection/test-datestrings-options.js
Just let me know if I should merge into master.

Add tests where JavaScript Date objects are supplied when dateStrings: truthy or 'date' and check that strings are returned as well as supplying strings when dateStrings: falsy and check that JavaScript Date objects are returned.

bbito · 2016-10-04T13:43:16Z

Latest commit in test-datestrings-options branch includes 'mis-matched' tests - that is supplying JavaScript Date objects to the module when dateStrings: is truthy or 'date' and conversely supplying strings when dateStrings: is falsy. Although I don't think it is clearly documented, it seems that it is by design that this behavior is supported.

dougwilson · 2016-10-07T03:01:49Z

I swear I though I replied here at some point in the last few days, but I guess I'm just making stuff up in my mind :) Anyway, I need to take a look. Seems every time I turn around, more stuff is added to this PR, so please bear with me in trying to look it over :)

dougwilson · 2016-10-07T04:23:47Z

lib/protocol/packets/RowDataPacket.js

@@ -62,7 +62,11 @@ function typeCast(field, parser, timeZone, supportBigNumbers, bigNumberStrings,
    case Types.NEWDATE:
      var dateString = parser.parseLengthCodedString();
      if (dateStrings) {
-        return dateString;
+          if (field.type === Types.DATE) {


Should this also include Types.NEWDATE? I believe that is a date string with no time component as well.

I'm happy to include it. When I first started poking into this module I found NEWDATE in the constants: https://github.com/mysqljs/mysql/blob/master/lib/protocol/constants/types.js , but I couldn't find it in mysql docs. Today I find 2 references to it in current docs: http://dev.mysql.com/doc/connector-net/en/connector-net-ref-mysqlclient-mysqlcommandmembers.html

Newdate: Obsolete. Use Datetime or Date type.

http://dev.mysql.com/doc/connectors/en/connector-net-ref-mysqlclient-mysqlcommand.html

Newdate: Obsolete. Use Datetime or Date type.

So unless there is a decision for this module to remove support for this obsolete Member/Type and it is removed from the switch block in this file, I think I should amend this PR to support it.

Types.NEWDATE support added with Oct. 7 2016 commit: 975b2ae

bbito · 2016-10-18T10:02:55Z

@dougwilson Is there anything more you want for this PR?

cfilby · 2016-10-27T22:09:32Z

Is there anything I could do to assist with this? Seems like the PR is just about done, but I'm definitely interested in this functionality and would be happy to help out.

dougwilson · 2016-10-27T22:33:32Z

This is on my backlog, but is low priority since it doesn't add any functionality that is not already in this module; it simply adds some sugar to what you can already do using a custom typecast.

bbito · 2016-10-27T22:53:38Z

@dougwilson in my scenario I don't think I have the ability to pass a custom typecast because I'm not using this module directly, but rather through Strongloop/Loopback: https://github.com/strongloop/loopback.
In this scenario I can pass connection options through the \server\datasources.json configuration file, but I haven't found another way to customize a connection in Loopback.

dougwilson · 2016-10-27T23:06:00Z

Hi @bbito, you may also want to make a PR to that wrapper in parallel to this PR, because the typecast is a very essential feature. I add things to this module from the perspective of using this module; if you are using a wrapper and it's forcing you to alter the module it's wrapping just to use it, you may want to use a different wrapper.

dougwilson · 2016-11-02T05:52:03Z

Hi @bbito thanks for all your hard work. I am working on merging this now, for inclusion in the next release. The API would be an array similar to debug rather than just a string, thus dateStrings: 'DATE' would become dateStrings: ['DATE'], which allows people to fine tune whatever types they want casted or not for whatever reason.

This was referenced Jul 28, 2016

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

Closed

DATE type casting #605

Closed

dougwilson self-assigned this Sep 26, 2016

dougwilson added the feature label Sep 26, 2016

dougwilson requested changes Sep 26, 2016

View reviewed changes

bbito force-pushed the master branch from e957f68 to 2570ada Compare September 30, 2016 00:19

bbito force-pushed the master branch from 2570ada to f7f278b Compare September 30, 2016 00:32

Add test for new dateStrings connection option: 'date'

436486a

This test confirms that DATETIME and TIMESTAMP columns are still exhibiting the default behavior, accepting and returning javascript date objects while DATE columns will accept and return a string value.

Add tests for various values for dateStrings

7c2fcc9

This test loops through various truthy, falsy and 'date'-ish values for connection option dateStrings: and runs corresponding typecasting checks.

Add tests mis-matching strings vs JS Date objects

ac51988

Add tests where JavaScript Date objects are supplied when dateStrings: truthy or 'date' and check that strings are returned as well as supplying strings when dateStrings: falsy and check that JavaScript Date objects are returned.

dougwilson reviewed Oct 7, 2016

View reviewed changes

bbito added 2 commits October 7, 2016 15:03

Add check for Types.NEWDATE

975b2ae

Fix code indent to 2 spaces

4b3c824

Merge remote-tracking branch 'upstream/master'

3490fcc

dougwilson approved these changes Nov 2, 2016

View reviewed changes

dougwilson closed this in 5cc6c68 Nov 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide new dateStrings option: "DATE" #1481

Provide new dateStrings option: "DATE" #1481

bbito commented Jul 21, 2016

bbito commented Jul 21, 2016

bbito commented Jul 21, 2016

dougwilson left a comment

dougwilson Sep 26, 2016

bbito Sep 29, 2016

dougwilson Sep 29, 2016

bbito Sep 29, 2016

dougwilson Sep 29, 2016

bbito Oct 4, 2016

dougwilson Oct 4, 2016

bbito Oct 4, 2016

bbito commented Sep 29, 2016 •

edited

Loading

dougwilson commented Sep 29, 2016

bbito commented Sep 29, 2016

dougwilson commented Sep 29, 2016

bbito commented Sep 29, 2016

dougwilson commented Sep 30, 2016

bbito commented Sep 30, 2016

dougwilson commented Sep 30, 2016

bbito commented Sep 30, 2016

bbito commented Oct 2, 2016

bbito commented Oct 3, 2016

bbito commented Oct 3, 2016

bbito commented Oct 4, 2016 •

edited

Loading

dougwilson commented Oct 7, 2016

dougwilson Oct 7, 2016

bbito Oct 7, 2016

bbito Oct 24, 2016

bbito commented Oct 18, 2016

cfilby commented Oct 27, 2016

dougwilson commented Oct 27, 2016

bbito commented Oct 27, 2016

dougwilson commented Oct 27, 2016

dougwilson commented Nov 2, 2016

Provide new dateStrings option: "DATE" #1481

Provide new dateStrings option: "DATE" #1481

Conversation

bbito commented Jul 21, 2016

bbito commented Jul 21, 2016

bbito commented Jul 21, 2016

dougwilson left a comment

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

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

Choose a reason for hiding this comment

bbito commented Sep 29, 2016 • edited Loading

dougwilson commented Sep 29, 2016

bbito commented Sep 29, 2016

dougwilson commented Sep 29, 2016

bbito commented Sep 29, 2016

dougwilson commented Sep 30, 2016

bbito commented Sep 30, 2016

dougwilson commented Sep 30, 2016

bbito commented Sep 30, 2016

bbito commented Oct 2, 2016

bbito commented Oct 3, 2016

bbito commented Oct 3, 2016

bbito commented Oct 4, 2016 • edited Loading

dougwilson commented Oct 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbito commented Oct 18, 2016

cfilby commented Oct 27, 2016

dougwilson commented Oct 27, 2016

bbito commented Oct 27, 2016

dougwilson commented Oct 27, 2016

dougwilson commented Nov 2, 2016

bbito commented Sep 29, 2016 •

edited

Loading

bbito commented Oct 4, 2016 •

edited

Loading