991682 - Sqlite.jsm API to clone an open db connection

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Marco Bonardo [:mak]]

I'm in need of an API to clone an existing Storage connection to a Sqlite.jsm connection.
My plan is quite simple, provide Sqlite.cloneStorageConnection(mozIStorageAsyncConnection, options);
it will call asyncClone on the connection and return an opened Sqlite.jsm connection. Options is generic options like shrinkMemoryOnIdleMs, plus readOnly.

I will also provide a clone(readOnly) method to an open connection to get a new Sqlite connection with the same options.

The alternative would be to provide a Sqlite.wrapStorageConnection(mozIStorageConnection) and then call .clone(readOnly) on it, though I feel like we should no promote this code-path, from js one should try to stick to Sqlite.jsm (to avoid foot guns) and if one needs to use a cpp connection either he wants a clone for concurrency, or something may be redesigned. Indeed I think we may want to wontfix bug 915525

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8401579 [details] [diff] [review]
patch v1

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

r=me with a few minor changes

::: toolkit/modules/Sqlite.jsm
@@ +124,5 @@
>    return deferred.promise;
>  }
>  
> +/**
> + * Clones an existing open Storage connection to a Sqlite one.

This sentence is not very clear.

@@ +153,5 @@
> + */
> +function cloneStorageConnection(options) {
> +  let log = Log.repository.getLogger("Sqlite.ConnectionCloner");
> +
> +  let source = options.connection;

|| null
  (let's avoid warnings)

@@ +155,5 @@
> +  let log = Log.repository.getLogger("Sqlite.ConnectionCloner");
> +
> +  let source = options.connection;
> +  if (!source) {
> +    throw new Error("connection not specified in clone options.");

I'd suggest making these errors instances of TypeError.

@@ +186,5 @@
> +    if (!connection) {
> +      log.warn("Could not clone connection: " + status);
> +      deferred.reject(new Error("Could not clone connection: " + status));
> +    }
> +    log.warn("Connection cloned");

Looks like a log.info rather than log.warn.

::: toolkit/modules/tests/xpcshell/test_sqlite.js
@@ +835,5 @@
>  
> +/**
> + * Test Sqlite.cloneStorageConnection.
> + */
> +add_task(function test_cloneStorageConnection() {

function*, these days

@@ +838,5 @@
> + */
> +add_task(function test_cloneStorageConnection() {
> +  let file = new FileUtils.File(OS.Path.join(OS.Constants.Path.profileDir,
> +                                             "test_cloneStorageConnection.sqlite"));
> +  let c = yield new Promise((success, failure) => {

Hey, new style DOM Promise! Does this work in a xpcshell test?

@@ +848,5 @@
> +      }
> +    });
> +  });
> +
> +  let clone = yield Sqlite.cloneStorageConnection({ connection: c, readOnly: true });

Could you check with both values of readOnly?

@@ +863,5 @@
> +add_task(function test_cloneStorageConnection() {
> +  try {
> +    let clone = yield Sqlite.cloneStorageConnection({ connection: null });
> +    do_throw(new Error("Should throw on invalid connection"));
> +  } catch (ex) {}

I'd prefer if the catch could be more specific.

@@ +894,5 @@
> +  // But should not be able to write.
> +  try {
> +    yield clone.execute("CREATE TABLE test (id INTEGER PRIMARY KEY)");
> +    do_throw(new Error("Should not be able to write to a read-only clone."));
> +  } catch (ex) {}

It's a shame the error is not specific. Could you file a followup bug?

Comment 3

[Marco Bonardo [:mak]]

Attachment: patch v2

addressed comments

Comment 4

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/fx-team/rev/bd137ef426a8

Comment 5

[Ryan VanderMeulen [:RyanVM]]

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