1202382 - Read-only Bookmarks data adapter

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Sync, defect)

Description

[Michiel de Jong [:michielbdejong]]

The bookmarks data items on the Sync server usually have base-64 id's, but they can also have one of the special id's "menu" or "toolbar" (are there more?).
Their payload contains string fields like 'URL' and 'name'.

The bookmarks data items in the DataStore are really simple, they have two string fields: 'url', and 'name' (where 'url' should be a valid URL).

So it looks very doable to create a Bookmarks data adapter once the History data adapter (bug 1191773) has landed.

Comment 1

[Michiel de Jong [:michielbdejong]]

Sorry, the data fields in the payload from the Sync server are bmkUri and title. You can see the bookmarks coming in if you run the sync app from my michielbdejong/1200539-sync-app-integration-tests branch.

Comment 3

[Fernando Jiménez Moreno [:ferjm]]

Sean, I believe you are working on this. So I am assigning the bug to you.

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Attachment: WIP patch: Bookmarks Adapter

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Attachment: example_bookmarks_2.json

Hey Michiel,

I got these error messages in SyncResults when retrieving(collection.sync()) the records from Bookmarks Collection.
    "Invalid UUID: places"
    "Invalid UUID: toolbar"
    "Invalid UUID: menu"
    "Invalid UUID: unfiled"

The 4 errors exactly have the same property that the type is "folder" in the JSON file dumped from decoding payload, so I use this workaround to validate the ID in 2 to 12 characters:
https://github.com/weilonge/gaia/commit/7c774ffc21307b490c7abc6eb1c88a4d48471dc1

Shall we provide a way for adapters to customize the validate function?

Thanks for your comments. :)

Comment 6

[Michiel de Jong [:michielbdejong]]

(In reply to Sean Lee [:seanlee] from comment #5)
> Created attachment 8665871 [details]
> example_bookmarks_2.json
> 
> Hey Michiel,
> 
> I got these error messages in SyncResults when retrieving(collection.sync())
> the records from Bookmarks Collection.
>     "Invalid UUID: places"
>     "Invalid UUID: toolbar"
>     "Invalid UUID: menu"
>     "Invalid UUID: unfiled"
> 
> The 4 errors exactly have the same property that the type is "folder" in the
> JSON file dumped from decoding payload, so I use this workaround to validate
> the ID in 2 to 12 characters:
> https://github.com/weilonge/gaia/commit/
> 7c774ffc21307b490c7abc6eb1c88a4d48471dc1
> 
> Shall we provide a way for adapters to customize the validate function?
> 
> Thanks for your comments. :)

Yes, this is implemented in https://github.com/michielbdejong/gaia/commit/ada4fc02c015134432f0c6c1ddeeb1fb5b2eb688

Which version are you using?

Comment 7

[Sean Lee [:seanlee][:weilonge]]

I use the latest version in master branch.
Is this patch landed to master?

Comment 8

[Sean Lee [:seanlee][:weilonge]]

The patch works! Thank you, Michiel. :)

Comment 9

[Michiel de Jong [:michielbdejong]]

> I use the latest version in master branch.

A lot of things are not in master yet, that's why it's better to test with my integration branch, but I realize this is a bit of a messy situation, so sorry about that. Should be resolved by next week.

Anyway, if it works for you in master, then that's solved. :)

Comment 10

[Sean Lee [:seanlee][:weilonge]]

Hi Fernando, Michiel,

"bookmarks_store" uses URL as DataStore ID (key), but there are some types in FxSync Bookmarks without a URL (type: 'folder', 'separator') or a general URL (type: 'query').
I would like to use FxSync ID (Syncto ID) as the ID for 'firefox-sync-bookmarks'. When deleting deleted: true records coming from FxSync, we don't have to look up the ID for datastore (in history adapter, it uses asyncStorage). It is also easier to build a folder tree by FxSync ID.

Could you give me some suggestions here? Thank you.

[1] https://docs.services.mozilla.com/sync/objectformats.html#bookmarks

Comment 11

[Michiel de Jong [:michielbdejong]]

(In reply to Sean Lee [:seanlee] from comment #10)
> Hi Fernando, Michiel,
> 
> "bookmarks_store" uses URL as DataStore ID (key), but there are some types
> in FxSync Bookmarks without a URL (type: 'folder', 'separator') or a general
> URL (type: 'query').
> I would like to use FxSync ID (Syncto ID) as the ID for
> 'firefox-sync-bookmarks'. When deleting deleted: true records coming from
> FxSync, we don't have to look up the ID for datastore (in history adapter,
> it uses asyncStorage). It is also easier to build a folder tree by FxSync ID.
> 
> Could you give me some suggestions here? Thank you.
> 
> [1] https://docs.services.mozilla.com/sync/objectformats.html#bookmarks

There's probably a reason why the URL is used as the id? I think it would be easier to keep it that way, and for bookmark-folders (which don't themselves have a URL), maybe you can add a special string to act as their "URL", for instance instead of "http://whatever.com/", their URL field would be "folder:whatever"

Comment 12

[Sean Lee [:seanlee][:weilonge]]

