Closed Bug 969055 Opened 6 years ago Closed 6 years ago

Validate items beings saved with HomeProvider API

Categories

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

All
Android
defect

Tracking

()

RESOLVED FIXED
Firefox 30

People

(Reporter: lucasr, Assigned: lucasr)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
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?
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.?
(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.
Duplicate of this bug: 974637
Assignee: nobody → margaret.leibovic
Priority: -- → P1
Assignee: margaret.leibovic → lucasr.at.mozilla
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.
Attachment #8389151 - Flags: review?(margaret.leibovic)
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.
Attachment #8389190 - Flags: review?(margaret.leibovic)
Attachment #8389151 - Attachment is obsolete: true
Attachment #8389151 - Flags: review?(margaret.leibovic)
Attachment #8389190 - Attachment is obsolete: true
Attachment #8389190 - Flags: review?(margaret.leibovic)
Comment on attachment 8389206 [details] [diff] [review]
Validate items being saved with HomeProvider API (r=margaret)

Properly encapsulate validateItem() function. Sorry for the noise.
Attachment #8389206 - Flags: review?(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.
Attachment #8389206 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/69fb6e09e4c6
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 30
Setting P1 hub bugs to block hub v1 EPIC bug (targeting fx30 release).

Filter on epic-hub-bugs.
Blocks: 1014025
You need to log in before you can comment on or make changes to this bug.