1557551 - Disqualify invalid items in the bookmarks mirror from deduping

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P3)

Description

[Lina Butler [:lina]]

fetch_new_remote_contents currently considers all new items, even ones that aren't valid. Including them isn't very useful—they don't have valid URLs, so we can't do content matching!

Comment 1

[Lina Butler [:lina]]

fetch_new_remote_contents isn't a thing anymore, but we can still add an item.validity != Validity::Valid condition to the ifs here. This is a great first Rust bug!

Comment 2

[Tanner Davies]

Could I take this bug, please?

Comment 3

[Tanner Davies]

Nevermind. I completed a different first-bug. This one should be left to someone else to learn the workflow.

Comment 4

[rushab.kumar]

(In reply to :Lina Cambridge from comment #1)

fetch_new_remote_contents isn't a thing anymore, but we can still add an item.validity != Validity::Valid condition to the ifs here. This is a great first Rust bug!

First timer here. Is this still a valid bug? The function signature has changed to fn local_row_to_item(&self, row: &Row<'_>) -> Result<Item> and no longer returns a Item & Content tuple.

Comment 5

[huzefa.jambughoda]

Need a first one. Is this available?

Comment 6

[Mark Hammond [:markh] [:mhammond]]

(In reply to rushab.kumar from comment #4)

(In reply to :Lina Cambridge from comment #1)

fetch_new_remote_contents isn't a thing anymore, but we can still add an item.validity != Validity::Valid condition to the ifs here. This is a great first Rust bug!

First timer here. Is this still a valid bug? The function signature has changed to fn local_row_to_item(&self, row: &Row<'_>) -> Result<Item> and no longer returns a Item & Content tuple.

Sorry for the delay - I think this is still a valid bug but that function does still take the tuple, so what Lina described still sounds OK. Ideally this would include a test, but it's not immediately clear yet how we would do that - let's just start with the described change and see what Lina thinks.

(I'm giving rushab.kumar the first option here because, sadly, we ignored them for a couple of months. If they don't get back in a week or so, I'll offer it to huzefa.jambughoda. Regardless, we don't actually assign a bug until a patch is first uploaded...)

Comment 7

[Jayati Shrivastava]

If nobody is working on this bug, may i submit a patch for it?

Comment 8

[Mark Hammond [:markh] [:mhammond]]

(In reply to Jayati Shrivastava from comment #7)

If nobody is working on this bug, may i submit a patch for it?

Please do!

Comment 9

[Lina Butler [:lina]]

I'm so sorry for the delay replying, this bug completely slipped off my radar! 😭

The two ifs we'll need to change are these. We'll want to add a condition to check if the validity is Validity::Replace, and, if it is, return None for the content.

We'll also want to add a test, to this file.

The way we'd test this is to add bookmarks locally and remotely, then change them to be invalid. For local bookmarks, we can do it like this, changing the URL to something invalid directly in moz_places. For remote bookmarks in the mirror, we can insert a valid bookmark, bookmarkBBB1 locally and bookmarkBBB2 remotely, then use this query:

await buf.db.execute(
  `UPDATE items SET
     validity = :validity
   WHERE guid = :guid`,
  { validity: Ci.mozISyncedBookmarksMerger.VALIDITY_REPLACE,
    guid: "bookmarkBBB2" });

...To flag B2 as invalid—so it should not dedupe to B1.

We need to make those changes after inserting local (via PlacesUtils.bookmarks.insertTree) and remote bookmarks (via storeRecords), but before we call await buf.apply(). Please have a look at some of the other tests in that file (you can copy-paste this one as a starting point), and let us know if you'd like help! We can always land the test in a follow-up bug 😊

Thanks so much for taking this on!

Comment 10

[Jayati Shrivastava]

Attachment: Bug 1557551 - Disqualify invalid items in the bookmarks mirror from deduping. r=Lina

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e5474686074d
Disqualify invalid items in the bookmarks mirror from deduping. r=lina

Comment 12

[Stefan Hindli [:stefan_hindli]]

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