952304 - (JSONStore) JSON storage API for addons to use in storing data and settings

Status: RESOLVED WONTFIX

(Toolkit :: Async Tooling, defect)

Description

[Alex Vincent [:WeirdAl]]

Spun off from bug 915838 comment 19:

I wrote this module for APN and have their written permission to submit it to Mozilla under my name.  Standard MPL/LGPL/GPL for Mozilla code applies.

This module is for storing and retrieving JSON values on the filesystem under the extension-data directory for an addon.  We go two levels deep (namespace, name, value) because our particular needs required that.  This should be a very acceptable alternative to preferences for addons to use.

I'm not submitting it as a patch just yet.  First, we'd have to determine where in the source tree and in the final build it should live.  Second, there is a known bug whereby we wait up to five seconds before flushing to the disk.  A FF crash or shutdown could cause data loss.  AsyncShutdown.jsm is probably the cure for that.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Seems like some overlap with bug 947694 and bug 866238. I'm not sure that we want something so add-on specific - why not make it a touch more generic and we can use it in product code as well? In general I don't like the idea of having APIs "only for addons" - it implies the use cases are drastically different, when they usually aren't.

Comment 2

[Alex Vincent [:WeirdAl]]

I have no real objection to expanding this beyond addons support.  As for overlap, I can see that could be a concern, yes.  We are entering new territory here, so I'm open to discussion.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Generally, I like higher-level APIs. However, there is a cost to introducing a new API, in particular if it "just" provides a new way to do something that is already possible with existing APIs, so I'd like you to convince me that the benefits of your new API are sufficient to justify its introduction.

For comparison, here's how you can read/modify/write a JSON objet in three lines (assuming Task.jsm):

 let object = JSON.parse(yield read(path));
 object.foo.bar = "sna";
 writeAtomic(JSON.stringify(object), path, { tmpPath: path + ".tmp" } );

Comment 4

[Alex Vincent [:WeirdAl]]

When I wrote this API, I did not consider the possibility of the API returning Promise objects, instead relying on the old callback model.  That is something to reconsider.

That said, I believe there are several advantages to the new API:

# All the file i/o management is hidden from the user.  (Taking into account gavin's concern earlier, we may initialize an instance of the JSONStore with a particular path, but that's about it.)
# The API is immediately available, even if the data or the files are not.  While the implementation is initializing, the API allows queuing up the various callbacks.
# The API is patterned after the JavaScript Map API (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map ) with an extra argument for namespaces.  The .get, .set, and .has API's are effectively the same.  Deleting a value is the same as calling .set with the value undefined.
# Looking up multiple properties at once is easy with the .getMultiple, .setMultiple, and .hasMultiple methods.  You pass in one callback instead of a sequence of nested callbacks.  (In the Promises/Task world, this would be one API call instead of several yield statements.)
# The API allows for migrating from one version of the data format to a newer one.  This is why the API has a migrate() method, and why the namespace.json files have a version field.  The migrate() method is present for this reason, even if unimplemented:  it is a reserved method name for updates.

Comment 5

[Techlive Zheng]

This is a step torwards into standardizing how Add-on stores its data, and I like it. Currently, add-on use all kinds of method store its data, this situation should be changed by providing a sdandard API like this one, and add-on should avoid use the internal high-level API directly as much as possible.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

(In reply to Alex Vincent [:WeirdAl] from comment #4)
> When I wrote this API, I did not consider the possibility of the API
> returning Promise objects, instead relying on the old callback model.  That
> is something to reconsider.

Sure, but that's not the immediate concern.

> That said, I believe there are several advantages to the new API:

I'll play the Devil's advocate.

> # All the file i/o management is hidden from the user.  (Taking into account
> gavin's concern earlier, we may initialize an instance of the JSONStore with
> a particular path, but that's about it.)

I see a drawback to this (harder to optimize). What is the advantage?

> # The API is immediately available, even if the data or the files are not. 
> While the implementation is initializing, the API allows queuing up the
> various callbacks.

I'm not sure I follow.

> # The API is patterned after the JavaScript Map API
> (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Map ) with an extra argument for namespaces.  The .get, .set,
> and .has API's are effectively the same.  Deleting a value is the same as
> calling .set with the value undefined.

A Map-style API might make sense for very large data, if we wish to save each key in a different file, but in other cases, I believe that it complicates things. Even if we have very large data that we wish to save in different files, this is something that should probably be decided by API clients (who know their data) rather than by us (who need to guess).

So, by default, I'd go for just reading/writing JSON.

> # Looking up multiple properties at once is easy with the .getMultiple,
> .setMultiple, and .hasMultiple methods.  You pass in one callback instead of
> a sequence of nested callbacks.  (In the Promises/Task world, this would be
> one API call instead of several yield statements.)

This looks nice but it doesn't strike me as very useful (and not applicable at all if we don't adopt the Map API), especially for a v1.

> # The API allows for migrating from one version of the data format to a
> newer one.  This is why the API has a migrate() method, and why the
> namespace.json files have a version field.  The migrate() method is present
> for this reason, even if unimplemented:  it is a reserved method name for
> updates.

What format are you talking about? Client format or backend format?

Comment 7

