293707 - update storage provider to handle alarms and other bits

Status: RESOLVED FIXED

(Calendar :: Provider: Local Storage, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

The attached patch does a bunch of fixes to the storage provider, fixing the
alarm handling functionality along the way.

It also moves events and todos into their own tables instead of putting them all
in one "cal_items" table, to allow sqlite to better take advantage of any
indexes we create.  (e.g. for start date.)

If anyone has a largeish database right now of items, i'd like to get a copy so
I can better test the migration code.

Comment 1

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: storage provider patch

Comment 2

[Mike Shaver (:shaver emeritus)]

Comment on attachment 183248 [details] [diff] [review]
storage provider patch

>+  /**
>+   * Check if the given table exists.
>+   *
>+   * @param aTableName   The table to check
>+   * @returns TRUE if table exists, FALSE otherwise.
>+   */
>+  boolean tableExists (in AUTF8String aTableName);

This is good and righteous, but I wonder if we don't want to be
returning some generation number here, instead of a boolean. I
think it's going to be pretty common for people to add columns
over time, and if we gave them a way to easily increase and check
the schema generation I think we could improve our data-migration
story before we suffer for it.

As an example, perhaps: you don't appear to increase the schema version in the
database after you update the tables.

This is an improvement, though, to be sure, so we should get it
in.

>+    nsCString query("SELECT name FROM sqlite_master WHERE type = 'table' AND name ='");
>+    query.Append(aSQLStatement);
>+    query.AppendLiteral("'");
>+
>+    sqlite3_stmt *stmt = nsnull;
>+    int srv = sqlite3_prepare (mDBConn, query.get(), query.Length(), &stmt, nsnull);

It's kinda too bad that we can't just sqlite3_exec this and
then ask for the row count, as a simpler check for presence in
the table, but we work with what sqlite gives us!

> function createStatement (dbconn, sql) {
>-    var stmt = dbconn.createStatement(sql);
>-    var wrapper = MozStorageStatementWrapper();
>-    wrapper.initialize(stmt);
>-    return wrapper;
>+    try {
>+        var stmt = dbconn.createStatement(sql);
>+        var wrapper = MozStorageStatementWrapper();
>+        wrapper.initialize(stmt);
>+        return wrapper;
>+    } catch (e) {
>+        dump ("Exception:\n" + e + "\n");
>+        dump ("While trying to createStatement '" + sql + "'\n");
>+        dump ("SQL Error: " + dbconn.lastErrorString + "\n");
>+    }
> }

You can use Components.reportError if you want these to be
visible in the console, BTW.  (It takes exception objects and
strings.)

>+            var expanded_items;

You seem to use camelCase for pretty much everything else in
this file!

>+        function set_date_param_helper(params, entryname, cdt) {

Except this, I guess.

r=shaver

Comment 3

[Mike Shaver (:shaver emeritus)]

Comment on attachment 183248 [details] [diff] [review]
storage provider patch

To be clearer: r=shaver with the naming and schema-update issues fixed.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch

I landed the mozStorage bits from the first patch; here's a new pass with fixed
CalendarManager (table is locked) stuff, and fixed table migration.

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked in with fixes for schema upgrade.