1384771 - bufferParentidMatchesStructure consumer is too optimistic about parentid

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, enhancement)

Description

[Richard Newman [:rnewman]]

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.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Band-aid.

Comment 2

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8890619 [details] [review]
Band-aid.

Looks good, let's ship it.

Comment 3

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Landed on master, uplifted to v8.x.

Comment 6

[alex_mayorga]

¡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

Comment 7

[Richard Newman [:rnewman]]

It should have shipped as part of 8.1, which is already in the App Store as of a few days ago.