906139 - Preserve unknown fields found in downloads.json

Status: RESOLVED FIXED

(Toolkit :: Downloads API, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

When we read the downloads.json storage we read the known fields but discard unknown ones, which means that when the file is serialized again to disk, those fields will be gone.
It's important that current versions do not discard data (even if it's unknown to them) in order to allow future versions to add new fields and not break when a user goes back and forth between different version using the same profile.

Comment 1

[:Paolo Amadini]

I'm marking this as tracking release, this is not strictly a blocker but more
of a nice-to-have for forward compatibility. This should also count as an
add-on extensibility feature, to attach serializable data to Download objects.

Comment 2

[Neil Deakin]

I worked on a partial patch for this which I'll finish up.

Comment 3

[Neil Deakin]

Attachment: preserveprops

Comment 4

[:Paolo Amadini]

Comment on attachment 800840 [details] [diff] [review]
preserveprops

The approach looks good to me, I'd also be interested in what Felipe thinks
since he worked on a similar feature for the Add-ons back-end.

Can we also use this as a mechanism that can be used by add-ons or other
Mozilla products to store their own properties that get serialized? Something
like calling the "unknownProperties" property with the name "data", or
extraData or something along the line.

I wonder if there might be a case where we _don't_ want a property we serialize
in a newer version to be kept after the profile has been used in a previous
version? In any case, as an alternative, we might also break compatibility
completely if we introduce a backwards-incompatible feature.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 800840 [details] [diff] [review]
preserveprops

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

Looks good to me too.  Maintaining the list of known properties will be a bit of a pain. To deal with that what we did something in the AddonRepository work was to use the Obj's prototype to get the list of known properties. I don't know how doable it would be to do here.. those objects though were mostly data and the functions were all prepended with "_", so we used it to skip any property or functions that we didn't want serialized.

In any case, I wonder if it's possible to write a test that would prevent us from adding new properties and forgetting to update the known properties list and it ending up in the unknwn properties

Comment 6

[Neil Deakin]

(In reply to :Felipe Gomes from comment #5)
> In any case, I wonder if it's possible to write a test that would prevent us
> from adding new properties and forgetting to update the known properties
> list and it ending up in the unknwn properties

It won't end up in the unknown properties list. New properties just won't be serialized.

Comment 7

[:Paolo Amadini]

Comment on attachment 800840 [details] [diff] [review]
preserveprops

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

After thinking more about it, I concluded that this mechanism should be used only internally for future unknown properties. An add-on extensibility mechanism, if required, should be implemented as a separate key-value store with an API that prevents large data from being saved in the JSON file, similarly to the session store mechanism.

To indicate that the object shouldn't be used externally, we should make the name private by prefixing it with an underscore, like "_unknownProperties".

With regard to setting arbitrary properties using "createDownload", I'm not sure, but we could just allow it for better compatibility in case an add-on is used on multiple versions of the browser.

This patch should be rebased on top of bug 906134, where we fixed the "startTime" serialization.

::: toolkit/components/jsdownloads/test/unit/test_DownloadStore.js
@@ +274,5 @@
> +  do_check_eq(itemsForLoad[2].target.unknownProperties.target2, "download3target2");
> +
> +  do_check_eq(Object.keys(itemsForLoad[2].saver.unknownProperties).length, 2);
> +  do_check_eq(itemsForLoad[2].saver.unknownProperties.saver1, "download3saver1");
> +  do_check_eq(itemsForLoad[2].saver.unknownProperties.saver2, "download3saver2");

nit: please indent to stay within 80 characters.

Comment 8

[Neil Deakin]

Attachment: preserveprops

Comment 9

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/5f7f853867cf

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5f7f853867cf