Good luck! :)

The migration code should definitely go in HomeProvider.jsm, and but we'll need to make sure the schema version numbers stay consistent between that and HomeProvider.java. Sqlite.jsm lets you get/set the schema version, so we can use that in our own little migration routine. We'll also have to be careful about what kinds of expectations we'll have about schema versions in the UI, since the Java side will be up and running before a migration would happen in JS.

I got tired of getting an error if I called deleteAll before save, and I decided to address that with a basic first step towards a migration system. Right now this only handles the migration from schema version 0 -> 1 (getSchemaVersion will return 0 if the schema version has never been set).

Comment on attachment 8371157 [details] [diff] [review] Create helper method for database creation and migration Review of attachment 8371157 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/modules/HomeProvider.jsm @@ +171,5 @@ > + // We only need to do this check once during the app lifetime. > + if (gDatabaseEnsured) { > + return; > + } > + gDatabaseEnsured = true; Put this at the bottom of the function, in case we throw. @@ +175,5 @@ > + gDatabaseEnsured = true; > + > + // There's nothing to do if the schema is already up to date. > + let dbVersion = yield db.getSchemaVersion(); > + if (dbVersion == SCHEMA_VERSION) { nit: === @@ +180,5 @@ > + return; > + } > + > + // Otherwise, we need to create the items table and set the schema version. > + // XXX: We will need to add migration logic if we ever rev SCHEMA_VERSION. nit: We should include a similar comment on SCHEMA_VERSION that if it's updated, migration logic needs to be added. @@ +204,5 @@ > return Task.spawn(function save_task() { > let db = yield Sqlite.openConnection({ path: DB_PATH }); > > try { > + yield Task.spawn(ensureDatabase(db)); I think it would be safer to add the `openConnection` call and return the db instance from this function.

