Don't try to write undefined parameters. #516
Conversation
|
I understand what problem you trying to solve ( driver should not crash regardless of the input !!! ) but a bit concerned with your solution
with extra Is there any reason we want to send execute packet in that case? Maybe document "undefined variables are not allowed" in js<->mysql types documentation and make code error early if undefined parameter is passed? |
|
At the moment, we are using prepared statements invoked with connection.execute(). When the wrong number of arguments is passed to execute(), the exception message does not include the actual SQL statement or the line of code from where connection.execute() was called. This is making it virtually impossible to identify and debug the code that is causing the problem. This is an example of what we currently get without the fix: We (and others) urgently need a runtime mechanism that will allow us to identify the specific SQL statement (and lines of code) for which undefined parameters are being passed. As you correctly say, it would technically not be necessary to send the execute packet - it would be more efficient to have code to detect this beforehand and throw an exception. However, this is not a situation that would happen very often - passing undefined arguments would almost always be programmer error and therefore would be fixed very quickly once discovered. Therefore, I don't consider the efficiency difference to be a strong reason to not merge this pull request which implements an extremely simple, straightforward fix. I would prefer this pull request to be merged so that everyone in a situation like ours can immediately debug and fix their code and let the more efficient approach be implemented later. |
I'm just suggesting even more simple fix. For example, add here https://github.com/vlasky/node-mysql2/blob/d347710630b49df709942dccd252f9630a11439f/lib/commands/execute.js#L49 something like this: if (!this.parameters.every(p => typeof p !== 'undefined')) {
const err = new Error(`Passed undefined parameter to prepared statement, query is ${this.sql}`)
if (this.onResult) {
this.onResult(err);
} else {
this.emit('error', err);
}
return;
} |
|
Yes that looks promising. Can we make it so that the error object includes both the query and the array of parameters that was passed as separate properties? This information would help narrow down the cause. This might require creating a custom ExecuteError object that derives from Error. |
yes (probably using util.inspect? - so that large Buffer/arrays are trimmed automatically and not blow up logs)
The code I posted wasn't tested at all. Would you want to explore that approach yourself and create another PR? Also, need to add unit test ( If you need my help - I might be able to allocate couple of hours to this later this week ) Sorry for nitpicking, just think this is better than applying temp solution first and refactor it after |
|
What happened with this? We are still getting these errors which provide no context as to the query or parameters which caused the query to fail. The code provided by @sidorares looked promising, why was it not incorporated? Even better if parameters can be returned in the error also as suggested by @vlasky, but understand that may cause an issue with large objects. Just having the query would probably be sufficient for us to at least know where the problem is occuring and therefore be able to fix the offending code. |
|
@vlasky would you have time to go through my suggestions? If not that's ok, I might be able to find some time over weekend to do that myself |
|
@sidorares I see that the code has changed a bit since I last looked at it. To help us debug a recent issue, I changed the following line https://github.com/sidorares/node-mysql2/blob/master/lib/connection.js#L617 from
to
|
|
Another question @sidorares - is it possible in the code to get a count of the number of question mark placeholders contained within a prepared statement? I want to put in a safety check to generate an exception if the number of question marks placeholders in the prepared statement does not equal the number of elements in the parameter value array passed to execute(). |
yes, result of node-mysql2/lib/commands/prepare.js Line 119 in 0c422dd see how it's passed to execute here: Line 643 in 0c422dd |
…keepalive on the network connection. If not set, it defaults to false. Resolves issue sidorares#1229 - Pool option "enableKeepAlive" is ignored? sidorares#1229
When running execute(), any placeholder parameter value with the JavaScript type 'number' will now be sent to MySQL via the binary protocol as a MySQL string instead of as a MySQL double. Resolves issue sidorares#1239, where it was reported that prepared statements that are called with parameters having the JavaSCript type 'number' are failing with the error "Incorrect arguments to mysqld_stmt_execute". It was determined that this was due to a change made to prepared statement handling in MySQL 8.0.22 and that this could be overcome by converting numerical parameter values to strings. The long-term fix requires the implementation of data type conversion of parameters using the type hints provided by MySQL in the COM_STMT_PREPARE Response. For more information: sidorares#1239 https://dev.mysql.com/worklog/task/?id=9384
Updated version to 2.2.6
…KeepAlive Updated version to 2.2.7
…n inferring from input variables
|
hey @vlasky this PR was ( is ) against your master and it is diverged quite a lot from initial discussion. I'm not sure if original issue is still there ( passing |
This is a re-submission of @ChiperSoft's fix, as requested by @sushantdhiman: