Closed Bug 889239 Opened 7 years ago Closed 7 years ago

Add a fast upgrade path for the Contacts DB

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: reuben, Assigned: reuben)

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Patch (obsolete) — Splinter Review
No reason to go through all the steps when we know we're creating the final schema.
Attachment #770068 - Flags: review?(anygregor)
+++ b/dom/contacts/fallback/ContactDB.jsm
@@ -104,16 +104,23 @@ this.ContactDB = function ContactDB(aGlo
+    if (aOldVersion == 0) {
+      this.createFinalSchema(aTransaction, aDb);
+      this.addDefaultContacts(aTransaction);
+      return;
+    }
+

@@ -379,65 +386,87 @@ ContactDB.prototype = { 
     // Add default contacts
     if (aOldVersion == 0) {
+      this.addDefaultContacts(aTransaction);
+    }
+  },

Will this second `if` statement ever get run? It looks like the first `if` returns if the same condition (aOldVersion == 0) is true.



Also, do we have tests that can verify that both the long-upgrade path and the new  quick-upgrade produce the same schema? The reason I ask is because looking at this I would be worried about regressions as we make schema changes down the line. (now we must modify the schema in two places)
(In reply to Michael Henretty [:mikehenrty] from comment #1)
> Will this second `if` statement ever get run? It looks like the first `if`
> returns if the same condition (aOldVersion == 0) is true.

It won't, nice catch. Second one should just be removed.

> Also, do we have tests that can verify that both the long-upgrade path and
> the new  quick-upgrade produce the same schema? The reason I ask is because
> looking at this I would be worried about regressions as we make schema
> changes down the line. (now we must modify the schema in two places)

Can't think of a way to do that without starting tests with an existing DB. The idea here is that if you want to check what the schema is you can quickly look at createFinalSchema. As you can see, it's small and easy to read, so it shouldn't be a problem.

It also doesn't change the situation a lot, since we don't currently test partial upgrades.
(In reply to Reuben Morais [:reuben] from comment #2)
> As you can see, it's small and easy to
> read, so it shouldn't be a problem.

True.

> It also doesn't change the situation a lot, since we don't currently test
> partial upgrades.

I see. Let's add some? :) You guys know this system way better than I do, but would it be extremely difficult to create those default contacts with the initial version of the DB, run through the long-upgrade, and then separately create the final schema, add the same defaults contacts, and then compare the results, all in the context of a mochitest?
(In reply to Michael Henretty [:mikehenrty] from comment #3)
> I see. Let's add some? :) You guys know this system way better than I do,
> but would it be extremely difficult to create those default contacts with
> the initial version of the DB, run through the long-upgrade, and then
> separately create the final schema, add the same defaults contacts, and then
> compare the results, all in the context of a mochitest?

I guess we could do that as a mochitest-chrome that loads ContactDB directly. And we can just steal fakecontacts.json from Gaia.
Note that to be really extensive we'd need to do the same thing for every DB version so we test every incremental upgrade.
(In reply to Reuben Morais [:reuben] from comment #5)
> Note that to be really extensive we'd need to do the same thing for every DB
> version so we test every incremental upgrade.

To me, it would be enough to just compare the final results of the long-upgrade and quick-upgrade paths. Yes it is not comprehensive, but having this sanity check would help us find problems in both the single version upgrades and the final schema code for any revisions going forward.
Attached patch Patch (obsolete) — Splinter Review
I'm working on the tests discussed here.
Attachment #770068 - Attachment is obsolete: true
Attachment #770068 - Flags: review?(anygregor)
Attachment #772949 - Flags: review?(anygregor)
Attached patch TestsSplinter Review
\o/
Attachment #773693 - Flags: review?(anygregor)
Comment on attachment 772949 [details] [diff] [review]
Patch

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

Nice!
Attachment #772949 - Flags: review?(anygregor) → review+
I guess this needs a rebase now?
Attached patch Patch, rebasedSplinter Review
Attachment #772949 - Attachment is obsolete: true
Attachment #776687 - Flags: review+
Comment on attachment 773693 [details] [diff] [review]
Tests

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +980,5 @@
>    enableSubstringMatching: function enableSubstringMatching(aDigits) {
>      this.substringMatching = aDigits;
>    },
>  
> +  init: function init(aGlobal, aDbVersion) {

When do you call it with a DB version?
(In reply to Gregor Wagner [:gwagner] from comment #12)
> When do you call it with a DB version?

In some past version of this patch ;) Please ignore that chunk, I removed it locally.
Comment on attachment 773693 [details] [diff] [review]
Tests

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +3,5 @@
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  "use strict";
>  
> +this.EXPORTED_SYMBOLS = ["ContactDB", "DB_NAME", "STORE_NAME", "SAVED_GETALL_STORE_NAME", "REVISION_STORE", "DB_VERSION"];

Add a comment saying everything except ContactDB is only needed for testing.
Attachment #773693 - Flags: review?(anygregor) → review+
https://hg.mozilla.org/mozilla-central/rev/fd676bc1011c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in before you can comment on or make changes to this bug.