Bug 1588329 - Introduce `mozIStorageAsyncConnection::variableLimit`. r?mak! draft
authorLina Cambridge <lina@yakshaving.ninja>
Sat, 12 Oct 2019 23:21:43 +0000
changeset 2378197 1363fe68bdd51223343e1d57b657dfe386b22c90
parent 2378121 7fc0a96a5ca10811216de69f4f54b7a13e3fee28
child 2378198 e31679dc527279c29f6049ba4c0f24c098913b39
push id434587
push userreviewbot
push dateSat, 12 Oct 2019 23:22:06 +0000
treeherdertry@55835b18a856 [default view] [failures only]
reviewersmak
bugs1588329
milestone71.0a1
Bug 1588329 - Introduce `mozIStorageAsyncConnection::variableLimit`. r?mak! This is a wrapper around the `sqlite3_limit` interface that returns the binding parameter limit. Adding this getter lets us clean up the inline `SQLITE_MAX_VARIABLE_NUMBER` constants scattered around Places. Differential Diff: PHID-DIFF-ntigwgzrro5tgglpskuq
dom/cache/Connection.cpp
storage/mozIStorageAsyncConnection.idl
storage/mozStorageConnection.cpp
storage/rust/src/lib.rs
storage/test/unit/test_storage_connection.js
third_party/sqlite3/src/sqlite.symbols
toolkit/modules/Sqlite.jsm
--- a/dom/cache/Connection.cpp
+++ b/dom/cache/Connection.cpp
@@ -221,16 +221,21 @@ Connection::GetDefaultTransactionType(in
 }
 
 NS_IMETHODIMP
 Connection::SetDefaultTransactionType(int32_t aType) {
   return mBase->SetDefaultTransactionType(aType);
 }
 
 NS_IMETHODIMP
+Connection::GetVariableLimit(int32_t* aResultOut) {
+  return mBase->GetVariableLimit(aResultOut);
+}
+
+NS_IMETHODIMP
 Connection::BeginTransaction() { return mBase->BeginTransaction(); }
 
 NS_IMETHODIMP
 Connection::CommitTransaction() { return mBase->CommitTransaction(); }
 
 NS_IMETHODIMP
 Connection::RollbackTransaction() { return mBase->RollbackTransaction(); }
 
