Use source parameter types

[gazoakley]

This PR is a follow on from #353 - it sets the type for bind parameters based on the source parameter type.

• Numbers are typed as DOUBLE
• JSON is typed as JSON
• Booleans are typed as TINY
• Dates are converted to string

There's a PR for sequelize that relies on these changes: sequelize/sequelize#8861

[gazoakley]

Node 8 build on Travis failed (but passed on AppVeyor). Looks like a random failure with npm. Closing (will reopen to retrigger Travis build).

[gazoakley]

Reopening to retrigger Travis build.

[sidorares]

thanks @gazoakley ! I'll try to fix build issue

btw date has it's own serialization format, not sure though if there is any benefit over sending them as a string ( you can see how it's serialised in packet.readDateTime -

node-mysql2/lib/packets/packet.js

Line 230 in 513451f

Packet.prototype.readDateTime = function() {

Haven't started reviewing carefully yet, but you set type for Date to be string, right?

[gazoakley]

Hi @sidorares - thanks for a quick response.

I didn't spot the date serialization that you suggested although to me I think it'd be preferable for Date objects to round trip back to Date if possible (I don't know if that would be a breaking change?). I'll give it a try.

[sidorares]

it'd be preferable for Date objects to round trip back to Date if possible

not sure I understand. This is client -> mysql communication, not casting results mysql -> js . Prior to your pr all parameters were sent as strings ( and marked as having string type ). On mysql side server would do casting like "prepared statement needs number here, we received "1111" so it's probably a number 1111).

[gazoakley]

Sequelize has its own tests that run against drivers, and it expects numbers to round trip with the same type. For example:

SELECT ? AS foo (where foo is a number parameter)

Without this PR the tests will fail, since Sequelize sends a number as the input parameter and gets back a string in the result. Granted it's a bit unusual to do this, but the test passes for other drivers (it doesn't currently support bind params for MySQL - sequelize/sequelize#8861 hopes to resolve that)

[sidorares]

oh ok I see now.

[sidorares]

what do you think is ideal behaviour when parameter is undefined? Return error immediately, convert to string "undefined", treat as NULL ?

[gazoakley]

Hi @sidorares

In response to your previous post, I'd argue for throwing an error since it doesn't map to a MySQL data type - if you want to provide a parameter as MySQL NULL then use JS null. One thing I struggled with is catching the error properly since I think it's being thrown inside a callback - I had to use process.on('uncaughtException', ...) to write a test to check it works.

I've implemented sending Date objects as DATETIME - other tests are still passing.

In response to your follow up, would you like to see an option like timezone? I'm guessing in these changes it would convert to UTC/specified offset (but elsewhere in the code it would need to convert from that back to local time). Should it go in a separate PR?

[gazoakley]

It looks like the previous behaviour didn't do anything with the timezone either? https://github.com/sidorares/node-mysql2/pull/705/files#diff-3b8f681f01aefd0299dedcf941cd7a3dL46

[sidorares]

In response to your previous post, I'd argue for throwing an error since it doesn't map to a MySQL data type - if you want to provide a parameter as MySQL NULL then use JS null. One thing I struggled with is catching the error properly since I think it's being thrown inside a callback - I had to use process.on('uncaughtException', ...) to write a test to check it works.

yes, this is how I think about this as well - most undefineds here in my experience are typos or similar silly errors, and it's better to throw early ( preferably before loosing sync stack ). This is however different from .query() casting where undefined is treated as NULL. I'd vote for "throw early"

would you like to see an option like timezone? I'm guessing in these changes it would convert to UTC/specified offset (but elsewhere in the code it would need to convert from that back to local time). Should it go in a separate PR?

yes, definitely separate pr, just wanted to flag that because it's often cause of a problem for anything related with mysql DATETIME type

[sidorares]

sorry, I'm extremely busy this week, I'll try to find time to help to land this PR

[sidorares]

It looks like the previous behaviour didn't do anything with the timezone either?

yes, that's what I'm saying by " does not behave well in communicating user intentions about timezone" - #642 #15 #262 #633

[gazoakley]

sorry, I'm extremely busy this week, I'll try to find time to help to land this PR

No worries - thanks for your help so far! 👍

[sidorares]

Hey @gazoakley sorry for silence.
I'll put few notes here as low priority and maybe tackle on them later, but want to merge this as is anyway, don't want this PR to sit any longer

• move toMysqlDateBuffer to packet.js - this is where all low level serialising/deserializing lives. Commands/packets are for higher level protocol logic
• JSON.stringify can throw if there are circular references. Maybe we need to handle this (maybe not)
• add handling of undefined (maybe earlier when command is added to make stack trace better)

[vlasky]

add handling of undefined (maybe earlier when command is added to make stack trace better)

Yes please do this post haste. This has been an outstanding issue for a long time (see #516 and #480).

[gazoakley]

Hi @sidorares - thanks for getting this merged. I've tackled some of the issues identified in #718