Remove the `RESULTS_AS_TAG_CONTENTS` query type

RESOLVED FIXED in Firefox 61

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: lina, Assigned: mak)

Tracking

(Blocks 1 bug)

unspecified
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(5 attachments, 4 obsolete attachments)

"RESULTS_AS_TAG_CONTENTS" queries encode the tag folder ID in the "place:" URL, which means Sync needs to rewrite remote tag query URLs to point to the local tag folder.

In bug 1274108, comment 59, Mak suggested changing the URL to a form like "place:tags=name" and removing RESULTS_AS_TAG_CONTENTS.
Assignee

Comment 1

3 years ago
we should already have support for queries having tags=list, iirc we introduced those some years ago but we never migrated the built-in queries to use those.
http://searchfox.org/mozilla-central/rev/bb22cc4067c3832b943507497389b0b13d6f3a2b/toolkit/components/places/nsINavHistoryService.idl#973
Assignee

Updated

3 years ago
Priority: -- → P3
Assignee

Updated

2 years ago
Blocks: 424160
I'm sorry there's been no update for a while from my side, I was caught up. I have started writing a migration which converts place:type=7&folder=[id] queries to place:tag=[tagname] type of queries. I will try to complete this by this weekend maybe with some tests for it. I'll assign myself as soon as I get a first patch in a workable state.
Assignee

Updated

2 years ago
Blocks: 1373821

Updated

2 years ago
Assignee: nobody → i.milind.luthra
Sorry for the delay!

This patch is the migration code for the Database moving all relevant places queries to switch from folder IDs to tags. Also attached is a basic demonstrative test and a places_v41.sqlite.

Some notes:

1. While adding entries in the test, I'm not sure what to do about the URL hashes in the moz_places table. One of the other tests (I think current from v34) says that it should be fixed up automatically. In any case, this causes the test to fail because of an assertion failure in Database.cpp even though all subtests are passing.

2. This is only the migration code; this does not look at take care of the many issues outlined in (https://bugzilla.mozilla.org/show_bug.cgi?id=1039200#c52, https://bugzilla.mozilla.org/show_bug.cgi?id=1039200#c53 etc). I felt that the patch was big enough that I want to make sure that I am moving in the right direction before proceeding.

Thanks!
Comment hidden (mozreview-request)
Assignee

Comment 7

2 years ago
Hi Milind, no worries for the delay, I'll try to have a first look at your patch today.
The Sync problem is something we can surely handle later, though we could even not handle it at all, mostly because the default tags queries (the library) are not handled by Sync and I expect only a tiny fraction of users may have copied those around, and they are more than able to copy them again if the query should break.
Assignee

Comment 8

2 years ago
mozreview-review
Comment on attachment 8920263 [details]
Bug 1293445 - Part 1: Add migration removing `RESULTS_AS_TAG_CONTENTS` from places.sqlite and associated tests.

https://reviewboard.mozilla.org/r/191270/#review197658

Ok, some first feedback.
I was thinking, if tags queries work well, we could probably land this before attacking all the rest (PlacesSQLQueryBuilder::SelectAsTag() first, then the other consumers). Along with some Sync handling that we can coordinate with Kit.

::: toolkit/components/places/Database.cpp:1134
(Diff revision 2)
>        }
>  
> -      // Firefox 58 uses schema version 40.
> +      if (currentSchemaVersion < 41) {
> +        rv = MigrateV41Up();
> +        NS_ENSURE_SUCCESS(rv, rv);
> +      }

Some of the code changed recently, so this may have slightly bitrotted.

::: toolkit/components/places/Database.cpp:2395
(Diff revision 2)
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  nsCOMPtr<mozIStorageStatement> placesQueryStmt;
> +  nsresult rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "SELECT p.id, p.url FROM moz_places p "
> +    "WHERE p.url LIKE \"place:%type=7%\""

This is likely going to be slow, there is no index on url, and regardless LIKE couldn't use an index.

Another minor thing, is that historically we use the "h" alias for moz_places (likely because of _h_istory), not p. Since you only have one table here, the alias should not be needed

