833965 - Sqlite.jsm should support an array of binding parameters

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Marco Bonardo [:mak]]

In storage you can crate a newBindingParamsArray andaddParams to it, so that if you have to insert 5 rows, you don't have to loop and execute 5 times, you can just build the params array of 5 entries and bind the statement to it, it will automatically execute it 5 times with the params.

Sqlite.jsm needs to support something similar, it currently supports an array or an object, could support an array of objects maybe? Though that may complicate a bit the API.

on the other side, it can't just accept a mozIStorageBindingParamsArray since to make one you need a mozIStorageStatement (I'm not sure if it's possible to CreateInstance it, I don't think it is).

Comment 1

[Richard Newman [:rnewman]]

Marco, Greg: what do you think about these two solutions?

• A `bulkParams` argument to execute/executeCached. Simple, synchronous, but sticks a `null` in the signature. Simpler than trying to introspect the `params` argument.

• Allow `params` to be a function that takes a `mozIStorageBindingParamsArray`, which returns a promise that resolves to the fully-populated array. Async, doesn't bloat the argument list, but now the caller needs to provide a function that fleshes out the params array.

Any preference? I'll probably start sketching out the latter tomorrow.

Comment 2

[Richard Newman [:rnewman]]

Attachment: The bare minimum. v1

I'm not sure what's going on with affectedRows. Looks like a storage issue, or a flawed assumption…

Comment 3

[Gregory Szorc [:gps]]

affectedRows will be killed in bug 702559.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 731900 [details] [diff] [review]
The bare minimum. v1

Review of attachment 731900 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/Sqlite.jsm
@@ +708,5 @@
>        }
> +    } else if (params && typeof(params) == "function") {
> +      let bindings = statement.newBindingParamsArray();
> +      params(bindings);
> +      statement.bindParameters(bindings);

First, I'm not sure how I feel about this API. An array of non-scalars seems like a less error-prone solution. Although, I suppose there technically is overhead associated with creating the data structure in JS then converting to a Storage type.

Second, what happens if params() throws? You at least need a test for this.

Third, where are the docs?

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +588,5 @@
> +    bp.bindByName("id", 5);
> +    bp.bindByName("path", "toofoo");
> +    array.addParams(bp);
> +  }
> +    

Nit: Trailing whitespace.

Comment 5

[Richard Newman [:rnewman]]

OK, I've tried several different ways to bind arrays. I've done so inside and outside of transactions. I've tried immediate and deferred. My conclusion is that running *within the context of Sqlite.jsm*, any execution of a statement bound to an array of params causes Error 1 to be logged, but everything to work fine.

The first test prints no error, the second prints an error.

add_task(function test_programmatic_binding() {
  let c = yield getDummyDatabase("programmatic_binding");

  let bindings = [
    {id: 1,    path: "foobar"},
    {id: null, path: "baznoo"},
    {id: 5,    path: "toofoo"},
  ];

  let sql = "INSERT INTO dirs VALUES (:id, :path)";
  let statement = c.bindCached(sql, bindings);

  let result = yield c.executeBoundStatement(statement);
  do_check_eq(result.length, 0);
  do_check_eq(c.lastInsertRowID, 5);
  yield c.close();
});

add_task(function test_programmatic_binding_transaction() {
  let c = yield getDummyDatabase("programmatic_binding_transaction");

  let bindings = [
    {id: 1,    path: "foobar"},
    {id: null, path: "baznoo"},
    {id: 5,    path: "toofoo"},
  ];

  let sql = "INSERT INTO dirs VALUES (:id, :path)";
  let statement = c.bindCached(sql, bindings);

  // Executing this statement *in any way* within a transaction
  // will result in an error being logged.
  yield c.executeTransaction(function transaction() {
    let result = yield c.executeBoundStatement(statement);
    do_check_eq(result.length, 0);
    do_check_eq(c.lastInsertRowID, 5);
  });
  yield c.close();
});

Comment 6

[Richard Newman [:rnewman]]

