1000829 - getDataStores() should be able to retrieve data stores by owner (comment #19)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Gene Lian (I already quit Mozilla)]

|name| sounds like the identification of a data store, which is supposed to be unique. Since we can have multiple data store share the same type (e.g., both the types of Facebook messages DS and built-in SMS messages DS can be "contacts"), |type| should be more accurate to describe the kind of data store.

Hope to align this to the W3C version at: http://airpingu.github.io/data-store-api/index.html

Comment 1

[Gene Lian (I already quit Mozilla)]

Attachment: Part 1, s/name/type in Gecko

For backward compatibility:

Part-1 patch supports both |name| and |type| on the Gecko side.
Part-2 patch will migrate |name| to |type| on the Gaia side.
Part-3 patch will then completely deprecate |name| on the Gecko side.

Comment 2

[Gene Lian (I already quit Mozilla)]

Try for the part-1 patch: https://tbpl.mozilla.org/?tree=Try&rev=6db7b0df047e

Comment 3

[Gene Lian (I already quit Mozilla)]

After looking into how our Gaia is using the data store, I feel it's not really worth renaming the "name" to "type". For example, in the gaia/apps/communications/manifest.webapp:

  "datastores-owned": {
    "Gaia_Facebook_Friends": {
      "description": "Imported Facebook Friends"
    }
  },

"Gaia_Facebook_Friends" sounds like a unique data store name. If |type| is a better naming, the manifest is supposed to categorize the data store into "contacts".

In the original design, |navigator.getDataStores(...)| can return an array of data stores that share the same type so it's designed as |getDataStore"s"(type)| instead of |getDataStore(name)| (which returns a unique data store only).

Hi Ehsan, what do you think? It's just a minor renaming issue, though, but I hope to clarify the best way to follow. Maybe, it's not really necessary to rename it and we should keep using |name| on the W3C proposal. Anyway, the W3C progress is postponed so either works for me.

Looping Jonas and Jose as well to see if they have different thoughts.

Comment 4

[Jose Manuel Cantera]

We can have something like

"datastores-owned": {
   "Gaia_Facebook_Friends": {
     "description": "Imported Facebook Friends"
     "type": "contacts"
   }
}

so the navigator.getDataStores method could return all the Datastores that match the string passed as parameter either by name or by type

What do you think?

Comment 5

[Gene Lian (I already quit Mozilla)]

Thanks Jose for the suggestion.

I'd like to clarify my thoughts about |getDataStores(type)| to make sure we're on the same page. I think one of the advantage is when you have multiple data stores which are all related to "contacts":

  - Facebook contacts
  - Linked-In contacts
  - SIM contacts
  - Built-in contacts
  - ... etc

when the app calls getDataStores("contacts"), it will return the app all the contacts-related data stores and pop up the prompt *just one time* to ask for the permission:

  "Foo App wants to access your Facebook App, Linked-In App and Contacts App to retrieve contacts data. Do you allow?"

If that's what the |getDataStores(type)| was designed for in the first place, then our current Gaia Facebook app already claimed its manifest in a wrong way. It's supposed to claim:

  "datastores-owned": {
     "contacts": {
       "description": "Imported Facebook Friends"
     }
  }

and the keyword "contacts" has to be formally defined somewhere in our API document because it's now a protocol to the public.

Please correct me if I'm wrong. Thanks!


-----
Back to Jose's comment #4, I think it's a good idea for some reasons:

1. The entry name has to be unique under the "datastores-[owned,access]" object. If we use type as the entry, the app cannot own/access multiple data stores which share the same type:

  "datastores-owned": {
     "contacts": {
       "description": "Imported Facebook Friends"
       "name": "Gaia_Facebook_Friends"
     },
     "contacts": { // This is impossible!
       "description": "Imported Linked-In Friends"
       "name": "Gaia_LinkedIn_Friends"
     }
  }

2. It's good to distinguish the data stores based on the unique name when they have different schema. For example, the Facebook contacts and the Linked-in contacts might have different DB schema. Using |getDataStores("contacts")| can retrieve these two data stores, but the app still needs a way to further distinguish them to have different app-specific behaviours (i.e. by name). For example:

  navigator.getDataStores("contacts").then(function(stores) {
    stores.forEach(function(store){
      switch (store.name) {
        case "Gaia_Facebook_Friends":
          // Some logic that can only handle the Facebook-specific contacts fields...
          break;
        case "Gaia_LinkedIn_Friends":
          // some logic that can only handle the LinkedIn-specific contacts fields...
          break;
      }
    });
  });

3. Also, I think it's good to support the capabilities of retrieving the data stores either by name or type, especially when the app only wants to access and manage one specific data store. For example, a Third-Party Facebook app can just call |getDataStores("Gaia_Facebook_Friends")| to get the specific data store without wasting platform's resources to get other contacts-related data stores that the app doesn't care about. So I'd suggest making the API to:

  Dictionary GetDataStoresOptions {
    DOMString type;
    DOMString name;
  };

  typedef (DOMString or GetDataStoresOptions) GetDataStoresParameter;

  partial interface Navigator {
    Promise getDataStores(GetDataStoresParameter parameter);
  };

If the |parameter| is passed as a DOMString, it will be considered as retrieving the data store by |name|, which is backward-compatible with our current Gaia codes which have been misunderstanding getDataStores(...) as retrieving by name. Or we don't really make it backward-compatible since this API still works for certified apps. Just redesign everything to:

   Dictionary GetDataStoresOptions {
    DOMString type;
    DOMString name;
  };

  partial interface Navigator {
    Promise getDataStores(GetDataStoresOptions options);
  };

which looks more clear.

Comment 6

[Jose Manuel Cantera]

>> If that's what the |getDataStores(type)| was designed for in the first place, then our current Gaia >> Facebook app already claimed its manifest in a wrong way. It's supposed to claim:

>>  "datastores-owned": {
>>     "contacts": {
>>       "description": "Imported Facebook Friends"
>>     }
>>  }

As you have said in point 1.,  We need to use the 'name' as key on the 'datastores-owned' object as if the app owned more than one 'contacts' datastore it won't be possible to declare them. So I think it will be wise to have something like

"datastores-owned": {
   "Gaia_Facebook_Friends": {
     "description": "Imported Facebook Friends"
     "type": "contacts"
   }
}

Concerning point 2. and point 3. I totally agree and I think that's the way to go. 

Thanks Gene

Comment 7

[(no longer active)]

The choice of which data stores provided by which apps an application gains access to is not reflected in the API.  What we currently have allows you to either provide multiple "contacts" DataStores if they are all supposed to contain data with a similar format and meaning, or provide a "google-contacts" and a separate "facebook-contacts" if the two data stores need to be separate for some reason.  With the example of the FB friends, AFAIK the goal is to have that DataStore be completely separate from all other contacts data stores, hence the different name.

It's not clear to me what problems "type" is intended to solve that "name" cannot solve already.  Having them both with almost the same meaning sounds worse than what we have to me.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Wait, why do we need both a name *and* a type? What use-case are we trying to solve here?

Comment 9

[Gene Lian (I already quit Mozilla)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #7)
> The choice of which data stores provided by which apps an application gains
> access to is not reflected in the API.  What we currently have allows you to
> either provide multiple "contacts" DataStores if they are all supposed to
> contain data with a similar format and meaning, or provide a
> "google-contacts" and a separate "facebook-contacts" if the two data stores
> need to be separate for some reason.  With the example of the FB friends,
> AFAIK the goal is to have that DataStore be completely separate from all
> other contacts data stores, hence the different name.

OK. I see. So, the "name" already plays the role of representing a certain format of data store. Then the current Gaia codes works in a correct way. :) I think that's something important we have to emphasize on the document so that the app can expect the data stores returned from |getDataStores(name)| must have the same format and can be handled by the same JS codes.

I still have one concern regarding the usage of "name". How to solve the following use case?

  1. The SystemApp/Gecko provides a built-in data store named "contacts".
  2. And a third-party app also owns a data store named "contacts" but it doesn't follow the same format as expected.
  3. When an app calls getDataStores("contacts"), it will get an array of 2 data stores. However, it would fail to manage the data store from #2 whose DB format isn't matched, thus breaking its synchronization.

That is, how can we force any other apps to follow the same DB format if they want to use the same name for their own data stores?

> 
> It's not clear to me what problems "type" is intended to solve that "name"
> cannot solve already.  Having them both with almost the same meaning sounds
> worse than what we have to me.

Yeap, sounds good to me. If "name" was already defined for the purpose of grouping data stores with the same DB format then another "type" will even confuse people. Let's drop the idea of "type".

Comment 10

[Gene Lian (I already quit Mozilla)]

(In reply to Jonas Sicking (:sicking) from comment #8)
> Wait, why do we need both a name *and* a type? What use-case are we trying
> to solve here?

Please see comment #5. In short, we hope to use "name" to identify the data store as well as a certain kind of DB format. And we hope to use "type" to group data stores so that it's easier for an app to call |getDataStores(type)| to get the data stores which works for the same purpose.

For example, the "google-contacts" DS and "facebook-contacts" DS can have the same type "contacts". Supposing there is a third-party app which desires to manage the contacts-related data, it will be convenient to call |getDataStores("contacts")| to get all of them at one time.

Comment 11

[Jose Manuel Cantera]

(In reply to Jonas Sicking (:sicking) from comment #8)
> Wait, why do we need both a name *and* a type? What use-case are we trying
> to solve here?

A use case that illustrates the need of the 'type' attribute is the following:

Imagine that a Twitter app exposes a datastore compatible with mozContacts, named 'contacts'. Imagine another app that wants to get access to that and only that datastore. So, such an app will call navigator.getDataStores('contacts'). As a result it will obtain an array of datastores, but the app will only be interested in one and only one specific datastore. Furthermore, the user will be prompted saying that such an app wants to get access to several datastores, but the reality is that that app only needs the Twitter one. That will be weird and unconsistent. 

Therefore: 

We need to clearly define the semantics of 'name'. Is it 'name' intended to be an identifier of the Datastore or is it 'name' intended to be a "word" that describes the type of data or schema supported by the datastore? 'name' is the former, if we want to properly support use cases as the described above. So, an app may decide to name their datastores as it wishes. Then, it can use the 'type' attribute to flag the fact that the schema supported by the datastore is a certain one and, for instance, compatible with what FirefoxOS define as a contact (mozContact). 

To sum up my point We need to have explicitly two attributes:

A/ 'name' that identifies the datastore and distinguishes it from other datastores
B 'type' that declares the 'schema' supported by such a datastore. Furthermore, it will avoid any kind of name clashes that may happen with the 'name' attribute. For instance, an app may name a datastore as 'contacts' but if it is not labeled as type="contacts", it will not be considered a 'mozContacts' datastore.

Comment 12

[(no longer active)]

(In reply to Gene Lian [:gene] (needinfo? encouraged) from comment #9)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #7)
> > The choice of which data stores provided by which apps an application gains
> > access to is not reflected in the API.  What we currently have allows you to
> > either provide multiple "contacts" DataStores if they are all supposed to
> > contain data with a similar format and meaning, or provide a
> > "google-contacts" and a separate "facebook-contacts" if the two data stores
> > need to be separate for some reason.  With the example of the FB friends,
> > AFAIK the goal is to have that DataStore be completely separate from all
> > other contacts data stores, hence the different name.
> 
> OK. I see. So, the "name" already plays the role of representing a certain
> format of data store. Then the current Gaia codes works in a correct way. :)
> I think that's something important we have to emphasize on the document so
> that the app can expect the data stores returned from |getDataStores(name)|
> must have the same format and can be handled by the same JS codes.
> 
> I still have one concern regarding the usage of "name". How to solve the
> following use case?
> 
>   1. The SystemApp/Gecko provides a built-in data store named "contacts".
>   2. And a third-party app also owns a data store named "contacts" but it
> doesn't follow the same format as expected.
>   3. When an app calls getDataStores("contacts"), it will get an array of 2
> data stores. However, it would fail to manage the data store from #2 whose
> DB format isn't matched, thus breaking its synchronization.
> 
> That is, how can we force any other apps to follow the same DB format if
> they want to use the same name for their own data stores?

We can't force them in any way with the existing API.  Basically you either need to include some kind of a schema in your API and you get some amount of protection against this problem or you don't and the application needs to handle it in its own application logic.  Data Store is built around the second idea.  Note that even if we added a type property as well, we would have the exact same issue still.

Comment 13

[(no longer active)]

(In reply to Jose Manuel Cantera from comment #11)
> (In reply to Jonas Sicking (:sicking) from comment #8)
> > Wait, why do we need both a name *and* a type? What use-case are we trying
> > to solve here?
> 
> A use case that illustrates the need of the 'type' attribute is the
> following:
> 
> Imagine that a Twitter app exposes a datastore compatible with mozContacts,
> named 'contacts'. Imagine another app that wants to get access to that and
> only that datastore. So, such an app will call
> navigator.getDataStores('contacts'). As a result it will obtain an array of
> datastores, but the app will only be interested in one and only one specific
> datastore. Furthermore, the user will be prompted saying that such an app
> wants to get access to several datastores, but the reality is that that app
> only needs the Twitter one. That will be weird and unconsistent. 
> 
> Therefore: 
> 
> We need to clearly define the semantics of 'name'. Is it 'name' intended to
> be an identifier of the Datastore or is it 'name' intended to be a "word"
> that describes the type of data or schema supported by the datastore?

name is currently the latter.

> 'name'
> is the former, if we want to properly support use cases as the described
> above. So, an app may decide to name their datastores as it wishes. Then, it
> can use the 'type' attribute to flag the fact that the schema supported by
> the datastore is a certain one and, for instance, compatible with what
> FirefoxOS define as a contact (mozContact). 
> 
> To sum up my point We need to have explicitly two attributes:
> 
> A/ 'name' that identifies the datastore and distinguishes it from other
> datastores
> B 'type' that declares the 'schema' supported by such a datastore.
> Furthermore, it will avoid any kind of name clashes that may happen with the
> 'name' attribute. For instance, an app may name a datastore as 'contacts'
> but if it is not labeled as type="contacts", it will not be considered a
> 'mozContacts' datastore.

I think one goal of the DataStore API was to allow the application to plug in to the data store no matter which application provides it, so that if you request "contacts", the idea is that in the majority of cases you don't care exactly what source you're getting the data from.

Jonas, do you think we should support the other use case for accessing a specific data store provided by a specific source?

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm fine with supporting reading from a particular app. However the way to do that is to specify a manifestURL along with the datastore-type.

Comment 15

[Jose Manuel Cantera]

(In reply to Jonas Sicking (:sicking) Vacation until July 24 from comment #14)
> I'm fine with supporting reading from a particular app. However the way to
> do that is to specify a manifestURL along with the datastore-type.

Yes, that will work as well

Comment 16

[Gene Lian (I already quit Mozilla)]

So, summarize the previous discussions a bit:

1. We keep the the current "name" but won't support a new "type", because "name" is already aimed to describe the type of data or schema supported by the data store.

2. The current spec already defined a DataStore.owner, which is exactly the manifestURL to help the caller filter out the app sources. So the apps can do something like:

  navigator.getDataStores("contacts").then(function(stores) {
    stores.forEach(function(store){
      switch (store.owner) {
        case "app://foo.gaiamobile.org/manifest.webapp":
          // ...
          break;
        case "app://bar.gaiamobile.org/manifest.webapp":
          // ...
          break;
      }
    });
  });

3. To retrieve the data stores based on the app source when calling getDataStores(...), we need to further support:

  Dictionary GetDataStoresOptions {
    DOMString name;
    DOMString owner;
  };

  typedef (DOMString or GetDataStoresOptions) GetDataStoresParameter;

  partial interface Navigator {
    Promise getDataStores(GetDataStoresParameter parameter);
  };

  If the |parameter| is passed as a DOMString, it will be considered as retrieving the data store by just |name|, which can be backward-compatible with our current Gaia codes.

The item #3 is the only new thing we need to support. Does that sound reasonable? Please correct me if I misunderstood. Thanks!

Comment 17

[Jose Manuel Cantera]

for me it is ok

Comment 18

[(no longer active)]

Making the argument a dictionary makes us have to define the behavior of the API when someone passes {owner:"foo"} and not name, which seems like something that we don't want to support, right?

We can use an optional argument for owner instead.

Comment 19

[Gene Lian (I already quit Mozilla)]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #18)
> Making the argument a dictionary makes us have to define the behavior of the
> API when someone passes {owner:"foo"} and not name, which seems like
> something that we don't want to support, right?
> 
> We can use an optional argument for owner instead.

OK, I see. Thanks for the advice. Then we'll do:

  partial interface Navigator {
    Promise getDataStores(DOMString name, optional DOMString owner);
  };

Comment 20

[(no longer active)]

(In reply to comment #19)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #18)
> > Making the argument a dictionary makes us have to define the behavior of the
> > API when someone passes {owner:"foo"} and not name, which seems like
> > something that we don't want to support, right?
> > 
> > We can use an optional argument for owner instead.
> 
> OK, I see. Thanks for the advice. Then we'll do:
> 
>   partial interface Navigator {
>     Promise getDataStores(DOMString name, optional DOMString owner);
>   };

Looks great!

Comment 21

[Gene Lian (I already quit Mozilla)]

Looks like a suitable task for Sean. Hi Sean, would you? (you know, I have to start clearing my queue gradually...)

Comment 22

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v1

Enhancing getDatastores().

Comment 23

[Andrea Marchesini [:baku]]

Comment on attachment 8458498 [details] [diff] [review]
Patch v1

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

lgtm

::: dom/datastore/DataStoreService.cpp
@@ +360,5 @@
>    if (!apps->Get(data->mAppId, &accessInfo)) {
>      return PL_DHASH_NEXT;
>    }
>  
> +  if (!data->mManifestURL.IsEmpty() &&

I would move this block before the DatStoreInfo* accessInfo = nullptr; Line 358?

Comment 24

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v2

Updating based on Andrea's r+ comments.

Comment 25

[Sean Lin [:seanlin] (inactive from Mar 2016)]

The try run for the latest patch. FYI
https://tbpl.mozilla.org/?tree=Try&rev=1663e2936dac

Please note that XPCShell tests were failed due to the known issus of bug 1045421.

Comment 26

[Carsten Book [:Tomcat]]

sean, this needs a review of a dom peer

remote: WebIDL file dom/webidl/Navigator.webidl altered in changeset 5868e0bd6387 without DOM peer review

remote: Changes to WebIDL files in this repo require review from a DOM peer in the form of r=...

Comment 27

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Comment on attachment 8463845 [details] [diff] [review]
Patch v2

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

Adding DOM peer for super review.

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 8463845 [details] [diff] [review]
Patch v2

Looks great!

Comment 29

[Sean Lin [:seanlin] (inactive from Mar 2016)]

Attachment: Patch v2 (reviewer appended)

Append r=baku sr=jst to the patch message.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9245b2bb1ecb

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9245b2bb1ecb