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

RESOLVED FIXED in Firefox 56

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lina, Assigned: lina)

Tracking

unspecified
Firefox 56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

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.
(It should also probably include folder=MOBILE_BOOKMARKS, but that's another bug).
Assignee: nobody → kit
Status: NEW → ASSIGNED
Priority: -- → P1
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?)
Attachment #8886459 - Flags: review?(tchiovoloni) → review+
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.
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e9158194b483
Don't check queries with `excludeQueries` for cycles in the bookmark validator. r=tcsc
https://hg.mozilla.org/mozilla-central/rev/e9158194b483
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
You need to log in before you can comment on or make changes to this bug.