[Alex Vincent [:WeirdAl]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #6)
> > # All the file i/o management is hidden from the user.  (Taking into account
> > gavin's concern earlier, we may initialize an instance of the JSONStore with
> > a particular path, but that's about it.)
> 
> I see a drawback to this (harder to optimize). What is the advantage?

Let me put it this way:  file i/o is a pain in the ass to get right.  If you as a chrome or add-on developer want to deal with that pain, go right ahead.  :)  

But a module, supported by Mozilla, which abstracts all that away behind a presumably well-tested API means you don't have to worry about that.  You have a choice between the wild, wild West and a nice apartment building that even has working plumbing.

> > # The API is immediately available, even if the data or the files are not. 
> > While the implementation is initializing, the API allows queuing up the
> > various callbacks.
> 
> I'm not sure I follow.

Once you create a JSONStore object, you can call its methods right away.  JSONStore operates asynchronously, so the responses will come when the implementation has finished reading the files it needs to read.

> > # The API is patterned after the JavaScript Map API
> > (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> > Global_Objects/Map ) with an extra argument for namespaces.  The .get, .set,
> > and .has API's are effectively the same.  Deleting a value is the same as
> > calling .set with the value undefined.
> 
> A Map-style API might make sense for very large data, if we wish to save
> each key in a different file, but in other cases, I believe that it
> complicates things. Even if we have very large data that we wish to save in
> different files, this is something that should probably be decided by API
> clients (who know their data) rather than by us (who need to guess).
> 
> So, by default, I'd go for just reading/writing JSON.

But that's the point:  the API consumer *does not care* how it works, only that it does work, reliably.  Large data or small, it doesn't matter.

What's the difference, fundamentally, between saying:

var x = new Map;
x.set("enabled", true);

and:

var x = new JSONStore(path);
x.set("myNamespace", "enabled", true);

The only differences from the consumer's point of view are the presence of the namespace, and the persistence of the data.

Granted, the user could also say:

var x = new JSONStore(path);
x.set(jsonObject);

But they would have to do so for every single itty-bitty change to jsonObject or its descendant properties.  Unless the chrome developer can schedule all of their changes to happen at once, that's not efficient, especially compared to how chrome developers use preferences today - something we're trying to discourage.

(The implementation tries to make sure that changes are batched together.)

Also, granted, with the path argument the concept of a namespace becomes irrelevant, since an addon can specify as many JSONStore instances as they want.

> > # Looking up multiple properties at once is easy with the .getMultiple,
> > .setMultiple, and .hasMultiple methods.  You pass in one callback instead of
> > a sequence of nested callbacks.  (In the Promises/Task world, this would be
> > one API call instead of several yield statements.)
> 
> This looks nice but it doesn't strike me as very useful (and not applicable
> at all if we don't adopt the Map API), especially for a v1.

I added this from painful experience writing code this way:

Store.get("foo", "prop1", function(p1) {
  Store.get("foo", "prop2", function(p2) {
    Store.get("foo", "prop3", function(p3) {
      goDoSomething(p1, p2, p3);
    });
  });
});

With Tasks, that might look like:

var p1 = yield Store.get("foo", "prop1");
var p2 = yield Store.get("foo", "prop2");
var p3 = yield Store.get("foo", "prop3");
goDoSomething(p1, p2, p3);

Even so, that's three separate calls to the API to do something.  Getters are cheap, but one call is less expensive than three.  Setters?  Especially setters that write to the disk at unknown times??  One call for batching, please.

> > # The API allows for migrating from one version of the data format to a
> > newer one.  This is why the API has a migrate() method, and why the
> > namespace.json files have a version field.  The migrate() method is present
> > for this reason, even if unimplemented:  it is a reserved method name for
> > updates.
> 
> What format are you talking about? Client format or backend format?

The format of the stored files.

---

Like I said earlier, nothing forces an add-on to use an API like this.  But if I as an add-on developer don't have to do file management, I'm perfectly fine trusting a Mozilla-provided API that handles all that for me.  The less work for me, the better.

Dealing with raw JSON files directly, there's so many ways for that to go wrong.  I could use nsILocalFile and not do any UTF-8 encoding/decoding, right on the main thread without caring of the poor, blocked user interface.  (A year ago, I would have!)  Either way, my philosophy as an addon developer is "I just want something that works and is easy to use, dammit, and don't make me worry about the details."

---

I don't know what else I can write to convince you that an API like this is a good idea.  I realize you're playing devil's advocate, and reasonably so:  impractical API should be resisted.  I just believe that this API is practical enough to chrome and addon developers to warrant its inclusion.  

Heck, we might even start replacing preferences itself with something like this.  (But that's just me being really ambitious.)

Comment 8

[Alex Vincent [:WeirdAl]]

(In reply to Alex Vincent [:WeirdAl] from comment #7)
> Granted, the user could also say:
> 
> var x = new JSONStore(path);
> x.set(jsonObject);

One last little comment about this:  if the user calls x.set(jsonObject) with two new jsonObject values, each having a different property change, there's no way to merge the changes together without calling .get first.

Comment 9

[Alex Vincent [:WeirdAl]]

Does anyone still want this?

Comment 10

[Kris Maglione [:kmag]]

Nope. We have JSONFile and IndexedDB for internal code, and WebExtensions have IndexedDB and the storage API.