Turn javascript arrays into SQL lists #411
Conversation
|
@iamleeg From what I've understood - sql statement and parameters (which should substitude "?" or similar symbols) goes directly into NativeModule (native part of react-native-sqlite-storage). So I think this fix should be there, on native side - that's where error happens, not inside javascript. |
|
So it looks like this is not a correct fix. |
|
@likern that's not quite correct. If you look at the rest of the function I changed, it's responsible for turning JS objects into strings that can be handled in the native module. Before the change, it got arrays wrong, because it just stringified them. Now, it gets them right. |
|
But what I've seen from the code it puts parameters into array which later in passed to Native part. So it is not stringified in JavaScript. Convertion from array to string happens somewhere in Java. |
| @@ -501,6 +501,16 @@ SQLitePluginTransaction.prototype.addStatement = function(sql, values, success, | |||
| } else if (t === 'boolean') { | |||
| //Convert true -> 1 / false -> 0 | |||
| params.push(~~v); | |||
| } else if (Array.isArray(v)) { | |||
| s = '('; | |||
| for (i = 0, len2 = v.length; len2 > 1 && i < len2 - 1; i++) { | |||
There was a problem hiding this comment.
This loop is overly complicated for a simple thing it is trying to do. I would suggest to use the following standard loop format
for (i = 0; i < v.length; i++) {
s += v[i].toString();
s += (i < v.length - 1) ? "," : "";
}
| for (i = 0, len2 = v.length; len2 > 1 && i < len2 - 1; i++) { | ||
| s = s + v[i].toString() + ','; | ||
| } | ||
| if (len2 > 0) { |
There was a problem hiding this comment.
this is no longer necessary if the suggested loop is used.
| s = s + v[len2 - 1]; | ||
| } | ||
| s = s + ')'; | ||
| params.push(s); |
There was a problem hiding this comment.
so a couple of things.
- if the array is empty, we will generate "()" - is this an expected behavior ?
- Doing toString() inside the conversion loop essentially coerces all values into strings - is this what you had in mind? I understand that the values in the array should be of the same type, but what if they are numbers or boolean ? Should we handle these cases as well ?
| @@ -501,6 +501,16 @@ SQLitePluginTransaction.prototype.addStatement = function(sql, values, success, | |||
| } else if (t === 'boolean') { | |||
| //Convert true -> 1 / false -> 0 | |||
| params.push(~~v); | |||
| } else if (Array.isArray(v)) { | |||
There was a problem hiding this comment.
perhaps you can used values.constructor === Array for consistency since it is already used on line 495 ?
| @@ -489,7 +489,7 @@ SQLitePluginTransaction.prototype.executeSql = function(sql, values, success, er | |||
| }; | |||
|
|
|||
| SQLitePluginTransaction.prototype.addStatement = function(sql, values, success, error) { | |||
| var j, len1, params, sqlStatement, t, v; | |||
| var j, len1, len2, params, sqlStatement, t, v, i, s; | |||
There was a problem hiding this comment.
len2 is not necessary if a simpler loop is used
|
@iamleeg did you test you fix on iOS/Android and native Android ? |
|
@iamleeg if selected_roles will be a string s = "(1,2,3,4)" then your query would have to become correct? It is either we generate a string with surrounding () or we generate a comma separated list instead s="1,2,3,4" and then the query would be I think my preference would be to generate a comma separated list only without creating artificial () which are really a function of SQL syntax and can vary.... What do you think ? |
Fixes #410.