1294291 - Enforce that all places items must have non-null GUIDs

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

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

Places goes to great lengths to ensure all items have GUIDs, and has a "unique" index on the column. It has DEBUG-only code in Database::Shutdown() that asserts if it finds any NULL items, but best I can tell, this isn't actually enforced.

Doing this would allow Sync to remove some complexity around ensuring items have GUIDs when best I can tell, that's already true, just not enforced.

ISTM this might be as easy as changing the GUID column to be "NOT NULL"?

Mak, is there something I'm missing here?

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Mark Hammond [:markh] from comment #0)
> ISTM this might be as easy as changing the GUID column to be "NOT NULL"?

The problem is that we can't do that, since Sqlite doesn't allow to alter a column to add not null, and you can't add a new not null column with a null value. Even if we had specified a default value and NOT NULL when we added the column, we could still end up with bogus entries having the default value, as today we can end up with NULL.

That said, I think today the only way to end up with a NULL guid is if a user/add-on runs a custom query on the database and doesn't set it, that while is bad, I'm not sure we should care about it, since he can cause far worse issues that way. The debug check is there just as an additional check so we can't break db creation.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

I sort of suspect the Sync guids management comes from when guids were annotations, and then it was far more likely for them to be missing.

Comment 4

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

(In reply to Marco Bonardo [::mak] from comment #3)
> I sort of suspect the Sync guids management comes from when guids were
> annotations, and then it was far more likely for them to be missing.

Yes - and thanks for the DB schema explanation. It sounds like we should (generally) be free to throw in that case - we could easily do more damage by ignoring that state, adding a GUID, then carrying on like nothing happened.

Comment 5

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

ack - to clarify - we should change the scope of this bug to be that PlacesSyncUtils should throw when it sees an item without a GUID. The other option is a simple "wontfix". Kit/Mak, what do you think?

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

yeah, I think "discarding" guid-missing entries should be fine. Whatever discarding means for Sync.

Comment 7

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

Kit, we have a comment "This method can be removed and replaced with `PlacesUtils.promiseItemGuid` once bug 1294291 lands" and I think it's reasonable to just make that change and allow any exception caused by a null GUID to percolate up the stack - bug 1297941 notwithstanding obviously ;) WDYT?

Comment 8

[Lina Butler [:lina]]

SGTM!

Comment 9

[Lina Butler [:lina]]

Attachment: Bug 1294291 - Remove missing GUID handling code from Sync and Places.

Review commit: https://reviewboard.mozilla.org/r/74218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74218/

Comment 10

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

Comment on attachment 8784903 [details]
Bug 1294291 - Remove missing GUID handling code from Sync and Places.

https://reviewboard.mozilla.org/r/74218/#review72264

Thanks - less is more! :)

Comment 11

[Lina Butler [:lina]]

Comment on attachment 8784903 [details]
Bug 1294291 - Remove missing GUID handling code from Sync and Places.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74218/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d090231db437
Remove missing GUID handling code from Sync and Places. r=markh

Comment 13

[Carsten Book [:Tomcat]]

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