Fix segmentation fault on base.QueryRequest request error

[cmorent]

This is a quick fix on the base.QueryBase function to avoid segmentation fault by checking if the resp is not nil before to check its status.

[ryankurte]

Hey, thanks for the PR!

The resp != nil check is definitely a good addition, but I'm not sure the logic is totally right here (probably because I definitely had it wrong before).

I think the cases should be:

• err != nil && resp == nil -> return err
• err != nil && resp != nil && resp.StatusCode != http.BadRequest -> return err
• err != nil && resp != nil && resp.StatusCode == http.BadRequest -> decode and return API error

And the first case was / is missing, would you be able to split it into a couple of conditions to cover all the cases? (If not, I can revise it).

Cheers,

Ryan

[cmorent]

Hey @ryankurte

Tell me if I'm wrong but in the QueryRequest function, if you return an error, the response is always nil, isn't it? If I'm right, the first case, err != nil && resp == nil will always be true and so we might do two different checks, one on the err and a second one on the resp.StatusCode? This way we would have something like:

if err != nil {
  return err
}

switch resp.StatusCode {
  case http.BadRequest:
    // decode and return API error
}

What do you think?

Cheers,

Cédric

[ryankurte]

I think you are correct, not sure why I was convinced otherwise.

From the net/http Client.Do godocs: An error is returned if the Client's CheckRedirect function fails or if there was an HTTP protocol error. A non-2xx response doesn't cause an error.

So the approach you proposed looks great.

[cmorent]

Ok! Maybe we could implement this logic around here https://github.com/ryankurte/go-mapbox/blob/master/lib/base/base.go#L91? Doing so the caller will just have to return err that will be properly formatted?