889239 - Add a fast upgrade path for the Contacts DB

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Reuben Morais [:reuben]]

Attachment: Patch

No reason to go through all the steps when we know we're creating the final schema.

Comment 1

[Michael Henretty [:mikehenrty][:mhenretty]]

+++ 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)

Comment 2

[Reuben Morais [:reuben]]

(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.

Comment 3

[Michael Henretty [:mikehenrty][:mhenretty]]

(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?

Comment 4

[Reuben Morais [:reuben]]

(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.

Comment 5

[Reuben Morais [:reuben]]

Note that to be really extensive we'd need to do the same thing for every DB version so we test every incremental upgrade.

Comment 6

[Michael Henretty [:mikehenrty][:mhenretty]]

(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.

Comment 7

[Reuben Morais [:reuben]]

Attachment: Patch

I'm working on the tests discussed here.

Comment 8

[Reuben Morais [:reuben]]

Attachment: Tests

\o/

Comment 9

[Gregor Wagner [:gwagner]]

Comment on attachment 772949 [details] [diff] [review]
Patch

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

Nice!

Comment 10

[Gregor Wagner [:gwagner]]

I guess this needs a rebase now?

Comment 11

[Reuben Morais [:reuben]]

Attachment: Patch, rebased

Comment 12

[Gregor Wagner [:gwagner]]

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?

Comment 13

[Reuben Morais [:reuben]]

(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 14

[Gregor Wagner [:gwagner]]

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.

Comment 15

[Reuben Morais [:reuben]]

Thanks!

https://hg.mozilla.org/integration/b2g-inbound/rev/fd676bc1011c

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd676bc1011c