1319884 - Address FirefoxAdapter feedback from kinto.js#589

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect)

Description

[Ethan Glasser-Camp (:glasserc)]

:n1k0 in https://github.com/Kinto/kinto.js/pull/589 commented on a few changes I made to the FirefoxAdapter storage adapter for Kinto.js. (That PR was the one that allowed FirefoxAdapter to use an externally-managed Sqlite connection rather than creating its own; this was necessary to address feedback from bug 1253740.) :n1k0 observed that this caused two issues for FirefoxAdapter:

- It now exports _init, which is simultaneously an implementation detail AND something that callers are supposed to know that they need to use.

- FirefoxAdapter is now responsible for functioning whether or not a Sqlite connection is present. This adds a certain amount of complexity to the module. :n1k0 suggested I pick one or the other and convert existing uses.

This bug represents the work that fixes these issues.

Comment 1

[Ethan Glasser-Camp (:glasserc)]

In doing this work, I looked at the other main client of kinto.js in Gecko is blocklist-clients.js, and it mainly calls collection.db.open() and collection.db.close(). In this approach, the Sqlite connection is internal to the Collection object. This does have a certain advantage in that 1. it's impossible to "forget" to _init the Sqlite connection (to create tables and so forth), 2. it makes the "common" case, where you don't care about the lifetime of the Sqlite connection, a little easier to manage.

An ideal API would cover all use cases to exist. So far, I haven't been able to find such an API.

Comment 2

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1319884 - Address FirefoxAdapter feedback from kinto.js#589,

Change FirefoxAdapter definitively to require an externally-managed
Sqlite connection in order to function. This connection must be
produced by calling an openConnection() static method, which does the
work of initializing the tables and schema. Passing any other
connection is wrong, but won't be detected at runtime, and might even
work depending on the previous state of the database. Future work
might define a new KintoSqliteConnection type that can only be
produced by this method, so that it's impossible to create an
uninitialized Kinto database.

This change, since it moves Sqlite connections out of the
FirefoxAdapter, also means that the path option is no longer handled
or provided with a default. This means that the previous default,
"kinto.sqlite", is now preserved in a bunch of places all over the
codebase. This is unfortunate, but a migration is outside the scope of
this patch.

Review commit: https://reviewboard.mozilla.org/r/95170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95170/

Comment 3

[Ethan Glasser-Camp (:glasserc)]

Attachment: Bug 1319884 - Leftover cleanups,

I didn't realize this file was here when I updated kinto.js to 6.0.0.

Review commit: https://reviewboard.mozilla.org/r/95172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/95172/

Comment 4

[Mark Goodwin [:mgoodwin]]

Comment on attachment 8813810 [details]
Bug 1319884 - Address FirefoxAdapter feedback from kinto.js#589,

https://reviewboard.mozilla.org/r/95170/#review96770

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8813811 [details]
Bug 1319884 - Leftover cleanups,

https://reviewboard.mozilla.org/r/95172/#review96890

Comment 6

[Ethan Glasser-Camp (:glasserc)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fc9190d4183

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/836efdd6c83e
Address FirefoxAdapter feedback from kinto.js#589, r=mgoodwin
https://hg.mozilla.org/integration/autoland/rev/2648cedc9591
Leftover cleanups, r=MattN

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/836efdd6c83e
https://hg.mozilla.org/mozilla-central/rev/2648cedc9591