1069808 - Make the LazyLoader.getJSON() helper invoke reject() in case of errors instead of resolving the promise to null

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Gabriele Svelto [:gsvelto]]

+++ This bug was initially created as a clone of Bug #1062806 +++

While investigating replacing existing code with LazyLoader.getJSON() we came across cases where the original code was checking for the response status rather than the returned object. In this cases we currently resolve() the promise to a null value which is not very pretty.

To improve upon this behavior we should change the helper to reject() whenever the status is not 0 or 200 or if the returned object is null (we might check only the latter because IIRC all erroneous HTTP states will yield a null response). This way resolve() will always return a valid JSON object. This is more consistent with the idea of a promise returning successfully a valid value when the resolve() handler is called.

Once this is done existing useless null checks should also be removed.

Comment 1

[Mathieu Rhéaume]

Attachment: Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

Hi Gabriele,

I just patched the lazy loader to implement the workaround we discussed earlier in bug 1062806.

I used the error object when we don't get the 200 or 0 status.

Thanks for taking a look into it !

Comment 2

[Mathieu Rhéaume]

Hi,

I just ran Gaia-try because it didn't picked it up in the first commit of the PR.

Everything is green.

https://tbpl.mozilla.org/?rev=d40566685baf84555f098d6533d7848ada7ca71b&tree=Gaia-Try

Comment 3

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8492650 [details] [review]
Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

Some comments on the PR but in a nutshell it's better to check for the response field rather than the status since the response field never lies. Additionally you'll have to add a unit test loading an existing but malformed JSON file as well as a non existing JSON file and ensure that we call reject() in both cases instead of resolve().

Comment 4

[Mathieu Rhéaume]

Comment on attachment 8492650 [details] [review]
Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

I added the following things to PR.

1. Modified the check status instead checking for response null.
2. Added unit test for : Malformed/Invalid JSON and non existing json file.

Thanks for looking into it.

Comment 5

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8492650 [details] [review]
Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

Looking good, thanks.

Comment 6

[Mathieu Rhéaume]

Comment on attachment 8492650 [details] [review]
Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

Added a Shared Gaia owner for review.

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8492650 [details] [review]
Github PR for Bug 1069808 checking if status 0 and 200 then resolve else reject.

r+ since gsvelto have already f+'d the patch :)

Comment 8

[Julien Wajsberg [:julienw]]

Mathieu, if it's ready to land, please add "checkin-needed" in the keywords section on the bug :)

Comment 9

[Salvador de la Puente González [:salva]]

Please notice my comment on GitHub about the null value [1] before merging.

[1] https://github.com/mozilla-b2g/gaia/pull/24243#discussion-diff-17957392

Comment 10

[Gabriele Svelto [:gsvelto]]

(In reply to Salvador de la Puente González [:salva] from comment #9)
> Please notice my comment on GitHub about the null value [1] before merging.
> 
> [1] https://github.com/mozilla-b2g/gaia/pull/24243#discussion-diff-17957392

You have a point, I didn't know that. However from a practical POV null needs to be special-cased in code whereas an empty object usually doesn't so I'd rather reject() all null values to simplify calling code. IMHO it's better this way.

Comment 11

[Mathieu Rhéaume]

Alright then, merging it.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/737738b7d50ea3f39ff6b43036b25e488bed2ae2