(In reply to Michael Comella (:mcomella) from comment #3) > @@ +175,5 @@ > > + gDatabaseEnsured = true; > > + > > + // There's nothing to do if the schema is already up to date. > > + let dbVersion = yield db.getSchemaVersion(); > > + if (dbVersion == SCHEMA_VERSION) { > > nit: === Ah, but no! getSchemaVersion returns a string not an integer. > @@ +204,5 @@ > > return Task.spawn(function save_task() { > > let db = yield Sqlite.openConnection({ path: DB_PATH }); > > > > try { > > + yield Task.spawn(ensureDatabase(db)); > > I think it would be safer to add the `openConnection` call and return the db > instance from this function. Nice idea, then any time we get the database we'll make sure it's up to date.

(In reply to :Margaret Leibovic from comment #4) > > nit: === > > Ah, but no! getSchemaVersion returns a string not an integer. Then I would prefer SCHEMA_VERSION is declared as a String, or if we need to do arithmetic operations on it elsewhere, construct a String from it, i.e. `String(SCHEMA_VERSION) === db.getSchemaVersion()`.

(In reply to Michael Comella (:mcomella) from comment #5) > (In reply to :Margaret Leibovic from comment #4) > > > nit: === > > > > Ah, but no! getSchemaVersion returns a string not an integer. > > Then I would prefer SCHEMA_VERSION is declared as a String, or if we need to > do arithmetic operations on it elsewhere, construct a String from it, i.e. > `String(SCHEMA_VERSION) === db.getSchemaVersion()`. This is JS, not Java. Embrace it.

Took your suggestion and made the helper method open the DB connection, and I renamed it to match its new behavior. This actually turned out to be a bit tricky because of the exception handing to make sure we always close the DB connection. We don't want to close the DB in a finally block in getDatabaseConnection, since we want to return an open connection. However, we can't count on consumers closing the DB if it fails during migration logic, since they don't have a reference to the DB at that point. I'm curious to know what you think of this new approach.

(In reply to :Margaret Leibovic from comment #6) > This is JS, not Java. Embrace it. =='s bug-prone coercion behavior isn't listed in JS: The Bad Parts for nothin'. :P I mean, people have spun whole conference talks out of how shitty this language feature (and concat/+) is. For example: what if getSchemaVersion erroneously returns an empty string? "" == 0 That'll happily pass this conditional, rather than being an error. I don't think it's worth *deliberately* baking in a type mismatch with resulting coercion, rounding, and numeric comparison, just because JS allows you to do "1" == 1. If the schema version from the DB is a string, I'd suggest: * fixing that (go ping gps) * parsing it explicitly as a base-10 integer (with an opportunity to handle an error) and using == * making SCHEMA_VERSION explicitly a string, or * at least commenting the shit out of it, lest some poor sucker who hasn't watched "wat: Destroy All Software" -- and thus isn't walking on eggshells every time they try to compare two things in JS -- comes along and cocks this up.

Threw in a parseInt to appease the freedom-haters. I think SCHEMA_VERISON should stay an integer, not a string, since we'll eventually need to do more comparisons with that once we handle migrations between different versions.

Comment on attachment 8371915 [details] [diff] [review] Helper method to get database connection (v2) Review of attachment 8371915 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to :Margaret Leibovic from comment #9) > Threw in a parseInt to appease the freedom-haters. Thanks. :) Convert to r+ with the yield issue resolved. I think we should add a SCHEMA_VERSION history in a comment above the associated variable (e.g. https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/background/healthreport/ProfileInformationCache.java?rev=6fe8a2c4841e#31). It's a good habit to start now before we forget later. ::: mobile/android/modules/HomeProvider.jsm @@ +200,5 @@ > + yield db.setSchemaVersion(SCHEMA_VERSION); > + } > + } catch(e) { > + // Close the DB connection before passing the exception to the consumer. > + yield db.close(); Do we need to yield here? I think we'd only need to if we wanted to wait for the DB to close, which we don't. I noticed we're the only ones who do that with Sqlite.jsm so far (e.g. [1] [2]) - we may want to change those uses too if it's unnecessary. [1]: https://mxr.mozilla.org/mozilla-central/search?string=yield+.*\.close\%28&regexp=1&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central [2]: https://mxr.mozilla.org/mozilla-central/search?string=yield+db\.close\%28&regexp=1&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

(In reply to Michael Comella (:mcomella) from comment #10) > I think we should add a SCHEMA_VERSION history in a comment above the > associated variable (e.g. > https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ > background/healthreport/ProfileInformationCache.java?rev=6fe8a2c4841e#31). > It's a good habit to start now before we forget later. Nice find, I like that. > ::: mobile/android/modules/HomeProvider.jsm > @@ +200,5 @@ > > + yield db.setSchemaVersion(SCHEMA_VERSION); > > + } > > + } catch(e) { > > + // Close the DB connection before passing the exception to the consumer. > > + yield db.close(); > > Do we need to yield here? I think we'd only need to if we wanted to wait for > the DB to close, which we don't. > > I noticed we're the only ones who do that with Sqlite.jsm so far (e.g. [1] > [2]) - we may want to change those uses too if it's unnecessary. > > [1]: > https://mxr.mozilla.org/mozilla-central/search?string=yield+.*\. > close\%28&regexp=1&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla- > central > [2]: > https://mxr.mozilla.org/mozilla-central/search?string=yield+db\. > close\%28&regexp=1&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla- > central Hm... I'm not so sure about that. It looks like you didn't actually find all the Sqlite.jsm usage in the tree. Here are a few examples: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/BookmarkJSONUtils.jsm#630 http://mxr.mozilla.org/mozilla-central/source/services/metrics/storage.jsm#679 http://mxr.mozilla.org/mozilla-central/source/toolkit/components/jsdownloads/src/DownloadImport.jsm#190 I think it's safer to have the yield statement than to leave it out, since weird things might happen if we don't yield and then we try to make another connection. Also, all of this is happening in a background task anyway, so it's not like waiting really hurts us much.

(In reply to :Margaret Leibovic from comment #11) > I think it's safer to have the yield statement than to leave it out, since > weird things might happen if we don't yield and then we try to make another > connection. Generally, it's possible to open multiple SQLite connections at once so I don't think this would be a problem. > Also, all of this is happening in a background task anyway, so > it's not like waiting really hurts us much. While true, if we use that mindset too much on easily preventable performance decreases, we may end up with gradual slowness with no easily identifiable cause - I'd rather dig in, find out if it's necessary or not, and take the appropriate action.

This will be more noticeable in HomeStorage.save() on sync where it gets called multiple times in succession, once for each dataset in an addon, and that for each addon.

Comment on attachment 8371915 [details] [diff] [review] Helper method to get database connection (v2) Review of attachment 8371915 [details] [diff] [review]: ----------------------------------------------------------------- Discussed on IRC. Consensus: * Promise.jsm will be unhappy if `db.close` fails and we don't yield * Thus there must be some reason they chose to return a promise * Negligible performance in a background task anyway