Closed Bug 1438577 Opened 4 years ago Closed 4 years ago

Add API to fetch bookmarks by GUID prefix

Categories

(Toolkit :: Places, enhancement, P1)

60 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

We're implementing a policy in the policy engine that allow sysadmins to specify bookmarks to be created and maintained by a sysadmin.

In order to avoid re-creating them on every startup, and to allow removing bookmarks that were removed from the list, the idea is as follows:

Create these bookmarks with a guid like:  "policy-bookmark-" + random()

Then, on next startup, we search for all guids on that format, and compare to the list specified by the sysadmin to find any changes that must be made (i.e., new insertions and removals).

This is better than removing and recreating them all the time as it allows the user to still customize and/or move them (we don't need to block these operations).

But in order to do that, we need this API in Places to support searching for a guid pattern
Hi Felipe! I have a few Sync-related questions about this. :-) Should these bookmarks, or Places policies in general, be synced? What happens if the user moves a bookmark created by a policy, or syncs it to a different device that doesn't have the policy (work to home computer, for instance, or desktop to mobile)? Should we leave that bookmark alone if the policy list changes?
Flags: needinfo?(felipc)
Just a note, guids must be 12 chars, and not all chars can be used.

Regarding Sync, imho it may not be that critical, these policies are for companies, it's less likely that the user syncs with his own account and even if he does, it's still syncing with the company, so those bookmarks may still be useful. Imo we should just sync as normal bookmarks.
Ideally, I think we should make these bookmarks not sync.. Would that be a matter of just adding a new source to the Bookmarks API? ("SOURCE_POLICY"?)..

However, I don't think it's a big deal if we don't do any special handling. There are valid use cases as Marco said. And also, there will be a policy to disable sync, so the admin can block it if it's really necessary for them.
Flags: needinfo?(felipc)
FWIW, I managed to implement what I needed in bug 1428924 without this, by adding some more parameters support to PlacesUtils.bookmarks.search. But I imagine that's a less efficient version of what we can do there.
(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> Ideally, I think we should make these bookmarks not sync.. Would that be a
> matter of just adding a new source to the Bookmarks API? ("SOURCE_POLICY"?)..

No, unfortunately, it would be more complicated than that. Today, everything under the four user content roots is synced everywhere; we don't have a notion of specific  bookmarks that shouldn't sync.

> However, I don't think it's a big deal if we don't do any special handling.

This is simpler, though, and makes sense. Thanks, and sorry for the drive-by comment!
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Summary: Add API to fetch GUIDs by pattern matching → Add API to fetch bookmarks by GUID prefix
Marco, is this what you had in mind?

The actual SQL query I basically copied it from the fetchBookmark function. I don't know the way I did it here is the right way to bind a variable to the LIKE query (by appending the "%" char).
Comment on attachment 8952210 [details]
Bug 1438577 - Add API to fetch bookmarks by GUID prefix.

https://reviewboard.mozilla.org/r/221464/#review227388

I had something more convoluted in mind (basically reusing the guid property with a wildcard), but in the end this is probably better and more maintainable. Very good job, I was planning to look into this in these days, but you anticipated me, I won't complain! Thanks.

::: toolkit/components/places/Bookmarks.jsm:940
(Diff revision 1)
>     *  - url
>     *      retrieves the most recent bookmark having the given URL.
>     *      To retrieve ALL of the bookmarks for that URL, you must pass in an
>     *      onResult callback, that will be invoked once for each found bookmark.
> +   *  - guidPrefix
> +   *      retrieves one item with the specified guid prefix.

one random one or the most recent? it should probably return the most recent one.

::: toolkit/components/places/Bookmarks.jsm:1775
(Diff revision 1)
> +  let query = async function(db) {
> +    let rows = await db.executeCached(
> +      `SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index',
> +              b.dateAdded, b.lastModified, b.type, IFNULL(b.title, "") AS title,
> +              h.url AS url, b.id AS _id, b.parent AS _parentId,
> +              (SELECT count(*) FROM moz_bookmarks WHERE parent = b.id) AS _childCount,

you can NULL AS _childCount for this specific case, we don't use that info, so no reason to fetch it.

::: toolkit/components/places/Bookmarks.jsm:1780
(Diff revision 1)
> +              (SELECT count(*) FROM moz_bookmarks WHERE parent = b.id) AS _childCount,
> +              p.parent AS _grandParentId, b.syncStatus AS _syncStatus
> +       FROM moz_bookmarks b
> +       LEFT JOIN moz_bookmarks p ON p.id = b.parent
> +       LEFT JOIN moz_places h ON h.id = b.fk
> +       WHERE b.guid LIKE :guidPrefix

add an ORDER BY b.lastModified DESC

::: toolkit/components/places/PlacesUtils.jsm:344
(Diff revision 1)
> +   *
> +   * @param guidPrefix: (String)
> +   * @return (Boolean)
> +   */
> +   isValidGuidPrefix(guidPrefix) {
> +    return typeof guidPrefix == "string" && guidPrefix &&

indentation is off here

::: toolkit/components/places/tests/bookmarks/test_bookmarks_fetch.js:204
(Diff revision 1)
> +  }
> +
> +  const PREFIX = "PREFIX-";
> +
> +  let bm1 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
> +                                                 type: PlacesUtils.bookmarks.TYPE_BOOKMARK,

type is optional when an url is specified

::: toolkit/components/places/tests/bookmarks/test_bookmarks_fetch.js:212
(Diff revision 1)
> +                                                 title: "bookmark 1" });
> +  checkBookmarkObject(bm1);
> +  Assert.ok(bm1.guid.startsWith(PREFIX));
> +
> +  let bm2 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
> +                                                 type: PlacesUtils.bookmarks.TYPE_BOOKMARK,

ditto

::: toolkit/components/places/tests/bookmarks/test_bookmarks_fetch.js:228
(Diff revision 1)
> +  checkBookmarkObject(bm3);
> +  Assert.ok(bm3.guid.startsWith(PREFIX));
> +
> +  // Bookmark 4 doesn't have the same guid prefix, so it shouldn't be returned in the results.
> +  let bm4 = await PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
> +                                                 type: PlacesUtils.bookmarks.TYPE_BOOKMARK,

ditto

::: toolkit/components/places/tests/bookmarks/test_bookmarks_fetch.js:242
(Diff revision 1)
> +
> +  Assert.deepEqual(bm1, gAccumulator.results[0]);
> +  Assert.deepEqual(bm2, gAccumulator.results[1]);
> +  Assert.deepEqual(bm3, gAccumulator.results[2]);
> +
> +  await PlacesUtils.bookmarks.remove(bm1.guid);

just pass the bookmark object, you don't have to extract the guid
Attachment #8952210 - Flags: review?(mak77) → review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b327b9ee01a6
Add API to fetch bookmarks by GUID prefix. r=mak
https://hg.mozilla.org/mozilla-central/rev/b327b9ee01a6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.