Keep both local and remote nodes with mismatched kinds #26
Conversation
This returns an iterator that yields the merged node's descendants, in level order.
Before `Unchanged`, `Local` meant "keep local and maybe upload", and
`LocalWithNewStructure` meant "keep local and always upload". Now
that `Unchanged` exists, `Local` means "keep local and upload", so
`LocalWithNewStructure` is redundant.
Further, it wasn't obvious that `Local { remote_node: None }` meant
the item only exists locally, and should be uploaded unconditionally,
and `Remote { local_node: None }` meant the item only exists remotely,
and should be applied unconditionally. This commit changes `Local`
and `Remote` to mean "exists on both sides, but prefer one", and adds
`LocalOnly`, `RemoteOnly`, and `RemoteOnlyWithNewStructure` to mean
"exists on one side, so take unconditionally".
To that end, this commit treats remotely changed user content roots as
`Unchanged`. As of bug 1432614, roots don't have user-editable titles,
so applying remote title changes is unnecessary. However, we also don't
mark the roots as `Local`, because that would mean we'd reupload the
root _every time_ it was changed remotely.
Finally, we want callers to easily determine whether to apply remote
changes, and upload local and new changes. This commit adds
`MergeState::{should_apply, should_upload}()` for that.
This ports the merger parts of https://bugzilla.mozilla.org/show_bug.cgi?id=1515784.
For mismatched kinds, we keep both nodes with new GUIDs. This is an edge case: Places maintenance on Desktop can change an item's kind without changing its GUID, but this should be rare. In general, we can't meaningfully pick a side when two items have different kinds (a bookmark and a folder with children? a bookmark and a separator?), so we keep both, with new structure, to avoid possible data loss. See #13.
Codecov Report
@@ Coverage Diff @@
## desktop #26 +/- ##
===========================================
- Coverage 95.15% 85.43% -9.73%
===========================================
Files 7 6 -1
Lines 2995 1977 -1018
===========================================
- Hits 2850 1689 -1161
- Misses 145 288 +143
Continue to review full report at Codecov.
|
| } else { | ||
| // For mismatched kinds, we keep both nodes with new GUIDs. This is | ||
| // an edge case: Places maintenance on Desktop can change an item's | ||
| // kind without changing its GUID (bug XXXXXXX), but this should be |
There was a problem hiding this comment.
Here's where we change bookmarks without URLs to folders, and anything with a URLs to bookmarks. I should file a bug so that we also update the GUID when we do that.
There was a problem hiding this comment.
Yeah, probably. As a note, I'm somewhat unconvinced that keeping both items is really the right call here. Yes, it avoids data loss, but these are items that are fairly corrupted to begin with.
That said, I'm not sure how that would actually work out, so we should probably just defer to your judgement here :)
|
Thinking some more, I wonder if we can drop this PR for now, have mismatched kinds return errors, and see what telemetry tells us. It's a chunk of complexity (in the old engine, too, where we remove and reinsert) for something that shouldn't really happen. The only mismatches we know about are livemark-folder and bookmark-query. The former shouldn't 🤞 be a problem now that we don't support livemarks, and the latter are totally fine. We can revisit if it turns out to be an issue in the wild. |
| } else { | ||
| // For mismatched kinds, we keep both nodes with new GUIDs. This is | ||
| // an edge case: Places maintenance on Desktop can change an item's | ||
| // kind without changing its GUID (bug XXXXXXX), but this should be |
There was a problem hiding this comment.
Yeah, probably. As a note, I'm somewhat unconvinced that keeping both items is really the right call here. Yes, it avoids data loss, but these are items that are fairly corrupted to begin with.
That said, I'm not sure how that would actually work out, so we should probably just defer to your judgement here :)
|
Oh, I missed your comment about this not being necessary. If it's not, then yeah I definitely am all for sitting on it and waiting for telemetry! |
|
Let's hold off on this and see what telemetry tells us. We can always dust it off later. |
For mismatched kinds, we keep both nodes with new GUIDs. This is an
edge case: Places maintenance on Desktop can change an item's
kind without changing its GUID, but this should be rare.
In general, we can't meaningfully pick a side when two items have
different kinds (a bookmark and a folder with children? a bookmark and
a separator?), so we keep both, with new structure, to avoid possible
data loss.
See #13.