Sqlite.jsm API to clone an open db connection

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks 1 bug, {dev-doc-needed})

unspecified
mozilla31
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [async:team])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

5 years ago
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
Assignee

Comment 1

5 years ago
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #8401579 - Flags: review?(dteller)
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?
Attachment #8401579 - Flags: review?(dteller) → review+
Assignee

Updated

5 years ago
Blocks: 993123
Assignee

Comment 3

5 years ago
Posted patch patch v2Splinter Review
addressed comments
Attachment #8401579 - Attachment is obsolete: true
Assignee

Comment 4

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/bd137ef426a8
Flags: in-testsuite+
Keywords: dev-doc-needed
Target Milestone: --- → mozilla31
https://hg.mozilla.org/mozilla-central/rev/bd137ef426a8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.