-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add an option to make queries always return an array #1367
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
Comments
I realize I didn't explain myself very well. I'd say that #770 and #658 aren't related. They are about getting an Array instead of an Object of the values. It is about the number of rows being returned and whether they are being wrapped up in an array or not. If only 1 row is returned, you unwrap it, if multiple rows are returned you return them as an array. I'd like it to alway be wrapped in an array, regardless of the number of results. |
Hi @wraithan, perhaps there is still not a very clear explanation going on? Even when we return one data row from a SQL query, it is still an array of one data row object: db.query('SELECT * FROM information_schema.tables LIMIT 1', function (err, rows) {
console.log('type: ' + typeof rows);
console.log('isarray: ' + Array.isArray(rows));
}); Gives me:
Perhaps if you can give an example of the code that returns a non-array, then the change you did, and the code that is now returning an array? |
oh, I see. IFAIK the code you are linking returns single array for non-multiple statements result set and array of arrays when there are multiple statements - one array per result |
Yeah, sorry I also misread the internal bug report, couple hours past the end of the day :). But yes, we do a lot of multi query operations to minimize round trips. We'd like to always get back an array of row sets, that way the return value is always the same. We have at least 1 case of a multi query operation being built up then sent that will have >=1 query in it, making us have special code around handling the results. |
I'm +1 on this. It's really hard to see what's inside result unless you attach 'fields' event and count number of times it's called before 'end' event |
-1, I'd suggest a different api specifically for multiple statements. That api would then always return an array. The current api cannot be changed this dramatically, imho. |
@q42jaap I was suggesting it as an option, not as a change for everyone. Another API is also an way but would be confusing when this API also support multiple queries. |
+1, I would like to see some method of selecting the return object - either an object or an array. I currently have helper methods to run map() against the return and strip off the LHS wheh only an array is needed but is not as elegant as I would like. |
- at the moment the return type is different depending on how many queries were run. - See: mysqljs/mysql#1367
Specifically I'm talking about: https://github.com/felixge/node-mysql/blob/master/lib/protocol/sequences/Query.js#L131-L145
We were bitten by a bug where we assumed a value (
Object
) was coming back (because it did before) then we changed some code and it then started returning anArray
. Convention (like always doing type checks) can be inconsistent, and writing a wrapper for this feels messy since we are creating a new array to put the value into after it was just popped out of an array.Since it would be a backwards incompatible change to turn this on for everyone, I'd like to suggest having an option.
Supposing you agree to this feature, if there are other areas that need to be patched, it would be awesome for you core devs to point them out in this ticket. That way if someone decides to write a patch it will catch the other cases of this, if they exist.
The text was updated successfully, but these errors were encountered: