Map JavaScript undefined to MySQL DEFAULT

[ghost]

SqlString.escape maps both JavaScript null and undefined to MySQL NULL. This change proposes JavaScript undefined be mapped to MySQL DEFAULT, as per feature request mysqljs/mysql#559 and as discussed in mysqljs/mysql#1568.

[ghost]

Looks like the automated tests are failing because they are testing for SqlString.escape to return NULL, not DEFAULT, when undefined is passed. Of course, returning DEFAULT is what this pull request is all about. Should I try to update the tests in my pull requests, or is this something best left for the project owner to handle?

[dougwilson]

Feel free to update the tests :) Since this is a breaking change that is not backwards-compatible, I cannot really guarantee any kind of timeline this would ever get merged, though. It would be around the time mysql@3 is being prepared most likely.

There may or may not be edge cases with this kind of change, so would also want to keep this PR open for a long time in order to get lots of eye balls on the change.

[fengmk2]

This will confuse for many exists use cases.
Lost's of people always thinks escape(null) equals to escape(undefined).

If we change this behavior, I don't think those exists projects can easy to upgrade mysql@3.

[ghost]

Ok, that's certainly understandable! :) It may be worth noting that MySQL automatically sets the DEFAULT to NULL on columns that can take a NULL value and the column definition does not include an explicit DEFAULT value, as per http://dev.mysql.com/doc/refman/5.7/en/data-type-defaults.html. So those cases would behave the same as if NULL was explicitly passed and shouldn't be affected by such a change as proposed in this PR. But, it's not clear to me after a first read through that doc what would happen if DEFAULT was passed to a column defined NOT NULL and with no DEFAULT value. I'll look into it when I have some more time. Thanks.

[dougwilson]

Hi @garudacrafts we don't document how MySQL works, as that is best left up to MySQL's own docs. Rather, we document how our module works. Think about if every single MySQL client module how to re-document how MySQL worked, that would be such a burden no one would make MySQL clients :)

[ghost]

@dougwilson yes, of course, I wasn't literally recommending that you include notation of MySQL docs when I wrote "it may be worth noting"! I was simply pointing out that explicitly passing DEFAULT behaves the same as passing NULL in those cases (and the hyperlink was included for reference).

[ghost]

Ok, I updated the test script, but it is failing on 4 out of 11 automated tests with the eslint error Trailing spaces not allowed no-trailing-spaces on lines 37 and 41. I didn't add any whitespace, only changed NULL to DEFAULT in the checks. Not sure how why it's failing on some tests but others, nor how to fix it, sorry.

[dougwilson]

@garudacrafts just run eslint --fix . and then commit those changes. Your commits show you added trailing spaces on those two lines in 2282c46

[ghost]

@dougwilson - yup, I just realized it wasn't the test file, but the changes I proposed in SqlString.js where that pesky whitespace crept up. Looks like they're passing now. It's getting late and my eyes are failing me... g'night, cheers.

[leopoldhub]

Hi there, I see that this pr wasn't updated for a while
I was curious to know if this was still work in progress and what the actual state of it was
(as well as if there is anything I can do to help with the remaining work)