New db function: execMany() - execute many statements with detailed results #413
Conversation
…query returned 0 rows.
containing multiple queries.
…d) SQL statements and provides detailed results on each. Also, removed Database.prepareMany() function; it ended up not being useful for what I really wanted to do.
|
Hi, just wondering if anyone has taken a look at this. These are features I need for my use case; I'd like to get some sense of whether or not you might include in sql-js in some form, either in current version or next. If in current version great, if in next version, I might fork and use my fork until the next version comes out. Thanks! |
|
I haven't taken a detailed look at the code yet, and am not particularly qualified to do so, but I have needed something similar, so I appreciate the contribution. I did notice that the statements don't allow for parameters, which would limit its use in my case and might lead to folks doing some dangerous SQL string manipulation to get this to work in their case. Without knowing the code well, I don't know how hard it would be to make it support parameters, but that was the first thing to stood out to me. |
|
It would be possible to include parameters, and obviously that would be preferable for use cases where SQL injection is a possibility. The main thing there would be to work out the interface details (how best to provide the parameters to the function - as an array of arrays matching the # of queries, or maybe just an array of parameters to be used in order?) Once that was decided, I don't think it would be terribly hard to expand the function to use the parameters. |
There was a problem hiding this comment.
I made comments on the implementation details of the PR. But my overall feeling on this is that it tries to address a very specific use case, and that the same could be achieved on the user side if we offered a more generic API. We could have a simple Database.iter_statements API that takes a string and returns an iterable over Statement objects. This would be more general, and would allow users to replicate this functionality in an idiomatic manner. Users could also bind parameters to their statements as they see fit. And we could additionally create a normalized_sql method on Statement that wraps sqlite3_normalized_sql.
| * many result elements as the number of statements in your sql string | ||
| * (statements are separated by a semicolon). | ||
| * | ||
| * Result objects may contain the following members: |
There was a problem hiding this comment.
may contain is not very clear. We should have a clear specification for what the function returns.
| * - success: true/false (all queries) | ||
| * - error: error message if success is false |
There was a problem hiding this comment.
We don't need two fields here. Let's just have an error field with type string|null
| * (even if empty) | ||
| * - values: query results, if query returns a | ||
| * result (even if empty) | ||
| * - rowsModified: rows affected, if query is INSERT, UPDATE, or DELETE |
There was a problem hiding this comment.
We can simply always include it, and set it to 0 if there is 0 modified rows.
| * | ||
| @param {string} sql a string containing some SQL text to execute | ||
| @param {boolean} exitOnError whether or not to continue after error | ||
| @return {Object[]} as described above |
There was a problem hiding this comment.
We will need a better type definition here.
| * - error: error message if success is false | ||
| * - columns: column headers, if query returns a result | ||
| * (even if empty) | ||
| * - values: query results, if query returns a |
There was a problem hiding this comment.
We will need to specify in what form the query results come.
| * Result objects may contain the following members: | ||
| * - success: true/false (all queries) | ||
| * - error: error message if success is false | ||
| * - columns: column headers, if query returns a result |
There was a problem hiding this comment.
We need to specify the type. And can't we always return it ? Just set it to the empty array when a query doesn't return results.
| // eslint-disable-next-line no-constant-condition | ||
| while (true) { |
There was a problem hiding this comment.
They advise to use for (;;) for that purpose, in the documentation: https://eslint.org/docs/rules/no-constant-condition
But this is a code smell anyway. Isn't there a way to reorganise the code to make the stopping condition explicit ?
| var answer = []; | ||
| var result; | ||
| var data; | ||
| var columns; | ||
| var stack; | ||
| var lastSql; | ||
| var nextSql; | ||
| var thisSql; | ||
| var normalizedSql; | ||
| var sqlType; | ||
| var stmt; | ||
| var pStmt; | ||
| var pzTail; | ||
| var returnCode; | ||
| var errorMessage; | ||
| var errorAnswer; |
There was a problem hiding this comment.
I know a lot of the code had this style after the automatic coffee to js translation, but we are now trying to have the variables declared at their first use site.
| // eslint-disable-next-line no-continue | ||
| continue; |
There was a problem hiding this comment.
Can't we have a simple if-else instead of disabling the lint ?
| // eslint-disable-next-line no-continue | ||
| continue; |
|
Closed in favor of #429 |
Consider this a feature request - I need functionality as implemented here for my particular use case. So I wrote some code for it - please let me know if this is something you are willing to incorporate into sql-js. Thanks!