703154 - Simplify the schema migration code

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop]]

Using PRAGMA table_info we can at runtime determine what fields are available in the old database and so selectively retrieve those rather than having to hardcode multiple queries.

Comment 1

[Dave Townsend [:mossop]]

Attachment: WIP

Haven't particularly looked into tests but this is what I'm thinking.

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 575055 [details] [diff] [review]
WIP

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

Me like. This should remove a bunch of maintenance costs, as well we make it a bit more robust.

::: toolkit/mozapps/extensions/XPIProvider.jsm
@@ +4425,5 @@
> +
> +      const REQUIRED = ["internal_id", "id", "location", "userDisabled",
> +                        "installDate", "version"];
> +      const OPTIONAL = ["softDisabled", "applyBackgroundUpdates", "sourceURI",
> +                        "releaseNotesURI", "foreignInstall"];

Can DB_METADATA and DB_BOOL_METADATA be used here to avoid maintaining too many lists? Something like:

REQURED = [whatever]
OPTIONAL = (DB_METADATA + DB_BOOL_METADATA) - REQUIRED

@@ +4431,5 @@
> +      let reqCount = 0;
> +      let props = "";
> +      for (let row in resultRows(stmt)) {
> +        if (REQUIRED.indexOf(row.name) != -1) {
> +          props += "," + row.name;

Would be neater if props were an array you pushed items onto, then you can just use props.join(",") later on (as opposed to props.substring(1)).

@@ +4457,5 @@
>            userDisabled: row.userDisabled == 1,
>            targetApplications: []
>          };
>  
> +        ["softDisabled", "applyBackgroundUpdates"].forEach(function(aProp) {

Should be able to use DB_BOOL_METADATA to make this a bit neater.

Comment 3

[Dave Townsend [:mossop]]

Attachment: patch rev 1

Ok this addresses the comments. It attempts to pull all the fields we know of out of the database so they are available to the code that actually does the migration. I still want a fixed and smaller list there since there are a number of the properties we really shouldn't migrate (stuff out of install.rdf in particular) since it is still possible the add-on could have changed.

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 575293 [details] [diff] [review]
patch rev 1

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

Looks good. Do the existing tests adequately cover having/not having the presence of all the REQUIRED fields?

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 2

Had to unbitrot this and also added a test for a truly unusable DB. No other changes otherwise.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 575545 [details] [diff] [review]
patch rev 2

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

::: toolkit/mozapps/extensions/test/xpcshell/test_migrate5.js
@@ +97,5 @@
> +  });
> +  stmt.finalize();
> +
> +  db.schemaVersion = 100;
> +  Services.prefs.setIntPref("extensions.databaseSchema", 100);

This is gonna be amusing when we eventually do get up to schema version 100 :) Maybe you should tack another 0 to that number.

Comment 7

[Dave Townsend [:mossop]]

Landed on inbound https://hg.mozilla.org/integration/mozilla-inbound/rev/f56181bf0d2f

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/83feacf6ca68

Comment 9

[Ed Morley [:emorley]]

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