The best way is probably
WHERE (url_hash BETWEEN hash("place", "prefix_lo") AND hash("place", "prefix_hi")) AND +url LIKE ...
This will just apply the LIKE to those urls having a "place" scheme.

::: toolkit/components/places/Database.cpp:2401
(Diff revision 2)
> +  ), getter_AddRefs(placesQueryStmt));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  bool hasMoreQueries = false;
> +  rv = placesQueryStmt->ExecuteStep(&hasMoreQueries);
> +  if (!NS_SUCCEEDED(rv) || !hasMoreQueries) {

Looks like you could just have a large
while (NS_SUCCEEDED(placesQueryStmt->ExecuteStep(&hasMoreQueries)) && hasMoreQueries) {
  ... migrate
}

Though, running writes in the middle of a large read may not be the best idea. Thus, I'd suggest to just run this while and accumulate ids and urls in a structure. At the end of the while, loop the structure. The 2 loops will practically be no-op if there's nothing to convert.

Since there is likely very few of these entries to convert, you can probably avoid fetching all of the tags. For each entry to convert I'd just extract the folder if, query the tag title, lowercase it and write the new url.

::: toolkit/components/places/Database.cpp:2429
(Diff revision 2)
> +  }
> +
> +  nsNavHistory* historyService = nsNavHistory::GetHistoryService();
> +  nsCOMPtr<mozIStorageAsyncStatement> replacementStmt;
> +  rv = mMainConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
> +    "UPDATE moz_places SET url=:new_url WHERE id=:id"

everytime you change the url, you should also set url_hash = hash(:your_new_url)

