Closed Bug 703154 Opened 8 years ago Closed 8 years ago

Simplify the schema migration code

Categories

(Toolkit :: Add-ons Manager, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: mossop, Assigned: mossop)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch WIP (obsolete) — Splinter Review
Haven't particularly looked into tests but this is what I'm thinking.
Attachment #575055 - Flags: feedback?(bmcbride)
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.
Attachment #575055 - Flags: feedback?(bmcbride) → feedback+
Attached patch patch rev 1 (obsolete) — Splinter Review
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.
Assignee: nobody → dtownsend
Attachment #575055 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #575293 - Flags: review?(bmcbride)
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?
Attachment #575293 - Flags: review?(bmcbride) → review+
Attached patch patch rev 2Splinter Review
Had to unbitrot this and also added a test for a truly unusable DB. No other changes otherwise.
Attachment #575293 - Attachment is obsolete: true
Attachment #575545 - Flags: review?(bmcbride)
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.
Attachment #575545 - Flags: review?(bmcbride) → review+
https://hg.mozilla.org/mozilla-central/rev/83feacf6ca68
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Blocks: 705631
You need to log in before you can comment on or make changes to this bug.