ForbesLindesay / atdatabases

TypeScript clients for databases that prevent SQL Injection
https://www.atdatabases.org
MIT License
608 stars 47 forks source link

sql.file does not support multiple statements for sqlite #90

Open petehunt opened 4 years ago

petehunt commented 4 years ago

hey - thanks for a great library. i haven't tested this with any other db besides sqlite, but it doesn't seem to support multiple statements via sql.file: https://gist.github.com/petehunt/6a889fc8d01e14fb1667a1e7f9cb0ffd

this is pretty important as most of the time i have a separate .sql file to set up the schema. right now i work around it by reading the file directly and splitting on semicolons -- not the most optimal way to do it i think :)

ForbesLindesay commented 4 years ago

I think this is specific to SQLite, because we use Database.all to run the underlying query and that API only supports passing a single query.

Overall this is pretty inconsistent between databases though. I'm starting to think the best option would be to have an API in @databases/sql that you can call to split multiple statements into an Array (i.e. SQLQuery[]). We could then make a breaking change to @databases/pg and possibly @databases/mysql so that the interface becomes overloaded like:

interface Queryable {
  /**
   * Accepts an SQLQuery with a single statement, and errors if there are multiple statements in one query
   */
  query(q: SQLQuery): Promise<any[]>

  /**
   * Accepts an array of SQLQueries with one statement each, and returns an array where each
   * element represents the results of one statement.
   *
   * If you pass it an empty array it returns an empty array without any call to the database.
   * If you pass it an array with only one statement, it returns an array of length 1 containing
   * an array of the results to the statement.
   */
  query(q: SQLQuery[]): Promise<any[][]>
}

We would then extend SQLQuery with:

interface SQLQuery {
  // Split this query into an array where each item in the array is one statement.
  // If this query is an empty string, or only contains comments, a string of length 0 is returned.
  split(): SQLQuery[]
  // ...existing methods & properties...
}

The breaking change here is that currently we allow you to pass multiple statements in one go to query in Postgres. If you do that, we run all statements, and return the results of the last statement.

Decisions / Questions

  1. Is it a good idea to require the caller of the API to be explicit about when a query has multiple statements? The problem with supporting this implicitly, is that it can be easy to have code that breaks when there is only one query or where there are 0 queries. We could keep the postgres behaviour for query (i.e. if there are multiple queries, just return the value from the last query).
  2. What happens when a query fails? If this API is used outside of a transaction, we could implicitly create a transaction for just these queries, this might be the safest thing to do. SQLite's .exec function just runs them one at a time and stops at the first error, but this feels pretty dangerous.