Here is a startlingly long test that indicates that this is a storage bug.

add_task(function test_direct() {
  let file = FileUtils.getFile("TmpD", ["test_direct.sqlite"]);
  file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
  print("Opening " + file.path);
 
  let db = Services.storage.openDatabase(file);
  print("Opened " + db);

  db.executeSimpleSQL("CREATE TABLE types (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT, UNIQUE (name))");
  print("Executed setup.");

  let statement = db.createAsyncStatement("INSERT INTO types (name) VALUES (:name)");
  let params = statement.newBindingParamsArray();
  let one = params.newBindingParams();
  one.bindByName("name", null);
  params.addParams(one);
  let two = params.newBindingParams();
  two.bindByName("name", "bar");
  params.addParams(two);

  print("Beginning transaction.");
  db.beginTransaction();
  statement.bindParameters(params);

  let deferred = Promise.defer();
  print("Executing async.");
  statement.executeAsync({
    handleResult: function (resultSet) {
    },

    handleError:  function (error) {
      print("Error when executing SQL (" + error.result + "): " +
            error.message);
      print("Original error: " + error.error);
      errors.push(error);
      deferred.reject();
    },

    handleCompletion: function (reason) {
      print("Completed.");
      deferred.resolve();
    }
  });

  yield deferred.promise;
  db.commitTransaction();
  statement.finalize();
  deferred = Promise.defer();
  db.asyncClose(function () {
    deferred.resolve()
  });
  yield deferred.promise; 
});


This prints the same error.

Comment 7

[Marco Bonardo [:mak]]

(In reply to Gregory Szorc [:gps] from comment #4)
> Although, I suppose there technically is
> overhead associated with creating the data structure in JS then converting
> to a Storage type.

In almost all of the cases the list of params will be quite short, the gain from a very simple js API could thus be much higher than the bare overhead cost of converting a few js. I think here we need to expose a very js-friendly API, even if we don't accept directly an object or an array we could expose something like a builder through the SQLite object.
Honestly the API in the v1 patch doesn't seem particularly funny or easy to use :(

Comment 8

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v2

Provide object bindings instead.

Note that I also clean up some whitespace here. Too lazy to upload new versions of the other patch; it's late.

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 733733 [details] [diff] [review]
Proposed patch. v2

Review of attachment 733733 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +588,5 @@
> +
> +  let sql = "INSERT INTO dirs VALUES (:id, :path)";
> +  let result = yield c.execute(sql, bindings);
> +  do_check_eq(result.length, 0);
> +  do_check_eq(c.lastInsertRowID, 5);

Please rewrite to not use lastInsertRowID since it is going away.

@@ +608,5 @@
> +    do_check_eq(result.length, 0);
> +    do_check_eq(c.lastInsertRowID, 5);
> +  });
> +  yield c.close();
> +});

Please add sister tests that incur an error on non-initial statement execution (perhaps a column violating a unique constraint) that verify the state of the database is what you expect. i.e. if the behavior of storage w.r.t. implicit transactions (and presumably rollback if 1 statement fails) changes, I want a test to fail. Also, please document what happens when errors occur.

Comment 10

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v3

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 735423 [details] [diff] [review]
Proposed patch. v3

Review of attachment 735423 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +686,5 @@
> +  // rolled back, but the first row still exists.
> +  let rows = yield c.executeCached("SELECT * from dirs");
> +  do_check_eq(rows.length, 1);
> +  do_check_eq(rows[0].getResultByName("path"), "works");
> +  yield c.close();

I don't see what this test is doing separately from the one above beside execute a statement before executeTransaction() and ensure the semantics of transactions work as they should.

I think the test you want to write is similar to this one except there is no c.executeTransaction(): we perform the multi-bound statement as a regular statement and test the behavior of the implicit transaction.

Comment 12

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v4

Comment 13

[Richard Newman [:rnewman]]

https://hg.mozilla.org/services/services-central/rev/df617f125392

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/df617f125392