(In reply to Michiel de Jong [:michielbdejong] from comment #11)
> There's probably a reason why the URL is used as the id? I think it would be
If you try to add a bookmark with the same URL in another bookmark record in Firefox browser, the browser will let you "edit" the existing one rather than adding a new one. I think that's probably one of the reasons.

> easier to keep it that way, and for bookmark-folders (which don't themselves
> have a URL), maybe you can add a special string to act as their "URL", for
> instance instead of "http://whatever.com/", their URL field would be
> "folder:whatever"

I ever consider this way, and my idea is like this:
ID for DataStore := [Record.Type]::[Record.URL or Record.FxSyncID]

For separator or folder without URL, I will use FxSyncID as it's unique ID.

How do you think about this? :)

Comment 13

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Sean Lee [:seanlee] from comment #12)
> (In reply to Michiel de Jong [:michielbdejong] from comment #11)
> > There's probably a reason why the URL is used as the id? I think it would be
> If you try to add a bookmark with the same URL in another bookmark record in
> Firefox browser, the browser will let you "edit" the existing one rather
> than adding a new one. I think that's probably one of the reasons.
> 

Also note that the browser needs an easy and fast way to display the bookmark icon (the star) in the awesome bar when the user types an URL that is stored as a bookmark. If we have the FxSyncID as the id of firefox_sync_bookmarks we'll need to iterate over the whole DS or have a local IDB with the bookmarks indexed by URL.

Given that DataStore doesn't allow indexes, we need records stored in the firefox_sync_bookmarks DS to have the URL as its key, just like the bookmarks_store does.

> > easier to keep it that way, and for bookmark-folders (which don't themselves
> > have a URL), maybe you can add a special string to act as their "URL", for
> > instance instead of "http://whatever.com/", their URL field would be
> > "folder:whatever"
> 
> I ever consider this way, and my idea is like this:
> ID for DataStore := [Record.Type]::[Record.URL or Record.FxSyncID]
> 
> For separator or folder without URL, I will use FxSyncID as it's unique ID.
> 
> How do you think about this? :)

I'd go with Michiel's proposal.

For regular records, (only) the URL should be the key, so we can easily do ds.get(url).

For folder or separator records, keys can be folder|separator:whatever or even the fxsyncID as you suggest, it doesn't really matter that much as long as keys are unique.

You'll also need to store the fxSyncID separately on each record (even if you use if for the key of folders or separators).

Comment 14

[Michiel de Jong [:michielbdejong]]

(In reply to Sean Lee [:seanlee] from comment #5)
> I got these error messages in SyncResults when retrieving(collection.sync())
> the records from Bookmarks Collection.
>     "Invalid UUID: places"
>     "Invalid UUID: toolbar"
>     "Invalid UUID: menu"
>     "Invalid UUID: unfiled"

You mean 'mobile', not 'places', right?

Comment 15

[Sean Lee [:seanlee][:weilonge]]

(In reply to Michiel de Jong [:michielbdejong] from comment #14)
> (In reply to Sean Lee [:seanlee] from comment #5)
> > I got these error messages in SyncResults when retrieving(collection.sync())
> > the records from Bookmarks Collection.
> >     "Invalid UUID: places"
> >     "Invalid UUID: toolbar"
> >     "Invalid UUID: menu"
> >     "Invalid UUID: unfiled"
> 
> You mean 'mobile', not 'places', right?

'places' is one of folder in my test bookmarks.

Comment 16

[Sean Lee [:seanlee][:weilonge]]

Here is the specialRecords that I am using:
  specialRecords: [
    'menu',
    'mobile',
    'toolbar',
    'places',
    'unfiled'
  ]

Comment 17

[GitHub Autolander Bot]

Attachment: [gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master

Comment 18

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8667832 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master

Hi Fernando, Michiel,

Bookmarks adapter is implemented in the similar login with History adapter.
Could you help to review the patch?

Thank you! :)

Comment 19

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8667832 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master

Nice work Sean! I left a few comments in the PR. The code looks good to me but there are some small things that needs to be sorted out. Also, I am missing a few test cases.
Thank you!

Comment 20

[Sean Lee [:seanlee][:weilonge]]

Hi Vivien,

I am trying to build a datastore for FxSync Bookmarks.
When I am considering the compatibility to bookmarks_store, could you give a hint that how many types would be in bookmarks_store? I suppose that 'url' is the only one.
 https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.js#L1043

Thank you! :)

Comment 21

[Michiel de Jong [:michielbdejong]]

Comment on attachment 8667832 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master

Great! Left some feedback on github. My main concern is why IIUC we're now using two different bookmark stores, 'firefox-sync-bookmarks' and 'bookmarks_store'.

Comment 22

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8667832 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master

Hey Fernando,

I have updated the patch based on your and Michiel's comment.
Could you help to review it again?

Thank you!


Hey Michiel,

Thank you for giving the feedback. :)

Comment 23

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8667832 [details] [review]
[gaia] weilonge:seanlee/DataSync/master/Bug1202382 > mozilla-b2g:master

Thank you Sean! r=me with the comments addressed, please.

Comment 24

[Sean Lee [:seanlee][:weilonge]]

landed on master: https://github.com/mozilla-b2g/gaia/commit/5e10cdeb6c9ebe0312c0283a894c227f79edcb1c
gaia test: https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=b7cbb6b202eaac5875c70dc4f682bc7884553c9b

Thank you, Fernando! :)