Closed Bug 1365143 Opened 3 years ago Closed 3 years ago

Add SchemaVersion for each record in ProfileStorage

Categories

(Toolkit :: Form Manager, enhancement, P1)

x86
macOS
enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: selee, Assigned: selee)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M3])

Attachments

(1 file)

When I implement `prefer` function in bug 1361242, the schema version for each record is a must to compare the two records in the different schema format.
There are some items related to this change:
1. Add schemaVersion field in the internal field.

2. Implement the migration function to fill the empty schema version of an old record with "1". IMHO, the migration is not serious as a major schema change, so it's fine to just fill the schema version for a record missing it with "1".

3. We can remove the schema version for the whole JSON object [1]. The global schema version looks redundant here. That's sufficient that we have add-on schema version (written in JS codes) and the version for each record.

4. Since the schema version will be for each record, the add-on schema version should be for AddressRecords and CreditCardRecords separately.


[1] http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/browser/extensions/formautofill/ProfileStorage.jsm#13
(In reply to Sean Lee [:seanlee][:weilonge] from comment #0)
> 3. We can remove the schema version for the whole JSON object [1]. The
> global schema version looks redundant here. That's sufficient that we have
> add-on schema version (written in JS codes) and the version for each record.

It will be useful if we want to change the top-level keys e.g. addresses + creditCards… perhaps to support Payment Apps in the future though I guess we could migrate from no version to version 2 at that point?
(In reply to Sean Lee [:seanlee][:weilonge] from comment #0)
> 3. We can remove the schema version for the whole JSON object [1]. The
> global schema version looks redundant here. That's sufficient that we have
> add-on schema version (written in JS codes) and the version for each record.
> 
If we will invoke migration operation during initialization, the schema version for the whole JSON object can be considered as the upgrade-to version.

If version 3 addon got a version 2 JSON file, the migration will migrate all records and mark the global version to 3. When initializing in the next time, the global version and add-on version is matched. Then the 2nd time migration can be skipped.
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #1)
> (In reply to Sean Lee [:seanlee][:weilonge] from comment #0)
> > 3. We can remove the schema version for the whole JSON object [1]. The
> > global schema version looks redundant here. That's sufficient that we have
> > add-on schema version (written in JS codes) and the version for each record.
> 
> It will be useful if we want to change the top-level keys e.g. addresses +
> creditCards… perhaps to support Payment Apps in the future though I guess we
> could migrate from no version to version 2 at that point?

Yes, that's the reason to keep the global version. Another reason is at comment 2. I will keep the global version in the JSON file.
Assignee: nobody → selee
Status: NEW → ASSIGNED
Comment on attachment 8868037 [details]
Bug 1365143 - Add schema version number for each Form Fill record in ProfileStorage.;

https://reviewboard.mozilla.org/r/139626/#review142978

r+ with two nits.

::: browser/extensions/formautofill/ProfileStorage.jsm:100
(Diff revision 3)
>                                     "@mozilla.org/uuid-generator;1",
>                                     "nsIUUIDGenerator");
>  
>  const PROFILE_JSON_FILE_NAME = "autofill-profiles.json";
>  
>  const SCHEMA_VERSION = 1;

nit: `STORAGE_SCHEMA_VERSION`

::: browser/extensions/formautofill/ProfileStorage.jsm:102
(Diff revision 3)
>  
>  const PROFILE_JSON_FILE_NAME = "autofill-profiles.json";
>  
>  const SCHEMA_VERSION = 1;
> +const ADDRESS_SCHEMA_VERSION = 1;
> +const CREDITCARD_SCHEMA_VERSION = 1;

nit: `CREDIT_CARD_SCHEMA_VERSION`
Attachment #8868037 - Flags: review?(lchang) → review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/53244a64eed9
Add schema version number for each Form Fill record in ProfileStorage.; r=lchang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/53244a64eed9
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.