969055 - Validate items beings saved with HomeProvider API

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect, P1)

Description

[Lucas Rocha (:lucasr)]

Right now we're just directly using the passed values in the query which is a bit too hand-wavy. We should at least log warnings if there's something unexpected in the passed list of items to be saved.

Comment 1

[:Margaret Leibovic]

What types of things should we validate? Some ideas:

* Length of title/description strings (avoid trying to insert super-long strings)
* URL-ness of url and image_url

Anything else?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Less performant, but it might help developers if we validated the data attributes and dumped any ones we can't handle into the console output (e.g. helpful for "'imageURL' is an invalid attribute" as opposed to the attribute "image_url"). This could be problematic if we ever update the schema, however, for addon developers use a spray-and-pray approach.

Additionally, if a given object does not have a particular key, we're throwing 'undefined' into the database - what happens?

Similarly, what if the values are null, empty arrays, objects, etc.?

Comment 3

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #2)

> Additionally, if a given object does not have a particular key, we're
> throwing 'undefined' into the database - what happens?
> 
> Similarly, what if the values are null, empty arrays, objects, etc.?

I think they'd all get converted to strings, so the developer would see some odd stuff show up like "undefined" or "[Object object]" for a title.

If we want to support null values (e.g. no description for an item) we could update the save logic to actually save `null` (or whatever SQLite expects), and then make sure the UI widgets can handle null values for those columns. Or perhaps we should just insert an empty string in the case of null/undefined values.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Validate items being saved with HomeProvider API (r=margaret)

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8389151 [details] [diff] [review]
Validate items being saved with HomeProvider API (r=margaret)

Here's a first take on this. This is kinda needed to test the transaction bits from bug 963817.

The validations are:
- Every row should have a URL
- Every row should have at least an image URL or a title or a description.

Comment 7

[Lucas Rocha (:lucasr)]

Attachment: Validate items being saved with HomeProvider API (r=margaret)

Comment 8

[Lucas Rocha (:lucasr)]

Comment on attachment 8389190 [details] [diff] [review]
Validate items being saved with HomeProvider API (r=margaret)

Here's a more complete patch with a validation test.

Comment 9

[Lucas Rocha (:lucasr)]

Attachment: Validate items being saved with HomeProvider API (r=margaret)

Comment 10

[Lucas Rocha (:lucasr)]

Comment on attachment 8389206 [details] [diff] [review]
Validate items being saved with HomeProvider API (r=margaret)

Properly encapsulate validateItem() function. Sorry for the noise.

Comment 11

[:Margaret Leibovic]

Comment on attachment 8389206 [details] [diff] [review]
Validate items being saved with HomeProvider API (r=margaret)

Review of attachment 8389206 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

Comment 12

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/integration/fx-team/rev/69fb6e09e4c6

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/69fb6e09e4c6

Comment 14

[:Margaret Leibovic]

Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.