::: toolkit/components/places/tests/migration/test_current_from_v40.js:4
(Diff revision 2)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +const gQueryData = [

You may add a places_v40.sqlite db (or modify an existing one) with prefilled data, if that helps you with having valid hashes.
Attachment #8920263 - Flags: review?(mak77)
Comment hidden (mozreview-request)
The new patch addresses the comments. 

> Ok, some first feedback.
> I was thinking, if tags queries work well, we could probably land this before attacking all the rest 
> (PlacesSQLQueryBuilder::SelectAsTag() first, then the other consumers). Along with some Sync handling that we can coordinate with   
> Kit.

That would mean not removing RESULTS_AS_TAG_QUERY_CONTENTS type in this bug, but only migrating current queries using that and making sure they work fine? 

Would that cause issues also with the user who adds a `type=7` query in the time between this migration and the time we remove RESULTS_AS_TAG_QUERY_CONTENTS? -- but probably the number of users who do that will be minimal.
Assignee

Comment 11

2 years ago
(In reply to Milind L (:milindl) from comment #10)
> Would that cause issues also with the user who adds a `type=7` query in the
> time between this migration and the time we remove
> RESULTS_AS_TAG_QUERY_CONTENTS? -- but probably the number of users who do
> that will be minimal.

We should surely change SelectAsTag. the most common case for a user to create a tag query is dragging from the Library left pane, that is populated by SelectAsTag. It should populate the Tags root with tags=xxx queries.

We should rewrite queries, as you did.

We must figure out the story with Sync (https://bugzilla.mozilla.org/show_bug.cgi?id=1039200#c53). As Kit said, we could even decide to break synced queries because it's a rare case. We just need an agreement, and the Sync team could help with the related changes.

Another thing to verify is the behavior of tags queries with casing and special characters (must be properly urlencoded and still match), I think the queries should always use all-lowercase tags but match in a case insensitive way. Maybe we should just decide to make tags lowercased by default. This could be verified and fixed even immediately. If we can ensure the new tags queries work fine, it will make our life easier.

Finally, there are a few consumers that may misbehave, nsNavHistory and nsNavHistoryResult.cpp are the 2 most compelling ones, but if the most common operations work out of the box, we can follow-up on these issues, it will mostly be visual problems like views updates and such.

To sum up, what we could do now:
1. check tag queries work in a case insensitive way and with special chars, have tests for that
2. fix selectAsTag
3. your patch here & figure out Sync problems with Kit
What we could do later:
5. fix remaining consumers and remove RESULTS_AS_TAG_QUERY_CONTENTS
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
There are three patches here - 

1. First one is places.sqlite migration, but doesn't do sync work (yet)
2. Second one has tests for case sensitivity/special chars and code to make them work

I suppose there are many ways to do that, I thought of two ways. One is to change the whole SQL query that is made to the database, so that the tag(s) we're looking for are lowercased as well as the tags in the database. This is done using the SQLite lower(). This is what I did.

We can also do this by modifying SetTags and lowercasing all tags in the query, and then while constructing SQL query, we only need to lowercase the tags in the database. Both these methods work with the test attached.

I tried to compare the speeds of the methods by running a places query with hundreds of tags (in a JS test file) and then printing the time of the SQL query using the `duration` in mozStorageConnection (in a debugger). The times were quite close and so I chose the first method which was easier. 

If there's a better way to decide/another method to do this I'll be happy to do that.

Tests were added to test_tags since that already tests for a lot of things.

3. Finally SelectAsTag was fixed, and I added a test for that.

At a glance, the Library/Sidebar seems to work with adding tagged bookmarks, deleting them and dragging them around.

Updated

2 years ago
Status: NEW → ASSIGNED
Assignee

Comment 16

2 years ago
mozreview-review
Comment on attachment 8930035 [details]
Bug 1293445 - Part 2: Make tag queries case insensitive and associated tests.

https://reviewboard.mozilla.org/r/201158/#review209380

This looks great! Could you please split it into a dependency bug, so that we can land it sooner?

::: toolkit/components/places/nsNavHistory.cpp:3277
(Diff revision 1)
> -           Str("AND tags.title IN (");
> +           Str("AND lower(tags.title) IN ( lower(");
>      for (uint32_t i = 0; i < tags.Length(); ++i) {
>        nsPrintfCString param(":tag%d_", i);
>        clause.Param(param.get());
>        if (i < tags.Length() - 1)
> -        clause.Str(",");
> +        clause.Str("), lower(");

nsUnicharUtils includes a toLowerCase() method that we could apply here https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/toolkit/components/places/nsNavHistory.cpp#3459 and avoid the many lower() inside IN()
Attachment #8930035 - Flags: review?(mak77) → review+

Updated

2 years ago
Depends on: 1421654
Assignee

Comment 17

2 years ago
mozreview-review
Comment on attachment 8930036 [details]
Bug 1293445 - Part 3: Fix SelectAsTag and add test.

https://reviewboard.mozilla.org/r/201160/#review209406

Please also split this to a separate dependency bug, it can land sooner.

There are a few manual tests to do in the UI in addition, especially in the Library left pane, you should check that these results update properly when adding/removing tags into bookmarks, that a user can drag these queries to somewhere else (like the toolbar) and both copies keep workind properly, and that their contents are read-only (can't New Bookmark, can't Cut, dragging a bookmark from them creates a copy of the bookmark, doesn't move it)

::: toolkit/components/places/nsNavHistory.cpp:1891
(Diff revision 1)
>    // This allows sorting by date fields what is not possible with
>    // other history queries.
>    mHasDateColumns = true;
>  
>    mQueryString = nsPrintfCString(
> -    "SELECT null, 'place:folder=' || id || '&queryType=%d&type=%d', "
> +    "SELECT null, 'place:tag=' || title || '&queryType=%d', "

I think we should not have an explicit queryType, queryType should be forced when parsing the query and finding a tag keyword.

For example here:
https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/toolkit/components/places/nsNavHistoryQuery.cpp#1366
https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/toolkit/components/places/nsNavHistoryQuery.cpp#1468
https://searchfox.org/mozilla-central/source/toolkit/components/places/nsNavHistoryQuery.cpp#1141
Attachment #8930036 - Flags: review?(mak77)

Updated

2 years ago
Depends on: 1421664

Updated

2 years ago
Attachment #8930036 - Attachment is obsolete: true

Updated

2 years ago
Attachment #8930035 - Attachment is obsolete: true
Blocks: 1433368
No longer blocks: 1433368
Assignee

Updated

a year ago
Assignee: i.milind.luthra → mak77
Priority: P3 → P1
Assignee

Updated

a year ago
Whiteboard: [fxsearch]
Assignee

Updated

a year ago
Attachment #8920263 - Attachment is obsolete: true
Attachment #8920263 - Flags: review?(mak77)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 20

a year ago
Kit, please triple-check my Sync changes, I'm a little bit out of my water there.
Reporter

Comment 21

a year ago
mozreview-review
Comment on attachment 8963978 [details]
Bug 1293445 - 3 - Handle Sync tag queries rewrite.

https://reviewboard.mozilla.org/r/232796/#review238400

This is looking good, and a nice simplification, thanks! Minusing because I think the mirror needs changes.

::: toolkit/components/places/PlacesSyncUtils.jsm:1405
(Diff revision 1)
>      }
>      throw ex;
>    }
>  };
>  
> -// Tag queries use a `place:` URL that refers to the tag folder ID. When we
> +// Legacy tag queries may use a `place:` URL that refers to the tag folder ID.

There's an edge case here, though I'm not sure it's worth worrying about. If you change or create a tag query in Firefox 61, then sync back to Firefox 60, the query won't work. One idea is to rewrite outgoing queries with a fake `folder` and `queryType`...old versions of Firefox will remap the folder ID, new versions will delete `folder` and use `tag`.

But, that adds complexity and tech debt. Given that tag queries are probably not that common, do you think it's fine to let this slide?

::: toolkit/components/places/PlacesSyncUtils.jsm:1426
(Diff revision 1)
>  
> -  // Rewrite the query to reference the new ID.
> -  params.set("folder", id);
> +  // Rewrite the query to directly reference the tag.
> +  params.delete("queryType");
> +  params.delete("type");
> +  params.delete("folder");
> +  params.set("tag", info.folder);

How do we handle queries where the tag doesn't exist? The old bookmarks engine might download and apply the query before it sees new bookmarks with the tag, but I'm guessing the query refresh, and everything will just work when we insert or update the tagged bookmark?

::: toolkit/components/places/PlacesSyncUtils.jsm:2166
(Diff revision 1)
>      item.description = description;
>    }
>  
> -  let folder = null;
>    let params = new URLSearchParams(bookmarkItem.url.pathname);
> -  let tagFolderId = +params.get("folder");
> +  let tags = params.getAll("tag");

Is it possible for a tag query to reference multiple tags? (I guess Sync broke tag queries with multiple tags before if so).

::: toolkit/components/places/SyncedBookmarksMirror.jsm:1669
(Diff revision 1)
>  
>      let tagFolderNameParams = [];
>      for (let row of queryRows) {
>        let url = new URL(row.getResultByName("url"));
> -      let tagQueryParams = new URLSearchParams(url.pathname);
> -      let type = Number(tagQueryParams.get("type"));
> +      let queryParams = new URLSearchParams(url.pathname);
> +      let tagFolderId = Number(queryParams.get("folder"));

This doesn't seem right...I think we want `queryParams.getAll("tag")[0]` here, just like in `placesBookmarkToSyncBookmark`, to fetch the tag name instead of the tag folder ID. `tagFolderNameParams` will need an update, too, and the query below can set them directly instead of the subquery to `SELECT b.title, ...`.

`test_queries` in `test_bookmark_kinds.js` will need an update, too.

::: toolkit/components/places/tests/sync/test_sync_utils.js:1548
(Diff revision 1)
>        url: "place:type=7&folder=90",
> -      folder: "taggy",
> +      folder: "nonexisting",
>        title: "Tagged stuff",
>      });
> -    notEqual(query.url.href, "place:type=7&folder=90",
> -      "Tag query URL for new tag should differ");
> +    let params = new URLSearchParams(query.url.pathname);
> +    equal(params.get("type"), null, "Should not preserve query type");

Miro-nit: `!params.has("type")` is shorter, but it doesn't really matter.
Attachment #8963978 - Flags: review?(kit) → review-
Reporter

Comment 22

a year ago
mozreview-review
Comment on attachment 8963977 [details]
Bug 1293445 - 2 - Migrate RESULTS_AS_TAG_CONTENTS queries to place:tag queries.

https://reviewboard.mozilla.org/r/232794/#review238326

Small nit about sync status, but the URL parsing logic looks a bit off.

::: toolkit/components/places/Database.cpp:2114
(Diff revision 1)
> +  nsresult rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "UPDATE moz_places "
> +       "SET url = 'place:tag=' || ( "
> +          "SELECT title FROM moz_bookmarks "
> +          "WHERE id = substr(url, "
> +                            "instr(url, 'folder=') + 7, "

Hmm, this won't do the right thing if `folder` comes last, or is the only param in the query. Both `place:folder=123` and `place:type=7&folder=123` parse as `=`.

Bug 1431449 added `URLParams::Parse`, it might be worth adding a SQL function like `get_query_param` to extract URL params.

We could also use that in part 2. Instead of fetching and iterating over query rows in the mirror and firing an `UPDATE`, we'd do `get_query_param(substr(url, 1, 6), 'tag')` when we insert into `itemsToUpload`. Ditto for selecting and rewriting legacy tag queries in the mirror.

::: toolkit/components/places/Database.cpp:2134
(Diff revision 1)
> +  NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Update Sync fields for all tag queries.
> +  nsCOMPtr<mozIStorageStatement> updateStmt;
> +  rv = mMainConn->CreateStatement(NS_LITERAL_CSTRING(
> +    "UPDATE moz_bookmarks SET syncStatus = :sync_status, syncChangeCounter = syncChangeCounter + 1 "

No need to reset `syncStatus`; just bumping `syncChangeCounter` is sufficient.
Attachment #8963977 - Flags: review?(kit) → review-
Posted patch get-params-func.patch (obsolete) — Splinter Review
I was kinda curious about how the SQL function might work, so I threw this together quickly. Feel free to use if it helps, or ignore. :-)
Assignee

Comment 24

a year ago
(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #21)
> There's an edge case here, though I'm not sure it's worth worrying about. If
> you change or create a tag query in Firefox 61, then sync back to Firefox
> 60, the query won't work.

Why? "place:tag=something" queries are supported from a lot of versions.

> How do we handle queries where the tag doesn't exist? The old bookmarks
> engine might download and apply the query before it sees new bookmarks with
> the tag, but I'm guessing the query refresh, and everything will just work
> when we insert or update the tagged bookmark?

If the tag doesn't exist, the query will just return an empty set, until something is added with that tag.
 
> ::: toolkit/components/places/PlacesSyncUtils.jsm:2166
> > -  let folder = null;
> >    let params = new URLSearchParams(bookmarkItem.url.pathname);
> > -  let tagFolderId = +params.get("folder");
> > +  let tags = params.getAll("tag");
> 
> Is it possible for a tag query to reference multiple tags? (I guess Sync
> broke tag queries with multiple tags before if so).

Yes, but it's not what's I'd call a "tag query".
Let's clarify. What PlacesUtils names as a TagQuery is a query like "place:tag=something" that is child of a RESULTS_AS_TAG_QUERY container. It's a virtual query that Sync will never see. In practice this only matters for the UI, these queries have nothing special, compared to the old ones.
Tag queries are just queries, and a query can be "place:tag:t1&tag=t2" like "place:tag=t1&terms=something". This is not special, it's a place query like any else.
Since we removed any distinction, Sync should just handle these as any other place query, copy it as-is and move on.
Assignee

Comment 25

a year ago
(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #22)
> Hmm, this won't do the right thing if `folder` comes last, or is the only
> param in the query. Both `place:folder=123` and `place:type=7&folder=123`
> parse as `=`.

True, even if with other conditions that is likely not happening.

> Bug 1431449 added `URLParams::Parse`, it might be worth adding a SQL
> function like `get_query_param` to extract URL params.

This sounds like a good idea, I'll look into your suggested patch for integration.

Comment 26

a year ago
mozreview-review
Comment on attachment 8963978 [details]
Bug 1293445 - 3 - Handle Sync tag queries rewrite.

https://reviewboard.mozilla.org/r/232796/#review238826

In looking at the other patch, I just spotted a couple of comments that reference this bug that need updating or handling.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:1660
(Diff revision 1)
>          feedURLAnno: PlacesUtils.LMANNO_FEEDURI,
>          siteURLAnno: PlacesUtils.LMANNO_SITEURI });
>  
>      // Record tag folder names for tag queries. Parsing query URLs one by one
>      // is inefficient, but queries aren't common today, and we can remove this
>      // logic entirely once bug 1293445 lands.

nit: we need to either update this comment to remove the bug reference, handle it or file a new bug for it.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:2456
(Diff revision 1)
>    // URLs, so two bookmarks with the same URL should have the same tags. Unlike
>    // keywords, one tag may be associated with many different URLs. Tags are also
>    // different because they're implemented as bookmarks under the hood. Each tag
>    // is stored as a folder under the tags root, and tagged URLs are stored as
>    // untitled bookmarks under these folders. This complexity, along with tag
>    // query rewriting, can be removed once bug 1293445 lands.

nit: comment needs updating/handling or new bug.

Comment 27

a year ago
mozreview-review
Comment on attachment 8963977 [details]
Bug 1293445 - 2 - Migrate RESULTS_AS_TAG_CONTENTS queries to place:tag queries.

https://reviewboard.mozilla.org/r/232794/#review238828

The basic migration looks fine, modulo Kit's comments. Though there's a wider question on removing usage of the option completely below.

In either case, I'd still like to see this again after update.

::: commit-message-1a2d6:1
(Diff revision 1)
> +Bug 1293445 - Remove RESULTS_AS_TAG_CONTENTS from Places. r=standard8,kitcambridge

We also need to handle updating controller.js somewhere:

https://searchfox.org/mozilla-central/rev/f5fb323246bf22a3a3b4185882a1c5d8a2c02996/browser/components/places/content/controller.js#814-824

or even actually removing RESULTS_AS_TAG_CONTENTS completely:

https://searchfox.org/mozilla-central/search?q=RESULTS_AS_TAG_CONTENTS&case=false&regexp=false&path=

or file a bug to handle it as follow-up. If you're filing a follow-up, then I suggest we change the check-in title to something like "Migrate old RESULTS_AS_TAG_CONTENTS queries to new tag queries".

::: toolkit/components/places/tests/migration/test_current_from_v45.js:8
(Diff revision 1)
> +
> +"use strict";
> +
> +let gTags = [
> +  { folder: 123456,
> +    url: "place:folder=123456&type=7&queryType=1",

Maybe also include a few different orders to check that we correctly handle the parameters?

::: toolkit/components/places/tests/migration/test_current_from_v45.js:27
(Diff revision 1)
> +  let path = OS.Path.join(OS.Constants.Path.profileDir, DB_FILENAME);
> +  let db = await Sqlite.openConnection({ path });
> +
> +  for (let tag of gTags) {
> +    // We can reuse the same guid, it doesn't matter for this test.
> +    let guid = tag.title.padEnd(12, "_");

I think you could just include the guids in gTags, at the moment, you're recalculating them three times throughout the test.
Attachment #8963977 - Flags: review?(standard8) → review-
Assignee

Comment 28

a year ago
I'm not sure what "test_queries in test_bookmark_kinds.js" is trying to do exactly, type=6 is RESULTS_AS_TAG_QUERY that is the tags query in the left pane folder, not a tag (type=7).
This mirror comment is wrong, and the test seems to go after it:
https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/toolkit/components/places/SyncedBookmarksMirror.jsm#1362

I'll try to get my head around rewriteRemoteTagQueries and this test.
Assignee

Comment 29

a year ago
to avoid future confusion, I think I'll also rename RESULTS_AS_TAG_QUERY to RESULTS_AS_TAGS_ROOT
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Updated

a year ago
Attachment #8964302 - Attachment is obsolete: true
Assignee

Comment 35

a year ago
mozreview-review
Comment on attachment 8964930 [details]
Bug 1293445 - 1 - Add a get_query_param SQL function.

https://reviewboard.mozilla.org/r/233668/#review239272
Attachment #8964930 - Flags: review?(mak77) → review+
Reporter

Comment 36

a year ago
mozreview-review
Comment on attachment 8963978 [details]
Bug 1293445 - 3 - Handle Sync tag queries rewrite.

https://reviewboard.mozilla.org/r/232796/#review239324

A question about if it's worth migrating any of the other params, since (IIUC) migration doesn't, but this looks good either way. Thanks!

::: toolkit/components/places/PlacesSyncUtils.jsm:1408
(Diff revision 2)
>  };
>  
> -// Tag queries use a `place:` URL that refers to the tag folder ID. When we
> -// apply a synced tag query from a remote client, we need to update the URL to
> -// point to the local tag folder.
> +// Legacy tag queries may use a `place:` URL that refers to the tag folder ID.
> +// When we apply a synced tag query from a remote client, we need to update the
> +// URL to point to the local tag.
>  async function updateTagQueryFolder(db, info) {

Nit: I don't think this function needs to be async anymore.

::: toolkit/components/places/SyncedBookmarksMirror.jsm:1366
(Diff revision 2)
> -      JOIN mergeStates r ON r.mergedGuid = v.guid
> -      WHERE r.valueState = :valueState AND
> -            v.tagFolderName NOT NULL`,
> -      { valueState: BookmarkMergeState.TYPE.REMOTE });
> -
>      let queryRows = await this.db.execute(`

Since we ignore other params when we migrate, I wonder if we could do the same here...remove `rewriteRemoteTagQueries` entirely, and have https://searchfox.org/mozilla-central/rev/a0665934fa05158a5a943d4c8b277465910c029c/toolkit/components/places/SyncedBookmarksMirror.jsm#1260-1270 use the same approach as the migration when inserting into `moz_places`. Maybe it's not worth the trouble, though; these seem like they're uncommon enough already.

Same question for `updateTagQueryFolder` in PSU.

::: toolkit/components/places/tests/sync/test_bookmark_kinds.js:283
(Diff revision 2)
>          {
> -          // this entry has a folder= query param for a folder that exists.
> +          // this entry has a tag= query param for a tag that exists.
>            guid: "queryAAAAAAA",
>            type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
>            title: "TAG_QUERY query",
> -          url: `place:type=6&sort=14&maxResults=10&folder=${tagid}`,
> +          url: `place:tag=a-tag&&sort=14&maxResults=10`,

Good catch on `type=6`! Sorry I didn't catch this when reviewing earlier.
Attachment #8963978 - Flags: review?(kit) → review+
Reporter

Comment 37

a year ago
mozreview-review
Comment on attachment 8963977 [details]
Bug 1293445 - 2 - Migrate RESULTS_AS_TAG_CONTENTS queries to place:tag queries.

https://reviewboard.mozilla.org/r/232794/#review239352

LGTM!
Attachment #8963977 - Flags: review?(kit) → review+
(In reply to Marco Bonardo [::mak] from comment #24)
> Yes, but it's not what's I'd call a "tag query".
> Let's clarify. What PlacesUtils names as a TagQuery is a query like
> "place:tag=something" that is child of a RESULTS_AS_TAG_QUERY container.
> It's a virtual query that Sync will never see. In practice this only matters
> for the UI, these queries have nothing special, compared to the old ones.
> Tag queries are just queries, and a query can be "place:tag:t1&tag=t2" like
> "place:tag=t1&terms=something". This is not special, it's a place query like
> any else.
> Since we removed any distinction, Sync should just handle these as any other
> place query, copy it as-is and move on.

Thanks for clarifying, this makes sense! I confused the two, and it didn't click that we're removing the distinction between `RESULTS_AS_TAG_CONTENTS` and `RESULTS_AS_TAGS_ROOT`.
Assignee

Comment 39

a year ago
(In reply to Kit Cambridge (they/them) [:kitcambridge] from comment #36)
> Comment on attachment 8963978 [details]
> Bug 1293445 - 3 - Handle Sync tag queries rewrite.
> 
> https://reviewboard.mozilla.org/r/232796/#review239324
> 
> A question about if it's worth migrating any of the other params, since
> (IIUC) migration doesn't, but this looks good either way. Thanks!

It's indeed not very important, considering it's unlikely to exist.
For migration I decided to just skip further params since it's a one-shot, the user can still refix/recreate the query once.
For Sync I was not sure, since it involves multiple profiles, so I kept the added params. If we think that removing those can largely simplify the code, we could surely re-evaluate.

> ::: toolkit/components/places/SyncedBookmarksMirror.jsm:1366
> (Diff revision 2)
> > -      JOIN mergeStates r ON r.mergedGuid = v.guid
> > -      WHERE r.valueState = :valueState AND
> > -            v.tagFolderName NOT NULL`,
> > -      { valueState: BookmarkMergeState.TYPE.REMOTE });
> > -
> >      let queryRows = await this.db.execute(`
> 
> Since we ignore other params when we migrate, I wonder if we could do the
> same here...remove `rewriteRemoteTagQueries` entirely, and have
> https://searchfox.org/mozilla-central/rev/
> a0665934fa05158a5a943d4c8b277465910c029c/toolkit/components/places/
> SyncedBookmarksMirror.jsm#1260-1270 use the same approach as the migration
> when inserting into `moz_places`. Maybe it's not worth the trouble, though;
> these seem like they're uncommon enough already.

Yep, I preferred retaining the code closer to the original because, as I said, I feel a little bit out of my water in these modules, and Sync is scary. Nothing prevents from doing further cleanups in follow-ups, I won't complain.
What do you think? Is this an issue that I should fix here and now?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Blocks: 1451537
(In reply to Marco Bonardo [::mak] from comment #39)
> What do you think? Is this an issue that I should fix here and now?

Nope, a follow-up is great. Filed bug 1451537.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 46

a year ago
mozreview-review
Comment on attachment 8963977 [details]
Bug 1293445 - 2 - Migrate RESULTS_AS_TAG_CONTENTS queries to place:tag queries.

https://reviewboard.mozilla.org/r/232794/#review239644
Attachment #8963977 - Flags: review?(standard8) → review+

Comment 47

a year ago
mozreview-review
Comment on attachment 8964931 [details]
Bug 1293445 - 4 - Remove RESULTS_AS_TAG_CONTENTS.

https://reviewboard.mozilla.org/r/233670/#review239652
Attachment #8964931 - Flags: review?(standard8) → review+

Comment 48

a year ago
mozreview-review
Comment on attachment 8964932 [details]
Bug 1293445 - 5 - Rename RESULTS_AS_TAG_QUERY.

https://reviewboard.mozilla.org/r/233672/#review239654

Great, much nicer.
Attachment #8964932 - Flags: review?(standard8) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 54

a year ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/ce41871980ad
1 - Add a get_query_param SQL function. r=mak
https://hg.mozilla.org/integration/autoland/rev/bd1263777ab2
2 - Migrate RESULTS_AS_TAG_CONTENTS queries to place:tag queries. r=kitcambridge,standard8
https://hg.mozilla.org/integration/autoland/rev/2396069193d1
3 - Handle Sync tag queries rewrite. r=kitcambridge
https://hg.mozilla.org/integration/autoland/rev/35f57c8366d7
4 - Remove RESULTS_AS_TAG_CONTENTS. r=standard8
https://hg.mozilla.org/integration/autoland/rev/86be79de40ad
5 - Rename RESULTS_AS_TAG_QUERY. r=standard8
Assignee

Updated

a year ago
Blocks: 1452621
You need to log in before you can comment on or make changes to this bug.