--- a/storage/mozIStorageAsyncConnection.idl
+++ b/storage/mozIStorageAsyncConnection.idl
@@ -36,16 +36,23 @@ interface mozIStorageAsyncConnection : n
   /**
    * The default behavior for all transactions run on this connection. Defaults
    * to `TRANSACTION_DEFERRED`, and can be overridden for individual
    * transactions.
    */
   attribute int32_t defaultTransactionType;
 
   /**
+   * The maximum number of bound parameters for statements executed on this
+   * connection. Exceeding this limit will cause `create{Async}Statement` to
+   * return a "too many SQL variables" error.
+   */
+  readonly attribute int32_t variableLimit;
+
+  /**
    * Close this database connection, allowing all pending statements
    * to complete first.
    *
    * @param aCallback [optional]
    *        A callback that will be notified when the close is completed,
    *        with the following arguments:
    *        - status: the status of the call
    *        - value: |null|
--- a/storage/mozStorageConnection.cpp
+++ b/storage/mozStorageConnection.cpp
@@ -1938,16 +1938,29 @@ Connection::GetDefaultTransactionType(in
 NS_IMETHODIMP
 Connection::SetDefaultTransactionType(int32_t aType) {
   NS_ENSURE_ARG_RANGE(aType, TRANSACTION_DEFERRED, TRANSACTION_EXCLUSIVE);
   mDefaultTransactionType = aType;
   return NS_OK;
 }
 
 NS_IMETHODIMP
+Connection::GetVariableLimit(int32_t* _limit) {
+  if (!connectionReady()) {
+    return NS_ERROR_NOT_INITIALIZED;
+  }
+  int limit = ::sqlite3_limit(mDBConn, SQLITE_LIMIT_VARIABLE_NUMBER, -1);
+  if (limit < 0) {
+    return NS_ERROR_UNEXPECTED;
+  }
+  *_limit = limit;
+  return NS_OK;
+}
+
+NS_IMETHODIMP
 Connection::BeginTransaction() {
   if (!connectionReady()) {
     return NS_ERROR_NOT_INITIALIZED;
   }
   nsresult rv = ensureOperationSupported(SYNCHRONOUS);
   if (NS_FAILED(rv)) {
     return rv;
   }
--- a/storage/rust/src/lib.rs
+++ b/storage/rust/src/lib.rs
@@ -21,19 +21,19 @@
 //! from Rust. It only wraps the synchronous API, so you can either manage
 //! the entire connection from a background thread, or use the `moz_task`
 //! crate to dispatch tasks to the connection's async thread. Executing
 //! synchronous statements on the main thread is not supported, and will
 //! assert in debug builds.
 
 #![allow(non_snake_case)]
 
-use std::{borrow::Cow, error, fmt, ops::Deref, result};
+use std::{borrow::Cow, convert::TryFrom, error, fmt, ops::Deref, result};
 
-use nserror::{nsresult, NS_ERROR_NO_INTERFACE};
+use nserror::{nsresult, NS_ERROR_NO_INTERFACE, NS_ERROR_UNEXPECTED};
 use nsstring::nsCString;
 use storage_variant::VariantType;
 use xpcom::{
     getter_addrefs,
     interfaces::{
         mozIStorageAsyncConnection, mozIStorageConnection, mozIStorageStatement, nsIEventTarget,
         nsIThread,
     },
@@ -63,16 +63,27 @@ impl Conn {
     }
 
     /// Returns the wrapped `mozIStorageConnection`.
     #[inline]
     pub fn connection(&self) -> &mozIStorageConnection {
         &self.handle
     }
 
+    /// Returns the maximum number of bound parameters for statements executed
+    /// on this connection.
+    pub fn variable_limit(&self) -> Result<usize> {
+        let mut limit = 0i32;
+        let rv = unsafe { self.handle.GetVariableLimit(&mut limit) };
+        if rv.failed() {
+            return Err(Error::Limit);
+        }
+        usize::try_from(limit).map_err(|_| Error::Limit)
+    }
+
     /// Returns the async thread for this connection. This can be used
     /// with `moz_task` to run synchronous statements on the storage
     /// thread, without blocking the main thread.
     pub fn thread(&self) -> Result<RefPtr<nsIThread>> {
         let target = self.handle.get_interface::<nsIEventTarget>();
         target
             .and_then(|t| t.query_interface::<nsIThread>())
             .ok_or(Error::NoThread)
@@ -382,16 +393,19 @@ impl DatabaseOp {
 
 /// Storage errors.
 #[derive(Debug)]
 pub enum Error {
     /// A connection doesn't have a usable async thread. The connection might be
     /// closed, or the thread manager may have shut down.
     NoThread,
 
+    /// Failed to get a limit for a database connection.
+    Limit,
+
     /// A database operation failed. The error includes a SQLite result code,
     /// and an explanation string.
     Database {
         rv: nsresult,
         op: DatabaseOp,
         code: i32,
         message: nsCString,
     },
@@ -448,31 +462,33 @@ impl From<nsresult> for Error {
         Error::Other(rv)
     }
 }
 
 impl From<Error> for nsresult {
     fn from(err: Error) -> nsresult {
         match err {
             Error::NoThread => NS_ERROR_NO_INTERFACE,
+            Error::Limit => NS_ERROR_UNEXPECTED,
             Error::Database { rv, .. }
             | Error::BindByIndex { rv, .. }
             | Error::BindByName { rv, .. }
             | Error::InvalidColumn { rv, .. }
             | Error::GetByIndex { rv, .. }
             | Error::GetByName { rv, .. }
             | Error::Other(rv) => rv,
         }
     }
 }
 
 impl fmt::Display for Error {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
             Error::NoThread => f.write_str("Async thread unavailable for storage connection"),
+            Error::Limit => f.write_str("Failed to get limit for storage connection"),
             Error::Database {
                 op, code, message, ..
             } => {
                 if message.is_empty() {
                     write!(f, "Failed to {} with code {}", op.what(), code)
                 } else {
                     write!(
                         f,
--- a/storage/test/unit/test_storage_connection.js
+++ b/storage/test/unit/test_storage_connection.js
@@ -1004,16 +1004,23 @@ add_task(async function test_defaultTran
   info("Clean up");
   for (let stmt of stmts) {
     stmt.finalize();
   }
   await asyncClose(clone);
   await asyncClose(db);
 });
 
+add_task(async function test_variableLimit() {
+  info("Open connection");
+  let db = Services.storage.openDatabase(getTestDB());
+  Assert.equal(db.variableLimit, 999, "Should return default limit");
+  await asyncClose(db);
+});
+
 add_task(async function test_getInterface() {
   let db = getOpenedDatabase();
   let target = db
     .QueryInterface(Ci.nsIInterfaceRequestor)
     .getInterface(Ci.nsIEventTarget);
   // Just check that target is non-null.  Other tests will ensure that it has
   // the correct value.
   Assert.ok(target != null);
--- a/third_party/sqlite3/src/sqlite.symbols
+++ b/third_party/sqlite3/src/sqlite.symbols
@@ -67,16 +67,17 @@ sqlite3_free_table
 sqlite3_get_autocommit
 sqlite3_get_auxdata
 sqlite3_get_table
 sqlite3_initialize
 sqlite3_interrupt
 sqlite3_last_insert_rowid
 sqlite3_libversion
 sqlite3_libversion_number
+sqlite3_limit
 sqlite3_load_extension
 sqlite3_malloc
 sqlite3_memory_highwater
 sqlite3_memory_used
 sqlite3_mutex_alloc
 sqlite3_mutex_enter
 sqlite3_mutex_free
 sqlite3_mutex_leave
--- a/toolkit/modules/Sqlite.jsm
+++ b/toolkit/modules/Sqlite.jsm
@@ -1345,16 +1345,26 @@ OpenedConnection.prototype = Object.free
    * Please do _not_ add new uses of `unsafeRawConnection` without review
    * from a storage peer.
    */
   get unsafeRawConnection() {
     return this._connectionData._dbConn;
   },
 
   /**
+   * Returns the maximum number of bound parameters for statements executed
+   * on this connection.
+   *
+   * @type {number}
+   */
+  get variableLimit() {
+    return this.unsafeRawConnection.variableLimit;
+  },
+
+  /**
    * The integer schema version of the database.
    *
    * This is 0 if not schema version has been set.
    *
    * @return Promise<int>
    */
   getSchemaVersion(schemaName = "main") {
     return this.execute(`PRAGMA ${schemaName}.user_version`).then(