Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Treat undefined as nulls for execute - Fixes #715 #716

Closed
wants to merge 1 commit into from

Conversation

aikar
Copy link
Contributor

@aikar aikar commented Jan 26, 2018

See #715

This fixes a bug where the query does not ever finish, and hangs forever
@sidorares
Copy link
Owner

I'm trying to balance "correctness" "least surprise" and "if it's a programmer error, give feedback as early as possible".

See #705 (comment)

To me better treat undefined as "you probably have a typo or forget parameter?" error. ( Depends of course on how often there is legit use case to set value to NULL when you actually send undefined ) There is no mysql undefined type, just NULL

What's your personal opinion @aikar ? (not saying that current behaviour is good, we need to fix it. Just do not want to replace crash with hiding issue)

@aikar
Copy link
Contributor Author

aikar commented Jan 26, 2018

@sidorares I battled with that decision when working on it tonight. At first I was going to just add an error if undefined is found before I call this API, but figured if null works, should just treat it as null.

But, I can also get behind the idea that undefined is more likely than not an application code bug, and it could help developers discover bugs that are otherwise able to be more sneaky (especially for inserts!)

So, I can get behind making it an error instead, but if that other PR is nearing landing, we can just close this then, as it def looks good!

@aikar
Copy link
Contributor Author

aikar commented Jan 26, 2018

Personally, Id say we could pull this for now, then iterate when a decision is made on treating undefines in that other PR

@vlasky
Copy link

vlasky commented Jan 29, 2018

We need a solution that allows the programmer to identify the query (and arguments) responsible for passing the undefined parameters to execute, as this is almost certainly due to a bug.

See previous attempts #516 and #480.

@aikar
Copy link
Contributor Author

aikar commented Jan 29, 2018

@sidorares see https://github.com/sidorares/node-mysql2/pull/689/files - may be a good basis for this.

Going to close this PR since #705 is merged, and i think most people do favor alerting you to an error vs treating as null.

@aikar aikar closed this Jan 29, 2018
@aikar aikar deleted the fix-execute branch January 29, 2018 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants