1380835 - Don't flag queries that reference their containing folder and `excludeQueries` as cycles

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Lina Butler [:lina]]

About Sync shows I have a cycle through "Recently Bookmarked", which is a query in the menu with the URL `place:folder=BOOKMARKS_MENU&folder=UNFILED_BOOKMARKS&folder=TOOLBAR&queryType=1&sort=12&maxResults=10&excludeQueries=1`.

IIUC, `excludeQueries` means the query will exclude itself from the result, so there's no cycle.

Comment 1

[Lina Butler [:lina]]

(It should also probably include folder=MOBILE_BOOKMARKS, but that's another bug).

Comment 2

[Lina Butler [:lina]]

Attachment: Bug 1380835 - Don't check queries with `excludeQueries` for cycles in the bookmark validator.

Review commit: https://reviewboard.mozilla.org/r/157244/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/157244/

Comment 3

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8886459 [details]
Bug 1380835 - Don't check queries with `excludeQueries` for cycles in the bookmark validator.

https://reviewboard.mozilla.org/r/157244/#review162526

One minor concern but other than that, makes sense to me.

::: services/sync/modules/bookmark_validator.js:255
(Diff revision 1)
>      for (let entry of recordsByQueryId.values()) {
>        if (entry.type !== "query" && (!entry.bmkUri || !entry.bmkUri.startsWith(QUERY_PROTOCOL))) {
>          continue;
>        }
>        let params = new URLSearchParams(entry.bmkUri.slice(QUERY_PROTOCOL.length));
> +      if (params.get("excludeQueries") === "1") {

Does the equality check here match the other uses of excludeQueries (e.g. the code that actually filters them?)

Comment 4

[Lina Butler [:lina]]

Comment on attachment 8886459 [details]
Bug 1380835 - Don't check queries with `excludeQueries` for cycles in the bookmark validator.

https://reviewboard.mozilla.org/r/157244/#review162526

> Does the equality check here match the other uses of excludeQueries (e.g. the code that actually filters them?)

I added `"true"` as well, to match `nsNavHistoryQuery::ParseQueryBooleanString`. `AppendBoolKeyValueIfTrue` will always serialize true values as `=1`, but it's good to be thorough.

Comment 5

[Lina Butler [:lina]]

Comment on attachment 8886459 [details]
Bug 1380835 - Don't check queries with `excludeQueries` for cycles in the bookmark validator.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/157244/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e9158194b483
Don't check queries with `excludeQueries` for cycles in the bookmark validator. r=tcsc

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e9158194b483