Closed Bug 1384771 Opened 2 years ago Closed 2 years ago

bufferParentidMatchesStructure consumer is too optimistic about parentid

Categories

(Firefox for iOS :: Sync, enhancement)

All
iOS
enhancement
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fxios 8.1+ ---

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file)

55 bytes, text/x-github-pull-request
st3fan
: review+
Details | Review
Our fourth validation step looks like this:

private let bufferParentidMatchesStructure = [
"SELECT b.guid, b.parentid, s.parent, s.child, s.idx FROM",
TableBookmarksBuffer, "b JOIN", TableBookmarksBufferStructure,
"s ON b.guid = s.child WHERE b.parentid IS NOT s.parent",
].joined(separator: " ")

which is a query like this:

SELECT b.guid, b.parentid, s.parent, s.child, s.idx FROM
buffer b JOIN structure s
ON b.guid = s.child WHERE b.parentid IS NOT s.parent;


Given a record that's marked as deleted -- null parentid -- but a non-updated parent record, this will yield NULL for the parentid.

This is OK. However, we then go on to try to turn the validation results into strings so we can report them:

    private func getConcernedIDs(colNames: [String]) -> ((SDRow) -> [String]) {
        return { (row: SDRow) in
             colNames.map({ row[$0] as! String})
        }
    }

Two potential fixes here:

- Drop null IDs -- use `flatMap` and `as? String`. We should do this to fix the crash.

- Split this validation in two: one query that looks for rows in the buffer that are deleted, but still mentioned in structure; another that looks for non-deleted buffer rows that disagree. We should do this if we want to spot dropped deletions.
Attached file Band-aid.
Attachment #8890619 - Flags: review?
Comment on attachment 8890619 [details] [review]
Band-aid.

Looks good, let's ship it.
Attachment #8890619 - Flags: review? → review+
Landed on master, uplifted to v8.x.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Assignee: nobody → rnewman
Duplicate of this bug: 1385195
Duplicate of this bug: 1383629
¡Hola Richard!

Ended up here after asking on #sync while trying to help a user with a question in the SuMo forum at https://support.mozilla.org/es/questions/1168700

When would this reach end users?

¡Gracias!
Alex
Flags: needinfo?(rnewman)
It should have shipped as part of 8.1, which is already in the App Store as of a few days ago.
Flags: needinfo?(rnewman)
You need to log in before you can comment on or make changes to this bug.